[PATCH 0/4] x86: Add CPUID region helper and clarify Xen startup

[PATCH 0/4] x86: Add CPUID region helper and clarify Xen startup

[Dave Hansen]

This peels off a patches from the top of another longer series
where Borislav suggested actually fixing a problem rather than
complaining about it:

	https://lore.kernel.org/all/20240307162752.GAZenrCDqs0lMTjmu1@fat_crate.local/

There's a little detour here to explain some mysterious (to me)
CPUID prodding.

The end result gets rid of some fragile Xen boot code.  It also clears
the way for a future series to bring some order to the early boot code
and remove some of the random tinkering that is there today.

[PATCH 1/4] x86/cpu: Add and use new CPUID region helper

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

There are some (before now) unwritten rules about CPUID "regions".
Basically, there is a 32-bit address space of CPUID leaves.  The
top 16 bits address a "region" and the first leaf in a region
is special.

The kernel only has a few spots that care about this, but it's
rather hard to make sense of the code as is.

Add a helper that explains regions.  Use it where applicable.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Juergen Gross <jgross@suse.com>
---

 b/arch/x86/include/asm/cpuid.h    |   59 ++++++++++++++++++++++++++++++++++++++
 b/arch/x86/kernel/cpu/common.c    |   13 +++-----
 b/arch/x86/kernel/cpu/transmeta.c |    9 +----
 b/arch/x86/xen/enlighten_pv.c     |    9 +----
 4 files changed, 69 insertions(+), 21 deletions(-)

diff -puN arch/x86/include/asm/cpuid.h~cpuid-regions arch/x86/include/asm/cpuid.h
--- a/arch/x86/include/asm/cpuid.h~cpuid-regions	2024-03-18 15:12:20.676308753 -0700
+++ b/arch/x86/include/asm/cpuid.h	2024-03-22 09:17:13.296507986 -0700
@@ -168,4 +168,63 @@ static inline uint32_t hypervisor_cpuid_
 	return 0;
 }
 
+/*
+ * By convention, CPUID is broken up into regions which each
+ * have 2^16 leaves.  EAX in the first leaf of each valid
+ * region returns the maximum valid leaf in that region.
+ *
+ * The regions can be thought of as being vendor-specific
+ * areas of CPUID, but that's imprecise because everybody
+ * implements the "Intel" region and Intel implements the
+ * AMD region.  There are a few well-known regions:
+ *  - Intel	(0x0000)
+ *  - AMD	(0x8000)
+ *  - Transmeta	(0x8086)
+ *  - Centaur	(0xC000)
+ *
+ * Consider a CPU that where the maximum leaf in the Transmeta
+ * region is 2.  On such a CPU, leaf 0x80860000 would contain:
+ * EAX==0x80860002.
+ * region-^^^^
+ *   max leaf-^^^^
+ */
+static inline u32 cpuid_region_max_leaf(u16 region)
+{
+	u32 eax = cpuid_eax(region << 16);
+
+	/*
+	 * An unsupported region may return data from the last
+	 * "basic" leaf, which is essentially garbage.  Avoid
+	 * mistaking basic leaf data for region data.
+	 *
+	 * Note: this is not perfect.  It is theoretically
+	 * possible for the last basic leaf to _resemble_ a
+	 * valid first leaf from a region that doesn't exist.
+	 * But Intel at least seems to pad out the basic region
+	 * with 0's, possibly to avoid this.
+	 */
+        if ((eax >> 16) != region)
+		return 0;
+
+	return eax;
+}
+
+/* Returns true if the leaf exists and @value was populated */
+static inline bool get_cpuid_region_leaf(u32 leaf, enum cpuid_regs_idx reg,
+					 u32 *value)
+{
+	u16 region = leaf >> 16;
+	u32 regs[4];
+
+	if (cpuid_region_max_leaf(region) < leaf)
+		return false;
+
+	cpuid(leaf, &regs[CPUID_EAX], &regs[CPUID_EBX],
+	            &regs[CPUID_ECX], &regs[CPUID_EDX]);
+
+	*value = regs[reg];
+
+	return true;
+}
+
 #endif /* _ASM_X86_CPUID_H */
diff -puN arch/x86/kernel/cpu/common.c~cpuid-regions arch/x86/kernel/cpu/common.c
--- a/arch/x86/kernel/cpu/common.c~cpuid-regions	2024-03-18 15:12:20.676308753 -0700
+++ b/arch/x86/kernel/cpu/common.c	2024-03-18 15:12:20.684309023 -0700
@@ -1049,16 +1049,13 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 	}
 
 	/* AMD-defined flags: level 0x80000001 */
-	eax = cpuid_eax(0x80000000);
-	c->extended_cpuid_level = eax;
+	c->extended_cpuid_level = cpuid_region_max_leaf(0x8000);
 
