linux-kernel - Re: [PATCH 1/2] perf vendor events: Add stepping in CPUID string for x86

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]

Date:   Thu, 15 Nov 2018 09:26:50 -0500
From:   "Liang, Kan" <kan.liang@...ux.intel.com>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     acme@...nel.org, mingo@...hat.com, peterz@...radead.org,
        linux-kernel@...r.kernel.org, namhyung@...nel.org,
        ak@...ux.intel.com
Subject: Re: [PATCH 1/2] perf vendor events: Add stepping in CPUID string for
 x86



On 11/15/2018 8:53 AM, Jiri Olsa wrote:
> On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.liang@...ux.intel.com wrote:
> 
> SNIP
> 
>> diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
>> index fb0d71afee8b..b428a4b00bf7 100644
>> --- a/tools/perf/arch/x86/util/header.c
>> +++ b/tools/perf/arch/x86/util/header.c
>> @@ -4,6 +4,7 @@
>>   #include <stdio.h>
>>   #include <stdlib.h>
>>   #include <string.h>
>> +#include <regex.h>
>>   
>>   #include "../../util/header.h"
>>   
>> @@ -70,9 +71,73 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
>>   {
>>   	char *buf = malloc(128);
>>   
>> -	if (buf && __get_cpuid(buf, 128, "%s-%u-%X$") < 0) {
>> +	if (buf && __get_cpuid(buf, 128, "%s-%u-%X-%X$") < 0) {
>>   		free(buf);
>>   		return NULL;
>>   	}
>>   	return buf;
>>   }
>> +
>> +/* Full CPUID format for x86 is vendor-family-model-stepping */
>> +static bool is_full_cpuid(const char *cpuid)
>> +{
>> +	const char *tmp = cpuid;
>> +	int count = 0;
>> +
>> +	while ((tmp = strchr(tmp, '-')) != NULL) {
>> +		count++;
>> +		tmp++;
>> +	}
>> +
>> +	if (count == 3)
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +int strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
>> +{
>> +	regex_t re;
>> +	regmatch_t pmatch[1];
>> +	int match;
>> +	bool full_mapcpuid = is_full_cpuid(mapcpuid);
>> +	bool full_cpuid = is_full_cpuid(cpuid);
> 
> cpuid will be always full from now right? why do we need to check it?
>

User may set cpuid by environment string "PERF_CPUID", which may not be 
full format.

> also please move this to arch/x86/util/pmu.c
> so it matches the weak function object

Sure.

> 
>> +
>> +	/*
>> +	 * Full CPUID format is required to identify a platform.
>> +	 * Error out if the cpuid string is incomplete.
>> +	 */
>> +	if (full_mapcpuid && !full_cpuid) {
>> +		pr_info("Invalid CPUID %s. Full CPUID is required, "
>> +			"vendor-family-model-stepping\n", cpuid);
>> +		return 1;
>> +	}
>> +
>> +	if (regcomp(&re, mapcpuid, REG_EXTENDED) != 0) {
>> +		/* Warn unable to generate match particular string. */
>> +		pr_info("Invalid regular expression %s\n", mapcpuid);
>> +		return 1;
>> +	}
>> +
>> +	match = !regexec(&re, cpuid, 1, pmatch, 0);
>> +	regfree(&re);
>> +	if (match) {
>> +		size_t match_len = (pmatch[0].rm_eo - pmatch[0].rm_so);
>> +		size_t cpuid_len;
>> +
>> +		/* If the full CPUID format isn't required,
>> +		 * ignoring the stepping.
>> +		 */
>> +		if (!full_mapcpuid && full_cpuid)
>> +			cpuid_len = strrchr(cpuid, '-') - cpuid;
>> +		else
>> +			cpuid_len = strlen(cpuid);
>> +
>> +
>> +		/* Verify the entire string matched. */
>> +		if (match_len == cpuid_len)
>> +			return 0;
> 
> why is this necessary?
> 

It's from previous common code. As my understanding, it just double 
check the matched strings. There is no harmful. So I keep it.

Thanks,
Kan