[PATCH 0/3] Add CpuidUserDis support

[PATCH 0/3] Add CpuidUserDis support

[Alejandro Vallejo]

Nowadays AMD supports trapping the CPUID instruction from ring3 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. Also
allows HVM guests to use CpuidUserDis via emulated "CPUID faulting".

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

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

Patch 3 enables HVM guests to use CpuidUserDis as if it was CPUID faulting,
saving an avoidable roundtrip through the hypervisor at fault handling.

Alejandro Vallejo (3):
  x86: Add AMD's CpuidUserDis bit definitions
  x86: Add support for CpuidUserDis
  x86: Use CpuidUserDis if an AMD HVM guest toggles CPUID faulting

 tools/libs/light/libxl_cpuid.c              |  1 +
 tools/misc/xen-cpuid.c                      |  2 +
 xen/arch/x86/cpu/amd.c                      | 29 +++++++++++-
 xen/arch/x86/cpu/common.c                   | 51 +++++++++++----------
 xen/arch/x86/cpu/intel.c                    | 11 ++++-
 xen/arch/x86/include/asm/amd.h              |  1 +
 xen/arch/x86/include/asm/msr-index.h        |  1 +
 xen/arch/x86/msr.c                          |  9 +++-
 xen/include/public/arch-x86/cpufeatureset.h |  1 +
 9 files changed, 79 insertions(+), 27 deletions(-)

-- 
2.34.1

[PATCH 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

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

[Alejandro Vallejo]

Includes a refactor to move vendor-specific probes to vendor-specific
files. Furthermore, 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>
---
 xen/arch/x86/cpu/amd.c         | 29 ++++++++++++++++++-
 xen/arch/x86/cpu/common.c      | 51 ++++++++++++++++++----------------
 xen/arch/x86/cpu/intel.c       | 11 +++++++-
 xen/arch/x86/include/asm/amd.h |  1 +
 4 files changed, 66 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index caafe44740..9269015edd 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -271,8 +271,20 @@ 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()) ||
+	    boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)) {
+		expected_levelling_cap |= LCAP_faulting;
+		levelling_caps |=  LCAP_faulting;
 		return;
+	}
 
 	probe_masking_msrs();
 
@@ -363,6 +375,21 @@ 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 msr_addr = MSR_K8_HWCR;
+	const uint64_t bit = K8_HWCR_CPUID_USER_DIS;
+	uint64_t val;
+
+	rdmsrl(msr_addr, val);
+
+	if (!!(val & bit) == enable)
+		return;
+
+	val ^= bit;
+	wrmsrl(msr_addr, 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 edc4db1335..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>
@@ -131,17 +132,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;
@@ -155,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;
@@ -179,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.
@@ -201,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;
 	}
 
@@ -415,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);
 
