[Patch 0/2] Enable topdown slots event in vPMU

[Patch 0/2] Enable topdown slots event in vPMU

[Dapeng Mi]

The topdown slots event counts the total number of available slots for
an unhalted logical processor. Software can use this event to calculate
the topdown metrics by collaborating with IA32_PERF_METRICS MSR.

Since Intel Icelake CPU starts, the topdown slots event can be
programmed both on GP counters or the exclusive fixed counter 3 but with
different event & umask code. The event with code (event=0xa4,umask=0x01)
is an architectural event which is represented in CPUID.0AH.EBX and can
be programed on any GP counter. Besides, Intel PMU from Icelake
introduces a new fixed counter (fixed counter 3) to count/sample
todpown slots event so the precious GP counters can be saved. The fixed
counter 3 uses an exclusive code (event=0x00,umask=0x04) to count/sample
the slots event.

Actually this patchset is a portion of the patchset "Enable fixed counter
 3 and topdown perf metrics for vPMU"[1]. As this original patchset needs to
make some fundamental changes on perf code and cause big arguments, it
leads to the vPMU topdown metrics patchset is hard to be merged in current
vPMU emulation framework.

The patches of enabling topdown slots event is simple and doesn't
touch any perf code. Moreover topdown slots event as an independent
feature is still valuable even though in no topdown metrics cases, some
perf metrics depend on slots event and need to be derived from slots
event.

Thus the patches of enabling slots event is extracted as an independent
patchset and resend.

Ref:
1. https://lore.kernel.org/all/20230927033124.1226509-1-dapeng1.mi@linux.intel.com/T/


Dapeng Mi (2):
  KVM: x86/pmu: Add Intel CPUID-hinted TopDown slots event
  KVM: x86/pmu: Support PMU fixed counter 3

 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/vmx/pmu_intel.c    | 12 ++++++++++++
 arch/x86/kvm/x86.c              |  4 ++--
 3 files changed, 15 insertions(+), 3 deletions(-)


base-commit: 35dcbd9e47035f98f3910ae420bf10892c9bdc99
-- 
2.34.1

[Patch 1/2] KVM: x86/pmu: Add Intel CPUID-hinted TopDown slots event

[Dapeng Mi]

This patch adds support for the architectural topdown slots event which
is hinted by CPUID.0AH.EBX.

The topdown slots event counts the total number of available slots for
an unhalted logical processor. Software can use this event as the
denominator for the top-level metrics of the topDown Microarchitecture
Analysis method.

Although the MSR_PERF_METRICS MSR required for topdown events is not
currently available in the guest, relying only on the data provided by
the slots event is sufficient for pmu users to perceive differences in
cpu pipeline machine-width across micro-architectures.

The standalone slots event, like the instruction event, can be counted
with gp counter or fixed counter 3 (if any). Its availability is also
controlled by CPUID.AH.EBX. On Linux, perf user may encode
"-e cpu/event=0xa4,umask=0x01/" or "-e cpu/slots/" to count slots events.

This patch only enables slots event on GP counters. The enabling on fixed
counter 3 will be supported in subsequent patches.

