Received-SPF: pass (google.com: domain of linux-kernel+bounces-126941-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 1 Apr 2024 11:18:57 -0700
From: Tony Luck <tony.luck@intel.com>
To: Borislav Petkov <bp@alien8.de>
Cc: "x86@kernel.org" <x86@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 01/74] x86/cpu/vfm: Add/initialize x86_vfm field to
 struct cpuinfo_x86
Message-ID: <Zgr6kT8oULbnmEXx@agluck-desk3>
References: <20240328163746.243023-1-tony.luck@intel.com>
 <20240328163746.243023-2-tony.luck@intel.com>
 <20240328164811.GDZgWfSzAWZXO7dUky@fat_crate.local>
 <20240328165251.GEZgWgY1Clb9z4t3VX@fat_crate.local>
 <SJ1PR11MB6083AADC97E50462C1137D71FC3B2@SJ1PR11MB6083.namprd11.prod.outlook.com>
 <20240328171204.GGZgWk5JNOzQzoaEql@fat_crate.local>
 <SJ1PR11MB6083D8BB65748452B204ED8AFC3B2@SJ1PR11MB6083.namprd11.prod.outlook.com>
 <20240329114007.GAZgaolwSFtjHStiuL@fat_crate.local>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240329114007.GAZgaolwSFtjHStiuL@fat_crate.local>

On Fri, Mar 29, 2024 at 12:40:07PM +0100, Borislav Petkov wrote:
> Because from looking at your set, I don't see a slick way to check
> whether a concrete f/m/s tuple belongs to a range without involved
> checking.
> 
> For example, models:
> 
>                 case 0x30 ... 0x4f:
>                 case 0x60 ... 0x7f:
>                 case 0x90 ... 0x91:
>                 case 0xa0 ... 0xaf:
> 
> are all Zen2. I could do a X86_MATCH_VF_MODEL_RANGE and we even had
> a patch like that at some point but it didn't go in. But even if I did
> that, I'd still need to do x86_match_cpu() instead of the current
> X86_FEATURE_ZEN* checks we're doing.

I realized the problem with ranges is the order I put the bits into the
x86_vfm field. If I swap around to put the vendor in high bits, family
in the middle, model in low bits like this:

struct cpuinfo_x86 {
        union {
                struct {
                        __u8    x86_model;
                        __u8    x86;            /* CPU family */
                        __u8    x86_vendor;     /* CPU vendor */
                        __u8    x86_reserved;
                };
                __u32           x86_vfm;        /* combined vendor, family, model */
        };

Then ranges of models within (or across) familiies can work.  E.g. the
AMD Zen generation checking could be changed from:


	/* Figure out Zen generations: */
	switch (c->x86) {
	case 0x17:
		switch (c->x86_model) {
		case 0x00 ... 0x2f:
		case 0x50 ... 0x5f:
			setup_force_cpu_cap(X86_FEATURE_ZEN1);
			break;
		case 0x30 ... 0x4f:
		case 0x60 ... 0x7f:
		case 0x90 ... 0x91:
		case 0xa0 ... 0xaf:
			setup_force_cpu_cap(X86_FEATURE_ZEN2);
			break;
		default:
			goto warn;
		}
		break;

	case 0x19:
		switch (c->x86_model) {
		case 0x00 ... 0x0f:
		case 0x20 ... 0x5f:
			setup_force_cpu_cap(X86_FEATURE_ZEN3);
			break;
		case 0x10 ... 0x1f:
		case 0x60 ... 0xaf:
			setup_force_cpu_cap(X86_FEATURE_ZEN4);
			break;
		default:
			goto warn;
		}
		break;

	case 0x1a:
		switch (c->x86_model) {
		case 0x00 ... 0x0f:
		case 0x20 ... 0x2f:
		case 0x40 ... 0x4f:
		case 0x70 ... 0x7f:
			setup_force_cpu_cap(X86_FEATURE_ZEN5);
			break;
		default:
			goto warn;
		}
		break;

	default:
		break;
	}

to:

	/* Figure out Zen generations: */
	switch (c->x86_vfm) {
	case AFM(0x17, 0x00) ... AFM(0x17, 0x2f):
	case AFM(0x17, 0x50) ... AFM(0x17, 0x5f):
		setup_force_cpu_cap(X86_FEATURE_ZEN1);
		break;
	case AFM(0x17, 0x30) ... AFM(0x17, 0x4f):
	case AFM(0x17, 0x60) ... AFM(0x17, 0x7f):
	case AFM(0x17, 0x90) ... AFM(0x17, 0x91):
	case AFM(0x17, 0xa0) ... AFM(0x17, 0xaf):
		setup_force_cpu_cap(X86_FEATURE_ZEN2);
		break;
	case AFM(0x19, 0x00) ... AFM(0x19, 0x0f):
	case AFM(0x19, 0x20) ... AFM(0x19, 0x5f):
		setup_force_cpu_cap(X86_FEATURE_ZEN3);
		break;
	case AFM(0x19, 0x10) ... AFM(0x19, 0x1f):
	case AFM(0x19, 0x60) ... AFM(0x19, 0xaf):
		setup_force_cpu_cap(X86_FEATURE_ZEN4);
		break;
	case AFM(0x1a, 0x00) ... AFM(0x1a, 0x0f):
	case AFM(0x1a, 0x20) ... AFM(0x1a, 0x2f):
	case AFM(0x1a, 0x40) ... AFM(0x1a, 0x4f):
	case AFM(0x1a, 0x70) ... AFM(0x1a, 0x7f):
		setup_force_cpu_cap(X86_FEATURE_ZEN5);
		break;
	default:
		goto warn;
	}


That's more visually more compact, but maybe not any more readable.
But you would have the *option* to do this.

I'll post V2 of parts 1 & 2 with the re-ordered fields. None of the rest
of the patches need to change.

-Tony