[PATCH v3 0/8] Clean up the supported xfeatures

[PATCH v3 0/8] Clean up the supported xfeatures

[Aaron Lewis]

Make sure the supported xfeatures, i.e. EDX:EAX of CPUID.(EAX=0DH,ECX=0),
for MPX, AVX-512, and AMX are in a valid state and follow the rules
outlined in the SDM vol 1, section 13.3 ENABLING THE XSAVE FEATURE SET
AND XSAVE-ENABLED FEATURES.  While those rules apply to the enabled
xfeatures, i.e. XCR0, use them to set the supported xfeatures.  That way
if they are used by userspace or a guest to set the enabled xfeatures,
they won't cause a #GP.  

A test is then added to verify the supported xfeatures are in this
sanitied state.

v2 -> v3:
 - Sanitize the supported XCR0 in XSAVES2 [Sean]
 - Split AVX-512 into 2 commits [Sean]
 - Added XFEATURE_MASK_FP to selftests [Sean]
 - Reworked XCR0 test to split up architectural and kvm rules [Sean]

Aaron Lewis (8):
  KVM: x86: Add kvm_permitted_xcr0()
  KVM: x86: Clear all supported MPX xfeatures if they are not all set
  KVM: x86: Clear all supported AVX-512 xfeatures if they are not all set
  KVM: x86: Clear AVX-512 xfeatures if SSE or AVX is clear
  KVM: x86: Clear all supported AMX xfeatures if they are not all set
  KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible
  KVM: selftests: Add XFEATURE masks to common code
  KVM: selftests: Add XCR0 Test

 arch/x86/kvm/cpuid.c                          |  27 +++-
 arch/x86/kvm/cpuid.h                          |   1 +
 arch/x86/kvm/x86.c                            |   4 +-
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/include/x86_64/processor.h  |  52 +++++++
 tools/testing/selftests/kvm/x86_64/amx_test.c |  46 ++-----
 .../selftests/kvm/x86_64/xcr0_cpuid_test.c    | 128 ++++++++++++++++++
 7 files changed, 220 insertions(+), 39 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c

-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 1/8] KVM: x86: Add kvm_permitted_xcr0()

[Aaron Lewis]

Add the helper, kvm_permitted_xcr0(), to make it easier to filter
the supported XCR0 before using it.

No functional changes intended.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 arch/x86/kvm/cpuid.c | 7 ++++++-
 arch/x86/kvm/cpuid.h | 1 +
 arch/x86/kvm/x86.c   | 4 +---
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 8f8edeaf8177..e1165c196970 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -60,6 +60,11 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
 	return ret;
 }
 
+u64 kvm_permitted_xcr0(void)
+{
+	return kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
+}
+
 /*
  * This one is tied to SSB in the user API, and not
  * visible in /proc/cpuinfo.
@@ -981,7 +986,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 		entry->eax = entry->ebx = entry->ecx = 0;
 		break;
 	case 0xd: {
-		u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
+		u64 permitted_xcr0 = kvm_permitted_xcr0();
 		u64 permitted_xss = kvm_caps.supported_xss;
 
 		entry->eax &= permitted_xcr0;
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index b1658c0de847..224c25e02748 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -33,6 +33,7 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
 	       u32 *ecx, u32 *edx, bool exact_only);
 
 u32 xstate_required_size(u64 xstate_bv, bool compacted);
+u64 kvm_permitted_xcr0(void);
 
 int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu);
 u64 kvm_vcpu_reserved_gpa_bits_raw(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f706621c35b8..596b234fc100 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4519,9 +4519,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 			r = 0;
 		break;
 	case KVM_CAP_XSAVE2: {
-		u64 guest_perm = xstate_get_guest_group_perm();
-
-		r = xstate_required_size(kvm_caps.supported_xcr0 & guest_perm, false);
+		r = xstate_required_size(kvm_permitted_xcr0(), false);
 		if (r < sizeof(struct kvm_xsave))
 			r = sizeof(struct kvm_xsave);
 		break;
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 2/8] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Aaron Lewis]

Be a good citizen and don't allow any of the supported MPX xfeatures[1]
to be set if they can't all be set.  That way userspace or a guest
doesn't fail if it attempts to set them in XCR0.

[1] CPUID.(EAX=0DH,ECX=0):EAX.BNDREGS[bit-3]
    CPUID.(EAX=0DH,ECX=0):EAX.BNDCSR[bit-4]

Suggested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 arch/x86/kvm/cpuid.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index e1165c196970..b2e7407cd114 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -60,9 +60,22 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
 	return ret;
 }
 
+static u64 sanitize_xcr0(u64 xcr0)
+{
+	u64 mask;
+
+	mask = XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR;
+	if ((xcr0 & mask) != mask)
+		xcr0 &= ~mask;
+
+	return xcr0;
+}
+
 u64 kvm_permitted_xcr0(void)
 {
-	return kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
+	u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
+
+	return sanitize_xcr0(permitted_xcr0);
 }
 
 /*
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 3/8] KVM: x86: Clear all supported AVX-512 xfeatures if they are not all set

[Aaron Lewis]

Be a good citizen and don't allow any of the supported AVX-512
xfeatures[1] to be set if they can't all be set.  That way userspace or
a guest doesn't fail if it attempts to set them in XCR0.

[1] CPUID.(EAX=0DH,ECX=0):EAX.OPMASK[bit-5]
    CPUID.(EAX=0DH,ECX=0):EAX.ZMM_Hi256[bit-6]
    CPUID.(EAX=0DH,ECX=0):EAX.ZMM_Hi16_ZMM[bit-7]

Suggested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 arch/x86/kvm/cpuid.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index b2e7407cd114..76379a51a16d 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -68,6 +68,10 @@ static u64 sanitize_xcr0(u64 xcr0)
 	if ((xcr0 & mask) != mask)
 		xcr0 &= ~mask;
 
+	mask = XFEATURE_MASK_AVX512;
+	if ((xcr0 & mask) != mask)
+		xcr0 &= ~XFEATURE_MASK_AVX512;
+
 	return xcr0;
 }
 
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 4/8] KVM: x86: Clear AVX-512 xfeatures if SSE or AVX is clear

[Aaron Lewis]

A requirement for setting AVX-512 is to have both SSE and AVX enabled.
Add these masks to ensure AVX-512 gets cleared if either SSE or AVX
are clear.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 arch/x86/kvm/cpuid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 76379a51a16d..1eff76f836a2 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -68,7 +68,7 @@ static u64 sanitize_xcr0(u64 xcr0)
 	if ((xcr0 & mask) != mask)
 		xcr0 &= ~mask;
 
-	mask = XFEATURE_MASK_AVX512;
+	mask = XFEATURE_MASK_AVX512 | XFEATURE_MASK_SSE | XFEATURE_MASK_YMM;
 	if ((xcr0 & mask) != mask)
 		xcr0 &= ~XFEATURE_MASK_AVX512;
 
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 5/8] KVM: x86: Clear all supported AMX xfeatures if they are not all set

[Aaron Lewis]

Be a good citizen and don't allow any of the supported AMX xfeatures[1]
to be set if they can't all be set.  That way userspace or a guest
doesn't fail if it attempts to set them in XCR0.

[1] CPUID.(EAX=0DH,ECX=0):EAX.XTILE_CFG[bit-17]
    CPUID.(EAX=0DH,ECX=0):EAX.XTILE_DATA[bit-18]

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 arch/x86/kvm/cpuid.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 1eff76f836a2..ac0423508b28 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -72,6 +72,9 @@ static u64 sanitize_xcr0(u64 xcr0)
 	if ((xcr0 & mask) != mask)
 		xcr0 &= ~XFEATURE_MASK_AVX512;
 
+	if ((xcr0 & XFEATURE_MASK_XTILE) != XFEATURE_MASK_XTILE)
+		xcr0 &= ~XFEATURE_MASK_XTILE;
+
 	return xcr0;
 }
 
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 6/8] KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible

[Aaron Lewis]

The instructions XGETBV and XSETBV are useful to other tests.  Move
them to processor.h to make them more broadly available.

No functional change intended.

Reviewed-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 .../selftests/kvm/include/x86_64/processor.h  | 18 ++++++++++++++
 tools/testing/selftests/kvm/x86_64/amx_test.c | 24 +++----------------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 53ffa43c90db..62dc54c8e0c4 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -496,6 +496,24 @@ static inline void set_cr4(uint64_t val)
 	__asm__ __volatile__("mov %0, %%cr4" : : "r" (val) : "memory");
 }
 
+static inline u64 xgetbv(u32 index)
+{
+	u32 eax, edx;
+
+	__asm__ __volatile__("xgetbv;"
+		     : "=a" (eax), "=d" (edx)
+		     : "c" (index));
+	return eax | ((u64)edx << 32);
+}
+
+static inline void xsetbv(u32 index, u64 value)
+{
+	u32 eax = value;
+	u32 edx = value >> 32;
+
+	__asm__ __volatile__("xsetbv" :: "a" (eax), "d" (edx), "c" (index));
+}
+
 static inline struct desc_ptr get_gdt(void)
 {
 	struct desc_ptr gdt;
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index bd72c6eb3b67..4b733ad21831 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -68,24 +68,6 @@ struct xtile_info {
 
 static struct xtile_info xtile;
 
-static inline u64 __xgetbv(u32 index)
-{
-	u32 eax, edx;
-
-	asm volatile("xgetbv;"
-		     : "=a" (eax), "=d" (edx)
-		     : "c" (index));
-	return eax + ((u64)edx << 32);
-}
-
-static inline void __xsetbv(u32 index, u64 value)
-{
-	u32 eax = value;
-	u32 edx = value >> 32;
-
-	asm volatile("xsetbv" :: "a" (eax), "d" (edx), "c" (index));
-}
-
 static inline void __ldtilecfg(void *cfg)
 {
 	asm volatile(".byte 0xc4,0xe2,0x78,0x49,0x00"
@@ -121,7 +103,7 @@ static inline void check_cpuid_xsave(void)
 
 static bool check_xsave_supports_xtile(void)
 {
-	return __xgetbv(0) & XFEATURE_MASK_XTILE;
+	return xgetbv(0) & XFEATURE_MASK_XTILE;
 }
 
 static void check_xtile_info(void)
@@ -177,9 +159,9 @@ static void init_regs(void)
 	cr4 |= X86_CR4_OSXSAVE;
 	set_cr4(cr4);
 
-	xcr0 = __xgetbv(0);
+	xcr0 = xgetbv(0);
 	xcr0 |= XFEATURE_MASK_XTILE;
-	__xsetbv(0x0, xcr0);
+	xsetbv(0x0, xcr0);
 }
 
 static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 7/8] KVM: selftests: Add XFEATURE masks to common code

[Aaron Lewis]

Add XFEATURE masks to processor.h to make them more broadly available
in KVM selftests.

Use the names from the kernel's fpu/types.h for consistency, i.e.
rename XTILECFG and XTILEDATA to XTILE_CFG and XTILE_DATA respectively.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 .../selftests/kvm/include/x86_64/processor.h  | 17 ++++++++++++++
 tools/testing/selftests/kvm/x86_64/amx_test.c | 22 +++++++------------
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 62dc54c8e0c4..ebe83cfe521c 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -48,6 +48,23 @@ extern bool host_cpu_is_amd;
 #define X86_CR4_SMAP		(1ul << 21)
 #define X86_CR4_PKE		(1ul << 22)
 
+#define XFEATURE_MASK_FP		BIT_ULL(0)
+#define XFEATURE_MASK_SSE		BIT_ULL(1)
+#define XFEATURE_MASK_YMM		BIT_ULL(2)
+#define XFEATURE_MASK_BNDREGS		BIT_ULL(3)
+#define XFEATURE_MASK_BNDCSR		BIT_ULL(4)
+#define XFEATURE_MASK_OPMASK		BIT_ULL(5)
+#define XFEATURE_MASK_ZMM_Hi256		BIT_ULL(6)
+#define XFEATURE_MASK_Hi16_ZMM		BIT_ULL(7)
+#define XFEATURE_MASK_XTILE_CFG		BIT_ULL(17)
+#define XFEATURE_MASK_XTILE_DATA	BIT_ULL(18)
+
+#define XFEATURE_MASK_AVX512		(XFEATURE_MASK_OPMASK | \
+					 XFEATURE_MASK_ZMM_Hi256 | \
+					 XFEATURE_MASK_Hi16_ZMM)
+#define XFEATURE_MASK_XTILE		(XFEATURE_MASK_XTILE_DATA | \
+					 XFEATURE_MASK_XTILE_CFG)
+
 /* Note, these are ordered alphabetically to match kvm_cpuid_entry2.  Eww. */
 enum cpuid_output_regs {
 	KVM_CPUID_EAX,
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index 4b733ad21831..14a7656620d5 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -33,12 +33,6 @@
 #define MAX_TILES			16
 #define RESERVED_BYTES			14
 
-#define XFEATURE_XTILECFG		17
-#define XFEATURE_XTILEDATA		18
-#define XFEATURE_MASK_XTILECFG		(1 << XFEATURE_XTILECFG)
-#define XFEATURE_MASK_XTILEDATA		(1 << XFEATURE_XTILEDATA)
-#define XFEATURE_MASK_XTILE		(XFEATURE_MASK_XTILECFG | XFEATURE_MASK_XTILEDATA)
-
 #define XSAVE_HDR_OFFSET		512
 
 struct xsave_data {
@@ -187,14 +181,14 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
 	__tilerelease();
 	GUEST_SYNC(5);
 	/* bit 18 not in the XCOMP_BV after xsavec() */
-	set_xstatebv(xsave_data, XFEATURE_MASK_XTILEDATA);
-	__xsavec(xsave_data, XFEATURE_MASK_XTILEDATA);
-	GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILEDATA) == 0);
+	set_xstatebv(xsave_data, XFEATURE_MASK_XTILE_DATA);
+	__xsavec(xsave_data, XFEATURE_MASK_XTILE_DATA);
+	GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILE_DATA) == 0);
 
 	/* xfd=0x40000, disable amx tiledata */
