... as far as we presently use them in the codebase.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Or should we make both parts proper featureset elements? At least
APERFMPERF could likely be made visible to guests (in principle).
--- 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;
+ } pm;
+
uint64_t :64, :64; /* Leaf 0x7 - Structured Features. */
uint64_t :64, :64; /* Leaf 0x8 - rsvd */
uint64_t :64, :64; /* Leaf 0x9 - DCA */On 18/11/2025 3:06 pm, Jan Beulich wrote:
> ... as far as we presently use them in the codebase.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Or should we make both parts proper featureset elements? At least
> APERFMPERF could likely be made visible to guests (in principle).
As before, A/MPERF can't be used safely by a VM.
In order to be persuaded to offer this to VMs, someone is going to have
to present a mechanism for how a VM could even figure out that it read
junk from the regsiters...
>
> --- 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. In particular using uint32_t:1 creates a latent bug in
patch 8.
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.
> + } pm;
Nothing else is sub-scoped. I'd prefer that you drop the 'pm'.
~Andrew
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.
> 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.
>> + } 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?
Jan
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.
>
>> 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).
I suggest a /* 31 spare bits */ comment, because the only other option
is 31x :1's.
>
>>> + } 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.
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.
~Andrew
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
© 2016 - 2026 Red Hat, Inc.