[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <df5b3001-e79b-4bfd-a2a1-c63111feba8a@suse.com>
Date: Mon, 1 Jul 2024 13:44:15 +0300
From: Nikolay Borisov <nik.borisov@...e.com>
To: "Xin Li (Intel)" <xin@...or.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 28.06.24 г. 20:45 ч., Xin Li (Intel) wrote:
> From: "H. Peter Anvin (Intel)" <hpa@...or.com>
>
> Introduce an AWK script to auto-generate a header with required and
> disabled feature masks based on <asm/cpufeatures.h> and current build
> config. Thus for any CPU feature with a build config, e.g., X86_FRED,
> simply add
>
> config X86_DISABLED_FEATURE_FRED
> def_bool y
> depends on !X86_FRED
>
> to arch/x86/Kconfig.cpufeatures, instead of adding a conditional CPU
> feature disable flag, e.g., DISABLE_FRED.
>
> Lastly the generated required and disabled feature masks will be added
> to their corresponding feature masks for this particular compile-time
> configuration.
>
> [ Xin: build integration improvements ]
>
> Signed-off-by: H. Peter Anvin (Intel) <hpa@...or.com>
> Signed-off-by: Xin Li (Intel) <xin@...or.com>
> Reviewed-by: Nikolay Borisov <nik.borisov@...e.com>
> ---
Overall it's better I still have some minor comments which can be
addressed only if you are going to make another submission.
<snip>
> 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).
> +}
> +
> +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?
> +
> + 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 }'
> + 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";
> +}
Powered by blists - more mailing lists