[PATCH v2 0/3] Add CpuidUserDis support

[PATCH v2 0/3] Add CpuidUserDis support

[Alejandro Vallejo]

v2:
 * Style changes
 * Remove v1/patch3: HVM not to be addressed by this series
 * Adds one patch between v1/patch1 and v1/patch2 with the vendor-specific
   refactor of probe_cpuid_faulting()

Nowadays AMD supports trapping the CPUID instruction from ring>0 to ring0,
(CpuidUserDis) akin to Intel's "CPUID faulting". There is a difference in
that the toggle bit is in a different MSR and the support bit is in CPUID
itself rather than yet another MSR. This patch enables AMD hosts to use it
when supported in order to provide correct CPUID contents to PV guests.

Patch 1 merely adds definitions to various places in CPUID and MSR

Patch 2 moves vendor-specific code on probe_cpuid_faulting() to amd.c/intel.c

Patch 3 adds support for CpuidUserDis, hooking it in the probing path and
the context switching path.

Alejandro Vallejo (3):
  x86: Add AMD's CpuidUserDis bit definitions
  x86: Refactor conditional guard in probe_cpuid_faulting()
  x86: Add support for CpuidUserDis

 tools/libs/light/libxl_cpuid.c              |  1 +
 tools/misc/xen-cpuid.c                      |  2 +
 xen/arch/x86/cpu/amd.c                      | 28 ++++++++++-
 xen/arch/x86/cpu/common.c                   | 51 +++++++++++----------
 xen/arch/x86/cpu/intel.c                    | 12 ++++-
 xen/arch/x86/include/asm/amd.h              |  1 +
 xen/arch/x86/include/asm/msr-index.h        |  1 +
 xen/include/public/arch-x86/cpufeatureset.h |  1 +
 8 files changed, 71 insertions(+), 26 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] x86: Add AMD's CpuidUserDis bit definitions

[Alejandro Vallejo]

AMD reports support for CpuidUserDis in CPUID and provides the toggle in HWCR.
This patch adds the positions of both of those bits to both xen and tools.

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 tools/libs/light/libxl_cpuid.c              | 1 +
 tools/misc/xen-cpuid.c                      | 2 ++
 xen/arch/x86/include/asm/msr-index.h        | 1 +
 xen/include/public/arch-x86/cpufeatureset.h | 1 +
 4 files changed, 5 insertions(+)

diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index 5f0bf93810..4d2fab5414 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -317,6 +317,7 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
 
         {"lfence+",      0x80000021, NA, CPUID_REG_EAX,  2,  1},
         {"nscb",         0x80000021, NA, CPUID_REG_EAX,  6,  1},
+        {"cpuid-user-dis", 0x80000021, NA, CPUID_REG_EAX, 17,  1},
 
         {"maxhvleaf",    0x40000000, NA, CPUID_REG_EAX,  0,  8},
 
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index d7efc59d31..8ec143ebc8 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -199,6 +199,8 @@ static const char *const str_e21a[32] =
 {
     [ 2] = "lfence+",
     [ 6] = "nscb",
+
+    /* 16 */                [17] = "cpuid-user-dis",
 };
 
 static const char *const str_7b1[32] =
diff --git a/xen/arch/x86/include/asm/msr-index.h b/xen/arch/x86/include/asm/msr-index.h
index fa771ed0b5..082fb2e0d9 100644
--- a/xen/arch/x86/include/asm/msr-index.h
+++ b/xen/arch/x86/include/asm/msr-index.h
@@ -337,6 +337,7 @@
 
 #define MSR_K8_HWCR			0xc0010015
 #define K8_HWCR_TSC_FREQ_SEL		(1ULL << 24)
+#define K8_HWCR_CPUID_USER_DIS		(1ULL << 35)
 
 #define MSR_K7_FID_VID_CTL		0xc0010041
 #define MSR_K7_FID_VID_STATUS		0xc0010042
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 12e3dc80c6..623dcb1bce 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -287,6 +287,7 @@ XEN_CPUFEATURE(AVX_IFMA,     10*32+23) /*A  AVX-IFMA Instructions */
 /* AMD-defined CPU features, CPUID level 0x80000021.eax, word 11 */
 XEN_CPUFEATURE(LFENCE_DISPATCH,    11*32+ 2) /*A  LFENCE always serializing */
 XEN_CPUFEATURE(NSCB,               11*32+ 6) /*A  Null Selector Clears Base (and limit too) */
