Re: [PATCHv5] - powerpc: Add hwcap/hwcap2/platform data to TCB.

[Steven Munroe <munroesj at linux dot vnet dot ibm dot com>]

On Mon, 2015-10-26 at 12:35 -0500, Peter Bergner wrote:
> On Fri, 2015-10-23 at 14:15 -0200, Carlos Eduardo Seo wrote:
> > +void
> > +__tcb_parse_hwcap_and_convert_at_platform (void)
> > +{
> > +
> > +  uint64_t h1, h2;
> 
> Is there a reason h1 and h2 are 64-bit types here?  It seems when
> this is being used in a 32-bit binary, the "long long" types this
> gets mapped to are unnecessary and expensive given how they're
> used below.  You only need to cast them just before you merge
> them into __tcb_hwcap.
> 

because later we need to shift left 32-bits, which drop information if
we declared them 32-bit.
> 
> 
> +  if (h1 & PPC_FEATURE_ARCH_2_06)
> +      h1 |= PPC_FEATURE_ARCH_2_05 |	      \
> +	    PPC_FEATURE_POWER5_PLUS |	      \
> +	    PPC_FEATURE_POWER5 |	      \
> +	    PPC_FEATURE_POWER4;
> +    else if (h1 & PPC_FEATURE_ARCH_2_05)
> +      h1 |= PPC_FEATURE_POWER5_PLUS |	      \
> +	    PPC_FEATURE_POWER5 |	      \
> +	    PPC_FEATURE_POWER4;
> 
> This isn't a macro, so you don't need the '\' continuation characters.
> I don't know if glibc formatting is different than gcc, but normally
> when we push parts of an expression onto the following lines, we start
> the line with the operator, like so:
> 
>   if (h1 & PPC_FEATURE_ARCH_2_06)
>     h1 |= PPC_FEATURE_ARCH_2_05
> 	  | PPC_FEATURE_POWER5_PLUS
> 	  | PPC_FEATURE_POWER5
> 	  | PPC_FEATURE_POWER4;
>   else ...
> 
> 
> 
> 
> +	    PPC_FEATURE_POWER4;
> +    else if (h1 & PPC_FEATURE_POWER5)
> +      h1|= PPC_FEATURE_POWER4;
> 
>          ^ missing space.
> 
> 
> +  /* Consolidate both HWCAP and HWCAP2 into a single doubleword so that
> > +     we can read both in a single load later.  */
> > +  __tcb_hwcap = h2;
> > +  __tcb_hwcap = (h1 << 32) | __tcb_hwcap;
> 
> So keeping h1 and h2 as uint32_t for the above, we could use:
> 
>   __tcb_hwcap = ((uint64_t)h1 << 32) | (uint64_t) h2;
> 

Are you sure current an future versions of GCC will handle this
promotion to 64-bit correctly across the shift and or operation?

Carlos' code is safer.


> 
> However, do we really want to have a combined hwcap type here?
> The problem I see is that if we combine these into one 64-bit
> type, then which TCB offset the HWCAP and HWCAP2 values reside
> changes depending on BE versus LE.  I can work around this I
> guess, but it just seems like having a fixed TCB offset for the
> HWCAP value and HWCAP2 value regardless of endianness is a
> good thing.
> 
You can not have both. You either have single 64-bit load for BE and LE,
or you can have fixed (same for BE/LE) offset for the separate
HWCAP/HWCAP2.

The alternative requires spectate mask generation for BE/LE around any
single 64-bit load (think about any if where you need to test one bit
each in HWCAP and HWCAP2).

This is more consistent.

> Note that you can still do one store of both values (in 64-bit mode)
> even if we have seperate/fixed offset slots for the HWCAP and HWCAP2
> values in the TCB.
> 
> 
> Peter
> 
>