[PATCH] x86/cpufeatures: Free up unused feature bits

[PATCH] x86/cpufeatures: Free up unused feature bits

[Sohil Mehta]

Linux defined feature bits X86_FEATURE_P3 and X86_FEATURE_P4 are not
used anywhere, neither are they visible to userspace.
commit f31d731e4467 ("x86: use X86_FEATURE_NOPL in alternatives") got
rid of the last usage. Remove the related mappings and code.

Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
---
 arch/x86/include/asm/cpufeatures.h | 4 ++--
 arch/x86/kernel/cpu/intel.c        | 5 -----
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 913fd3a7bac6..f5c4ea3fd364 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -83,8 +83,8 @@
 #define X86_FEATURE_CENTAUR_MCR		( 3*32+ 3) /* "centaur_mcr" Centaur MCRs (= MTRRs) */
 #define X86_FEATURE_K8			( 3*32+ 4) /* Opteron, Athlon64 */
 #define X86_FEATURE_ZEN5		( 3*32+ 5) /* CPU based on Zen5 microarchitecture */
-#define X86_FEATURE_P3			( 3*32+ 6) /* P3 */
-#define X86_FEATURE_P4			( 3*32+ 7) /* P4 */
+/* Free                                 ( 3*32+ 6) */
+/* Free                                 ( 3*32+ 7) */
 #define X86_FEATURE_CONSTANT_TSC	( 3*32+ 8) /* "constant_tsc" TSC ticks at a constant rate */
 #define X86_FEATURE_UP			( 3*32+ 9) /* "up" SMP kernel running on UP */
 #define X86_FEATURE_ART			( 3*32+10) /* "art" Always running timer (ART) */
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index e7656cbef68d..1c892ebaa68c 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -628,11 +628,6 @@ static void init_intel(struct cpuinfo_x86 *c)
 		if (p)
 			strcpy(c->x86_model_id, p);
 	}
-
-	if (c->x86 == 15)
-		set_cpu_cap(c, X86_FEATURE_P4);
-	if (c->x86 == 6)
-		set_cpu_cap(c, X86_FEATURE_P3);
 #endif
 
 	/* Work around errata */
-- 
2.34.1

Re: [PATCH] x86/cpufeatures: Free up unused feature bits

[Dave Hansen]

On 11/7/24 15:30, Sohil Mehta wrote:
> Linux defined feature bits X86_FEATURE_P3 and X86_FEATURE_P4 are not
> used anywhere, neither are they visible to userspace.
> commit f31d731e4467 ("x86: use X86_FEATURE_NOPL in alternatives") got
> rid of the last usage. Remove the related mappings and code.

Hah, not referenced since 2008!  This one seems like a no-brainer.
Thanks for finding it!

Acked-by: Dave Hansen <dave.hansen@linux.intel.com>

Re: [PATCH] x86/cpufeatures: Free up unused feature bits

[Sohil Mehta]

On 11/7/2024 3:36 PM, Dave Hansen wrote:
> On 11/7/24 15:30, Sohil Mehta wrote:
>> Linux defined feature bits X86_FEATURE_P3 and X86_FEATURE_P4 are not
>> used anywhere, neither are they visible to userspace.
>> commit f31d731e4467 ("x86: use X86_FEATURE_NOPL in alternatives") got
>> rid of the last usage. Remove the related mappings and code.
> 
> Hah, not referenced since 2008!  This one seems like a no-brainer.

Thankfully, it wasn't referenced anywhere. For a couple of minutes I was
wondering why all family 6 CPUs are marked as Pentium 3 on 32-bit.

> @@ -628,11 +628,6 @@ static void init_intel(struct cpuinfo_x86 *c)
>  		if (p)
>  			strcpy(c->x86_model_id, p);
>  	}

Here..

> -	if (c->x86 == 6)
> -		set_cpu_cap(c, X86_FEATURE_P3);

Re: [PATCH] x86/cpufeatures: Free up unused feature bits

[H. Peter Anvin]

On November 8, 2024 12:44:57 AM GMT+01:00, Sohil Mehta <sohil.mehta@intel.com> wrote:
>On 11/7/2024 3:36 PM, Dave Hansen wrote:
>> On 11/7/24 15:30, Sohil Mehta wrote:
>>> Linux defined feature bits X86_FEATURE_P3 and X86_FEATURE_P4 are not
>>> used anywhere, neither are they visible to userspace.
>>> commit f31d731e4467 ("x86: use X86_FEATURE_NOPL in alternatives") got
>>> rid of the last usage. Remove the related mappings and code.
>> 
>> Hah, not referenced since 2008!  This one seems like a no-brainer.
>
>Thankfully, it wasn't referenced anywhere. For a couple of minutes I was
>wondering why all family 6 CPUs are marked as Pentium 3 on 32-bit.
>
>> @@ -628,11 +628,6 @@ static void init_intel(struct cpuinfo_x86 *c)
>>  		if (p)
>>  			strcpy(c->x86_model_id, p);
>>  	}
>
>Here..
>
>> -	if (c->x86 == 6)
>> -		set_cpu_cap(c, X86_FEATURE_P3);
>