-	if ((eax & 0xffff0000) == 0x80000000) {
-		if (eax >= 0x80000001) {
-			cpuid(0x80000001, &eax, &ebx, &ecx, &edx);
+	if (c->extended_cpuid_level >= 0x80000001) {
+		cpuid(0x80000001, &eax, &ebx, &ecx, &edx);
 
-			c->x86_capability[CPUID_8000_0001_ECX] = ecx;
-			c->x86_capability[CPUID_8000_0001_EDX] = edx;
-		}
+		c->x86_capability[CPUID_8000_0001_ECX] = ecx;
+		c->x86_capability[CPUID_8000_0001_EDX] = edx;
 	}
 
 	if (c->extended_cpuid_level >= 0x80000007) {
diff -puN arch/x86/kernel/cpu/transmeta.c~cpuid-regions arch/x86/kernel/cpu/transmeta.c
--- a/arch/x86/kernel/cpu/transmeta.c~cpuid-regions	2024-03-18 15:12:20.680308889 -0700
+++ b/arch/x86/kernel/cpu/transmeta.c	2024-03-18 15:12:20.684309023 -0700
@@ -9,14 +9,9 @@
 
 static void early_init_transmeta(struct cpuinfo_x86 *c)
 {
-	u32 xlvl;
-
 	/* Transmeta-defined flags: level 0x80860001 */
-	xlvl = cpuid_eax(0x80860000);
-	if ((xlvl & 0xffff0000) == 0x80860000) {
-		if (xlvl >= 0x80860001)
-			c->x86_capability[CPUID_8086_0001_EDX] = cpuid_edx(0x80860001);
-	}
+	get_cpuid_region_leaf(0x80860001, CPUID_EDX,
+			  &c->x86_capability[CPUID_8086_0001_EDX]);
 }
 
 static void init_transmeta(struct cpuinfo_x86 *c)
diff -puN arch/x86/xen/enlighten_pv.c~cpuid-regions arch/x86/xen/enlighten_pv.c
--- a/arch/x86/xen/enlighten_pv.c~cpuid-regions	2024-03-18 15:12:20.680308889 -0700
+++ b/arch/x86/xen/enlighten_pv.c	2024-03-22 09:15:33.280428290 -0700
@@ -141,16 +141,13 @@ static void __init xen_set_mtrr_data(voi
 	};
 	unsigned int reg;
 	unsigned long mask;
-	uint32_t eax, width;
+	uint32_t width;
 	static struct mtrr_var_range var[MTRR_MAX_VAR_RANGES] __initdata;
 
 	/* Get physical address width (only 64-bit cpus supported). */
 	width = 36;
-	eax = cpuid_eax(0x80000000);
-	if ((eax >> 16) == 0x8000 && eax >= 0x80000008) {
-		eax = cpuid_eax(0x80000008);
-		width = eax & 0xff;
-	}
+	/* Will overwrite 'width' if available in CPUID: */
+	get_cpuid_region_leaf(0x80000008, CPUID_EAX, &width);
 
 	for (reg = 0; reg < MTRR_MAX_VAR_RANGES; reg++) {
 		op.u.read_memtype.reg = reg;
_

[PATCH 2/4] perf/x86/ibs: Use CPUID region helper

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

IBS details are enumerated in an extended CPUID leaf.  But
the support has an open-coded CPUID region check.  Use the
new helper to trim down the code.

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

Note: this cleanup could take another form:

        if (boot_cpu_data->extended_cpuid_level >= IBS_CPUID_FEATURES)
                caps = cpuid_eax(IBS_CPUID_FEATURES);

that would be one fewer CPUID invocations, but one more
line of code.
---

 b/arch/x86/events/amd/ibs.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff -puN arch/x86/events/amd/ibs.c~ibs-region-helpers arch/x86/events/amd/ibs.c
--- a/arch/x86/events/amd/ibs.c~ibs-region-helpers	2024-03-18 15:12:21.228327350 -0700
+++ b/arch/x86/events/amd/ibs.c	2024-03-18 15:12:21.228327350 -0700
@@ -1278,18 +1278,13 @@ static __init int perf_event_ibs_init(vo
 
 static __init u32 __get_ibs_caps(void)
 {
-	u32 caps;
-	unsigned int max_level;
+	u32 caps = 0;
 
 	if (!boot_cpu_has(X86_FEATURE_IBS))
 		return 0;
 
-	/* check IBS cpuid feature flags */
-	max_level = cpuid_eax(0x80000000);
-	if (max_level < IBS_CPUID_FEATURES)
-		return IBS_CAPS_DEFAULT;
+	get_cpuid_region_leaf(IBS_CPUID_FEATURES, CPUID_EAX, &caps);
 
-	caps = cpuid_eax(IBS_CPUID_FEATURES);
 	if (!(caps & IBS_CAPS_AVAIL))
 		/* cpuid flags not valid */
 		return IBS_CAPS_DEFAULT;
_

[PATCH 3/4] x86/boot: Explicitly pass NX enabling status

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

The kernel sometimes needs to mask unsupported bits out of page
table entries.  It does that with a mask: '__supported_pte_mask'.

That mask can obviously only contain the No-eXecute bit ( _PAGE_NX)
on hardware where NX is supported.  x86_configure_nx() checks the
boot CPU's NX support and adjusts the mask appropriately.

But it doesn't check support directly.  It uses the venerable
'boot_cpu_data' which is a software approximation of the actual CPU
support.  Unfortunately, Xen wants to set up '__supported_pte_mask'
before 'boot_cpu_data' has been initialized.  It hacks around this
problem by repeating some of the 'boot_cpu_data' setup *just* for
NX.

Have x86_configure_nx() stop consulting 'boot_cpu_data' and move
the NX detection to the caller.

No functional change.  That will come later.

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

 b/arch/x86/include/asm/proto.h |    2 +-
 b/arch/x86/kernel/setup.c      |    6 +++---
 b/arch/x86/xen/enlighten_pv.c  |    2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff -puN arch/x86/include/asm/proto.h~x86_configure_nx-arg arch/x86/include/asm/proto.h
--- a/arch/x86/include/asm/proto.h~x86_configure_nx-arg	2024-03-18 15:12:21.704343388 -0700
+++ b/arch/x86/include/asm/proto.h	2024-03-18 15:12:21.708343524 -0700
@@ -37,7 +37,7 @@ void entry_SYSRETL_compat_end(void);
 #define entry_SYSENTER_compat NULL
 #endif
 
-void x86_configure_nx(void);
+void x86_configure_nx(bool nx_supported);
 
 extern int reboot_force;
 
diff -puN arch/x86/kernel/setup.c~x86_configure_nx-arg arch/x86/kernel/setup.c
--- a/arch/x86/kernel/setup.c~x86_configure_nx-arg	2024-03-18 15:12:21.704343388 -0700
+++ b/arch/x86/kernel/setup.c	2024-03-18 15:12:21.708343524 -0700
@@ -687,9 +687,9 @@ dump_kernel_offset(struct notifier_block
 	return 0;
 }
 
-void x86_configure_nx(void)
+void x86_configure_nx(bool nx_supported)
 {
-	if (boot_cpu_has(X86_FEATURE_NX))
+	if (nx_supported)
 		__supported_pte_mask |= _PAGE_NX;
 	else
 		__supported_pte_mask &= ~_PAGE_NX;
@@ -853,7 +853,7 @@ void __init setup_arch(char **cmdline_p)
 	 * whether hardware doesn't support NX (so that the early EHCI debug
 	 * console setup can safely call set_fixmap()).
 	 */
-	x86_configure_nx();
+	x86_configure_nx(boot_cpu_has(X86_FEATURE_NX));
 
 	parse_early_param();
 
diff -puN arch/x86/xen/enlighten_pv.c~x86_configure_nx-arg arch/x86/xen/enlighten_pv.c
--- a/arch/x86/xen/enlighten_pv.c~x86_configure_nx-arg	2024-03-18 15:12:21.704343388 -0700
+++ b/arch/x86/xen/enlighten_pv.c	2024-03-18 15:12:21.708343524 -0700
@@ -1371,7 +1371,7 @@ asmlinkage __visible void __init xen_sta
 
 	/* Work out if we support NX */
 	get_cpu_cap(&boot_cpu_data);
-	x86_configure_nx();
+	x86_configure_nx(boot_cpu_has(X86_FEATURE_NX));
 
 	/*
 	 * Set up kernel GDT and segment registers, mainly so that
_

[PATCH 4/4] x86/xen: Enumerate NX from CPUID directly

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

xen_start_kernel() is some of the first C code run "Xen PV" systems.
It precedes things like setup_arch() or processor initialization code
that would be thought of as "very early".

That means that 'boot_cpu_data' is garbage.  It has not even
established the utter basics like if the CPU supports the CPUID
instruction.  Unfortunately get_cpu_cap() requires this exact
information.

Nevertheless, xen_start_kernel() calls get_cpu_cap().  But it
works out in practice because it's looking for the NX bit which
comes from an extended CPUID leaf that doesn't depend on
c->cpuid_level being set.

Stop calling get_cpu_cap().  Use CPUID directly.  Add a little
helper to avoid cluttering up the xen_start_kernel() flow.

This removes the risky, barely-working call to get_cpu_cap().

Note: The '& 31' strips the "capability" word off of an
      X86_FEATURE_* value.  It's a magic number, but there are
      a few of them sparsely scattered around and it's way
      shorter than any helper.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Juergen Gross <jgross@suse.com>
---

 b/arch/x86/xen/enlighten_pv.c |   19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff -puN arch/x86/xen/enlighten_pv.c~xen-explain1 arch/x86/xen/enlighten_pv.c
--- a/arch/x86/xen/enlighten_pv.c~xen-explain1	2024-03-22 09:30:25.499892614 -0700
+++ b/arch/x86/xen/enlighten_pv.c	2024-03-22 09:30:25.503892742 -0700
@@ -1306,6 +1306,21 @@ static void __init xen_domu_set_legacy_f
 
 extern void early_xen_iret_patch(void);
 
+/*
+ * It is too early for boot_cpu_has() and friends to work.
+ * Use CPUID to directly enumerate NX support.
+ */
+static inline void xen_configure_nx(void)
+{
+	bool nx_supported;
+	u32 eax = 0;
+
+	get_cpuid_region_leaf(0x80000001, CPUID_EDX, &eax);
+
+	nx_supported = eax & (X86_FEATURE_NX & 31);
+	x86_configure_nx(nx_supported);
+}
+
 /* First C function to be called on Xen boot */
 asmlinkage __visible void __init xen_start_kernel(struct start_info *si)
 {
@@ -1369,9 +1384,7 @@ asmlinkage __visible void __init xen_sta
 	/* Get mfn list */
 	xen_build_dynamic_phys_to_machine();
 
-	/* Work out if we support NX */
-	get_cpu_cap(&boot_cpu_data);
-	x86_configure_nx(boot_cpu_has(X86_FEATURE_NX));
+	xen_configure_nx();
 
 	/*
 	 * Set up kernel GDT and segment registers, mainly so that
_

Re: [PATCH 1/4] x86/cpu: Add and use new CPUID region helper

[Ingo Molnar]

* Dave Hansen <dave.hansen@linux.intel.com> wrote:

> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> There are some (before now) unwritten rules about CPUID "regions".
> Basically, there is a 32-bit address space of CPUID leaves.  The
> top 16 bits address a "region" and the first leaf in a region
> is special.
> 
> The kernel only has a few spots that care about this, but it's
> rather hard to make sense of the code as is.
> 
> Add a helper that explains regions.  Use it where applicable.
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Juergen Gross <jgross@suse.com>
> ---
> 
>  b/arch/x86/include/asm/cpuid.h    |   59 ++++++++++++++++++++++++++++++++++++++
>  b/arch/x86/kernel/cpu/common.c    |   13 +++-----
>  b/arch/x86/kernel/cpu/transmeta.c |    9 +----
>  b/arch/x86/xen/enlighten_pv.c     |    9 +----
>  4 files changed, 69 insertions(+), 21 deletions(-)
> 
> diff -puN arch/x86/include/asm/cpuid.h~cpuid-regions arch/x86/include/asm/cpuid.h
> --- a/arch/x86/include/asm/cpuid.h~cpuid-regions	2024-03-18 15:12:20.676308753 -0700
> +++ b/arch/x86/include/asm/cpuid.h	2024-03-22 09:17:13.296507986 -0700
> @@ -168,4 +168,63 @@ static inline uint32_t hypervisor_cpuid_
>  	return 0;
>  }
>  
> +/*
> + * By convention, CPUID is broken up into regions which each
> + * have 2^16 leaves.  EAX in the first leaf of each valid
> + * region returns the maximum valid leaf in that region.
> + *
> + * The regions can be thought of as being vendor-specific
> + * areas of CPUID, but that's imprecise because everybody
> + * implements the "Intel" region and Intel implements the
> + * AMD region.  There are a few well-known regions:
> + *  - Intel	(0x0000)
> + *  - AMD	(0x8000)
> + *  - Transmeta	(0x8086)
> + *  - Centaur	(0xC000)
> + *
> + * Consider a CPU that where the maximum leaf in the Transmeta
> + * region is 2.  On such a CPU, leaf 0x80860000 would contain:
> + * EAX==0x80860002.
> + * region-^^^^
> + *   max leaf-^^^^

Minor nit:

 s/a CPU that where the
  /a CPU where the

> +	 * possible for the last basic leaf to _resemble_ a
> +	 * valid first leaf from a region that doesn't exist.
> +	 * But Intel at least seems to pad out the basic region
> +	 * with 0's, possibly to avoid this.
> +	 */
> +        if ((eax >> 16) != region)
> +		return 0;
> +
> +	return eax;

There's whitespace damage at the 'if' line.

Thanks,

	Ingo

Re: [PATCH 3/4] x86/boot: Explicitly pass NX enabling status

[Huang, Kai]

On Fri, 2024-03-22 at 10:56 -0700, Dave Hansen wrote:
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> The kernel sometimes needs to mask unsupported bits out of page
> table entries.  It does that with a mask: '__supported_pte_mask'.
> 
> That mask can obviously only contain the No-eXecute bit ( _PAGE_NX)

							   ^ unnecessary space

> on hardware where NX is supported.  x86_configure_nx() checks the
> boot CPU's NX support and adjusts the mask appropriately.
> 
> But it doesn't check support directly.  It uses the venerable
> 'boot_cpu_data' which is a software approximation of the actual CPU
> support.  Unfortunately, Xen wants to set up '__supported_pte_mask'
> before 'boot_cpu_data' has been initialized.  It hacks around this
> problem by repeating some of the 'boot_cpu_data' setup *just* for
> NX.
> 
> Have x86_configure_nx() stop consulting 'boot_cpu_data' and move
> the NX detection to the caller.
> 
> No functional change.  That will come later.
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH 1/4] x86/cpu: Add and use new CPUID region helper

[Huang, Kai]

Nit:

> +
> +/* Returns true if the leaf exists and @value was populated */

						 ^ is ?

> +static inline bool get_cpuid_region_leaf(u32 leaf, enum cpuid_regs_idx reg,
> +					 u32 *value)
> +{
> +	u16 region = leaf >> 16;
> +	u32 regs[4];
> +
> +	if (cpuid_region_max_leaf(region) < leaf)
> +		return false;
> +
> +	cpuid(leaf, &regs[CPUID_EAX], &regs[CPUID_EBX],
> +	            &regs[CPUID_ECX], &regs[CPUID_EDX]);
> +
> +	*value = regs[reg];
> +
> +	return true;
> +}

I found despite the get_cpuid_region_leaf() returns true/false, the return value
is never used in this series.  Instead, this series uses below pattern:

	u32 data = 0; 	/* explicit initialization */

	get_cpuid_region_leaf(leaf, ..., &data);

Which kinda implies the 'data' won't be touched if the requested leaf isn't
supported I suppose?

Since the return value is never used, should we consider just making this
function void?

Re: [PATCH 1/4] x86/cpu: Add and use new CPUID region helper

[Dave Hansen]

On 3/25/24 05:24, Huang, Kai wrote:
> 
> Nit:
> 
>> +
>> +/* Returns true if the leaf exists and @value was populated */
> 
> 						 ^ is ?

It's a subtle difference, but I think it's better as I wrote it.
Returning true happens *after* the value _was_ populated.

>> +static inline bool get_cpuid_region_leaf(u32 leaf, enum cpuid_regs_idx reg,
>> +					 u32 *value)
>> +{
>> +	u16 region = leaf >> 16;
>> +	u32 regs[4];
>> +
>> +	if (cpuid_region_max_leaf(region) < leaf)
>> +		return false;
>> +
>> +	cpuid(leaf, &regs[CPUID_EAX], &regs[CPUID_EBX],
>> +	            &regs[CPUID_ECX], &regs[CPUID_EDX]);
>> +
>> +	*value = regs[reg];
>> +
>> +	return true;
>> +}
> 
> I found despite the get_cpuid_region_leaf() returns true/false, the return value
> is never used in this series.  Instead, this series uses below pattern:
> 
> 	u32 data = 0; 	/* explicit initialization */
> 
> 	get_cpuid_region_leaf(leaf, ..., &data);
> 
> Which kinda implies the 'data' won't be touched if the requested leaf isn't
> supported I suppose?
> 
> Since the return value is never used, should we consider just making this
> function void?

I certainly considered it.

But I do think that get_cpuid_region_leaf() looks a lot more obviously
correct and useful when it explicitly returns what it did, even if the
existing callers don't take advantage of it.

I suspect it generates the same code either way.

Re: [PATCH 1/4] x86/cpu: Add and use new CPUID region helper

[Huang, Kai]

On Tue, 2024-04-02 at 10:13 -0700, Dave Hansen wrote:
> On 3/25/24 05:24, Huang, Kai wrote:
> > 
> > Nit:
> > 
> > > +
> > > +/* Returns true if the leaf exists and @value was populated */
> > 
> > 						 ^ is ?
> 
> It's a subtle difference, but I think it's better as I wrote it.
> Returning true happens *after* the value _was_ populated.
> 
> > > +static inline bool get_cpuid_region_leaf(u32 leaf, enum cpuid_regs_idx reg,
> > > +					 u32 *value)
> > > +{
> > > +	u16 region = leaf >> 16;
> > > +	u32 regs[4];
> > > +
> > > +	if (cpuid_region_max_leaf(region) < leaf)
> > > +		return false;
> > > +
> > > +	cpuid(leaf, &regs[CPUID_EAX], &regs[CPUID_EBX],
> > > +	            &regs[CPUID_ECX], &regs[CPUID_EDX]);
> > > +
> > > +	*value = regs[reg];
> > > +
> > > +	return true;
> > > +}
> > 
> > I found despite the get_cpuid_region_leaf() returns true/false, the return value
> > is never used in this series.  Instead, this series uses below pattern:
> > 
> > 	u32 data = 0; 	/* explicit initialization */
> > 
> > 	get_cpuid_region_leaf(leaf, ..., &data);
> > 
> > Which kinda implies the 'data' won't be touched if the requested leaf isn't
> > supported I suppose?
> > 
> > Since the return value is never used, should we consider just making this
> > function void?
> 
> I certainly considered it.
> 
> But I do think that get_cpuid_region_leaf() looks a lot more obviously
> correct and useful when it explicitly returns what it did, even if the
> existing callers don't take advantage of it.
> 
> I suspect it generates the same code either way.

Agreed:

Reviewed-by: Kai Huang <kai.huang@intel.com>

[PATCH 1/4] x86/cpu: Add and use new CPUID region helper

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

There are some (before now) unwritten rules about CPUID "regions".
Basically, there is a 32-bit address space of CPUID leaves.  The
top 16 bits address a "region" and the first leaf in a region
is special.

The kernel only has a few spots that care about this, but it's
rather hard to make sense of the code as is.

Add a helper that explains regions.  Use it where applicable.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Kai Huang <kai.huang@intel.com>
Cc: Juergen Gross <jgross@suse.com>

--

changes from v1:
 * Fix typo comment and whitespace gunk noted by Ingo

---

 b/arch/x86/include/asm/cpuid.h    |   59 ++++++++++++++++++++++++++++++++++++++
 b/arch/x86/kernel/cpu/common.c    |   13 +++-----
 b/arch/x86/kernel/cpu/transmeta.c |    9 +----
 b/arch/x86/xen/enlighten_pv.c     |    9 +----
 4 files changed, 69 insertions(+), 21 deletions(-)

diff -puN arch/x86/include/asm/cpuid.h~cpuid-regions arch/x86/include/asm/cpuid.h
--- a/arch/x86/include/asm/cpuid.h~cpuid-regions	2024-04-02 15:22:58.838912075 -0700
+++ b/arch/x86/include/asm/cpuid.h	2024-04-02 15:22:58.846912085 -0700
@@ -168,4 +168,63 @@ static inline uint32_t hypervisor_cpuid_
 	return 0;
 }
 
+/*
+ * By convention, CPUID is broken up into regions which each
+ * have 2^16 leaves.  EAX in the first leaf of each valid
+ * region returns the maximum valid leaf in that region.
+ *
+ * The regions can be thought of as being vendor-specific
+ * areas of CPUID, but that's imprecise because everybody
+ * implements the "Intel" region and Intel implements the
+ * AMD region.  There are a few well-known regions:
+ *  - Intel	(0x0000)
+ *  - AMD	(0x8000)
+ *  - Transmeta	(0x8086)
+ *  - Centaur	(0xC000)
+ *
+ * Consider a CPU where the maximum leaf in the Transmeta
+ * region is 2.  On such a CPU, leaf 0x80860000 would contain:
+ * EAX==0x80860002.
+ * region-^^^^
+ *   max leaf-^^^^
+ */
+static inline u32 cpuid_region_max_leaf(u16 region)
+{
+	u32 eax = cpuid_eax(region << 16);
+
+	/*
+	 * An unsupported region may return data from the last
+	 * "basic" leaf, which is essentially garbage.  Avoid
+	 * mistaking basic leaf data for region data.
+	 *
+	 * Note: this is not perfect.  It is theoretically
+	 * possible for the last basic leaf to _resemble_ a
+	 * valid first leaf from a region that doesn't exist.
+	 * But Intel at least seems to pad out the basic region
+	 * with 0's, possibly to avoid this.
+	 */
+	if ((eax >> 16) != region)
+		return 0;
+
+	return eax;
+}
+
+/* Returns true if the leaf exists and @value was populated */
+static inline bool get_cpuid_region_leaf(u32 leaf, enum cpuid_regs_idx reg,
+					 u32 *value)
+{
+	u16 region = leaf >> 16;
+	u32 regs[4];
+
+	if (cpuid_region_max_leaf(region) < leaf)
+		return false;
+
+	cpuid(leaf, &regs[CPUID_EAX], &regs[CPUID_EBX],
+	            &regs[CPUID_ECX], &regs[CPUID_EDX]);
+
+	*value = regs[reg];
+
+	return true;
+}
+
 #endif /* _ASM_X86_CPUID_H */
diff -puN arch/x86/kernel/cpu/common.c~cpuid-regions arch/x86/kernel/cpu/common.c
--- a/arch/x86/kernel/cpu/common.c~cpuid-regions	2024-04-02 15:22:58.842912079 -0700
+++ b/arch/x86/kernel/cpu/common.c	2024-04-02 15:22:58.846912085 -0700
@@ -1049,16 +1049,13 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 	}
 
 	/* AMD-defined flags: level 0x80000001 */
-	eax = cpuid_eax(0x80000000);
-	c->extended_cpuid_level = eax;
+	c->extended_cpuid_level = cpuid_region_max_leaf(0x8000);
 
-	if ((eax & 0xffff0000) == 0x80000000) {
-		if (eax >= 0x80000001) {
-			cpuid(0x80000001, &eax, &ebx, &ecx, &edx);
+	if (c->extended_cpuid_level >= 0x80000001) {
+		cpuid(0x80000001, &eax, &ebx, &ecx, &edx);
 
-			c->x86_capability[CPUID_8000_0001_ECX] = ecx;
-			c->x86_capability[CPUID_8000_0001_EDX] = edx;
-		}
+		c->x86_capability[CPUID_8000_0001_ECX] = ecx;
+		c->x86_capability[CPUID_8000_0001_EDX] = edx;
 	}
 
 	if (c->extended_cpuid_level >= 0x80000007) {
diff -puN arch/x86/kernel/cpu/transmeta.c~cpuid-regions arch/x86/kernel/cpu/transmeta.c
--- a/arch/x86/kernel/cpu/transmeta.c~cpuid-regions	2024-04-02 15:22:58.842912079 -0700
+++ b/arch/x86/kernel/cpu/transmeta.c	2024-04-02 15:22:58.846912085 -0700
@@ -9,14 +9,9 @@
 
 static void early_init_transmeta(struct cpuinfo_x86 *c)
 {
-	u32 xlvl;
-
 	/* Transmeta-defined flags: level 0x80860001 */
-	xlvl = cpuid_eax(0x80860000);
-	if ((xlvl & 0xffff0000) == 0x80860000) {
-		if (xlvl >= 0x80860001)
-			c->x86_capability[CPUID_8086_0001_EDX] = cpuid_edx(0x80860001);
-	}
+	get_cpuid_region_leaf(0x80860001, CPUID_EDX,
+			  &c->x86_capability[CPUID_8086_0001_EDX]);
 }
 
 static void init_transmeta(struct cpuinfo_x86 *c)
diff -puN arch/x86/xen/enlighten_pv.c~cpuid-regions arch/x86/xen/enlighten_pv.c
--- a/arch/x86/xen/enlighten_pv.c~cpuid-regions	2024-04-02 15:22:58.842912079 -0700
+++ b/arch/x86/xen/enlighten_pv.c	2024-04-03 08:34:28.221534043 -0700
@@ -141,16 +141,13 @@ static void __init xen_set_mtrr_data(voi
 	};
 	unsigned int reg;
 	unsigned long mask;
-	uint32_t eax, width;
+	uint32_t width;
 	static struct mtrr_var_range var[MTRR_MAX_VAR_RANGES] __initdata;
 
 	/* Get physical address width (only 64-bit cpus supported). */
 	width = 36;
-	eax = cpuid_eax(0x80000000);
-	if ((eax >> 16) == 0x8000 && eax >= 0x80000008) {
-		eax = cpuid_eax(0x80000008);
-		width = eax & 0xff;
-	}
+	/* Will overwrite 'width' if available in CPUID: */
+	get_cpuid_region_leaf(0x80000008, CPUID_EAX, &width);
 
 	for (reg = 0; reg < MTRR_MAX_VAR_RANGES; reg++) {
 		op.u.read_memtype.reg = reg;
_

Re: [PATCH 1/4] x86/cpu: Add and use new CPUID region helper

[Jürgen Groß]

On 03.04.24 17:35, Dave Hansen wrote:
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> There are some (before now) unwritten rules about CPUID "regions".
> Basically, there is a 32-bit address space of CPUID leaves.  The
> top 16 bits address a "region" and the first leaf in a region
> is special.
> 
> The kernel only has a few spots that care about this, but it's
> rather hard to make sense of the code as is.
> 
> Add a helper that explains regions.  Use it where applicable.
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Cc: Juergen Gross <jgross@suse.com>
> 
> --
> 
> changes from v1:
>   * Fix typo comment and whitespace gunk noted by Ingo
> 
> ---
> 
>   b/arch/x86/include/asm/cpuid.h    |   59 ++++++++++++++++++++++++++++++++++++++
>   b/arch/x86/kernel/cpu/common.c    |   13 +++-----
>   b/arch/x86/kernel/cpu/transmeta.c |    9 +----
>   b/arch/x86/xen/enlighten_pv.c     |    9 +----
>   4 files changed, 69 insertions(+), 21 deletions(-)
> 
> diff -puN arch/x86/include/asm/cpuid.h~cpuid-regions arch/x86/include/asm/cpuid.h
> --- a/arch/x86/include/asm/cpuid.h~cpuid-regions	2024-04-02 15:22:58.838912075 -0700
> +++ b/arch/x86/include/asm/cpuid.h	2024-04-02 15:22:58.846912085 -0700
> @@ -168,4 +168,63 @@ static inline uint32_t hypervisor_cpuid_
>   	return 0;
>   }
>   
> +/*
> + * By convention, CPUID is broken up into regions which each
> + * have 2^16 leaves.  EAX in the first leaf of each valid
> + * region returns the maximum valid leaf in that region.
> + *
> + * The regions can be thought of as being vendor-specific
> + * areas of CPUID, but that's imprecise because everybody
> + * implements the "Intel" region and Intel implements the
> + * AMD region.  There are a few well-known regions:
> + *  - Intel	(0x0000)
> + *  - AMD	(0x8000)
> + *  - Transmeta	(0x8086)
> + *  - Centaur	(0xC000)
> + *
> + * Consider a CPU where the maximum leaf in the Transmeta
> + * region is 2.  On such a CPU, leaf 0x80860000 would contain:
> + * EAX==0x80860002.
> + * region-^^^^
> + *   max leaf-^^^^
> + */
> +static inline u32 cpuid_region_max_leaf(u16 region)
> +{
> +	u32 eax = cpuid_eax(region << 16);
> +
> +	/*
> +	 * An unsupported region may return data from the last
> +	 * "basic" leaf, which is essentially garbage.  Avoid
> +	 * mistaking basic leaf data for region data.
> +	 *
> +	 * Note: this is not perfect.  It is theoretically
> +	 * possible for the last basic leaf to _resemble_ a
> +	 * valid first leaf from a region that doesn't exist.
> +	 * But Intel at least seems to pad out the basic region
> +	 * with 0's, possibly to avoid this.
> +	 */
> +	if ((eax >> 16) != region)
> +		return 0;
> +
> +	return eax;
> +}
> +
> +/* Returns true if the leaf exists and @value was populated */
> +static inline bool get_cpuid_region_leaf(u32 leaf, enum cpuid_regs_idx reg,
> +					 u32 *value)
> +{
> +	u16 region = leaf >> 16;
> +	u32 regs[4];
> +
> +	if (cpuid_region_max_leaf(region) < leaf)
> +		return false;
> +
> +	cpuid(leaf, &regs[CPUID_EAX], &regs[CPUID_EBX],
> +	            &regs[CPUID_ECX], &regs[CPUID_EDX]);
> +
> +	*value = regs[reg];
> +
> +	return true;
> +}
> +
>   #endif /* _ASM_X86_CPUID_H */
> diff -puN arch/x86/kernel/cpu/common.c~cpuid-regions arch/x86/kernel/cpu/common.c
> --- a/arch/x86/kernel/cpu/common.c~cpuid-regions	2024-04-02 15:22:58.842912079 -0700
> +++ b/arch/x86/kernel/cpu/common.c	2024-04-02 15:22:58.846912085 -0700
> @@ -1049,16 +1049,13 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
>   	}
>   
>   	/* AMD-defined flags: level 0x80000001 */
> -	eax = cpuid_eax(0x80000000);
> -	c->extended_cpuid_level = eax;
> +	c->extended_cpuid_level = cpuid_region_max_leaf(0x8000);
>   
> -	if ((eax & 0xffff0000) == 0x80000000) {
> -		if (eax >= 0x80000001) {
> -			cpuid(0x80000001, &eax, &ebx, &ecx, &edx);
> +	if (c->extended_cpuid_level >= 0x80000001) {
> +		cpuid(0x80000001, &eax, &ebx, &ecx, &edx);
>   
> -			c->x86_capability[CPUID_8000_0001_ECX] = ecx;
> -			c->x86_capability[CPUID_8000_0001_EDX] = edx;
> -		}
> +		c->x86_capability[CPUID_8000_0001_ECX] = ecx;
> +		c->x86_capability[CPUID_8000_0001_EDX] = edx;
>   	}
>   
>   	if (c->extended_cpuid_level >= 0x80000007) {
> diff -puN arch/x86/kernel/cpu/transmeta.c~cpuid-regions arch/x86/kernel/cpu/transmeta.c
> --- a/arch/x86/kernel/cpu/transmeta.c~cpuid-regions	2024-04-02 15:22:58.842912079 -0700
> +++ b/arch/x86/kernel/cpu/transmeta.c	2024-04-02 15:22:58.846912085 -0700
> @@ -9,14 +9,9 @@
>   
>   static void early_init_transmeta(struct cpuinfo_x86 *c)
>   {
> -	u32 xlvl;
> -
>   	/* Transmeta-defined flags: level 0x80860001 */
> -	xlvl = cpuid_eax(0x80860000);
> -	if ((xlvl & 0xffff0000) == 0x80860000) {
> -		if (xlvl >= 0x80860001)
> -			c->x86_capability[CPUID_8086_0001_EDX] = cpuid_edx(0x80860001);
> -	}
> +	get_cpuid_region_leaf(0x80860001, CPUID_EDX,
> +			  &c->x86_capability[CPUID_8086_0001_EDX]);
>   }
>   
>   static void init_transmeta(struct cpuinfo_x86 *c)
> diff -puN arch/x86/xen/enlighten_pv.c~cpuid-regions arch/x86/xen/enlighten_pv.c
> --- a/arch/x86/xen/enlighten_pv.c~cpuid-regions	2024-04-02 15:22:58.842912079 -0700
> +++ b/arch/x86/xen/enlighten_pv.c	2024-04-03 08:34:28.221534043 -0700
> @@ -141,16 +141,13 @@ static void __init xen_set_mtrr_data(voi
>   	};
>   	unsigned int reg;
>   	unsigned long mask;
> -	uint32_t eax, width;
> +	uint32_t width;
>   	static struct mtrr_var_range var[MTRR_MAX_VAR_RANGES] __initdata;
>   
>   	/* Get physical address width (only 64-bit cpus supported). */
>   	width = 36;
> -	eax = cpuid_eax(0x80000000);
> -	if ((eax >> 16) == 0x8000 && eax >= 0x80000008) {
> -		eax = cpuid_eax(0x80000008);
> -		width = eax & 0xff;
> -	}
> +	/* Will overwrite 'width' if available in CPUID: */
> +	get_cpuid_region_leaf(0x80000008, CPUID_EAX, &width);

Aren't you missing "width &= 0xff;" here? get_cpuid_region_leaf() doesn't
truncate the returned value, which has the linear address width at bits 8-15.


Juergen

Re: [PATCH 1/4] x86/cpu: Add and use new CPUID region helper

[Chang S. Bae]

On 3/22/2024 10:56 AM, Dave Hansen wrote:
> 
> diff -puN arch/x86/kernel/cpu/transmeta.c~cpuid-regions arch/x86/kernel/cpu/transmeta.c
> --- a/arch/x86/kernel/cpu/transmeta.c~cpuid-regions	2024-03-18 15:12:20.680308889 -0700
> +++ b/arch/x86/kernel/cpu/transmeta.c	2024-03-18 15:12:20.684309023 -0700
> @@ -9,14 +9,9 @@
>   
>   static void early_init_transmeta(struct cpuinfo_x86 *c)
>   {
> -	u32 xlvl;
> -
>   	/* Transmeta-defined flags: level 0x80860001 */
> -	xlvl = cpuid_eax(0x80860000);
> -	if ((xlvl & 0xffff0000) == 0x80860000) {
> -		if (xlvl >= 0x80860001)
> -			c->x86_capability[CPUID_8086_0001_EDX] = cpuid_edx(0x80860001);
> -	}
> +	get_cpuid_region_leaf(0x80860001, CPUID_EDX,
> +			  &c->x86_capability[CPUID_8086_0001_EDX]);

Just nitpicking one minor thing:

CHECK: Alignment should match open parenthesis
#136: FILE: arch/x86/kernel/cpu/transmeta.c:14:
+	get_cpuid_region_leaf(0x80860001, CPUID_EDX,
+			  &c->x86_capability[CPUID_8086_0001_EDX]);

Thanks,
Chang

Re: [PATCH 1/4] x86/cpu: Add and use new CPUID region helper

[Borislav Petkov]

On Thu, Apr 04, 2024 at 11:04:49AM +0200, Jürgen Groß wrote:
> Aren't you missing "width &= 0xff;" here? get_cpuid_region_leaf() doesn't
> truncate the returned value, which has the linear address width at bits 8-15.

Yeah.

Other than that, the intention looks ok as it documents what that code
magic was.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette