Re: [PATCH] powerpc/chrp: Fix enter_rtas() with CONFIG_VMAP_STACK

[Michael Neuling]

On Mon, 2020-02-17 at 07:40 +0100, Christophe Leroy wrote:
> 
> Le 16/02/2020 Ã 23:40, Michael Neuling a Ãcrit :
> > On Fri, 2020-02-14 at 08:33 +0000, Christophe Leroy wrote:
> > > With CONFIG_VMAP_STACK, data MMU has to be enabled
> > > to read data on the stack.
> > 
> > Can you describe what goes wrong without this? Some oops message? rtas blows
> > up?
> > Get corrupt data?
> 
> Larry reported a machine check. Or in fact, he reported a Oops in 
> kprobe_handler(), that Oops being a bug in kprobe_handle() triggered by 
> this machine check.
> 
> By converting a VM address to a phys-like address as if is was linear 
> mem, you get in the dark. Either there is some physical memory at that 
> address and you corrupt it. Or there is none and you get a machine check.

Excellent. Please put that in the commit message.

> > Also can you say what you're actually doing (ie turning on MSR[DR])
> 
> Euh ... I'm saying that data MMU has to be enabled, so I'm enabling it.

Yeah, it's a minor point.

> > 
> > > Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxx>
> > > ---
> > >   arch/powerpc/kernel/entry_32.S | 9 +++++++--
> > >   1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/powerpc/kernel/entry_32.S
> > > b/arch/powerpc/kernel/entry_32.S
> > > index 0713daa651d9..bc056d906b51 100644
> > > --- a/arch/powerpc/kernel/entry_32.S
> > > +++ b/arch/powerpc/kernel/entry_32.S
> > > @@ -1354,12 +1354,17 @@ _GLOBAL(enter_rtas)
> > >   	mtspr	SPRN_SRR0,r8
> > >   	mtspr	SPRN_SRR1,r9
> > >   	RFI
> > > -1:	tophys(r9,r1)
> > > +1:	tophys_novmstack r9, r1
> > > +#ifdef CONFIG_VMAP_STACKisntruction
> > > +	li	r0, MSR_KERNEL & ~MSR_IR	/* can take DTLB miss */
> > 
> > You're potentially turning on more than MSR DR here. This should be clear in
> > the
> > commit message.
> 
> Am I ?
> 
> At the time of the RFI just above, SRR1 contains the value of r9 which 
> has been set 2 lines before to MSR_KERNEL & ~(MSR_IR|MSR_DR).
> 
> What should be clear in the commit message ?

You're right. I was just looking at the patch and not the code. It's clearer in
the code.

Mikey

> 
> > > +	mtmsr	r0
> > > +	isync
> > > +#endif
> > >   	lwz	r8,INT_FRAME_SIZE+4(r9)	/* get return address */
> > >   	lwz	r9,8(r9)	/* original msr value */
> > >   	addi	r1,r1,INT_FRAME_SIZE
> > >   	li	r0,0
> > > -	tophys(r7, r2)
> > > +	tophys_novmstack r7, r2
> > >   	stw	r0, THREAD + RTAS_SP(r7)
> > >   	mtspr	SPRN_SRR0,r8
> > >   	mtspr	SPRN_SRR1,r9
> 
> Christophe
>