[PATCH v3 00/13] Overhauling amx_test

[PATCH v3 00/13] Overhauling amx_test

[Mingwei Zhang]

In this version, I have integrated Aaron's changes to the amx_test. In
addition, we also integrated one fix patch for a kernel warning due to
xsave address issue.

Patch 1:
Fix a host FPU kernel warning due to missing XTILEDATA in xinit.

Patch 2-8:
Overhaul amx_test. These patches were basically from v2.

Patch 9-13:
Overhaul amx_test from Aaron. I modified the changelog a little bit.


v2 -> v3:
 - integrate Aaron's 5 commits with minor changes on commit message.
 - Add one fix patch for a kernel warning.

v2:
https://lore.kernel.org/all/20230214184606.510551-1-mizhang@google.com/


Aaron Lewis (5):
  KVM: selftests: x86: Assert that XTILE is XSAVE-enabled
  KVM: selftests: x86: Assert that both XTILE{CFG,DATA} are
    XSAVE-enabled
  KVM: selftests: x86: Remove redundant check that XSAVE is supported
  KVM: selftests: x86: Check that the palette table exists before using
    it
  KVM: selftests: x86: Check that XTILEDATA supports XFD

Mingwei Zhang (8):
  x86/fpu/xstate: Avoid getting xstate address of init_fpstate if
    fpstate contains the component
  KVM: selftests: x86: Add a working xstate data structure
  KVM: selftests: x86: Fix an error in comment of amx_test
  KVM: selftests: x86: Add check of CR0.TS in the #NM handler in
    amx_test
  KVM: selftests: x86: Add the XFD check to IA32_XFD in #NM handler
  KVM: selftests: x86: Fix the checks to XFD_ERR using and operation
  KVM: selftests: x86: Enable checking on xcomp_bv in amx_test
  KVM: selftests: x86: Repeat the checking of xheader when
    IA32_XFD[XTILEDATA] is set in amx_test

 arch/x86/kernel/fpu/xstate.c                  | 10 ++-
 .../selftests/kvm/include/x86_64/processor.h  | 14 ++++
 tools/testing/selftests/kvm/x86_64/amx_test.c | 80 +++++++++----------
 3 files changed, 59 insertions(+), 45 deletions(-)

-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 01/13] x86/fpu/xstate: Avoid getting xstate address of init_fpstate if fpstate contains the component

[Mingwei Zhang]

Avoid getting xstate address of init_fpstate if fpstate contains the xstate
component. Since XTILEDATA (bit 18) was turned off in xinit, when KVM calls
__raw_xsave_addr(xinit, 18), it triggers a warning as follows.

__raw_xsave_addr() is an internal function that assume caller does the
checking, ie., all function arguments should be checked before calling.
So, instead of removing the WARNING, add checks in
__copy_xstate_to_uabi_buf().

[  168.814082] ------------[ cut here ]------------
[  168.814083] WARNING: CPU: 35 PID: 15304 at arch/x86/kernel/fpu/xstate.c:934 __raw_xsave_addr+0xc8/0xe0
[  168.814088] Modules linked in: kvm_intel dummy bridge stp llc cdc_ncm cdc_eem cdc_ether usbnet mii ehci_pci ehci_hcd vfat fat cdc_acm xhci_pci xhci_hcd idpf(O)
[  168.814100] CPU: 35 PID: 15304 Comm: amx_test Tainted: G S         O       6.2.0-smp-DEV #6
[  168.814103] RIP: 0010:__raw_xsave_addr+0xc8/0xe0
[  168.814105] Code: 83 f9 40 72 b0 eb 10 48 63 ca 44 8b 04 8d 60 13 1e 82 eb 03 41 89 f8 44 89 c1 48 01 c8 48 83 c4 08 5d c3 cc 0f 0b 31 c0 eb f3 <0f> 0b 48 c7 c7 c7 28 11 82 e8 da 30 b0 00 31 c0 eb e1 66 0f 1f 44
[  168.814106] RSP: 0018:ff110020ef79bc90 EFLAGS: 00010246
[  168.814108] RAX: ffffffff821e0340 RBX: 0000000000000012 RCX: 0000000000000012
[  168.814109] RDX: 0000000000000012 RSI: 80000000000206e7 RDI: 0000000000040000
[  168.814110] RBP: ff110020ef79bc98 R08: 0000000000000a00 R09: 0000000000000012
[  168.814112] R10: 0000000000000012 R11: 0000000000000004 R12: ffa00000089f2a40
[  168.814113] R13: 0000001200000000 R14: 0000000000000012 R15: ff110020ef288b00
[  168.814114] FS:  00007f1812761300(0000) GS:ff11003fff4c0000(0000) knlGS:0000000000000000
[  168.814116] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  168.814117] CR2: 00007f1812555008 CR3: 0000002093a80002 CR4: 0000000000373ee0
[  168.814118] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  168.814119] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
[  168.814120] Call Trace:
[  168.814121]  <TASK>
[  168.814122]  __copy_xstate_to_uabi_buf+0x3cb/0x520
[  168.814125]  fpu_copy_guest_fpstate_to_uabi+0x29/0x50
[  168.814127]  kvm_arch_vcpu_ioctl+0x9f7/0xee0
[  168.814130]  ? __kmem_cache_free+0x16b/0x220
[  168.814133]  kvm_vcpu_ioctl+0x47c/0x5a0
[  168.814136]  __se_sys_ioctl+0x77/0xc0
[  168.814138]  __x64_sys_ioctl+0x1d/0x20
[  168.814139]  do_syscall_64+0x3d/0x80
[  168.814142]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
[  168.814146] RIP: 0033:0x7f1812892c87
[  168.814148] Code: 5d c3 cc 48 8b 05 39 1d 07 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 cc cc cc cc cc cc cc cc cc cc b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 09 1d 07 00 f7 d8 64 89 01 48
[  168.814149] RSP: 002b:00007ffc4cebf538 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[  168.814151] RAX: ffffffffffffffda RBX: 00007f1812761280 RCX: 00007f1812892c87
[  168.814152] RDX: 00000000004dcda0 RSI: 000000009000aecf RDI: 0000000000000007
[  168.814153] RBP: 0000000000002b00 R08: 00000000004d5010 R09: 0000000000002710
[  168.814154] R10: 00007f1812906980 R11: 0000000000000246 R12: 00000000004d8110
[  168.814155] R13: 0000000000000004 R14: 00000000004d78b0 R15: 0000000000000004
[  168.814156]  </TASK>
[  168.814157] ---[ end trace 0000000000000000 ]---

Fixes: e84ba47e313d ("x86/fpu: Hook up PKRU into ptrace()")
Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 arch/x86/kernel/fpu/xstate.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 714166cc25f2..5cc1426c3800 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1063,6 +1063,7 @@ void __copy_xstate_to_uabi_buf(struct membuf to, struct fpstate *fpstate,
 	struct xregs_state *xsave = &fpstate->regs.xsave;
 	struct xstate_header header;
 	unsigned int zerofrom;
+	void *xsave_addr;
 	u64 mask;
 	int i;
 
@@ -1151,10 +1152,11 @@ void __copy_xstate_to_uabi_buf(struct membuf to, struct fpstate *fpstate,
 			pkru.pkru = pkru_val;
 			membuf_write(&to, &pkru, sizeof(pkru));
 		} else {
-			copy_feature(header.xfeatures & BIT_ULL(i), &to,
-				     __raw_xsave_addr(xsave, i),
-				     __raw_xsave_addr(xinit, i),
-				     xstate_sizes[i]);
+			xsave_addr = (header.xfeatures & BIT_ULL(i)) ?
+				__raw_xsave_addr(xsave, i) :
+				__raw_xsave_addr(xinit, i);
+
+			membuf_write(&to, xsave_addr, xstate_sizes[i]);
 		}
 		/*
 		 * Keep track of the last copied state in the non-compacted
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 02/13] KVM: selftests: x86: Add a working xstate data structure

[Mingwei Zhang]

Add a working xstate data structure for the usage of AMX and potential
future usage on other xstate components. AMX selftest requires checking
both the xstate_bv and xcomp_bv. Existing code relies on pointer
arithmetics to fetch xstate_bv and does not support xcomp_bv.

So, add a working xstate data structure into processor.h for x86.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 .../selftests/kvm/include/x86_64/processor.h  | 12 +++++++
 tools/testing/selftests/kvm/x86_64/amx_test.c | 36 ++++++-------------
 2 files changed, 23 insertions(+), 25 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index b1a31de7108a..5cfd7ef40d78 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -45,6 +45,18 @@
 #define X86_CR4_SMAP		(1ul << 21)
 #define X86_CR4_PKE		(1ul << 22)
 
+struct xstate_header {
+	u64				xstate_bv;
+	u64				xcomp_bv;
+	u64				reserved[6];
+} __attribute__((packed));
+
+struct xstate {
+	u8				i387[512];
+	struct xstate_header		header;
+	u8				extended_state_area[0];
+} __attribute__ ((packed, aligned (64)));
+
 /* 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 bd72c6eb3b67..bb9dc0008f43 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -41,10 +41,6 @@
 
 #define XSAVE_HDR_OFFSET		512
 
-struct xsave_data {
-	u8 area[XSAVE_SIZE];
-} __aligned(64);
-
 struct tile_config {
 	u8  palette_id;
 	u8  start_row;
@@ -103,13 +99,13 @@ static inline void __tilerelease(void)
 	asm volatile(".byte 0xc4, 0xe2, 0x78, 0x49, 0xc0" ::);
 }
 
-static inline void __xsavec(struct xsave_data *data, uint64_t rfbm)
+static inline void __xsavec(struct xstate *xstate, uint64_t rfbm)
 {
 	uint32_t rfbm_lo = rfbm;
 	uint32_t rfbm_hi = rfbm >> 32;
 
 	asm volatile("xsavec (%%rdi)"
-		     : : "D" (data), "a" (rfbm_lo), "d" (rfbm_hi)
+		     : : "D" (xstate), "a" (rfbm_lo), "d" (rfbm_hi)
 		     : "memory");
 }
 
@@ -158,16 +154,6 @@ static void set_tilecfg(struct tile_config *cfg)
 	}
 }
 
-static void set_xstatebv(void *data, uint64_t bv)
-{
-	*(uint64_t *)(data + XSAVE_HDR_OFFSET) = bv;
-}
-
-static u64 get_xstatebv(void *data)
-{
-	return *(u64 *)(data + XSAVE_HDR_OFFSET);
-}
-
 static void init_regs(void)
 {
 	uint64_t cr4, xcr0;
@@ -184,7 +170,7 @@ static void init_regs(void)
 
 static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
 						    struct tile_data *tiledata,
-						    struct xsave_data *xsave_data)
+						    struct xstate *xstate)
 {
 	init_regs();
 	check_cpuid_xsave();
@@ -205,9 +191,9 @@ 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);
+	xstate->header.xstate_bv = XFEATURE_MASK_XTILEDATA;
+	__xsavec(xstate, XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(!(xstate->header.xstate_bv & XFEATURE_MASK_XTILEDATA));
 
 	/* xfd=0x40000, disable amx tiledata */
 	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA);
