Re: [PATCH 2/8] x86/cpu-policy: define bits of leaf 6

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.11.2025 18:20, Andrew Cooper wrote:
> On 18/11/2025 4:53 pm, Jan Beulich wrote:
>> On 18.11.2025 16:30, Andrew Cooper wrote:
>>> On 18/11/2025 3:06 pm, Jan Beulich wrote:
>>>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>>>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>>>> @@ -121,7 +121,31 @@ 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. */
>>>> +            struct {
>>>> +                uint32_t /* a */:1,
>>>> +                    turbo:1,
>>>> +                    arat:1,
>>>> +                    :4,
>>>> +                    hwp:1,
>>>> +                    hwp_notification:1,
>>>> +                    hwp_activity_window:1,
>>>> +                    hwp_epp:1,
>>>> +                    hwp_plr:1,
>>>> +                    :1,
>>>> +                    hdc:1,
>>>> +                    :2,
>>>> +                    hwp_peci:1,
>>>> +                    :2,
>>>> +                    hw_feedback:1,
>>>> +                    :12;
>>>> +                uint32_t /* b */:32;
>>>> +                uint32_t /* c */ aperfmperf:1,
>>>> +                    :31;
>>>> +                uint32_t /* d */:32;
>>> Elsewhere, single bit fields are bool foo:1, and these want to match for
>>> consistency.
>> Oh, yes, will change.
>>
>>>   In particular using uint32_t:1 creates a latent bug in
>>> patch 8.
>> I don't see where that would be.
> 
> In the printf.  %d vs %u.  Latent because it's ok until bit 31 gets
> used, and then it's not ok.

How would there be a difference? Every bit is individually unsigned when
the field type is uint32_t, so even bit 31 will cleanly produce 0 / 1
when read.

>>> One problem with bool bitfields is that your :4 needs to become 4x :1. 
>>> Right now his hidden in the macros that gen-cpuid.py makes.
>>>
>>> Given that b is of type uint32_t, you can omit the :12 from the end of a
>>> and leave a comment.  Similarly, the trailing :31 on c can be dropped.
>> We have these in many other places, and omitting in particular the :31
>> would also feel somewhat fragile / misleading. It'll need to be
>>
>>                 bool     /* c */ aperfmperf:1;
>>                 uint32_t :31;
>>
>> or something along these lines.
> 
> This doesn't work.  A gap of 31 bits gets inserted because of uint32_t's
> alignment, which is why the suggestion to ignore it does work (even if
> fragile).

Interesting. When (iirc) we converted the AMD IOMMU machinery to use
struct-s, it was me to be concerned of this, and you telling me the
opposite of what you say now. See how there's no problem in e.g.
struct amd_iommu_dte.

> I suggest a /* 31 spare bits */ comment, because the only other option
> is 31x :1's.

I'll do this differently anyway, ..

>>>> +            } pm;
>>> Nothing else is sub-scoped.  I'd prefer that you drop the 'pm'.
>> Wouldn't that require the use of the very extension you just talked about
>> at the committer's call?
> 
> No. It would just be a plain anonymous struct in this case, but it
> doesn't even need to be a struct.

... having realized over night that this would be what you ask for, and ...

> leaf 0,2,10 are all "top level" insofar as they're all inside .basic. 
> leaf 1 only has anonymous unions to join the the bitfield names and the
> field-wide name.

... getting things closer to what we use there:

            union {
                uint32_t _6c;
                struct { ... };
            };

Then I can safely and obviously omit any unused tail bits in the bitfield
set. I hope that's going to be okay with you.

Jan