Re: [PATCH v3] PowerPC: Influence cpu/arch hwcap features via GLIBC_TUNABLES.

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 06/07/23 18:05, Peter Bergner via Libc-alpha wrote:
> On 7/6/23 7:25 AM, bmahi496@linux.ibm.com wrote:
>> +  /* Copy the features from dl_powerpc_cpu_features, which contains the
>> +     features provided by AT_HWCAP and AT_HWCAP2.  */
>> +  struct cpu_features *cpu_features = &GLRO(dl_powerpc_cpu_features);
>> +  unsigned long int tcbv_hwcap = cpu_features->hwcap;
>> +  unsigned long int tcbv_hwcap2 = cpu_features->hwcap2;
> [snip]
>> +	      else
>> +		{
>> +		  /* Enable the features and also checking that no unsupported
>> +		     features were enabled by user.  */
>> +		  if (hwcap_tunables[i].id)
>> +		    cpu_features->hwcap2 |= (tcbv_hwcap2 & hwcap_tunables[i].mask);
>> +		  else
>> +		    cpu_features->hwcap |= (tcbv_hwcap & hwcap_tunables[i].mask);
>> +		}
> 
> I don't see how this code can ever "add" new bits to cpu_features->hwcap
> or cpu_features->hwcap2.  We cache their values at the top of the loop
> and then OR in the tunable mask, but only if they're not already set
> in the cached values.  If the tunable mask has a bit that isn't already
> present in cpu_features->hwcap/cpu_features->hwcap2, we'll never set
> them.  It seems your code as is, can only ever remove bits from hwcap/hwcap2.
> 
> Question for you or anyone else, is there a scenario where we can execute
> our set_hwcaps tunable callback function where the set of HWCAP/2 feature
> bits is a subset of the HWCAP/2 bits that the kernel passed to us?
> 
> For example, could our set_hwcaps tunable callback function be called multiple
> times in some scenario like via a fork or ???  If yes, then tcbv_hwcap and
> tcbv_hwcap2 really should be set to the HWCAP/2 values the kernel gives us
> and not some possibly modified version in cpu_features->hwcap/2.  If no, then
> I think we probably don't even need to support trying to enable/add bits to
> the cpu_features->hwcap/2 masks at all, since we'd only try setting bits
> that are already set or bits we're not supposed to set since they aren't
> in the kernel's version of the hwcaps.

The set_hwcaps is called once only at process execution. Adding bits might
make sense in a scenario that you are running a new glibc on older kernels
and you know that some string routines are safe to run on that hardware
(although it might come with some caveats, since it might still require some
kernel support).

With this patch GLIBC_TUNABLES=glibc.cpu.hwcaps=feature is ignored if 'feature'
is not support by HWCAP/2, which is not explicit state in documentation and
it slight differ from s390x/x86. The s390x only considers features that are 
used in ifunc resolver, but since the idea is to use __builtin_cpu_supports
maybe powerpc should handle everything.

It also bothers me that our documentation references to a source file to
actually get the supported 'features' strings.  I think we can make it better
and proper document the correct list of features and maybe add a ld.so
option to also print the supported values.