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

[Liam Merwick]

On 10/03/2022 18:34, Dongli Si wrote:
> From: Dongli Si <sidongli1997@xxxxxxxxx>
>
> With nested virtualization, when the guest hypervisor runs a nested guest
> and if uses "perf record" in an AMD Milan guest hypervisor, the guest
> hypervisor dmesg will reports the following warning message:

I think it might be clearer with L0/L1/L2 terminology. Maybe something 
like the following?

"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, while
> the guest hypervisor performance monitor unit should avoid such use.

"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@xxxxxxxxx>

Tested-by: Liam Merwick <liam.merwick@xxxxxxxxxx>
Reviewed-by: Liam Merwick <liam.merwick@xxxxxxxxxx>


> ---
> v2: Add run_as_host function and improve description
> v1: https://lore.kernel.org/all/20220227132640.3-1-sidongli1997@xxxxxxxxx/
>
>   arch/x86/events/amd/core.c        |  4 +++-
>   arch/x86/include/asm/hypervisor.h | 10 ++++++++++
>   2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> index 9687a8aef01c..14cd079243a4 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"
>   
> @@ -1027,7 +1028,8 @@ void amd_pmu_enable_virt(void)
>   {
>   	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>   
> -	cpuc->perf_ctr_virt_mask = 0;
> +	if (run_as_host())
> +		cpuc->perf_ctr_virt_mask = 0;
>   
>   	/* Reload all events */
>   	amd_pmu_disable_all();
> diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
> index e41cbf2ec41d..fcc66c23cc72 100644
> --- a/arch/x86/include/asm/hypervisor.h
> +++ b/arch/x86/include/asm/hypervisor.h
> @@ -73,11 +73,21 @@ static inline bool hypervisor_is_type(enum x86_hypervisor_type type)
>   {
>   	return x86_hyper_type == type;
>   }
> +
> +static inline bool run_as_host(void)
> +{
> +	return hypervisor_is_type(X86_HYPER_NATIVE);
> +}
>   #else
>   static inline void init_hypervisor_platform(void) { }
>   static inline bool hypervisor_is_type(enum x86_hypervisor_type type)
>   {
>   	return type == X86_HYPER_NATIVE;
>   }
> +
> +static inline bool run_as_host(void)
> +{
> +	return true;
> +}
>   #endif /* CONFIG_HYPERVISOR_GUEST */
>   #endif /* _ASM_X86_HYPERVISOR_H */