-	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA);
+	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILE_DATA);
 	GUEST_SYNC(6);
-	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILE_DATA);
 	set_tilecfg(amx_cfg);
 	__ldtilecfg(amx_cfg);
 	/* Trigger #NM exception */
@@ -206,11 +200,11 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
 
 void guest_nm_handler(struct ex_regs *regs)
 {
-	/* Check if #NM is triggered by XFEATURE_MASK_XTILEDATA */
+	/* Check if #NM is triggered by XFEATURE_MASK_XTILE_DATA */
 	GUEST_SYNC(7);
-	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILE_DATA);
 	GUEST_SYNC(8);
-	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILE_DATA);
 	/* Clear xfd_err */
 	wrmsr(MSR_IA32_XFD_ERR, 0);
 	/* xfd=0, enable amx */
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 8/8] KVM: selftests: Add XCR0 Test

[Aaron Lewis]

Check both architectural rules and KVM's own software-defined rules to
ensure the supported xfeatures[1] don't violate any of them.

The architectural rules[2] and KVM's rules ensure for a given
feature, e.g. sse, avx, amx, etc... their associated xfeatures are
either all sets or none of them are set, and any dependencies
are enabled if needed.

[1] EDX:EAX of CPUID.(EAX=0DH,ECX=0)
[2] SDM vol 1, 13.3 ENABLING THE XSAVE FEATURE SET AND XSAVE-ENABLED
    FEATURES

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/include/x86_64/processor.h  |  17 +++
 .../selftests/kvm/x86_64/xcr0_cpuid_test.c    | 128 ++++++++++++++++++
 3 files changed, 146 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 84a627c43795..18cadc669798 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -105,6 +105,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/vmx_tsc_adjust_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_nested_tsc_scaling_test
 TEST_GEN_PROGS_x86_64 += x86_64/xapic_ipi_test
 TEST_GEN_PROGS_x86_64 += x86_64/xapic_state_test
+TEST_GEN_PROGS_x86_64 += x86_64/xcr0_cpuid_test
 TEST_GEN_PROGS_x86_64 += x86_64/xss_msr_test
 TEST_GEN_PROGS_x86_64 += x86_64/debug_regs
 TEST_GEN_PROGS_x86_64 += x86_64/tsc_msrs_test
diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index ebe83cfe521c..380daa82b023 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -667,6 +667,15 @@ static inline bool this_pmu_has(struct kvm_x86_pmu_feature feature)
 	       !this_cpu_has(feature.anti_feature);
 }
 
+static __always_inline uint64_t this_cpu_supported_xcr0(void)
+{
+	uint32_t a, b, c, d;
+
+	cpuid(0xd, &a, &b, &c, &d);
+
+	return a | ((uint64_t)d << 32);
+}
+
 typedef u32		__attribute__((vector_size(16))) sse128_t;
 #define __sse128_u	union { sse128_t vec; u64 as_u64[2]; u32 as_u32[4]; }
 #define sse128_lo(x)	({ __sse128_u t; t.vec = x; t.as_u64[0]; })
@@ -1090,6 +1099,14 @@ static inline uint8_t wrmsr_safe(uint32_t msr, uint64_t val)
 	return kvm_asm_safe("wrmsr", "a"(val & -1u), "d"(val >> 32), "c"(msr));
 }
 
