[PATCH 0/2] x86/amd: LFENCE always serializing CPUID bit

[PATCH 0/2] x86/amd: LFENCE always serializing CPUID bit

[Roger Pau Monne]

Hello,

The following patches aim to add support for the LFENCE always
serializing CPUID bit found in leaf 80000021.eax. In order to do that
the featureset needs to be expanded to support leaf 80000021.eax.

Thanks, Roger.

Roger Pau Monne (2):
  x86/amd: split LFENCE dispatch serializing setup logic into helper
  x86/cpuid: support LFENCE always serializing CPUID bit

 tools/misc/xen-cpuid.c                      |  6 ++
 xen/arch/x86/cpu/amd.c                      | 69 ++++++++++++---------
 xen/arch/x86/cpu/common.c                   |  3 +
 xen/arch/x86/cpu/cpu.h                      |  1 +
 xen/arch/x86/cpu/hygon.c                    | 27 +-------
 xen/include/asm-x86/cpufeatures.h           |  2 +-
 xen/include/public/arch-x86/cpufeatureset.h |  3 +
 xen/include/xen/lib/x86/cpuid.h             | 37 ++++++++++-
 8 files changed, 92 insertions(+), 56 deletions(-)

-- 
2.30.1

[PATCH 1/2] x86/amd: split LFENCE dispatch serializing setup logic into helper

[Roger Pau Monne]

Split the logic to attempt to setup the LFENCE to be dispatch
serializing on AMD into a helper, so it can be shared with Hygon.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/amd.c   | 62 ++++++++++++++++++++++------------------
 xen/arch/x86/cpu/cpu.h   |  1 +
 xen/arch/x86/cpu/hygon.c | 27 +----------------
 3 files changed, 36 insertions(+), 54 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 8bc51bec10d..9c8dcd91eef 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -642,6 +642,38 @@ void early_init_amd(struct cpuinfo_x86 *c)
 	ctxt_switch_levelling(NULL);
 }
 
+void amd_init_lfence(struct cpuinfo_x86 *c)
+{
+	uint64_t value;
+
+	/*
+	 * Attempt to set lfence to be Dispatch Serialising.  This MSR almost
+	 * certainly isn't virtualised (and Xen at least will leak the real
+	 * value in but silently discard writes), as well as being per-core
+	 * rather than per-thread, so do a full safe read/write/readback cycle
+	 * in the worst case.
+	 */
+	if (rdmsr_safe(MSR_AMD64_DE_CFG, value))
+		/* Unable to read.  Assume the safer default. */
+		__clear_bit(X86_FEATURE_LFENCE_DISPATCH,
+			    c->x86_capability);
+	else if (value & AMD64_DE_CFG_LFENCE_SERIALISE)
+		/* Already dispatch serialising. */
+		__set_bit(X86_FEATURE_LFENCE_DISPATCH,
+			  c->x86_capability);
+	else if (wrmsr_safe(MSR_AMD64_DE_CFG,
+			    value | AMD64_DE_CFG_LFENCE_SERIALISE) ||
+		 rdmsr_safe(MSR_AMD64_DE_CFG, value) ||
+		 !(value & AMD64_DE_CFG_LFENCE_SERIALISE))
+		/* Attempt to set failed.  Assume the safer default. */
+		__clear_bit(X86_FEATURE_LFENCE_DISPATCH,
+			    c->x86_capability);
+	else
+		/* Successfully enabled! */
+		__set_bit(X86_FEATURE_LFENCE_DISPATCH,
+			  c->x86_capability);
+}
+
 static void init_amd(struct cpuinfo_x86 *c)
 {
 	u32 l, h;
@@ -686,37 +718,11 @@ static void init_amd(struct cpuinfo_x86 *c)
 	if (c == &boot_cpu_data && !cpu_has(c, X86_FEATURE_RSTR_FP_ERR_PTRS))
 		setup_force_cpu_cap(X86_BUG_FPU_PTRS);
 
-	/*
-	 * Attempt to set lfence to be Dispatch Serialising.  This MSR almost
-	 * certainly isn't virtualised (and Xen at least will leak the real
-	 * value in but silently discard writes), as well as being per-core
-	 * rather than per-thread, so do a full safe read/write/readback cycle
-	 * in the worst case.
-	 */
 	if (c->x86 == 0x0f || c->x86 == 0x11)
 		/* Always dispatch serialising on this hardare. */
 		__set_bit(X86_FEATURE_LFENCE_DISPATCH, c->x86_capability);
-	else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */ {
-		if (rdmsr_safe(MSR_AMD64_DE_CFG, value))
-			/* Unable to read.  Assume the safer default. */
-			__clear_bit(X86_FEATURE_LFENCE_DISPATCH,
-				    c->x86_capability);
-		else if (value & AMD64_DE_CFG_LFENCE_SERIALISE)
-			/* Already dispatch serialising. */
-			__set_bit(X86_FEATURE_LFENCE_DISPATCH,
-				  c->x86_capability);
-		else if (wrmsr_safe(MSR_AMD64_DE_CFG,
-				    value | AMD64_DE_CFG_LFENCE_SERIALISE) ||
-			 rdmsr_safe(MSR_AMD64_DE_CFG, value) ||
-			 !(value & AMD64_DE_CFG_LFENCE_SERIALISE))
-			/* Attempt to set failed.  Assume the safer default. */
-			__clear_bit(X86_FEATURE_LFENCE_DISPATCH,
-				    c->x86_capability);
-		else
-			/* Successfully enabled! */
-			__set_bit(X86_FEATURE_LFENCE_DISPATCH,
-				  c->x86_capability);
-	}
+	else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */
+		amd_init_lfence(c);
 
 	/*
 	 * If the user has explicitly chosen to disable Memory Disambiguation
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index 1992596d1b2..1ac3b2867a0 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -20,3 +20,4 @@ extern bool detect_extended_topology(struct cpuinfo_x86 *c);
 
 void early_init_amd(struct cpuinfo_x86 *c);
 void amd_log_freq(const struct cpuinfo_x86 *c);
+void amd_init_lfence(struct cpuinfo_x86 *c);
diff --git a/xen/arch/x86/cpu/hygon.c b/xen/arch/x86/cpu/hygon.c
index 46293f1f367..2272e1113f1 100644
--- a/xen/arch/x86/cpu/hygon.c
+++ b/xen/arch/x86/cpu/hygon.c
@@ -32,32 +32,7 @@ static void init_hygon(struct cpuinfo_x86 *c)
 {
 	unsigned long long value;
 
-	/*
-	 * Attempt to set lfence to be Dispatch Serialising.  This MSR almost
-	 * certainly isn't virtualised (and Xen at least will leak the real
-	 * value in but silently discard writes), as well as being per-core
-	 * rather than per-thread, so do a full safe read/write/readback cycle
-	 * in the worst case.
-	 */
-	if (rdmsr_safe(MSR_AMD64_DE_CFG, value))
-		/* Unable to read.  Assume the safer default. */
-		__clear_bit(X86_FEATURE_LFENCE_DISPATCH,
-			    c->x86_capability);
-	else if (value & AMD64_DE_CFG_LFENCE_SERIALISE)
-		/* Already dispatch serialising. */
-		__set_bit(X86_FEATURE_LFENCE_DISPATCH,
-			  c->x86_capability);
-	else if (wrmsr_safe(MSR_AMD64_DE_CFG,
-			    value | AMD64_DE_CFG_LFENCE_SERIALISE) ||
-		 rdmsr_safe(MSR_AMD64_DE_CFG, value) ||
-		 !(value & AMD64_DE_CFG_LFENCE_SERIALISE))
-		/* Attempt to set failed.  Assume the safer default. */
-		__clear_bit(X86_FEATURE_LFENCE_DISPATCH,
-			    c->x86_capability);
-	else
-		/* Successfully enabled! */
-		__set_bit(X86_FEATURE_LFENCE_DISPATCH,
-			  c->x86_capability);
+	amd_init_lfence(c);
 
 	/*
 	 * If the user has explicitly chosen to disable Memory Disambiguation
-- 
2.30.1

[PATCH 2/2] x86/cpuid: support LFENCE always serializing CPUID bit

[Roger Pau Monne]

Milan AMD CPUs have an LFENCE Always Serializing CPUID bit in leaf
80000021.eax. Previous AMD versions used to have a user settable bit
in DE_CFG MSR to select whether LFENCE was dispatch serializing, which
Xen always attempts to set.

In order to support this new CPUID bit move the LFENCE_DISPATCH
synthetic CPUID bit to map the hardware bit (leaving a hole in the
synthetic range) and either rely on the bit already being set by the
native CPUID output, or attempt to fake it in Xen by modifying the
DE_CFG MSR. This requires adding one more entry to the featureset to
support leaf 80000021.eax.

The bit is exposed to guests by default, as a way to signal that
LFENCE is always serializing, either because it's mandated by
hardware, or because Xen has performed the necessary arrangements.
Note that Xen doesn't allow guests to change the DE_CFG value, so once
set by Xen LFENCE will always be serializing.

Note that the access to DE_CFG by guests is left as-is: reads will
unconditionally return LFENCE_SERIALISE bit set, while writes are
silently dropped. The MSR is not present on AMD Milan hardware, but
I'm not sure there's any value in adding logic in Xen to also hide it
from guests when running on such hardware.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/misc/xen-cpuid.c                      |  6 ++++
 xen/arch/x86/cpu/amd.c                      |  7 ++++
 xen/arch/x86/cpu/common.c                   |  3 ++
 xen/include/asm-x86/cpufeatures.h           |  2 +-
 xen/include/public/arch-x86/cpufeatureset.h |  3 ++
 xen/include/xen/lib/x86/cpuid.h             | 37 ++++++++++++++++++++-
 6 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 2d04162d8d8..38406baadad 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -178,6 +178,11 @@ static const char *const str_7a1[32] =
     [ 4] = "avx-vnni",      [ 5] = "avx512-bf16",
 };
 
+static const char *const str_e21a[32] =
+{
+    [ 2] = "lfence-always-serializing",
+};
+
 static const struct {
     const char *name;
     const char *abbr;
@@ -195,6 +200,7 @@ static const struct {
     { "0x80000008.ebx",   "e8b", str_e8b },
     { "0x00000007:0.edx", "7d0", str_7d0 },
     { "0x00000007:1.eax", "7a1", str_7a1 },
+    { "0x80000021.eax",  "e21a", str_e21a },
 };
 
 #define COL_ALIGN "18"
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 9c8dcd91eef..35f22c24762 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -646,6 +646,13 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
 {
 	uint64_t value;
 
+	/*
+	 * Some hardware has LFENCE dispatch serializing always enabled,
+	 * nothing to do on that case.
+	 */
+	if (test_bit(X86_FEATURE_LFENCE_DISPATCH, c->x86_capability))
+		return;
+
 	/*
 	 * Attempt to set lfence to be Dispatch Serialising.  This MSR almost
 	 * certainly isn't virtualised (and Xen at least will leak the real
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index e5c3caf41d5..0eb364f8a65 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -412,6 +412,9 @@ static void generic_identify(struct cpuinfo_x86 *c)
 	if (c->extended_cpuid_level >= 0x80000008)
 		c->x86_capability[cpufeat_word(X86_FEATURE_CLZERO)]
 			= cpuid_ebx(0x80000008);
+	if (c->extended_cpuid_level >= 0x80000021)
+		c->x86_capability[cpufeat_word(X86_FEATURE_LFENCE_DISPATCH)]
+			= cpuid_eax(0x80000021);
 
 	/* Intel-defined flags: level 0x00000007 */
 	if ( c->cpuid_level >= 0x00000007 ) {
diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h
index d7e42d9bb6a..6c8f432aee4 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -24,7 +24,7 @@ XEN_CPUFEATURE(APERFMPERF,        X86_SYNTH( 8)) /* APERFMPERF */
 XEN_CPUFEATURE(MFENCE_RDTSC,      X86_SYNTH( 9)) /* MFENCE synchronizes RDTSC */
 XEN_CPUFEATURE(XEN_SMEP,          X86_SYNTH(10)) /* SMEP gets used by Xen itself */
 XEN_CPUFEATURE(XEN_SMAP,          X86_SYNTH(11)) /* SMAP gets used by Xen itself */
-XEN_CPUFEATURE(LFENCE_DISPATCH,   X86_SYNTH(12)) /* lfence set as Dispatch Serialising */
+/* Bit 12 - unused. */
 XEN_CPUFEATURE(IND_THUNK_LFENCE,  X86_SYNTH(13)) /* Use IND_THUNK_LFENCE */
 XEN_CPUFEATURE(IND_THUNK_JMP,     X86_SYNTH(14)) /* Use IND_THUNK_JMP */
 XEN_CPUFEATURE(SC_BRANCH_HARDEN,  X86_SYNTH(15)) /* Conditional Branch Hardening */
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index a5014798209..c67cd07b26b 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -277,6 +277,9 @@ XEN_CPUFEATURE(SSBD,          9*32+31) /*A  MSR_SPEC_CTRL.SSBD available */
 XEN_CPUFEATURE(AVX_VNNI,     10*32+ 4) /*A  AVX-VNNI Instructions */
 XEN_CPUFEATURE(AVX512_BF16,  10*32+ 5) /*A  AVX512 BFloat16 Instructions */
 
+/* AMD-defined CPU features, CPUID level 0x80000021.eax, word 11 */
+XEN_CPUFEATURE(LFENCE_DISPATCH,    11*32+ 2) /*A  LFENCE always serializing */
+
 #endif /* XEN_CPUFEATURE */
 
 /* Clean up from a default include.  Close the enum (for C). */
diff --git a/xen/include/xen/lib/x86/cpuid.h b/xen/include/xen/lib/x86/cpuid.h
index f4ef8a9f2f0..a4d254ea96e 100644
--- a/xen/include/xen/lib/x86/cpuid.h
+++ b/xen/include/xen/lib/x86/cpuid.h
@@ -15,6 +15,7 @@
 #define FEATURESET_e8b    8 /* 0x80000008.ebx      */
 #define FEATURESET_7d0    9 /* 0x00000007:0.edx    */
 #define FEATURESET_7a1   10 /* 0x00000007:1.eax    */
+#define FEATURESET_e21a  11 /* 0x80000021.eax      */
 
 struct cpuid_leaf
 {
@@ -84,7 +85,7 @@ const char *x86_cpuid_vendor_to_str(unsigned int vendor);
 #define CPUID_GUEST_NR_TOPO       (1u + 1)
 #define CPUID_GUEST_NR_XSTATE     (62u + 1)
 #define CPUID_GUEST_NR_EXTD_INTEL (0x8u + 1)
-#define CPUID_GUEST_NR_EXTD_AMD   (0x1cu + 1)
+#define CPUID_GUEST_NR_EXTD_AMD   (0x21u + 1)
 #define CPUID_GUEST_NR_EXTD       MAX(CPUID_GUEST_NR_EXTD_INTEL, \
                                       CPUID_GUEST_NR_EXTD_AMD)
 
@@ -264,6 +265,38 @@ struct cpuid_policy
             };
             uint32_t nc:8, :4, apic_id_size:4, :16;
             uint32_t /* d */:32;
+
+            uint64_t :64, :64; /* Leaf 0x80000009. */
+            uint64_t :64, :64; /* Leaf 0x8000000a - SVM rev and features. */
+            uint64_t :64, :64; /* Leaf 0x8000000b. */
+            uint64_t :64, :64; /* Leaf 0x8000000c. */
+            uint64_t :64, :64; /* Leaf 0x8000000d. */
+            uint64_t :64, :64; /* Leaf 0x8000000e. */
+            uint64_t :64, :64; /* Leaf 0x8000000f. */
+            uint64_t :64, :64; /* Leaf 0x80000010. */
+            uint64_t :64, :64; /* Leaf 0x80000011. */
+            uint64_t :64, :64; /* Leaf 0x80000012. */
+            uint64_t :64, :64; /* Leaf 0x80000013. */
+            uint64_t :64, :64; /* Leaf 0x80000014. */
+            uint64_t :64, :64; /* Leaf 0x80000015. */
+            uint64_t :64, :64; /* Leaf 0x80000016. */
+            uint64_t :64, :64; /* Leaf 0x80000017. */
+            uint64_t :64, :64; /* Leaf 0x80000018. */
+            uint64_t :64, :64; /* Leaf 0x80000019 - TLB 1GB Identifiers. */
+            uint64_t :64, :64; /* Leaf 0x8000001a - Performance related info. */
+            uint64_t :64, :64; /* Leaf 0x8000001b - IBS feature information. */
+            uint64_t :64, :64; /* Leaf 0x8000001c. */
+            uint64_t :64, :64; /* Leaf 0x8000001d - Cache properties. */
+            uint64_t :64, :64; /* Leaf 0x8000001e - Extd APIC/Core/Node IDs. */
+            uint64_t :64, :64; /* Leaf 0x8000001f - AMD Secure Encryption. */
+            uint64_t :64, :64; /* Leaf 0x80000020 - Platform QoS. */
+
+            /* Leaf 0x80000021 - Extended Feature 2 */
+            union {
+                uint32_t e21a;
+                struct { DECL_BITFIELD(e21a); };
+            };
+            uint32_t /* b */:32, /* c */:32, /* d */:32;
         };
     } extd;
 
@@ -293,6 +326,7 @@ static inline void cpuid_policy_to_featureset(
     fs[FEATURESET_e8b] = p->extd.e8b;
     fs[FEATURESET_7d0] = p->feat._7d0;
     fs[FEATURESET_7a1] = p->feat._7a1;
+    fs[FEATURESET_e21a] = p->extd.e21a;
 }
 
 /* Fill in a CPUID policy from a featureset bitmap. */
@@ -310,6 +344,7 @@ static inline void cpuid_featureset_to_policy(
     p->extd.e8b   = fs[FEATURESET_e8b];
     p->feat._7d0  = fs[FEATURESET_7d0];
     p->feat._7a1  = fs[FEATURESET_7a1];
+    p->extd.e21a  = fs[FEATURESET_e21a];
 }
 
 static inline uint64_t cpuid_policy_xcr0_max(const struct cpuid_policy *p)
-- 
2.30.1

Re: [PATCH 1/2] x86/amd: split LFENCE dispatch serializing setup logic into helper

[Andrew Cooper]

On 14/04/2021 12:04, Roger Pau Monne wrote:
> Split the logic to attempt to setup the LFENCE to be dispatch

"the LFENCE instruction", or drop "the".

> serializing on AMD into a helper, so it can be shared with Hygon.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 2/2] x86/cpuid: support LFENCE always serializing CPUID bit

[Andrew Cooper]

On 14/04/2021 12:04, Roger Pau Monne wrote:
> Milan AMD CPUs

I'd suggest "AMD Milan (Zen3) CPUs" for people who don't know the AMD
codenames inside/out.

>  have an LFENCE Always Serializing CPUID bit in leaf
> 80000021.eax.

Its probably worth noting that this is because of SEV-SNP, which is a
headline feature in Milan.

There must not be anything the VMM can do to impact the confidentiality
of an SEV-SNP VM, and breaking Spectre protections is definitely a
problem.  In a post-Spectre world, no system should be running without
LFENCE set to dispatch serialising.

>  Previous AMD versions used to have a user settable bit
> in DE_CFG MSR to select whether LFENCE was dispatch serializing, which
> Xen always attempts to set.
>
> In order to support this new CPUID bit move the LFENCE_DISPATCH
> synthetic CPUID bit to map the hardware bit (leaving a hole in the
> synthetic range) and either rely on the bit already being set by the
> native CPUID output, or attempt to fake it in Xen by modifying the
> DE_CFG MSR. This requires adding one more entry to the featureset to
> support leaf 80000021.eax.
>
> The bit is exposed to guests by default, as a way to signal that
> LFENCE is always serializing, either because it's mandated by
> hardware, or because Xen has performed the necessary arrangements.
> Note that Xen doesn't allow guests to change the DE_CFG value, so once
> set by Xen LFENCE will always be serializing.
>
> Note that the access to DE_CFG by guests is left as-is: reads will
> unconditionally return LFENCE_SERIALISE bit set, while writes are
> silently dropped. The MSR is not present on AMD Milan hardware

Not all MSRs are listed in the PPR.

This MSR does exist, and is my understanding that the bit still exists,
and is read-as-1/write-discard, so the past 3 years of software will
still conclude "lfence safe" when inspecting the MSR.

One of the speculation whitepapers says that this bit is going to remain
fixed in the future to simplify software needing to interact with it.

> , but
> I'm not sure there's any value in adding logic in Xen to also hide it
> from guests when running on such hardware.
>
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  tools/misc/xen-cpuid.c                      |  6 ++++

Need to patch the translation table in libxl_cpuid.c too.  See c/s
23ccf530431561 for a reference.

>  xen/arch/x86/cpu/amd.c                      |  7 ++++
>  xen/arch/x86/cpu/common.c                   |  3 ++
>  xen/include/asm-x86/cpufeatures.h           |  2 +-
>  xen/include/public/arch-x86/cpufeatureset.h |  3 ++
>  xen/include/xen/lib/x86/cpuid.h             | 37 ++++++++++++++++++++-
>  6 files changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
> index 2d04162d8d8..38406baadad 100644
> --- a/tools/misc/xen-cpuid.c
> +++ b/tools/misc/xen-cpuid.c
> @@ -178,6 +178,11 @@ static const char *const str_7a1[32] =
>      [ 4] = "avx-vnni",      [ 5] = "avx512-bf16",
>  };
>  
> +static const char *const str_e21a[32] =
> +{
> +    [ 2] = "lfence-always-serializing",

This is a bit of a mouthful.  One problem is the fact that "serialising"
is an ambiguous term, because neither Intel nor AMD formally specify
what it means in the architecture.

There is a description of what "architecturally serialising" does, which
does occasionally move over time, and the name of this CPUID bit in the
PPR confusing at best, because it very much isn't "architecturally
serialising", and the term "dispatch serialising" isn't actually defined
anywhere.

Intel have now started referring to LFENCE as a "speculative execution
barrier", but this still doesn't have a precise definition.

I'm afraid I don't have a useful suggestion for something short and
concise, which also conveys the precise meaning.

~Andrew

Re: [PATCH 2/2] x86/cpuid: support LFENCE always serializing CPUID bit

[Jan Beulich]

On 14.04.2021 14:33, Andrew Cooper wrote:
> On 14/04/2021 12:04, Roger Pau Monne wrote:
>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -178,6 +178,11 @@ static const char *const str_7a1[32] =
>>      [ 4] = "avx-vnni",      [ 5] = "avx512-bf16",
>>  };
>>  
>> +static const char *const str_e21a[32] =
>> +{
>> +    [ 2] = "lfence-always-serializing",
> 
> This is a bit of a mouthful.  One problem is the fact that "serialising"
> is an ambiguous term, because neither Intel nor AMD formally specify
> what it means in the architecture.
> 
> There is a description of what "architecturally serialising" does, which
> does occasionally move over time, and the name of this CPUID bit in the
> PPR confusing at best, because it very much isn't "architecturally
> serialising", and the term "dispatch serialising" isn't actually defined
> anywhere.
> 
> Intel have now started referring to LFENCE as a "speculative execution
> barrier", but this still doesn't have a precise definition.
> 
> I'm afraid I don't have a useful suggestion for something short and
> concise, which also conveys the precise meaning.

How about "lfence+" or some such?

Jan

Re: [PATCH 2/2] x86/cpuid: support LFENCE always serializing CPUID bit

[Andrew Cooper]

On 14/04/2021 13:45, Jan Beulich wrote:
> On 14.04.2021 14:33, Andrew Cooper wrote:
>> On 14/04/2021 12:04, Roger Pau Monne wrote:
>>> --- a/tools/misc/xen-cpuid.c
>>> +++ b/tools/misc/xen-cpuid.c
>>> @@ -178,6 +178,11 @@ static const char *const str_7a1[32] =
>>>      [ 4] = "avx-vnni",      [ 5] = "avx512-bf16",
>>>  };
>>>  
>>> +static const char *const str_e21a[32] =
>>> +{
>>> +    [ 2] = "lfence-always-serializing",
>> This is a bit of a mouthful.  One problem is the fact that "serialising"
>> is an ambiguous term, because neither Intel nor AMD formally specify
>> what it means in the architecture.
>>
>> There is a description of what "architecturally serialising" does, which
>> does occasionally move over time, and the name of this CPUID bit in the
>> PPR confusing at best, because it very much isn't "architecturally
>> serialising", and the term "dispatch serialising" isn't actually defined
>> anywhere.
>>
>> Intel have now started referring to LFENCE as a "speculative execution
>> barrier", but this still doesn't have a precise definition.
>>
>> I'm afraid I don't have a useful suggestion for something short and
>> concise, which also conveys the precise meaning.
> How about "lfence+" or some such?

Hmm yes - for xen-cpuid, that's probably fine, and for churn reasons, we
can keep X86_FEATURE_LFENCE_DISPATCH which was the best name I could
come up with at the time.

~Andrew

Re: [PATCH 2/2] x86/cpuid: support LFENCE always serializing CPUID bit

[Jan Beulich]

On 14.04.2021 13:04, Roger Pau Monne wrote:
> @@ -264,6 +265,38 @@ struct cpuid_policy
>              };
>              uint32_t nc:8, :4, apic_id_size:4, :16;
>              uint32_t /* d */:32;
> +
> +            uint64_t :64, :64; /* Leaf 0x80000009. */
> +            uint64_t :64, :64; /* Leaf 0x8000000a - SVM rev and features. */
> +            uint64_t :64, :64; /* Leaf 0x8000000b. */
> +            uint64_t :64, :64; /* Leaf 0x8000000c. */
> +            uint64_t :64, :64; /* Leaf 0x8000000d. */
> +            uint64_t :64, :64; /* Leaf 0x8000000e. */
> +            uint64_t :64, :64; /* Leaf 0x8000000f. */
> +            uint64_t :64, :64; /* Leaf 0x80000010. */
> +            uint64_t :64, :64; /* Leaf 0x80000011. */
> +            uint64_t :64, :64; /* Leaf 0x80000012. */
> +            uint64_t :64, :64; /* Leaf 0x80000013. */
> +            uint64_t :64, :64; /* Leaf 0x80000014. */
> +            uint64_t :64, :64; /* Leaf 0x80000015. */
> +            uint64_t :64, :64; /* Leaf 0x80000016. */
> +            uint64_t :64, :64; /* Leaf 0x80000017. */
> +            uint64_t :64, :64; /* Leaf 0x80000018. */
> +            uint64_t :64, :64; /* Leaf 0x80000019 - TLB 1GB Identifiers. */
> +            uint64_t :64, :64; /* Leaf 0x8000001a - Performance related info. */
> +            uint64_t :64, :64; /* Leaf 0x8000001b - IBS feature information. */
> +            uint64_t :64, :64; /* Leaf 0x8000001c. */
> +            uint64_t :64, :64; /* Leaf 0x8000001d - Cache properties. */
> +            uint64_t :64, :64; /* Leaf 0x8000001e - Extd APIC/Core/Node IDs. */
> +            uint64_t :64, :64; /* Leaf 0x8000001f - AMD Secure Encryption. */
> +            uint64_t :64, :64; /* Leaf 0x80000020 - Platform QoS. */
> +
> +            /* Leaf 0x80000021 - Extended Feature 2 */
> +            union {
> +                uint32_t e21a;
> +                struct { DECL_BITFIELD(e21a); };
> +            };
> +            uint32_t /* b */:32, /* c */:32, /* d */:32;
>          };
>      } extd;

Due to the effect of this on what guests get to see, I think this
wants to take my "x86/CPUID: shrink max_{,sub}leaf fields according
to actual leaf contents" as a prereq, which in turn may better
remain on top of "x86/CPUID: adjust extended leaves out of range
clearing" (both are neighbors in that over 4 months old series,
fair parts of which could imo go in irrespective of the unsettled
dispute on xvmalloc() - unfortunately I had made that patch 2 of
the series, not expecting it to be blocked for so long, and then
presumably signaling to others that the rest of the series is also
blocked).

Jan

Re: [PATCH 2/2] x86/cpuid: support LFENCE always serializing CPUID bit

[Andrew Cooper]

On 14/04/2021 13:57, Jan Beulich wrote:
> On 14.04.2021 13:04, Roger Pau Monne wrote:
>> @@ -264,6 +265,38 @@ struct cpuid_policy
>>              };
>>              uint32_t nc:8, :4, apic_id_size:4, :16;
>>              uint32_t /* d */:32;
>> +
>> +            uint64_t :64, :64; /* Leaf 0x80000009. */
>> +            uint64_t :64, :64; /* Leaf 0x8000000a - SVM rev and features. */
>> +            uint64_t :64, :64; /* Leaf 0x8000000b. */
>> +            uint64_t :64, :64; /* Leaf 0x8000000c. */
>> +            uint64_t :64, :64; /* Leaf 0x8000000d. */
>> +            uint64_t :64, :64; /* Leaf 0x8000000e. */
>> +            uint64_t :64, :64; /* Leaf 0x8000000f. */
>> +            uint64_t :64, :64; /* Leaf 0x80000010. */
>> +            uint64_t :64, :64; /* Leaf 0x80000011. */
>> +            uint64_t :64, :64; /* Leaf 0x80000012. */
>> +            uint64_t :64, :64; /* Leaf 0x80000013. */
>> +            uint64_t :64, :64; /* Leaf 0x80000014. */
>> +            uint64_t :64, :64; /* Leaf 0x80000015. */
>> +            uint64_t :64, :64; /* Leaf 0x80000016. */
>> +            uint64_t :64, :64; /* Leaf 0x80000017. */
>> +            uint64_t :64, :64; /* Leaf 0x80000018. */
>> +            uint64_t :64, :64; /* Leaf 0x80000019 - TLB 1GB Identifiers. */
>> +            uint64_t :64, :64; /* Leaf 0x8000001a - Performance related info. */
>> +            uint64_t :64, :64; /* Leaf 0x8000001b - IBS feature information. */
>> +            uint64_t :64, :64; /* Leaf 0x8000001c. */
>> +            uint64_t :64, :64; /* Leaf 0x8000001d - Cache properties. */
>> +            uint64_t :64, :64; /* Leaf 0x8000001e - Extd APIC/Core/Node IDs. */
>> +            uint64_t :64, :64; /* Leaf 0x8000001f - AMD Secure Encryption. */
>> +            uint64_t :64, :64; /* Leaf 0x80000020 - Platform QoS. */
>> +
>> +            /* Leaf 0x80000021 - Extended Feature 2 */
>> +            union {
>> +                uint32_t e21a;
>> +                struct { DECL_BITFIELD(e21a); };
>> +            };
>> +            uint32_t /* b */:32, /* c */:32, /* d */:32;
>>          };
>>      } extd;
> Due to the effect of this on what guests get to see, I think this
> wants to take my "x86/CPUID: shrink max_{,sub}leaf fields according
> to actual leaf contents" as a prereq, which in turn may better
> remain on top of "x86/CPUID: adjust extended leaves out of range
> clearing" (both are neighbors in that over 4 months old series,
> fair parts of which could imo go in irrespective of the unsettled
> dispute on xvmalloc() - unfortunately I had made that patch 2 of
> the series, not expecting it to be blocked for so long, and then
> presumably signaling to others that the rest of the series is also
> blocked).

There is no shrinking to be done in this case.  The bit is set across
the board on AMD/Hygon hardware, even on older parts.

What does need changing however is the logic to trim max_extd_leaf down
to what hardware supports, so the bit is visible on Rome/older
hardware.  I.e. after this change, all VMs should get 0x80000021 by
default on AMD hardware.

(A curious observation of Milan hardware is that it actually advertises
0x80000023 as max_extd_leaf, and has two leaves of zeros at the end. 
I've got an open query about this.)

~Andrew

Re: [PATCH 2/2] x86/cpuid: support LFENCE always serializing CPUID bit

[Jan Beulich]

On 14.04.2021 15:05, Andrew Cooper wrote:
> On 14/04/2021 13:57, Jan Beulich wrote:
>> On 14.04.2021 13:04, Roger Pau Monne wrote:
>>> @@ -264,6 +265,38 @@ struct cpuid_policy
>>>              };
>>>              uint32_t nc:8, :4, apic_id_size:4, :16;
>>>              uint32_t /* d */:32;
>>> +
>>> +            uint64_t :64, :64; /* Leaf 0x80000009. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000000a - SVM rev and features. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000000b. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000000c. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000000d. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000000e. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000000f. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000010. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000011. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000012. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000013. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000014. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000015. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000016. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000017. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000018. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000019 - TLB 1GB Identifiers. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000001a - Performance related info. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000001b - IBS feature information. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000001c. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000001d - Cache properties. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000001e - Extd APIC/Core/Node IDs. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000001f - AMD Secure Encryption. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000020 - Platform QoS. */
>>> +
>>> +            /* Leaf 0x80000021 - Extended Feature 2 */
>>> +            union {
>>> +                uint32_t e21a;
>>> +                struct { DECL_BITFIELD(e21a); };
>>> +            };
>>> +            uint32_t /* b */:32, /* c */:32, /* d */:32;
>>>          };
>>>      } extd;
>> Due to the effect of this on what guests get to see, I think this
>> wants to take my "x86/CPUID: shrink max_{,sub}leaf fields according
>> to actual leaf contents" as a prereq, which in turn may better
>> remain on top of "x86/CPUID: adjust extended leaves out of range
>> clearing" (both are neighbors in that over 4 months old series,
>> fair parts of which could imo go in irrespective of the unsettled
>> dispute on xvmalloc() - unfortunately I had made that patch 2 of
>> the series, not expecting it to be blocked for so long, and then
>> presumably signaling to others that the rest of the series is also
>> blocked).
> 
> There is no shrinking to be done in this case.  The bit is set across
> the board on AMD/Hygon hardware, even on older parts.
> 
> What does need changing however is the logic to trim max_extd_leaf down
> to what hardware supports, so the bit is visible on Rome/older
> hardware.  I.e. after this change, all VMs should get 0x80000021 by
> default on AMD hardware.

As to this last sentence - not really. Only if we managed to set the
flag (if it needs setting).

It's a two-way thing really: If the flag ends up set, I agree there's
no need to shrink max_extd_leaf. But if the flag ends up clear, the
issue we have today (hence my patch) becomes worse: Trailing all zero
leaves would result because we don't properly reduce the maximum from
what hardware reports to what is actually populated. In any event - I
think I'd much rather see issues with my patch pointed out, if any.

Jan

Re: [PATCH 2/2] x86/cpuid: support LFENCE always serializing CPUID bit

[Andrew Cooper]

On 14/04/2021 14:05, Andrew Cooper wrote:
> On 14/04/2021 13:57, Jan Beulich wrote:
>> On 14.04.2021 13:04, Roger Pau Monne wrote:
>>> @@ -264,6 +265,38 @@ struct cpuid_policy
>>>              };
>>>              uint32_t nc:8, :4, apic_id_size:4, :16;
>>>              uint32_t /* d */:32;
>>> +
>>> +            uint64_t :64, :64; /* Leaf 0x80000009. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000000a - SVM rev and features. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000000b. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000000c. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000000d. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000000e. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000000f. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000010. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000011. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000012. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000013. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000014. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000015. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000016. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000017. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000018. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000019 - TLB 1GB Identifiers. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000001a - Performance related info. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000001b - IBS feature information. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000001c. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000001d - Cache properties. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000001e - Extd APIC/Core/Node IDs. */
>>> +            uint64_t :64, :64; /* Leaf 0x8000001f - AMD Secure Encryption. */
>>> +            uint64_t :64, :64; /* Leaf 0x80000020 - Platform QoS. */
>>> +
>>> +            /* Leaf 0x80000021 - Extended Feature 2 */
>>> +            union {
>>> +                uint32_t e21a;
>>> +                struct { DECL_BITFIELD(e21a); };
>>> +            };
>>> +            uint32_t /* b */:32, /* c */:32, /* d */:32;
>>>          };
>>>      } extd;
>> Due to the effect of this on what guests get to see, I think this
>> wants to take my "x86/CPUID: shrink max_{,sub}leaf fields according
>> to actual leaf contents" as a prereq, which in turn may better
>> remain on top of "x86/CPUID: adjust extended leaves out of range
>> clearing" (both are neighbors in that over 4 months old series,
>> fair parts of which could imo go in irrespective of the unsettled
>> dispute on xvmalloc() - unfortunately I had made that patch 2 of
>> the series, not expecting it to be blocked for so long, and then
>> presumably signaling to others that the rest of the series is also
>> blocked).
> There is no shrinking to be done in this case.  The bit is set across
> the board on AMD/Hygon hardware, even on older parts.
>
> What does need changing however is the logic to trim max_extd_leaf down
> to what hardware supports, so the bit is visible on Rome/older
> hardware.  I.e. after this change, all VMs should get 0x80000021 by
> default on AMD hardware.
>
> (A curious observation of Milan hardware is that it actually advertises
> 0x80000023 as max_extd_leaf, and has two leaves of zeros at the end. 
> I've got an open query about this.)

Something like this:

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 050cd5713e..d9eb2878c5 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -311,6 +311,7 @@ static void __init calculate_raw_policy(void)
 static void __init calculate_host_policy(void)
 {
     struct cpuid_policy *p = &host_cpuid_policy;
+    unsigned int max_extd_leaf;
 
     *p = raw_cpuid_policy;
 
@@ -318,7 +319,18 @@ static void __init calculate_host_policy(void)
         min_t(uint32_t, p->basic.max_leaf,   ARRAY_SIZE(p->basic.raw) - 1);
     p->feat.max_subleaf =
         min_t(uint32_t, p->feat.max_subleaf, ARRAY_SIZE(p->feat.raw) - 1);
-    p->extd.max_leaf = 0x80000000 | min_t(uint32_t, p->extd.max_leaf &
0xffff,
+
+    max_extd_leaf = p->extd.max_leaf;
+
+    /*
+     * For AMD/Hygon hardware before Zen3, we modify LFENCE to be dispatch
+     * serialsing.  Extend max_extd_leaf beyond what hardware supports, to
+     * include the feature leaf containing this information.
+     */
+    if ( cpu_has_lfence_dispatch )
+        max_extd_leaf = max(max_extd_leaf, 0x80000021);
+
+    p->extd.max_leaf = 0x80000000 | min_t(uint32_t, max_extd_leaf & 0xffff,
                                           ARRAY_SIZE(p->extd.raw) - 1);
 
     cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p);



Only compile tested.  Needs checking on Zen2 or older.  The Raw policy
should have the real max_extd_leaf, while host and derived should have
0x80000021.

~Andrew

Re: [PATCH 2/2] x86/cpuid: support LFENCE always serializing CPUID bit

[Andrew Cooper]

On 14/04/2021 14:24, Jan Beulich wrote:
> On 14.04.2021 15:05, Andrew Cooper wrote:
>> On 14/04/2021 13:57, Jan Beulich wrote:
>>> On 14.04.2021 13:04, Roger Pau Monne wrote:
>>>> @@ -264,6 +265,38 @@ struct cpuid_policy
>>>>              };
>>>>              uint32_t nc:8, :4, apic_id_size:4, :16;
>>>>              uint32_t /* d */:32;
>>>> +
>>>> +            uint64_t :64, :64; /* Leaf 0x80000009. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000a - SVM rev and features. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000b. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000c. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000d. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000e. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000f. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000010. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000011. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000012. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000013. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000014. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000015. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000016. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000017. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000018. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000019 - TLB 1GB Identifiers. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001a - Performance related info. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001b - IBS feature information. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001c. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001d - Cache properties. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001e - Extd APIC/Core/Node IDs. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001f - AMD Secure Encryption. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000020 - Platform QoS. */
>>>> +
>>>> +            /* Leaf 0x80000021 - Extended Feature 2 */
>>>> +            union {
>>>> +                uint32_t e21a;
>>>> +                struct { DECL_BITFIELD(e21a); };
>>>> +            };
>>>> +            uint32_t /* b */:32, /* c */:32, /* d */:32;
>>>>          };
>>>>      } extd;
>>> Due to the effect of this on what guests get to see, I think this
>>> wants to take my "x86/CPUID: shrink max_{,sub}leaf fields according
>>> to actual leaf contents" as a prereq, which in turn may better
>>> remain on top of "x86/CPUID: adjust extended leaves out of range
>>> clearing" (both are neighbors in that over 4 months old series,
>>> fair parts of which could imo go in irrespective of the unsettled
>>> dispute on xvmalloc() - unfortunately I had made that patch 2 of
>>> the series, not expecting it to be blocked for so long, and then
>>> presumably signaling to others that the rest of the series is also
>>> blocked).
>> There is no shrinking to be done in this case.  The bit is set across
>> the board on AMD/Hygon hardware, even on older parts.
>>
>> What does need changing however is the logic to trim max_extd_leaf down
>> to what hardware supports, so the bit is visible on Rome/older
>> hardware.  I.e. after this change, all VMs should get 0x80000021 by
>> default on AMD hardware.
> As to this last sentence - not really. Only if we managed to set the
> flag (if it needs setting).

