[5/5] KVM: x86/pmu: Hide guest counter updates from the VMRUN instruction

  From: Like Xu <likexu@tencent.com>

When AMD guest is counting (branch) instructions event, its vPMU should
first subtract one for any relevant (branch)-instructions enabled counter
(when it precedes VMRUN and cannot be preempted) to offset the inevitable
plus-one effect of the VMRUN instruction immediately follows.

Based on a number of micro observations (also the reason why x86_64/
pmu_event_filter_test fails on AMD Zen platforms), each VMRUN will
increment all hw-(branch)-instructions counters by 1, even if they are
only enabled for guest code. This issue seriously affects the performance
understanding of guest developers based on (branch) instruction events.

If the current physical register value on the hardware is ~0x0, it triggers
an overflow in the guest world right after running VMRUN. Although this
cannot be avoided on mainstream released hardware, the resulting PMI
(if configured) will not be incorrectly injected into the guest by vPMU,
since the delayed injection mechanism for a normal counter overflow
depends only on the change of pmc->counter values.

The pmu_hide_vmrun() is called before each VMRUN and its overhead
depends on the number of counters enabled by the guest and is negligible
when none of the counters are used.

Cc: Ravi Bangoria <ravi.bangoria@amd.com>
Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/include/asm/kvm_host.h |  4 ++++
 arch/x86/kvm/pmu.c              | 18 ++++++++++++++++++
 arch/x86/kvm/pmu.h              | 24 +++++++++++++++++++++++-
 arch/x86/kvm/svm/pmu.c          |  1 +
 arch/x86/kvm/svm/svm.c          | 28 ++++++++++++++++++++++++++++
 5 files changed, 74 insertions(+), 1 deletion(-)
  

On Fri, Mar 10, 2023, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> When AMD guest is counting (branch) instructions event, its vPMU should
> first subtract one for any relevant (branch)-instructions enabled counter
> (when it precedes VMRUN and cannot be preempted) to offset the inevitable
> plus-one effect of the VMRUN instruction immediately follows.
> 
> Based on a number of micro observations (also the reason why x86_64/
> pmu_event_filter_test fails on AMD Zen platforms), each VMRUN will
> increment all hw-(branch)-instructions counters by 1, even if they are
> only enabled for guest code. This issue seriously affects the performance
> understanding of guest developers based on (branch) instruction events.
> 
> If the current physical register value on the hardware is ~0x0, it triggers
> an overflow in the guest world right after running VMRUN. Although this
> cannot be avoided on mainstream released hardware, the resulting PMI
> (if configured) will not be incorrectly injected into the guest by vPMU,
> since the delayed injection mechanism for a normal counter overflow
> depends only on the change of pmc->counter values.

IIUC, this is saying that KVM may get a spurious PMI, but otherwise nothing bad
will happen?