+static inline uint8_t xsetbv_safe(uint32_t index, uint64_t value)
+{
+	u32 eax = value;
+	u32 edx = value >> 32;
+
+	return kvm_asm_safe("xsetbv", "a" (eax), "d" (edx), "c" (index));
+}
+
 bool kvm_is_tdp_enabled(void);
 
 uint64_t *__vm_get_page_table_entry(struct kvm_vm *vm, uint64_t vaddr,
diff --git a/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c b/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c
new file mode 100644
index 000000000000..7ca0dea3d144
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * XCR0 cpuid test
+ *
+ * Copyright (C) 2022, Google LLC.
+ */
+
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+
+#include "test_util.h"
+
+#include "kvm_util.h"
+#include "processor.h"
+
+/* Architectural check. */
+#define ASSERT_XFEATURE_DEPENDENCIES(supported_xcr0, xfeatures, dependencies)	  \
+do {										  \
+	uint64_t __supported = (supported_xcr0) & ((xfeatures) | (dependencies)); \
+										  \
+	GUEST_ASSERT_3((__supported & (xfeatures)) != (xfeatures) ||		  \
+		       __supported == ((xfeatures) | (dependencies)),		  \
+		       __supported, (xfeatures), (dependencies));		  \
+} while (0)
+
+/*
+ * KVM's own software-defined rules.  While not architectural it really
+ * ought to be true.
+ */
+#define ASSERT_ALL_OR_NONE_XFEATURE(supported_xcr0, xfeatures)		\
+do {									\
+	uint64_t __supported = (supported_xcr0) & (xfeatures);		\
+									\
+	GUEST_ASSERT_2(!__supported || __supported == (xfeatures),	\
+		       __supported, (xfeatures));			\
+} while (0)
+
+static void guest_code(void)
+{
+	uint64_t xcr0_reset;
+	uint64_t supported_xcr0;
+	int i, vector;
+
+	set_cr4(get_cr4() | X86_CR4_OSXSAVE);
+
+	xcr0_reset = xgetbv(0);
+	supported_xcr0 = this_cpu_supported_xcr0();
+
+	GUEST_ASSERT(xcr0_reset == XFEATURE_MASK_FP);
+
+	/* Check AVX */
+	ASSERT_XFEATURE_DEPENDENCIES(supported_xcr0,
+				     XFEATURE_MASK_YMM,
+				     XFEATURE_MASK_SSE);
+
+	/* Check MPX */
+	ASSERT_ALL_OR_NONE_XFEATURE(supported_xcr0,
+				    XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR);
+
+	/* Check AVX-512 */
+	ASSERT_XFEATURE_DEPENDENCIES(supported_xcr0,
+				     XFEATURE_MASK_AVX512,
+				     XFEATURE_MASK_SSE | XFEATURE_MASK_YMM);
+	ASSERT_ALL_OR_NONE_XFEATURE(supported_xcr0,
+				    XFEATURE_MASK_AVX512);
+
+	/* Check AMX */
+	ASSERT_ALL_OR_NONE_XFEATURE(supported_xcr0,
+				    XFEATURE_MASK_XTILE);
+
+	vector = xsetbv_safe(0, supported_xcr0);
+	GUEST_ASSERT_2(!vector, supported_xcr0, vector);
+
+	for (i = 0; i < 64; i++) {
+		if (supported_xcr0 & BIT_ULL(i))
+			continue;
+
+		vector = xsetbv_safe(0, supported_xcr0 | BIT_ULL(i));
+		GUEST_ASSERT_3(vector == GP_VECTOR, supported_xcr0, vector, BIT_ULL(i));
+	}
+
+	GUEST_DONE();
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_run *run;
+	struct kvm_vm *vm;
+	struct ucall uc;
+
+	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XSAVE));
+
+	/* Tell stdout not to buffer its content */
+	setbuf(stdout, NULL);
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+	run = vcpu->run;
+
+	vm_init_descriptor_tables(vm);
+	vcpu_init_descriptor_tables(vcpu);
+
+	while (1) {
+		vcpu_run(vcpu);
+
+		TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
+			    "Unexpected exit reason: %u (%s),\n",
+			    run->exit_reason,
+			    exit_reason_str(run->exit_reason));
+
+		switch (get_ucall(vcpu, &uc)) {
+		case UCALL_ABORT:
+			REPORT_GUEST_ASSERT_3(uc, "0x%lx 0x%lx 0x%lx");
+			break;
+		case UCALL_DONE:
+			goto done;
+		default:
+			TEST_FAIL("Unknown ucall %lu", uc.cmd);
+		}
+	}
+
+done:
+	kvm_vm_free(vm);
+	return 0;
+}
-- 
2.39.2.637.g21b0678d19-goog

Re: [PATCH v3 8/8] KVM: selftests: Add XCR0 Test

[Mingwei Zhang]

On Fri, Feb 24, 2023, Aaron Lewis wrote:
> Check both architectural rules and KVM's own software-defined rules to
> ensure the supported xfeatures[1] don't violate any of them.
> 
> The architectural rules[2] and KVM's rules ensure for a given
> feature, e.g. sse, avx, amx, etc... their associated xfeatures are
> either all sets or none of them are set, and any dependencies
> are enabled if needed.
> 
> [1] EDX:EAX of CPUID.(EAX=0DH,ECX=0)
> [2] SDM vol 1, 13.3 ENABLING THE XSAVE FEATURE SET AND XSAVE-ENABLED
>     FEATURES
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>

Sorry, I did not get the point of this test? I run your test in an old
(unpatched) kernel on two machines: 1) one with AMX and 2) one without
it. (SPR and Skylake). Neither of them fails. Do you want to clarify a
little bit?


Thanks.
-Mingwei

Re: [PATCH v3 8/8] KVM: selftests: Add XCR0 Test

[Aaron Lewis]

On Thu, Mar 2, 2023 at 8:34 PM Mingwei Zhang <mizhang@google.com> wrote:
>
> On Fri, Feb 24, 2023, Aaron Lewis wrote:
> > Check both architectural rules and KVM's own software-defined rules to
> > ensure the supported xfeatures[1] don't violate any of them.
> >
> > The architectural rules[2] and KVM's rules ensure for a given
> > feature, e.g. sse, avx, amx, etc... their associated xfeatures are
> > either all sets or none of them are set, and any dependencies
> > are enabled if needed.
> >
> > [1] EDX:EAX of CPUID.(EAX=0DH,ECX=0)
> > [2] SDM vol 1, 13.3 ENABLING THE XSAVE FEATURE SET AND XSAVE-ENABLED
> >     FEATURES
> >
> > Signed-off-by: Aaron Lewis <aaronlewis@google.com>
>
> Sorry, I did not get the point of this test? I run your test in an old
> (unpatched) kernel on two machines: 1) one with AMX and 2) one without
> it. (SPR and Skylake). Neither of them fails. Do you want to clarify a
> little bit?

The only known issue exists on newer versions of the kernel when run
on SPR.  It occurs after the syscall, prctl (to enable XTILEDATA), was
introduced.  If you run this test without the fix[1] you will get the
assert below, indicating the XTILECFG is supported by the guest, but
XTILEDATA isn't.

==== Test Assertion Failure ====
  x86_64/xcr0_cpuid_test.c:116: false
  pid=18124 tid=18124 errno=4 - Interrupted system call
     1 0x0000000000401894: main at xcr0_cpuid_test.c:116
     2 0x0000000000414263: __libc_start_call_main at libc-start.o:?
     3 0x00000000004158af: __libc_start_main_impl at ??:?
     4 0x0000000000401660: _start at ??:?
  Failed guest assert: !__supported || __supported == ((((((1ULL))) <<
(18)) | ((((1ULL))) << (17)))) at x86_64/xcr0_cpuid_test.c:72
0x20000 0x60000 0x0

[1] KVM: x86: Clear all supported AMX xfeatures if they are not all set
https://lore.kernel.org/kvm/20230224223607.1580880-6-aaronlewis@google.com/

>
>
> Thanks.
> -Mingwei

Re: [PATCH v3 8/8] KVM: selftests: Add XCR0 Test

[Mingwei Zhang]

On Thu, Mar 02, 2023, Aaron Lewis wrote:
> On Thu, Mar 2, 2023 at 8:34 PM Mingwei Zhang <mizhang@google.com> wrote:
> > > > On Fri, Feb 24, 2023, Aaron Lewis wrote:
> > > Check both architectural rules and KVM's own software-defined rules to
> > > ensure the supported xfeatures[1] don't violate any of them.
> > >
> > > The architectural rules[2] and KVM's rules ensure for a given
> > > feature, e.g. sse, avx, amx, etc... their associated xfeatures are
> > > either all sets or none of them are set, and any dependencies
> > > are enabled if needed.
> > >
> > > [1] EDX:EAX of CPUID.(EAX=0DH,ECX=0)
> > > [2] SDM vol 1, 13.3 ENABLING THE XSAVE FEATURE SET AND XSAVE-ENABLED
> > >     FEATURES
> > >
> > > Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> >
> > Sorry, I did not get the point of this test? I run your test in an old
> > (unpatched) kernel on two machines: 1) one with AMX and 2) one without
> > it. (SPR and Skylake). Neither of them fails. Do you want to clarify a
> > little bit?
> 
> The only known issue exists on newer versions of the kernel when run
> on SPR.  It occurs after the syscall, prctl (to enable XTILEDATA), was
> introduced.  If you run this test without the fix[1] you will get the
> assert below, indicating the XTILECFG is supported by the guest, but
> XTILEDATA isn't.
> 
> ==== Test Assertion Failure ====
>   x86_64/xcr0_cpuid_test.c:116: false
>   pid=18124 tid=18124 errno=4 - Interrupted system call
>      1 0x0000000000401894: main at xcr0_cpuid_test.c:116
>      2 0x0000000000414263: __libc_start_call_main at libc-start.o:?
>      3 0x00000000004158af: __libc_start_main_impl at ??:?
>      4 0x0000000000401660: _start at ??:?
>   Failed guest assert: !__supported || __supported == ((((((1ULL))) <<
> (18)) | ((((1ULL))) << (17)))) at x86_64/xcr0_cpuid_test.c:72
> 0x20000 0x60000 0x0
> 
> [1] KVM: x86: Clear all supported AMX xfeatures if they are not all set
> https://lore.kernel.org/kvm/20230224223607.1580880-6-aaronlewis@google.com/
> 