Be careful - these bits are used in module strings and so modutils need to understand them.

Re: [PATCH] x86/cpufeatures: Free up unused feature bits

[Sohil Mehta]

On 11/7/2024 3:49 PM, H. Peter Anvin wrote:
>>> -	if (c->x86 == 6)
>>> -		set_cpu_cap(c, X86_FEATURE_P3);
>>
> 
> Be careful - these bits are used in module strings and so modutils need to understand them.

Sorry, I didn't understand this properly. How do I figure out whether
these bits are used elsewhere? Can you please provide a pointer?

Sohil

Re: [PATCH] x86/cpufeatures: Free up unused feature bits

[H. Peter Anvin]

On November 8, 2024 1:35:59 AM GMT+01:00, Sohil Mehta <sohil.mehta@intel.com> wrote:
>On 11/7/2024 3:49 PM, H. Peter Anvin wrote:
>>>> -	if (c->x86 == 6)
>>>> -		set_cpu_cap(c, X86_FEATURE_P3);
>>>
>> 
>> Be careful - these bits are used in module strings and so modutils need to understand them.
>
>Sorry, I didn't understand this properly. How do I figure out whether
>these bits are used elsewhere? Can you please provide a pointer?
>
>Sohil

Look at the modutils sources.

Re: [PATCH] x86/cpufeatures: Free up unused feature bits

[Dave Hansen]

On 11/7/24 15:49, H. Peter Anvin wrote:
> Be careful - these bits are used in module strings and so modutils need to understand them.

Yeah, very true.  But I didn't ever see these features get used in a
MODULE_DEVICE_TABLE().  Do you still have concerns if there was never an
in-tree user that used X86_FEATURE_P3/P4 in a MODULE_DEVICE_TABLE()?$

Sohil, go look at:

# cat /sys/devices/system/cpu/modalias
cpu:type:x86,ven0000fam0006mod008C:feature:,0000,0001,0002,0003,0004,0005,0006,...

and, for instance:

# modinfo /lib/modules/5.17.0-rc4/kernel/arch/x86/kvm/kvm-intel.ko
filename:       /lib/modules/5.17.0-rc4/kernel/arch/x86/kvm/kvm-intel.ko
license:        GPL
author:         Qumranet
srcversion:     ED99EA15FCA9B58172BAEB4
alias:          cpu:type:x86,ven*fam*mod*:feature:*0085*

Those magic strings get matched up by udev (I think) to auto-load
modules when the CPU 'modalias' matches the module 'alias'.  Let's say
we had an ooooooooold module that did this:

#ifdef MODULE
static const struct x86_cpu_id foo_cpu_id[] = {
        X86_MATCH_FEATURE(X86_FEATURE_P3, NULL),
        {}
};
MODULE_DEVICE_TABLE(x86cpu, foo_cpu_id);
#endif

which generated a modalias like this:

	alias:          cpu:type:x86,ven*fam*mod*:feature:*0067*

and then we recycled number 67:

-#define X86_FEATURE_P3                  ( 3*32+ 6) /* P3 */
+#define X86_FEATURE_WHIZZY_NEW_FEATURE  ( 3*32+ 6) /* P3 */

udev might try to load the old module on a new CPU with
X86_FEATURE_WHIZZY_NEW_FEATURE that's not a P3.

I sure hope we haven't been using too many of these synthetic features
in MODULE_DEVICE_TABLE()s, because we tend to move them around, but I
guess it's possible.

Re: [PATCH] x86/cpufeatures: Free up unused feature bits

[Sohil Mehta]

On 11/7/2024 5:12 PM, Dave Hansen wrote:
> Sohil, go look at:
> 
> # cat /sys/devices/system/cpu/modalias
> cpu:type:x86,ven0000fam0006mod008C:feature:,0000,0001,0002,0003,0004,0005,0006,...
> 

Thanks for the explanation. Peter's comment makes sense to me now.

> 
> I sure hope we haven't been using too many of these synthetic features
> in MODULE_DEVICE_TABLE()s, because we tend to move them around, but I
> guess it's possible.