> +static inline bool event_is_branch_instruction(struct kvm_pmc *pmc)
> +{
> +	return eventsel_match_perf_hw_id(pmc, PERF_COUNT_HW_INSTRUCTIONS) ||
> +		eventsel_match_perf_hw_id(pmc,
> +					  PERF_COUNT_HW_BRANCH_INSTRUCTIONS);
> +}
> +
> +static inline bool quirky_pmc_will_count_vmrun(struct kvm_pmc *pmc)
> +{
> +	return event_is_branch_instruction(pmc) && event_is_allowed(pmc) &&
> +		!static_call(kvm_x86_get_cpl)(pmc->vcpu);

Wait, really?  VMRUN is counted if and only if it enters to a CPL0 guest?  Can
someone from AMD confirm this?  I was going to say we should just treat this as
"normal" behavior, but counting CPL0 but not CPL>0 is definitely quirky.
  

On 7/4/2023 10:18 am, Sean Christopherson wrote:
> On Fri, Mar 10, 2023, Like Xu wrote:
>> From: Like Xu <likexu@tencent.com>
>>
>> When AMD guest is counting (branch) instructions event, its vPMU should
>> first subtract one for any relevant (branch)-instructions enabled counter
>> (when it precedes VMRUN and cannot be preempted) to offset the inevitable
>> plus-one effect of the VMRUN instruction immediately follows.
>>
>> Based on a number of micro observations (also the reason why x86_64/
>> pmu_event_filter_test fails on AMD Zen platforms), each VMRUN will
>> increment all hw-(branch)-instructions counters by 1, even if they are
>> only enabled for guest code. This issue seriously affects the performance
>> understanding of guest developers based on (branch) instruction events.
>>
>> If the current physical register value on the hardware is ~0x0, it triggers
>> an overflow in the guest world right after running VMRUN. Although this
>> cannot be avoided on mainstream released hardware, the resulting PMI
>> (if configured) will not be incorrectly injected into the guest by vPMU,
>> since the delayed injection mechanism for a normal counter overflow
>> depends only on the change of pmc->counter values.
> 
> IIUC, this is saying that KVM may get a spurious PMI, but otherwise nothing bad
> will happen?

Guests will have nothing to lose, except gaining vPMI accuracy under this proposal.

When a host gets an overflow interrupt caused by a VMRUN, it forwards it to KVM.
KVM does not inject it into the VM, but discards it. For those using PMU to 
profiling
the hypervisor itself, they lose an interrupt or a sample on VMRUN context.

> 
>> +static inline bool event_is_branch_instruction(struct kvm_pmc *pmc)
>> +{
>> +	return eventsel_match_perf_hw_id(pmc, PERF_COUNT_HW_INSTRUCTIONS) ||
>> +		eventsel_match_perf_hw_id(pmc,
>> +					  PERF_COUNT_HW_BRANCH_INSTRUCTIONS);
>> +}
>> +
>> +static inline bool quirky_pmc_will_count_vmrun(struct kvm_pmc *pmc)
>> +{
>> +	return event_is_branch_instruction(pmc) && event_is_allowed(pmc) &&
>> +		!static_call(kvm_x86_get_cpl)(pmc->vcpu);
> 
> Wait, really?  VMRUN is counted if and only if it enters to a CPL0 guest?  Can
> someone from AMD confirm this?  I was going to say we should just treat this as
> "normal" behavior, but counting CPL0 but not CPL>0 is definitely quirky.

VMRUN is only counted on a CPL0-target (branch) instruction counter. The VMRUN
is not expected to be counted by the guest counters, regardless of the guest CPL.

This issue makes a guest CPL0-target instruction counter inexplicably increase, 
as if it
would have been under-counted before the virtualization instructions were counted.

Treating the host hypervisor instructions like VMRUN as guest workload instructions
is already an error in itself not "normal" behavior that affects guest accuracy.
  

On Fri, Apr 07, 2023, Like Xu wrote:
> On 7/4/2023 10:18 am, Sean Christopherson wrote:
> > Wait, really?  VMRUN is counted if and only if it enters to a CPL0 guest?  Can
> > someone from AMD confirm this?  I was going to say we should just treat this as
> > "normal" behavior, but counting CPL0 but not CPL>0 is definitely quirky.
> 
> VMRUN is only counted on a CPL0-target (branch) instruction counter.

Yes or no question: if KVM does VMRUN and a PMC is programmed to count _all_ taken
branches, will the PMC count VMRUN as a branch if guest CPL>0 according to the VMCB?

> This issue makes a guest CPL0-target instruction counter inexplicably
> increase, as if it would have been under-counted before the virtualization
> instructions were counted.

Heh, it's very much explicable, it's just not desirable, and you and I would argue
that it's also incorrect.

AMD folks, are there plans to document this as an erratum?  I agree with Like that
counting VMRUN as a taken branch in guest context is a CPU bug, even if the behavior
is known/expected.
  

On 7/4/2023 10:56 pm, Sean Christopherson wrote:
> On Fri, Apr 07, 2023, Like Xu wrote:
>> On 7/4/2023 10:18 am, Sean Christopherson wrote:
>>> Wait, really?  VMRUN is counted if and only if it enters to a CPL0 guest?  Can
>>> someone from AMD confirm this?  I was going to say we should just treat this as
>>> "normal" behavior, but counting CPL0 but not CPL>0 is definitely quirky.
>>
>> VMRUN is only counted on a CPL0-target (branch) instruction counter.
> 
> Yes or no question: if KVM does VMRUN and a PMC is programmed to count _all_ taken
> branches, will the PMC count VMRUN as a branch if guest CPL>0 according to the VMCB?

YES, my quick tests (based on run_in_user() from KUT on Zen4) show:

EVENTSEL_GUESTONLY + EVENTSEL_ALL + VMRUN_to_USR -> AMD_ZEN_BR_RETIRED + 1
EVENTSEL_GUESTONLY + EVENTSEL_ALL + VMRUN_to_OS -> AMD_ZEN_BR_RETIRED + 1

EVENTSEL_GUESTONLY + EVENTSEL_USR + VMRUN_to_USR -> AMD_ZEN_BR_RETIRED + 1
EVENTSEL_GUESTONLY + EVENTSEL_OS + VMRUN_to_OS -> AMD_ZEN_BR_RETIRED + 1

VENTSEL_GUESTONLY + EVENTSEL_OS + VMRUN_to_USR -> No change
VENTSEL_GUESTONLY + EVENTSEL_USR + VMRUN_to_OS -> No change

I'm actually not surprised and related test would be posted later.

> 
>> This issue makes a guest CPL0-target instruction counter inexplicably
>> increase, as if it would have been under-counted before the virtualization
>> instructions were counted.
> 
> Heh, it's very much explicable, it's just not desirable, and you and I would argue
> that it's also incorrect.

This is completely inaccurate from the end guest pmu user's perspective.

I have a toy that looks like virtio-pmu, through which guest users can get 
hypervisor performance data.
But the side effect of letting the guest see the VMRUN instruction by default is 
unacceptable, isn't it ?

> 
> AMD folks, are there plans to document this as an erratum?  I agree with Like that
> counting VMRUN as a taken branch in guest context is a CPU bug, even if the behavior
> is known/expected.

+CC: Santosh, Tom, Ananth
  

Hi Sean, Like,

On 4/19/2023 7:11 PM, Like Xu wrote:
> On 7/4/2023 10:56 pm, Sean Christopherson wrote:
>> On Fri, Apr 07, 2023, Like Xu wrote:
>>> On 7/4/2023 10:18 am, Sean Christopherson wrote:
>>>> Wait, really?  VMRUN is counted if and only if it enters to a CPL0 guest?  Can
>>>> someone from AMD confirm this?  I was going to say we should just treat this as
>>>> "normal" behavior, but counting CPL0 but not CPL>0 is definitely quirky.
>>>
>>> VMRUN is only counted on a CPL0-target (branch) instruction counter.
>>
>> Yes or no question: if KVM does VMRUN and a PMC is programmed to count _all_ taken
>> branches, will the PMC count VMRUN as a branch if guest CPL>0 according to the VMCB?
> 
> YES, my quick tests (based on run_in_user() from KUT on Zen4) show:
> 
> EVENTSEL_GUESTONLY + EVENTSEL_ALL + VMRUN_to_USR -> AMD_ZEN_BR_RETIRED + 1
> EVENTSEL_GUESTONLY + EVENTSEL_ALL + VMRUN_to_OS -> AMD_ZEN_BR_RETIRED + 1
> 
> EVENTSEL_GUESTONLY + EVENTSEL_USR + VMRUN_to_USR -> AMD_ZEN_BR_RETIRED + 1
> EVENTSEL_GUESTONLY + EVENTSEL_OS + VMRUN_to_OS -> AMD_ZEN_BR_RETIRED + 1
> 
> VENTSEL_GUESTONLY + EVENTSEL_OS + VMRUN_to_USR -> No change
> VENTSEL_GUESTONLY + EVENTSEL_USR + VMRUN_to_OS -> No change
> 
> I'm actually not surprised and related test would be posted later.
> 
>>
>>> This issue makes a guest CPL0-target instruction counter inexplicably
>>> increase, as if it would have been under-counted before the virtualization
>>> instructions were counted.
>>
>> Heh, it's very much explicable, it's just not desirable, and you and I would argue
>> that it's also incorrect.
> 
> This is completely inaccurate from the end guest pmu user's perspective.
> 
> I have a toy that looks like virtio-pmu, through which guest users can get hypervisor performance data.
> But the side effect of letting the guest see the VMRUN instruction by default is unacceptable, isn't it ?
> 
>>
>> AMD folks, are there plans to document this as an erratum?  I agree with Like that
>> counting VMRUN as a taken branch in guest context is a CPU bug, even if the behavior
>> is known/expected.
> 

This behaviour is architectural and an erratum will not be issued. However, for clarity, a future
release of the APM will include additional details like the following:

  1) From the perspective of performance monitoring counters, VMRUNs are considered as far control
     transfers and VMEXITs as exceptions.

  2) When the performance monitoring counters are set up to count events only in certain modes
     through the "OsUserMode" and "HostGuestOnly" bits, instructions and events that change the
     mode are counted in the target mode. For example, a SYSCALL from CPL 3 to CPL 0 with a
     counter set to count retired instructions with USR=1 and OS=0 will not cause an increment of
     the counter. However, the SYSRET back from CPL 0 to CPL 3 will cause an increment of the
     counter and the total count will end up correct. Similarly, when counting PMCx0C6 (retired
     far control transfers, including exceptions and interrupts) with Guest=1 and Host=0, a VMRUN
     instruction will cause an increment of the counter. However, the subsequent VMEXIT that occurs,
     since the target is in the host, will not cause an increment of the counter and so the total
     count will end up correct.
  

