Received-SPF: pass (google.com: domain of linux-kernel+bounces-112582-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Sender: Ingo Molnar <mingo.kernel.org@gmail.com>
Date: Sun, 24 Mar 2024 05:38:02 +0100
From: Ingo Molnar <mingo@kernel.org>
To: Dave Hansen <dave.hansen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org, jgross@suse.com, tglx@linutronix.de,
	x86@kernel.org, bp@alien8.de
Subject: Re: [PATCH 1/4] x86/cpu: Add and use new CPUID region helper
Message-ID: <Zf+uKi42qFoHLAUg@gmail.com>
References: <20240322175629.01E8B39D@davehans-spike.ostc.intel.com>
 <20240322175630.72CE974F@davehans-spike.ostc.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240322175630.72CE974F@davehans-spike.ostc.intel.com>


* 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