Re: [RFC PATCH] powerpc/6xx: Don't set back MSR_RI before reenabling MMU

[Christophe Leroy]

Le 01/02/2019 Ã 12:51, Christophe Leroy a ÃcritÂ:
> Le 01/02/2019 Ã 12:10, Michael Ellerman a ÃcritÂ:
> > Christophe Leroy <christophe.leroy@xxxxxx> writes:
> >
> > > By delaying the setting of MSR_RI, a 1% improvment is optained on
> > > null_syscall selftest on an mpc8321.
> > >
> > > Without this patch:
> > >
> > > root@vgoippro:~# ./null_syscall
> > > ÂÂÂ 1134.33 nsÂÂÂÂ 378.11 cycles
> > >
> > > With this patch:
> > >
> > > root@vgoippro:~# ./null_syscall
> > > ÂÂÂ 1121.85 nsÂÂÂÂ 373.95 cycles
> > >
> > > The drawback is that a machine check during that period
> > > would be unrecoverable, but as only main memory is accessed
> > > during that period, it shouldn't be a concern.
> >
> > On 64-bit server CPUs accessing main memory can cause a UE
> > (Uncorrectable Error) which can trigger a machine check.
> >
> > So it may still be a concern, it depends how paranoid you are.
> >
> > > diff --git a/arch/powerpc/kernel/head_32.S 
> > > b/arch/powerpc/kernel/head_32.S
> > > index 146385b1c2da..ea28a6ab56ec 100644
> > > --- a/arch/powerpc/kernel/head_32.S
> > > +++ b/arch/powerpc/kernel/head_32.S
> > > @@ -282,8 +282,6 @@ __secondary_hold_acknowledge:
> > > ÂÂÂÂÂ stwÂÂÂ r1,GPR1(r11);ÂÂÂ \
> > > ÂÂÂÂÂ stwÂÂÂ r1,0(r11);ÂÂÂ \
> > > ÂÂÂÂÂ tovirt(r1,r11);ÂÂÂÂÂÂÂÂÂÂÂ /* set new kernel sp */ÂÂÂ \
> > > -ÂÂÂ liÂÂÂ r10,MSR_KERNEL & ~(MSR_IR|MSR_DR); /* can take exceptions 
> > > */ \
> > > -ÂÂÂ MTMSRD(r10);ÂÂÂÂÂÂÂÂÂÂÂ /* (except for mach check in rtas) */ \
> > > ÂÂÂÂÂ stwÂÂÂ r0,GPR0(r11);ÂÂÂ \
> > > ÂÂÂÂÂ lisÂÂÂ r10,STACK_FRAME_REGS_MARKER@ha; /* exception frame 
> > > marker */ \
> > > ÂÂÂÂÂ addiÂÂÂ r10,r10,STACK_FRAME_REGS_MARKER@l; \
> >
> > Where does RI get enabled? I don't see it anywhere obvious.
>
> MSR_RI is part of MSR_KERNEL, it gets then enabled when reenabling MMU 
> when calling the exception handler.
>
> #define EXC_XFER_TEMPLATE(n, hdlr, trap, copyee, tfer, ret)ÂÂÂ \
>  ÂÂÂÂliÂÂÂ r10,trap;ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
>  ÂÂÂÂstwÂÂÂ r10,_TRAP(r11);ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
>  ÂÂÂÂliÂÂÂ r10,MSR_KERNEL;ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
>  ÂÂÂÂcopyee(r10, r9);ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
>  ÂÂÂÂblÂÂÂ tfer;ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
> i##n:ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
>  ÂÂÂÂ.longÂÂÂ hdlr;ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
>  ÂÂÂÂ.longÂÂÂ ret
>
> where tfer = transfer_to_handler.
>
> In transfer_to_handler (kernel/entry_32.S) you have:
>
> transfer_to_handler_cont:
> 3:
>  ÂÂÂÂmflrÂÂÂ r9
>  ÂÂÂÂlwzÂÂÂ r11,0(r9)ÂÂÂÂÂÂÂ /* virtual address of handler */
>  ÂÂÂÂlwzÂÂÂ r9,4(r9)ÂÂÂÂÂÂÂ /* where to go when done */
> [...]
>  ÂÂÂÂmtsprÂÂÂ SPRN_SRR0,r11
>  ÂÂÂÂmtsprÂÂÂ SPRN_SRR1,r10
>  ÂÂÂÂmtlrÂÂÂ r9
>  ÂÂÂÂSYNC
>  ÂÂÂÂRFIÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ /* jump to handler, enable MMU */
>
> So MSR_RI is restored above as r10 contains MSR_KERNEL [ | MSR_EE ]

Looks like fast_exception_return, which is called by hash page handlers 
at least, expects MSR_RI to be set. Allthough it works well on 603 
(because it doesn't hash), I would most likely not work on others.

This 1% improvment is not worth it, I give up for now.