On 26/4/2023 1:25 pm, Sandipan Das wrote:
> Hi Sean, Like,
> 
> On 4/19/2023 7:11 PM, Like Xu wrote:
>> On 7/4/2023 10:56 pm, Sean Christopherson wrote:
>>> On Fri, Apr 07, 2023, Like Xu wrote:
>>>> On 7/4/2023 10:18 am, Sean Christopherson wrote:
>>>>> Wait, really?  VMRUN is counted if and only if it enters to a CPL0 guest?  Can
>>>>> someone from AMD confirm this?  I was going to say we should just treat this as
>>>>> "normal" behavior, but counting CPL0 but not CPL>0 is definitely quirky.
>>>>
>>>> VMRUN is only counted on a CPL0-target (branch) instruction counter.
>>>
>>> Yes or no question: if KVM does VMRUN and a PMC is programmed to count _all_ taken
>>> branches, will the PMC count VMRUN as a branch if guest CPL>0 according to the VMCB?
>>
>> YES, my quick tests (based on run_in_user() from KUT on Zen4) show:
>>
>> EVENTSEL_GUESTONLY + EVENTSEL_ALL + VMRUN_to_USR -> AMD_ZEN_BR_RETIRED + 1
>> EVENTSEL_GUESTONLY + EVENTSEL_ALL + VMRUN_to_OS -> AMD_ZEN_BR_RETIRED + 1
>>
>> EVENTSEL_GUESTONLY + EVENTSEL_USR + VMRUN_to_USR -> AMD_ZEN_BR_RETIRED + 1
>> EVENTSEL_GUESTONLY + EVENTSEL_OS + VMRUN_to_OS -> AMD_ZEN_BR_RETIRED + 1
>>
>> VENTSEL_GUESTONLY + EVENTSEL_OS + VMRUN_to_USR -> No change
>> VENTSEL_GUESTONLY + EVENTSEL_USR + VMRUN_to_OS -> No change
>>
>> I'm actually not surprised and related test would be posted later.
>>
>>>
>>>> This issue makes a guest CPL0-target instruction counter inexplicably
>>>> increase, as if it would have been under-counted before the virtualization
>>>> instructions were counted.
>>>
>>> Heh, it's very much explicable, it's just not desirable, and you and I would argue
>>> that it's also incorrect.
>>
>> This is completely inaccurate from the end guest pmu user's perspective.
>>
>> I have a toy that looks like virtio-pmu, through which guest users can get hypervisor performance data.
>> But the side effect of letting the guest see the VMRUN instruction by default is unacceptable, isn't it ?
>>
>>>
>>> AMD folks, are there plans to document this as an erratum?  I agree with Like that
>>> counting VMRUN as a taken branch in guest context is a CPU bug, even if the behavior
>>> is known/expected.
>>
> 
> This behaviour is architectural and an erratum will not be issued. However, for clarity, a future
> release of the APM will include additional details like the following:
> 
>    1) From the perspective of performance monitoring counters, VMRUNs are considered as far control
>       transfers and VMEXITs as exceptions.
> 
>    2) When the performance monitoring counters are set up to count events only in certain modes
>       through the "OsUserMode" and "HostGuestOnly" bits, instructions and events that change the
>       mode are counted in the target mode. For example, a SYSCALL from CPL 3 to CPL 0 with a
>       counter set to count retired instructions with USR=1 and OS=0 will not cause an increment of
>       the counter. However, the SYSRET back from CPL 0 to CPL 3 will cause an increment of the
>       counter and the total count will end up correct. Similarly, when counting PMCx0C6 (retired
>       far control transfers, including exceptions and interrupts) with Guest=1 and Host=0, a VMRUN
>       instruction will cause an increment of the counter. However, the subsequent VMEXIT that occurs,
>       since the target is in the host, will not cause an increment of the counter and so the total
>       count will end up correct.
> 

Thanks for the clarification, that fits my understanding.

"Calculated in target mode" and "correct total count" are architectural choices,
which is not a problem if the consumers of PMU data are on the same side.

But for a VM user, seeing SYSRET in the user mode is completely and functionally
different from seeing VMRUN in the guest context. Since the host user and
the guest user are two separate pmu data consumers, and they do not aggregate
or share the so-called "total" PMU data.

This situation is even worse for nested SVM guests and SEV-SNP guests.

I'm not urging that AMD hardware should change, but it is entirely necessary
for our software layer to take this step, as it is part of the hypervisor's 
responsibility
to hide itself by default.
  

On Wed, Apr 26, 2023, Sandipan Das wrote:
> Hi Sean, Like,
> 
> On 4/19/2023 7:11 PM, Like Xu wrote:
> >> Heh, it's very much explicable, it's just not desirable, and you and I would argue
> >> that it's also incorrect.
> > 
> > This is completely inaccurate from the end guest pmu user's perspective.
> > 
> > I have a toy that looks like virtio-pmu, through which guest users can get hypervisor performance data.
> > But the side effect of letting the guest see the VMRUN instruction by default is unacceptable, isn't it ?
> > 
> >>
> >> AMD folks, are there plans to document this as an erratum?� I agree with Like that
> >> counting VMRUN as a taken branch in guest context is a CPU bug, even if the behavior
> >> is known/expected.
> > 
> 
> This behaviour is architectural and an erratum will not be issued. However, for clarity, a future
> release of the APM will include additional details like the following:
> 
>   1) From the perspective of performance monitoring counters, VMRUNs are considered as far control
>      transfers and VMEXITs as exceptions.
> 
>   2) When the performance monitoring counters are set up to count events only in certain modes
>      through the "OsUserMode" and "HostGuestOnly" bits, instructions and events that change the
>      mode are counted in the target mode. For example, a SYSCALL from CPL 3 to CPL 0 with a
>      counter set to count retired instructions with USR=1 and OS=0 will not cause an increment of
>      the counter. However, the SYSRET back from CPL 0 to CPL 3 will cause an increment of the
>      counter and the total count will end up correct. Similarly, when counting PMCx0C6 (retired
>      far control transfers, including exceptions and interrupts) with Guest=1 and Host=0, a VMRUN
>      instruction will cause an increment of the counter. However, the subsequent VMEXIT that occurs,
>      since the target is in the host, will not cause an increment of the counter and so the total
>      count will end up correct.

