perf/x86/amd: cpu_hw_events::perf_ctr_virt_mask should only be used on host

[PATCH] perf/x86/amd: cpu_hw_events::perf_ctr_virt_mask should only be used on host

Posted by Dongli Si 4 years, 2 months ago

From: Dongli Si <sidongli1997@gmail.com>

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.

Revert commit df51fe7ea1c1c
("perf/x86/amd: Don't touch the AMD64_EVENTSEL_HOSTONLY bit inside the guest"),
Because it make things a little obscure and this #GP has been fixed in KVM.

Fixes: 1018faa6cf23 ("perf/x86/kvm: Fix Host-Only/Guest-Only counting with SVM disabled")
Signed-off-by: Dongli Si <sidongli1997@gmail.com>
---
 arch/x86/events/amd/core.c   | 19 ++++++++++++++++++-
 arch/x86/events/perf_event.h |  3 +--
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
index 9687a8aef01c..5ac7d9410d36 100644
--- a/arch/x86/events/amd/core.c
+++ b/arch/x86/events/amd/core.c
@@ -533,7 +533,12 @@ static void amd_pmu_cpu_starting(int cpu)
 	struct amd_nb *nb;
 	int i, nb_id;
 
-	cpuc->perf_ctr_virt_mask = AMD64_EVENTSEL_HOSTONLY;
+	/*
+	 * When SVM is disabled, set the Host-Only bit will cause the
+	 * performance counter to not work.
+	 */
+	if (!boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		cpuc->perf_ctr_virt_mask = AMD64_EVENTSEL_HOSTONLY;
 
 	if (!x86_pmu.amd_nb_constraints)
 		return;
@@ -1023,10 +1028,16 @@ __init int amd_pmu_init(void)
 	return 0;
 }
 
+/*
+ * Unmask the Host-only bit when SVM is enabled on the Host Hypervisor
+ */
 void amd_pmu_enable_virt(void)
 {
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		return;
+
 	cpuc->perf_ctr_virt_mask = 0;
 
 	/* Reload all events */
@@ -1035,10 +1046,16 @@ void amd_pmu_enable_virt(void)
 }
 EXPORT_SYMBOL_GPL(amd_pmu_enable_virt);
 
