[PATCH 0/3] x86: Overhaul of vendor/family/model logic to support DMR

[PATCH 0/3] x86: Overhaul of vendor/family/model logic to support DMR

[Andrew Cooper]

Intel have run out of model space in Family 6 and will start using Family 19
starting with Diamond Rapids.  Xen, like Linux, has model checking logic which
will malfunction owing to bad assumptions about the family field.

Follow Linux's lead and combine Vendor / Family / Model into a single
constant, so it is harder to mismatch family and model checks.

https://gitlab.com/xen-project/hardware/xen-staging/-/pipelines/1930124712

Andrew Cooper (3):
  x86: Rearrange struct cpuinfo_x86 to introduce a vfm field
  x86/intel-family: Resync with Linux
  x86/vtd: Switch model check to VFM

 xen/arch/x86/include/asm/cpufeature.h   | 45 ++++++++++--
 xen/arch/x86/include/asm/intel-family.h | 96 ++++++++++++++++++++++++-
 xen/arch/x86/setup.c                    |  4 +-
 xen/drivers/passthrough/vtd/quirks.c    |  7 +-
 4 files changed, 141 insertions(+), 11 deletions(-)

-- 
2.39.5

[PATCH 1/3] x86: Rearrange struct cpuinfo_x86 to introduce a vfm field

[Andrew Cooper]

Intel have run out of model space in Family 6 and will start using Family 19
starting with Diamond Rapids.  Xen, like Linux, has model checking logic which
will malfunction owing to bad assumptions about the family field.

Reorder the family, vendor and model fields so they can be accessed together
as a single vfm field.

As we're cleaning up the logic, take the opportunity to introduce better
names, dropping the x86 prefix.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/include/asm/cpufeature.h | 28 +++++++++++++++++++++++----
 xen/arch/x86/setup.c                  |  4 +++-
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h
index 3c2ac964e410..707b134c09c7 100644
--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
@@ -20,10 +20,30 @@
 #ifndef __ASSEMBLY__
 
 struct cpuinfo_x86 {
-    unsigned char x86;                 /* CPU family */
-    unsigned char x86_vendor;          /* CPU vendor */
-    unsigned char x86_model;
-    unsigned char x86_mask;
+    /* TODO: Phase out the x86 prefixed names. */
+    union {
+        struct {
+            union {
+                uint8_t x86_model;
+                uint8_t model;
+            };
+            union {
+                uint8_t x86;
+                uint8_t family;
+            };
+            union {
+                uint8_t x86_vendor;
+                uint8_t vendor;
+            };
+            uint8_t _rsvd;
+        };
+        uint32_t vfm;                  /* Vendor Family Model */
+    };
+    union {
+        uint8_t x86_mask;
+        uint8_t stepping;
+    };
+
     unsigned int cpuid_level;          /* Maximum supported CPUID level */
     unsigned int extended_cpuid_level; /* Maximum supported CPUID extended level */
     unsigned int x86_capability[NCAPINTS];
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 24e4f5ac7f5d..37421ac9d05b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -178,7 +178,9 @@ void *stack_start = cpu0_stack + STACK_SIZE - sizeof(struct cpu_info);
 /* Used by the boot asm and EFI to stash the multiboot_info paddr. */
 unsigned int __initdata multiboot_ptr;
 
-struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 };
+struct cpuinfo_x86 __read_mostly boot_cpu_data = {
+    .cpuid_level = -1,
+};
 
 unsigned long __read_mostly mmu_cr4_features = XEN_MINIMAL_CR4;
 
-- 
2.39.5

[PATCH 2/3] x86/intel-family: Resync with Linux

[Andrew Cooper]

