[Skiboot] [PATCH 1/1] Disable protected execution facility
Alistair Popple
alistair at popple.id.au
Fri Feb 21 11:53:25 AEDT 2020
Also I think Oliver wanted to know what would happen if Hostboot boots Skiboot
expecting an Ultravisor (ie. with secure memory setup and the xscom base
address in secure memory). I assume we would crash and burn in some rather
unfortunate way? At the very least we should probably print a dirty big
warning if we're supposed to be starting an Ultravisor but instead just
disable SMF and continue.
Also I guess this is unlikely but do we need to worry about ever being booted
with urmor being set?
- Alistair
On Tuesday, 18 February 2020 4:29:35 PM AEDT Alistair Popple wrote:
> We've run into this issue with Simics this works there as well so:
>
> Tested-by: Alistair Popple <alistair at popple.id.au>
>
> Couple of questions though:
> > +.global exit_uv_mode
> > +exit_uv_mode:
> > + mfmsr %r4
> > + LOAD_IMM64(%r5, ~MSR_S)
> > + and %r4,%r4,%r5
> > + mtspr SPR_USRR1,%r4
> > +
> > + mfspr %r4,SPR_HSRR1
> > + and %r4,%r4,%r5
> > + mtspr SPR_HSRR1,%r3
> > +
> > + mfspr %r4,SPR_SRR1
> > + and %r4,%r4,%r5
> > + mtspr SPR_SRR1,%r4
>
> Is there a reason we need to update [H]SRR1 as well? I doubt we'd be running
> this in the context of an exception and other uses of SRR1 tend to set it
> explicitly rather than relying on existing values, although I may be
> missing something.
>
> > + cmpdi %r3,1
> > + bne 1f
> > + mfspr %r4, SPR_SMFCTRL
> > + LOAD_IMM64(%r5, ~PPC_BIT(0))
> > + and %r4,%r4,%r5
> > + mtspr SPR_SMFCTRL,%r4
> > +1:
> > + isync
> > +
> > + mflr %r4
> > + mtspr SPR_USRR0,%r4
> > +
> > + urfid
> > diff --git a/core/cpu.c b/core/cpu.c
> > index d5b7d623..1adf16cc 100644
> > --- a/core/cpu.c
> > +++ b/core/cpu.c
> > @@ -1644,3 +1644,62 @@ static int64_t opal_nmmu_set_ptcr(uint64_t chip_id,
> > uint64_t ptcr) return rc;
> >
> > }
> > opal_call(OPAL_NMMU_SET_PTCR, opal_nmmu_set_ptcr, 2);
> >
> > +
> > +static void _exit_uv_mode(void *data __unused)
> > +{
> > + prlog(PR_DEBUG, "Exit uv mode on cpu pir 0x%04x\n", this_cpu()->pir);
> > + /* HW has smfctrl shared between threads but on Mambo it is per-thread
> > */
> > + if (chip_quirk(QUIRK_MAMBO_CALLOUTS))
> > + exit_uv_mode(1);
> > + else
> > + exit_uv_mode(cpu_is_thread0(this_cpu()));
> > +}
> > +
> > +void cpu_disable_pef(void)
> > +{
> > + struct cpu_thread *cpu;
> > + struct cpu_job **jobs;
> > +
> > + if (!(mfmsr() & MSR_S)) {
> > + prlog(PR_DEBUG, "UV mode off on cpu pir 0x%04x\n", this_cpu()-
>pir);
> > + return;
> > + }
> > +
> > + jobs = zalloc(sizeof(struct cpu_job *) * (cpu_max_pir + 1));
> > + assert(jobs);
> > +
> > + /* Exit uv mode on all secondary threads before touching
> > + * smfctrl on thread 0 */
>
> Do we need to separate things out this way though? It seems like it should
> have much the same affect to clear SMFCTRL and UV mode on every thread.
> Doing so might simplify the code a bit as you could just call
> exit_uv_mode() directly from main/secondary_cpu_init().
>
> > + for_each_available_cpu(cpu) {
> > + if (cpu == this_cpu())
> > + continue;
> > +
> > + if (!cpu_is_thread0(cpu))
> > + jobs[cpu->pir] = cpu_queue_job(cpu, "exit_uv_mode",
> > + _exit_uv_mode, NULL);
> > + }
> > +
> > + for_each_available_cpu(cpu)
> > + if (jobs[cpu->pir]) {
> > + cpu_wait_job(jobs[cpu->pir], true);
> > + jobs[cpu->pir] = NULL;
> > + }
> > +
> > + /* Exit uv mode and disable smfctrl on primary threads */
> > + for_each_available_cpu(cpu) {
>
> Bit of a nit-pick but you could use for_each_available_core_in_chip()
> instead.
>
> - Alistair
>
> > + if (cpu == this_cpu())
> > + continue;
> > +
> > + if (cpu_is_thread0(cpu))
> > + jobs[cpu->pir] = cpu_queue_job(cpu, "exit_uv_mode",
> > + _exit_uv_mode, NULL);
> > + }
> > +
> > + for_each_available_cpu(cpu)
> > + if (jobs[cpu->pir])
> > + cpu_wait_job(jobs[cpu->pir], true);
> > +
> > + free(jobs);
> > +
> > + _exit_uv_mode(NULL);
> > +}
> > diff --git a/core/init.c b/core/init.c
> > index 339462e5..0d993abb 100644
> > --- a/core/init.c
> > +++ b/core/init.c
> > @@ -1354,6 +1354,9 @@ void __noreturn __nomcount main_cpu_entry(const void
> > *fdt) /* Add the list of interrupts going to OPAL */
> >
> > add_opal_interrupts();
> >
> > + /* Disable protected execution facility in BML */
> > + cpu_disable_pef();
> > +
> >
> > /* Now release parts of memory nodes we haven't used ourselves... */
> > mem_region_release_unused();
> >
> > diff --git a/include/cpu.h b/include/cpu.h
> > index 686310d7..cab63360 100644
> > --- a/include/cpu.h
> > +++ b/include/cpu.h
> > @@ -309,4 +309,7 @@ int dctl_set_special_wakeup(struct cpu_thread *t);
> >
> > int dctl_clear_special_wakeup(struct cpu_thread *t);
> > int dctl_core_is_gated(struct cpu_thread *t);
> >
> > +extern void exit_uv_mode(int);
> > +void cpu_disable_pef(void);
> > +
> >
> > #endif /* __CPU_H */
> >
> > diff --git a/include/processor.h b/include/processor.h
> > index a0c2864a..1fdcc02b 100644
> > --- a/include/processor.h
> > +++ b/include/processor.h
> > @@ -11,6 +11,7 @@
> >
> > #define MSR_HV PPC_BIT(3) /* Hypervisor mode */
> > #define MSR_VEC PPC_BIT(38) /* VMX enable */
> > #define MSR_VSX PPC_BIT(40) /* VSX enable */
> >
> > +#define MSR_S PPC_BIT(41) /* Secure mode */
> >
> > #define MSR_EE PPC_BIT(48) /* External Int. Enable */
> > #define MSR_PR PPC_BIT(49) /* Problem state */
> > #define MSR_FP PPC_BIT(50) /* Floating Point Enable */
> >
> > @@ -65,6 +66,9 @@
> >
> > #define SPR_HMEER 0x151 /* HMER interrupt enable mask */
> > #define SPR_PCR 0x152
> > #define SPR_AMOR 0x15d
> >
> > +#define SPR_USRR0 0x1fa /* RW: Ultravisor Save/Restore Register 0 */
> > +#define SPR_USRR1 0x1fb /* RW: Ultravisor Save/Restore Register 1 */
> > +#define SPR_SMFCTRL 0x1ff /* RW: Secure Memory Facility Control */
> >
> > #define SPR_PSSCR 0x357 /* RW: Stop status and control (ISA 3) */
> > #define SPR_TSCR 0x399
> > #define SPR_HID0 0x3f0
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
More information about the Skiboot
mailing list