Received-SPF: pass (google.com: domain of linux-kernel+bounces-131147-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <bab18bb7-ef6d-477a-845a-de9b52dd9591@suse.com>
Date: Thu, 4 Apr 2024 11:04:49 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/4] x86/cpu: Add and use new CPUID region helper
Content-Language: en-US
To: Dave Hansen <dave.hansen@linux.intel.com>, linux-kernel@vger.kernel.org
Cc: tglx@linutronix.de, x86@kernel.org, bp@alien8.de, kai.huang@intel.com
References: <20240403153508.7328E749@davehans-spike.ostc.intel.com>
 <20240403153510.F327603D@davehans-spike.ostc.intel.com>
From: =?UTF-8?B?SsO8cmdlbiBHcm/Dnw==?= <jgross@suse.com>
In-Reply-To: <20240403153510.F327603D@davehans-spike.ostc.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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