This snapshot is prior to Linux commit db4001f9cc32 ("x86/cpu/vfm: Delete all
the *_FAM6_ CPU #defines") at the end of their conversion phase.

In addition to non-FAM6 infixed names, defines are added for the Pentium Pro,
ArrowLake U, and reintroduced the PHI defines which were incorrectly deleted
in the past.

In cpufeature.h, provide VFM_* macros to transform constants to/from the
cpuinfo_x86 representation.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

I meant to object to deleting PHI at the time, but was too late.  Just because
Xen has stopped supporting the PHI doesn't mean the model numbers have ceased
existing.
---
 xen/arch/x86/include/asm/cpufeature.h   | 17 +++++
 xen/arch/x86/include/asm/intel-family.h | 96 ++++++++++++++++++++++++-
 2 files changed, 111 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h
index 707b134c09c7..ba2c1c1c7416 100644
--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
@@ -8,6 +8,8 @@
 
 #include <xen/cache.h>
 #include <xen/const.h>
+#include <xen/macros.h>
+
 #include <asm/cpuid.h>
 
 #define cpufeat_word(idx)	((idx) / 32)
@@ -17,6 +19,21 @@
 /* An alias of a feature we know is always going to be present. */
 #define X86_FEATURE_ALWAYS      X86_FEATURE_LM
 
+/*
+ * Layout tied to cpuinfo_x86.vfm
+ */
+#define VFM_MODEL_MASK  0x000000ff
+#define VFM_FAMILY_MASK 0x0000ff00
+#define VFM_VENDOR_MASK 0x00ff0000
+
+#define VFM_MAKE(v, f, m) (MASK_INSR(v, VFM_VENDOR_MASK) | \
+                           MASK_INSR(f, VFM_FAMILY_MASK) | \
+                           MASK_INSR(f, VFM_MODEL_MASK))
+
+#define VFM_MODEL(vfm)  MASK_EXTR(vfm, VFM_MODEL_MASK)
+#define VFM_FAMILY(vfm) MASK_EXTR(vfm, VFM_FAMILY_MASK)
+#define VFM_VENDOR(vfm) MASK_EXTR(vfm, VFM_VENDOR_MASK)
+
 #ifndef __ASSEMBLY__
 
 struct cpuinfo_x86 {
diff --git a/xen/arch/x86/include/asm/intel-family.h b/xen/arch/x86/include/asm/intel-family.h
index ab20cce12492..5858e7398570 100644
--- a/xen/arch/x86/include/asm/intel-family.h
+++ b/xen/arch/x86/include/asm/intel-family.h
@@ -13,8 +13,8 @@
  *	INTEL_FAM6{OPTFAMILY}_{MICROARCH}{OPTDIFF}
  * where:
  * OPTFAMILY	Describes the family of CPUs that this belongs to. Default
- *		is assumed to be "_CORE" (and should be omitted). The other
- *		value currently in use is _ATOM.
+ *		is assumed to be "_CORE" (and should be omitted). Other values
+ *		currently in use are _ATOM and _XEON_PHI
  * MICROARCH	Is the code name for the micro-architecture for this core.
  *		N.B. Not the platform name.
  * OPTDIFF	If needed, a short string to differentiate by market segment.
@@ -40,131 +40,223 @@
  * their own names :-(
  */
 
+#define IFM(_fam, _model)	VFM_MAKE(X86_VENDOR_INTEL, _fam, _model)
+
 /* Wildcard match for FAM6 so X86_MATCH_INTEL_FAM6_MODEL(ANY) works */
 #define INTEL_FAM6_ANY			X86_MODEL_ANY
+/* Wildcard match for FAM6 so X86_MATCH_VFM(ANY) works */
+#define INTEL_ANY			IFM(X86_FAMILY_ANY, X86_MODEL_ANY)
+
+#define INTEL_PENTIUM_PRO		IFM(6, 0x01)
 
 #define INTEL_FAM6_CORE_YONAH		0x0E
+#define INTEL_CORE_YONAH		IFM(6, 0x0E)
 
 #define INTEL_FAM6_CORE2_MEROM		0x0F
+#define INTEL_CORE2_MEROM		IFM(6, 0x0F)
 #define INTEL_FAM6_CORE2_MEROM_L	0x16
+#define INTEL_CORE2_MEROM_L		IFM(6, 0x16)
 #define INTEL_FAM6_CORE2_PENRYN		0x17
+#define INTEL_CORE2_PENRYN		IFM(6, 0x17)
 #define INTEL_FAM6_CORE2_DUNNINGTON	0x1D
+#define INTEL_CORE2_DUNNINGTON		IFM(6, 0x1D)
 
 #define INTEL_FAM6_NEHALEM		0x1E
+#define INTEL_NEHALEM			IFM(6, 0x1E)
 #define INTEL_FAM6_NEHALEM_G		0x1F /* Auburndale / Havendale */
+#define INTEL_NEHALEM_G			IFM(6, 0x1F) /* Auburndale / Havendale */
 #define INTEL_FAM6_NEHALEM_EP		0x1A
+#define INTEL_NEHALEM_EP		IFM(6, 0x1A)
 #define INTEL_FAM6_NEHALEM_EX		0x2E
+#define INTEL_NEHALEM_EX		IFM(6, 0x2E)
 
 #define INTEL_FAM6_WESTMERE		0x25
+#define INTEL_WESTMERE			IFM(6, 0x25)
 #define INTEL_FAM6_WESTMERE_EP		0x2C
+#define INTEL_WESTMERE_EP		IFM(6, 0x2C)
 #define INTEL_FAM6_WESTMERE_EX		0x2F
+#define INTEL_WESTMERE_EX		IFM(6, 0x2F)
 
 #define INTEL_FAM6_SANDYBRIDGE		0x2A
+#define INTEL_SANDYBRIDGE		IFM(6, 0x2A)
 #define INTEL_FAM6_SANDYBRIDGE_X	0x2D
+#define INTEL_SANDYBRIDGE_X		IFM(6, 0x2D)
 #define INTEL_FAM6_IVYBRIDGE		0x3A
+#define INTEL_IVYBRIDGE			IFM(6, 0x3A)
 #define INTEL_FAM6_IVYBRIDGE_X		0x3E
+#define INTEL_IVYBRIDGE_X		IFM(6, 0x3E)
 
 #define INTEL_FAM6_HASWELL		0x3C
+#define INTEL_HASWELL			IFM(6, 0x3C)
 #define INTEL_FAM6_HASWELL_X		0x3F
+#define INTEL_HASWELL_X			IFM(6, 0x3F)
 #define INTEL_FAM6_HASWELL_L		0x45
+#define INTEL_HASWELL_L			IFM(6, 0x45)
 #define INTEL_FAM6_HASWELL_G		0x46
+#define INTEL_HASWELL_G			IFM(6, 0x46)
 
 #define INTEL_FAM6_BROADWELL		0x3D
+#define INTEL_BROADWELL			IFM(6, 0x3D)
 #define INTEL_FAM6_BROADWELL_G		0x47
+#define INTEL_BROADWELL_G		IFM(6, 0x47)
 #define INTEL_FAM6_BROADWELL_X		0x4F
+#define INTEL_BROADWELL_X		IFM(6, 0x4F)
 #define INTEL_FAM6_BROADWELL_D		0x56
+#define INTEL_BROADWELL_D		IFM(6, 0x56)
 
 #define INTEL_FAM6_SKYLAKE_L		0x4E	/* Sky Lake             */
+#define INTEL_SKYLAKE_L			IFM(6, 0x4E) /* Sky Lake */
 #define INTEL_FAM6_SKYLAKE		0x5E	/* Sky Lake             */
+#define INTEL_SKYLAKE			IFM(6, 0x5E) /* Sky Lake */
 #define INTEL_FAM6_SKYLAKE_X		0x55	/* Sky Lake             */
+#define INTEL_SKYLAKE_X			IFM(6, 0x55) /* Sky Lake */
 /*                 CASCADELAKE_X	0x55	   Sky Lake -- s: 7     */
 /*                 COOPERLAKE_X		0x55	   Sky Lake -- s: 11    */
 
 #define INTEL_FAM6_KABYLAKE_L		0x8E	/* Sky Lake             */
+#define INTEL_KABYLAKE_L		IFM(6, 0x8E) /* Sky Lake */
 /*                 AMBERLAKE_L		0x8E	   Sky Lake -- s: 9     */
 /*                 COFFEELAKE_L		0x8E	   Sky Lake -- s: 10    */
 /*                 WHISKEYLAKE_L	0x8E       Sky Lake -- s: 11,12 */
 
 #define INTEL_FAM6_KABYLAKE		0x9E	/* Sky Lake             */
+#define INTEL_KABYLAKE			IFM(6, 0x9E) /* Sky Lake */
 /*                 COFFEELAKE		0x9E	   Sky Lake -- s: 10-13 */
 
 #define INTEL_FAM6_COMETLAKE		0xA5	/* Sky Lake             */
+#define INTEL_COMETLAKE			IFM(6, 0xA5) /* Sky Lake */
 #define INTEL_FAM6_COMETLAKE_L		0xA6	/* Sky Lake             */
+#define INTEL_COMETLAKE_L		IFM(6, 0xA6) /* Sky Lake */
 
 #define INTEL_FAM6_CANNONLAKE_L		0x66	/* Palm Cove */
+#define INTEL_CANNONLAKE_L		IFM(6, 0x66) /* Palm Cove */
 
 #define INTEL_FAM6_ICELAKE_X		0x6A	/* Sunny Cove */
+#define INTEL_ICELAKE_X			IFM(6, 0x6A) /* Sunny Cove */
 #define INTEL_FAM6_ICELAKE_D		0x6C	/* Sunny Cove */
+#define INTEL_ICELAKE_D			IFM(6, 0x6C) /* Sunny Cove */
 #define INTEL_FAM6_ICELAKE		0x7D	/* Sunny Cove */
+#define INTEL_ICELAKE			IFM(6, 0x7D) /* Sunny Cove */
 #define INTEL_FAM6_ICELAKE_L		0x7E	/* Sunny Cove */
+#define INTEL_ICELAKE_L			IFM(6, 0x7E) /* Sunny Cove */
 #define INTEL_FAM6_ICELAKE_NNPI		0x9D	/* Sunny Cove */
+#define INTEL_ICELAKE_NNPI		IFM(6, 0x9D) /* Sunny Cove */
 
 #define INTEL_FAM6_ROCKETLAKE		0xA7	/* Cypress Cove */
+#define INTEL_ROCKETLAKE		IFM(6, 0xA7) /* Cypress Cove */
 
 #define INTEL_FAM6_TIGERLAKE_L		0x8C	/* Willow Cove */
+#define INTEL_TIGERLAKE_L		IFM(6, 0x8C) /* Willow Cove */
 #define INTEL_FAM6_TIGERLAKE		0x8D	/* Willow Cove */
+#define INTEL_TIGERLAKE			IFM(6, 0x8D) /* Willow Cove */
 
 #define INTEL_FAM6_SAPPHIRERAPIDS_X	0x8F	/* Golden Cove */
+#define INTEL_SAPPHIRERAPIDS_X		IFM(6, 0x8F) /* Golden Cove */
 
 #define INTEL_FAM6_EMERALDRAPIDS_X	0xCF
+#define INTEL_EMERALDRAPIDS_X		IFM(6, 0xCF)
 
 #define INTEL_FAM6_GRANITERAPIDS_X	0xAD
+#define INTEL_GRANITERAPIDS_X		IFM(6, 0xAD)
 #define INTEL_FAM6_GRANITERAPIDS_D	0xAE
+#define INTEL_GRANITERAPIDS_D		IFM(6, 0xAE)
 
 /* "Hybrid" Processors (P-Core/E-Core) */
 
 #define INTEL_FAM6_LAKEFIELD		0x8A	/* Sunny Cove / Tremont */
+#define INTEL_LAKEFIELD			IFM(6, 0x8A) /* Sunny Cove / Tremont */
 
 #define INTEL_FAM6_ALDERLAKE		0x97	/* Golden Cove / Gracemont */
+#define INTEL_ALDERLAKE			IFM(6, 0x97) /* Golden Cove / Gracemont */
 #define INTEL_FAM6_ALDERLAKE_L		0x9A	/* Golden Cove / Gracemont */
+#define INTEL_ALDERLAKE_L		IFM(6, 0x9A) /* Golden Cove / Gracemont */
 
 #define INTEL_FAM6_RAPTORLAKE		0xB7	/* Raptor Cove / Enhanced Gracemont */
+#define INTEL_RAPTORLAKE		IFM(6, 0xB7) /* Raptor Cove / Enhanced Gracemont */
 #define INTEL_FAM6_RAPTORLAKE_P		0xBA
+#define INTEL_RAPTORLAKE_P		IFM(6, 0xBA)
 #define INTEL_FAM6_RAPTORLAKE_S		0xBF
+#define INTEL_RAPTORLAKE_S		IFM(6, 0xBF)
 
 #define INTEL_FAM6_METEORLAKE		0xAC
+#define INTEL_METEORLAKE		IFM(6, 0xAC)
 #define INTEL_FAM6_METEORLAKE_L		0xAA
+#define INTEL_METEORLAKE_L		IFM(6, 0xAA)
 
 #define INTEL_FAM6_ARROWLAKE_H		0xC5
+#define INTEL_ARROWLAKE_H		IFM(6, 0xC5)
 #define INTEL_FAM6_ARROWLAKE		0xC6
+#define INTEL_ARROWLAKE			IFM(6, 0xC6)
+#define INTEL_FAM6_ARROWLAKE_U		0xB5
+#define INTEL_ARROWLAKE_U		IFM(6, 0xB5)
 
 #define INTEL_FAM6_LUNARLAKE_M		0xBD
+#define INTEL_LUNARLAKE_M		IFM(6, 0xBD)
 
 /* "Small Core" Processors (Atom/E-Core) */
 
 #define INTEL_FAM6_ATOM_BONNELL		0x1C /* Diamondville, Pineview */
+#define INTEL_ATOM_BONNELL		IFM(6, 0x1C) /* Diamondville, Pineview */
 #define INTEL_FAM6_ATOM_BONNELL_MID	0x26 /* Silverthorne, Lincroft */
+#define INTEL_ATOM_BONNELL_MID		IFM(6, 0x26) /* Silverthorne, Lincroft */
 
 #define INTEL_FAM6_ATOM_SALTWELL	0x36 /* Cedarview */
+#define INTEL_ATOM_SALTWELL		IFM(6, 0x36) /* Cedarview */
 #define INTEL_FAM6_ATOM_SALTWELL_MID	0x27 /* Penwell */
+#define INTEL_ATOM_SALTWELL_MID		IFM(6, 0x27) /* Penwell */
 #define INTEL_FAM6_ATOM_SALTWELL_TABLET	0x35 /* Cloverview */
+#define INTEL_ATOM_SALTWELL_TABLET	IFM(6, 0x35) /* Cloverview */
 
 #define INTEL_FAM6_ATOM_SILVERMONT	0x37 /* Bay Trail, Valleyview */
+#define INTEL_ATOM_SILVERMONT		IFM(6, 0x37) /* Bay Trail, Valleyview */
 #define INTEL_FAM6_ATOM_SILVERMONT_D	0x4D /* Avaton, Rangely */
+#define INTEL_ATOM_SILVERMONT_D		IFM(6, 0x4D) /* Avaton, Rangely */
 #define INTEL_FAM6_ATOM_SILVERMONT_MID	0x4A /* Merriefield */
+#define INTEL_ATOM_SILVERMONT_MID	IFM(6, 0x4A) /* Merriefield */
 
 #define INTEL_FAM6_ATOM_AIRMONT		0x4C /* Cherry Trail, Braswell */
+#define INTEL_ATOM_AIRMONT		IFM(6, 0x4C) /* Cherry Trail, Braswell */
 #define INTEL_FAM6_ATOM_AIRMONT_MID	0x5A /* Moorefield */
+#define INTEL_ATOM_AIRMONT_MID		IFM(6, 0x5A) /* Moorefield */
 #define INTEL_FAM6_ATOM_AIRMONT_NP	0x75 /* Lightning Mountain */
+#define INTEL_ATOM_AIRMONT_NP		IFM(6, 0x75) /* Lightning Mountain */
 
 #define INTEL_FAM6_ATOM_GOLDMONT	0x5C /* Apollo Lake */
+#define INTEL_ATOM_GOLDMONT		IFM(6, 0x5C) /* Apollo Lake */
 #define INTEL_FAM6_ATOM_GOLDMONT_D	0x5F /* Denverton */
+#define INTEL_ATOM_GOLDMONT_D		IFM(6, 0x5F) /* Denverton */
 
 /* Note: the micro-architecture is "Goldmont Plus" */
 #define INTEL_FAM6_ATOM_GOLDMONT_PLUS	0x7A /* Gemini Lake */
+#define INTEL_ATOM_GOLDMONT_PLUS	IFM(6, 0x7A) /* Gemini Lake */
 
 #define INTEL_FAM6_ATOM_TREMONT_D	0x86 /* Jacobsville */
+#define INTEL_ATOM_TREMONT_D		IFM(6, 0x86) /* Jacobsville */
 #define INTEL_FAM6_ATOM_TREMONT		0x96 /* Elkhart Lake */
+#define INTEL_ATOM_TREMONT		IFM(6, 0x96) /* Elkhart Lake */
 #define INTEL_FAM6_ATOM_TREMONT_L	0x9C /* Jasper Lake */
+#define INTEL_ATOM_TREMONT_L		IFM(6, 0x9C) /* Jasper Lake */
 
 #define INTEL_FAM6_ATOM_GRACEMONT	0xBE /* Alderlake N */
+#define INTEL_ATOM_GRACEMONT		IFM(6, 0xBE) /* Alderlake N */
 
 #define INTEL_FAM6_ATOM_CRESTMONT_X	0xAF /* Sierra Forest */
+#define INTEL_ATOM_CRESTMONT_X		IFM(6, 0xAF) /* Sierra Forest */
 #define INTEL_FAM6_ATOM_CRESTMONT	0xB6 /* Grand Ridge */
+#define INTEL_ATOM_CRESTMONT		IFM(6, 0xB6) /* Grand Ridge */
 
 #define INTEL_FAM6_ATOM_DARKMONT_X	0xDD /* Clearwater Forest */
+#define INTEL_ATOM_DARKMONT_X		IFM(6, 0xDD) /* Clearwater Forest */
+
+/* Xeon Phi */
+
+#define INTEL_FAM6_XEON_PHI_KNL		0x57 /* Knights Landing */
+#define INTEL_XEON_PHI_KNL		IFM(6, 0x57) /* Knights Landing */
+#define INTEL_FAM6_XEON_PHI_KNM		0x85 /* Knights Mill */
+#define INTEL_XEON_PHI_KNM		IFM(6, 0x85) /* Knights Mill */
 
 /* Family 5 */
 #define INTEL_FAM5_QUARK_X1000		0x09 /* Quark X1000 SoC */
+#define INTEL_QUARK_X1000		IFM(5, 0x09) /* Quark X1000 SoC */
 
 #endif /* _ASM_X86_INTEL_FAMILY_H */
-- 
2.39.5

[PATCH 3/3] x86/vtd: Switch model check to VFM

[Andrew Cooper]

This form is shorer and more legible.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

As this is the first transformation, an anlysis of the code generation change:

before:
<quirk_iommu_caps>:
	       8b 05 4a 7e 09 00    	mov    0x97e4a(%rip),%eax        # <boot_cpu_data>
	       25 00 ff ff 00       	and    $0xffff00,%eax
	       3d 00 06 01 00       	cmp    $0x10600,%eax
	   /-- 74 0e                	je     <quirk_iommu_caps+0x20>
	/--|-> e9 59 8a dc ff       	jmp    <__x86_return_thunk>
	|  |   66 0f 1f 84 00 00 00 	nopw   0x0(%rax,%rax,1)
	|  |   00 00
	|  \-> 0f b6 05 29 7e 09 00 	movzbl 0x97e29(%rip),%eax        # <boot_cpu_data>
	|      3c 2a                	cmp    $0x2a,%al
	|  /-- 74 04                	je     <quirk_iommu_caps+0x2f>
	|  |   3c 2d                	cmp    $0x2d,%al
	\--|-- 75 e3                	jne    <quirk_iommu_caps+0x12>
	   \-> 48 b8 ff ff ff ff c3 	movabs $0xffffffc3ffffffff,%rax
	       ff ff ff
	       48 21 47 20          	and    %rax,0x20(%rdi)
	       e9 2e 8a dc ff       	jmp    <__x86_return_thunk>

after:
<quirk_iommu_caps>:
	    8b 05 4a 7e 09 00    	mov    0x97e4a(%rip),%eax        # <boot_cpu_data>
	    3d 2a 06 01 00       	cmp    $0x1062a,%eax
	/-- 74 13                	je     <quirk_iommu_caps+0x20>
	|   3d 2d 06 01 00       	cmp    $0x1062d,%eax
	+-- 74 0c                	je     <quirk_iommu_caps+0x20>
	|   e9 57 8a dc ff       	jmp    <__x86_return_thunk>
	|   0f 1f 80 00 00 00 00 	nopl   0x0(%rax)
	\-> 48 b8 ff ff ff ff c3 	movabs $0xffffffc3ffffffff,%rax
	    ff ff ff
	    48 21 47 20          	and    %rax,0x20(%rdi)
	    e9 3d 8a dc ff       	jmp    <__x86_return_thunk>

Previously, GCC managed to merge the "x86_vendor == X86_VENDOR_INTEL && x86 ==
6" condition, but performed a second load for x86_model.

Afterwards, there's a single load, and two compares.

Neither managed to merge the model checks together by playing with bit 2, but
that's something that compilers could learn to and is not suitable to express
at the C level for this kind of logic.

Unrelated to this change, it would be rather better to do `andl $0xc3,
0x24(%rdi)` than to manifest a 64bit constant.
---
 xen/arch/x86/include/asm/cpufeature.h | 2 +-
 xen/drivers/passthrough/vtd/quirks.c  | 7 +++----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h
index ba2c1c1c7416..f8b85c0f9520 100644
--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
@@ -28,7 +28,7 @@
 
 #define VFM_MAKE(v, f, m) (MASK_INSR(v, VFM_VENDOR_MASK) | \
                            MASK_INSR(f, VFM_FAMILY_MASK) | \
-                           MASK_INSR(f, VFM_MODEL_MASK))
+                           MASK_INSR(m, VFM_MODEL_MASK))
 
 #define VFM_MODEL(vfm)  MASK_EXTR(vfm, VFM_MODEL_MASK)
 #define VFM_FAMILY(vfm) MASK_EXTR(vfm, VFM_FAMILY_MASK)