@@ -244,7 +230,7 @@ int main(int argc, char *argv[])
 	struct kvm_run *run;
 	struct kvm_x86_state *state;
 	int xsave_restore_size;
-	vm_vaddr_t amx_cfg, tiledata, xsavedata;
+	vm_vaddr_t amx_cfg, tiledata, xstate;
 	struct ucall uc;
 	u32 amx_offset;
 	int stage, ret;
@@ -284,10 +270,10 @@ int main(int argc, char *argv[])
 	tiledata = vm_vaddr_alloc_pages(vm, 2);
 	memset(addr_gva2hva(vm, tiledata), rand() | 1, 2 * getpagesize());
 
-	/* xsave data for guest_code */
-	xsavedata = vm_vaddr_alloc_pages(vm, 3);
-	memset(addr_gva2hva(vm, xsavedata), 0, 3 * getpagesize());
-	vcpu_args_set(vcpu, 3, amx_cfg, tiledata, xsavedata);
+	/* XSAVE state for guest_code */
+	xstate = vm_vaddr_alloc_pages(vm, DIV_ROUND_UP(XSAVE_SIZE, PAGE_SIZE));
+	memset(addr_gva2hva(vm, xstate), 0, PAGE_SIZE * DIV_ROUND_UP(XSAVE_SIZE, PAGE_SIZE));
+	vcpu_args_set(vcpu, 3, amx_cfg, tiledata, xstate);
 
 	for (stage = 1; ; stage++) {
 		vcpu_run(vcpu);
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 03/13] KVM: selftests: x86: Fix an error in comment of amx_test

[Mingwei Zhang]

After the execution of __tilerelease(), AMX component will be in INIT
state. Therefore, execution of XSAVEC saving the AMX state into memory will
cause the xstate_bv[18] cleared in xheader. However, the xcomp_bv[18] will
remain set. Fix the error in comment. Also, update xsavec() to XSAVEC
because xcomp_bv[18] is set due to the instruction, not the function.
Finally, use XTILEDATA instead 'bit 18' in comments.

Cc: Jim Mattson <jmattson@google.com>
Cc: Venkatesh Srinivas <venkateshs@google.com>
Cc: Aaron Lewis <aaronlewis@google.com>
Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 tools/testing/selftests/kvm/x86_64/amx_test.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index bb9dc0008f43..16c857c1052e 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -190,7 +190,10 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
 	GUEST_SYNC(4);
 	__tilerelease();
 	GUEST_SYNC(5);
-	/* bit 18 not in the XCOMP_BV after xsavec() */
+	/*
+	 * After XSAVEC, XTILEDATA is cleared in the xstate_bv but is set in
+	 * the xcomp_bv.
+	 */
 	xstate->header.xstate_bv = XFEATURE_MASK_XTILEDATA;
 	__xsavec(xstate, XFEATURE_MASK_XTILEDATA);
 	GUEST_ASSERT(!(xstate->header.xstate_bv & XFEATURE_MASK_XTILEDATA));
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 04/13] KVM: selftests: x86: Enable checking on xcomp_bv in amx_test

[Mingwei Zhang]

After tilerelease instruction, AMX tiles are in INIT state. According to
Intel SDM vol 1. 13.10: "If RFBM[i] = 1, XSTATE_BV[i] is set to the
value of XINUSE[i].", XSTATE_BV[18] should be cleared after xsavec.

On the other hand, according to Intel SDM vol 1. 13.4.3: "If XCOMP_BV[i] =
1, state component i is located at a byte offset locationI from the base
address of the XSAVE area". Since at the time of xsavec, XCR0[18] is set
indicating AMX tile data component is still enabled, xcomp_bv[18] should be
set.

Complete the checks by adding the assert to xcomp_bv[18] after xsavec.

Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 tools/testing/selftests/kvm/x86_64/amx_test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index 16c857c1052e..ba8c0afdbac8 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -197,6 +197,7 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
 	xstate->header.xstate_bv = XFEATURE_MASK_XTILEDATA;
 	__xsavec(xstate, XFEATURE_MASK_XTILEDATA);
 	GUEST_ASSERT(!(xstate->header.xstate_bv & XFEATURE_MASK_XTILEDATA));
+	GUEST_ASSERT((xstate->header.xcomp_bv & XFEATURE_MASK_XTILEDATA));
 
 	/* xfd=0x40000, disable amx tiledata */
 	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA);
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 05/13] KVM: selftests: x86: Add check of CR0.TS in the #NM handler in amx_test

[Mingwei Zhang]

Add check of CR0.TS[bit 3] before the check of IA32_XFD_ERR in the #NM
handler in amx_test. This is because XFD may not be the only reason of
the IA32_XFD MSR and the bitmap corresponding to the state components
required by the faulting instruction." (Intel SDM vol 1. Section 13.14)

Add the missing check of CR0.TS.

Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 tools/testing/selftests/kvm/x86_64/amx_test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index ba8c0afdbac8..ac49b14460b6 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -216,6 +216,7 @@ void guest_nm_handler(struct ex_regs *regs)
 {
 	/* Check if #NM is triggered by XFEATURE_MASK_XTILEDATA */
 	GUEST_SYNC(7);
+	GUEST_ASSERT((get_cr0() & X86_CR0_TS) == 0);
 	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
 	GUEST_SYNC(8);
 	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 06/13] KVM: selftests: x86: Add the XFD check to IA32_XFD in #NM handler

[Mingwei Zhang]

Add an extra check to IA32_XFD to ensure the behavior is consistent with
the AMX archtecture. In addition, repeat the checks across context switch
to ensure the values of IA32_XFD and IA32_XFD_ERR are well preserved.

Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 tools/testing/selftests/kvm/x86_64/amx_test.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index ac49b14460b6..296c954dfd6d 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -218,8 +218,10 @@ void guest_nm_handler(struct ex_regs *regs)
 	GUEST_SYNC(7);
 	GUEST_ASSERT((get_cr0() & X86_CR0_TS) == 0);
 	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) & XFEATURE_MASK_XTILEDATA);
 	GUEST_SYNC(8);
 	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) & XFEATURE_MASK_XTILEDATA);
 	/* Clear xfd_err */
 	wrmsr(MSR_IA32_XFD_ERR, 0);
 	/* xfd=0, enable amx */
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 07/13] KVM: selftests: x86: Fix the checks to XFD_ERR using and operation

[Mingwei Zhang]

Fix the checks to XFD_ERR using logical AND operation because XFD_ERR might
contain more information in the future. According Intel SDM Vol 1. 13.14:

"Specifically, the MSR is loaded with the logical AND of the IA32_XFD MSR
and the bitmap corresponding to the state component(s) required by the
faulting instruction."

So fix the check by using AND instead of '=='.

Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 tools/testing/selftests/kvm/x86_64/amx_test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index 296c954dfd6d..62fff3363b3b 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -217,10 +217,10 @@ void guest_nm_handler(struct ex_regs *regs)
 	/* Check if #NM is triggered by XFEATURE_MASK_XTILEDATA */
 	GUEST_SYNC(7);
 	GUEST_ASSERT((get_cr0() & X86_CR0_TS) == 0);
-	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) & XFEATURE_MASK_XTILEDATA);
 	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) & XFEATURE_MASK_XTILEDATA);
 	GUEST_SYNC(8);
