Re: [PATCH] PowerPC: Influence hwcaps via cpu arch-level GLIBC_TUNABLES.

[Peter Bergner <bergner@linux.ibm.com>]

On 6/20/23 12:45 PM, MAHESH BODAPATI wrote:
> On 20/06/23 9:50 pm, Peter Bergner wrote:
>> I'm all for allowing modifying full cpu specific hwcap tunables with one
>> "cpu" option, but it's hard to tell whether this change allows modifying
>> single HWCAP/HWCAP2 features too.  Say I only want to disable the VSX
>> feature or the MMA feature and nothing else.  Does this patch support that?
>> We *do* want that ability!
>>
> This patch will not support single HWCAP/HWCAP2 features. This is only for CPU arch-level features.
> We can add tunable support for single HWCAP/HWCAP2 in a separate patch.

Great to hear!  Like I said, we do want/need that and I actually think
that will be the most common usage for users.



>>> +  if (disable_vsx)
>>> +    cpu_features_curr.hwcap &= ~PPC_FEATURE_HAS_VSX;
>> Why the special handling for the VSX feature here?  How is it different
>> than say the Altivec feature or any of our other feature bits which don't
>> have special handling?  It's not obvious to me why we need special handling,
>> so it's probably not obvious to others either.  If we really do need special
>> handling for this, you should add a comment explaining why.
>>
> On PowerPC32, The function selection happened through VSX feature on some libraries.
> Say I set tunable as "power7,power6" then it should set to power6 but it's picking the power7 specific code
> So I am disabling VSX feature on the machines which are lower than power7 and the code should work on the precedence as well,
> For suppose "power6,power7" then it should set to power7.

So for the "power7,power6" example, you're saying that handling the
power7 tunable enables the VSX HWCAP bit, but when we handle power6,
we need to disable it because last option wins?  If that is the case,
then the current code needs a lot more special handling!  Take for
example "power6,power5".  In this case, power6 will enable the
altivec bit, but power5 doesn't have altivec, so you'll need to
disable that like you disable vsx.  That's only one example, there
are MANY more special cases.  There are also cases where an older
cpu has a feature that doesn't exist in new cpus (eg, htm is in
power8, but not power10).  Those too would have to be handled
specially.

I think the whole issue here, is that you're updating cpu_features_curr
hwcap and hwcap2 values inside the do-while loop, and you have to back
out bits you set when you see another cpu in the tunables list that doesn't
have those bits.  It seems to me that if you wait until after the do-while
loop to update cpu_features_curr with the hwcap and hwcap2 bits of the last
cpu seen, then won't everything just work out without any special handling
needed at all?

So thinking out loud here, it seems when you see a new cpu in the
tunables list, you want to clear out the temporary hwcap/hwcap2
masks (which you're doing unconditionally right now) which throws
away the hwcap/hwcap2 mask from any previous handled cpu.
Then at the end of the do-while loop, you use those temp masks
to set cpu_features_curr.  In the future patch to add support for
handling single feature tunables, you'd just reuse the current temp
hwcap/hwcap2 masks without clearing it.  That way, one could do
"power5,altivec" and you'd get all the power5 hwcap/hwcap2 masks
in addition to altivec.  On the other hand, if you said "altivec,power5",
you'd end up with just the power5 bits, which is what we'd want.


Peter