@@ -431,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 71fc1a1e18..7e5c657758 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -226,8 +226,17 @@ 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 ((cpu_has_hypervisor || boot_cpu_data.x86 !=0xf) &&
+	    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

[PATCH 3/3] x86: Use CpuidUserDis if an AMD HVM guest toggles CPUID faulting

[Alejandro Vallejo]

This is in order to aid guests of AMD hardware that we have exposed
CPUID faulting to. If they try to modify the Intel MSR that enables
the feature, trigger levelling so AMD's version of it (CpuidUserDis)
is used instead.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/arch/x86/msr.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index ecf126566d..984aedf180 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -431,6 +431,13 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     {
         bool old_cpuid_faulting = msrs->misc_features_enables.cpuid_faulting;
 
+        /*
+         * The boot CPU must support Intel's CPUID faulting _or_
+         * AMD's CpuidUserDis.
+         */
+        bool can_fault_cpuid = cpu_has_cpuid_faulting ||
+                               boot_cpu_has(X86_FEATURE_CPUID_USER_DIS);
+
         rsvd = ~0ull;
         if ( cp->platform_info.cpuid_faulting )
             rsvd &= ~MSR_MISC_FEATURES_CPUID_FAULTING;
@@ -440,7 +447,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
 
         msrs->misc_features_enables.raw = val;
 
-        if ( v == curr && is_hvm_domain(d) && cpu_has_cpuid_faulting &&
+        if ( v == curr && is_hvm_domain(d) && can_fault_cpuid &&
              (old_cpuid_faulting ^ msrs->misc_features_enables.cpuid_faulting) )
             ctxt_switch_levelling(v);
         break;
-- 
2.34.1

Re: [PATCH 0/3] Add CpuidUserDis support

[Jan Beulich]

On 05.05.2023 19:57, Alejandro Vallejo wrote:
> Nowadays AMD supports trapping the CPUID instruction from ring3 to ring0,

Since it's relevant for PV32: Their doc talks about CPL > 0, i.e. not just
ring 3. Therefore I wonder whether ...

> (CpuidUserDis)

... we shouldn't deviate from the PM and avoid the misleading use of "user"
in our internal naming.

Jan

> 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. Also
> allows HVM guests to use CpuidUserDis via emulated "CPUID faulting".
> 
> Patch 1 merely adds definitions to various places in CPUID and MSR
> 
> Patch 2 adds support for CpuidUserDis, hooking it in the probing path and
> the context switching path.
> 
> Patch 3 enables HVM guests to use CpuidUserDis as if it was CPUID faulting,
> saving an avoidable roundtrip through the hypervisor at fault handling.
> 
> Alejandro Vallejo (3):
>   x86: Add AMD's CpuidUserDis bit definitions
>   x86: Add support for CpuidUserDis
>   x86: Use CpuidUserDis if an AMD HVM guest toggles CPUID faulting
> 
>  tools/libs/light/libxl_cpuid.c              |  1 +
>  tools/misc/xen-cpuid.c                      |  2 +
>  xen/arch/x86/cpu/amd.c                      | 29 +++++++++++-
>  xen/arch/x86/cpu/common.c                   | 51 +++++++++++----------
>  xen/arch/x86/cpu/intel.c                    | 11 ++++-
>  xen/arch/x86/include/asm/amd.h              |  1 +
>  xen/arch/x86/include/asm/msr-index.h        |  1 +
>  xen/arch/x86/msr.c                          |  9 +++-
>  xen/include/public/arch-x86/cpufeatureset.h |  1 +
>  9 files changed, 79 insertions(+), 27 deletions(-)
> 

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

[Jan Beulich]

On 05.05.2023 19:57, Alejandro Vallejo wrote:
> Includes a refactor to move vendor-specific probes to vendor-specific
> files.

I wonder whether the refactoring parts wouldn't better be split off.

> @@ -363,6 +375,21 @@ 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 msr_addr = MSR_K8_HWCR;

Nit: No MSR index is ever a 64-bit quantity. I'd recommend using MSR_K8_HWCR
directly in the two accesses below anyway, but otherwise the variable wants
to be "const unsigned int".

> +	const uint64_t bit = K8_HWCR_CPUID_USER_DIS;
> +	uint64_t val;
> +
> +	rdmsrl(msr_addr, val);
> +
> +	if (!!(val & bit) == enable)
> +		return;
> +
> +	val ^= bit;
> +	wrmsrl(msr_addr, val);
> +}
>[...]
> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -226,8 +226,17 @@ 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

Nit: Style (/* on its own line).

> +	 * 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 ((cpu_has_hypervisor || boot_cpu_data.x86 !=0xf) &&

Nit: Style (blanks around binary operators). I'd also suggest swapping both
sides of the ||, to have the commonly true case first.

Jan

Re: [PATCH 3/3] x86: Use CpuidUserDis if an AMD HVM guest toggles CPUID faulting

[Jan Beulich]

On 05.05.2023 19:57, Alejandro Vallejo wrote:
> This is in order to aid guests of AMD hardware that we have exposed
> CPUID faulting to. If they try to modify the Intel MSR that enables
> the feature, trigger levelling so AMD's version of it (CpuidUserDis)
> is used instead.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  xen/arch/x86/msr.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Don't you also need to update cpu-policy.c:calculate_host_policy()
for the guest to actually know it can use the functionality? Which
in turn would appear to require some form of adjustment to
lib/x86/policy.c:x86_cpu_policies_are_compatible().

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -431,6 +431,13 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>      {
>          bool old_cpuid_faulting = msrs->misc_features_enables.cpuid_faulting;
>  
> +        /*
> +         * The boot CPU must support Intel's CPUID faulting _or_
> +         * AMD's CpuidUserDis.
> +         */
> +        bool can_fault_cpuid = cpu_has_cpuid_faulting ||
> +                               boot_cpu_has(X86_FEATURE_CPUID_USER_DIS);

I'd like to suggest that in such a comment it not be emphasized that
it's the boot CPU (alone) we check. In fact I'm not convinced any
comment is needed here at all. I'm further inclined to suggest to
omit this (single-use) variable altogether.

Jan

Re: [PATCH 3/3] x86: Use CpuidUserDis if an AMD HVM guest toggles CPUID faulting

[Andrew Cooper]

On 08/05/2023 2:18 pm, Jan Beulich wrote:
> On 05.05.2023 19:57, Alejandro Vallejo wrote:
>> This is in order to aid guests of AMD hardware that we have exposed
>> CPUID faulting to. If they try to modify the Intel MSR that enables
>> the feature, trigger levelling so AMD's version of it (CpuidUserDis)
>> is used instead.
>>
>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>> ---
>>  xen/arch/x86/msr.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
> Don't you also need to update cpu-policy.c:calculate_host_policy()
> for the guest to actually know it can use the functionality? Which
> in turn would appear to require some form of adjustment to
> lib/x86/policy.c:x86_cpu_policies_are_compatible().

I asked Alejandro to do it like this.

Advertising this to guests requires plumbing another MSR into the
infrastructure which isn't quite set up properly let, and is in flux
from my work.

For now, this just lets Xen enforce the policy over PV guests, which is
an improvement in and of itself.

~Andrew

Re: [PATCH 3/3] x86: Use CpuidUserDis if an AMD HVM guest toggles CPUID faulting

[Jan Beulich]

On 09.05.2023 12:05, Andrew Cooper wrote:
> On 08/05/2023 2:18 pm, Jan Beulich wrote:
>> On 05.05.2023 19:57, Alejandro Vallejo wrote:
>>> This is in order to aid guests of AMD hardware that we have exposed
>>> CPUID faulting to. If they try to modify the Intel MSR that enables
>>> the feature, trigger levelling so AMD's version of it (CpuidUserDis)
>>> is used instead.
>>>
>>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>> ---
>>>  xen/arch/x86/msr.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>> Don't you also need to update cpu-policy.c:calculate_host_policy()
>> for the guest to actually know it can use the functionality? Which
>> in turn would appear to require some form of adjustment to
>> lib/x86/policy.c:x86_cpu_policies_are_compatible().
> 
> I asked Alejandro to do it like this.
> 
> Advertising this to guests requires plumbing another MSR into the
> infrastructure which isn't quite set up properly let, and is in flux
> from my work.
> 
> For now, this just lets Xen enforce the policy over PV guests, which is
> an improvement in and of itself.

But as per the title this patch is about HVM guests (aiui the PV aspect
is taken care of already without the patch here). In any event - if the
omissions are intentional (for the time being), then I think that wants
mentioning in the description.

Jan

Re: [PATCH 3/3] x86: Use CpuidUserDis if an AMD HVM guest toggles CPUID faulting

[Alejandro Vallejo]

On Tue, May 09, 2023 at 04:41:49PM +0200, Jan Beulich wrote:
> > I asked Alejandro to do it like this.
> > 
> > Advertising this to guests requires plumbing another MSR into the
> > infrastructure which isn't quite set up properly let, and is in flux
> > from my work.
> > 
> > For now, this just lets Xen enforce the policy over PV guests, which is
> > an improvement in and of itself.
> 
> But as per the title this patch is about HVM guests (aiui the PV aspect
> is taken care of already without the patch here). In any event - if the
> omissions are intentional (for the time being), then I think that wants
> mentioning in the description.
> 
> Jan

HVM guests are always exposed the Intel interface (emulated if not natively
available). The HVM max policy forces it on, and I don't see anything in
the default policy overriding it. My attempt here was to let AMD guests use
the emulated Intel MSR and trigger levellling that would itself rely on
AMD's CpuidUserDis without guest intervention. That said, several cans of
worms exist in mantaining this internal routing. I'll get rid of that last
patch and leave HVM guests alone for the time being. They are functionally 
correct (albeit their CPUID take 2 faults, whereas 1 would suffice).

Alejandro

Re: [PATCH 3/3] x86: Use CpuidUserDis if an AMD HVM guest toggles CPUID faulting

[Jan Beulich]

On 09.05.2023 12:05, Andrew Cooper wrote:
> On 08/05/2023 2:18 pm, Jan Beulich wrote:
>> On 05.05.2023 19:57, Alejandro Vallejo wrote:
>>> This is in order to aid guests of AMD hardware that we have exposed
>>> CPUID faulting to. If they try to modify the Intel MSR that enables
>>> the feature, trigger levelling so AMD's version of it (CpuidUserDis)
>>> is used instead.
>>>
>>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>> ---
>>>  xen/arch/x86/msr.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>> Don't you also need to update cpu-policy.c:calculate_host_policy()
>> for the guest to actually know it can use the functionality? Which
>> in turn would appear to require some form of adjustment to
>> lib/x86/policy.c:x86_cpu_policies_are_compatible().
> 
> I asked Alejandro to do it like this.
> 
> Advertising this to guests requires plumbing another MSR into the
> infrastructure which isn't quite set up properly let, and is in flux
> from my work.

Maybe there was some misunderstanding here, as I realize only now. I
wasn't asking to expose the AMD feature, but instead I was after

    /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
    /* probe_cpuid_faulting() sanity checks presence of MISC_FEATURES_ENABLES */
    p->platform_info.cpuid_faulting = cpu_has_cpuid_faulting;

wanting to be extended by "|| boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)".
That, afaict, has no connection to plumbing yet another MSR.

Jan

Re: [PATCH 3/3] x86: Use CpuidUserDis if an AMD HVM guest toggles CPUID faulting

[Alejandro Vallejo]

On Wed, May 10, 2023 at 10:15:31AM +0200, Jan Beulich wrote:
> On 09.05.2023 12:05, Andrew Cooper wrote:
> > On 08/05/2023 2:18 pm, Jan Beulich wrote:
> >> On 05.05.2023 19:57, Alejandro Vallejo wrote:
> >>> This is in order to aid guests of AMD hardware that we have exposed
> >>> CPUID faulting to. If they try to modify the Intel MSR that enables
> >>> the feature, trigger levelling so AMD's version of it (CpuidUserDis)
> >>> is used instead.
> >>>
> >>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> >>> ---
> >>>  xen/arch/x86/msr.c | 9 ++++++++-
> >>>  1 file changed, 8 insertions(+), 1 deletion(-)
> >> Don't you also need to update cpu-policy.c:calculate_host_policy()
> >> for the guest to actually know it can use the functionality? Which
> >> in turn would appear to require some form of adjustment to
> >> lib/x86/policy.c:x86_cpu_policies_are_compatible().
> > 
> > I asked Alejandro to do it like this.
> > 
> > Advertising this to guests requires plumbing another MSR into the
> > infrastructure which isn't quite set up properly let, and is in flux
> > from my work.
> 
> Maybe there was some misunderstanding here, as I realize only now. I
> wasn't asking to expose the AMD feature, but instead I was after
> 
>     /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
>     /* probe_cpuid_faulting() sanity checks presence of MISC_FEATURES_ENABLES */
>     p->platform_info.cpuid_faulting = cpu_has_cpuid_faulting;
> 
> wanting to be extended by "|| boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)".
> That, afaict, has no connection to plumbing yet another MSR.
> 
> Jan

Let me backtrack a little. There's 2 different (but related) matters under
discussion:

 1. Trapping PV guest attempts to run the cpuid instruction
 2. Providing a virtualized CPUID faulting interface so a guest kernel
    can intercept the CPUID instructions of code running under it.

This series was meant to provide fix (1) on AMD hardware. I did go a bit
beyond in v1, trying to provide support for a unified faulting mechanism
on AMD, but providing a virtualized CPUID faulting interface needs to be
done a bit more carefully than that. Doing it partially now just adds tech
debt to be paid when CpuidUserDis is exposed to the domains.

Changing the policy to expose the Intel interface when AMD is the backend
falls on (2), which is probably better off done separately in a unified way.

v2 removes the changes to x86/msr.c so only (1) is addressed.

Does this make sense?

Alejandro

Re: [PATCH 0/3] Add CpuidUserDis support

[Alejandro Vallejo]

On Mon, May 08, 2023 at 11:06:31AM +0200, Jan Beulich wrote:
> On 05.05.2023 19:57, Alejandro Vallejo wrote:
> > Nowadays AMD supports trapping the CPUID instruction from ring3 to ring0,
> 
> Since it's relevant for PV32: Their doc talks about CPL > 0, i.e. not just
> ring 3. Therefore I wonder whether ...

Very true. It's CPL>0, not just ring3. Noted and changed on v2.

> 
> > (CpuidUserDis)
> 
> ... we shouldn't deviate from the PM and avoid the misleading use of "user"
> in our internal naming.
> 
> Jan
> 

IMO it's going to be confusing enough as is. We'll eventually have
virtualized versions of both Intel and AMD that may or may not be
cross-hooked with each other (e.g: virtualized Intel interface on
an AMD host) due to backward compatibility. That means we'll probably
want:

 1. A name for the Intel mechanism, currently "CPUID faulting"
 2. A name for the AMD mechanism, currently "CpuidUserDis"
 3. A generic name for the cpuid-can-be-trapped behaviour, currently
    overloaded with the Intel name (but could do with a Xen-specific one).
    It doesn't matter a lot now, but it will once the AMD interface is
    virtualized.

Sure, we could give it an alternative name on AMD, but we still need yet
another one to disambiguate the trapping behaviour from the specific
mechanism that does it.

Using the AMD manual name does have the upside that it's easier to check
the manual without having to remember the AMD-specific feature that
corresponds to a Xen-specific name. That said, if you have a good suggestion
I'm happy to change it. So long as in the end result is (1), (2) and (3)
have non-ambiguous names.

Alejandro

Re: [PATCH 3/3] x86: Use CpuidUserDis if an AMD HVM guest toggles CPUID faulting

[Jan Beulich]

On 10.05.2023 12:52, Alejandro Vallejo wrote:
> On Wed, May 10, 2023 at 10:15:31AM +0200, Jan Beulich wrote:
>> On 09.05.2023 12:05, Andrew Cooper wrote:
>>> On 08/05/2023 2:18 pm, Jan Beulich wrote:
>>>> On 05.05.2023 19:57, Alejandro Vallejo wrote:
>>>>> This is in order to aid guests of AMD hardware that we have exposed
>>>>> CPUID faulting to. If they try to modify the Intel MSR that enables
>>>>> the feature, trigger levelling so AMD's version of it (CpuidUserDis)
>>>>> is used instead.
>>>>>
>>>>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>>>> ---
>>>>>  xen/arch/x86/msr.c | 9 ++++++++-
>>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>> Don't you also need to update cpu-policy.c:calculate_host_policy()
>>>> for the guest to actually know it can use the functionality? Which
>>>> in turn would appear to require some form of adjustment to
>>>> lib/x86/policy.c:x86_cpu_policies_are_compatible().
>>>
>>> I asked Alejandro to do it like this.
>>>
>>> Advertising this to guests requires plumbing another MSR into the
>>> infrastructure which isn't quite set up properly let, and is in flux
>>> from my work.
>>
>> Maybe there was some misunderstanding here, as I realize only now. I
>> wasn't asking to expose the AMD feature, but instead I was after
>>
>>     /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
>>     /* probe_cpuid_faulting() sanity checks presence of MISC_FEATURES_ENABLES */
>>     p->platform_info.cpuid_faulting = cpu_has_cpuid_faulting;
>>
>> wanting to be extended by "|| boot_cpu_has(X86_FEATURE_CPUID_USER_DIS)".
>> That, afaict, has no connection to plumbing yet another MSR.
> 
> Let me backtrack a little. There's 2 different (but related) matters under
> discussion:
> 
>  1. Trapping PV guest attempts to run the cpuid instruction
>  2. Providing a virtualized CPUID faulting interface so a guest kernel
>     can intercept the CPUID instructions of code running under it.
> 
> This series was meant to provide fix (1) on AMD hardware. I did go a bit
> beyond in v1, trying to provide support for a unified faulting mechanism
> on AMD, but providing a virtualized CPUID faulting interface needs to be
> done a bit more carefully than that. Doing it partially now just adds tech
> debt to be paid when CpuidUserDis is exposed to the domains.
> 
> Changing the policy to expose the Intel interface when AMD is the backend
> falls on (2), which is probably better off done separately in a unified way.
> 
> v2 removes the changes to x86/msr.c so only (1) is addressed.
> 
> Does this make sense?

Certainly. Nevertheless I would like to understand what Andrew meant,
even if only for staying better in sync with all the work he has been
doing (and is still planning to do) in the area of CPU policies and
leveling.

Jan