The count from the guest's perspective does not "end up correct".  Unlike SYSCALL,
where _userspace_ deliberately and synchronously executes a branch instruction,
VMEXIT and VMRUN are supposed to be transparent to the guest and can be completely
asynchronous with respect to guest code execution, e.g. if the host is spamming
IRQs, the guest will see a potentially large number of bogus (from it's perspective)
branches retired.
  

On Wed, May 24, 2023 at 1:41 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Apr 26, 2023, Sandipan Das wrote:
> > Hi Sean, Like,
> >
> > On 4/19/2023 7:11 PM, Like Xu wrote:
> > >> Heh, it's very much explicable, it's just not desirable, and you and I would argue
> > >> that it's also incorrect.
> > >
> > > This is completely inaccurate from the end guest pmu user's perspective.
> > >
> > > I have a toy that looks like virtio-pmu, through which guest users can get hypervisor performance data.
> > > But the side effect of letting the guest see the VMRUN instruction by default is unacceptable, isn't it ?
> > >
> > >>
> > >> AMD folks, are there plans to document this as an erratum?� I agree with Like that
> > >> counting VMRUN as a taken branch in guest context is a CPU bug, even if the behavior
> > >> is known/expected.
> > >
> >
> > This behaviour is architectural and an erratum will not be issued. However, for clarity, a future
> > release of the APM will include additional details like the following:
> >
> >   1) From the perspective of performance monitoring counters, VMRUNs are considered as far control
> >      transfers and VMEXITs as exceptions.
> >
> >   2) When the performance monitoring counters are set up to count events only in certain modes
> >      through the "OsUserMode" and "HostGuestOnly" bits, instructions and events that change the
> >      mode are counted in the target mode. For example, a SYSCALL from CPL 3 to CPL 0 with a
> >      counter set to count retired instructions with USR=1 and OS=0 will not cause an increment of
> >      the counter. However, the SYSRET back from CPL 0 to CPL 3 will cause an increment of the
> >      counter and the total count will end up correct. Similarly, when counting PMCx0C6 (retired
> >      far control transfers, including exceptions and interrupts) with Guest=1 and Host=0, a VMRUN
> >      instruction will cause an increment of the counter. However, the subsequent VMEXIT that occurs,
> >      since the target is in the host, will not cause an increment of the counter and so the total
> >      count will end up correct.
>
> The count from the guest's perspective does not "end up correct".  Unlike SYSCALL,
> where _userspace_ deliberately and synchronously executes a branch instruction,
> VMEXIT and VMRUN are supposed to be transparent to the guest and can be completely
> asynchronous with respect to guest code execution, e.g. if the host is spamming
> IRQs, the guest will see a potentially large number of bogus (from it's perspective)
> branches retired.

The reverse problem occurs when a PMC is configured to count "CPUID
instructions retired." Since KVM intercepts CPUID and emulates it, the
PMC will always read 0, even if the guest executes a tight loop of
CPUID instructions.

The PMU is not virtualizable on AMD CPUs without significant
hypervisor corrections. I have to wonder if it's really worth the
effort.
  

On Fri, Mar 10, 2023, Like Xu wrote:
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index adb92fc4d7c9..d6fcbf233cb3 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -561,6 +561,10 @@ struct kvm_pmu {
>  	 */
>  	u64 host_cross_mapped_mask;
>  
> +	/* Flags to track any HW quirks that need to be fixed by vPMU. */
> +	u64 quirk_flags;
> +	DECLARE_BITMAP(hide_vmrun_pmc_idx, X86_PMC_IDX_MAX);

Since it sounds like AMD isn't changing the behavior, let's forego the quirk and
just hardcode the fixup.

> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index 2a0504732966..315dca021d57 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -254,6 +254,7 @@ static void pmc_pause_counter(struct kvm_pmc *pmc)
>  	counter += perf_event_pause(pmc->perf_event, true);
>  	pmc->counter = counter & pmc_bitmask(pmc);
>  	pmc->is_paused = true;
> +	kvm_mark_pmc_is_quirky(pmc);
>  }
>  
>  static bool pmc_resume_counter(struct kvm_pmc *pmc)
> @@ -822,6 +823,19 @@ int kvm_vm_ioctl_set_pmu_event_filter(struct kvm *kvm, void __user *argp)
>  	return r;
>  }
>  
> +static inline bool event_is_branch_instruction(struct kvm_pmc *pmc)

How about pmc_is_counting_branches()?  The "event" itself isn't a branch
instruction.

> +{
> +	return eventsel_match_perf_hw_id(pmc, PERF_COUNT_HW_INSTRUCTIONS) ||
> +		eventsel_match_perf_hw_id(pmc,
> +					  PERF_COUNT_HW_BRANCH_INSTRUCTIONS);

Let this poke out.

> +}
> +
> +static inline bool quirky_pmc_will_count_vmrun(struct kvm_pmc *pmc)
> +{
> +	return event_is_branch_instruction(pmc) && event_is_allowed(pmc) &&
> +		!static_call(kvm_x86_get_cpl)(pmc->vcpu);
> +}
> +
>  void kvm_pmu_handle_event(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> @@ -837,6 +851,10 @@ void kvm_pmu_handle_event(struct kvm_vcpu *vcpu)
>  
>  		reprogram_counter(pmc);
>  		kvm_pmu_handle_pmc_overflow(pmc);
> +
> +		if (vcpu_has_pmu_quirks(vcpu) &&
> +		    quirky_pmc_will_count_vmrun(pmc))
> +			set_bit(pmc->idx, pmu->hide_vmrun_pmc_idx);

Doesn't this need to adjust the count _before_ handling overflow?  I.e. isn't it
possible for the bogus counts to cause bogus overflow?

> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> index a47b579667c6..30f6f58f4c38 100644
> --- a/arch/x86/kvm/pmu.h
> +++ b/arch/x86/kvm/pmu.h
> @@ -18,6 +18,9 @@
>  #define VMWARE_BACKDOOR_PMC_REAL_TIME		0x10001
>  #define VMWARE_BACKDOOR_PMC_APPARENT_TIME	0x10002
>  
> +#define X86_PMU_COUNT_VMRUN	BIT_ULL(0)
> +#define X86_PMU_QUIRKS_MASK	X86_PMU_COUNT_VMRUN
> +
>  struct kvm_pmu_ops {
>  	bool (*hw_event_available)(struct kvm_pmc *pmc);
>  	bool (*pmc_is_enabled)(struct kvm_pmc *pmc);
> @@ -54,14 +57,33 @@ static inline void kvm_pmu_request_counter_reprogram(struct kvm_pmc *pmc)
>  	kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
>  }
>  
> +static inline bool vcpu_has_pmu_quirks(struct kvm_vcpu *vcpu)
> +{
> +	return vcpu_to_pmu(vcpu)->quirk_flags & X86_PMU_QUIRKS_MASK;
> +}
> +
> +/*
> + * The time to mark pmc is when the accumulation value returned
> + * by perf API based on a HW counter has just taken effect.
> + */
> +static inline void kvm_mark_pmc_is_quirky(struct kvm_pmc *pmc)
> +{
> +	if (!vcpu_has_pmu_quirks(pmc->vcpu))
> +		return;
> +
> +	kvm_pmu_request_counter_reprogram(pmc);
> +}
> +
>  static inline u64 pmc_read_counter(struct kvm_pmc *pmc)
>  {
>  	u64 counter, enabled, running;
>  
>  	counter = pmc->counter;
> -	if (pmc->perf_event && !pmc->is_paused)
> +	if (pmc->perf_event && !pmc->is_paused) {
>  		counter += perf_event_read_value(pmc->perf_event,
>  						 &enabled, &running);
> +		kvm_mark_pmc_is_quirky(pmc);
> +	}
>  	/* FIXME: Scaling needed? */
>  	return counter & pmc_bitmask(pmc);
>  }
> diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
> index 5fa939e411d8..130991a97f22 100644
> --- a/arch/x86/kvm/svm/pmu.c
> +++ b/arch/x86/kvm/svm/pmu.c
> @@ -187,6 +187,7 @@ static void amd_pmu_refresh(struct kvm_vcpu *vcpu)
>  	pmu->nr_arch_fixed_counters = 0;
>  	pmu->global_status = 0;
>  	bitmap_set(pmu->all_valid_pmc_idx, 0, pmu->nr_arch_gp_counters);
> +	pmu->quirk_flags |= X86_PMU_COUNT_VMRUN;
>  }
>  
>  static void amd_pmu_init(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index f41d96e638ef..f6b33d172481 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3919,6 +3919,31 @@ static fastpath_t svm_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
>  	return EXIT_FASTPATH_NONE;
>  }
>  
> +static void pmu_hide_vmrun(struct kvm_vcpu *vcpu)

