Re: [PATCH 5/6] powerpc/mm/64s/hash: Add real-mode change_memory_range() for hash LPAR

[Nicholas Piggin <npiggin@gmail.com>]

Excerpts from Michael Ellerman's message of March 20, 2021 11:04 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
>> Excerpts from Michael Ellerman's message of February 11, 2021 11:51 pm:
> ...
>>> diff --git a/arch/powerpc/mm/book3s64/hash_pgtable.c b/arch/powerpc/mm/book3s64/hash_pgtable.c
>>> index 3663d3cdffac..01de985df2c4 100644
>>> --- a/arch/powerpc/mm/book3s64/hash_pgtable.c
>>> +++ b/arch/powerpc/mm/book3s64/hash_pgtable.c
>>> @@ -414,6 +428,73 @@ static void change_memory_range(unsigned long start, unsigned long end,
>>>  							mmu_kernel_ssize);
>>>  }
>>>  
>>> +static int notrace chmem_secondary_loop(struct change_memory_parms *parms)
>>> +{
>>> +	unsigned long msr, tmp, flags;
>>> +	int *p;
>>> +
>>> +	p = &parms->cpu_counter.counter;
>>> +
>>> +	local_irq_save(flags);
>>> +	__hard_EE_RI_disable();
>>> +
>>> +	asm volatile (
>>> +	// Switch to real mode and leave interrupts off
>>> +	"mfmsr	%[msr]			;"
>>> +	"li	%[tmp], %[MSR_IR_DR]	;"
>>> +	"andc	%[tmp], %[msr], %[tmp]	;"
>>> +	"mtmsrd %[tmp]			;"
>>> +
>>> +	// Tell the master we are in real mode
>>> +	"1:				"
>>> +	"lwarx	%[tmp], 0, %[p]		;"
>>> +	"addic	%[tmp], %[tmp], -1	;"
>>> +	"stwcx.	%[tmp], 0, %[p]		;"
>>> +	"bne-	1b			;"
>>> +
>>> +	// Spin until the counter goes to zero
>>> +	"2:				;"
>>> +	"lwz	%[tmp], 0(%[p])		;"
>>> +	"cmpwi	%[tmp], 0		;"
>>> +	"bne-	2b			;"
>>> +
>>> +	// Switch back to virtual mode
>>> +	"mtmsrd %[msr]			;"
>>> +
>>> +	: // outputs
>>> +	  [msr] "=&r" (msr), [tmp] "=&b" (tmp), "+m" (*p)
>>> +	: // inputs
>>> +	  [p] "b" (p), [MSR_IR_DR] "i" (MSR_IR | MSR_DR)
>>> +	: // clobbers
>>> +	  "cc", "xer"
>>> +	);
>>> +
>>> +	local_irq_restore(flags);
>>
>> Hmm. __hard_EE_RI_disable won't get restored by this because it doesn't
>> set the HARD_DIS flag. Also we don't want RI disabled here because 
>> tracing will get called first (which might take SLB or HPTE fault).
> 
> Thanks for noticing. I originally wrote hard_irq_disable() but then
> thought disabling RI also would be good.
> 
>> But it's also slightly rude to ever enable EE under an irq soft mask,
>> because you don't know if it had been disabled by the masked interrupt 
>> handler. It's not strictly a problem AFAIK because the interrupt would
>> just get masked again, but if we try to maintain a good pattern would
>> be good. Hmm that means we should add a check for irqs soft masked in
>> __hard_irq_enable(), I'm not sure if all existing users would follow
>> this rule.
>>
>> Might be better to call hard_irq_disable(); after the local_irq_save();
>> and then clear and reset RI inside that region (could just do it at the
>> same time as disabling MMU).
> 
> Thinking about it more, there's no real reason to disable RI.
> 
> We should be able to return from an interrupt in there, it's just that
> if we do take one we'll probably die before we get a chance to return
> because the mapping of text will be missing.

Yeah it probably will because the pseries hash machine check handler has 
some hacks in it that require turning the MMU on. We might never fix 
that if we're moving to radix, but if we did then in theory we'd be able 
to take a MCE here and recover.

> So disabling RI doesn't really gain us anything I don't think.

Yeah I probably agree. So local_irq_save(flags); hard_irq_disable(); 
should do the trick.

Thanks,
Nick