+XEN_CPUFEATURE(CPUID_USER_DIS,     11*32+17) /*   CPUID disable for non-privileged software */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:1.ebx, word 12 */
 XEN_CPUFEATURE(INTEL_PPIN,         12*32+ 0) /*   Protected Processor Inventory Number */
-- 
2.34.1

Re: [PATCH v2 1/3] x86: Add AMD's CpuidUserDis bit definitions

[Jan Beulich]

On 09.05.2023 18:43, Alejandro Vallejo wrote:
> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -287,6 +287,7 @@ XEN_CPUFEATURE(AVX_IFMA,     10*32+23) /*A  AVX-IFMA Instructions */
>  /* AMD-defined CPU features, CPUID level 0x80000021.eax, word 11 */
>  XEN_CPUFEATURE(LFENCE_DISPATCH,    11*32+ 2) /*A  LFENCE always serializing */
>  XEN_CPUFEATURE(NSCB,               11*32+ 6) /*A  Null Selector Clears Base (and limit too) */
> +XEN_CPUFEATURE(CPUID_USER_DIS,     11*32+17) /*   CPUID disable for non-privileged software */

While I can accept your argument towards staying close to AMD's doc
with the name, I'd really like to ask that the comment then be
disambiguated: "non-privileged" is more likely mean CPL=3 than all
of CPL>0. Since "not fully privileged" is getting a little long,
maybe indeed say "CPL > 0 software"? I would then offer you my R-b,
if only I could find proof of the HWCR bit being bit 35. The PM
mentions it only by name, and the PPRs I've checked all have it
marked reserved.

Jan

Re: [PATCH v2 1/3] x86: Add AMD's CpuidUserDis bit definitions

[Alejandro Vallejo]

On Thu, May 11, 2023 at 11:41:13AM +0200, Jan Beulich wrote:
> On 09.05.2023 18:43, Alejandro Vallejo wrote:
> > --- a/xen/include/public/arch-x86/cpufeatureset.h
> > +++ b/xen/include/public/arch-x86/cpufeatureset.h
> > @@ -287,6 +287,7 @@ XEN_CPUFEATURE(AVX_IFMA,     10*32+23) /*A  AVX-IFMA Instructions */
> >  /* AMD-defined CPU features, CPUID level 0x80000021.eax, word 11 */
> >  XEN_CPUFEATURE(LFENCE_DISPATCH,    11*32+ 2) /*A  LFENCE always serializing */
> >  XEN_CPUFEATURE(NSCB,               11*32+ 6) /*A  Null Selector Clears Base (and limit too) */
> > +XEN_CPUFEATURE(CPUID_USER_DIS,     11*32+17) /*   CPUID disable for non-privileged software */
> 
> While I can accept your argument towards staying close to AMD's doc
> with the name, I'd really like to ask that the comment then be
> disambiguated: "non-privileged" is more likely mean CPL=3 than all
> of CPL>0. Since "not fully privileged" is getting a little long,
> maybe indeed say "CPL > 0 software"? [...]

Fair point. That was copied from AMD's PM, but there's no good reason to
keep it that way. I'll modify it as you pointed out.

> I would then offer you my R-b,
> if only I could find proof of the HWCR bit being bit 35. The PM
> mentions it only by name, and the PPRs I've checked all have it
> marked reserved.
> 
> Jan

It is in the Vol2 of the PM. Section 3.2.10 on the HWCR. I'm looking at
revision 4.06, from January 2023.

---
CpuidUserDis. Bit 35. Setting this bit to 1 causes #GP(0) when the CPUID
instruction is executed by non-privileged software (CPL > 0) outside SMM.
Support for the CPUID User Disable feature is indicated by CPUID
Fn80000021_EAX[CpuidUserDis]=1.
---

Alejandro

Re: [PATCH v2 1/3] x86: Add AMD's CpuidUserDis bit definitions

[Jan Beulich]

On 11.05.2023 12:38, Alejandro Vallejo wrote:
> On Thu, May 11, 2023 at 11:41:13AM +0200, Jan Beulich wrote:
>> On 09.05.2023 18:43, Alejandro Vallejo wrote:
>>> --- a/xen/include/public/arch-x86/cpufeatureset.h
>>> +++ b/xen/include/public/arch-x86/cpufeatureset.h
>>> @@ -287,6 +287,7 @@ XEN_CPUFEATURE(AVX_IFMA,     10*32+23) /*A  AVX-IFMA Instructions */
>>>  /* AMD-defined CPU features, CPUID level 0x80000021.eax, word 11 */
>>>  XEN_CPUFEATURE(LFENCE_DISPATCH,    11*32+ 2) /*A  LFENCE always serializing */
>>>  XEN_CPUFEATURE(NSCB,               11*32+ 6) /*A  Null Selector Clears Base (and limit too) */
>>> +XEN_CPUFEATURE(CPUID_USER_DIS,     11*32+17) /*   CPUID disable for non-privileged software */
>>
>> While I can accept your argument towards staying close to AMD's doc
>> with the name, I'd really like to ask that the comment then be
>> disambiguated: "non-privileged" is more likely mean CPL=3 than all
>> of CPL>0. Since "not fully privileged" is getting a little long,
>> maybe indeed say "CPL > 0 software"? [...]
> 
> Fair point. That was copied from AMD's PM, but there's no good reason to
> keep it that way. I'll modify it as you pointed out.
> 
>> I would then offer you my R-b,
>> if only I could find proof of the HWCR bit being bit 35. The PM
>> mentions it only by name, and the PPRs I've checked all have it
>> marked reserved.
> 
> It is in the Vol2 of the PM. Section 3.2.10 on the HWCR. I'm looking at
> revision 4.06, from January 2023.

Oh, my fault then: It didn't even occur to me to check Vol 2, as normally
it's the other way around: Only the PPRs can be sufficiently relied upon
to be at least halfway complete.

With the comment adjustment (which I'd also be okay to do while committing)
then
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

[PATCH v2 2/3] x86: Refactor conditional guard in probe_cpuid_faulting()

[Alejandro Vallejo]

Move vendor-specific checks to the vendor-specific callers.

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v2:
  * Patch factored out from patch2 of v1
---
 xen/arch/x86/cpu/amd.c    | 10 +++++++++-
 xen/arch/x86/cpu/common.c | 11 -----------
 xen/arch/x86/cpu/intel.c  |  9 ++++++++-
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index caafe44740..899bae7a10 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -271,7 +271,15 @@ static void __init noinline amd_init_levelling(void)
 {
 	const struct cpuidmask *m = NULL;
 
-	if (probe_cpuid_faulting())
+	/*
+	 * If there's support for CpuidUserDis or CPUID faulting then
+	 * we can skip levelling because CPUID accesses are trapped anyway.
+	 *
+	 * CPUID faulting is an Intel feature analogous to CpuidUserDis, so
+	 * that can only be present when Xen is itself virtualized (because
+	 * it can be emulated)
+	 */
+	if (cpu_has_hypervisor && probe_cpuid_faulting())
 		return;
 
 	probe_masking_msrs();
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index edc4db1335..4bfaac4590 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -131,17 +131,6 @@ bool __init probe_cpuid_faulting(void)
 	uint64_t val;
 	int rc;
 
-	/*
-	 * Don't bother looking for CPUID faulting if we aren't virtualised on
-	 * AMD or Hygon hardware - it won't be present.  Likewise for Fam0F
-	 * Intel hardware.
-	 */
-	if (((boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) ||
-	     ((boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) &&
-	      boot_cpu_data.x86 == 0xf)) &&
-	    !cpu_has_hypervisor)
-		return false;
-
 	if ((rc = rdmsr_safe(MSR_INTEL_PLATFORM_INFO, val)) == 0)
 		raw_cpu_policy.platform_info.cpuid_faulting =
 			val & MSR_PLATFORM_INFO_CPUID_FAULTING;
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index 71fc1a1e18..a04414ba1d 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -226,7 +226,14 @@ static void cf_check intel_ctxt_switch_masking(const struct vcpu *next)
  */
 static void __init noinline intel_init_levelling(void)
 {
-	if (probe_cpuid_faulting())
+	/*
+	 * Intel Fam0f is old enough that probing for CPUID faulting support
+	 * introduces spurious #GP(0) when the appropriate MSRs are read,
+	 * so skip it altogether. In the case where Xen is virtualized these
+	 * MSRs may be emulated though, so we allow it in that case.
+	 */
+	if ((boot_cpu_data.x86 != 0xf || cpu_has_hypervisor) &&
+	    probe_cpuid_faulting())
 		return;
 
 	probe_masking_msrs();
-- 
2.34.1

Re: [PATCH v2 2/3] x86: Refactor conditional guard in probe_cpuid_faulting()

[Jan Beulich]

On 09.05.2023 18:43, Alejandro Vallejo wrote:
> Move vendor-specific checks to the vendor-specific callers.
> 
> No functional change.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

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

[PATCH v2 3/3] x86: Add support for CpuidUserDis

[Alejandro Vallejo]

Because CpuIdUserDis is reported in CPUID itself, the extended leaf
containing that bit must be retrieved before calling c_early_init()

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v2:
 * Style fixes
 * MSR index inlined in rdmsr/wrmsr
 * Swapped Intel's conditional guard so typically true condition goes first
 * Factored the vendor-specific non functional changes into another patch
---
 xen/arch/x86/cpu/amd.c         | 20 ++++++++++++++++-
 xen/arch/x86/cpu/common.c      | 40 +++++++++++++++++++++++-----------
 xen/arch/x86/cpu/intel.c       |  5 ++++-
 xen/arch/x86/include/asm/amd.h |  1 +
 4 files changed, 51 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 899bae7a10..cc9c89fd19 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -279,8 +279,12 @@ static void __init noinline amd_init_levelling(void)
 	 * that can only be present when Xen is itself virtualized (because
 	 * it can be emulated)
 	 */
-	if (cpu_has_hypervisor && probe_cpuid_faulting())
+	if ((cpu_has_hypervisor && probe_cpuid_faulting()) ||
+	    boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)) {
+		expected_levelling_cap |= LCAP_faulting;
+		levelling_caps |=  LCAP_faulting;
 		return;
+	}
 
 	probe_masking_msrs();
 
@@ -371,6 +375,20 @@ static void __init noinline amd_init_levelling(void)
 		ctxt_switch_masking = amd_ctxt_switch_masking;
 }
 
+void amd_set_cpuid_user_dis(bool enable)
+{
+	const uint64_t bit = K8_HWCR_CPUID_USER_DIS;
+	uint64_t val;
+
+	rdmsrl(MSR_K8_HWCR, val);
+
+	if (!!(val & bit) == enable)
+		return;
+
+	val ^= bit;
+	wrmsrl(MSR_K8_HWCR, val);
+}
+
 /*
  * Check for the presence of an AMD erratum. Arguments are defined in amd.h 
  * for each known erratum. Return 1 if erratum is found.
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 4bfaac4590..9bbb385db4 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -4,6 +4,7 @@
 #include <xen/param.h>
 #include <xen/smp.h>
 
+#include <asm/amd.h>
 #include <asm/cpu-policy.h>
 #include <asm/current.h>
 #include <asm/debugreg.h>
@@ -144,8 +145,6 @@ bool __init probe_cpuid_faulting(void)
 		return false;
 	}
 
-	expected_levelling_cap |= LCAP_faulting;
-	levelling_caps |=  LCAP_faulting;
 	setup_force_cpu_cap(X86_FEATURE_CPUID_FAULTING);
 
 	return true;
@@ -168,8 +167,10 @@ static void set_cpuid_faulting(bool enable)
 void ctxt_switch_levelling(const struct vcpu *next)
 {
 	const struct domain *nextd = next ? next->domain : NULL;
+	bool enable_cpuid_faulting;
 
-	if (cpu_has_cpuid_faulting) {
+	if (cpu_has_cpuid_faulting ||
+	    boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)) {
 		/*
 		 * No need to alter the faulting setting if we are switching
 		 * to idle; it won't affect any code running in idle context.
@@ -190,12 +191,18 @@ void ctxt_switch_levelling(const struct vcpu *next)
 		 * an interim escape hatch in the form of
 		 * `dom0=no-cpuid-faulting` to restore the older behaviour.
 		 */
-		set_cpuid_faulting(nextd && (opt_dom0_cpuid_faulting ||
-					     !is_control_domain(nextd) ||
-					     !is_pv_domain(nextd)) &&
-				   (is_pv_domain(nextd) ||
-				    next->arch.msrs->
-				    misc_features_enables.cpuid_faulting));
+		enable_cpuid_faulting = nextd && (opt_dom0_cpuid_faulting ||
+		                                  !is_control_domain(nextd) ||
+		                                  !is_pv_domain(nextd)) &&
+		                        (is_pv_domain(nextd) ||
+		                         next->arch.msrs->
+		                         misc_features_enables.cpuid_faulting);
+
+		if (cpu_has_cpuid_faulting)
+			set_cpuid_faulting(enable_cpuid_faulting);
+		else
+			amd_set_cpuid_user_dis(enable_cpuid_faulting);
+
 		return;
 	}
 
@@ -404,6 +411,17 @@ static void generic_identify(struct cpuinfo_x86 *c)
 	c->apicid = phys_pkg_id((ebx >> 24) & 0xFF, 0);
 	c->phys_proc_id = c->apicid;
 
+	eax = cpuid_eax(0x80000000);
+	if ((eax >> 16) == 0x8000)
+		c->extended_cpuid_level = eax;
+
+	/*
+	 * These AMD-defined flags are out of place, but we need
+	 * them early for the CPUID faulting probe code
+	 */
+	if (c->extended_cpuid_level >= 0x80000021)
+		c->x86_capability[FEATURESET_e21a] = cpuid_eax(0x80000021);
+
 	if (this_cpu->c_early_init)
 		this_cpu->c_early_init(c);
 
@@ -420,10 +438,6 @@ static void generic_identify(struct cpuinfo_x86 *c)
 	     (cpuid_ecx(CPUID_PM_LEAF) & CPUID6_ECX_APERFMPERF_CAPABILITY) )
 		__set_bit(X86_FEATURE_APERFMPERF, c->x86_capability);
 
-	eax = cpuid_eax(0x80000000);
-	if ((eax >> 16) == 0x8000)
-		c->extended_cpuid_level = eax;
-
 	/* AMD-defined flags: level 0x80000001 */
 	if (c->extended_cpuid_level >= 0x80000001)
 		cpuid(0x80000001, &tmp, &tmp,
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index a04414ba1d..bbe7b7d1ce 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -233,8 +233,11 @@ static void __init noinline intel_init_levelling(void)
 	 * MSRs may be emulated though, so we allow it in that case.
 	 */
 	if ((boot_cpu_data.x86 != 0xf || cpu_has_hypervisor) &&
-	    probe_cpuid_faulting())
+	    probe_cpuid_faulting()) {
+		expected_levelling_cap |= LCAP_faulting;
+		levelling_caps |=  LCAP_faulting;
 		return;
+	}
 
 	probe_masking_msrs();
 
diff --git a/xen/arch/x86/include/asm/amd.h b/xen/arch/x86/include/asm/amd.h
index a975d3de26..09ee52dc1c 100644
--- a/xen/arch/x86/include/asm/amd.h
+++ b/xen/arch/x86/include/asm/amd.h
@@ -155,5 +155,6 @@ extern bool amd_legacy_ssbd;
 extern bool amd_virt_spec_ctrl;
 bool amd_setup_legacy_ssbd(void);
 void amd_set_legacy_ssbd(bool enable);
+void amd_set_cpuid_user_dis(bool enable);
 
 #endif /* __AMD_H__ */
-- 
2.34.1

Re: [PATCH v2 3/3] x86: Add support for CpuidUserDis

[Jan Beulich]

On 09.05.2023 18:43, Alejandro Vallejo wrote:
> Because CpuIdUserDis is reported in CPUID itself, the extended leaf
> containing that bit must be retrieved before calling c_early_init()
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Looks largely okay when taken together with patch 2, but ...

> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -279,8 +279,12 @@ static void __init noinline amd_init_levelling(void)
>  	 * that can only be present when Xen is itself virtualized (because
>  	 * it can be emulated)
>  	 */
> -	if (cpu_has_hypervisor && probe_cpuid_faulting())
> +	if ((cpu_has_hypervisor && probe_cpuid_faulting()) ||
> +	    boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)) {

... imo the probe_cpuid_faulting() call would better be avoided when
the CPUID bit is set.

> +		expected_levelling_cap |= LCAP_faulting;
> +		levelling_caps |=  LCAP_faulting;

Further the movement of these two lines from ...

> @@ -144,8 +145,6 @@ bool __init probe_cpuid_faulting(void)
>  		return false;
>  	}
>  
> -	expected_levelling_cap |= LCAP_faulting;
> -	levelling_caps |=  LCAP_faulting;
>  	setup_force_cpu_cap(X86_FEATURE_CPUID_FAULTING);