diff --git a/xen/drivers/passthrough/vtd/quirks.c b/xen/drivers/passthrough/vtd/quirks.c
index dc3dac749ce6..0a10a46d90f7 100644
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -30,6 +30,7 @@
 #include <xen/pci_regs.h>
 #include <xen/keyhandler.h>
 #include <asm/msi.h>
+#include <asm/intel-family.h>
 #include <asm/irq.h>
 #include <asm/pci.h>
 
@@ -630,9 +631,7 @@ void __init quirk_iommu_caps(struct vtd_iommu *iommu)
      * model because the client parts don't expose their IOMMUs as PCI devices
      * we could match with a Device ID.
      */
-    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
-         boot_cpu_data.x86 == 6 &&
-         (boot_cpu_data.x86_model == 0x2a ||
-          boot_cpu_data.x86_model == 0x2d) )
+    if ( boot_cpu_data.vfm == INTEL_SANDYBRIDGE ||
+         boot_cpu_data.vfm == INTEL_SANDYBRIDGE_X )
         iommu->cap &= ~(0xful << 34);
 }
-- 
2.39.5

Re: [PATCH 3/3] x86/vtd: Switch model check to VFM

[Andrew Cooper]

On 16/07/2025 2:28 pm, Andrew Cooper wrote:
> diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h
> index ba2c1c1c7416..f8b85c0f9520 100644
> --- a/xen/arch/x86/include/asm/cpufeature.h
> +++ b/xen/arch/x86/include/asm/cpufeature.h
> @@ -28,7 +28,7 @@
>  
>  #define VFM_MAKE(v, f, m) (MASK_INSR(v, VFM_VENDOR_MASK) | \
>                             MASK_INSR(f, VFM_FAMILY_MASK) | \
> -                           MASK_INSR(f, VFM_MODEL_MASK))
> +                           MASK_INSR(m, VFM_MODEL_MASK))
>  