-	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) & XFEATURE_MASK_XTILEDATA);
 	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) & XFEATURE_MASK_XTILEDATA);
 	/* Clear xfd_err */
 	wrmsr(MSR_IA32_XFD_ERR, 0);
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 08/13] KVM: selftests: x86: Repeat the checking of xheader when IA32_XFD[XTILEDATA] is set in amx_test

[Mingwei Zhang]

Repeat the checking of AMX component in xheader after XSAVEC when
IA32_XFD[XTILEDATA] is set. This check calibrates the functionality scope
of IA32_XFD: it does not intercept the XSAVE state management. Regardless
of the values in IA32_XFD, AMX component state will still be managed by
XSAVE* and XRSTOR* as long as the corresponding bits are set XCR0.

Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 tools/testing/selftests/kvm/x86_64/amx_test.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index 62fff3363b3b..724e991ba814 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -201,6 +201,16 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
 
 	/* xfd=0x40000, disable amx tiledata */
 	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA);
+
+	/*
+	 * XTILEDATA is cleared in xstate_bv but set in xcomp_bv, this property
+	 * remains the same even when amx tiledata is disabled by IA32_XFD.
+	 */
+	xstate->header.xstate_bv = XFEATURE_MASK_XTILEDATA;
+	__xsavec(xstate, XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(!(xstate->header.xstate_bv & XFEATURE_MASK_XTILEDATA));
+	GUEST_ASSERT((xstate->header.xcomp_bv & XFEATURE_MASK_XTILEDATA));
+
 	GUEST_SYNC(6);
 	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILEDATA);
 	set_tilecfg(amx_cfg);
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 09/13] KVM: selftests: x86: Assert that XTILE is XSAVE-enabled

[Mingwei Zhang]

From: Aaron Lewis <aaronlewis@google.com>

Assert that XTILE is XSAVE-enabled. check_xsave_supports_xtile() doesn't
actually check anything since its return value is not used. Add the
intended assert.

Opportunistically, move the assert to a more appropriate location:
immediately after XSETBV and remove check_xsave_supports_xtile().

Fixes: 5dc19f1c7dd3 ("KVM: selftests: Convert AMX test to use X86_PROPRETY_XXX")
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 tools/testing/selftests/kvm/x86_64/amx_test.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index 724e991ba814..d1fbf8936192 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -115,11 +115,6 @@ static inline void check_cpuid_xsave(void)
 	GUEST_ASSERT(this_cpu_has(X86_FEATURE_OSXSAVE));
 }
 
-static bool check_xsave_supports_xtile(void)
-{
-	return __xgetbv(0) & XFEATURE_MASK_XTILE;
-}
-
 static void check_xtile_info(void)
 {
 	GUEST_ASSERT(this_cpu_has_p(X86_PROPERTY_XSTATE_MAX_SIZE_XCR0));
@@ -166,6 +161,7 @@ static void init_regs(void)
 	xcr0 = __xgetbv(0);
 	xcr0 |= XFEATURE_MASK_XTILE;
 	__xsetbv(0x0, xcr0);
+	GUEST_ASSERT(__xgetbv(0) & XFEATURE_MASK_XTILE);
 }
 
 static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
@@ -174,7 +170,6 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
 {
 	init_regs();
 	check_cpuid_xsave();
-	check_xsave_supports_xtile();
 	check_xtile_info();
 	GUEST_SYNC(1);
 
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 10/13] KVM: selftests: x86: Assert that both XTILE{CFG,DATA} are XSAVE-enabled

[Mingwei Zhang]

From: Aaron Lewis <aaronlewis@google.com>

Assert that both XTILE{CFG,DATA} are XSAVE-enabled.  The original check in
amx_test only ensures at least one of the XTILE bits are set, XTILECFG or
XTILEDATA, when it really should be checking that both are set.

Assert that both XTILECFG and XTILEDATA a set.

Fixes: bf70636d9443 ("selftest: kvm: Add amx selftest")
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 tools/testing/selftests/kvm/x86_64/amx_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index d1fbf8936192..1a1565126255 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -161,7 +161,7 @@ static void init_regs(void)
 	xcr0 = __xgetbv(0);
 	xcr0 |= XFEATURE_MASK_XTILE;
 	__xsetbv(0x0, xcr0);
-	GUEST_ASSERT(__xgetbv(0) & XFEATURE_MASK_XTILE);
+	GUEST_ASSERT((__xgetbv(0) & XFEATURE_MASK_XTILE) == XFEATURE_MASK_XTILE);
 }
 
 static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 11/13] KVM: selftests: x86: Remove redundant check that XSAVE is supported

[Mingwei Zhang]

From: Aaron Lewis <aaronlewis@google.com>

In amx_test, userspace requires that XSAVE is supported before running
the test, then the guest checks that it is supported after enabling
AMX.  Remove the redundant check in the guest that XSAVE is supported.

Opportunistically, move the check that OSXSAVE is set to immediately
after the guest sets it, rather than in a separate helper.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 tools/testing/selftests/kvm/x86_64/amx_test.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index 1a1565126255..deacd21cf744 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -109,12 +109,6 @@ static inline void __xsavec(struct xstate *xstate, uint64_t rfbm)
 		     : "memory");
 }
 
-static inline void check_cpuid_xsave(void)
-{
-	GUEST_ASSERT(this_cpu_has(X86_FEATURE_XSAVE));
-	GUEST_ASSERT(this_cpu_has(X86_FEATURE_OSXSAVE));
-}
-
 static void check_xtile_info(void)
 {
 	GUEST_ASSERT(this_cpu_has_p(X86_PROPERTY_XSTATE_MAX_SIZE_XCR0));
@@ -157,6 +151,7 @@ static void init_regs(void)
 	cr4 = get_cr4();
 	cr4 |= X86_CR4_OSXSAVE;
 	set_cr4(cr4);
+	GUEST_ASSERT(this_cpu_has(X86_FEATURE_OSXSAVE));
 
 	xcr0 = __xgetbv(0);
 	xcr0 |= XFEATURE_MASK_XTILE;
@@ -169,7 +164,6 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
 						    struct xstate *xstate)
 {
 	init_regs();
-	check_cpuid_xsave();
 	check_xtile_info();
 	GUEST_SYNC(1);
 
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 12/13] KVM: selftests: x86: Check that the palette table exists before using it

[Mingwei Zhang]

From: Aaron Lewis <aaronlewis@google.com>

