Re: [PATCH v3 2/2] x86/cpufeature: Add a debug print for unmet dependencies

[H. Peter Anvin]

On December 6, 2024 4:41:26 PM PST, Sohil Mehta <sohil.mehta@xxxxxxxxx> wrote:
>Instead of silently disabling features, add a print which might be
>useful to users if their favorite feature gets unexpectedly disabled.
>
>Features are typically represented through unsigned integers though some
>of them have user friendly names if they are exposed via cpuinfo.  Show
>the friendlier name if available otherwise display the X86_FEATURE_*
>numerals to make it easier to identify the feature.
>
>Use pr_debug() to avoid spamming the kernel log and generating false
>alarms. Note that the print will occur once per disabled feature on
>every CPU. Show this information only when someone is really looking for
>it.
>
>Suggested-by: Tony Luck <tony.luck@xxxxxxxxx>
>Signed-off-by: Sohil Mehta <sohil.mehta@xxxxxxxxx>
>---
>Sent as a separate patch to make it easier to review and drop if it
>feels unnecessary.
>
>I can see both sides of the argument. The pr_debug() serves a
>compromise between the two.
>
>v3: New patch.
>
>---
> arch/x86/kernel/cpu/cpuid-deps.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
>diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
>index 8bea5c5e4fd2..c72f2dd77d72 100644
>--- a/arch/x86/kernel/cpu/cpuid-deps.c
>+++ b/arch/x86/kernel/cpu/cpuid-deps.c
>@@ -147,12 +147,32 @@ void setup_clear_cpu_cap(unsigned int feature)
> 	do_clear_cpu_cap(NULL, feature);
> }
> 
>+/*
>+ * Return the feature "name" if available otherwise return
>+ * the X86_FEATURE_* numerals to make it easier to identify
>+ * the feature.
>+ */
>+static const char *x86_feature_name(unsigned int feature, char *buf)
>+{
>+	if (x86_cap_flags[feature])
>+		return x86_cap_flags[feature];
>+
>+	snprintf(buf, 12, "%d*32+%2d", feature / 32, feature % 32);
>+
>+	return buf;
>+}
>+
> void filter_feature_dependencies(struct cpuinfo_x86 *c)
> {
>+	char feature_buf[12], depends_buf[12];
> 	const struct cpuid_dep *d;
> 
> 	for (d = cpuid_deps; d->feature; d++) {
>-		if (cpu_has(c, d->feature) && !cpu_has(c, d->depends))
>+		if (cpu_has(c, d->feature) && !cpu_has(c, d->depends)) {
>+			pr_debug("x86/cpu: Disabling feature %s since feature %s is missing\n",
>+				 x86_feature_name(d->feature, feature_buf),
>+				 x86_feature_name(d->depends, depends_buf));
> 			do_clear_cpu_cap(c, d->feature);
>+		}
> 	}
> }

Ok, I realize that the x86 maintainers **very legitimately** don't want more crap in /proc/cpuinfo, but perhaps we could include the strings for printing debug messages in cleartext? Add a bitmap for which entries should go into /proc/cpuinfo.