... or we find it already set.  Remember that we don't even offer an
option to let the user avoid this behaviour.

The only case where we'll boot with lfence not being dispatch
serialising is when we're virutalised under a pre-2018 hypervisor with
has no Spectre knowledge.  There are more important things in life, than
to worry about this case.

> It's a two-way thing really: If the flag ends up set, I agree there's
> no need to shrink max_extd_leaf. But if the flag ends up clear, the
> issue we have today (hence my patch) becomes worse: Trailing all zero
> leaves would result because we don't properly reduce the maximum from
> what hardware reports to what is actually populated. In any event - I
> think I'd much rather see issues with my patch pointed out, if any.

I don't have a problem with the shrinking change in principle, but it is
definitely not a prereq to this change.

Trailing zeroes aren't going to cause guests to malfunction, even if
we'd ideally be neater about the result.

~Andrew

Re: [PATCH 2/2] x86/cpuid: support LFENCE always serializing CPUID bit

[Jan Beulich]

On 14.04.2021 15:25, Andrew Cooper wrote:
> On 14/04/2021 14:05, Andrew Cooper wrote:
>> On 14/04/2021 13:57, Jan Beulich wrote:
>>> On 14.04.2021 13:04, Roger Pau Monne wrote:
>>>> @@ -264,6 +265,38 @@ struct cpuid_policy
>>>>              };
>>>>              uint32_t nc:8, :4, apic_id_size:4, :16;
>>>>              uint32_t /* d */:32;
>>>> +
>>>> +            uint64_t :64, :64; /* Leaf 0x80000009. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000a - SVM rev and features. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000b. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000c. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000d. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000e. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000000f. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000010. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000011. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000012. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000013. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000014. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000015. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000016. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000017. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000018. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000019 - TLB 1GB Identifiers. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001a - Performance related info. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001b - IBS feature information. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001c. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001d - Cache properties. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001e - Extd APIC/Core/Node IDs. */
>>>> +            uint64_t :64, :64; /* Leaf 0x8000001f - AMD Secure Encryption. */
>>>> +            uint64_t :64, :64; /* Leaf 0x80000020 - Platform QoS. */
>>>> +
>>>> +            /* Leaf 0x80000021 - Extended Feature 2 */
>>>> +            union {
>>>> +                uint32_t e21a;
>>>> +                struct { DECL_BITFIELD(e21a); };
>>>> +            };
>>>> +            uint32_t /* b */:32, /* c */:32, /* d */:32;
>>>>          };
>>>>      } extd;
>>> Due to the effect of this on what guests get to see, I think this
>>> wants to take my "x86/CPUID: shrink max_{,sub}leaf fields according
>>> to actual leaf contents" as a prereq, which in turn may better
>>> remain on top of "x86/CPUID: adjust extended leaves out of range
>>> clearing" (both are neighbors in that over 4 months old series,
>>> fair parts of which could imo go in irrespective of the unsettled
>>> dispute on xvmalloc() - unfortunately I had made that patch 2 of
>>> the series, not expecting it to be blocked for so long, and then
>>> presumably signaling to others that the rest of the series is also
>>> blocked).
>> There is no shrinking to be done in this case.  The bit is set across
>> the board on AMD/Hygon hardware, even on older parts.
>>
>> What does need changing however is the logic to trim max_extd_leaf down
>> to what hardware supports, so the bit is visible on Rome/older
>> hardware.  I.e. after this change, all VMs should get 0x80000021 by
>> default on AMD hardware.
>>
>> (A curious observation of Milan hardware is that it actually advertises
>> 0x80000023 as max_extd_leaf, and has two leaves of zeros at the end. 
>> I've got an open query about this.)
> 
> Something like this:
> 
> diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
> index 050cd5713e..d9eb2878c5 100644
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -311,6 +311,7 @@ static void __init calculate_raw_policy(void)
>  static void __init calculate_host_policy(void)
>  {
>      struct cpuid_policy *p = &host_cpuid_policy;
> +    unsigned int max_extd_leaf;
>  
>      *p = raw_cpuid_policy;
>  
> @@ -318,7 +319,18 @@ static void __init calculate_host_policy(void)
>          min_t(uint32_t, p->basic.max_leaf,   ARRAY_SIZE(p->basic.raw) - 1);
>      p->feat.max_subleaf =
>          min_t(uint32_t, p->feat.max_subleaf, ARRAY_SIZE(p->feat.raw) - 1);
> -    p->extd.max_leaf = 0x80000000 | min_t(uint32_t, p->extd.max_leaf &
> 0xffff,
> +
> +    max_extd_leaf = p->extd.max_leaf;
> +
> +    /*
> +     * For AMD/Hygon hardware before Zen3, we modify LFENCE to be dispatch
> +     * serialsing.  Extend max_extd_leaf beyond what hardware supports, to
> +     * include the feature leaf containing this information.
> +     */
> +    if ( cpu_has_lfence_dispatch )
> +        max_extd_leaf = max(max_extd_leaf, 0x80000021);
> +
> +    p->extd.max_leaf = 0x80000000 | min_t(uint32_t, max_extd_leaf & 0xffff,
>                                            ARRAY_SIZE(p->extd.raw) - 1);
>  
>      cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p);

Well, why not set it to ARRAY_SIZE() and then have
x86_cpuid_policy_shrink_max_leaves() (from "x86/CPUID: shrink
max_{,sub}leaf fields according to actual leaf contents") have
a go? It'll recognize the non-zero leaf ... Otherwise, if we
gain a few more such special cases, things are going to get
ugly here.

Jan

Re: [PATCH 2/2] x86/cpuid: support LFENCE always serializing CPUID bit

[Roger Pau Monné]

On Wed, Apr 14, 2021 at 03:57:22PM +0200, Jan Beulich wrote:
> On 14.04.2021 15:25, Andrew Cooper wrote:
> > On 14/04/2021 14:05, Andrew Cooper wrote:
> >> On 14/04/2021 13:57, Jan Beulich wrote:
> >>> On 14.04.2021 13:04, Roger Pau Monne wrote:
> >>>> @@ -264,6 +265,38 @@ struct cpuid_policy
> >>>>              };
> >>>>              uint32_t nc:8, :4, apic_id_size:4, :16;
> >>>>              uint32_t /* d */:32;
> >>>> +
> >>>> +            uint64_t :64, :64; /* Leaf 0x80000009. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x8000000a - SVM rev and features. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x8000000b. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x8000000c. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x8000000d. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x8000000e. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x8000000f. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x80000010. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x80000011. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x80000012. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x80000013. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x80000014. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x80000015. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x80000016. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x80000017. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x80000018. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x80000019 - TLB 1GB Identifiers. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x8000001a - Performance related info. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x8000001b - IBS feature information. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x8000001c. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x8000001d - Cache properties. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x8000001e - Extd APIC/Core/Node IDs. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x8000001f - AMD Secure Encryption. */
> >>>> +            uint64_t :64, :64; /* Leaf 0x80000020 - Platform QoS. */
> >>>> +
> >>>> +            /* Leaf 0x80000021 - Extended Feature 2 */
> >>>> +            union {
> >>>> +                uint32_t e21a;
> >>>> +                struct { DECL_BITFIELD(e21a); };
> >>>> +            };
> >>>> +            uint32_t /* b */:32, /* c */:32, /* d */:32;
> >>>>          };
> >>>>      } extd;
> >>> Due to the effect of this on what guests get to see, I think this
> >>> wants to take my "x86/CPUID: shrink max_{,sub}leaf fields according
> >>> to actual leaf contents" as a prereq, which in turn may better
> >>> remain on top of "x86/CPUID: adjust extended leaves out of range
> >>> clearing" (both are neighbors in that over 4 months old series,
> >>> fair parts of which could imo go in irrespective of the unsettled
> >>> dispute on xvmalloc() - unfortunately I had made that patch 2 of
> >>> the series, not expecting it to be blocked for so long, and then
> >>> presumably signaling to others that the rest of the series is also
> >>> blocked).
> >> There is no shrinking to be done in this case.  The bit is set across
> >> the board on AMD/Hygon hardware, even on older parts.
> >>
> >> What does need changing however is the logic to trim max_extd_leaf down
> >> to what hardware supports, so the bit is visible on Rome/older
> >> hardware.  I.e. after this change, all VMs should get 0x80000021 by
> >> default on AMD hardware.
> >>
> >> (A curious observation of Milan hardware is that it actually advertises
> >> 0x80000023 as max_extd_leaf, and has two leaves of zeros at the end. 
> >> I've got an open query about this.)
> > 
> > Something like this:
> > 
> > diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
> > index 050cd5713e..d9eb2878c5 100644
> > --- a/xen/arch/x86/cpuid.c
> > +++ b/xen/arch/x86/cpuid.c
> > @@ -311,6 +311,7 @@ static void __init calculate_raw_policy(void)
> >  static void __init calculate_host_policy(void)
> >  {
> >      struct cpuid_policy *p = &host_cpuid_policy;
> > +    unsigned int max_extd_leaf;
> >  
> >      *p = raw_cpuid_policy;
> >  
> > @@ -318,7 +319,18 @@ static void __init calculate_host_policy(void)
> >          min_t(uint32_t, p->basic.max_leaf,   ARRAY_SIZE(p->basic.raw) - 1);
> >      p->feat.max_subleaf =
> >          min_t(uint32_t, p->feat.max_subleaf, ARRAY_SIZE(p->feat.raw) - 1);
> > -    p->extd.max_leaf = 0x80000000 | min_t(uint32_t, p->extd.max_leaf &
> > 0xffff,
> > +
> > +    max_extd_leaf = p->extd.max_leaf;
> > +
> > +    /*
> > +     * For AMD/Hygon hardware before Zen3, we modify LFENCE to be dispatch
> > +     * serialsing.  Extend max_extd_leaf beyond what hardware supports, to
> > +     * include the feature leaf containing this information.
> > +     */
> > +    if ( cpu_has_lfence_dispatch )
> > +        max_extd_leaf = max(max_extd_leaf, 0x80000021);
> > +
> > +    p->extd.max_leaf = 0x80000000 | min_t(uint32_t, max_extd_leaf & 0xffff,
> >                                            ARRAY_SIZE(p->extd.raw) - 1);
> >  
> >      cpuid_featureset_to_policy(boot_cpu_data.x86_capability, p);
> 
> Well, why not set it to ARRAY_SIZE() and then have
> x86_cpuid_policy_shrink_max_leaves() (from "x86/CPUID: shrink
> max_{,sub}leaf fields according to actual leaf contents") have
> a go? It'll recognize the non-zero leaf ... Otherwise, if we
> gain a few more such special cases, things are going to get
> ugly here.

I will wait for Jan to post the updated version of his shrink patch
and then rebase mine on top in order to set extd.max_leaf to
ARRAY_SIZE and let the shrink logic deal with it.

Thanks, Roger.