This hunk should be in the prior patch.  Fixed up locally.

~Andrew

Re: [PATCH 1/3] x86: Rearrange struct cpuinfo_x86 to introduce a vfm field

[Jan Beulich]

On 16.07.2025 15:28, Andrew Cooper wrote:
> Intel have run out of model space in Family 6 and will start using Family 19
> starting with Diamond Rapids.  Xen, like Linux, has model checking logic which
> will malfunction owing to bad assumptions about the family field.
> 
> Reorder the family, vendor and model fields so they can be accessed together
> as a single vfm field.
> 
> As we're cleaning up the logic, take the opportunity to introduce better
> names, dropping the x86 prefix.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

In principle
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Two remarks, though:

> --- a/xen/arch/x86/include/asm/cpufeature.h
> +++ b/xen/arch/x86/include/asm/cpufeature.h
> @@ -20,10 +20,30 @@
>  #ifndef __ASSEMBLY__
>  
>  struct cpuinfo_x86 {
> -    unsigned char x86;                 /* CPU family */
> -    unsigned char x86_vendor;          /* CPU vendor */
> -    unsigned char x86_model;
> -    unsigned char x86_mask;
> +    /* TODO: Phase out the x86 prefixed names. */
> +    union {
> +        struct {
> +            union {
> +                uint8_t x86_model;
> +                uint8_t model;
> +            };
> +            union {
> +                uint8_t x86;
> +                uint8_t family;
> +            };
> +            union {
> +                uint8_t x86_vendor;
> +                uint8_t vendor;
> +            };
> +            uint8_t _rsvd;

Can we perhaps name this e.g. _zero, so it's clear that it cannot be
repurposed?

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -178,7 +178,9 @@ void *stack_start = cpu0_stack + STACK_SIZE - sizeof(struct cpu_info);
>  /* Used by the boot asm and EFI to stash the multiboot_info paddr. */
>  unsigned int __initdata multiboot_ptr;
>  
> -struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 };
> +struct cpuinfo_x86 __read_mostly boot_cpu_data = {
> +    .cpuid_level = -1,
> +};

So you retain the bogus setting of this field. Would you mind taking a
look at [1], one of the many things that I never heard back on? I'm
deliberately purging that non-sense there as a (side-)effect. Plus
really I'm getting tired of having to re-base my long-pending changes
over ones you are helped getting in pretty quickly. No matter that this
one's going to be one of the easy ones (I hope).

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2024-02/msg00726.html

Re: [PATCH 2/3] x86/intel-family: Resync with Linux

[Jan Beulich]

On 16.07.2025 15:28, Andrew Cooper wrote:
> This snapshot is prior to Linux commit db4001f9cc32 ("x86/cpu/vfm: Delete all
> the *_FAM6_ CPU #defines") at the end of their conversion phase.
> 
> In addition to non-FAM6 infixed names, defines are added for the Pentium Pro,
> ArrowLake U, and reintroduced the PHI defines which were incorrectly deleted
> in the past.
> 
> In cpufeature.h, provide VFM_* macros to transform constants to/from the
> cpuinfo_x86 representation.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> I meant to object to deleting PHI at the time, but was too late.  Just because
> Xen has stopped supporting the PHI doesn't mean the model numbers have ceased
> existing.

I don't mind their re-introduction, yet I wonder what value their presence has
when we don't use them anywhere. In the public cpufeatureset.h they serve a
documentation purpose, to have less holes there. This could be view the same
here, yes, but it still seems (yet) less relevant.

> --- a/xen/arch/x86/include/asm/intel-family.h
> +++ b/xen/arch/x86/include/asm/intel-family.h
> @@ -13,8 +13,8 @@
>   *	INTEL_FAM6{OPTFAMILY}_{MICROARCH}{OPTDIFF}
>   * where:
>   * OPTFAMILY	Describes the family of CPUs that this belongs to. Default
> - *		is assumed to be "_CORE" (and should be omitted). The other
> - *		value currently in use is _ATOM.
> + *		is assumed to be "_CORE" (and should be omitted). Other values
> + *		currently in use are _ATOM and _XEON_PHI

Nit: You lost the full stop.

> @@ -40,131 +40,223 @@
>   * their own names :-(
>   */
>  
> +#define IFM(_fam, _model)	VFM_MAKE(X86_VENDOR_INTEL, _fam, _model)
> +
>  /* Wildcard match for FAM6 so X86_MATCH_INTEL_FAM6_MODEL(ANY) works */
>  #define INTEL_FAM6_ANY			X86_MODEL_ANY
> +/* Wildcard match for FAM6 so X86_MATCH_VFM(ANY) works */
> +#define INTEL_ANY			IFM(X86_FAMILY_ANY, X86_MODEL_ANY)
> +
> +#define INTEL_PENTIUM_PRO		IFM(6, 0x01)
>  
>  #define INTEL_FAM6_CORE_YONAH		0x0E
> +#define INTEL_CORE_YONAH		IFM(6, 0x0E)

Here and below I assume it is deliberate that you use a raw number again,
rather than making use of the immediately preceding #define? Perhaps for
the sake of brevity?

Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH 3/3] x86/vtd: Switch model check to VFM

[Jan Beulich]

On 16.07.2025 15:32, Andrew Cooper wrote:
> On 16/07/2025 2:28 pm, Andrew Cooper wrote:
>> diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h
>> index ba2c1c1c7416..f8b85c0f9520 100644
>> --- a/xen/arch/x86/include/asm/cpufeature.h
>> +++ b/xen/arch/x86/include/asm/cpufeature.h
>> @@ -28,7 +28,7 @@
>>  
>>  #define VFM_MAKE(v, f, m) (MASK_INSR(v, VFM_VENDOR_MASK) | \
>>                             MASK_INSR(f, VFM_FAMILY_MASK) | \
>> -                           MASK_INSR(f, VFM_MODEL_MASK))
>> +                           MASK_INSR(m, VFM_MODEL_MASK))
>>  
> 
> This hunk should be in the prior patch.  Fixed up locally.

Then:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH 1/3] x86: Rearrange struct cpuinfo_x86 to introduce a vfm field

