Received-SPF: pass (google.com: domain of linux-kernel+bounces-182221-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Fri, 17 May 2024 16:43:12 +0200
From: Borislav Petkov <bp@alien8.de>
To: Tony Luck <tony.luck@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
	Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
	"H. Peter Anvin" <hpa@zytor.com>,
	"Peter Zijlstra (Intel)" <peterz@infradead.org>,
	Uros Bizjak <ubizjak@gmail.com>,
	Rick Edgecombe <rick.p.edgecombe@intel.com>,
	Arnd Bergmann <arnd@arndb.de>, Mateusz Guzik <mjguzik@gmail.com>,
	Thomas Renninger <trenn@suse.de>,
	Greg Kroah-Hartman <gregkh@suse.de>,
	Andi Kleen <ak@linux.intel.com>, linux-kernel@vger.kernel.org,
	patches@lists.linux.dev
Subject: Re: [PATCH v2] x86/cpu: Fix x86_match_cpu() to match just
 X86_VENDOR_INTEL
Message-ID: <20240517144312.GBZkdtAOuJZCvxhFbJ@fat_crate.local>
References: <20240516162925.79245-1-tony.luck@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240516162925.79245-1-tony.luck@intel.com>

On Thu, May 16, 2024 at 09:29:25AM -0700, Tony Luck wrote:
> -#define X86_VENDOR_INTEL	0
>  #define X86_VENDOR_CYRIX	1
>  #define X86_VENDOR_AMD		2
>  #define X86_VENDOR_UMC		3
> +#define X86_VENDOR_INTEL	4
>  #define X86_VENDOR_CENTAUR	5
>  #define X86_VENDOR_TRANSMETA	7
>  #define X86_VENDOR_NSC		8
> -- 

From my last review:

> But the Intel vendor has been 0 for what, 30 years?

> Are you sure no other code in the tree relies on that? Audited?

But nope, apparently that's too much to ask. :-(

modinfo ./arch/x86/events/intel/intel-uncore.ko
filename:       ./arch/x86/events/intel/intel-uncore.ko
license:        GPL
srcversion:     ECE38449B18DD83223B93FD
alias:          cpu:type:x86,ven0000fam0006mod00B6:feature:*
alias:          cpu:type:x86,ven0000fam0006mod00AF:feature:*
alias:          cpu:type:x86,ven0000fam0006mod00BE:feature:*
			     ^^^^^^^^^

Would everything still work if it said "ven0004" now?

So tglx and I just did some poking and we think the best solution would
be to add a __u16 flags field to struct x86_cpu_id right...

struct x86_cpu_id {
        __u16                      vendor;               /*     0     2 */
        __u16                      family;               /*     2     2 */
        __u16                      model;                /*     4     2 */
        __u16                      steppings;            /*     6     2 */
        __u16                      feature;              /*     8     2 */

        /* XXX 6 bytes hole, try to pack */

<--- HERE

        kernel_ulong_t             driver_data;          /*    16     8 */

        /* size: 24, cachelines: 1, members: 6 */
        /* sum members: 18, holes: 1, sum holes: 6 */
        /* last cacheline: 24 bytes */
};

and the 32-bit version has the same hole:

struct x86_cpu_id {
        __u16                      vendor;               /*     0     2 */
        __u16                      family;               /*     2     2 */
        __u16                      model;                /*     4     2 */
        __u16                      steppings;            /*     6     2 */
        __u16                      feature;              /*     8     2 */

        /* XXX 2 bytes hole, try to pack */

<--- HERE

        kernel_ulong_t             driver_data;          /*    12     4 */

        /* size: 16, cachelines: 1, members: 6 */
        /* sum members: 14, holes: 1, sum holes: 2 */
        /* last cacheline: 16 bytes */
};

And then do:

struct x86_cpu_id {
        __u16 vendor;
        __u16 family;
        __u16 model;
        __u16 steppings;
        __u16 feature;  /* bit index */
	__u16 flags;
        kernel_ulong_t driver_data;
};

#define X86_CPU_ID_FLAG_VENDOR_VALID		BIT(0)

and then have the macros in arch/x86/include/asm/cpu_device_id.h set
that valid flag and then have x86_match_cpu() check it.

Then you don't risk a userspace breakage and that x86_match_cpu() crap
thing is fixed.

Thx.

-- 
Regards/Gruss,
    Boris.

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