... here (and also to intel.c) should imo be part of patch 2. While
moving them, I think you also want to deal with the stray double
blank.

Jan

Re: [PATCH v2 3/3] x86: Add support for CpuidUserDis

[Alejandro Vallejo]

On Thu, May 11, 2023 at 01:05:42PM +0200, Jan Beulich wrote:
> > --- a/xen/arch/x86/cpu/amd.c
> > +++ b/xen/arch/x86/cpu/amd.c
> > @@ -279,8 +279,12 @@ static void __init noinline amd_init_levelling(void)
> >  	 * that can only be present when Xen is itself virtualized (because
> >  	 * it can be emulated)
> >  	 */
> > -	if (cpu_has_hypervisor && probe_cpuid_faulting())
> > +	if ((cpu_has_hypervisor && probe_cpuid_faulting()) ||
> > +	    boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)) {
> 
> ... imo the probe_cpuid_faulting() call would better be avoided when
> the CPUID bit is set.

I wrote it like that originally. However, it felt wrong to leave
raw_policy.platform_info unset, as it's set inside probe_cpuid_faulting().
While it's highly unlikely a real AMD machine will have CPUID faulting
support, Xen might see both if it's itself virtualized under Xen.

The crux of the matter here is whether we want the raw policy to be an
accurate representation of _all_ the features of the machine (real or
virtual) or we're ok with it not having features we don't intend to use in
practice. It certainly can be argued either way. CpuidUserDis naturally
gets to the policy through CPUID leaf enumeration, so that's done
regardless.

My .02 is that raw means uncooked and as such should have the actual
physical features reported by the machine, but I could be persuaded either
way.

> 
> > +		expected_levelling_cap |= LCAP_faulting;
> > +		levelling_caps |=  LCAP_faulting;
> 
> Further the movement of these two lines from ...
> 
> > @@ -144,8 +145,6 @@ bool __init probe_cpuid_faulting(void)
> >  		return false;
> >  	}
> >  
> > -	expected_levelling_cap |= LCAP_faulting;
> > -	levelling_caps |=  LCAP_faulting;
> >  	setup_force_cpu_cap(X86_FEATURE_CPUID_FAULTING);
> 
> ... here (and also to intel.c) should imo be part of patch 2. While
> moving them, I think you also want to deal with the stray double
> blank.
> 
> Jan

Sure.

Alejandro

Re: [PATCH v2 3/3] x86: Add support for CpuidUserDis

[Jan Beulich]

On 11.05.2023 14:12, Alejandro Vallejo wrote:
> On Thu, May 11, 2023 at 01:05:42PM +0200, Jan Beulich wrote:
>>> --- a/xen/arch/x86/cpu/amd.c
>>> +++ b/xen/arch/x86/cpu/amd.c
>>> @@ -279,8 +279,12 @@ static void __init noinline amd_init_levelling(void)
>>>  	 * that can only be present when Xen is itself virtualized (because
>>>  	 * it can be emulated)
>>>  	 */
>>> -	if (cpu_has_hypervisor && probe_cpuid_faulting())
>>> +	if ((cpu_has_hypervisor && probe_cpuid_faulting()) ||
>>> +	    boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)) {
>>
>> ... imo the probe_cpuid_faulting() call would better be avoided when
>> the CPUID bit is set.
> 
> I wrote it like that originally. However, it felt wrong to leave
> raw_policy.platform_info unset, as it's set inside probe_cpuid_faulting().
> While it's highly unlikely a real AMD machine will have CPUID faulting
> support, Xen might see both if it's itself virtualized under Xen.
> 
> The crux of the matter here is whether we want the raw policy to be an
> accurate representation of _all_ the features of the machine (real or
> virtual) or we're ok with it not having features we don't intend to use in
> practice. It certainly can be argued either way. CpuidUserDis naturally
> gets to the policy through CPUID leaf enumeration, so that's done
> regardless.
> 
> My .02 is that raw means uncooked and as such should have the actual
> physical features reported by the machine, but I could be persuaded either
> way.

I think I would be okay if that was (in perhaps slightly abridged form)
made part of the description (or if the code comment there said so, then
also preventing someone [like me] coming and re-ordering the conditional).

Nevertheless having raw_policy populated like this seems a little fragile
in the first place. Andrew - any particular thoughts from you in this
regard?

Jan