Signed-off-by: Teddy Astie <teddy.astie@vates.tech>
---
xen/arch/x86/cpu/vpmu_intel.c | 4 ++--
xen/arch/x86/domain.c | 2 +-
xen/include/xen/lib/x86/cpu-policy.h | 10 +++++++++-
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index 1e3b06ef8e..f43faf9567 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -818,8 +818,8 @@ static int cf_check core2_vpmu_initialise(struct vcpu *v)
u64 msr_content;
static bool ds_warned;
- if ( v->domain->arch.cpuid->basic.pmu_version <= 1 ||
- v->domain->arch.cpuid->basic.pmu_version >= 6 )
+ if ( v->domain->arch.cpuid->basic.pmu.version <= 1 ||
+ v->domain->arch.cpuid->basic.pmu.version >= 6 )
return -EINVAL;
if ( (arch_pmc_cnt + fixed_pmc_cnt) == 0 )
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index e658c2d647..5762b38fce 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -458,7 +458,7 @@ void domain_cpu_policy_changed(struct domain *d)
/* If PMU version is zero then the guest doesn't have VPMU */
if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
- p->basic.pmu_version == 0 )
+ p->basic.pmu.version == 0 )
vpmu_destroy(v);
}
}
diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
index d29e380359..9161e2ad8d 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -162,7 +162,15 @@ struct cpu_policy
uint64_t :64, :64; /* Leaf 0x9 - DCA */
/* Leaf 0xa - Intel PMU. */
- uint8_t pmu_version, _pmu[15];
+ struct {
+ uint8_t /* a */ version, num_gp_ctrs, gp_ctr_width,
+ event_enum_length;
+ uint32_t /* b */:32;
+ uint32_t /* c */ fixed_ctr_mask;
+ uint32_t /* d */ num_fixed_ctr:5, fixed_ctr_width:8, :1,
+ anythread_depreciation:1, slots_per_cyc:4,
+ :13;
+ } pmu;
uint64_t :64, :64; /* Leaf 0xb - Topology. */
uint64_t :64, :64; /* Leaf 0xc - rsvd */
--
2.53.0
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.techOn 10.03.2026 17:44, Teddy Astie wrote:
> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -162,7 +162,15 @@ struct cpu_policy
> uint64_t :64, :64; /* Leaf 0x9 - DCA */
>
> /* Leaf 0xa - Intel PMU. */
> - uint8_t pmu_version, _pmu[15];
> + struct {
> + uint8_t /* a */ version, num_gp_ctrs, gp_ctr_width,
> + event_enum_length;
> + uint32_t /* b */:32;
> + uint32_t /* c */ fixed_ctr_mask;
> + uint32_t /* d */ num_fixed_ctr:5, fixed_ctr_width:8, :1,
> + anythread_depreciation:1, slots_per_cyc:4,
> + :13;
> + } pmu;
Style-wise this looks to follow e.g. the cache leaf, so perhaps okay, even
if I would have preferred you to follow what we did for leaf 6. The named
boolean field, however, wants to be of type bool. And then the unnamed 1-bit
field really wants to be 2 bits, for anythread_depreciation to be bit 15
(etc).
Jan
Le 24/03/2026 à 10:25, Jan Beulich a écrit :
> On 10.03.2026 17:44, Teddy Astie wrote:
>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>> @@ -162,7 +162,15 @@ struct cpu_policy
>> uint64_t :64, :64; /* Leaf 0x9 - DCA */
>>
>> /* Leaf 0xa - Intel PMU. */
>> - uint8_t pmu_version, _pmu[15];
>> + struct {
>> + uint8_t /* a */ version, num_gp_ctrs, gp_ctr_width,
>> + event_enum_length;
>> + uint32_t /* b */:32;
>> + uint32_t /* c */ fixed_ctr_mask;
>> + uint32_t /* d */ num_fixed_ctr:5, fixed_ctr_width:8, :1,
>> + anythread_depreciation:1, slots_per_cyc:4,
>> + :13;
>> + } pmu;
>
> Style-wise this looks to follow e.g. the cache leaf, so perhaps okay, even
> if I would have preferred you to follow what we did for leaf 6.
My idea was to put all that as .pmu.*, so I wouldn't need to prefix
everything with "pmu_". I'm not sure if you're talking about a different
approach.
> The named> boolean field, however, wants to be of type bool.
Which fields ?
> And then the unnamed 1-bit> field really wants to be 2 bits, for
anythread_depreciation to be bit 15
> (etc).
>
Ah yes thanks, I got confused with the fields size for a second.
I also found that slots_per_cyc is 3 bits instead of 4.
I think this diff fixes it overall.
diff --git a/xen/include/xen/lib/x86/cpu-policy.h
b/xen/include/xen/lib/x86/cpu-policy.h
index 9161e2ad8d..796c2edb0e 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -167,9 +167,9 @@ struct cpu_policy
event_enum_length;
uint32_t /* b */:32;
uint32_t /* c */ fixed_ctr_mask;
- uint32_t /* d */ num_fixed_ctr:5, fixed_ctr_width:8, :1,
- anythread_depreciation:1, slots_per_cyc:4,
- :13;
+ uint32_t /* d */ num_fixed_ctr:5, fixed_ctr_width:8, :2,
+ anythread_depreciation:1, slots_per_cyc:3,
+ :11;
} pmu;
uint64_t :64, :64; /* Leaf 0xb - Topology. */
Making the first edx reserved gap actually 2 bits, slots_per_cyc
actually 3 bits and adjusting the end reserved part that is actually 11
bits.
> Jan
>
Teddy
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
On 25.03.2026 10:48, Teddy Astie wrote:
> Le 24/03/2026 à 10:25, Jan Beulich a écrit :
>> On 10.03.2026 17:44, Teddy Astie wrote:
>>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>>> @@ -162,7 +162,15 @@ struct cpu_policy
>>> uint64_t :64, :64; /* Leaf 0x9 - DCA */
>>>
>>> /* Leaf 0xa - Intel PMU. */
>>> - uint8_t pmu_version, _pmu[15];
>>> + struct {
>>> + uint8_t /* a */ version, num_gp_ctrs, gp_ctr_width,
>>> + event_enum_length;
>>> + uint32_t /* b */:32;
>>> + uint32_t /* c */ fixed_ctr_mask;
>>> + uint32_t /* d */ num_fixed_ctr:5, fixed_ctr_width:8, :1,
>>> + anythread_depreciation:1, slots_per_cyc:4,
>>> + :13;
>>> + } pmu;
>>
>> Style-wise this looks to follow e.g. the cache leaf, so perhaps okay, even
>> if I would have preferred you to follow what we did for leaf 6.
>
> My idea was to put all that as .pmu.*, so I wouldn't need to prefix
> everything with "pmu_". I'm not sure if you're talking about a different
> approach.
The "pmu" is fine. I'm talking of what's inside the struct {}.
> > The named> boolean field, however, wants to be of type bool.
>
> Which fields ?
There's only one named 1-bit field: anythread_depreciation.
> > And then the unnamed 1-bit> field really wants to be 2 bits, for
> anythread_depreciation to be bit 15
>> (etc).
>>
>
> Ah yes thanks, I got confused with the fields size for a second.
> I also found that slots_per_cyc is 3 bits instead of 4.
Not as far as I can see.
> I think this diff fixes it overall.
>
> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -167,9 +167,9 @@ struct cpu_policy
> event_enum_length;
> uint32_t /* b */:32;
> uint32_t /* c */ fixed_ctr_mask;
> - uint32_t /* d */ num_fixed_ctr:5, fixed_ctr_width:8, :1,
> - anythread_depreciation:1, slots_per_cyc:4,
> - :13;
> + uint32_t /* d */ num_fixed_ctr:5, fixed_ctr_width:8, :2,
> + anythread_depreciation:1, slots_per_cyc:3,
> + :11;
Why 11 all of the sudden?
Jan
Le 25/03/2026 à 12:43, Jan Beulich a écrit :
> On 25.03.2026 10:48, Teddy Astie wrote:
>> Le 24/03/2026 à 10:25, Jan Beulich a écrit :
>>> On 10.03.2026 17:44, Teddy Astie wrote:
>>>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>>>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>>>> @@ -162,7 +162,15 @@ struct cpu_policy
>>>> uint64_t :64, :64; /* Leaf 0x9 - DCA */
>>>>
>>>> /* Leaf 0xa - Intel PMU. */
>>>> - uint8_t pmu_version, _pmu[15];
>>>> + struct {
>>>> + uint8_t /* a */ version, num_gp_ctrs, gp_ctr_width,
>>>> + event_enum_length;
>>>> + uint32_t /* b */:32;
>>>> + uint32_t /* c */ fixed_ctr_mask;
>>>> + uint32_t /* d */ num_fixed_ctr:5, fixed_ctr_width:8, :1,
>>>> + anythread_depreciation:1, slots_per_cyc:4,
>>>> + :13;
>>>> + } pmu;
>>>
>>> Style-wise this looks to follow e.g. the cache leaf, so perhaps okay, even
>>> if I would have preferred you to follow what we did for leaf 6.
>>
>> My idea was to put all that as .pmu.*, so I wouldn't need to prefix
>> everything with "pmu_". I'm not sure if you're talking about a different
>> approach.
>
> The "pmu" is fine. I'm talking of what's inside the struct {}.
>
Is it regarding having union and _aa, _ab, (...) fields or prefixing
fields with pmu_ ?
>> > The named> boolean field, however, wants to be of type bool.
>>
>> Which fields ?
>
> There's only one named 1-bit field: anythread_depreciation.
>
>> > And then the unnamed 1-bit> field really wants to be 2 bits, for
>> anythread_depreciation to be bit 15
>>> (etc).
>>>
>>
>> Ah yes thanks, I got confused with the fields size for a second.
>> I also found that slots_per_cyc is 3 bits instead of 4.
>
> Not as far as I can see.
>
>> I think this diff fixes it overall.
>>
>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>> @@ -167,9 +167,9 @@ struct cpu_policy
>> event_enum_length;
>> uint32_t /* b */:32;
>> uint32_t /* c */ fixed_ctr_mask;
>> - uint32_t /* d */ num_fixed_ctr:5, fixed_ctr_width:8, :1,
>> - anythread_depreciation:1, slots_per_cyc:4,
>> - :13;
>> + uint32_t /* d */ num_fixed_ctr:5, fixed_ctr_width:8, :2,
>> + anythread_depreciation:1, slots_per_cyc:3,
>> + :11;
>
> Why 11 all of the sudden?
>
Okay, I think I finally figured out field sizes (5-bit field confuses me).
struct {
uint8_t /* a */ version, num_gp_ctrs, gp_ctr_width,
event_enum_length;
uint32_t /* b */:32;
uint32_t /* c */ fixed_ctr_mask;
uint32_t /* d */ num_fixed_ctr:5, fixed_ctr_width:8, :2;
bool anythread_depreciation:1;
uint32_t slots_per_cyc:4, :12;
} pmu;
> Jan
>
Teddy
--
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
On 25.03.2026 14:05, Teddy Astie wrote:
> Le 25/03/2026 à 12:43, Jan Beulich a écrit :
>> On 25.03.2026 10:48, Teddy Astie wrote:
>>> Le 24/03/2026 à 10:25, Jan Beulich a écrit :
>>>> On 10.03.2026 17:44, Teddy Astie wrote:
>>>>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>>>>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>>>>> @@ -162,7 +162,15 @@ struct cpu_policy
>>>>> uint64_t :64, :64; /* Leaf 0x9 - DCA */
>>>>>
>>>>> /* Leaf 0xa - Intel PMU. */
>>>>> - uint8_t pmu_version, _pmu[15];
>>>>> + struct {
>>>>> + uint8_t /* a */ version, num_gp_ctrs, gp_ctr_width,
>>>>> + event_enum_length;
>>>>> + uint32_t /* b */:32;
>>>>> + uint32_t /* c */ fixed_ctr_mask;
>>>>> + uint32_t /* d */ num_fixed_ctr:5, fixed_ctr_width:8, :1,
>>>>> + anythread_depreciation:1, slots_per_cyc:4,
>>>>> + :13;
>>>>> + } pmu;
>>>>
>>>> Style-wise this looks to follow e.g. the cache leaf, so perhaps okay, even
>>>> if I would have preferred you to follow what we did for leaf 6.
>>>
>>> My idea was to put all that as .pmu.*, so I wouldn't need to prefix
>>> everything with "pmu_". I'm not sure if you're talking about a different
>>> approach.
>>
>> The "pmu" is fine. I'm talking of what's inside the struct {}.
>>
>
> Is it regarding having union and _aa, _ab, (...) fields or prefixing
> fields with pmu_ ?
As said, the "pmu" name of the containing struct is fine. Obviously then there
is no need for pmu_ prefixes. The differences between the cache leaf and leaf
6 go beyond the union aspect though, and I really mean all differences there.
As Andrew wasn't overly happy with the _6a and _6c union members, I wouldn't
insist on the introduction of counterparts here (and if they were omitted,
unions wouldn't be needed either). I think it's advisable though, allowing to
omit the trailing unnamed bitfield (the size of which you've now corrected a
2nd time).
Jan
© 2016 - 2026 Red Hat, Inc.