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

[Ryan Grimm]

On Tue, 2020-02-18 at 16:29 +1100, 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.
> 

This sequence is documented in the P9 user manual section 25.3.7
"Firmware code sequence for disabling SMF".  We have to do it in such
an explict way because of the hardware.  I'll add this info to the
patch.


> > +	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().

Well, the sequence doesn't say what you're describing won't work.  But,
firmware does it in smt1.  The sequence wasn't tested on smt2 and smt4.
I tested clearing the bit in smfctrl on every thread concurrently on
p9ndd2.3 with threads enabled and the result was a core logic checktop.

We could go to smt1 mode before we run it, but I'm going on gut
instinct to say that sounds like more of a pain.  And now trying to
think about it more, that doesn't even make sense for skiboot to do,
right? 

I tinkered with coding it all in assembly early on in asm/head.S and
abandoned that plan.

> 
> > +	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.
> 

The code would then require another local variable, chip, and would add
an extra line, how is that better?

-Ryan