[Skiboot] [PATCH 1/1] Disable protected execution facility

Alistair Popple alistair at popple.id.au
Tue Feb 18 16:29:35 AEDT 2020


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






More information about the Skiboot mailing list