Re: [PATCH V3 2/2] powerpc/kexec: Reset HILE before entering target kernel

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

On Fri, Jul 17, 2015 at 11:53:38AM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2015-07-10 at 15:19 +1000, Samuel Mendoza-Jonas wrote:
> > +#if defined(CONFIG_PPC_BOOK3S_64) && defined(CONFIG_PPC_POWERNV)
> > +       li      r3,(FW_FEATURE_OPAL >> 16)
> > +       rldicr  r3,r3,16,63
> > +       and.    r3,r3,r26
> > +       cmpwi   r3,0
> > +       beq     99f
> 
> If FW_FEATRURE_OPAL is 0x80000000 then the li will sign extend.
> 
> The rldicr has a mask of all F's so it will keep all the bits you
> don't care about.
> 
> So together, you'll get compares happening on bits above the 16 you care
> about that might change the result of your comparison incorrectly.
> 
> Since FW_FEATURE_* bits aren't ABI, they can change, so we don't want
> to impose a constraint on them.
> 
> Thus I would recommend using an rdlicl r3,r3,16,48 (aka srdi r3,r3,48)
> instead which is going to clear all bits above 0xffff.

Or the other way around: instead of loading and masking, load zero and
OR the bits in:

+       li      r3,0
+       ori     r3,(FW_FEATURE_OPAL >> 16)
+       rldicr  r3,r3,16,63
+       and.    r3,r3,r26
+       cmpwi   r3,0
+       beq     99f

which of course is better written as

+       li      r3,0
+       oris    r3,(FW_FEATURE_OPAL >> 16)
+       and.    r3,r3,r26
+       cmpwi   r3,0
+       beq     99f

which is

+       andis.  r3,r26,(FW_FEATURE_OPAL >> 16)
+       cmpwi   r3,0
+       beq     99f

which is

+       andis.  r3,r26,(FW_FEATURE_OPAL >> 16)
+       beq     99f

> Now, that being said, FW_FEATURE_* can be 64-bit

All of the code above only works for single-bit constants inside
0xffff0000 though (or 0x7fff0000 for the original).

> and this isn't perf
> critical so why not just load the full 64-bit constant into r3 and
> be done with it ? There's a macro to do that:
> 
> 	LOAD_REG_IMMEDIATE(r3,FW_FEATURE_OPAL)

And as you say later, use C.  Yeah.


Segher