x86: events: Do not return bogus capabilities if PMU is broken

[PATCH] x86: events: Do not return bogus capabilities if PMU is broken

Posted by Paolo Bonzini 3 years, 11 months ago

From: Like Xu <likexu@tencent.com>

If the PMU is broken due to firmware issues, check_hw_exists() will return
false but perf_get_x86_pmu_capability() will still return data from x86_pmu.
Likewise if some of the hotplug callbacks cannot be installed the contents
of x86_pmu will not be reverted.

Handle the failure in both cases by clearing x86_pmu if init_hw_perf_events()
or reverts to software events only.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/events/core.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 99cf67d63cf3..d23f3821daf5 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -2095,14 +2095,15 @@ static int __init init_hw_perf_events(void)
 	}
 	if (err != 0) {
 		pr_cont("no PMU driver, software events only.\n");
-		return 0;
+		err = 0;
+		goto out_bad_pmu;
 	}
 
 	pmu_check_apic();
 
 	/* sanity check that the hardware exists or is emulated */
 	if (!check_hw_exists(&pmu, x86_pmu.num_counters, x86_pmu.num_counters_fixed))
-		return 0;
+		goto out_bad_pmu;
 
 	pr_cont("%s PMU driver.\n", x86_pmu.name);
 
@@ -2211,6 +2212,8 @@ static int __init init_hw_perf_events(void)
 	cpuhp_remove_state(CPUHP_AP_PERF_X86_STARTING);
 out:
 	cpuhp_remove_state(CPUHP_PERF_X86_PREPARE);
+out_bad_pmu:
+	memset(&x86_pmu, 0, sizeof(x86_pmu));
 	return err;
 }
 early_initcall(init_hw_perf_events);
@@ -2982,6 +2985,11 @@ unsigned long perf_misc_flags(struct pt_regs *regs)
 
 void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap)
 {
+	if (!x86_pmu.name) {
+		memset(cap, 0, sizeof(*cap));
+		return;
+	}
+
 	cap->version		= x86_pmu.version;
 	/*
 	 * KVM doesn't support the hybrid PMU yet.
-- 
2.31.1

Re: [PATCH] x86: events: Do not return bogus capabilities if PMU is broken

Posted by Peter Zijlstra 3 years, 11 months ago

On Wed, Jun 01, 2022 at 05:42:56AM -0400, Paolo Bonzini wrote:
> From: Like Xu <likexu@tencent.com>
> 
> If the PMU is broken due to firmware issues, check_hw_exists() will return
> false but perf_get_x86_pmu_capability() will still return data from x86_pmu.
> Likewise if some of the hotplug callbacks cannot be installed the contents
> of x86_pmu will not be reverted.
> 
> Handle the failure in both cases by clearing x86_pmu if init_hw_perf_events()
> or reverts to software events only.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

No SoB from Like,

> ---
>  arch/x86/events/core.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 99cf67d63cf3..d23f3821daf5 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2095,14 +2095,15 @@ static int __init init_hw_perf_events(void)
>  	}
>  	if (err != 0) {
>  		pr_cont("no PMU driver, software events only.\n");
> -		return 0;
> +		err = 0;
> +		goto out_bad_pmu;
>  	}
>  
>  	pmu_check_apic();
>  
>  	/* sanity check that the hardware exists or is emulated */
>  	if (!check_hw_exists(&pmu, x86_pmu.num_counters, x86_pmu.num_counters_fixed))
> -		return 0;
> +		goto out_bad_pmu;
>  
>  	pr_cont("%s PMU driver.\n", x86_pmu.name);
>  
> @@ -2211,6 +2212,8 @@ static int __init init_hw_perf_events(void)
>  	cpuhp_remove_state(CPUHP_AP_PERF_X86_STARTING);
>  out:
>  	cpuhp_remove_state(CPUHP_PERF_X86_PREPARE);
> +out_bad_pmu:
> +	memset(&x86_pmu, 0, sizeof(x86_pmu));
>  	return err;
>  }
>  early_initcall(init_hw_perf_events);
> @@ -2982,6 +2985,11 @@ unsigned long perf_misc_flags(struct pt_regs *regs)
>  
>  void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap)
>  {
> +	if (!x86_pmu.name) {

We have x86_pmu_initialized(), the implementation is a bit daft, but
might as well use it here too, no?

> +		memset(cap, 0, sizeof(*cap));
> +		return;
> +	}
> +
>  	cap->version		= x86_pmu.version;
>  	/*
>  	 * KVM doesn't support the hybrid PMU yet.

Otherwise seems fine I suppose.

Re: [PATCH] x86: events: Do not return bogus capabilities if PMU is broken

Posted by Paolo Bonzini 3 years, 11 months ago

On 6/1/22 13:07, Peter Zijlstra wrote:
> On Wed, Jun 01, 2022 at 05:42:56AM -0400, Paolo Bonzini wrote:
>> From: Like Xu <likexu@tencent.com>
>>
>> If the PMU is broken due to firmware issues, check_hw_exists() will return
>> false but perf_get_x86_pmu_capability() will still return data from x86_pmu.
>> Likewise if some of the hotplug callbacks cannot be installed the contents
>> of x86_pmu will not be reverted.
>>
>> Handle the failure in both cases by clearing x86_pmu if init_hw_perf_events()
>> or reverts to software events only.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> No SoB from Like,

And hardly any code from him either except for a single memset, :) but 
I'll add

	Signed-off-by: Like Xu <likexu@tencent.com>
	Co-developed-by: Like Xu <likexu@tencent.com>

(I have hooks that notice this when pushing, but they don't help with 
git-send-email...).


>> @@ -2982,6 +2985,11 @@ unsigned long perf_misc_flags(struct pt_regs *regs)
>>   
>>   void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap)
>>   {
>> +	if (!x86_pmu.name) {
> 
> We have x86_pmu_initialized(), the implementation is a bit daft, but
> might as well use it here too, no?

Yes, thanks.

Paolo