Re: [PATCH v4] perf/x86/amd: Don't touch the Host-only bit inside the guest hypervisor

[Peter Zijlstra <peterz@infradead.org>]

On Sun, Mar 27, 2022 at 06:56:03PM +0800, Dongli Si wrote:

> From: Dongli Si <sidongli1997@gmail.com>
> 
> With nested virtualization on AMD Milan, if "perf record" is run in an
> L1 hypervisor with an L2 guest, the following warning is emitted in
> the L1 guest.
> 
> [] unchecked MSR access error: WRMSR to 0xc0010200 (tried to write 0x0000020000510076)
> at rIP: 0xffffffff81003a50 (x86_pmu_enable_all+0x60/0x100)
> [] Call Trace:
> []  <IRQ>
> []  ? x86_pmu_enable+0x146/0x300
> []  __perf_install_in_context+0x150/0x170
> 
> The AMD64_EVENTSEL_HOSTONLY bit is defined and used on the host (L0),
> while the L1 hypervisor Performance Monitor Unit should avoid such use.
> 
> Fixes: 1018faa6cf23 ("perf/x86/kvm: Fix Host-Only/Guest-Only counting with SVM disabled")
> Signed-off-by: Dongli Si <sidongli1997@gmail.com>
> Tested-by: Liam Merwick <liam.merwick@oracle.com>
> Reviewed-by: Liam Merwick <liam.merwick@oracle.com>
> ---
>  arch/x86/events/amd/core.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> index 9687a8aef01c..5a1657c684f0 100644
> --- a/arch/x86/events/amd/core.c
> +++ b/arch/x86/events/amd/core.c
> @@ -8,6 +8,7 @@
>  #include <linux/jiffies.h>
>  #include <asm/apicdef.h>
>  #include <asm/nmi.h>
> +#include <asm/hypervisor.h>
>  
>  #include "../perf_event.h"
>  
> @@ -1023,10 +1024,16 @@ __init int amd_pmu_init(void)
>  	return 0;
>  }
>  
> +/*
> + * Set the Host-only bit when virtualization is enabled on the Host Hypervisor
> + */
>  void amd_pmu_enable_virt(void)
>  {
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>  
> +	if (!hypervisor_is_type(X86_HYPER_NATIVE))
> +		return;
> +
>  	cpuc->perf_ctr_virt_mask = 0;
>  
>  	/* Reload all events */
> @@ -1035,10 +1042,16 @@ void amd_pmu_enable_virt(void)
>  }
>  EXPORT_SYMBOL_GPL(amd_pmu_enable_virt);
>  
> +/*
> + * Mask the Host-only bit when virtualization is disabled on the Host Hypervisor
> + */
>  void amd_pmu_disable_virt(void)
>  {
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>  
> +	if (!hypervisor_is_type(X86_HYPER_NATIVE))
> +		return;
> +
>  	/*
>  	 * We only mask out the Host-only bit so that host-only counting works
>  	 * when SVM is disabled. If someone sets up a guest-only counter when

Better I suppose, but I think the comments can be improved by covering
the 'why' of things. We can all read the code to see the what of it.

Anyway, doesn't this also affect behaviour? I'm guessing this HO bit is
only set by perf-record for events it wants to record on the host. But
by not setting it, we'll also record the activity of the guest.

So suppose we create a CPU wide HO event, then it will only count L0
activity, right? Any L1 (or higher) activite will be invisible.


But with this change on, the L1 HV doesn't provide these same semantics,
it's guest will be included in that host counter.

Or is there additional counter {dis,en}abling on virt enter,exit (resp.)
to achieve these semantics?