Understood. Thanks for reminding me. Yes, without the invocation of
vm_xsave_require_permission(XSTATE_XTILE_DATA_BIT); VM guest will see a
partially enabled feature (XTILECFG without XTILEDATA).

Reviewed-by: Mingwei Zhang <mizhang@google.com>
> >
> >
> > Thanks.
> > -Mingwei

Re: [PATCH v3 5/8] KVM: x86: Clear all supported AMX xfeatures if they are not all set

[Mingwei Zhang]

On Fri, Feb 24, 2023, Aaron Lewis wrote:
> Be a good citizen and don't allow any of the supported AMX xfeatures[1]
> to be set if they can't all be set.  That way userspace or a guest
> doesn't fail if it attempts to set them in XCR0.
> 
> [1] CPUID.(EAX=0DH,ECX=0):EAX.XTILE_CFG[bit-17]
>     CPUID.(EAX=0DH,ECX=0):EAX.XTILE_DATA[bit-18]
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>

Reviewed-by: Mingwei Zhang <mizhang@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 1eff76f836a2..ac0423508b28 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -72,6 +72,9 @@ static u64 sanitize_xcr0(u64 xcr0)
>  	if ((xcr0 & mask) != mask)
>  		xcr0 &= ~XFEATURE_MASK_AVX512;
>  
> +	if ((xcr0 & XFEATURE_MASK_XTILE) != XFEATURE_MASK_XTILE)
> +		xcr0 &= ~XFEATURE_MASK_XTILE;
> +
>  	return xcr0;
>  }
>  
> -- 
> 2.39.2.637.g21b0678d19-goog
> 

Re: [PATCH v3 1/8] KVM: x86: Add kvm_permitted_xcr0()

[Mingwei Zhang]

On Fri, Feb 24, 2023, Aaron Lewis wrote:
> Add the helper, kvm_permitted_xcr0(), to make it easier to filter
> the supported XCR0 before using it.
> 
> No functional changes intended.
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
Reviewed-by: Mingwei Zhang <mizhang@google.com>

Re: [PATCH v3 7/8] KVM: selftests: Add XFEATURE masks to common code

[Mingwei Zhang]

On Fri, Feb 24, 2023, Aaron Lewis wrote:
> Add XFEATURE masks to processor.h to make them more broadly available
> in KVM selftests.
> 
> Use the names from the kernel's fpu/types.h for consistency, i.e.
> rename XTILECFG and XTILEDATA to XTILE_CFG and XTILE_DATA respectively.
> 
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  .../selftests/kvm/include/x86_64/processor.h  | 17 ++++++++++++++
>  tools/testing/selftests/kvm/x86_64/amx_test.c | 22 +++++++------------
>  2 files changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index 62dc54c8e0c4..ebe83cfe521c 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -48,6 +48,23 @@ extern bool host_cpu_is_amd;
>  #define X86_CR4_SMAP		(1ul << 21)
>  #define X86_CR4_PKE		(1ul << 22)
>  
> +#define XFEATURE_MASK_FP		BIT_ULL(0)
> +#define XFEATURE_MASK_SSE		BIT_ULL(1)
> +#define XFEATURE_MASK_YMM		BIT_ULL(2)
> +#define XFEATURE_MASK_BNDREGS		BIT_ULL(3)
> +#define XFEATURE_MASK_BNDCSR		BIT_ULL(4)
> +#define XFEATURE_MASK_OPMASK		BIT_ULL(5)
> +#define XFEATURE_MASK_ZMM_Hi256		BIT_ULL(6)
> +#define XFEATURE_MASK_Hi16_ZMM		BIT_ULL(7)
> +#define XFEATURE_MASK_XTILE_CFG		BIT_ULL(17)
> +#define XFEATURE_MASK_XTILE_DATA	BIT_ULL(18)
> +
> +#define XFEATURE_MASK_AVX512		(XFEATURE_MASK_OPMASK | \
> +					 XFEATURE_MASK_ZMM_Hi256 | \
> +					 XFEATURE_MASK_Hi16_ZMM)
> +#define XFEATURE_MASK_XTILE		(XFEATURE_MASK_XTILE_DATA | \
> +					 XFEATURE_MASK_XTILE_CFG)
> +
>  /* Note, these are ordered alphabetically to match kvm_cpuid_entry2.  Eww. */
>  enum cpuid_output_regs {
>  	KVM_CPUID_EAX,
> diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
> index 4b733ad21831..14a7656620d5 100644
> --- a/tools/testing/selftests/kvm/x86_64/amx_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
> @@ -33,12 +33,6 @@
>  #define MAX_TILES			16
>  #define RESERVED_BYTES			14
>  
> -#define XFEATURE_XTILECFG		17
> -#define XFEATURE_XTILEDATA		18
> -#define XFEATURE_MASK_XTILECFG		(1 << XFEATURE_XTILECFG)
> -#define XFEATURE_MASK_XTILEDATA		(1 << XFEATURE_XTILEDATA)
> -#define XFEATURE_MASK_XTILE		(XFEATURE_MASK_XTILECFG | XFEATURE_MASK_XTILEDATA)
> -
>  #define XSAVE_HDR_OFFSET		512
>  
>  struct xsave_data {
> @@ -187,14 +181,14 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
>  	__tilerelease();
>  	GUEST_SYNC(5);
>  	/* bit 18 not in the XCOMP_BV after xsavec() */
> -	set_xstatebv(xsave_data, XFEATURE_MASK_XTILEDATA);
> -	__xsavec(xsave_data, XFEATURE_MASK_XTILEDATA);
> -	GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILEDATA) == 0);
> +	set_xstatebv(xsave_data, XFEATURE_MASK_XTILE_DATA);
> +	__xsavec(xsave_data, XFEATURE_MASK_XTILE_DATA);
> +	GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILE_DATA) == 0);
>  
>  	/* xfd=0x40000, disable amx tiledata */
> -	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA);
> +	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILE_DATA);
>  	GUEST_SYNC(6);
> -	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILEDATA);
> +	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILE_DATA);
>  	set_tilecfg(amx_cfg);
>  	__ldtilecfg(amx_cfg);
>  	/* Trigger #NM exception */
> @@ -206,11 +200,11 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
>  
>  void guest_nm_handler(struct ex_regs *regs)
>  {
> -	/* Check if #NM is triggered by XFEATURE_MASK_XTILEDATA */
> +	/* Check if #NM is triggered by XFEATURE_MASK_XTILE_DATA */
>  	GUEST_SYNC(7);
> -	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
> +	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILE_DATA);
>  	GUEST_SYNC(8);
> -	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
> +	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILE_DATA);
>  	/* Clear xfd_err */
>  	wrmsr(MSR_IA32_XFD_ERR, 0);
>  	/* xfd=0, enable amx */
> -- 
> 2.39.2.637.g21b0678d19-goog
> 
Can I take your commit into my series? This seems to be closely related
with amx_test itself without much relationship with the xcr0 test.
Thoughts?

Re: [PATCH v3 2/8] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Mingwei Zhang]

On Fri, Feb 24, 2023, Aaron Lewis wrote:
> Be a good citizen and don't allow any of the supported MPX xfeatures[1]
> to be set if they can't all be set.  That way userspace or a guest
> doesn't fail if it attempts to set them in XCR0.
> 
> [1] CPUID.(EAX=0DH,ECX=0):EAX.BNDREGS[bit-3]
>     CPUID.(EAX=0DH,ECX=0):EAX.BNDCSR[bit-4]
> 
> Suggested-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index e1165c196970..b2e7407cd114 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -60,9 +60,22 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
>  	return ret;
>  }
>  
> +static u64 sanitize_xcr0(u64 xcr0)
> +{
> +	u64 mask;
> +
> +	mask = XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR;
> +	if ((xcr0 & mask) != mask)
> +		xcr0 &= ~mask;
> +
> +	return xcr0;
> +}

Is it better to put sanitize_xcr0() into the previous patch? If we do
that, this one will be just adding purely the MPX related logic and thus
cleaner I think.
> +
>  u64 kvm_permitted_xcr0(void)
>  {
> -	return kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
> +	u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
> +
> +	return sanitize_xcr0(permitted_xcr0);
>  }
>  
>  /*
> -- 
> 2.39.2.637.g21b0678d19-goog
> 

Re: [PATCH v3 6/8] KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible

[Mingwei Zhang]

On Fri, Feb 24, 2023, Aaron Lewis wrote:
> The instructions XGETBV and XSETBV are useful to other tests.  Move
> them to processor.h to make them more broadly available.
> 
> No functional change intended.
> 
> Reviewed-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
Reviewed-by: Mingwei Zhang <mizhang@google.com>

Re: [PATCH v3 2/8] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Aaron Lewis]

On Fri, Mar 3, 2023 at 9:23 PM Mingwei Zhang <mizhang@google.com> wrote:
>
> On Fri, Feb 24, 2023, Aaron Lewis wrote:
> > Be a good citizen and don't allow any of the supported MPX xfeatures[1]
> > to be set if they can't all be set.  That way userspace or a guest
> > doesn't fail if it attempts to set them in XCR0.
> >
> > [1] CPUID.(EAX=0DH,ECX=0):EAX.BNDREGS[bit-3]
> >     CPUID.(EAX=0DH,ECX=0):EAX.BNDCSR[bit-4]
> >
> > Suggested-by: Jim Mattson <jmattson@google.com>
> > Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index e1165c196970..b2e7407cd114 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -60,9 +60,22 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
> >       return ret;
> >  }
> >
> > +static u64 sanitize_xcr0(u64 xcr0)
> > +{
> > +     u64 mask;
> > +
> > +     mask = XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR;
> > +     if ((xcr0 & mask) != mask)
> > +             xcr0 &= ~mask;
> > +
> > +     return xcr0;
> > +}
>
> Is it better to put sanitize_xcr0() into the previous patch? If we do
> that, this one will be just adding purely the MPX related logic and thus
> cleaner I think.

I don't mind doing that.  I considered putting in its own commit
actually.  The only reason I didn't is I wasn't sure it was
appropriate to have a commit that only added an empty function.  If
that's okay I think I'd lean more towards doing it that way.

> > +
> >  u64 kvm_permitted_xcr0(void)
> >  {
> > -     return kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
> > +     u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
> > +
> > +     return sanitize_xcr0(permitted_xcr0);
> >  }
> >
> >  /*
> > --
> > 2.39.2.637.g21b0678d19-goog
> >

Re: [PATCH v3 7/8] KVM: selftests: Add XFEATURE masks to common code

[Aaron Lewis]

On Fri, Mar 3, 2023 at 9:16 PM Mingwei Zhang <mizhang@google.com> wrote:
>
> On Fri, Feb 24, 2023, Aaron Lewis wrote:
> > Add XFEATURE masks to processor.h to make them more broadly available
> > in KVM selftests.
> >
> > Use the names from the kernel's fpu/types.h for consistency, i.e.
> > rename XTILECFG and XTILEDATA to XTILE_CFG and XTILE_DATA respectively.
> >
> > Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> > ---
> >  .../selftests/kvm/include/x86_64/processor.h  | 17 ++++++++++++++
> >  tools/testing/selftests/kvm/x86_64/amx_test.c | 22 +++++++------------
> >  2 files changed, 25 insertions(+), 14 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> > index 62dc54c8e0c4..ebe83cfe521c 100644
> > --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> > +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> > @@ -48,6 +48,23 @@ extern bool host_cpu_is_amd;
> >  #define X86_CR4_SMAP         (1ul << 21)
> >  #define X86_CR4_PKE          (1ul << 22)
> >
> > +#define XFEATURE_MASK_FP             BIT_ULL(0)
> > +#define XFEATURE_MASK_SSE            BIT_ULL(1)
> > +#define XFEATURE_MASK_YMM            BIT_ULL(2)
> > +#define XFEATURE_MASK_BNDREGS                BIT_ULL(3)
> > +#define XFEATURE_MASK_BNDCSR         BIT_ULL(4)
> > +#define XFEATURE_MASK_OPMASK         BIT_ULL(5)
> > +#define XFEATURE_MASK_ZMM_Hi256              BIT_ULL(6)
> > +#define XFEATURE_MASK_Hi16_ZMM               BIT_ULL(7)
> > +#define XFEATURE_MASK_XTILE_CFG              BIT_ULL(17)
> > +#define XFEATURE_MASK_XTILE_DATA     BIT_ULL(18)
> > +
> > +#define XFEATURE_MASK_AVX512         (XFEATURE_MASK_OPMASK | \
> > +                                      XFEATURE_MASK_ZMM_Hi256 | \
> > +                                      XFEATURE_MASK_Hi16_ZMM)
> > +#define XFEATURE_MASK_XTILE          (XFEATURE_MASK_XTILE_DATA | \
> > +                                      XFEATURE_MASK_XTILE_CFG)
> > +
> >  /* Note, these are ordered alphabetically to match kvm_cpuid_entry2.  Eww. */
> >  enum cpuid_output_regs {
> >       KVM_CPUID_EAX,
> > diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
> > index 4b733ad21831..14a7656620d5 100644
> > --- a/tools/testing/selftests/kvm/x86_64/amx_test.c
> > +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
> > @@ -33,12 +33,6 @@
> >  #define MAX_TILES                    16
> >  #define RESERVED_BYTES                       14
> >
> > -#define XFEATURE_XTILECFG            17
> > -#define XFEATURE_XTILEDATA           18
> > -#define XFEATURE_MASK_XTILECFG               (1 << XFEATURE_XTILECFG)
> > -#define XFEATURE_MASK_XTILEDATA              (1 << XFEATURE_XTILEDATA)
> > -#define XFEATURE_MASK_XTILE          (XFEATURE_MASK_XTILECFG | XFEATURE_MASK_XTILEDATA)
> > -
> >  #define XSAVE_HDR_OFFSET             512
> >
> >  struct xsave_data {
> > @@ -187,14 +181,14 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
> >       __tilerelease();
> >       GUEST_SYNC(5);
> >       /* bit 18 not in the XCOMP_BV after xsavec() */
> > -     set_xstatebv(xsave_data, XFEATURE_MASK_XTILEDATA);
> > -     __xsavec(xsave_data, XFEATURE_MASK_XTILEDATA);
> > -     GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILEDATA) == 0);
> > +     set_xstatebv(xsave_data, XFEATURE_MASK_XTILE_DATA);
> > +     __xsavec(xsave_data, XFEATURE_MASK_XTILE_DATA);
> > +     GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILE_DATA) == 0);
> >
> >       /* xfd=0x40000, disable amx tiledata */
> > -     wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA);
> > +     wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILE_DATA);
> >       GUEST_SYNC(6);
> > -     GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILEDATA);
> > +     GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILE_DATA);
> >       set_tilecfg(amx_cfg);
> >       __ldtilecfg(amx_cfg);
> >       /* Trigger #NM exception */
> > @@ -206,11 +200,11 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
> >
> >  void guest_nm_handler(struct ex_regs *regs)
> >  {
> > -     /* Check if #NM is triggered by XFEATURE_MASK_XTILEDATA */
> > +     /* Check if #NM is triggered by XFEATURE_MASK_XTILE_DATA */
> >       GUEST_SYNC(7);
> > -     GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
> > +     GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILE_DATA);
> >       GUEST_SYNC(8);
> > -     GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
> > +     GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILE_DATA);
> >       /* Clear xfd_err */
> >       wrmsr(MSR_IA32_XFD_ERR, 0);
> >       /* xfd=0, enable amx */
> > --
> > 2.39.2.637.g21b0678d19-goog
> >
> Can I take your commit into my series? This seems to be closely related
> with amx_test itself without much relationship with the xcr0 test.
> Thoughts?

Yes, please do.  I still need to have it here to have access to the
common xfeatures.

Re: [PATCH v3 2/8] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Mingwei Zhang]

On Fri, Mar 3, 2023 at 2:23 PM Aaron Lewis <aaronlewis@google.com> wrote:
>
> On Fri, Mar 3, 2023 at 9:23 PM Mingwei Zhang <mizhang@google.com> wrote:
> >
> > On Fri, Feb 24, 2023, Aaron Lewis wrote:
> > > Be a good citizen and don't allow any of the supported MPX xfeatures[1]
> > > to be set if they can't all be set.  That way userspace or a guest
> > > doesn't fail if it attempts to set them in XCR0.
> > >
> > > [1] CPUID.(EAX=0DH,ECX=0):EAX.BNDREGS[bit-3]
> > >     CPUID.(EAX=0DH,ECX=0):EAX.BNDCSR[bit-4]
> > >
> > > Suggested-by: Jim Mattson <jmattson@google.com>
> > > Signed-off-by: Aaron Lewis <aaronlewis@google.com>

Reviewed-by: Mingwei Zhang <mizhang@google.com>

> > > ---
> > >  arch/x86/kvm/cpuid.c | 15 ++++++++++++++-
> > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > index e1165c196970..b2e7407cd114 100644
> > > --- a/arch/x86/kvm/cpuid.c
> > > +++ b/arch/x86/kvm/cpuid.c
> > > @@ -60,9 +60,22 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
> > >       return ret;
> > >  }
> > >
> > > +static u64 sanitize_xcr0(u64 xcr0)
> > > +{
> > > +     u64 mask;
> > > +
> > > +     mask = XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR;
> > > +     if ((xcr0 & mask) != mask)
> > > +             xcr0 &= ~mask;
> > > +
> > > +     return xcr0;
> > > +}
> >
> > Is it better to put sanitize_xcr0() into the previous patch? If we do
> > that, this one will be just adding purely the MPX related logic and thus
> > cleaner I think.
>
> I don't mind doing that.  I considered putting in its own commit
> actually.  The only reason I didn't is I wasn't sure it was
> appropriate to have a commit that only added an empty function.  If
> that's okay I think I'd lean more towards doing it that way.
>

Yeah, either way works for me.

Re: [PATCH v3 7/8] KVM: selftests: Add XFEATURE masks to common code

[Mingwei Zhang]

> > Can I take your commit into my series? This seems to be closely related
> > with amx_test itself without much relationship with the xcr0 test.
> > Thoughts?
>
> Yes, please do.  I still need to have it here to have access to the
> common xfeatures.

hmm, you are right. Then, this series should go before the selftest series.

Thanks.
-Mingwei

Re: [PATCH v3 0/8] Clean up the supported xfeatures

