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

[MAHESH BODAPATI <bmahi496@linux.ibm.com>]

On 21/06/23 9:41 am, Peter Bergner wrote:
> 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.


Powerpc32/*/ifunc-impl-list.c ,if we look at the function selection ,
it always happened through ISA and VSX bits but not with altivec bits
so altivec check is redundant here.
Powerpc64/*/ifunc-impl-list.c ,if we see the code. The function selection
always happened through ISA,VSX and altivec bits and altivec got enabled on
higher capable machines always so i didn't make any changes to that.


> 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.
>

If we have disable tunable set like "power8,-power6,-power9" then it 
should set to power8
but power6 and power9 has to be disabled . I disabled ISA bits but not 
VSX and altivec bits
so i didn't include VSX and altivec on the enable/disable sets inside 
the loop.
I saw a specific case where function selection happened on powerpc32 
with only VSX on some libraries so i handled it separately.
I can add altivec similarly but i felt it's redundant for CPU arch-level 
tunable.
If you want me to integrate the single hwcap features (VSX,altivec) then 
i will integrate it and submit a new patch.


> Peter
>
>