Co-developed-by: Like Xu <likexu@tencent.com>
Signed-off-by: Like Xu <likexu@tencent.com>
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 arch/x86/kvm/vmx/pmu_intel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 820d3e1f6b4f..e32353f1143f 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -34,6 +34,7 @@ enum intel_pmu_architectural_events {
 	INTEL_ARCH_LLC_MISSES,
 	INTEL_ARCH_BRANCHES_RETIRED,
 	INTEL_ARCH_BRANCHES_MISPREDICTED,
+	INTEL_ARCH_TOPDOWN_SLOTS,
 
 	NR_REAL_INTEL_ARCH_EVENTS,
 
@@ -58,6 +59,7 @@ static struct {
 	[INTEL_ARCH_LLC_MISSES]			= { 0x2e, 0x41 },
 	[INTEL_ARCH_BRANCHES_RETIRED]		= { 0xc4, 0x00 },
 	[INTEL_ARCH_BRANCHES_MISPREDICTED]	= { 0xc5, 0x00 },
+	[INTEL_ARCH_TOPDOWN_SLOTS]		= { 0xa4, 0x01 },
 	[PSEUDO_ARCH_REFERENCE_CYCLES]		= { 0x00, 0x03 },
 };
 
-- 
2.34.1

Re: [Patch 1/2] KVM: x86/pmu: Add Intel CPUID-hinted TopDown slots event

[Jim Mattson]

On Tue, Oct 31, 2023 at 1:58 AM Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
>
> This patch adds support for the architectural topdown slots event which
> is hinted by CPUID.0AH.EBX.

Can't a guest already program an event selector to count event select
0xa4, unit mask 1, unless the event is prohibited by
KVM_SET_PMU_EVENT_FILTER?

AFAICT, this change just enables event filtering based on
CPUID.0AH:EBX[bit 7] (though it's not clear to me why two independent
mechanisms are necessary for event filtering).

Re: [Patch 1/2] KVM: x86/pmu: Add Intel CPUID-hinted TopDown slots event

[Mi, Dapeng]

On 11/1/2023 2:22 AM, Jim Mattson wrote:
> On Tue, Oct 31, 2023 at 1:58 AM Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
>> This patch adds support for the architectural topdown slots event which
>> is hinted by CPUID.0AH.EBX.
> Can't a guest already program an event selector to count event select
> 0xa4, unit mask 1, unless the event is prohibited by
> KVM_SET_PMU_EVENT_FILTER?

Actually defining this new slots arch event is to do the sanity check 
for supported arch-events which is enumerated by CPUID.0AH.EBX. 
Currently vPMU would check if the arch event from guest is supported by 
KVM. If not, it would be rejected just like intel_hw_event_available() 
shows.

If we don't add the slots event in the intel_arch_events[] array, guest 
may program the slots event and pass the sanity check of KVM on a 
platform which actually doesn't support slots event and program the 
event on a real GP counter and got an invalid count. This is not correct.


>
> AFAICT, this change just enables event filtering based on
> CPUID.0AH:EBX[bit 7] (though it's not clear to me why two independent
> mechanisms are necessary for event filtering).


IMO, these are two different things. this change is just to enable the 
supported arch events check for slot events, the event filtering is 
another thing.

Re: [Patch 1/2] KVM: x86/pmu: Add Intel CPUID-hinted TopDown slots event

[Jim Mattson]

On Tue, Oct 31, 2023 at 6:59 PM Mi, Dapeng <dapeng1.mi@linux.intel.com> wrote:
>
> On 11/1/2023 2:22 AM, Jim Mattson wrote:
> > On Tue, Oct 31, 2023 at 1:58 AM Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
> >> This patch adds support for the architectural topdown slots event which
> >> is hinted by CPUID.0AH.EBX.
> > Can't a guest already program an event selector to count event select
> > 0xa4, unit mask 1, unless the event is prohibited by
> > KVM_SET_PMU_EVENT_FILTER?
>
> Actually defining this new slots arch event is to do the sanity check
> for supported arch-events which is enumerated by CPUID.0AH.EBX.
> Currently vPMU would check if the arch event from guest is supported by
> KVM. If not, it would be rejected just like intel_hw_event_available()
> shows.
>
> If we don't add the slots event in the intel_arch_events[] array, guest
> may program the slots event and pass the sanity check of KVM on a
> platform which actually doesn't support slots event and program the
> event on a real GP counter and got an invalid count. This is not correct.

On physical hardware, it is possible to program a GP counter with the
event selector and unit mask of the slots event whether or not the
platform supports it. Isn't KVM wrong to disallow something that a
physical CPU allows?

> >
> > AFAICT, this change just enables event filtering based on
> > CPUID.0AH:EBX[bit 7] (though it's not clear to me why two independent
> > mechanisms are necessary for event filtering).
>
>
> IMO, these are two different things. this change is just to enable the
> supported arch events check for slot events, the event filtering is
> another thing.

How is clearing CPUID.0AH:EBX[bit 7] any different from putting {event
select 0xa4, unit mask 1} in a deny list with the PMU event filter?

Re: [Patch 1/2] KVM: x86/pmu: Add Intel CPUID-hinted TopDown slots event

[Mi, Dapeng]

On 11/1/2023 11:04 AM, Jim Mattson wrote:
> On Tue, Oct 31, 2023 at 6:59 PM Mi, Dapeng <dapeng1.mi@linux.intel.com> wrote:
>> On 11/1/2023 2:22 AM, Jim Mattson wrote:
>>> On Tue, Oct 31, 2023 at 1:58 AM Dapeng Mi <dapeng1.mi@linux.intel.com> wrote:
>>>> This patch adds support for the architectural topdown slots event which
>>>> is hinted by CPUID.0AH.EBX.
>>> Can't a guest already program an event selector to count event select
>>> 0xa4, unit mask 1, unless the event is prohibited by
>>> KVM_SET_PMU_EVENT_FILTER?
>> Actually defining this new slots arch event is to do the sanity check
>> for supported arch-events which is enumerated by CPUID.0AH.EBX.
>> Currently vPMU would check if the arch event from guest is supported by
>> KVM. If not, it would be rejected just like intel_hw_event_available()
>> shows.
>>
>> If we don't add the slots event in the intel_arch_events[] array, guest
>> may program the slots event and pass the sanity check of KVM on a
>> platform which actually doesn't support slots event and program the
>> event on a real GP counter and got an invalid count. This is not correct.
> On physical hardware, it is possible to program a GP counter with the
> event selector and unit mask of the slots event whether or not the
> platform supports it. Isn't KVM wrong to disallow something that a
> physical CPU allows?


Yeah, I agree. But I'm not sure if this is a flaw on PMU driver. If an 
event is not supported by the hardware,  we can't predict the PMU's 
behavior and a meaningless count may be returned and this could mislead 
the user.

Add Kan to confirm this.

Hi Kan,

Have you any comments on this? Thanks.


>
>>> AFAICT, this change just enables event filtering based on
>>> CPUID.0AH:EBX[bit 7] (though it's not clear to me why two independent
>>> mechanisms are necessary for event filtering).
>>
>> IMO, these are two different things. this change is just to enable the
>> supported arch events check for slot events, the event filtering is
>> another thing.
> How is clearing CPUID.0AH:EBX[bit 7] any different from putting {event
> select 0xa4, unit mask 1} in a deny list with the PMU event filter?

I think there is no difference in the conclusion but with two different 
methods.

Re: [Patch 1/2] KVM: x86/pmu: Add Intel CPUID-hinted TopDown slots event

[Liang, Kan]

On 2023-10-31 11:31 p.m., Mi, Dapeng wrote:
> 
> On 11/1/2023 11:04 AM, Jim Mattson wrote:
>> On Tue, Oct 31, 2023 at 6:59 PM Mi, Dapeng
>> <dapeng1.mi@linux.intel.com> wrote:
>>> On 11/1/2023 2:22 AM, Jim Mattson wrote:
>>>> On Tue, Oct 31, 2023 at 1:58 AM Dapeng Mi
>>>> <dapeng1.mi@linux.intel.com> wrote:
>>>>> This patch adds support for the architectural topdown slots event
>>>>> which
>>>>> is hinted by CPUID.0AH.EBX.
>>>> Can't a guest already program an event selector to count event select
>>>> 0xa4, unit mask 1, unless the event is prohibited by
>>>> KVM_SET_PMU_EVENT_FILTER?
>>> Actually defining this new slots arch event is to do the sanity check
>>> for supported arch-events which is enumerated by CPUID.0AH.EBX.
>>> Currently vPMU would check if the arch event from guest is supported by
>>> KVM. If not, it would be rejected just like intel_hw_event_available()
>>> shows.
>>>
>>> If we don't add the slots event in the intel_arch_events[] array, guest
>>> may program the slots event and pass the sanity check of KVM on a
>>> platform which actually doesn't support slots event and program the
>>> event on a real GP counter and got an invalid count. This is not
>>> correct.
>> On physical hardware, it is possible to program a GP counter with the
>> event selector and unit mask of the slots event whether or not the
>> platform supports it. Isn't KVM wrong to disallow something that a
>> physical CPU allows?
> 
> 
> Yeah, I agree. But I'm not sure if this is a flaw on PMU driver. If an
> event is not supported by the hardware,  we can't predict the PMU's
> behavior and a meaningless count may be returned and this could mislead
> the user.

The user can program any events on the GP counter. The perf doesn't
limit it. For the unsupported event, 0 should be returned. Please keep
in mind, the event list keeps updating. If the kernel checks for each
event, it could be a disaster. I don't think it's a flaw.

Thanks,
Kan
> 
> Add Kan to confirm this.
> 
> Hi Kan,
> 
> Have you any comments on this? Thanks.
> 
> 
>>
>>>> AFAICT, this change just enables event filtering based on
>>>> CPUID.0AH:EBX[bit 7] (though it's not clear to me why two independent
>>>> mechanisms are necessary for event filtering).
>>>
>>> IMO, these are two different things. this change is just to enable the
>>> supported arch events check for slot events, the event filtering is
>>> another thing.
>> How is clearing CPUID.0AH:EBX[bit 7] any different from putting {event
>> select 0xa4, unit mask 1} in a deny list with the PMU event filter?
> 
> I think there is no difference in the conclusion but with two different
> methods.
> 
> 

Re: [Patch 1/2] KVM: x86/pmu: Add Intel CPUID-hinted TopDown slots event

[Mi, Dapeng]

On 11/1/2023 9:33 PM, Liang, Kan wrote:
>
> On 2023-10-31 11:31 p.m., Mi, Dapeng wrote:
>> On 11/1/2023 11:04 AM, Jim Mattson wrote:
>>> On Tue, Oct 31, 2023 at 6:59 PM Mi, Dapeng
>>> <dapeng1.mi@linux.intel.com> wrote:
>>>> On 11/1/2023 2:22 AM, Jim Mattson wrote:
>>>>> On Tue, Oct 31, 2023 at 1:58 AM Dapeng Mi
>>>>> <dapeng1.mi@linux.intel.com> wrote:
>>>>>> This patch adds support for the architectural topdown slots event
>>>>>> which
>>>>>> is hinted by CPUID.0AH.EBX.
>>>>> Can't a guest already program an event selector to count event select
>>>>> 0xa4, unit mask 1, unless the event is prohibited by
>>>>> KVM_SET_PMU_EVENT_FILTER?
>>>> Actually defining this new slots arch event is to do the sanity check
>>>> for supported arch-events which is enumerated by CPUID.0AH.EBX.
>>>> Currently vPMU would check if the arch event from guest is supported by
>>>> KVM. If not, it would be rejected just like intel_hw_event_available()
>>>> shows.
>>>>
>>>> If we don't add the slots event in the intel_arch_events[] array, guest
>>>> may program the slots event and pass the sanity check of KVM on a
>>>> platform which actually doesn't support slots event and program the
>>>> event on a real GP counter and got an invalid count. This is not
>>>> correct.
>>> On physical hardware, it is possible to program a GP counter with the
>>> event selector and unit mask of the slots event whether or not the
>>> platform supports it. Isn't KVM wrong to disallow something that a
>>> physical CPU allows?
>>
>> Yeah, I agree. But I'm not sure if this is a flaw on PMU driver. If an
>> event is not supported by the hardware,  we can't predict the PMU's
>> behavior and a meaningless count may be returned and this could mislead
>> the user.
> The user can program any events on the GP counter. The perf doesn't
> limit it. For the unsupported event, 0 should be returned. Please keep
> in mind, the event list keeps updating. If the kernel checks for each
> event, it could be a disaster. I don't think it's a flaw.


Thanks Kan, it would be ok as long as 0 is always returned for 
unsupported events. IMO, it's a nice to have feature that KVM does this 
sanity check for supported arch events, it won't break anything.


>
> Thanks,
> Kan
>> Add Kan to confirm this.
>>
>> Hi Kan,
>>
>> Have you any comments on this? Thanks.
>>
>>
>>>>> AFAICT, this change just enables event filtering based on
>>>>> CPUID.0AH:EBX[bit 7] (though it's not clear to me why two independent
>>>>> mechanisms are necessary for event filtering).
>>>> IMO, these are two different things. this change is just to enable the
>>>> supported arch events check for slot events, the event filtering is
>>>> another thing.
>>> How is clearing CPUID.0AH:EBX[bit 7] any different from putting {event
>>> select 0xa4, unit mask 1} in a deny list with the PMU event filter?
>> I think there is no difference in the conclusion but with two different
>> methods.
>>
>>

Re: [Patch 1/2] KVM: x86/pmu: Add Intel CPUID-hinted TopDown slots event

[Jim Mattson]

On Wed, Nov 1, 2023 at 7:07 PM Mi, Dapeng <dapeng1.mi@linux.intel.com> wrote:
>
>
> On 11/1/2023 9:33 PM, Liang, Kan wrote:
> >
> > On 2023-10-31 11:31 p.m., Mi, Dapeng wrote:
> >> On 11/1/2023 11:04 AM, Jim Mattson wrote:
> >>> On Tue, Oct 31, 2023 at 6:59 PM Mi, Dapeng
> >>> <dapeng1.mi@linux.intel.com> wrote:
> >>>> On 11/1/2023 2:22 AM, Jim Mattson wrote:
> >>>>> On Tue, Oct 31, 2023 at 1:58 AM Dapeng Mi
> >>>>> <dapeng1.mi@linux.intel.com> wrote:
> >>>>>> This patch adds support for the architectural topdown slots event
> >>>>>> which
> >>>>>> is hinted by CPUID.0AH.EBX.
> >>>>> Can't a guest already program an event selector to count event select
> >>>>> 0xa4, unit mask 1, unless the event is prohibited by
> >>>>> KVM_SET_PMU_EVENT_FILTER?
> >>>> Actually defining this new slots arch event is to do the sanity check
> >>>> for supported arch-events which is enumerated by CPUID.0AH.EBX.
> >>>> Currently vPMU would check if the arch event from guest is supported by
> >>>> KVM. If not, it would be rejected just like intel_hw_event_available()
> >>>> shows.
> >>>>
> >>>> If we don't add the slots event in the intel_arch_events[] array, guest
> >>>> may program the slots event and pass the sanity check of KVM on a
> >>>> platform which actually doesn't support slots event and program the
> >>>> event on a real GP counter and got an invalid count. This is not
> >>>> correct.
> >>> On physical hardware, it is possible to program a GP counter with the
> >>> event selector and unit mask of the slots event whether or not the
> >>> platform supports it. Isn't KVM wrong to disallow something that a
> >>> physical CPU allows?
> >>
> >> Yeah, I agree. But I'm not sure if this is a flaw on PMU driver. If an
> >> event is not supported by the hardware,  we can't predict the PMU's
> >> behavior and a meaningless count may be returned and this could mislead
> >> the user.
> > The user can program any events on the GP counter. The perf doesn't
> > limit it. For the unsupported event, 0 should be returned. Please keep
> > in mind, the event list keeps updating. If the kernel checks for each
> > event, it could be a disaster. I don't think it's a flaw.
>
>
> Thanks Kan, it would be ok as long as 0 is always returned for
> unsupported events. IMO, it's a nice to have feature that KVM does this
> sanity check for supported arch events, it won't break anything.

The hardware PMU most assuredly does not return 0 for unsupported events.

For example, if I use host perf to sample event selector 0xa4 unit
mask 1 on a Broadwell host (406f1), I get...

# perf stat -e r01a4 sleep 10

 Performance counter stats for 'sleep 10':

           386,964      r01a4

      10.000907211 seconds time elapsed

Broadwell does not advertise support for architectural event 7 in
CPUID.0AH:EBX, so KVM will refuse to measure this event inside a
guest. That seems broken to me.

>
> >
> > Thanks,
> > Kan
> >> Add Kan to confirm this.
> >>
> >> Hi Kan,
> >>
> >> Have you any comments on this? Thanks.
> >>
> >>
> >>>>> AFAICT, this change just enables event filtering based on
> >>>>> CPUID.0AH:EBX[bit 7] (though it's not clear to me why two independent
> >>>>> mechanisms are necessary for event filtering).
> >>>> IMO, these are two different things. this change is just to enable the
> >>>> supported arch events check for slot events, the event filtering is
> >>>> another thing.
> >>> How is clearing CPUID.0AH:EBX[bit 7] any different from putting {event
> >>> select 0xa4, unit mask 1} in a deny list with the PMU event filter?
> >> I think there is no difference in the conclusion but with two different
> >> methods.
> >>
> >>

Re: [Patch 1/2] KVM: x86/pmu: Add Intel CPUID-hinted TopDown slots event

[Mi, Dapeng]

On 11/3/2023 1:45 AM, Jim Mattson wrote:
> On Wed, Nov 1, 2023 at 7:07 PM Mi, Dapeng <dapeng1.mi@linux.intel.com> wrote:
>>
>> On 11/1/2023 9:33 PM, Liang, Kan wrote:
>>> On 2023-10-31 11:31 p.m., Mi, Dapeng wrote:
>>>> On 11/1/2023 11:04 AM, Jim Mattson wrote:
>>>>> On Tue, Oct 31, 2023 at 6:59 PM Mi, Dapeng
>>>>> <dapeng1.mi@linux.intel.com> wrote:
>>>>>> On 11/1/2023 2:22 AM, Jim Mattson wrote:
>>>>>>> On Tue, Oct 31, 2023 at 1:58 AM Dapeng Mi
>>>>>>> <dapeng1.mi@linux.intel.com> wrote:
>>>>>>>> This patch adds support for the architectural topdown slots event
>>>>>>>> which
>>>>>>>> is hinted by CPUID.0AH.EBX.
>>>>>>> Can't a guest already program an event selector to count event select
>>>>>>> 0xa4, unit mask 1, unless the event is prohibited by
>>>>>>> KVM_SET_PMU_EVENT_FILTER?
>>>>>> Actually defining this new slots arch event is to do the sanity check
>>>>>> for supported arch-events which is enumerated by CPUID.0AH.EBX.
>>>>>> Currently vPMU would check if the arch event from guest is supported by
>>>>>> KVM. If not, it would be rejected just like intel_hw_event_available()
>>>>>> shows.
>>>>>>
>>>>>> If we don't add the slots event in the intel_arch_events[] array, guest
>>>>>> may program the slots event and pass the sanity check of KVM on a
>>>>>> platform which actually doesn't support slots event and program the
>>>>>> event on a real GP counter and got an invalid count. This is not
>>>>>> correct.
>>>>> On physical hardware, it is possible to program a GP counter with the
>>>>> event selector and unit mask of the slots event whether or not the
>>>>> platform supports it. Isn't KVM wrong to disallow something that a
>>>>> physical CPU allows?
>>>> Yeah, I agree. But I'm not sure if this is a flaw on PMU driver. If an
>>>> event is not supported by the hardware,  we can't predict the PMU's
>>>> behavior and a meaningless count may be returned and this could mislead
>>>> the user.
>>> The user can program any events on the GP counter. The perf doesn't
>>> limit it. For the unsupported event, 0 should be returned. Please keep
>>> in mind, the event list keeps updating. If the kernel checks for each
>>> event, it could be a disaster. I don't think it's a flaw.
>>
>> Thanks Kan, it would be ok as long as 0 is always returned for
>> unsupported events. IMO, it's a nice to have feature that KVM does this
>> sanity check for supported arch events, it won't break anything.
> The hardware PMU most assuredly does not return 0 for unsupported events.
>
> For example, if I use host perf to sample event selector 0xa4 unit
> mask 1 on a Broadwell host (406f1), I get...
>
> # perf stat -e r01a4 sleep 10
>
>   Performance counter stats for 'sleep 10':
>
>             386,964      r01a4
>
>        10.000907211 seconds time elapsed
>
> Broadwell does not advertise support for architectural event 7 in
> CPUID.0AH:EBX, so KVM will refuse to measure this event inside a
> guest. That seems broken to me.


Yeah, I also saw similar results on Coffee Lake which doesn't support 
slots events and the return count seems to be a random and meaningless 
value. If so, this meaningless value may mislead the guest perf user. 
 From this point view it looks the sanity check in KVM is useful, but it 
indeed leads to different behavior between guest and host.

I'm neutral on either keeping or removing this check. How's other 
reviewers' opinion on this?


>
>>> Thanks,
>>> Kan
>>>> Add Kan to confirm this.
>>>>
>>>> Hi Kan,
>>>>
>>>> Have you any comments on this? Thanks.
>>>>
>>>>
>>>>>>> AFAICT, this change just enables event filtering based on
>>>>>>> CPUID.0AH:EBX[bit 7] (though it's not clear to me why two independent
>>>>>>> mechanisms are necessary for event filtering).
>>>>>> IMO, these are two different things. this change is just to enable the
>>>>>> supported arch events check for slot events, the event filtering is
>>>>>> another thing.
>>>>> How is clearing CPUID.0AH:EBX[bit 7] any different from putting {event
>>>>> select 0xa4, unit mask 1} in a deny list with the PMU event filter?
>>>> I think there is no difference in the conclusion but with two different
>>>> methods.
>>>>
>>>>

Re: [Patch 1/2] KVM: x86/pmu: Add Intel CPUID-hinted TopDown slots event

[Jim Mattson]

On Fri, Nov 3, 2023 at 3:03 AM Mi, Dapeng <dapeng1.mi@linux.intel.com> wrote:
>
>
> On 11/3/2023 1:45 AM, Jim Mattson wrote:
> > On Wed, Nov 1, 2023 at 7:07 PM Mi, Dapeng <dapeng1.mi@linux.intel.com> wrote:
> >>
> >> On 11/1/2023 9:33 PM, Liang, Kan wrote:
> >>> On 2023-10-31 11:31 p.m., Mi, Dapeng wrote:
> >>>> On 11/1/2023 11:04 AM, Jim Mattson wrote:
> >>>>> On Tue, Oct 31, 2023 at 6:59 PM Mi, Dapeng
> >>>>> <dapeng1.mi@linux.intel.com> wrote:
> >>>>>> On 11/1/2023 2:22 AM, Jim Mattson wrote:
> >>>>>>> On Tue, Oct 31, 2023 at 1:58 AM Dapeng Mi
> >>>>>>> <dapeng1.mi@linux.intel.com> wrote:
> >>>>>>>> This patch adds support for the architectural topdown slots event
> >>>>>>>> which
> >>>>>>>> is hinted by CPUID.0AH.EBX.
> >>>>>>> Can't a guest already program an event selector to count event select
> >>>>>>> 0xa4, unit mask 1, unless the event is prohibited by
> >>>>>>> KVM_SET_PMU_EVENT_FILTER?
> >>>>>> Actually defining this new slots arch event is to do the sanity check
> >>>>>> for supported arch-events which is enumerated by CPUID.0AH.EBX.
> >>>>>> Currently vPMU would check if the arch event from guest is supported by
> >>>>>> KVM. If not, it would be rejected just like intel_hw_event_available()
> >>>>>> shows.
> >>>>>>
> >>>>>> If we don't add the slots event in the intel_arch_events[] array, guest
> >>>>>> may program the slots event and pass the sanity check of KVM on a
> >>>>>> platform which actually doesn't support slots event and program the
> >>>>>> event on a real GP counter and got an invalid count. This is not
> >>>>>> correct.
> >>>>> On physical hardware, it is possible to program a GP counter with the
> >>>>> event selector and unit mask of the slots event whether or not the
> >>>>> platform supports it. Isn't KVM wrong to disallow something that a
> >>>>> physical CPU allows?
> >>>> Yeah, I agree. But I'm not sure if this is a flaw on PMU driver. If an
> >>>> event is not supported by the hardware,  we can't predict the PMU's
> >>>> behavior and a meaningless count may be returned and this could mislead
> >>>> the user.
> >>> The user can program any events on the GP counter. The perf doesn't
> >>> limit it. For the unsupported event, 0 should be returned. Please keep
> >>> in mind, the event list keeps updating. If the kernel checks for each
> >>> event, it could be a disaster. I don't think it's a flaw.
> >>
> >> Thanks Kan, it would be ok as long as 0 is always returned for
> >> unsupported events. IMO, it's a nice to have feature that KVM does this
> >> sanity check for supported arch events, it won't break anything.
> > The hardware PMU most assuredly does not return 0 for unsupported events.
> >
> > For example, if I use host perf to sample event selector 0xa4 unit
> > mask 1 on a Broadwell host (406f1), I get...
> >
> > # perf stat -e r01a4 sleep 10
> >
> >   Performance counter stats for 'sleep 10':
> >
> >             386,964      r01a4
> >
> >        10.000907211 seconds time elapsed
> >
> > Broadwell does not advertise support for architectural event 7 in
> > CPUID.0AH:EBX, so KVM will refuse to measure this event inside a
> > guest. That seems broken to me.
>
>
> Yeah, I also saw similar results on Coffee Lake which doesn't support
> slots events and the return count seems to be a random and meaningless
> value. If so, this meaningless value may mislead the guest perf user.
>  From this point view it looks the sanity check in KVM is useful, but it
> indeed leads to different behavior between guest and host.

Calling this a "sanity check" is somewhat specious, since the guards
are based on the guest CPUID rather than the host CPUID. There is
nothing to prevent the hypervisor from setting guest
CPUID.0AH:EAX[31:24] to 32 and guest CPUID.0AH:EBX to 0, making 32
architectural events available to the guest. If it were really a
sanity check, it would prevent the guest from using architectural
events that are not supported by the host.

Moreover, I'm pretty sure that a "sanity check" would only apply to
top-down slots. Have there been any physical CPUs to date that support
architectural events, but do not support all of the first seven
architectural events?

> I'm neutral on either keeping or removing this check. How's other
> reviewers' opinion on this?
>
>
> >
> >>> Thanks,
> >>> Kan
> >>>> Add Kan to confirm this.
> >>>>
> >>>> Hi Kan,
> >>>>
> >>>> Have you any comments on this? Thanks.
> >>>>
> >>>>
> >>>>>>> AFAICT, this change just enables event filtering based on
> >>>>>>> CPUID.0AH:EBX[bit 7] (though it's not clear to me why two independent
> >>>>>>> mechanisms are necessary for event filtering).
> >>>>>> IMO, these are two different things. this change is just to enable the
> >>>>>> supported arch events check for slot events, the event filtering is
> >>>>>> another thing.
> >>>>> How is clearing CPUID.0AH:EBX[bit 7] any different from putting {event
> >>>>> select 0xa4, unit mask 1} in a deny list with the PMU event filter?
> >>>> I think there is no difference in the conclusion but with two different
> >>>> methods.
> >>>>
> >>>>

Re: [Patch 1/2] KVM: x86/pmu: Add Intel CPUID-hinted TopDown slots event

[Liang, Kan]

On 2023-11-02 1:45 p.m., Jim Mattson wrote:
> On Wed, Nov 1, 2023 at 7:07 PM Mi, Dapeng <dapeng1.mi@linux.intel.com> wrote:
>>
>>
>> On 11/1/2023 9:33 PM, Liang, Kan wrote:
>>>
>>> On 2023-10-31 11:31 p.m., Mi, Dapeng wrote:
>>>> On 11/1/2023 11:04 AM, Jim Mattson wrote:
>>>>> On Tue, Oct 31, 2023 at 6:59 PM Mi, Dapeng
>>>>> <dapeng1.mi@linux.intel.com> wrote:
>>>>>> On 11/1/2023 2:22 AM, Jim Mattson wrote:
>>>>>>> On Tue, Oct 31, 2023 at 1:58 AM Dapeng Mi
>>>>>>> <dapeng1.mi@linux.intel.com> wrote:
>>>>>>>> This patch adds support for the architectural topdown slots event
>>>>>>>> which
>>>>>>>> is hinted by CPUID.0AH.EBX.
>>>>>>> Can't a guest already program an event selector to count event select
>>>>>>> 0xa4, unit mask 1, unless the event is prohibited by
>>>>>>> KVM_SET_PMU_EVENT_FILTER?
>>>>>> Actually defining this new slots arch event is to do the sanity check
>>>>>> for supported arch-events which is enumerated by CPUID.0AH.EBX.
>>>>>> Currently vPMU would check if the arch event from guest is supported by
>>>>>> KVM. If not, it would be rejected just like intel_hw_event_available()
>>>>>> shows.
>>>>>>
>>>>>> If we don't add the slots event in the intel_arch_events[] array, guest
>>>>>> may program the slots event and pass the sanity check of KVM on a
>>>>>> platform which actually doesn't support slots event and program the
>>>>>> event on a real GP counter and got an invalid count. This is not
>>>>>> correct.
>>>>> On physical hardware, it is possible to program a GP counter with the
>>>>> event selector and unit mask of the slots event whether or not the
>>>>> platform supports it. Isn't KVM wrong to disallow something that a
>>>>> physical CPU allows?
>>>>
>>>> Yeah, I agree. But I'm not sure if this is a flaw on PMU driver. If an
>>>> event is not supported by the hardware,  we can't predict the PMU's
>>>> behavior and a meaningless count may be returned and this could mislead
>>>> the user.
>>> The user can program any events on the GP counter. The perf doesn't
>>> limit it. For the unsupported event, 0 should be returned. Please keep
>>> in mind, the event list keeps updating. If the kernel checks for each
>>> event, it could be a disaster. I don't think it's a flaw.
>>
>>
>> Thanks Kan, it would be ok as long as 0 is always returned for
>> unsupported events. IMO, it's a nice to have feature that KVM does this
>> sanity check for supported arch events, it won't break anything.
> 
> The hardware PMU most assuredly does not return 0 for unsupported events.
>
> For example, if I use host perf to sample event selector 0xa4 unit
> mask 1 on a Broadwell host (406f1), I get...

I think we have different understanding about the meaning of the
"unsupported". There is no enumeration of the Architectural Topdown
Slots, which only means the Topdown Slots/01a4 is not an architectural
event on the platform. It doesn't mean that the event encoding is
unsupported. It could be used by another event, especially on the
previous platform.

Except for the architectural events, the event encoding of model
specific event is not guaranteed to be the same among different
generations. On BDW, the 01a4 is a model specific event with other
meanings. That's why you can observe some values.

Please make sure to only test the event on an enumerated platform.

Thanks,
Kan
> 
> # perf stat -e r01a4 sleep 10
> 
>  Performance counter stats for 'sleep 10':
> 
>            386,964      r01a4
> 
>       10.000907211 seconds time elapsed
> 
> Broadwell does not advertise support for architectural event 7 in
> CPUID.0AH:EBX, so KVM will refuse to measure this event inside a
> guest. That seems broken to me.
> 
>>
>>>
>>> Thanks,
>>> Kan
>>>> Add Kan to confirm this.
>>>>
>>>> Hi Kan,
>>>>
>>>> Have you any comments on this? Thanks.
>>>>
>>>>
>>>>>>> AFAICT, this change just enables event filtering based on
>>>>>>> CPUID.0AH:EBX[bit 7] (though it's not clear to me why two independent
>>>>>>> mechanisms are necessary for event filtering).
>>>>>> IMO, these are two different things. this change is just to enable the
>>>>>> supported arch events check for slot events, the event filtering is
>>>>>> another thing.
>>>>> How is clearing CPUID.0AH:EBX[bit 7] any different from putting {event
>>>>> select 0xa4, unit mask 1} in a deny list with the PMU event filter?
>>>> I think there is no difference in the conclusion but with two different
>>>> methods.
>>>>
>>>>

Re: [Patch 1/2] KVM: x86/pmu: Add Intel CPUID-hinted TopDown slots event

[Jim Mattson]

On Fri, Nov 3, 2023 at 8:13 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2023-11-02 1:45 p.m., Jim Mattson wrote:
> > On Wed, Nov 1, 2023 at 7:07 PM Mi, Dapeng <dapeng1.mi@linux.intel.com> wrote:
> >>
> >>
> >> On 11/1/2023 9:33 PM, Liang, Kan wrote:
> >>>
> >>> On 2023-10-31 11:31 p.m., Mi, Dapeng wrote:
> >>>> On 11/1/2023 11:04 AM, Jim Mattson wrote:
> >>>>> On Tue, Oct 31, 2023 at 6:59 PM Mi, Dapeng
> >>>>> <dapeng1.mi@linux.intel.com> wrote:
> >>>>>> On 11/1/2023 2:22 AM, Jim Mattson wrote:
> >>>>>>> On Tue, Oct 31, 2023 at 1:58 AM Dapeng Mi
> >>>>>>> <dapeng1.mi@linux.intel.com> wrote:
> >>>>>>>> This patch adds support for the architectural topdown slots event
> >>>>>>>> which
> >>>>>>>> is hinted by CPUID.0AH.EBX.
> >>>>>>> Can't a guest already program an event selector to count event select
> >>>>>>> 0xa4, unit mask 1, unless the event is prohibited by
> >>>>>>> KVM_SET_PMU_EVENT_FILTER?
> >>>>>> Actually defining this new slots arch event is to do the sanity check
> >>>>>> for supported arch-events which is enumerated by CPUID.0AH.EBX.
> >>>>>> Currently vPMU would check if the arch event from guest is supported by
> >>>>>> KVM. If not, it would be rejected just like intel_hw_event_available()
> >>>>>> shows.
> >>>>>>
> >>>>>> If we don't add the slots event in the intel_arch_events[] array, guest
> >>>>>> may program the slots event and pass the sanity check of KVM on a
> >>>>>> platform which actually doesn't support slots event and program the
> >>>>>> event on a real GP counter and got an invalid count. This is not
> >>>>>> correct.
> >>>>> On physical hardware, it is possible to program a GP counter with the
> >>>>> event selector and unit mask of the slots event whether or not the
> >>>>> platform supports it. Isn't KVM wrong to disallow something that a
> >>>>> physical CPU allows?
> >>>>
> >>>> Yeah, I agree. But I'm not sure if this is a flaw on PMU driver. If an
> >>>> event is not supported by the hardware,  we can't predict the PMU's
> >>>> behavior and a meaningless count may be returned and this could mislead
> >>>> the user.
> >>> The user can program any events on the GP counter. The perf doesn't
> >>> limit it. For the unsupported event, 0 should be returned. Please keep
> >>> in mind, the event list keeps updating. If the kernel checks for each
> >>> event, it could be a disaster. I don't think it's a flaw.
> >>
> >>
> >> Thanks Kan, it would be ok as long as 0 is always returned for
> >> unsupported events. IMO, it's a nice to have feature that KVM does this
> >> sanity check for supported arch events, it won't break anything.
> >
> > The hardware PMU most assuredly does not return 0 for unsupported events.
> >
> > For example, if I use host perf to sample event selector 0xa4 unit
> > mask 1 on a Broadwell host (406f1), I get...
>
> I think we have different understanding about the meaning of the
> "unsupported". There is no enumeration of the Architectural Topdown
> Slots, which only means the Topdown Slots/01a4 is not an architectural
> event on the platform. It doesn't mean that the event encoding is
> unsupported. It could be used by another event, especially on the
> previous platform.

If the same event encoding could be used by a microarchitectural event
on a prior platform, then it is *definitely* wrong for KVM to refuse
to monitor the event just because it isn't enumerated as a supported
architectural event.

> Except for the architectural events, the event encoding of model
> specific event is not guaranteed to be the same among different
> generations. On BDW, the 01a4 is a model specific event with other
> meanings. That's why you can observe some values.
>
> Please make sure to only test the event on an enumerated platform.
>
> Thanks,
> Kan
> >
> > # perf stat -e r01a4 sleep 10
> >
> >  Performance counter stats for 'sleep 10':
> >
> >            386,964      r01a4
> >
> >       10.000907211 seconds time elapsed
> >
> > Broadwell does not advertise support for architectural event 7 in
> > CPUID.0AH:EBX, so KVM will refuse to measure this event inside a
> > guest. That seems broken to me.
> >
> >>
> >>>
> >>> Thanks,
> >>> Kan
> >>>> Add Kan to confirm this.
> >>>>
> >>>> Hi Kan,
> >>>>
> >>>> Have you any comments on this? Thanks.
> >>>>
> >>>>
> >>>>>>> AFAICT, this change just enables event filtering based on
> >>>>>>> CPUID.0AH:EBX[bit 7] (though it's not clear to me why two independent
> >>>>>>> mechanisms are necessary for event filtering).
> >>>>>> IMO, these are two different things. this change is just to enable the
> >>>>>> supported arch events check for slot events, the event filtering is
> >>>>>> another thing.
> >>>>> How is clearing CPUID.0AH:EBX[bit 7] any different from putting {event
> >>>>> select 0xa4, unit mask 1} in a deny list with the PMU event filter?
> >>>> I think there is no difference in the conclusion but with two different
> >>>> methods.
> >>>>
> >>>>

Re: [Patch 1/2] KVM: x86/pmu: Add Intel CPUID-hinted TopDown slots event

[Sean Christopherson]

On Fri, Nov 03, 2023, Jim Mattson wrote:
> On Fri, Nov 3, 2023 at 8:13 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >
> >
> >
> > On 2023-11-02 1:45 p.m., Jim Mattson wrote:
> > > On Wed, Nov 1, 2023 at 7:07 PM Mi, Dapeng <dapeng1.mi@linux.intel.com> wrote:
> > >>
> > >>
> > >> On 11/1/2023 9:33 PM, Liang, Kan wrote:
> > >>>
> > >>> On 2023-10-31 11:31 p.m., Mi, Dapeng wrote:
> > >>>> On 11/1/2023 11:04 AM, Jim Mattson wrote:
> > >>>>> On Tue, Oct 31, 2023 at 6:59 PM Mi, Dapeng
> > >>>>> <dapeng1.mi@linux.intel.com> wrote:
> > >>>>>> On 11/1/2023 2:22 AM, Jim Mattson wrote:
> > >>>>>>> On Tue, Oct 31, 2023 at 1:58 AM Dapeng Mi
> > >>>>>>> <dapeng1.mi@linux.intel.com> wrote:
> > >>>>>>>> This patch adds support for the architectural topdown slots event
> > >>>>>>>> which
> > >>>>>>>> is hinted by CPUID.0AH.EBX.
> > >>>>>>> Can't a guest already program an event selector to count event select
> > >>>>>>> 0xa4, unit mask 1, unless the event is prohibited by
> > >>>>>>> KVM_SET_PMU_EVENT_FILTER?
> > >>>>>> Actually defining this new slots arch event is to do the sanity check
> > >>>>>> for supported arch-events which is enumerated by CPUID.0AH.EBX.
> > >>>>>> Currently vPMU would check if the arch event from guest is supported by
> > >>>>>> KVM. If not, it would be rejected just like intel_hw_event_available()
> > >>>>>> shows.
> > >>>>>>
> > >>>>>> If we don't add the slots event in the intel_arch_events[] array, guest
> > >>>>>> may program the slots event and pass the sanity check of KVM on a
> > >>>>>> platform which actually doesn't support slots event and program the
> > >>>>>> event on a real GP counter and got an invalid count. This is not
> > >>>>>> correct.
> > >>>>> On physical hardware, it is possible to program a GP counter with the
> > >>>>> event selector and unit mask of the slots event whether or not the
> > >>>>> platform supports it. Isn't KVM wrong to disallow something that a
> > >>>>> physical CPU allows?
> > >>>>
> > >>>> Yeah, I agree. But I'm not sure if this is a flaw on PMU driver. If an
> > >>>> event is not supported by the hardware,  we can't predict the PMU's
> > >>>> behavior and a meaningless count may be returned and this could mislead
> > >>>> the user.
> > >>> The user can program any events on the GP counter. The perf doesn't
> > >>> limit it. For the unsupported event, 0 should be returned. Please keep
> > >>> in mind, the event list keeps updating. If the kernel checks for each
> > >>> event, it could be a disaster. I don't think it's a flaw.
> > >>
> > >>
> > >> Thanks Kan, it would be ok as long as 0 is always returned for
> > >> unsupported events. IMO, it's a nice to have feature that KVM does this
> > >> sanity check for supported arch events, it won't break anything.
> > >
> > > The hardware PMU most assuredly does not return 0 for unsupported events.
> > >
> > > For example, if I use host perf to sample event selector 0xa4 unit
> > > mask 1 on a Broadwell host (406f1), I get...
> >
> > I think we have different understanding about the meaning of the
> > "unsupported". There is no enumeration of the Architectural Topdown
> > Slots, which only means the Topdown Slots/01a4 is not an architectural
> > event on the platform. It doesn't mean that the event encoding is
> > unsupported. It could be used by another event, especially on the
> > previous platform.
> 
> If the same event encoding could be used by a microarchitectural event
> on a prior platform, then it is *definitely* wrong for KVM to refuse
> to monitor the event just because it isn't enumerated as a supported
> architectural event.

+1000!  Thanks Kan, this is exactly the info we need!

I'll add a patch to build on "Always treat Fixed counters as available when
supported"[*] and rip out intel_hw_event_available().

[*] https://lore.kernel.org/all/20231024002633.2540714-4-seanjc@google.com

[Patch 2/2] KVM: x86/pmu: Support PMU fixed counter 3

[Dapeng Mi]

The TopDown slots event can be enabled on gp counter or fixed counter 3
and it does not differ from other fixed counters in terms of the use of
count and sampling modes (except for the hardware logic for event
accumulation).

According to commit 6017608936c1 ("perf/x86/intel: Add Icelake
support"), KVM or any perf in-kernel user needs to reprogram fixed
counter 3 via the kernel-defined TopDown slots event for real fixed
counter 3 on the host.

Co-developed-by: Like Xu <likexu@tencent.com>
Signed-off-by: Like Xu <likexu@tencent.com>
Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
---
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/vmx/pmu_intel.c    | 10 ++++++++++
 arch/x86/kvm/x86.c              |  4 ++--
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index d7036982332e..44b47950491a 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -517,7 +517,7 @@ struct kvm_pmc {
 #define KVM_INTEL_PMC_MAX_GENERIC	8
 #define MSR_ARCH_PERFMON_PERFCTR_MAX	(MSR_ARCH_PERFMON_PERFCTR0 + KVM_INTEL_PMC_MAX_GENERIC - 1)
 #define MSR_ARCH_PERFMON_EVENTSEL_MAX	(MSR_ARCH_PERFMON_EVENTSEL0 + KVM_INTEL_PMC_MAX_GENERIC - 1)
-#define KVM_PMC_MAX_FIXED	3
+#define KVM_PMC_MAX_FIXED		4
 #define MSR_ARCH_PERFMON_FIXED_CTR_MAX	(MSR_ARCH_PERFMON_FIXED_CTR0 + KVM_PMC_MAX_FIXED - 1)
 #define KVM_AMD_PMC_MAX_GENERIC	6
 struct kvm_pmu {
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index e32353f1143f..d5af64b1ef69 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -45,6 +45,14 @@ enum intel_pmu_architectural_events {
 	 * core crystal clock or the bus clock (yeah, "architectural").
 	 */
 	PSEUDO_ARCH_REFERENCE_CYCLES = NR_REAL_INTEL_ARCH_EVENTS,
+	/*
+	 * Pseudo-architectural event used to implement IA32_FIXED_CTR3, a.k.a.
+	 * topDown slots. The topdown slots event counts the total number of
+	 * available slots for an unhalted logical processor. The topdwon slots
+	 * event with PERF_METRICS MSR together provides support for topdown
+	 * micro-architecture analysis method.
+	 */
+	PSEUDO_ARCH_TOPDOWN_SLOTS,
 	NR_INTEL_ARCH_EVENTS,
 };
 
@@ -61,6 +69,7 @@ static struct {
 	[INTEL_ARCH_BRANCHES_MISPREDICTED]	= { 0xc5, 0x00 },
 	[INTEL_ARCH_TOPDOWN_SLOTS]		= { 0xa4, 0x01 },
 	[PSEUDO_ARCH_REFERENCE_CYCLES]		= { 0x00, 0x03 },
+	[PSEUDO_ARCH_TOPDOWN_SLOTS]		= { 0x00, 0x04 },
 };
 
 /* mapping between fixed pmc index and intel_arch_events array */
@@ -68,6 +77,7 @@ static int fixed_pmc_events[] = {
 	[0] = INTEL_ARCH_INSTRUCTIONS_RETIRED,
 	[1] = INTEL_ARCH_CPU_CYCLES,
 	[2] = PSEUDO_ARCH_REFERENCE_CYCLES,
+	[3] = PSEUDO_ARCH_TOPDOWN_SLOTS,
 };
 
 static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2c924075f6f1..90c60b6899a5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1468,7 +1468,7 @@ static const u32 msrs_to_save_base[] = {
 
 static const u32 msrs_to_save_pmu[] = {
 	MSR_ARCH_PERFMON_FIXED_CTR0, MSR_ARCH_PERFMON_FIXED_CTR1,
-	MSR_ARCH_PERFMON_FIXED_CTR0 + 2,
+	MSR_ARCH_PERFMON_FIXED_CTR2, MSR_ARCH_PERFMON_FIXED_CTR3,
 	MSR_CORE_PERF_FIXED_CTR_CTRL, MSR_CORE_PERF_GLOBAL_STATUS,
 	MSR_CORE_PERF_GLOBAL_CTRL, MSR_CORE_PERF_GLOBAL_OVF_CTRL,
 	MSR_IA32_PEBS_ENABLE, MSR_IA32_DS_AREA, MSR_PEBS_DATA_CFG,
@@ -7318,7 +7318,7 @@ static void kvm_init_msr_lists(void)
 {
 	unsigned i;
 
-	BUILD_BUG_ON_MSG(KVM_PMC_MAX_FIXED != 3,
+	BUILD_BUG_ON_MSG(KVM_PMC_MAX_FIXED != 4,
 			 "Please update the fixed PMCs in msrs_to_save_pmu[]");
 
 	num_msrs_to_save = 0;
-- 
2.34.1