[Mingwei Zhang]

On Fri, Feb 24, 2023, Aaron Lewis wrote:
> Make sure the supported xfeatures, i.e. EDX:EAX of CPUID.(EAX=0DH,ECX=0),
> for MPX, AVX-512, and AMX are in a valid state and follow the rules
> outlined in the SDM vol 1, section 13.3 ENABLING THE XSAVE FEATURE SET
> AND XSAVE-ENABLED FEATURES.  While those rules apply to the enabled
> xfeatures, i.e. XCR0, use them to set the supported xfeatures.  That way
> if they are used by userspace or a guest to set the enabled xfeatures,
> they won't cause a #GP.  
> 
> A test is then added to verify the supported xfeatures are in this
> sanitied state.
> 
> v2 -> v3:
>  - Sanitize the supported XCR0 in XSAVES2 [Sean]
>  - Split AVX-512 into 2 commits [Sean]
>  - Added XFEATURE_MASK_FP to selftests [Sean]
>  - Reworked XCR0 test to split up architectural and kvm rules [Sean]
> 
> Aaron Lewis (8):
>   KVM: x86: Add kvm_permitted_xcr0()
>   KVM: x86: Clear all supported MPX xfeatures if they are not all set
>   KVM: x86: Clear all supported AVX-512 xfeatures if they are not all set
>   KVM: x86: Clear AVX-512 xfeatures if SSE or AVX is clear
>   KVM: x86: Clear all supported AMX xfeatures if they are not all set
>   KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible
>   KVM: selftests: Add XFEATURE masks to common code
>   KVM: selftests: Add XCR0 Test
> 
>  arch/x86/kvm/cpuid.c                          |  27 +++-
>  arch/x86/kvm/cpuid.h                          |   1 +
>  arch/x86/kvm/x86.c                            |   4 +-
>  tools/testing/selftests/kvm/Makefile          |   1 +
>  .../selftests/kvm/include/x86_64/processor.h  |  52 +++++++
>  tools/testing/selftests/kvm/x86_64/amx_test.c |  46 ++-----
>  .../selftests/kvm/x86_64/xcr0_cpuid_test.c    | 128 ++++++++++++++++++
>  7 files changed, 220 insertions(+), 39 deletions(-)
>  create mode 100644 tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c
> 
ping? This series has been soaked for a while. Sean, would you mind
taking another look?

Re: [PATCH v3 2/8] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Sean Christopherson]

