linux-kernel - Re: [PATCH v4 2/4] x86/cpufeatures: Generate a feature mask header based on build config

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: <8744fbf0-46b9-4df9-951c-4f1ddafd7b36@zytor.com>
Date: Wed, 3 Jul 2024 02:23:08 -0700
From: Xin Li <xin@...or.com>
To: Nikolay Borisov <nik.borisov@...e.com>, linux-kernel@...r.kernel.org,
        linux-perf-users@...r.kernel.org
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
        will@...nel.org, peterz@...radead.org, akpm@...ux-foundation.org,
        acme@...nel.org, namhyung@...nel.org, brgerst@...il.com,
        andrew.cooper3@...rix.com
Subject: Re: [PATCH v4 2/4] x86/cpufeatures: Generate a feature mask header
 based on build config

On 7/1/2024 3:44 AM, Nikolay Borisov wrote:
> 
> 
> On 28.06.24 г. 20:45 ч., Xin Li (Intel) wrote:

> Overall it's better I still have some minor comments which can be 
> addressed only if you are going to make another submission.

I can send a v4A for this patch.

> 
>> diff --git a/arch/x86/tools/featuremasks.awk 
>> b/arch/x86/tools/featuremasks.awk
>> new file mode 100755
>> index 000000000000..351d5a4c7d64
>> --- /dev/null
>> +++ b/arch/x86/tools/featuremasks.awk
>> @@ -0,0 +1,85 @@
>> +#!/usr/bin/awk
>> +#
>> +# Convert cpufeatures.h to a list of compile-time masks
>> +# Note: this blithly assumes that each word has at least one
>> +# feature defined in it; if not, something else is wrong!
>> +#
>> +
>> +BEGIN {
>> +    printf "#ifndef _ASM_X86_FEATUREMASKS_H\n";
>> +    printf "#define _ASM_X86_FEATUREMASKS_H\n\n";
>> +
>> +    file = 0
>> +}
>> +
>> +BEGINFILE {
>> +    switch (++file) {
>> +    case 1:            # cpufeatures.h
>> +        FPAT = "#[ \t]*[a-z]+|[A-Za-z0-9_]+|[^ \t]";
>> +        break;
>> +    case 2:            # .config
>> +        FPAT = "CONFIG_[A-Z0-9_]+|is not set|[yn]";
>> +        break;
>> +    }
>> +}
>> +
>> +# Create a dictionary of sorts, containing all defined feature bits
>> +file == 1 && $1 ~ /^#[ \t]*define$/ && $2 ~ /^X86_FEATURE_/ &&
>> +$3 == "(" && $5 == "*" && $7 == "+" && $9 == ")" {
>> +    nfeat = $4 * $6 + $8;
>> +    feat = $2;
>> +    sub(/^X86_FEATURE_/, "", feat);
>> +    feats[nfeat] = feat;
>> +}
>> +file == 1 && $1 ~ /^#[ \t]*define$/ && $2 == "NCAPINTS" {
>> +    ncapints = strtonum($3);
>> +}
>> +
>> +# Create a dictionary featstat[REQUIRED|DISABLED, FEATURE_NAME] = on 
>> | off
>> +file == 2 && $1 ~ /^CONFIG_X86_[A-Z]*_FEATURE_/ {
>> +    on = ($2 == "y");
>> +    if (split($1, fs, "CONFIG_X86_|_FEATURE_") == 3)
>> +        featstat[fs[2], fs[3]] = on;
> 
> nit: 'on' here will either be 0 or 1 (awk doesn't have a separate 
> boolean type).

No change proposed here, right?

> 
>> +}
>> +
>> +END {
>> +    sets[1] = "REQUIRED";
>> +    sets[2] = "DISABLED";
>> +
>> +    for (ns in sets) {
>> +        s = sets[ns];
> nit: 's' -> 'state', though is 'state' the most appropriate word here?

's' is the tag string, "REQUIRED" or "DISABLED".

> 
>> +
>> +        printf "/*\n";
>> +        printf " * %s features:\n", s;
>> +        printf " *\n";
>> +        fstr = "";
>> +        for (i = 0; i < ncapints; i++) {
>> +            mask = 0;
>> +            for (j = 0; j < 32; j++) {
>> +                nfeat = i*32 + j;
>> +                feat = feats[nfeat];
>> +                if (feat) {
>> +                    st = !!featstat[s, feat];
> 
> No point in doing the !! dance here, the value written there is already 
> true or false since it comes as a result from the earlier check. Just 
> drop the !! also what's with the 1 or 2 letter variables. 'st'-> 
> 'enabled' then the check reads 'if (enabled) { foo }'

Sounds reasonable, I can make the change.

> 
>> +                    if (st) {
>> +                        nfstr = fstr " " feat;
>> +                        if (length(nfstr) > 72) {
>> +                            printf " *   %s\n", fstr;
>> +                            nfstr = " " feat;
>> +                        }
>> +                        fstr = nfstr;
>> +                        mask += (2 ^ j);
>> +                    }
>> +                }
>> +            }
>> +            masks[i] = mask;
>> +        }
>> +        printf " *   %s\n */\n", fstr;
>> +
>> +        for (i = 0; i < ncapints; i++)
>> +            printf "#define %s_MASK%d\t0x%08xU\n", s, i, masks[i];
>> +
>> +        printf "#define %s_MASK_CHECK BUILD_BUG_ON_ZERO(NCAPINTS != 
>> %d)\n\n", s, ncapints;
>> +    }
>> +
>> +    printf "#endif /* _ASM_X86_FEATUREMASKS_H */\n";
>> +}
>