Check that the palette table exists before using it. The maximum number of
AMX palette tables is enumerated by CPUID.1DH:EAX. Assert that the palette
used in amx_test, CPUID.1DH.1H, does not exceed that maximum.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 tools/testing/selftests/kvm/include/x86_64/processor.h | 1 +
 tools/testing/selftests/kvm/x86_64/amx_test.c          | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 5cfd7ef40d78..a6a86c41ed75 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -223,6 +223,7 @@ struct kvm_x86_cpu_property {
 #define X86_PROPERTY_XSTATE_MAX_SIZE		KVM_X86_CPU_PROPERTY(0xd,  0, ECX,  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_MAX_PALETTE_TABLES	KVM_X86_CPU_PROPERTY(0x1d, 0, EAX,  0, 31)
 #define X86_PROPERTY_AMX_TOTAL_TILE_BYTES	KVM_X86_CPU_PROPERTY(0x1d, 1, EAX,  0, 15)
 #define X86_PROPERTY_AMX_BYTES_PER_TILE		KVM_X86_CPU_PROPERTY(0x1d, 1, EAX, 16, 31)
 #define X86_PROPERTY_AMX_BYTES_PER_ROW		KVM_X86_CPU_PROPERTY(0x1d, 1, EBX, 0,  15)
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index deacd21cf744..2fd6a8a928d9 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -30,6 +30,7 @@
 #define XSAVE_SIZE			((NUM_TILES * TILE_SIZE) + PAGE_SIZE)
 
 /* Tile configuration associated: */
+#define PALETTE_TABLE_INDEX		1
 #define MAX_TILES			16
 #define RESERVED_BYTES			14
 
@@ -120,6 +121,10 @@ static void check_xtile_info(void)
 	GUEST_ASSERT(xtile.xsave_size == 8192);
 	GUEST_ASSERT(sizeof(struct tile_data) >= xtile.xsave_size);
 
+	GUEST_ASSERT(this_cpu_has_p(X86_PROPERTY_AMX_MAX_PALETTE_TABLES));
+	GUEST_ASSERT(this_cpu_property(X86_PROPERTY_AMX_MAX_PALETTE_TABLES) >=
+		     PALETTE_TABLE_INDEX);
+
 	GUEST_ASSERT(this_cpu_has_p(X86_PROPERTY_AMX_NR_TILE_REGS));
 	xtile.max_names = this_cpu_property(X86_PROPERTY_AMX_NR_TILE_REGS);
 	GUEST_ASSERT(xtile.max_names == 8);
-- 
2.39.2.637.g21b0678d19-goog

[PATCH v3 13/13] KVM: selftests: x86: Check that XTILEDATA supports XFD

[Mingwei Zhang]

From: Aaron Lewis <aaronlewis@google.com>

Check that XTILEDATA supports XFD. In amx_test, add the requirement that
the guest allows the xfeature, XTILEDATA, to be set in XFD. Otherwise, the
test may fail.

Signed-off-by: Aaron Lewis <aaronlewis@google.com>
Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 tools/testing/selftests/kvm/include/x86_64/processor.h | 1 +
 tools/testing/selftests/kvm/x86_64/amx_test.c          | 1 +
 2 files changed, 2 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index a6a86c41ed75..4f6d2d31ff34 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -140,6 +140,7 @@ struct kvm_x86_cpu_feature {
 #define	X86_FEATURE_XTILEDATA		KVM_X86_CPU_FEATURE(0xD, 0, EAX, 18)
 #define	X86_FEATURE_XSAVES		KVM_X86_CPU_FEATURE(0xD, 1, EAX, 3)
 #define	X86_FEATURE_XFD			KVM_X86_CPU_FEATURE(0xD, 1, EAX, 4)
+#define X86_FEATURE_XTILEDATA_XFD	KVM_X86_CPU_FEATURE(0xD, 18, ECX, 2)
 
 /*
  * Extended Leafs, a.k.a. AMD defined
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index 2fd6a8a928d9..2eb265297898 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -257,6 +257,7 @@ int main(int argc, char *argv[])
 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_AMX_TILE));
 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XTILECFG));
 	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XTILEDATA));
+	TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XTILEDATA_XFD));
 
 	/* Create VM */
 	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
-- 
2.39.2.637.g21b0678d19-goog

Re: [PATCH v3 01/13] x86/fpu/xstate: Avoid getting xstate address of init_fpstate if fpstate contains the component

[Thomas Gleixner]

Mingwei!

On Tue, Feb 21 2023 at 16:36, Mingwei Zhang wrote:
> Avoid getting xstate address of init_fpstate if fpstate contains the xstate
> component. Since XTILEDATA (bit 18) was turned off in xinit, when KVM calls
> __raw_xsave_addr(xinit, 18), it triggers a warning as follows.

This does not parse. Aside of that it gets the ordering of the changelog
wrong. It explain first what the patch is doing by repeating the way too
long subject line and then tries to give some explanation about the
problem.

KVM does not call __raw_xsave_addr() and the problem is completely
independent of KVM.

> __raw_xsave_addr() is an internal function that assume caller does the
> checking, ie., all function arguments should be checked before calling.
> So, instead of removing the WARNING, add checks in
> __copy_xstate_to_uabi_buf().

I don't see checks added. The patch open codes copy_feature() and makes
the __raw_xsave_addr() invocations conditional.

So something like this:

   Subject: x86/fpu/xstate: Prevent false-positive warning in __copy_xstate_to_uabi_buf()

   __copy_xstate_to_uabi_buf() copies either from the tasks XSAVE buffer
   or from init_fpstate into the ptrace buffer. Dynamic features, like
   XTILEDATA, have an all zeroes init state and are not saved in
   init_fpstate, which means the corresponding bit is not set in the
   xfeatures bitmap of the init_fpstate header.

   But __copy_xstate_to_uabi_buf() retrieves addresses for both the
   tasks xstate and init_fpstate unconditionally via __raw_xsave_addr().

   So if the tasks XSAVE buffer has a dynamic feature set, then the
   address retrieval for init_fpstate triggers the warning in
   __raw_xsave_addr() which checks the feature bit in the init_fpstate
   header.

   Prevent this by open coding copy_feature() and making the address
   retrieval conditional on the tasks XSAVE header bit.

So the order here is (in paragraphs):

   1) Explain the context
   2) Explain whats wrong
   3) Explain the consequence
   4) Explain the solution briefly

It does not even need a backtrace, because the backtrace does not give
any relevant information which is not in the text above already.

The actual callchain is completely irrelevant for desribing this
issue. If you really want to add a backtrace, then please trim it by
removing the irrelevant information. See
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces
for further information.

So for this case this would be:

WARNING: CPU: 35 PID: 15304 at arch/x86/kernel/fpu/xstate.c:934 __raw_xsave_addr+0xc8/0xe0
Call Trace:
 <TASK>
 __copy_xstate_to_uabi_buf+0x3cb/0x520
 fpu_copy_guest_fpstate_to_uabi+0x29/0x50

But even fpu_copy_guest_fpstate_to_uabi() is already useless because
__copy_xstate_to_uabi_buf() has multiple callers which all have the very
same problem and they are very simple to find.

Backtraces are useful to illustrate a hard to understand callchain, but
having ~40 lines of backtrace which only contains two relevant lines is
just a distraction.

See?

> Fixes: e84ba47e313d ("x86/fpu: Hook up PKRU into ptrace()")

The problem did not exist at this point in time because dynamic
xfeatures did not exist, neither in hardware nor in kernel support.

Even if dynamic features would have existed then the commit would not be
the one which introduced the problem, All the commit does is to move
back then valid code into a conditional code path.

It fixes:

  471f0aa7fa64 ("x86/fpu: Fix copy_xstate_to_uabi() to copy init states correctly")

which attempted to fix exactly the problem you are seeing, but only
addressed half of it. The underlying problem was introduced with
2308ee57d93d ("x86/fpu/amx: Enable the AMX feature in 64-bit mode")

Random fixes tags are not really helpful.

