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

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

[Dongli Si]

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>
---
v4: Remove my run_as_host function for better descriptive.
v3: https://lore.kernel.org/all/20220314042254.1487836-1-sidongli1997@gmail.com/
v2: https://lore.kernel.org/all/20220310183404.1291725-1-sidongli1997@gmail.com/
v1: https://lore.kernel.org/all/20220227132640.3-1-sidongli1997@gmail.com/

 arch/x86/events/amd/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
index 9687a8aef01c..3fafd1e46ada 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 (hypervisor_is_type(X86_HYPER_NATIVE))
+		cpuc->perf_ctr_virt_mask = 0;
 
 	/* Reload all events */
 	amd_pmu_disable_all();
-- 
2.32.0

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

[Peter Zijlstra]

On Sun, Mar 20, 2022 at 08:21:06AM +0800, Dongli Si wrote:

> @@ -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 (hypervisor_is_type(X86_HYPER_NATIVE))
> +		cpuc->perf_ctr_virt_mask = 0;

So I had to go and read the original commit to figure out wth this code
is supposed to be doing. I'm thinking this all can use a wee comment.

Also, the above is very ambiguous as to what it does. Specifically if we
don't set perf_ctr_virt_mask, then what is the actual value (yes, I
know, but its not very clear is it). Also, if we don't in fact change
perf_ctr_virt_mask then these following two lines:

>  
>  	/* Reload all events */
>  	amd_pmu_disable_all();
	amd_pmu_enable_all(0);

are somewhat completely pointless, no?

So can we please get a patch that clarifies things instead of making
things even more obscure?

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

[Dongli Si]

On 24/03/2022 10:42, Peter Zijlstra wrote:
> So I had to go and read the original commit to figure out wth this code
> is supposed to be doing. I'm thinking this all can use a wee comment.

Hi Peter, I will add a wee comment to clarifies things.

> Also, the above is very ambiguous as to what it does. Specifically if we
> don't set perf_ctr_virt_mask, then what is the actual value (yes, I
> know, but its not very clear is it). Also, if we don't in fact change
> perf_ctr_virt_mask then these following two lines:
> 
> >  
> >  	/* Reload all events */
> >  	amd_pmu_disable_all();
> 	amd_pmu_enable_all(0);
> 
> are somewhat completely pointless, no?

Thank you for pointing out the problem with my patch.

> So can we please get a patch that clarifies things instead of making
> things even more obscure?

The following is an improved patch, is it good?

Regards,
Dongli

---
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>
---
v5: Add a wee comment to clarifies things and improve code
v4: https://lore.kernel.org/all/20220320002106.1800166-1-sidongli1997@gmail.com/
v3: https://lore.kernel.org/all/20220314042254.1487836-1-sidongli1997@gmail.com/
v2: https://lore.kernel.org/all/20220310183404.1291725-1-sidongli1997@gmail.com/
v1: https://lore.kernel.org/all/20220227132640.3-1-sidongli1997@gmail.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
-- 
2.32.0

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

[Peter Zijlstra]

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?

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

[Dongli Si]

On 28/03/2022 14:03, Peter Zijlstra wrote:
> 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.

I will add comments to the code to explain 'why'.

> 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.

I think the HO/GO bit can only be set on the host, and should only be set
if SVM is enabled.

When the SVM is disabled, set the HO/GO bit will cause the performance
counters to not work.

Set the HO/GO bit inside the guest will cause the guest emitted
"unchecked MSR access error" warning, can be triggered by running
"perf stat -e instructions:G ls" in the guest, because this will set
the GO bit in the guest, and perf_ctr_virt_mask just mask the HO bit.

My patch does not affect the host, it just fixes the bug in 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.

I don't quite understand your question.

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

I don't think applying this patch will cause L2 guests to be included in
the host counter.

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

I don't think there is such a counter.

Also, I found that the L1 HV will emitted "unchecked MSR access error"
warning when "perf record" is executed on L2,
because the GO bit of L1 HV is set.

I wrote a new patch to mask the HO/GO bit in the guest, I will send it later.

Regards,
Dongli

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

[Peter Zijlstra]

On Fri, Apr 01, 2022 at 04:29:11PM +0800, Dongli Si wrote:
> On 28/03/2022 14:03, Peter Zijlstra wrote:
> > 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.
> 
> I will add comments to the code to explain 'why'.
> 
> > 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.
> 
> I think the HO/GO bit can only be set on the host, and should only be set
> if SVM is enabled.
> 
> When the SVM is disabled, set the HO/GO bit will cause the performance
> counters to not work.
> 
> Set the HO/GO bit inside the guest will cause the guest emitted
> "unchecked MSR access error" warning, can be triggered by running
> "perf stat -e instructions:G ls" in the guest, because this will set
> the GO bit in the guest, and perf_ctr_virt_mask just mask the HO bit.
> 
> My patch does not affect the host, it just fixes the bug in 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.
> 
> I don't quite understand your question.
> 
> > But with this change on, the L1 HV doesn't provide these same semantics,
> > it's guest will be included in that host counter.
> 
> I don't think applying this patch will cause L2 guests to be included in
> the host counter.
> 
> > Or is there additional counter {dis,en}abling on virt enter,exit (resp.)
> > to achieve these semantics?
> 
> I don't think there is such a counter.

If SVM enter/exit don't twiddle with counter EN bits, how is all this
supposed to work consistently then?

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

[Dongli Si]

On Fri, 1 Apr 2022 15:06:27 +0200, Peter Zijlstra wrote:
> If SVM enter/exit don't twiddle with counter EN bits, how is all this
> supposed to work consistently then?

Since KVM currently does not support the "Host/Guest Only" bits (41:40),
CPU wide events created on L1 HV will always count L1 HV and L2, so no
twiddle counter EN bits are needed when {dis,en}abling SVM on L1 HV.

This #GP warning is because KVM does not allow guest to set HO/GO bit,
It has been fixed in this commit 9b026073db2f
("KVM: x86/svm: Clear reserved bits written to PerfEvtSeln MSRs")
and has been merged to 5.18-rc1.

Because the commit df51fe7ea1c1c
("perf/x86/amd: Don't touch the AMD64_EVENTSEL_HOSTONLY bit inside the guest")
is to fix a very similar problem caused by the same reason,
I think it should be revert now, it make things obscure.

And perf_ctr_virt_mask is used to mask Host-Only bit when SVM is disabled,
Using it on a guest doesn't make sense and make things obscure.

I wrote a patch to clarifies what perf_ctr_virt_mask does.

Regards,
Dongli