I found at least one recent usage that matches this pattern.
Look at commit cbcddaa33d7e ("perf/x86/rapl: Use CPUID bit on AMD and
Hygon parts"). It defines a synthetic feature bit X86_FEATURE_RAPL and
adds it to the rapl_model_match[] table.

	MODULE_DEVICE_TABLE(x86cpu, rapl_model_match);

It almost seems like some of these bits are now ABI. We probably need to
mark them and keep these mappings pinned to avoid future issues.
Recycling these bits seems to be very common.

Re: [PATCH] x86/cpufeatures: Free up unused feature bits

[H. Peter Anvin]

On November 8, 2024 3:17:24 AM GMT+01:00, Sohil Mehta <sohil.mehta@intel.com> wrote:
>On 11/7/2024 5:12 PM, Dave Hansen wrote:
>> Sohil, go look at:
>> 
>> # cat /sys/devices/system/cpu/modalias
>> cpu:type:x86,ven0000fam0006mod008C:feature:,0000,0001,0002,0003,0004,0005,0006,...
>> 
>
>Thanks for the explanation. Peter's comment makes sense to me now.
>
>> 
>> I sure hope we haven't been using too many of these synthetic features
>> in MODULE_DEVICE_TABLE()s, because we tend to move them around, but I
>> guess it's possible.
>
>I found at least one recent usage that matches this pattern.
>Look at commit cbcddaa33d7e ("perf/x86/rapl: Use CPUID bit on AMD and
>Hygon parts"). It defines a synthetic feature bit X86_FEATURE_RAPL and
>adds it to the rapl_model_match[] table.
>
>	MODULE_DEVICE_TABLE(x86cpu, rapl_model_match);
>
>It almost seems like some of these bits are now ABI. We probably need to
>mark them and keep these mappings pinned to avoid future issues.
>Recycling these bits seems to be very common.

It was a really unfortunate choice of ABI design, especially since there already were name strings available...

Re: [PATCH] x86/cpufeatures: Free up unused feature bits

[Dave Hansen]

On 11/7/24 17:12, Dave Hansen wrote:
> and then we recycled number 67:
> 
> -#define X86_FEATURE_P3                  ( 3*32+ 6) /* P3 */
> +#define X86_FEATURE_WHIZZY_NEW_FEATURE  ( 3*32+ 6) /* P3 */
> 
> udev might try to load the old module on a new CPU with
> X86_FEATURE_WHIZZY_NEW_FEATURE that's not a P3.

Thinking about this a bit more...

The kernel generates _both_ the
"cpu:type:x86,ven*fam*mod*:feature:*0067*" string and the sysfs modalias
string.  So the issue isn't practically a mismatch between those.

It's if some consumer of those fields (like /lib/udev/hwdb.d/) was
looking for feature 67.

The good news is that I don't see any of those today.  But it's totally
possible that folks have some crazy rules out there.  So we should
probably be _careful_ about changing those values and not just change
them *ALL*.  But I think it's pretty unlikely we'll break anybody by
reusing a bit or two.

Re: [PATCH] x86/cpufeatures: Free up unused feature bits

[Sohil Mehta]

On 11/7/2024 5:12 PM, Dave Hansen wrote:
> Let's say
> we had an ooooooooold module that did this:
> 
> #ifdef MODULE
> static const struct x86_cpu_id foo_cpu_id[] = {
>         X86_MATCH_FEATURE(X86_FEATURE_P3, NULL),
>         {}
> };
> MODULE_DEVICE_TABLE(x86cpu, foo_cpu_id);
> #endif
> 
> which generated a modalias like this:
> 
> 	alias:          cpu:type:x86,ven*fam*mod*:feature:*0067*
> 
> and then we recycled number 67:
> 
> -#define X86_FEATURE_P3                  ( 3*32+ 6) /* P3 */
> +#define X86_FEATURE_WHIZZY_NEW_FEATURE  ( 3*32+ 6) /* P3 */
> 

Coretemp (hwmon) seems to follow this pattern exactly.

commit 9b38096fde5f ("HWMON: Convert coretemp to x86 cpuid autoprobing")
adds X86_FEATURE_DTS to MODULE_DEVICE_TABLE(x86cpu, coretemp_ids)

commit 2ccd71f1b278 ("x86/cpufeature: Move some of the scattered feature
bits to x86_capability") frees up X86_FEATURE_DTS(DTHERM).

-#define X86_FEATURE_DTHERM     ( 7*32+ 7) /* Digital Thermal Sensor */

commit 765a0542fdc7 ("x86/virt/tdx: Detect TDX during kernel boot")
reuses that bit.

+#define X86_FEATURE_TDX_HOST_PLATFORM  ( 7*32+ 7) /* Platform supports
being a TDX host */

> udev might try to load the old module on a new CPU with
> X86_FEATURE_WHIZZY_NEW_FEATURE that's not a P3.
> 

So an old coretemp module could get loaded when the above TDX bit is
set. Not sure how likely this scenario is or what can we do to avoid it now?

> I sure hope we haven't been using too many of these synthetic features
> in MODULE_DEVICE_TABLE()s, because we tend to move them around, but I
> guess it's possible.

At least features X86_FEATURE_P3 and X86_FEATURE_P4 seem safe to be
recycled as they haven't been used in MODULE_DEVICE_TABLE().

Re: [PATCH] x86/cpufeatures: Free up unused feature bits

[Dave Hansen]

On 11/8/24 10:42, Sohil Mehta wrote:
>> udev might try to load the old module on a new CPU with
>> X86_FEATURE_WHIZZY_NEW_FEATURE that's not a P3.
>>
> So an old coretemp module could get loaded when the above TDX bit is
> set. Not sure how likely this scenario is or what can we do to avoid it now?

Pretty unlikely that someone would change the bit, have modversions
disabled, *AND* need to load a module built before the bit changed.

I think we can mostly ignore it.  If we break something, it _can_ be
backed out and fixed.