Re: [PATCH v2 1/2] powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state

[Segher Boessenkool <segher@kernel.crashing.org>]

On Thu, Sep 02, 2021 at 01:33:10PM +1000, Nicholas Piggin wrote:
> Excerpts from Christophe Leroy's message of September 2, 2021 3:21 am:
> >> -	/* Firstly we need to enable TM in the kernel */
> >> +	/* We need to enable TM in the kernel, and disable EE (for scv) */
> >>   	mfmsr	r10
> >>   	li	r9, 1
> >>   	rldimi	r10, r9, MSR_TM_LG, 63-MSR_TM_LG
> >> +	LOAD_REG_IMMEDIATE(r9, MSR_EE)
> >> +	andc	r10, r10, r9
> > 
> > Why not use 'rlwinm' to mask out MSR_EE ?
> > 
> > Something like
> > 
> > 	rlwinm	r10, r10, 0, ~MSR_EE
> 
> Mainly because I'm bad at powerpc assembly. Why do you think I'm trying 
> to change as much as possible to C?

The actual bit (bit 31, i.e. with value 1UL << 32) cannot be cleared
with rlwinm (only the low 32 bits can).  There are many ways to do it
using two insns of course.

> Actually there should really be no need for mfmsr either, I wanted to
> rewrite the thing entirely as
> 
> 	ld      r10,PACAKMSR(r13)
> 	LOAD_REG_IMMEDIATE(r9, MSR_TM)
> 	or	r10,r10,r9
> 	mtmsrd	r10
> 
> But I thought that's not a minimal bug fix.

That (LOAD_REG_IMMEDIATE+or) can be done with just two insns, not three
as written:
  li r0,1
  rldimi r10,r0,32,31
(you can write that last insns as
  rldimi r10,r0,MSR_TM_LG,MSR_TM_LG-1
if you insist :-) )

It isn't like a few integer computational insns will kill you here of
course, there are much more important cycles to shave off :-)


Segher