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

[Peter Bergner <bergner at vnet dot ibm dot com>]

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.



+  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;


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.

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