> @@ -1151,10 +1152,11 @@ void __copy_xstate_to_uabi_buf(struct membuf to, struct fpstate *fpstate,
>  			pkru.pkru = pkru_val;
>  			membuf_write(&to, &pkru, sizeof(pkru));
>  		} else {
> -			copy_feature(header.xfeatures & BIT_ULL(i), &to,
> -				     __raw_xsave_addr(xsave, i),
> -				     __raw_xsave_addr(xinit, i),
> -				     xstate_sizes[i]);

Please add a comment here to explain why this is open coded and does not
use copy_feature(). Something like:

    			/*
                         * Open code copy_feature() to prevent retrieval
                         * of a dynamic features address from xinit, which
                         * is invalid because xinit does not contain the
                         * all zeros init states of dynamic features and
                         * emits a warning.
                         */

> +			xsave_addr = (header.xfeatures & BIT_ULL(i)) ?
> +				__raw_xsave_addr(xsave, i) :
> +				__raw_xsave_addr(xinit, i);
> +
> +			membuf_write(&to, xsave_addr, xstate_sizes[i]);

Other than that. Nice detective work!

Thanks,

        tglx

Re: [PATCH v3 01/13] x86/fpu/xstate: Avoid getting xstate address of init_fpstate if fpstate contains the component

[Chang S. Bae]

On 2/21/2023 8:36 AM, Mingwei Zhang wrote:
> Avoid getting xstate address of init_fpstate if fpstate contains the xstate
> component. Since XTILEDATA (bit 18) was turned off in xinit, when KVM calls
> __raw_xsave_addr(xinit, 18), it triggers a warning as follows.
> 
> __raw_xsave_addr() is an internal function that assume caller does the
> checking, ie., all function arguments should be checked before calling.
> So, instead of removing the WARNING, add checks in
> __copy_xstate_to_uabi_buf().
> 

<snip>

> @@ -1151,10 +1152,11 @@ void __copy_xstate_to_uabi_buf(struct membuf to, struct fpstate *fpstate,
>   			pkru.pkru = pkru_val;
>   			membuf_write(&to, &pkru, sizeof(pkru));
>   		} else {
> -			copy_feature(header.xfeatures & BIT_ULL(i), &to,
> -				     __raw_xsave_addr(xsave, i),
> -				     __raw_xsave_addr(xinit, i),
> -				     xstate_sizes[i]);
> +			xsave_addr = (header.xfeatures & BIT_ULL(i)) ?
> +				__raw_xsave_addr(xsave, i) :
> +				__raw_xsave_addr(xinit, i);
> +
> +			membuf_write(&to, xsave_addr, xstate_sizes[i]);
>   		}
>   		/*
>   		 * Keep track of the last copied state in the non-compacted

So this hunk is under for_each_extended_xfeature(i, mask) -- it skips 
the copy routine if mask[i] == 0; instead, it fills zeros.

We have this [1]:

	if (fpu_state_size_dynamic())
		mask &= (header.xfeatures | xinit->header.xcomp_bv);

If header.xfeatures[18] = 0 then mask[18] = 0 because 
xinit->header.xcomp_bv[18] = 0. Then, it won't hit that code. So, I'm 
confused about the problem that you described here.

Can you elaborate on your test case a bit? Let me try to reproduce the 
issue on my end.

Thanks,
Chang

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/xstate.c#n1134

Re: [PATCH v3 01/13] x86/fpu/xstate: Avoid getting xstate address of init_fpstate if fpstate contains the component

[Thomas Gleixner]

On Tue, Feb 21 2023 at 19:05, Chang S. Bae wrote:
> On 2/21/2023 8:36 AM, Mingwei Zhang wrote:
>> @@ -1151,10 +1152,11 @@ void __copy_xstate_to_uabi_buf(struct membuf to, struct fpstate *fpstate,
>>   			pkru.pkru = pkru_val;
>>   			membuf_write(&to, &pkru, sizeof(pkru));
>>   		} else {
>> -			copy_feature(header.xfeatures & BIT_ULL(i), &to,
>> -				     __raw_xsave_addr(xsave, i),
>> -				     __raw_xsave_addr(xinit, i),
>> -				     xstate_sizes[i]);
>> +			xsave_addr = (header.xfeatures & BIT_ULL(i)) ?
>> +				__raw_xsave_addr(xsave, i) :
>> +				__raw_xsave_addr(xinit, i);
>> +
>> +			membuf_write(&to, xsave_addr, xstate_sizes[i]);
>>   		}
>>   		/*
>>   		 * Keep track of the last copied state in the non-compacted
>
> So this hunk is under for_each_extended_xfeature(i, mask) -- it skips 
> the copy routine if mask[i] == 0; instead, it fills zeros.
>
> We have this [1]:
>
> 	if (fpu_state_size_dynamic())
> 		mask &= (header.xfeatures | xinit->header.xcomp_bv);
>
> If header.xfeatures[18] = 0 then mask[18] = 0 because 
> xinit->header.xcomp_bv[18] = 0. Then, it won't hit that code. So, I'm 
> confused about the problem that you described here.

Read the suggested changelog I wrote in my reply to Mingwei.

TLDR:

        xsave.header.xfeatures[18] = 1
        xinit.header.xfeatures[18] = 0
    ->  mask[18] = 1
    ->  __raw_xsave_addr(xsave, 18)     <- Success
    ->  __raw_xsave_addr(xinit, 18)     <- WARN

Thanks,

        tglx

Re: [PATCH v3 01/13] x86/fpu/xstate: Avoid getting xstate address of init_fpstate if fpstate contains the component

[Mingwei Zhang]

> > We have this [1]:
> >
> >       if (fpu_state_size_dynamic())
> >               mask &= (header.xfeatures | xinit->header.xcomp_bv);
> >
> > If header.xfeatures[18] = 0 then mask[18] = 0 because
> > xinit->header.xcomp_bv[18] = 0. Then, it won't hit that code. So, I'm
> > confused about the problem that you described here.
>
> Read the suggested changelog I wrote in my reply to Mingwei.
>
> TLDR:
>
>         xsave.header.xfeatures[18] = 1
>         xinit.header.xfeatures[18] = 0
>     ->  mask[18] = 1
>     ->  __raw_xsave_addr(xsave, 18)     <- Success
>     ->  __raw_xsave_addr(xinit, 18)     <- WARN
>
> Thanks,
>
>         tglx

Hi Thomas,

Thanks for the review and I will provide the next version separately
from the series, since this one is independent from the rest.

Chang: to reproduce this issue, you can simply run the amx_test in the
kvm selftest directory.

Thanks.
-Mingwei

Re: [PATCH v3 01/13] x86/fpu/xstate: Avoid getting xstate address of init_fpstate if fpstate contains the component

[Chang S. Bae]

On 2/22/2023 10:40 AM, Mingwei Zhang wrote:
>>> We have this [1]:
>>>
>>>        if (fpu_state_size_dynamic())
>>>                mask &= (header.xfeatures | xinit->header.xcomp_bv);
>>>
>>> If header.xfeatures[18] = 0 then mask[18] = 0 because
>>> xinit->header.xcomp_bv[18] = 0. Then, it won't hit that code. So, I'm
>>> confused about the problem that you described here.
>>
>> Read the suggested changelog I wrote in my reply to Mingwei.
>>
>> TLDR:
>>
>>          xsave.header.xfeatures[18] = 1
>>          xinit.header.xfeatures[18] = 0
>>      ->  mask[18] = 1
>>      ->  __raw_xsave_addr(xsave, 18)     <- Success
>>      ->  __raw_xsave_addr(xinit, 18)     <- WARN

Oh, sigh.. This should be caught last time.

Hmm, then since we store init state for legacy ones [1], unless it is 
too aggressive, perhaps the loop can be simplified like this:

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 714166cc25f2..2dac6f5f3ade 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1118,21 +1118,13 @@ void __copy_xstate_to_uabi_buf(struct membuf to, 
struct fpstate *fpstate,
         zerofrom = offsetof(struct xregs_state, extended_state_area);

         /*
-        * The ptrace buffer is in non-compacted XSAVE format.  In
-        * non-compacted format disabled features still occupy state space,
-        * but there is no state to copy from in the compacted
-        * init_fpstate. The gap tracking will zero these states.
+        * Indicate which states to copy from fpstate. When not present in
+        * fpstate, those extended states are either initialized or
+        * disabled. They are also known to have an all zeros init state.
+        * Thus, remove them from 'mask' to zero those features in the user
+        * buffer instead of retrieving them from init_fpstate.
          */
-       mask = fpstate->user_xfeatures;
-
-       /*
-        * Dynamic features are not present in init_fpstate. When they are
-        * in an all zeros init state, remove those from 'mask' to zero
-        * those features in the user buffer instead of retrieving them
-        * from init_fpstate.
-        */
-       if (fpu_state_size_dynamic())
-               mask &= (header.xfeatures | xinit->header.xcomp_bv);
+       mask = header.xfeatures;

         for_each_extended_xfeature(i, mask) {
                 /*
@@ -1151,9 +1143,8 @@ void __copy_xstate_to_uabi_buf(struct membuf to, 
struct fpstate *fpstate,
                         pkru.pkru = pkru_val;
                         membuf_write(&to, &pkru, sizeof(pkru));
                 } else {
-                       copy_feature(header.xfeatures & BIT_ULL(i), &to,
+                       membuf_write(&to,
                                      __raw_xsave_addr(xsave, i),
-                                    __raw_xsave_addr(xinit, i),
                                      xstate_sizes[i]);
                 }
                 /*

> Chang: to reproduce this issue, you can simply run the amx_test in the
> kvm selftest directory.

Yeah, I was able to reproduce it with this ptrace test:

diff --git a/tools/testing/selftests/x86/amx.c 
b/tools/testing/selftests/x86/amx.c
index 625e42901237..ae02bc81846d 100644
--- a/tools/testing/selftests/x86/amx.c
+++ b/tools/testing/selftests/x86/amx.c
@@ -14,8 +14,10 @@
  #include <sys/auxv.h>
  #include <sys/mman.h>
  #include <sys/shm.h>
+#include <sys/ptrace.h>
  #include <sys/syscall.h>
  #include <sys/wait.h>
+#include <sys/uio.h>

  #include "../kselftest.h" /* For __cpuid_count() */

@@ -826,6 +828,76 @@ static void test_context_switch(void)
         free(finfo);
  }

+/* Ptrace test */
+
+static bool inject_tiledata(pid_t target)
+{
+       struct xsave_buffer *xbuf;
+       struct iovec iov;
+
+       xbuf = alloc_xbuf();
+       if (!xbuf)
+               fatal_error("unable to allocate XSAVE buffer");
+
+       load_rand_tiledata(xbuf);
+
+       memcpy(&stashed_xsave->bytes[xtiledata.xbuf_offset],
+              &xbuf->bytes[xtiledata.xbuf_offset],
+              xtiledata.size);
+
+       iov.iov_base = xbuf;
+       iov.iov_len = xbuf_size;
+
+       if (ptrace(PTRACE_SETREGSET, target, (uint32_t)NT_X86_XSTATE, &iov))
+               fatal_error("PTRACE_SETREGSET");
+
+       if (ptrace(PTRACE_GETREGSET, target, (uint32_t)NT_X86_XSTATE, &iov))
+               err(1, "PTRACE_GETREGSET");
+
+       if (!memcmp(&stashed_xsave->bytes[xtiledata.xbuf_offset],
+                   &xbuf->bytes[xtiledata.xbuf_offset],
+                   xtiledata.size))
+               return true;
+       else
+               return false;
+}
+
+static void test_ptrace(void)
+{
+       pid_t child;
+       int status;
+
+       child = fork();
+       if (child < 0) {
+               err(1, "fork");
+       } else if (!child) {
+               if (ptrace(PTRACE_TRACEME, 0, NULL, NULL))
+                       err(1, "PTRACE_TRACEME");
+
+               /* Use the state to expand the kernel buffer */
+               load_rand_tiledata(stashed_xsave);
+
+               raise(SIGTRAP);
+               _exit(0);
+       }
+
+       do {
+               wait(&status);
+       } while (WSTOPSIG(status) != SIGTRAP);
+
+       printf("\tInject tile data via ptrace()\n");
+
+       if (inject_tiledata(child))
+               printf("[OK]\tTile data was written on ptracee.\n");
+       else
+               printf("[FAIL]\tTile data was not written on ptracee.\n");
+
+       ptrace(PTRACE_DETACH, child, NULL, NULL);
+       wait(&status);
+       if (!WIFEXITED(status) || WEXITSTATUS(status))
+               err(1, "ptrace test");
+}
+
  int main(void)
  {
         /* Check hardware availability at first */
@@ -846,6 +918,8 @@ int main(void)
         ctxtswtest_config.num_threads = 5;
         test_context_switch();

+       test_ptrace();
+
         clearhandler(SIGILL);
         free_stashed_xsave();

Thanks,
Chang

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/xstate.c#n386

Re: [PATCH v3 01/13] x86/fpu/xstate: Avoid getting xstate address of init_fpstate if fpstate contains the component

[Mingwei Zhang]

On Wed, Feb 22, 2023, Chang S. Bae wrote:
> On 2/22/2023 10:40 AM, Mingwei Zhang wrote:
> > > > We have this [1]:
> > > > 
> > > >        if (fpu_state_size_dynamic())
> > > >                mask &= (header.xfeatures | xinit->header.xcomp_bv);
> > > > 
> > > > If header.xfeatures[18] = 0 then mask[18] = 0 because
> > > > xinit->header.xcomp_bv[18] = 0. Then, it won't hit that code. So, I'm
> > > > confused about the problem that you described here.
> > > 
> > > Read the suggested changelog I wrote in my reply to Mingwei.
> > > 
> > > TLDR:
> > > 
> > >          xsave.header.xfeatures[18] = 1
> > >          xinit.header.xfeatures[18] = 0
> > >      ->  mask[18] = 1
> > >      ->  __raw_xsave_addr(xsave, 18)     <- Success
> > >      ->  __raw_xsave_addr(xinit, 18)     <- WARN
> 
> Oh, sigh.. This should be caught last time.
> 
> Hmm, then since we store init state for legacy ones [1], unless it is too
> aggressive, perhaps the loop can be simplified like this:
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 714166cc25f2..2dac6f5f3ade 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1118,21 +1118,13 @@ void __copy_xstate_to_uabi_buf(struct membuf to,
> struct fpstate *fpstate,
>         zerofrom = offsetof(struct xregs_state, extended_state_area);
> 
>         /*
> -        * The ptrace buffer is in non-compacted XSAVE format.  In
> -        * non-compacted format disabled features still occupy state space,
> -        * but there is no state to copy from in the compacted
> -        * init_fpstate. The gap tracking will zero these states.
> +        * Indicate which states to copy from fpstate. When not present in
> +        * fpstate, those extended states are either initialized or
> +        * disabled. They are also known to have an all zeros init state.
> +        * Thus, remove them from 'mask' to zero those features in the user
> +        * buffer instead of retrieving them from init_fpstate.
>          */
> -       mask = fpstate->user_xfeatures;

Do we need to change this line and the comments? I don't see any of
these was relevant to this issue. The original code semantic is to
traverse all user_xfeatures, if it is available in fpstate, copy it from
there; otherwise, copy it from init_fpstate. We do not assume the
component in init_fpstate (but not in fpstate) are all zeros, do we? If
it is safe to assume that, then it might be ok. But at least in this
patch, I want to keep the original semantics as is without the
assumption.
> -
> -       /*
> -        * Dynamic features are not present in init_fpstate. When they are
> -        * in an all zeros init state, remove those from 'mask' to zero
> -        * those features in the user buffer instead of retrieving them
> -        * from init_fpstate.
> -        */
> -       if (fpu_state_size_dynamic())
> -               mask &= (header.xfeatures | xinit->header.xcomp_bv);
> +       mask = header.xfeatures;

Same here. Let's not adding this optimization in this patch.

>
>         for_each_extended_xfeature(i, mask) {
>                 /*
> @@ -1151,9 +1143,8 @@ void __copy_xstate_to_uabi_buf(struct membuf to,
> struct fpstate *fpstate,
>                         pkru.pkru = pkru_val;
>                         membuf_write(&to, &pkru, sizeof(pkru));
>                 } else {
> -                       copy_feature(header.xfeatures & BIT_ULL(i), &to,
> +                       membuf_write(&to,
>                                      __raw_xsave_addr(xsave, i),
> -                                    __raw_xsave_addr(xinit, i),
>                                      xstate_sizes[i]);
>                 }
>                 /*
> 
> > Chang: to reproduce this issue, you can simply run the amx_test in the
> > kvm selftest directory.
> 
> Yeah, I was able to reproduce it with this ptrace test:
> 
> diff --git a/tools/testing/selftests/x86/amx.c
> b/tools/testing/selftests/x86/amx.c
> index 625e42901237..ae02bc81846d 100644
> --- a/tools/testing/selftests/x86/amx.c
> +++ b/tools/testing/selftests/x86/amx.c
> @@ -14,8 +14,10 @@
>  #include <sys/auxv.h>
>  #include <sys/mman.h>
>  #include <sys/shm.h>
> +#include <sys/ptrace.h>
>  #include <sys/syscall.h>
>  #include <sys/wait.h>
> +#include <sys/uio.h>
> 
>  #include "../kselftest.h" /* For __cpuid_count() */
> 
> @@ -826,6 +828,76 @@ static void test_context_switch(void)
>         free(finfo);
>  }
> 
> +/* Ptrace test */
> +
> +static bool inject_tiledata(pid_t target)
> +{
> +       struct xsave_buffer *xbuf;
> +       struct iovec iov;
> +
> +       xbuf = alloc_xbuf();
> +       if (!xbuf)
> +               fatal_error("unable to allocate XSAVE buffer");
> +
> +       load_rand_tiledata(xbuf);
> +
> +       memcpy(&stashed_xsave->bytes[xtiledata.xbuf_offset],
> +              &xbuf->bytes[xtiledata.xbuf_offset],
> +              xtiledata.size);
> +
> +       iov.iov_base = xbuf;
> +       iov.iov_len = xbuf_size;
> +
> +       if (ptrace(PTRACE_SETREGSET, target, (uint32_t)NT_X86_XSTATE, &iov))
> +               fatal_error("PTRACE_SETREGSET");
> +
> +       if (ptrace(PTRACE_GETREGSET, target, (uint32_t)NT_X86_XSTATE, &iov))
> +               err(1, "PTRACE_GETREGSET");
> +
> +       if (!memcmp(&stashed_xsave->bytes[xtiledata.xbuf_offset],
> +                   &xbuf->bytes[xtiledata.xbuf_offset],
> +                   xtiledata.size))
> +               return true;
> +       else
> +               return false;
> +}
> +
> +static void test_ptrace(void)
> +{
> +       pid_t child;
> +       int status;
> +
> +       child = fork();
> +       if (child < 0) {
> +               err(1, "fork");
> +       } else if (!child) {
> +               if (ptrace(PTRACE_TRACEME, 0, NULL, NULL))
> +                       err(1, "PTRACE_TRACEME");
> +
> +               /* Use the state to expand the kernel buffer */
> +               load_rand_tiledata(stashed_xsave);
> +
> +               raise(SIGTRAP);
> +               _exit(0);
> +       }
> +
> +       do {
> +               wait(&status);
> +       } while (WSTOPSIG(status) != SIGTRAP);
> +
> +       printf("\tInject tile data via ptrace()\n");
> +
> +       if (inject_tiledata(child))
> +               printf("[OK]\tTile data was written on ptracee.\n");
> +       else
> +               printf("[FAIL]\tTile data was not written on ptracee.\n");
> +
> +       ptrace(PTRACE_DETACH, child, NULL, NULL);
> +       wait(&status);
> +       if (!WIFEXITED(status) || WEXITSTATUS(status))
> +               err(1, "ptrace test");
> +}
> +
>  int main(void)
>  {
>         /* Check hardware availability at first */
> @@ -846,6 +918,8 @@ int main(void)
>         ctxtswtest_config.num_threads = 5;
>         test_context_switch();
> 
> +       test_ptrace();
> +
>         clearhandler(SIGILL);
>         free_stashed_xsave();
> 
> Thanks,
> Chang
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/xstate.c#n386
> 

Nice one. Yeah both ptrace and KVM are calling this function so the above
code would also be enough to trigger the bug.


Thanks.
-Mingwei

Re: [PATCH v3 01/13] x86/fpu/xstate: Avoid getting xstate address of init_fpstate if fpstate contains the component

[Chang S. Bae]

On 2/24/2023 3:56 PM, Mingwei Zhang wrote:
> On Wed, Feb 22, 2023, Chang S. Bae wrote:
>>
>>          /*
>> -        * The ptrace buffer is in non-compacted XSAVE format.  In
>> -        * non-compacted format disabled features still occupy state space,
>> -        * but there is no state to copy from in the compacted
>> -        * init_fpstate. The gap tracking will zero these states.
>> +        * Indicate which states to copy from fpstate. When not present in
>> +        * fpstate, those extended states are either initialized or
>> +        * disabled. They are also known to have an all zeros init state.
>> +        * Thus, remove them from 'mask' to zero those features in the user
>> +        * buffer instead of retrieving them from init_fpstate.
>>           */
>> -       mask = fpstate->user_xfeatures;
> 
> Do we need to change this line and the comments? I don't see any of
> these was relevant to this issue. The original code semantic is to
> traverse all user_xfeatures, if it is available in fpstate, copy it from
> there; otherwise, copy it from init_fpstate. We do not assume the
> component in init_fpstate (but not in fpstate) are all zeros, do we? If
> it is safe to assume that, then it might be ok. But at least in this
> patch, I want to keep the original semantics as is without the
> assumption.

Here it has [1]:

	 *
	 * XSAVE could be used, but that would require to reshuffle the
	 * data when XSAVEC/S is available because XSAVEC/S uses xstate
	 * compaction. But doing so is a pointless exercise because most
	 * components have an all zeros init state except for the legacy
	 * ones (FP and SSE). Those can be saved with FXSAVE into the
	 * legacy area. Adding new features requires to ensure that init
	 * state is all zeroes or if not to add the necessary handling
	 * here.
	 */
	fxsave(&init_fpstate.regs.fxsave);

Thus, init_fpstate has zeros for those extended states. Then, copying 
from init_fpstate is the same as membuf_zero() by the gap tracking. But, 
we have two ways to do the same thing here.

So I think it works that simply copying the state from fpstate only for 
those present there, then letting the gap tracking zero out for the rest 
of the userspace buffer for features that are either disabled or 
initialized.

Then, we can remove accessing init_fpstate in the copy loop and which is 
the source of the problem. So I think this line change is relevant and 
also makes the code simple.

I guess I'm fine if you don't want to do this. Then, let me follow up 
with something like this at first. Something like yours could be a 
fallback option for other good reasons, otherwise.

Thanks,
Chang

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/xstate.c#n386

Re: [PATCH v3 01/13] x86/fpu/xstate: Avoid getting xstate address of init_fpstate if fpstate contains the component

[Mingwei Zhang]

On Fri, Feb 24, 2023, Chang S. Bae wrote:
> On 2/24/2023 3:56 PM, Mingwei Zhang wrote:
> > On Wed, Feb 22, 2023, Chang S. Bae wrote:
> > > 
> > >          /*
> > > -        * The ptrace buffer is in non-compacted XSAVE format.  In
> > > -        * non-compacted format disabled features still occupy state space,
> > > -        * but there is no state to copy from in the compacted
> > > -        * init_fpstate. The gap tracking will zero these states.
> > > +        * Indicate which states to copy from fpstate. When not present in
> > > +        * fpstate, those extended states are either initialized or
> > > +        * disabled. They are also known to have an all zeros init state.
> > > +        * Thus, remove them from 'mask' to zero those features in the user
> > > +        * buffer instead of retrieving them from init_fpstate.
> > >           */
> > > -       mask = fpstate->user_xfeatures;
> > 
> > Do we need to change this line and the comments? I don't see any of
> > these was relevant to this issue. The original code semantic is to
> > traverse all user_xfeatures, if it is available in fpstate, copy it from
> > there; otherwise, copy it from init_fpstate. We do not assume the
> > component in init_fpstate (but not in fpstate) are all zeros, do we? If
> > it is safe to assume that, then it might be ok. But at least in this
> > patch, I want to keep the original semantics as is without the
> > assumption.
> 
> Here it has [1]:
> 
> 	 *
> 	 * XSAVE could be used, but that would require to reshuffle the
> 	 * data when XSAVEC/S is available because XSAVEC/S uses xstate
> 	 * compaction. But doing so is a pointless exercise because most
> 	 * components have an all zeros init state except for the legacy
> 	 * ones (FP and SSE). Those can be saved with FXSAVE into the
> 	 * legacy area. Adding new features requires to ensure that init
> 	 * state is all zeroes or if not to add the necessary handling
> 	 * here.
> 	 */
> 	fxsave(&init_fpstate.regs.fxsave);

ah, I see.
> 
> Thus, init_fpstate has zeros for those extended states. Then, copying from
> init_fpstate is the same as membuf_zero() by the gap tracking. But, we have
> two ways to do the same thing here.
> 
> So I think it works that simply copying the state from fpstate only for
> those present there, then letting the gap tracking zero out for the rest of
> the userspace buffer for features that are either disabled or initialized.
> 
> Then, we can remove accessing init_fpstate in the copy loop and which is the
> source of the problem. So I think this line change is relevant and also
> makes the code simple.
> 
> I guess I'm fine if you don't want to do this. Then, let me follow up with
> something like this at first. Something like yours could be a fallback
> option for other good reasons, otherwise.

hmm. I see. But this is still because of the software implementation.
What if there is a new hardware component that requires a non-zero init
state.

For instance, in the past, we had PKRU component, whose init value is
0x555...54. Of course, that is a bad example because now we kick it out
of the XSAVE/XRSTOR and special handling that, but there is no guarantee
that in the future we will never need a non-zero init state.

So, I will send out my fix and let you, Thomas and potentially other
folks to decide what is the best option. Overall, I get your point.

Thanks
-Mingwei
>
> Thanks,
> Chang
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/fpu/xstate.c#n386
> 
> 

Re: [PATCH v3 01/13] x86/fpu/xstate: Avoid getting xstate address of init_fpstate if fpstate contains the component

[Chang S. Bae]

On 2/24/2023 5:09 PM, Mingwei Zhang wrote:
> On Fri, Feb 24, 2023, Chang S. Bae wrote:
>> On 2/24/2023 3:56 PM, Mingwei Zhang wrote:
>>> On Wed, Feb 22, 2023, Chang S. Bae wrote:
>>>>
>>>>           /*
>>>> -        * The ptrace buffer is in non-compacted XSAVE format.  In
>>>> -        * non-compacted format disabled features still occupy state space,
>>>> -        * but there is no state to copy from in the compacted
>>>> -        * init_fpstate. The gap tracking will zero these states.
>>>> +        * Indicate which states to copy from fpstate. When not present in
>>>> +        * fpstate, those extended states are either initialized or
>>>> +        * disabled. They are also known to have an all zeros init state.
>>>> +        * Thus, remove them from 'mask' to zero those features in the user
>>>> +        * buffer instead of retrieving them from init_fpstate.
>>>>            */
>>>> -       mask = fpstate->user_xfeatures;
>>>
>>> Do we need to change this line and the comments? I don't see any of
>>> these was relevant to this issue. The original code semantic is to
>>> traverse all user_xfeatures, if it is available in fpstate, copy it from
>>> there; otherwise, copy it from init_fpstate. We do not assume the
>>> component in init_fpstate (but not in fpstate) are all zeros, do we? If
>>> it is safe to assume that, then it might be ok. But at least in this
>>> patch, I want to keep the original semantics as is without the
>>> assumption.
>>
>> Here it has [1]:
>>
>> 	 *
>> 	 * XSAVE could be used, but that would require to reshuffle the
>> 	 * data when XSAVEC/S is available because XSAVEC/S uses xstate
>> 	 * compaction. But doing so is a pointless exercise because most
>> 	 * components have an all zeros init state except for the legacy
>> 	 * ones (FP and SSE). Those can be saved with FXSAVE into the
>> 	 * legacy area. Adding new features requires to ensure that init
>> 	 * state is all zeroes or if not to add the necessary handling
>> 	 * here.
>> 	 */
>> 	fxsave(&init_fpstate.regs.fxsave);
> 
> ah, I see.
>>
>> Thus, init_fpstate has zeros for those extended states. Then, copying from
>> init_fpstate is the same as membuf_zero() by the gap tracking. But, we have
>> two ways to do the same thing here.
>>
>> So I think it works that simply copying the state from fpstate only for
>> those present there, then letting the gap tracking zero out for the rest of
>> the userspace buffer for features that are either disabled or initialized.
>>
>> Then, we can remove accessing init_fpstate in the copy loop and which is the
>> source of the problem. So I think this line change is relevant and also
>> makes the code simple.
>>
>> I guess I'm fine if you don't want to do this. Then, let me follow up with
>> something like this at first. Something like yours could be a fallback
>> option for other good reasons, otherwise.
> 
> hmm. I see. But this is still because of the software implementation.
> What if there is a new hardware component that requires a non-zero init
> state.
> 
> For instance, in the past, we had PKRU component, whose init value is
> 0x555...54. Of course, that is a bad example because now we kick it out
> of the XSAVE/XRSTOR and special handling that, but there is no guarantee
> that in the future we will never need a non-zero init state.

Yeah, then that feature enabling has to update [1] to record the special 
init value there. So one who writes that code has to responsibly check 
what has to be adjusted like for other feature enabling in general.

Now we have established the dynamic states which are also assumed to 
have an all-zeros init state. Anything new state located after that 
should be a dynamic state because of the sigaltstack.

Of course, with that though, I don't know whether we will see anything 
with a non-zero init state in the future or not.

> So, I will send out my fix and let you, Thomas and potentially other
> folks to decide what is the best option. Overall, I get your point.

Let's coordinate this. We shouldn't throw multiple versions at the same 
time. (Let me follow up with you.)

Thanks,
Chang

Re: [PATCH v3 02/13] KVM: selftests: x86: Add a working xstate data structure

[Sean Christopherson]

Please omit the "x86:" from the shortlog.  I'm not necessarily against capturing
the arch somewhere in the shortlog for KVM selftests changes, but I want the
community as a whole to make a concious decision on if and how to do it.  I.e. I
don't want a bunch of ad hoc versions popping up because we'll end up with a mess.
E.g. I personally think three levels of scope is too much, and would rather do
something like "KVM: x86/selftests:" or "KVM: selftests/x86:".

Re: [PATCH v3 05/13] KVM: selftests: x86: Add check of CR0.TS in the #NM handler in amx_test

[Sean Christopherson]

On Tue, Feb 21, 2023, Mingwei Zhang wrote:
> Add check of CR0.TS[bit 3] before the check of IA32_XFD_ERR in the #NM
> handler in amx_test. This is because XFD may not be the only reason of
> the IA32_XFD MSR and the bitmap corresponding to the state components
> required by the faulting instruction." (Intel SDM vol 1. Section 13.14)
> 
> Add the missing check of CR0.TS.

The check is not missing.  CR0.TS is never set in selftests, i.e. this is pure
paranoia.  I've no objection to adding the paranoid check, but it should not presented
as a "flaw" in the existing code.

Re: [PATCH v3 06/13] KVM: selftests: x86: Add the XFD check to IA32_XFD in #NM handler

[Sean Christopherson]

On Tue, Feb 21, 2023, Mingwei Zhang wrote:
> Add an extra check to IA32_XFD to ensure the behavior is consistent with
> the AMX archtecture. In addition, repeat the checks across context switch
> to ensure the values of IA32_XFD and IA32_XFD_ERR are well preserved.
> 
> Signed-off-by: Mingwei Zhang <mizhang@google.com>
> ---
>  tools/testing/selftests/kvm/x86_64/amx_test.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
> index ac49b14460b6..296c954dfd6d 100644
> --- a/tools/testing/selftests/kvm/x86_64/amx_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
> @@ -218,8 +218,10 @@ void guest_nm_handler(struct ex_regs *regs)
>  	GUEST_SYNC(7);
>  	GUEST_ASSERT((get_cr0() & X86_CR0_TS) == 0);
>  	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
> +	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) & XFEATURE_MASK_XTILEDATA);

These should use ==, not &.  The test explicitly writes MSR_IA32_XFD, i.e. if
there are extra bits set then something is buggy.

Re: [PATCH v3 07/13] KVM: selftests: x86: Fix the checks to XFD_ERR using and operation

[Sean Christopherson]

On Tue, Feb 21, 2023, Mingwei Zhang wrote:
> Fix the checks to XFD_ERR using logical AND operation because XFD_ERR might
> contain more information in the future. According Intel SDM Vol 1. 13.14:

If that happens, then the future change is responsible for updating the check.
The test very clearly does a straight write of MSR_IA32_XFD.  If there are extra
bits set then something is broken.

	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA);

Re: [PATCH v3 11/13] KVM: selftests: x86: Remove redundant check that XSAVE is supported

[Sean Christopherson]

On Tue, Feb 21, 2023, Mingwei Zhang wrote:
> From: Aaron Lewis <aaronlewis@google.com>
> 
> In amx_test, userspace requires that XSAVE is supported before running
> the test, then the guest checks that it is supported after enabling
> AMX.  Remove the redundant check in the guest that XSAVE is supported.

It's a bit paranoid, but I actually don't mind the extra check.  It's not redundant
per se, just useless in its current location.  If the check is moved _before_
CR4 is set, then it actually provides value, e.g. if something does go sideways,
will fire an assert instead of getting a #GP on set_cr4().

Re: [PATCH v3 00/13] Overhauling amx_test

[Sean Christopherson]

On Tue, 21 Feb 2023 16:36:42 +0000, Mingwei Zhang wrote:
> In this version, I have integrated Aaron's changes to the amx_test. In
> addition, we also integrated one fix patch for a kernel warning due to
> xsave address issue.
> 
> Patch 1:
> Fix a host FPU kernel warning due to missing XTILEDATA in xinit.
> 
> [...]

Applied everything except patch 7 to kvm-x86 selftests.  Please holler if I
missed something subtle about patch 7 (using & vs. ==).  This is at the head
of kvm-x86/selftests, i.e. I can fix it up if necessary.

[01/13] x86/fpu/xstate: Avoid getting xstate address of init_fpstate if fpstate contains the component
        (no commit info)
[02/13] KVM: selftests: x86: Add a working xstate data structure
        https://github.com/kvm-x86/linux/commit/03e8ddff78ac
[03/13] KVM: selftests: x86: Fix an error in comment of amx_test
        https://github.com/kvm-x86/linux/commit/c23d3b210baf
[04/13] KVM: selftests: x86: Enable checking on xcomp_bv in amx_test
        https://github.com/kvm-x86/linux/commit/1e2de0651567
[05/13] KVM: selftests: x86: Add check of CR0.TS in the #NM handler in amx_test
        https://github.com/kvm-x86/linux/commit/04f5d4539105
[06/13] KVM: selftests: x86: Add the XFD check to IA32_XFD in #NM handler
        https://github.com/kvm-x86/linux/commit/eeae141ddb54

[08/13] KVM: selftests: x86: Repeat the checking of xheader when IA32_XFD[XTILEDATA] is set in amx_test
        https://github.com/kvm-x86/linux/commit/cabed3f958e9
[09/13] KVM: selftests: x86: Assert that XTILE is XSAVE-enabled
        https://github.com/kvm-x86/linux/commit/44217830267d
[10/13] KVM: selftests: x86: Assert that both XTILE{CFG,DATA} are XSAVE-enabled
        https://github.com/kvm-x86/linux/commit/7b328195c462
[11/13] KVM: selftests: x86: Remove redundant check that XSAVE is supported
        https://github.com/kvm-x86/linux/commit/08f63d826980
[12/13] KVM: selftests: x86: Check that the palette table exists before using it
        https://github.com/kvm-x86/linux/commit/7042ef575496
[13/13] KVM: selftests: x86: Check that XTILEDATA supports XFD
        https://github.com/kvm-x86/linux/commit/564435d346ff

--
https://github.com/kvm-x86/linux/tree/next
https://github.com/kvm-x86/linux/tree/fixes

Re: [PATCH v3 00/13] Overhauling amx_test

[Sean Christopherson]

On Fri, Mar 24, 2023, Sean Christopherson wrote:
> On Tue, 21 Feb 2023 16:36:42 +0000, Mingwei Zhang wrote:
> > In this version, I have integrated Aaron's changes to the amx_test. In
> > addition, we also integrated one fix patch for a kernel warning due to
> > xsave address issue.
> > 
> > Patch 1:
> > Fix a host FPU kernel warning due to missing XTILEDATA in xinit.
> > 
> > [...]
> 
> Applied everything except patch 7 to kvm-x86 selftests.  Please holler if I
> missed something subtle about patch 7 (using & vs. ==).  This is at the head
> of kvm-x86/selftests, i.e. I can fix it up if necessary.
> 
> [01/13] x86/fpu/xstate: Avoid getting xstate address of init_fpstate if fpstate contains the component
>         (no commit info)

*sigh*  And by "everything" I meant "all of the selftests patches".

Re: [PATCH v3 00/13] Overhauling amx_test

[Sean Christopherson]

On Fri, Mar 24, 2023, Sean Christopherson wrote:
> On Fri, Mar 24, 2023, Sean Christopherson wrote:
> > On Tue, 21 Feb 2023 16:36:42 +0000, Mingwei Zhang wrote:
> > > In this version, I have integrated Aaron's changes to the amx_test. In
> > > addition, we also integrated one fix patch for a kernel warning due to
> > > xsave address issue.
> > > 
> > > Patch 1:
> > > Fix a host FPU kernel warning due to missing XTILEDATA in xinit.
> > > 
> > > [...]
> > 
> > Applied everything except patch 7 to kvm-x86 selftests.  Please holler if I
> > missed something subtle about patch 7 (using & vs. ==).  This is at the head
> > of kvm-x86/selftests, i.e. I can fix it up if necessary.
> > 
> > [01/13] x86/fpu/xstate: Avoid getting xstate address of init_fpstate if fpstate contains the component
> >         (no commit info)
> 
> *sigh*  And by "everything" I meant "all of the selftests patches".

Continuing my circus of goofs, I already force pushed selftests due to an unrelated
mixup.  New hashes below (the comment above still stands in case another overwrite
is necessary).

       [1/11] KVM: selftests: Add a fully functional "struct xstate" for x86
             https://github.com/kvm-x86/linux/commit/5de4a3765b7e
       [2/11] KVM: selftests: Fix an error in comment of amx_test
             https://github.com/kvm-x86/linux/commit/bec357a4af55
       [3/11] KVM: selftests: Enable checking on xcomp_bv in amx_test
             https://github.com/kvm-x86/linux/commit/48ad4222c43c
       [4/11] KVM: selftests: Add check of CR0.TS in the #NM handler in amx_test
             https://github.com/kvm-x86/linux/commit/0aeb9729486a
       [5/11] KVM: selftests: Assert that XTILE_DATA is set in IA32_XFD on #NM
             https://github.com/kvm-x86/linux/commit/9cbd9aaa670f
       [6/11] KVM: selftests: Verify XTILE_DATA in XSTATE isn't affected by IA32_XFD
             https://github.com/kvm-x86/linux/commit/bfc5afc37c9d
       [7/11] KVM: selftests: Assert that XTILE is XSAVE-enabled
             https://github.com/kvm-x86/linux/commit/7e1075f05078
       [8/11] KVM: selftests: Assert that both XTILE{CFG,DATA} are XSAVE-enabled
             https://github.com/kvm-x86/linux/commit/2ab3991b0b9b
       [9/11] KVM: selftests: Move XSAVE and OSXSAVE CPUID checks into AMX's init_regs()
             https://github.com/kvm-x86/linux/commit/d01d4a4f7bd2
       [10/11] KVM: selftests: Check that the palette table exists before using it
             https://github.com/kvm-x86/linux/commit/d32fb0714293
       [11/11] KVM: selftests: Check that XTILEDATA supports XFD
             https://github.com/kvm-x86/linux/commit/d563164eaeb1