linux-kernel - Re: [PATCHv2 1/1] perf/x86/intel: make error messages less confusing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180904182312.GA11911@u40b0340c692b58f6553c.ant.amazon.com>
Date:   Tue, 4 Sep 2018 11:23:12 -0700
From:   Eduardo Valentin <eduval@...zon.com>
To:     Eduardo Valentin <eduval@...zon.com>
CC:     Andi Kleen <ak@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, <x86@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        "Jia Zhang" <qianyue.zj@...baba-inc.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv2 1/1] perf/x86/intel: make error messages less confusing

Hello,

On Thu, Aug 23, 2018 at 08:07:32AM -0700, Eduardo Valentin wrote:
> On a system with X86_FEATURE_ARCH_PERFMON disabled
> and with a model not known by family PMU drivers,
> user gets a kernel message log like the following:
> [ 0.100114] Performance Events: unsupported p6 CPU model 85 no PMU driver, software events only.
> 
> The "unsupported .. CPU" part may be confusing for some
> users leading to wrong understanding that the kernel
> does not support the CPU model.
> 
> This patch rewords the messages on the failure path to:
> [ 0.667154] Performance Events: CPU does not support PMU: no PMU driver, software events only.
> 


Gentle ping on this patch, any further comments?

> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: "H. Peter Anvin" <hpa@...or.com>
> Cc: x86@...nel.org
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Andi Kleen <ak@...ux.intel.com>
> Cc: Kan Liang <kan.liang@...ux.intel.com>
> Cc: Dan Carpenter <dan.carpenter@...cle.com>
> Cc: Eduardo Valentin <eduval@...zon.com>
> Cc: Jia Zhang <qianyue.zj@...baba-inc.com>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: linux-kernel@...r.kernel.org
> Signed-off-by: Eduardo Valentin <eduval@...zon.com>
> ---
> 
> Changes from V1->V2:
> - As per initial review, the propose messaging was even
> more confusing. Simplified it by only saying that
> the CPU does not support PMU.
> 
> 
>  arch/x86/events/intel/core.c | 15 +++++++++++----
>  arch/x86/events/intel/p4.c   |  5 +----
>  arch/x86/events/intel/p6.c   |  1 -
>  3 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 035c37481f57..2ddb97f03f4a 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -3889,15 +3889,22 @@ __init int intel_pmu_init(void)
>  	char *name;
>  
>  	if (!cpu_has(&boot_cpu_data, X86_FEATURE_ARCH_PERFMON)) {
> +		int ret = -ENODEV;
> +
>  		switch (boot_cpu_data.x86) {
>  		case 0x6:
> -			return p6_pmu_init();
> +			ret = p6_pmu_init();
> +			break;
>  		case 0xb:
> -			return knc_pmu_init();
> +			ret = knc_pmu_init();
> +			break;
>  		case 0xf:
> -			return p4_pmu_init();
> +			ret = p4_pmu_init();
> +			break;
>  		}
> -		return -ENODEV;
> +		if (ret)
> +			pr_cont("CPU does not support PMU: ");
> +		return ret;
>  	}
>  
>  	/*
> diff --git a/arch/x86/events/intel/p4.c b/arch/x86/events/intel/p4.c
> index d32c0eed38ca..fb5e8576d9ac 100644
> --- a/arch/x86/events/intel/p4.c
> +++ b/arch/x86/events/intel/p4.c
> @@ -1345,11 +1345,8 @@ __init int p4_pmu_init(void)
>  	BUILD_BUG_ON(ARCH_P4_MAX_CCCR > INTEL_PMC_MAX_GENERIC);
>  
>  	rdmsr(MSR_IA32_MISC_ENABLE, low, high);
> -	if (!(low & (1 << 7))) {
> -		pr_cont("unsupported Netburst CPU model %d ",
> -			boot_cpu_data.x86_model);
> +	if (!(low & (1 << 7)))
>  		return -ENODEV;
> -	}
>  
>  	memcpy(hw_cache_event_ids, p4_hw_cache_event_ids,
>  		sizeof(hw_cache_event_ids));
> diff --git a/arch/x86/events/intel/p6.c b/arch/x86/events/intel/p6.c
> index 408879b0c0d4..e8e03e68b22f 100644
> --- a/arch/x86/events/intel/p6.c
> +++ b/arch/x86/events/intel/p6.c
> @@ -269,7 +269,6 @@ __init int p6_pmu_init(void)
>  		break;
>  
>  	default:
> -		pr_cont("unsupported p6 CPU model %d ", boot_cpu_data.x86_model);
>  		return -ENODEV;
>  	}
>  
> -- 
> 2.18.0
> 

-- 
All the best,
Eduardo Valentin