[Andrew Cooper]

On 16/07/2025 2:47 pm, Jan Beulich wrote:
> On 16.07.2025 15:28, Andrew Cooper wrote:
>> Intel have run out of model space in Family 6 and will start using Family 19
>> starting with Diamond Rapids.  Xen, like Linux, has model checking logic which
>> will malfunction owing to bad assumptions about the family field.
>>
>> Reorder the family, vendor and model fields so they can be accessed together
>> as a single vfm field.
>>
>> As we're cleaning up the logic, take the opportunity to introduce better
>> names, dropping the x86 prefix.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> In principle
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Two remarks, though:
>
>> --- a/xen/arch/x86/include/asm/cpufeature.h
>> +++ b/xen/arch/x86/include/asm/cpufeature.h
>> @@ -20,10 +20,30 @@
>>  #ifndef __ASSEMBLY__
>>  
>>  struct cpuinfo_x86 {
>> -    unsigned char x86;                 /* CPU family */
>> -    unsigned char x86_vendor;          /* CPU vendor */
>> -    unsigned char x86_model;
>> -    unsigned char x86_mask;
>> +    /* TODO: Phase out the x86 prefixed names. */
>> +    union {
>> +        struct {
>> +            union {
>> +                uint8_t x86_model;
>> +                uint8_t model;
>> +            };
>> +            union {
>> +                uint8_t x86;
>> +                uint8_t family;
>> +            };
>> +            union {
>> +                uint8_t x86_vendor;
>> +                uint8_t vendor;
>> +            };
>> +            uint8_t _rsvd;
> Can we perhaps name this e.g. _zero, so it's clear that it cannot be
> repurposed?

It can be repurposed; it just must be done in coordination with VFM_MAKE().

I can add a comment to this effect, but it would need to be in the
subsequent patch when VFM_MAKE() is introduced.

>
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -178,7 +178,9 @@ void *stack_start = cpu0_stack + STACK_SIZE - sizeof(struct cpu_info);
>>  /* Used by the boot asm and EFI to stash the multiboot_info paddr. */
>>  unsigned int __initdata multiboot_ptr;
>>  
>> -struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 };
>> +struct cpuinfo_x86 __read_mostly boot_cpu_data = {
>> +    .cpuid_level = -1,
>> +};
> So you retain the bogus setting of this field. Would you mind taking a
> look at [1], one of the many things that I never heard back on? I'm
> deliberately purging that non-sense there as a (side-)effect. Plus
> really I'm getting tired of having to re-base my long-pending changes
> over ones you are helped getting in pretty quickly. No matter that this
> one's going to be one of the easy ones (I hope).
>
> Jan
>
> [1] https://lists.xen.org/archives/html/xen-devel/2024-02/msg00726.html

I can rebase.

~Andrew

Re: [PATCH 1/3] x86: Rearrange struct cpuinfo_x86 to introduce a vfm field

[Jan Beulich]

On 16.07.2025 17:45, Andrew Cooper wrote:
> On 16/07/2025 2:47 pm, Jan Beulich wrote:
>> On 16.07.2025 15:28, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/include/asm/cpufeature.h
>>> +++ b/xen/arch/x86/include/asm/cpufeature.h
>>> @@ -20,10 +20,30 @@
>>>  #ifndef __ASSEMBLY__
>>>  
>>>  struct cpuinfo_x86 {
>>> -    unsigned char x86;                 /* CPU family */
>>> -    unsigned char x86_vendor;          /* CPU vendor */
>>> -    unsigned char x86_model;
>>> -    unsigned char x86_mask;
>>> +    /* TODO: Phase out the x86 prefixed names. */
>>> +    union {
>>> +        struct {
>>> +            union {
>>> +                uint8_t x86_model;
>>> +                uint8_t model;
>>> +            };
>>> +            union {
>>> +                uint8_t x86;
>>> +                uint8_t family;
>>> +            };
>>> +            union {
>>> +                uint8_t x86_vendor;
>>> +                uint8_t vendor;
>>> +            };
>>> +            uint8_t _rsvd;
>> Can we perhaps name this e.g. _zero, so it's clear that it cannot be
>> repurposed?
> 
> It can be repurposed; it just must be done in coordination with VFM_MAKE().

Hmm, true.

> I can add a comment to this effect, but it would need to be in the
> subsequent patch when VFM_MAKE() is introduced.

Okay, and thanks.

Jan

Re: [PATCH 2/3] x86/intel-family: Resync with Linux

[Andrew Cooper]

On 16/07/2025 2:56 pm, Jan Beulich wrote:
> On 16.07.2025 15:28, Andrew Cooper wrote:
>> This snapshot is prior to Linux commit db4001f9cc32 ("x86/cpu/vfm: Delete all
>> the *_FAM6_ CPU #defines") at the end of their conversion phase.
>>
>> In addition to non-FAM6 infixed names, defines are added for the Pentium Pro,
>> ArrowLake U, and reintroduced the PHI defines which were incorrectly deleted
>> in the past.
>>
>> In cpufeature.h, provide VFM_* macros to transform constants to/from the
>> cpuinfo_x86 representation.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> I meant to object to deleting PHI at the time, but was too late.  Just because
>> Xen has stopped supporting the PHI doesn't mean the model numbers have ceased
>> existing.
> I don't mind their re-introduction, yet I wonder what value their presence has
> when we don't use them anywhere.

I spend a lot of time cross-checking tables in the speculative papers. 
Knowing which were the released CPUs is important for finding typos.  No
comment on how many tables were correct on their first revision...

That said, there's SoFIA which is not included in intel-family, and for
which we have partial coverage owing to early speculative tables.  IIRC,
it was a custom chip for a mobile base-station, and runs an entirely
custom software stack.

>> --- a/xen/arch/x86/include/asm/intel-family.h
>> +++ b/xen/arch/x86/include/asm/intel-family.h
>> @@ -13,8 +13,8 @@
>>   *	INTEL_FAM6{OPTFAMILY}_{MICROARCH}{OPTDIFF}
>>   * where:
>>   * OPTFAMILY	Describes the family of CPUs that this belongs to. Default
>> - *		is assumed to be "_CORE" (and should be omitted). The other
>> - *		value currently in use is _ATOM.
>> + *		is assumed to be "_CORE" (and should be omitted). Other values
>> + *		currently in use are _ATOM and _XEON_PHI
> Nit: You lost the full stop.

That seems to be missing in Linux, even at master.

I've got an outstanding task in Linux's intel-family.  I'll try to
remember to fix it upstream.

>
>> @@ -40,131 +40,223 @@
>>   * their own names :-(
>>   */
>>  
>> +#define IFM(_fam, _model)	VFM_MAKE(X86_VENDOR_INTEL, _fam, _model)
>> +
>>  /* Wildcard match for FAM6 so X86_MATCH_INTEL_FAM6_MODEL(ANY) works */
>>  #define INTEL_FAM6_ANY			X86_MODEL_ANY
>> +/* Wildcard match for FAM6 so X86_MATCH_VFM(ANY) works */
>> +#define INTEL_ANY			IFM(X86_FAMILY_ANY, X86_MODEL_ANY)
>> +
>> +#define INTEL_PENTIUM_PRO		IFM(6, 0x01)
>>  
>>  #define INTEL_FAM6_CORE_YONAH		0x0E
>> +#define INTEL_CORE_YONAH		IFM(6, 0x0E)
> Here and below I assume it is deliberate that you use a raw number again,
> rather than making use of the immediately preceding #define? Perhaps for
> the sake of brevity?

Because it's transient.

All the FAM* constants get deleted in db4001f9cc32 ("x86/cpu/vfm: Delete all
the *_FAM6_ CPU #defines") and I've got another resync in my local
branch, but it obviously doesn't compile in Xen yet.

> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks.

~Andrew