This needs to be noinstr.

> +{
> +	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> +	struct kvm_pmc *pmc;
> +	unsigned int i;
> +
> +	for_each_set_bit(i, pmu->hide_vmrun_pmc_idx, X86_PMC_IDX_MAX) {
> +		clear_bit(i, pmu->hide_vmrun_pmc_idx);

Clearing the bit will hide only the first VMRUN after the guest attempts to read
the counter, no?  The fixup needs to apply to every VMRUN that is executed after
the PMC is programmed.  Or am I misreading the patch?

> +
> +		/* AMD doesn't have fixed counters at now. */
> +		if (i >= pmu->nr_arch_gp_counters)
> +			continue;
> +
> +		/*
> +		 * The prerequisite for fixing HW quirks is that there is indeed
> +		 * HW working and perf has no chance to retrieve the counter.

I don't follow the "perf has no chance to retrieve the counter" part.

> +		 */
> +		pmc = &pmu->gp_counters[i];
> +		if (!pmc->perf_event || pmc->perf_event->hw.idx < 0)
> +			continue;
> +
> +		pmc->counter--;
> +	}
> +}
> +
>  static noinstr void svm_vcpu_enter_exit(struct kvm_vcpu *vcpu, bool spec_ctrl_intercepted)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
> @@ -3986,6 +4011,9 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
>  
>  	kvm_wait_lapic_expire(vcpu);
>  
> +	if (vcpu->kvm->arch.enable_pmu && vcpu_has_pmu_quirks(vcpu))
> +		pmu_hide_vmrun(vcpu);
> +
>  	/*
>  	 * If this vCPU has touched SPEC_CTRL, restore the guest's value if
>  	 * it's non-zero. Since vmentry is serialising on affected CPUs, there
> -- 
> 2.39.2
>
  

On Wed, May 24, 2023, Jim Mattson wrote:
> On Wed, May 24, 2023 at 1:41 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Wed, Apr 26, 2023, Sandipan Das wrote:
> > > Hi Sean, Like,
> > >
> > > On 4/19/2023 7:11 PM, Like Xu wrote:
> > > >> Heh, it's very much explicable, it's just not desirable, and you and I would argue
> > > >> that it's also incorrect.
> > > >
> > > > This is completely inaccurate from the end guest pmu user's perspective.
> > > >
> > > > I have a toy that looks like virtio-pmu, through which guest users can get hypervisor performance data.
> > > > But the side effect of letting the guest see the VMRUN instruction by default is unacceptable, isn't it ?
> > > >
> > > >>
> > > >> AMD folks, are there plans to document this as an erratum?� I agree with Like that
> > > >> counting VMRUN as a taken branch in guest context is a CPU bug, even if the behavior
> > > >> is known/expected.
> > > >
> > >
> > > This behaviour is architectural and an erratum will not be issued. However, for clarity, a future
> > > release of the APM will include additional details like the following:
> > >
> > >   1) From the perspective of performance monitoring counters, VMRUNs are considered as far control
> > >      transfers and VMEXITs as exceptions.
> > >
> > >   2) When the performance monitoring counters are set up to count events only in certain modes
> > >      through the "OsUserMode" and "HostGuestOnly" bits, instructions and events that change the
> > >      mode are counted in the target mode. For example, a SYSCALL from CPL 3 to CPL 0 with a
> > >      counter set to count retired instructions with USR=1 and OS=0 will not cause an increment of
> > >      the counter. However, the SYSRET back from CPL 0 to CPL 3 will cause an increment of the
> > >      counter and the total count will end up correct. Similarly, when counting PMCx0C6 (retired
> > >      far control transfers, including exceptions and interrupts) with Guest=1 and Host=0, a VMRUN
> > >      instruction will cause an increment of the counter. However, the subsequent VMEXIT that occurs,
> > >      since the target is in the host, will not cause an increment of the counter and so the total
> > >      count will end up correct.
> >
> > The count from the guest's perspective does not "end up correct".  Unlike SYSCALL,
> > where _userspace_ deliberately and synchronously executes a branch instruction,
> > VMEXIT and VMRUN are supposed to be transparent to the guest and can be completely
> > asynchronous with respect to guest code execution, e.g. if the host is spamming
> > IRQs, the guest will see a potentially large number of bogus (from it's perspective)
> > branches retired.
> 
> The reverse problem occurs when a PMC is configured to count "CPUID
> instructions retired." Since KVM intercepts CPUID and emulates it, the
> PMC will always read 0, even if the guest executes a tight loop of
> CPUID instructions.
>
> The PMU is not virtualizable on AMD CPUs without significant
> hypervisor corrections. I have to wonder if it's really worth the
> effort.

Per our offlist chat, my understanding is that there are caveats with vPMUs that
it's simply not feasible for a hypervisor to handle.  I.e. virtualizing any x86
PMU with 100% accuracy isn't happening anytime soon.

The way forward is likely to evaluate each caveat on a case-by-case basis to
determine whether or not the cost of the fixup in KVM is worth the benefit to
the guest.  E.g. emulating "CPUID instructions retired" seems like it would be
fairly straightforward.  AFAICT, fixing up the VMRUN stuff is quite difficult though.
  

On Wed, May 24, 2023 at 2:23 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Mar 10, 2023, Like Xu wrote:
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index adb92fc4d7c9..d6fcbf233cb3 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -561,6 +561,10 @@ struct kvm_pmu {
> >        */
> >       u64 host_cross_mapped_mask;
> >
> > +     /* Flags to track any HW quirks that need to be fixed by vPMU. */
> > +     u64 quirk_flags;
> > +     DECLARE_BITMAP(hide_vmrun_pmc_idx, X86_PMC_IDX_MAX);
>
> Since it sounds like AMD isn't changing the behavior, let's forego the quirk and
> just hardcode the fixup.
>
> > diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> > index 2a0504732966..315dca021d57 100644
> > --- a/arch/x86/kvm/pmu.c
> > +++ b/arch/x86/kvm/pmu.c
> > @@ -254,6 +254,7 @@ static void pmc_pause_counter(struct kvm_pmc *pmc)
> >       counter += perf_event_pause(pmc->perf_event, true);
> >       pmc->counter = counter & pmc_bitmask(pmc);
> >       pmc->is_paused = true;
> > +     kvm_mark_pmc_is_quirky(pmc);
> >  }
> >
> >  static bool pmc_resume_counter(struct kvm_pmc *pmc)
> > @@ -822,6 +823,19 @@ int kvm_vm_ioctl_set_pmu_event_filter(struct kvm *kvm, void __user *argp)
> >       return r;
> >  }
> >
> > +static inline bool event_is_branch_instruction(struct kvm_pmc *pmc)
>
> How about pmc_is_counting_branches()?  The "event" itself isn't a branch
> instruction.