[-- Attachment #1: Type: text/plain, Size: 5400 bytes --]

On Fri, Feb 24, 2023, Aaron Lewis wrote:
> Be a good citizen and don't allow any of the supported MPX xfeatures[1]
> to be set if they can't all be set.  That way userspace or a guest
> doesn't fail if it attempts to set them in XCR0.
> 
> [1] CPUID.(EAX=0DH,ECX=0):EAX.BNDREGS[bit-3]
>     CPUID.(EAX=0DH,ECX=0):EAX.BNDCSR[bit-4]
> 
> Suggested-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index e1165c196970..b2e7407cd114 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -60,9 +60,22 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
>  	return ret;
>  }
>  
> +static u64 sanitize_xcr0(u64 xcr0)
> +{
> +	u64 mask;
> +
> +	mask = XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR;
> +	if ((xcr0 & mask) != mask)
> +		xcr0 &= ~mask;
> +
> +	return xcr0;
> +}
> +
>  u64 kvm_permitted_xcr0(void)
>  {
> -	return kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
> +	u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
> +
> +	return sanitize_xcr0(permitted_xcr0);

Sanitizing XCR0 in this "permitted" helper conflates two separate things (sanity
vs. permissions) and overall leads to a really, really confusing mess.  E.g.
kvm_vcpu_ioctl_x86_set_xsave(), cpuid_get_supported_xcr0(), kvm_x86_dev_get_attr(),
and kvm_mpx_supported() all use the non-sanitized value, which appears nonsensical,
but is actually correct because the whole "permitted" thing is funky.

On a related topic, isn't cpuid_get_supported_xcr0() buggy?  Ah, no, the wonderfully
named kvm_check_cpuid() ensures fpu_enable_guest_xfd_features() is invoked before
the result can actually be consumed.

Ugh, and past me created a royal mess with SGX subleaf 0x12.1.  KVM shouldn't
be manipulating guest CPUID just to ensure userspace doesn't bypass the guest's
permitted XCR0 by launching an enclave.  I.e. the SGX code that consumes
cpuid_get_supported_xcr0() in __kvm_update_cpuid_runtime() should not exists.
I'm 99.9% certain we can simply nuke that code without breaking userspace, e.g.
QEMU correctly sets the data.

LOL, or at least it used to.  QEMU commit 301e90675c ("target/i386: Enable support
for XSAVES based features") completely broke SGX by using allowed XSS instead of
XCR0.

Anyways, back to this mess.  I was going to say "As I suggested in v2[0], sanitize
kvm_caps.supported_xcr0 itself", but after a _lot_ of starting, I realized (or
finally remembered) that that doesn't work because supported_xcr0 is already sane,
the issue is specifically with xstate_get_guest_group_perm() clearing XTILE_DATA
and leaving behind the XTILE_CFG landmine.

And for all intents and purposes, supported_xcr0 _must_ be sane since it comes
from host_xcr0, which is pulled straight from hardware.  So barring a _completely_
busted CPU or hypervisor _and_ a busted kernel, supported_xcr0 itself can never
be insane.  So not only is my proposal to sanitize supported_xcr0 not viable, the
entire idea of generically sanitizing XCR0 is bunk.  The _only_ issue is XTILE_CFG,
and it's very specifically because of the dynamic feature crud.

So rather than add a bunch of logic to sanitize XCR0, which is at best superfluous
and at worst misleading (again, only XTILE_CFG can ever be insane), I think we
should just special case XTILE_CFG and add a big fat comment explaining why KVM
further manipulates the "supported" XCR0.

Ah, and that's more or less what Aaron had in v1, but then Jim asked about MPX[1]
and things went off the rails (I'm just relieved that I wasn't the one to send
you awry and then complain about the result).

Aaron and/or Mingwei, can you give the attached patches a spin?  Patch 1 is a
slightly reworked version of Aaron's patch 1, and patch 2 implements what I just
described (guts of patch 2 also pasted below).  If things look good, I'll post a v4
of this series.

[0] https://lore.kernel.org/all/Y7R36wsXn3JqwfEv@google.com
[1] https://lore.kernel.org/all/CALMp9eQD8EpS50A0iAxsoaW-_yFmWERWw6rbAh9VSEJjDrMkNQ@mail.gmail.com


---
 arch/x86/kvm/x86.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index b6c6988d99b5..ae235bc2b9bc 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -3,6 +3,7 @@
 #define ARCH_X86_KVM_X86_H
 
 #include <linux/kvm_host.h>
+#include <asm/fpu/xstate.h>
 #include <asm/mce.h>
 #include <asm/pvclock.h>
 #include "kvm_cache_regs.h"
@@ -325,7 +326,22 @@ extern bool enable_pmu;
  */
 static inline u64 kvm_get_filtered_xcr0(void)
 {
-	return kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
+	u64 supported_xcr0 = kvm_caps.supported_xcr0;
+
+	BUILD_BUG_ON(XFEATURE_MASK_USER_DYNAMIC != XFEATURE_MASK_XTILE_DATA);
+
+	if (supported_xcr0 & XFEATURE_MASK_USER_DYNAMIC) {
+		supported_xcr0 &= xstate_get_guest_group_perm();
+
+		/*
+		 * Treat XTILE_CFG as unsupported if the current process isn't
+		 * allowed to use XTILE_DATA, as attempting to set XTILE_CFG in
+		 * XCR0 without setting XTILE_DATA is architecturally illegal.
+		 */
+		if (!(supported_xcr0 & XFEATURE_MASK_XTILE_DATA))
+			supported_xcr0 &= XFEATURE_MASK_XTILE_CFG;
+	}
+	return supported_xcr0;
 }
 
 static inline bool kvm_mpx_supported(void)
-- 


[-- Attachment #2: 0001-KVM-x86-Add-a-helper-to-handle-filtering-of-unpermit.patch --]
[-- Type: text/x-diff, Size: 3207 bytes --]

From 0c0a6bdcd8edd04874344dd71c135bb211605c25 Mon Sep 17 00:00:00 2001
From: Aaron Lewis <aaronlewis@google.com>
Date: Fri, 24 Feb 2023 22:36:00 +0000
Subject: [PATCH 1/2] KVM: x86: Add a helper to handle filtering of unpermitted
 XCR0 features

Add a helper, kvm_get_filtered_xcr0(), to dedup code that needs to account
for XCR0 features that require explicit opt-in on a per-process basis.  In
addition to documenting when KVM should/shouldn't consult
xstate_get_guest_group_perm(), the helper will also allow sanitizing the
filtered XCR0 to avoid enumerating architecturally illegal XCR0 values,
e.g. XTILE_CFG without XTILE_DATA.

No functional changes intended.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
Reviewed-by: Mingwei Zhang <mizhang@google.com>
Link: https://lore.kernel.org/r/20230224223607.1580880-2-aaronlewis@google.com
[sean: rename helper, move to x86.h, massage changelog]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/cpuid.c |  2 +-
 arch/x86/kvm/x86.c   |  4 +---
 arch/x86/kvm/x86.h   | 13 +++++++++++++
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 1ad3bde72526..98a46e46ee9e 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -1002,7 +1002,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
 		entry->eax = entry->ebx = entry->ecx = 0;
 		break;
 	case 0xd: {
-		u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
+		u64 permitted_xcr0 = kvm_get_filtered_xcr0();
 		u64 permitted_xss = kvm_caps.supported_xss;
 
 		entry->eax &= permitted_xcr0;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3fab192862d4..0e5a0078bb4d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4547,9 +4547,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 			r = 0;
 		break;
 	case KVM_CAP_XSAVE2: {
-		u64 guest_perm = xstate_get_guest_group_perm();
-
-		r = xstate_required_size(kvm_caps.supported_xcr0 & guest_perm, false);
+		r = xstate_required_size(kvm_get_filtered_xcr0(), false);
 		if (r < sizeof(struct kvm_xsave))
 			r = sizeof(struct kvm_xsave);
 		break;
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 203fb6640b5b..b6c6988d99b5 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -315,6 +315,19 @@ extern struct kvm_caps kvm_caps;
 
 extern bool enable_pmu;
 
+/*
+ * Get a filtered version of KVM's supported XCR0 that strips out dynamic
+ * features for which the current process doesn't (yet) have permission to use.
+ * This is intended to be used only when enumerating support to userspace,
+ * e.g. in KVM_GET_SUPPORTED_CPUID and KVM_CAP_XSAVE2, it does NOT need to be
+ * used to check/restrict guest behavior as KVM rejects KVM_SET_CPUID{2} if
+ * userspace attempts to enable unpermitted features.
+ */
+static inline u64 kvm_get_filtered_xcr0(void)
+{
+	return kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
+}
+
 static inline bool kvm_mpx_supported(void)
 {
 	return (kvm_caps.supported_xcr0 & (XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR))

base-commit: 68f7c82ab1b8c7057b0c241907ff7906c7407e6d
-- 
2.40.0.348.gf938b09366-goog


[-- Attachment #3: 0002-KVM-x86-Filter-out-XTILE_CFG-if-XTILE_DATA-isn-t-per.patch --]
[-- Type: text/x-diff, Size: 2037 bytes --]

From 39775081bd99ef8dc8b38852f5dad82ca216de5e Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Thu, 23 Mar 2023 12:52:29 -0700
Subject: [PATCH 2/2] KVM: x86: Filter out XTILE_CFG if XTILE_DATA isn't
 permitted

Filter out XTILE_CFG from the supported XCR0 reported to userspace if the
current process doesn't have access to XTILE_DATA.  Attempting to set
XTILE_CFG in XCR0 will #GP if XTILE_DATA is also not set, and so keeping
XTILE_CFG as supported results in xplosions if userspace feeds
KVM_GET_SUPPORTED_CPUID back into KVM and the guest doesn't sanity check
CPUID.

Fixes: 445ecdf79be0 ("kvm: x86: Exclude unpermitted xfeatures at KVM_GET_SUPPORTED_CPUID")
Reported-by: Aaron Lewis <aaronlewis@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index b6c6988d99b5..ae235bc2b9bc 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -3,6 +3,7 @@
 #define ARCH_X86_KVM_X86_H
 
 #include <linux/kvm_host.h>
+#include <asm/fpu/xstate.h>
 #include <asm/mce.h>
 #include <asm/pvclock.h>
 #include "kvm_cache_regs.h"
@@ -325,7 +326,22 @@ extern bool enable_pmu;
  */
 static inline u64 kvm_get_filtered_xcr0(void)
 {
-	return kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
+	u64 supported_xcr0 = kvm_caps.supported_xcr0;
+
+	BUILD_BUG_ON(XFEATURE_MASK_USER_DYNAMIC != XFEATURE_MASK_XTILE_DATA);
+
+	if (supported_xcr0 & XFEATURE_MASK_USER_DYNAMIC) {
+		supported_xcr0 &= xstate_get_guest_group_perm();
+
+		/*
+		 * Treat XTILE_CFG as unsupported if the current process isn't
+		 * allowed to use XTILE_DATA, as attempting to set XTILE_CFG in
+		 * XCR0 without setting XTILE_DATA is architecturally illegal.
+		 */
+		if (!(supported_xcr0 & XFEATURE_MASK_XTILE_DATA))
+			supported_xcr0 &= XFEATURE_MASK_XTILE_CFG;
+	}
+	return supported_xcr0;
 }
 
 static inline bool kvm_mpx_supported(void)
-- 
2.40.0.348.gf938b09366-goog

Re: [PATCH v3 2/8] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Mingwei Zhang]

>
> Aaron and/or Mingwei, can you give the attached patches a spin?  Patch 1 is a
> slightly reworked version of Aaron's patch 1, and patch 2 implements what I just
> described (guts of patch 2 also pasted below).  If things look good, I'll post a v4
> of this series.

Overall I don't have a strong opinion. Extending the sanitization from
AMX to AVX512 and MPX does seem to slightly change the purpose from
permission adjustment to hw feature sanitization. So, it is ok for me
to see the push for AVX512 and MPX as a different motivation, thus
peeling them off from this series.

>
> [0] https://lore.kernel.org/all/Y7R36wsXn3JqwfEv@google.com
> [1] https://lore.kernel.org/all/CALMp9eQD8EpS50A0iAxsoaW-_yFmWERWw6rbAh9VSEJjDrMkNQ@mail.gmail.com
>
I can take a look but in general [1] the whole series should fix the
problem. Let me check the following code.

>
> ---
>  arch/x86/kvm/x86.h | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index b6c6988d99b5..ae235bc2b9bc 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -3,6 +3,7 @@
>  #define ARCH_X86_KVM_X86_H
>
>  #include <linux/kvm_host.h>
> +#include <asm/fpu/xstate.h>
>  #include <asm/mce.h>
>  #include <asm/pvclock.h>
>  #include "kvm_cache_regs.h"
> @@ -325,7 +326,22 @@ extern bool enable_pmu;
>   */
>  static inline u64 kvm_get_filtered_xcr0(void)
>  {
> -       return kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
> +       u64 supported_xcr0 = kvm_caps.supported_xcr0;
> +
> +       BUILD_BUG_ON(XFEATURE_MASK_USER_DYNAMIC != XFEATURE_MASK_XTILE_DATA);
> +
> +       if (supported_xcr0 & XFEATURE_MASK_USER_DYNAMIC) {
> +               supported_xcr0 &= xstate_get_guest_group_perm();
> +
> +               /*
> +                * Treat XTILE_CFG as unsupported if the current process isn't
> +                * allowed to use XTILE_DATA, as attempting to set XTILE_CFG in
> +                * XCR0 without setting XTILE_DATA is architecturally illegal.
> +                */
> +               if (!(supported_xcr0 & XFEATURE_MASK_XTILE_DATA))
> +                       supported_xcr0 &= XFEATURE_MASK_XTILE_CFG;

should be this? supported_xcr0 &= ~XFEATURE_MASK_XTILE_CFG;


> +       }
> +       return supported_xcr0;
>  }
>
>  static inline bool kvm_mpx_supported(void)
> --
>

Re: [PATCH v3 2/8] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Mingwei Zhang]

> >  static inline u64 kvm_get_filtered_xcr0(void)
> >  {
> > -       return kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
> > +       u64 supported_xcr0 = kvm_caps.supported_xcr0;
> > +
> > +       BUILD_BUG_ON(XFEATURE_MASK_USER_DYNAMIC != XFEATURE_MASK_XTILE_DATA);
> > +
> > +       if (supported_xcr0 & XFEATURE_MASK_USER_DYNAMIC) {
> > +               supported_xcr0 &= xstate_get_guest_group_perm();
> > +
> > +               /*
> > +                * Treat XTILE_CFG as unsupported if the current process isn't
> > +                * allowed to use XTILE_DATA, as attempting to set XTILE_CFG in
> > +                * XCR0 without setting XTILE_DATA is architecturally illegal.
> > +                */
> > +               if (!(supported_xcr0 & XFEATURE_MASK_XTILE_DATA))
> > +                       supported_xcr0 &= XFEATURE_MASK_XTILE_CFG;
>
> should be this? supported_xcr0 &= ~XFEATURE_MASK_XTILE_CFG;
>
>
> > +       }
> > +       return supported_xcr0;
> >  }
Also, a minor opinion: shall we use permitted_xcr0 instead of
supported_xcr0 to be consistent with other places? This way,  it is
clear that supported_xcr0 is (almost) never changing.  permitted_xcr0,
as its name suggested, will be subject to permission change.

Re: [PATCH v3 2/8] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Sean Christopherson]

On Thu, Mar 23, 2023, Mingwei Zhang wrote:
> > --- a/arch/x86/kvm/x86.h
> > +++ b/arch/x86/kvm/x86.h
> > @@ -3,6 +3,7 @@
> >  #define ARCH_X86_KVM_X86_H
> >
> >  #include <linux/kvm_host.h>
> > +#include <asm/fpu/xstate.h>
> >  #include <asm/mce.h>
> >  #include <asm/pvclock.h>
> >  #include "kvm_cache_regs.h"
> > @@ -325,7 +326,22 @@ extern bool enable_pmu;
> >   */
> >  static inline u64 kvm_get_filtered_xcr0(void)
> >  {
> > -       return kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
> > +       u64 supported_xcr0 = kvm_caps.supported_xcr0;
> > +
> > +       BUILD_BUG_ON(XFEATURE_MASK_USER_DYNAMIC != XFEATURE_MASK_XTILE_DATA);
> > +
> > +       if (supported_xcr0 & XFEATURE_MASK_USER_DYNAMIC) {
> > +               supported_xcr0 &= xstate_get_guest_group_perm();
> > +
> > +               /*
> > +                * Treat XTILE_CFG as unsupported if the current process isn't
> > +                * allowed to use XTILE_DATA, as attempting to set XTILE_CFG in
> > +                * XCR0 without setting XTILE_DATA is architecturally illegal.
> > +                */
> > +               if (!(supported_xcr0 & XFEATURE_MASK_XTILE_DATA))
> > +                       supported_xcr0 &= XFEATURE_MASK_XTILE_CFG;
> 
> should be this? supported_xcr0 &= ~XFEATURE_MASK_XTILE_CFG;

Doh, yes.

Re: [PATCH v3 2/8] KVM: x86: Clear all supported MPX xfeatures if they are not all set

[Sean Christopherson]

On Thu, Mar 23, 2023, Mingwei Zhang wrote:
> > >  static inline u64 kvm_get_filtered_xcr0(void)
> > >  {
> > > -       return kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
> > > +       u64 supported_xcr0 = kvm_caps.supported_xcr0;
> > > +
> > > +       BUILD_BUG_ON(XFEATURE_MASK_USER_DYNAMIC != XFEATURE_MASK_XTILE_DATA);
> > > +
> > > +       if (supported_xcr0 & XFEATURE_MASK_USER_DYNAMIC) {
> > > +               supported_xcr0 &= xstate_get_guest_group_perm();
> > > +
> > > +               /*
> > > +                * Treat XTILE_CFG as unsupported if the current process isn't
> > > +                * allowed to use XTILE_DATA, as attempting to set XTILE_CFG in
> > > +                * XCR0 without setting XTILE_DATA is architecturally illegal.
> > > +                */
> > > +               if (!(supported_xcr0 & XFEATURE_MASK_XTILE_DATA))
> > > +                       supported_xcr0 &= XFEATURE_MASK_XTILE_CFG;
> >
> > should be this? supported_xcr0 &= ~XFEATURE_MASK_XTILE_CFG;
> >
> >
> > > +       }
> > > +       return supported_xcr0;
> > >  }
> Also, a minor opinion: shall we use permitted_xcr0 instead of
> supported_xcr0 to be consistent with other places? This way,  it is
> clear that supported_xcr0 is (almost) never changing.  permitted_xcr0,
> as its name suggested, will be subject to permission change.

Ya, works for me.

Re: [PATCH v3 8/8] KVM: selftests: Add XCR0 Test

[Sean Christopherson]

On Fri, Feb 24, 2023, Aaron Lewis wrote:
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index ebe83cfe521c..380daa82b023 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -667,6 +667,15 @@ static inline bool this_pmu_has(struct kvm_x86_pmu_feature feature)
>  	       !this_cpu_has(feature.anti_feature);
>  }
>  
> +static __always_inline uint64_t this_cpu_supported_xcr0(void)
> +{
> +	uint32_t a, b, c, d;
> +
> +	cpuid(0xd, &a, &b, &c, &d);
> +
> +	return a | ((uint64_t)d << 32);

This can be done via X86_PROPERTY.  It's not that much prettier, but it at least
avoids open coding things.

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index fa49d51753e5..2bb0ec8dddf3 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -228,8 +228,11 @@ struct kvm_x86_cpu_property {
 #define X86_PROPERTY_PMU_NR_GP_COUNTERS                KVM_X86_CPU_PROPERTY(0xa, 0, EAX, 8, 15)
 #define X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH KVM_X86_CPU_PROPERTY(0xa, 0, EAX, 24, 31)
 
+#define X86_PROPERTY_SUPPORTED_XCR0_LO         KVM_X86_CPU_PROPERTY(0xd,  0, EAX,  0, 31)
 #define X86_PROPERTY_XSTATE_MAX_SIZE_XCR0      KVM_X86_CPU_PROPERTY(0xd,  0, EBX,  0, 31)
 #define X86_PROPERTY_XSTATE_MAX_SIZE           KVM_X86_CPU_PROPERTY(0xd,  0, ECX,  0, 31)
+#define X86_PROPERTY_SUPPORTED_XCR0_HI         KVM_X86_CPU_PROPERTY(0xd,  0, EDX,  0, 31)
+
 #define X86_PROPERTY_XSTATE_TILE_SIZE          KVM_X86_CPU_PROPERTY(0xd, 18, EAX,  0, 31)
 #define X86_PROPERTY_XSTATE_TILE_OFFSET                KVM_X86_CPU_PROPERTY(0xd, 18, EBX,  0, 31)
 #define X86_PROPERTY_AMX_TOTAL_TILE_BYTES      KVM_X86_CPU_PROPERTY(0x1d, 1, EAX,  0, 15)
@@ -669,11 +672,11 @@ static inline bool this_pmu_has(struct kvm_x86_pmu_feature feature)
 
 static __always_inline uint64_t this_cpu_supported_xcr0(void)
 {
-       uint32_t a, b, c, d;
+       if (!this_cpu_has_p(X86_PROPERTY_SUPPORTED_XCR0_LO))
+               return 0;
 
-       cpuid(0xd, &a, &b, &c, &d);
-
-       return a | ((uint64_t)d << 32);
+       return this_cpu_property(X86_PROPERTY_SUPPORTED_XCR0_LO) |
+              ((uint64_t)this_cpu_property(X86_PROPERTY_SUPPORTED_XCR0_HI) << 32);
 }
 
 typedef u32            __attribute__((vector_size(16))) sse128_t;

> +/* Architectural check. */

If you're going to bother with a comment, might as well actually explain the
architectural rule.

/*
 * Assert that architectural dependency rules are satisfied, e.g. that AVX is
 * supported if and only if SSE is supported.
 */

> +#define ASSERT_XFEATURE_DEPENDENCIES(supported_xcr0, xfeatures, dependencies)	  \
> +do {										  \
> +	uint64_t __supported = (supported_xcr0) & ((xfeatures) | (dependencies)); \
> +										  \
> +	GUEST_ASSERT_3((__supported & (xfeatures)) != (xfeatures) ||		  \
> +		       __supported == ((xfeatures) | (dependencies)),		  \
> +		       __supported, (xfeatures), (dependencies));		  \
> +} while (0)
> +

> +/*
> + * KVM's own software-defined rules.  While not architectural it really
> + * ought to be true.

This should call out what KVM's "rule" is.  But it's not really a rule, it's more
of a contract with userspace.

/*
 * Assert that KVM reports a sane, usable as-is XCR0.  Architecturally, a CPU
 * isn't strictly required to _support_ all XFeatures related to a feature, but
 * at the same time XSETBV will #GP if bundled XFeatures aren't enabled and
 * disabled coherently.  E.g. a CPU can technically enumerate supported for
 * XTILE_CFG but not XTILE_DATA, but attempt to enable XTILE_CFG without also
 * enabling XTILE_DATA will #GP.
 */

> +	/* Tell stdout not to buffer its content */
> +	setbuf(stdout, NULL);

This copy pasta is no longer necessary, see kvm_selftest_init().

Re: [PATCH v3 0/8] Clean up the supported xfeatures

[Sean Christopherson]

On Fri, Feb 24, 2023, Aaron Lewis wrote:
> Make sure the supported xfeatures, i.e. EDX:EAX of CPUID.(EAX=0DH,ECX=0),
> for MPX, AVX-512, and AMX are in a valid state and follow the rules
> outlined in the SDM vol 1, section 13.3 ENABLING THE XSAVE FEATURE SET
> AND XSAVE-ENABLED FEATURES.  While those rules apply to the enabled
> xfeatures, i.e. XCR0, use them to set the supported xfeatures.  That way
> if they are used by userspace or a guest to set the enabled xfeatures,
> they won't cause a #GP.  
> 
> A test is then added to verify the supported xfeatures are in this
> sanitied state.
> 
> v2 -> v3:
>  - Sanitize the supported XCR0 in XSAVES2 [Sean]
>  - Split AVX-512 into 2 commits [Sean]
>  - Added XFEATURE_MASK_FP to selftests [Sean]
>  - Reworked XCR0 test to split up architectural and kvm rules [Sean]
> 
> Aaron Lewis (8):
>   KVM: x86: Add kvm_permitted_xcr0()
>   KVM: x86: Clear all supported MPX xfeatures if they are not all set
>   KVM: x86: Clear all supported AVX-512 xfeatures if they are not all set
>   KVM: x86: Clear AVX-512 xfeatures if SSE or AVX is clear
>   KVM: x86: Clear all supported AMX xfeatures if they are not all set
>   KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible
>   KVM: selftests: Add XFEATURE masks to common code
>   KVM: selftests: Add XCR0 Test

A few nits on the new selftest, but I can clean those up when posting v4.  I'll
wait to do that until you've had a chance to weigh in on my proposal (see response
to patch 2).

Thanks!