Re: [PATCH] powerpc/chrp: Fix enter_rtas() with CONFIG_VMAP_STACK
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 +, 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 > > > --- > > > 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 */ > > > addir1,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 >
Re: [PATCH] powerpc/chrp: Fix enter_rtas() with CONFIG_VMAP_STACK
Le 16/02/2020 à 23:40, Michael Neuling a écrit : On Fri, 2020-02-14 at 08:33 +, 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. 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. Signed-off-by: Christophe Leroy --- 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_STACK + 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 ? + mtmsr r0 + isync +#endif lwz r8,INT_FRAME_SIZE+4(r9) /* get return address */ lwz r9,8(r9)/* original msr value */ addir1,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
Re: [PATCH] powerpc/chrp: Fix enter_rtas() with CONFIG_VMAP_STACK
On Fri, 2020-02-14 at 08:33 +, 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? Also can you say what you're actually doing (ie turning on MSR[DR]) > Signed-off-by: Christophe Leroy > --- > 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_STACK > + 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. > + mtmsr r0 > + isync > +#endif > lwz r8,INT_FRAME_SIZE+4(r9) /* get return address */ > lwz r9,8(r9)/* original msr value */ > addir1,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
[PATCH] powerpc/chrp: Fix enter_rtas() with CONFIG_VMAP_STACK
With CONFIG_VMAP_STACK, data MMU has to be enabled to read data on the stack. Signed-off-by: Christophe Leroy --- 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_STACK + li r0, MSR_KERNEL & ~MSR_IR/* can take DTLB miss */ + mtmsr r0 + isync +#endif lwz r8,INT_FRAME_SIZE+4(r9) /* get return address */ lwz r9,8(r9)/* original msr value */ addir1,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 -- 2.25.0