Note that there's a bug in the original code for this that has
probably never been fixed: it ignores CMASK and INV in the PerfEvtSel.

> > +{
> > +     return eventsel_match_perf_hw_id(pmc, PERF_COUNT_HW_INSTRUCTIONS) ||
> > +             eventsel_match_perf_hw_id(pmc,
> > +                                       PERF_COUNT_HW_BRANCH_INSTRUCTIONS);
>
> Let this poke out.
>
> > +}
> > +
> > +static inline bool quirky_pmc_will_count_vmrun(struct kvm_pmc *pmc)
> > +{
> > +     return event_is_branch_instruction(pmc) && event_is_allowed(pmc) &&
> > +             !static_call(kvm_x86_get_cpl)(pmc->vcpu);
> > +}
> > +
> >  void kvm_pmu_handle_event(struct kvm_vcpu *vcpu)
> >  {
> >       struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> > @@ -837,6 +851,10 @@ void kvm_pmu_handle_event(struct kvm_vcpu *vcpu)
> >
> >               reprogram_counter(pmc);
> >               kvm_pmu_handle_pmc_overflow(pmc);
> > +
> > +             if (vcpu_has_pmu_quirks(vcpu) &&
> > +                 quirky_pmc_will_count_vmrun(pmc))
> > +                     set_bit(pmc->idx, pmu->hide_vmrun_pmc_idx);
>
> Doesn't this need to adjust the count _before_ handling overflow?  I.e. isn't it
> possible for the bogus counts to cause bogus overflow?
>
> > diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> > index a47b579667c6..30f6f58f4c38 100644
> > --- a/arch/x86/kvm/pmu.h
> > +++ b/arch/x86/kvm/pmu.h
> > @@ -18,6 +18,9 @@
> >  #define VMWARE_BACKDOOR_PMC_REAL_TIME                0x10001
> >  #define VMWARE_BACKDOOR_PMC_APPARENT_TIME    0x10002
> >
> > +#define X86_PMU_COUNT_VMRUN  BIT_ULL(0)
> > +#define X86_PMU_QUIRKS_MASK  X86_PMU_COUNT_VMRUN
> > +
> >  struct kvm_pmu_ops {
> >       bool (*hw_event_available)(struct kvm_pmc *pmc);
> >       bool (*pmc_is_enabled)(struct kvm_pmc *pmc);
> > @@ -54,14 +57,33 @@ static inline void kvm_pmu_request_counter_reprogram(struct kvm_pmc *pmc)
> >       kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
> >  }
> >
> > +static inline bool vcpu_has_pmu_quirks(struct kvm_vcpu *vcpu)
> > +{
> > +     return vcpu_to_pmu(vcpu)->quirk_flags & X86_PMU_QUIRKS_MASK;
> > +}
> > +
> > +/*
> > + * The time to mark pmc is when the accumulation value returned
> > + * by perf API based on a HW counter has just taken effect.
> > + */
> > +static inline void kvm_mark_pmc_is_quirky(struct kvm_pmc *pmc)
> > +{
> > +     if (!vcpu_has_pmu_quirks(pmc->vcpu))
> > +             return;
> > +
> > +     kvm_pmu_request_counter_reprogram(pmc);
> > +}
> > +
> >  static inline u64 pmc_read_counter(struct kvm_pmc *pmc)
> >  {
> >       u64 counter, enabled, running;
> >
> >       counter = pmc->counter;
> > -     if (pmc->perf_event && !pmc->is_paused)
> > +     if (pmc->perf_event && !pmc->is_paused) {
> >               counter += perf_event_read_value(pmc->perf_event,
> >                                                &enabled, &running);
> > +             kvm_mark_pmc_is_quirky(pmc);
> > +     }
> >       /* FIXME: Scaling needed? */
> >       return counter & pmc_bitmask(pmc);
> >  }
> > diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c
> > index 5fa939e411d8..130991a97f22 100644
> > --- a/arch/x86/kvm/svm/pmu.c
> > +++ b/arch/x86/kvm/svm/pmu.c
> > @@ -187,6 +187,7 @@ static void amd_pmu_refresh(struct kvm_vcpu *vcpu)
> >       pmu->nr_arch_fixed_counters = 0;
> >       pmu->global_status = 0;
> >       bitmap_set(pmu->all_valid_pmc_idx, 0, pmu->nr_arch_gp_counters);
> > +     pmu->quirk_flags |= X86_PMU_COUNT_VMRUN;
> >  }
> >
> >  static void amd_pmu_init(struct kvm_vcpu *vcpu)
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index f41d96e638ef..f6b33d172481 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -3919,6 +3919,31 @@ static fastpath_t svm_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
> >       return EXIT_FASTPATH_NONE;
> >  }
> >
> > +static void pmu_hide_vmrun(struct kvm_vcpu *vcpu)
>
> This needs to be noinstr.
>
> > +{
> > +     struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> > +     struct kvm_pmc *pmc;
> > +     unsigned int i;
> > +
> > +     for_each_set_bit(i, pmu->hide_vmrun_pmc_idx, X86_PMC_IDX_MAX) {
> > +             clear_bit(i, pmu->hide_vmrun_pmc_idx);
>
> Clearing the bit will hide only the first VMRUN after the guest attempts to read
> the counter, no?  The fixup needs to apply to every VMRUN that is executed after
> the PMC is programmed.  Or am I misreading the patch?
>
> > +
> > +             /* AMD doesn't have fixed counters at now. */
> > +             if (i >= pmu->nr_arch_gp_counters)
> > +                     continue;
> > +
> > +             /*
> > +              * The prerequisite for fixing HW quirks is that there is indeed
> > +              * HW working and perf has no chance to retrieve the counter.
>
> I don't follow the "perf has no chance to retrieve the counter" part.
>
> > +              */
> > +             pmc = &pmu->gp_counters[i];
> > +             if (!pmc->perf_event || pmc->perf_event->hw.idx < 0)
> > +                     continue;
> > +
> > +             pmc->counter--;
> > +     }
> > +}
> > +
> >  static noinstr void svm_vcpu_enter_exit(struct kvm_vcpu *vcpu, bool spec_ctrl_intercepted)
> >  {
> >       struct vcpu_svm *svm = to_svm(vcpu);
> > @@ -3986,6 +4011,9 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
> >
> >       kvm_wait_lapic_expire(vcpu);
> >
> > +     if (vcpu->kvm->arch.enable_pmu && vcpu_has_pmu_quirks(vcpu))
> > +             pmu_hide_vmrun(vcpu);
> > +
> >       /*
> >        * If this vCPU has touched SPEC_CTRL, restore the guest's value if
> >        * it's non-zero. Since vmentry is serialising on affected CPUs, there
> > --
> > 2.39.2
> >
  

On Wed, May 24, 2023 at 2:29 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, May 24, 2023, Jim Mattson wrote:
> > On Wed, May 24, 2023 at 1:41 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Wed, Apr 26, 2023, Sandipan Das wrote:
> > > > Hi Sean, Like,
> > > >
> > > > On 4/19/2023 7:11 PM, Like Xu wrote:
> > > > >> Heh, it's very much explicable, it's just not desirable, and you and I would argue
> > > > >> that it's also incorrect.
> > > > >
> > > > > This is completely inaccurate from the end guest pmu user's perspective.
> > > > >
> > > > > I have a toy that looks like virtio-pmu, through which guest users can get hypervisor performance data.
> > > > > But the side effect of letting the guest see the VMRUN instruction by default is unacceptable, isn't it ?
> > > > >
> > > > >>
> > > > >> AMD folks, are there plans to document this as an erratum?� I agree with Like that
> > > > >> counting VMRUN as a taken branch in guest context is a CPU bug, even if the behavior
> > > > >> is known/expected.
> > > > >
> > > >
> > > > This behaviour is architectural and an erratum will not be issued. However, for clarity, a future
> > > > release of the APM will include additional details like the following:
> > > >
> > > >   1) From the perspective of performance monitoring counters, VMRUNs are considered as far control
> > > >      transfers and VMEXITs as exceptions.
> > > >
> > > >   2) When the performance monitoring counters are set up to count events only in certain modes
> > > >      through the "OsUserMode" and "HostGuestOnly" bits, instructions and events that change the
> > > >      mode are counted in the target mode. For example, a SYSCALL from CPL 3 to CPL 0 with a
> > > >      counter set to count retired instructions with USR=1 and OS=0 will not cause an increment of
> > > >      the counter. However, the SYSRET back from CPL 0 to CPL 3 will cause an increment of the
> > > >      counter and the total count will end up correct. Similarly, when counting PMCx0C6 (retired
> > > >      far control transfers, including exceptions and interrupts) with Guest=1 and Host=0, a VMRUN
> > > >      instruction will cause an increment of the counter. However, the subsequent VMEXIT that occurs,
> > > >      since the target is in the host, will not cause an increment of the counter and so the total
> > > >      count will end up correct.
> > >
> > > The count from the guest's perspective does not "end up correct".  Unlike SYSCALL,
> > > where _userspace_ deliberately and synchronously executes a branch instruction,
> > > VMEXIT and VMRUN are supposed to be transparent to the guest and can be completely
> > > asynchronous with respect to guest code execution, e.g. if the host is spamming
> > > IRQs, the guest will see a potentially large number of bogus (from it's perspective)
> > > branches retired.
> >
> > The reverse problem occurs when a PMC is configured to count "CPUID
> > instructions retired." Since KVM intercepts CPUID and emulates it, the
> > PMC will always read 0, even if the guest executes a tight loop of
> > CPUID instructions.
> >
> > The PMU is not virtualizable on AMD CPUs without significant
> > hypervisor corrections. I have to wonder if it's really worth the
> > effort.
>
> Per our offlist chat, my understanding is that there are caveats with vPMUs that
> it's simply not feasible for a hypervisor to handle.  I.e. virtualizing any x86
> PMU with 100% accuracy isn't happening anytime soon.
>
> The way forward is likely to evaluate each caveat on a case-by-case basis to
> determine whether or not the cost of the fixup in KVM is worth the benefit to
> the guest.  E.g. emulating "CPUID instructions retired" seems like it would be
> fairly straightforward.  AFAICT, fixing up the VMRUN stuff is quite difficult though.

Yeah. The problem with fixing up "CPUID instructions retired" is
tracking what the event encoding is for every F/M/S out there. It's
not worth it.
  

On 25/5/2023 5:30 am, Jim Mattson wrote:
> Note that there's a bug in the original code for this that has
> probably never been fixed: it ignores CMASK and INV in the PerfEvtSel.

Regarding the emulation of CMASK, INV and PIN_CONTROL bits on vPMU,
please forgive me for never having the right expectations (so there will be no
correct emulation), an share more *architecture* descriptions of those hw
behavior, and it would be great to have corresponding selftests as a bug report.
  

On 25/5/2023 5:32 am, Jim Mattson wrote:
> On Wed, May 24, 2023 at 2:29 PM Sean Christopherson <seanjc@google.com> wrote:
>>
>> On Wed, May 24, 2023, Jim Mattson wrote:
>>> On Wed, May 24, 2023 at 1:41 PM Sean Christopherson <seanjc@google.com> wrote:
>>>>
>>>> On Wed, Apr 26, 2023, Sandipan Das wrote:
>>>>> Hi Sean, Like,
>>>>>
>>>>> On 4/19/2023 7:11 PM, Like Xu wrote:
>>>>>>> Heh, it's very much explicable, it's just not desirable, and you and I would argue
>>>>>>> that it's also incorrect.
>>>>>>
>>>>>> This is completely inaccurate from the end guest pmu user's perspective.
>>>>>>
>>>>>> I have a toy that looks like virtio-pmu, through which guest users can get hypervisor performance data.
>>>>>> But the side effect of letting the guest see the VMRUN instruction by default is unacceptable, isn't it ?
>>>>>>
>>>>>>>
>>>>>>> AMD folks, are there plans to document this as an erratum?� I agree with Like that
>>>>>>> counting VMRUN as a taken branch in guest context is a CPU bug, even if the behavior
>>>>>>> is known/expected.
>>>>>>
>>>>>
>>>>> This behaviour is architectural and an erratum will not be issued. However, for clarity, a future
>>>>> release of the APM will include additional details like the following:
>>>>>
>>>>>    1) From the perspective of performance monitoring counters, VMRUNs are considered as far control
>>>>>       transfers and VMEXITs as exceptions.
>>>>>
>>>>>    2) When the performance monitoring counters are set up to count events only in certain modes
>>>>>       through the "OsUserMode" and "HostGuestOnly" bits, instructions and events that change the
>>>>>       mode are counted in the target mode. For example, a SYSCALL from CPL 3 to CPL 0 with a
>>>>>       counter set to count retired instructions with USR=1 and OS=0 will not cause an increment of
>>>>>       the counter. However, the SYSRET back from CPL 0 to CPL 3 will cause an increment of the
>>>>>       counter and the total count will end up correct. Similarly, when counting PMCx0C6 (retired
>>>>>       far control transfers, including exceptions and interrupts) with Guest=1 and Host=0, a VMRUN
>>>>>       instruction will cause an increment of the counter. However, the subsequent VMEXIT that occurs,
>>>>>       since the target is in the host, will not cause an increment of the counter and so the total
>>>>>       count will end up correct.
>>>>
>>>> The count from the guest's perspective does not "end up correct".  Unlike SYSCALL,
>>>> where _userspace_ deliberately and synchronously executes a branch instruction,
>>>> VMEXIT and VMRUN are supposed to be transparent to the guest and can be completely
>>>> asynchronous with respect to guest code execution, e.g. if the host is spamming
>>>> IRQs, the guest will see a potentially large number of bogus (from it's perspective)
>>>> branches retired.
>>>
>>> The reverse problem occurs when a PMC is configured to count "CPUID
>>> instructions retired." Since KVM intercepts CPUID and emulates it, the
>>> PMC will always read 0, even if the guest executes a tight loop of
>>> CPUID instructions.

Unlikely. KVM will count any emulated instructions based on kvm_pmu_incr_counter().
Did I miss some conditions ?

>>>
>>> The PMU is not virtualizable on AMD CPUs without significant
>>> hypervisor corrections. I have to wonder if it's really worth the
>>> effort.

I used to think so, until I saw the AMD64_EVENTSEL_GUESTONLY bit.
Hardware architects are expected to put more effort into this area.

>>
>> Per our offlist chat, my understanding is that there are caveats with vPMUs that
>> it's simply not feasible for a hypervisor to handle.  I.e. virtualizing any x86
>> PMU with 100% accuracy isn't happening anytime soon.

Indeed, and any more detailed complaints ?

>>
>> The way forward is likely to evaluate each caveat on a case-by-case basis to
>> determine whether or not the cost of the fixup in KVM is worth the benefit to
>> the guest.  E.g. emulating "CPUID instructions retired" seems like it would be
>> fairly straightforward.  AFAICT, fixing up the VMRUN stuff is quite difficult though.
> 
> Yeah. The problem with fixing up "CPUID instructions retired" is
> tracking what the event encoding is for every F/M/S out there. It's
> not worth it.

I don't think it's feasible to emulate 100% accuracy on Intel. For guest pmu
users, it is motivated by wanting to know how effective they are running on
the current pCPU, and any vPMU eimulation behavior that helps this
understanding would be valuable.
  

On Mon, May 29, 2023 at 7:51 AM Like Xu <like.xu.linux@gmail.com> wrote:
>
> On 25/5/2023 5:32 am, Jim Mattson wrote:
> > On Wed, May 24, 2023 at 2:29 PM Sean Christopherson <seanjc@google.com> wrote:
> >>
> >> On Wed, May 24, 2023, Jim Mattson wrote:
> >>> On Wed, May 24, 2023 at 1:41 PM Sean Christopherson <seanjc@google.com> wrote:
> >>>>
> >>>> On Wed, Apr 26, 2023, Sandipan Das wrote:
> >>>>> Hi Sean, Like,
> >>>>>
> >>>>> On 4/19/2023 7:11 PM, Like Xu wrote:
> >>>>>>> Heh, it's very much explicable, it's just not desirable, and you and I would argue
> >>>>>>> that it's also incorrect.
> >>>>>>
> >>>>>> This is completely inaccurate from the end guest pmu user's perspective.
> >>>>>>
> >>>>>> I have a toy that looks like virtio-pmu, through which guest users can get hypervisor performance data.
> >>>>>> But the side effect of letting the guest see the VMRUN instruction by default is unacceptable, isn't it ?
> >>>>>>
> >>>>>>>
> >>>>>>> AMD folks, are there plans to document this as an erratum?� I agree with Like that
> >>>>>>> counting VMRUN as a taken branch in guest context is a CPU bug, even if the behavior
> >>>>>>> is known/expected.
> >>>>>>
> >>>>>
> >>>>> This behaviour is architectural and an erratum will not be issued. However, for clarity, a future
> >>>>> release of the APM will include additional details like the following:
> >>>>>
> >>>>>    1) From the perspective of performance monitoring counters, VMRUNs are considered as far control
> >>>>>       transfers and VMEXITs as exceptions.
> >>>>>
> >>>>>    2) When the performance monitoring counters are set up to count events only in certain modes
> >>>>>       through the "OsUserMode" and "HostGuestOnly" bits, instructions and events that change the
> >>>>>       mode are counted in the target mode. For example, a SYSCALL from CPL 3 to CPL 0 with a
> >>>>>       counter set to count retired instructions with USR=1 and OS=0 will not cause an increment of
> >>>>>       the counter. However, the SYSRET back from CPL 0 to CPL 3 will cause an increment of the
> >>>>>       counter and the total count will end up correct. Similarly, when counting PMCx0C6 (retired
> >>>>>       far control transfers, including exceptions and interrupts) with Guest=1 and Host=0, a VMRUN
> >>>>>       instruction will cause an increment of the counter. However, the subsequent VMEXIT that occurs,
> >>>>>       since the target is in the host, will not cause an increment of the counter and so the total
> >>>>>       count will end up correct.
> >>>>
> >>>> The count from the guest's perspective does not "end up correct".  Unlike SYSCALL,
> >>>> where _userspace_ deliberately and synchronously executes a branch instruction,
> >>>> VMEXIT and VMRUN are supposed to be transparent to the guest and can be completely
> >>>> asynchronous with respect to guest code execution, e.g. if the host is spamming
> >>>> IRQs, the guest will see a potentially large number of bogus (from it's perspective)
> >>>> branches retired.
> >>>
> >>> The reverse problem occurs when a PMC is configured to count "CPUID
> >>> instructions retired." Since KVM intercepts CPUID and emulates it, the
> >>> PMC will always read 0, even if the guest executes a tight loop of
> >>> CPUID instructions.
>
> Unlikely. KVM will count any emulated instructions based on kvm_pmu_incr_counter().
> Did I miss some conditions ?

That code only increments PMCs configured to count "instructions
retired" and "branch instructions retired." It does not increment PMCs
configured to count "CPUID instructions retired."

> >>>
> >>> The PMU is not virtualizable on AMD CPUs without significant
> >>> hypervisor corrections. I have to wonder if it's really worth the
> >>> effort.
>
> I used to think so, until I saw the AMD64_EVENTSEL_GUESTONLY bit.
> Hardware architects are expected to put more effort into this area.
>
> >>
> >> Per our offlist chat, my understanding is that there are caveats with vPMUs that
> >> it's simply not feasible for a hypervisor to handle.  I.e. virtualizing any x86
> >> PMU with 100% accuracy isn't happening anytime soon.
>
> Indeed, and any more detailed complaints ?

Reference cycles unhalted fails to increment outside of guest mode.

SMIs received counts *physical* rather than virtual SMIs

Interrupts taken counts *physical* rather than virtual interrupts taken.

> >>
> >> The way forward is likely to evaluate each caveat on a case-by-case basis to
> >> determine whether or not the cost of the fixup in KVM is worth the benefit to
> >> the guest.  E.g. emulating "CPUID instructions retired" seems like it would be
> >> fairly straightforward.  AFAICT, fixing up the VMRUN stuff is quite difficult though.
> >
> > Yeah. The problem with fixing up "CPUID instructions retired" is
> > tracking what the event encoding is for every F/M/S out there. It's
> > not worth it.
>
> I don't think it's feasible to emulate 100% accuracy on Intel. For guest pmu
> users, it is motivated by wanting to know how effective they are running on
> the current pCPU, and any vPMU eimulation behavior that helps this
> understanding would be valuable.

But at least Intel has a list of architected events, which are mostly
amenable to virtualization.