Re: [PATCH v2 1/6] x86/cpu-policy: define bits of leaf 6

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.12.2025 14:37, Andrew Cooper wrote:
> On 24/11/2025 12:23 pm, Jan Beulich wrote:
>> ... as far as we presently use them in the codebase.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> Or should we make both parts proper featureset elements? At least
>> APERFMPERF could likely be made visible to guests (in principle).
>> ---
>> v2: Use bool and unions.
>>
>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>> @@ -121,7 +121,46 @@ struct cpu_policy
>>              uint64_t :64, :64; /* Leaf 0x3 - PSN. */
>>              uint64_t :64, :64; /* Leaf 0x4 - Structured Cache. */
>>              uint64_t :64, :64; /* Leaf 0x5 - MONITOR. */
>> -            uint64_t :64, :64; /* Leaf 0x6 - Therm/Perf. */
>> +
>> +            /* Leaf 0x6 - Therm/Perf. */
>> +            union {
>> +                uint32_t _6a;
>> +                struct {
>> +                    bool :1,
>> +                        turbo:1,
>> +                        arat:1,
>> +                        :1,
>> +                        :1,
>> +                        :1,
>> +                        :1,
>> +                        hwp:1,
>> +                        hwp_notification:1,
>> +                        hwp_activity_window:1,
>> +                        hwp_epp:1,
>> +                        hwp_plr:1,
>> +                        :1,
>> +                        hdc:1,
>> +                        :1,
>> +                        :1,
>> +                        hwp_peci:1,
>> +                        :1,
>> +                        :1,
>> +                        hw_feedback:1;
>> +                };
>> +            };
>> +            union {
>> +                uint32_t _6b;
>> +            };
>> +            union {
>> +                uint32_t _6c;
>> +                struct {
>> +                    bool aperfmperf:1;
>> +                };
>> +            };
>> +            union {
>> +                uint32_t _6d;
>> +            };
> 
> The _6[a-d] variables are only needed for the featureset <-> policy
> conversion which isn't the case here (notice how you don't need it the
> series), and we're unlikely to want in the future.
> 
> This wants to read:
> 
>             /* Leaf 0x6 - Therm/Perf. */
>             bool :1,
>                 turbo:1,
>                 arat:1,
>                 :1,
>                 :1,
>                 :1,
>                 :1,
>                 hwp:1,
>                 hwp_notification:1,
>                 hwp_activity_window:1,
>                 hwp_epp:1,
>                 hwp_plr:1,
>                 :1,
>                 hdc:1,
>                 :1,
>                 :1,
>                 hwp_peci:1,
>                 :1,
>                 :1,
>                 hw_feedback:1;
>             uint32_t :32; /* b */
>             bool aperfmperf:1;
>             uint32_t :32; /* d */
> 
> and with that, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Thanks, but: If then someone doesn't pay close attention when breaking up the 
"b"
field (perhaps also into booleans), things may go wrong. I deliberately added 
the
"raw" fields, despite not (presently) being used.

And to be frank: I said I would even before I sent the new version, without you
indicating I shouldn't go this route. Now what is pretty much a mechanical patch
will face yet more of a delay going in. I think requests like this should either
be made in a timely manner, or simply be omitted (in particular if otherwise a
patch is ready to go in). Had Jason not reviewed the series, and had I not
indicated that on this basis I would commit it today, how much longer would I
have had to wait? How's the backlog ever going to reduce this way?

Now that this is going to take yet longer anyway, we can as well deal with your
naming request in reply to patch 6 (which really would apply here first, imo). I
picked names largely based on what was used in the HWP driver, as that was the
in-tree status quo. I can certainly move closer to what the SDM has, but your
reply to patch 6 ended up being non-conclusive to me. I'll reply in more detail
there.

Jan