+/*
+ * Mask the Host-only bit when SVM is disabled on the Host Hypervisor
+ */
 void amd_pmu_disable_virt(void)
 {
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		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
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 150261d929b9..fa1428ca60b6 100644
--- a/arch/x86/events/perf_event.h
+++ b/arch/x86/events/perf_event.h
@@ -1138,10 +1138,9 @@ void x86_pmu_stop(struct perf_event *event, int flags);
 
 static inline void x86_pmu_disable_event(struct perf_event *event)
 {
-	u64 disable_mask = __this_cpu_read(cpu_hw_events.perf_ctr_virt_mask);
 	struct hw_perf_event *hwc = &event->hw;
 
-	wrmsrl(hwc->config_base, hwc->config & ~disable_mask);
+	wrmsrl(hwc->config_base, hwc->config);
 
 	if (is_counter_pair(hwc))
 		wrmsrl(x86_pmu_config_addr(hwc->idx + 1), 0);
-- 
2.32.0

Re: [PATCH] perf/x86/amd: cpu_hw_events::perf_ctr_virt_mask should only be used on host

Posted by Like Xu 4 years, 2 months ago

Dongli,

On 11/4/2022 9:46 pm, Dongli Si wrote:
> From: Dongli Si <sidongli1997@gmail.com>
> 
> 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.

Or you can work it out to make nested vPMU functional on AMD.

> 
> Revert commit df51fe7ea1c1c
> ("perf/x86/amd: Don't touch the AMD64_EVENTSEL_HOSTONLY bit inside the guest"),

This is not a typical revert commit.

> Because it make things a little obscure and this #GP has been fixed in KVM.

Please check the chronological order of the related commits and the motivations.

> 
> Fixes: 1018faa6cf23 ("perf/x86/kvm: Fix Host-Only/Guest-Only counting with SVM disabled")
> Signed-off-by: Dongli Si <sidongli1997@gmail.com>
> ---
>   arch/x86/events/amd/core.c   | 19 ++++++++++++++++++-
>   arch/x86/events/perf_event.h |  3 +--
>   2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> index 9687a8aef01c..5ac7d9410d36 100644
> --- a/arch/x86/events/amd/core.c
> +++ b/arch/x86/events/amd/core.c
> @@ -533,7 +533,12 @@ static void amd_pmu_cpu_starting(int cpu)
>   	struct amd_nb *nb;
>   	int i, nb_id;
>   
> -	cpuc->perf_ctr_virt_mask = AMD64_EVENTSEL_HOSTONLY;
> +	/*
> +	 * When SVM is disabled, set the Host-Only bit will cause the
> +	 * performance counter to not work.

It's ridiculous. Based on the AMD APM Table 13-3. Host/Guest Only Bits,
the performance counter would count "Host events" rather than "not work".

Note, your proposal change should work on the L0, L1 and L2.

> +	 */
> +	if (!boot_cpu_has(X86_FEATURE_HYPERVISOR))
> +		cpuc->perf_ctr_virt_mask = AMD64_EVENTSEL_HOSTONLY;
>   
>   	if (!x86_pmu.amd_nb_constraints)
>   		return;
> @@ -1023,10 +1028,16 @@ __init int amd_pmu_init(void)
>   	return 0;
>   }
>   
> +/*
> + * Unmask the Host-only bit when SVM is enabled on the Host Hypervisor
> + */
>   void amd_pmu_enable_virt(void)
>   {
>   	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>   
> +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
> +		return;
> +
>   	cpuc->perf_ctr_virt_mask = 0;
>   
>   	/* Reload all events */
> @@ -1035,10 +1046,16 @@ void amd_pmu_enable_virt(void)
>   }
>   EXPORT_SYMBOL_GPL(amd_pmu_enable_virt);
>   
> +/*
> + * Mask the Host-only bit when SVM is disabled on the Host Hypervisor
> + */
>   void amd_pmu_disable_virt(void)
>   {
>   	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>   
> +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
> +		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
> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> index 150261d929b9..fa1428ca60b6 100644
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -1138,10 +1138,9 @@ void x86_pmu_stop(struct perf_event *event, int flags);
>   
>   static inline void x86_pmu_disable_event(struct perf_event *event)
>   {
> -	u64 disable_mask = __this_cpu_read(cpu_hw_events.perf_ctr_virt_mask);
>   	struct hw_perf_event *hwc = &event->hw;
>   
> -	wrmsrl(hwc->config_base, hwc->config & ~disable_mask);
> +	wrmsrl(hwc->config_base, hwc->config);
>   
>   	if (is_counter_pair(hwc))
>   		wrmsrl(x86_pmu_config_addr(hwc->idx + 1), 0);

Re: [PATCH] perf/x86/amd: cpu_hw_events::perf_ctr_virt_mask should only be used on host

Posted by Dongli Si 4 years, 2 months ago

On Mon 11 Apr 2022 22:29:18 +0800, Like Xu wrote:
> Or you can work it out to make nested vPMU functional on AMD.
Unless in the future kvm wants to emulate on L1 HV the behavior of not
count when HO bit is set and SVM is disabled, otherwise it doesn't make
sense to use perf_ctr_virt_mask in the guest to mask HO bit. At least for
now, this patch helps clarify what perf_ctr_virt_mask actually does.

> This is not a typical revert commit.
Thanks for pointing out the problem with my patch,
I will write another patch specifically to revert this commit.

> Please check the chronological order of the related commits and the motivations.
I know that commit df51fe7ea1c1c fixed the problem of use vPMU on old KVM,
but I think it's a speculative way and make things a little obscure,
because this #GP is actually a KVM problem rather than a guest problem,
I think it is the user's responsibility to update their host kernel.

> > +	/*
> > +	 * When SVM is disabled, set the Host-Only bit will cause the
> > +	 * performance counter to not work.
> It's ridiculous. Based on the AMD APM Table 13-3. Host/Guest Only Bits,
> the performance counter would count "Host events" rather than "not work".
You are wrong, you can test it on the host, and the description of the
commit 1018faa6cf23 also pointed out this problem, this is the result of an
experiment, AMD APM has not documented this problem.

I forgot to say this is the behavior on the host, I will improve this
comment to specify 'why' more clearly, like this:
	/*
	 * It turns out that when SVM is disabled on the host (L0), set the
	 * Host-Only bit will cause the performance counter to not count.
	 */

> Note, your proposal change should work on the L0, L1 and L2.
Yes, I tested it on L0, L1, L2 with 5.18-rc1 and it works as expected.

There is a related discussion here:
https://lore.kernel.org/all/20220320002106.1800166-1-sidongli1997@gmail.com/

Regards,
Dongli

Re: [PATCH] perf/x86/amd: cpu_hw_events::perf_ctr_virt_mask should only be used on host

Posted by Like Xu 4 years, 2 months ago

On 12/4/2022 9:25 am, Dongli Si wrote:
> On Mon 11 Apr 2022 22:29:18 +0800, Like Xu wrote:
>> Or you can work it out to make nested vPMU functional on AMD.
> Unless in the future kvm wants to emulate on L1 HV the behavior of not
> count when HO bit is set and SVM is disabled, otherwise it doesn't make

In fact, I would prefer that we make a little effort to enable nested vPMU.

> sense to use perf_ctr_virt_mask in the guest to mask HO bit. At least for
> now, this patch helps clarify what perf_ctr_virt_mask actually does.

This has been clarified by the commit 1018faa6cf23.

> 
>> This is not a typical revert commit.
> Thanks for pointing out the problem with my patch,
> I will write another patch specifically to revert this commit.

The indispensable commit df51fe7ea1c1 is concerned with the symmetric use of
'disable_mask' in both __x86_pmu_enable_event() and x86_pmu_disable_event().

I had this false assumption at that time, and we'd better support HOST, GUEST}ONLY
bits in the L1 for L2 guest, and if not, it's bug. Please help. :D

> 
>> Please check the chronological order of the related commits and the motivations.
> I know that commit df51fe7ea1c1c fixed the problem of use vPMU on old KVM,
> but I think it's a speculative way and make things a little obscure,
> because this #GP is actually a KVM problem rather than a guest problem,

And we have 9b026073db2f to fix KVM for older guest kernel.

> I think it is the user's responsibility to update their host kernel.
> 
>>> +	/*
>>> +	 * When SVM is disabled, set the Host-Only bit will cause the
>>> +	 * performance counter to not work.
>> It's ridiculous. Based on the AMD APM Table 13-3. Host/Guest Only Bits,
>> the performance counter would count "Host events" rather than "not work".
> You are wrong, you can test it on the host, and the description of the
> commit 1018faa6cf23 also pointed out this problem, this is the result of an
> experiment, AMD APM has not documented this problem.

I have to say it's true on a ZEN3 host after a quick experiment.

> 
> I forgot to say this is the behavior on the host, I will improve this
> comment to specify 'why' more clearly, like this:
> 	/*
> 	 * It turns out that when SVM is disabled on the host (L0), set the

Again, we need the semantics to hold true on L1.

> 	 * Host-Only bit will cause the performance counter to not count.
> 	 */
> 
>> Note, your proposal change should work on the L0, L1 and L2.
> Yes, I tested it on L0, L1, L2 with 5.18-rc1 and it works as expected.

Specifically for L1, we need rely on the EFER[SVME] check instead of the
meaningless "boot_cpu_has(X86_FEATURE_HYPERVISOR)".

> 
> There is a related discussion here:
> https://lore.kernel.org/all/20220320002106.1800166-1-sidongli1997@gmail.com/
> 
> Regards,
> Dongli
> 
>