AMX architecture involves several entities such as xstate, XCR0, IA32_XFD. This series add several missing checks on top of the existing amx_test. In particular, the 1st commit just fixes a typo in comment. The 2nd and 4th commit focus on the properties of IA32_XFD/IA32_XFD_ERR when interating with xsavec and #NM, while the 3rd commit adds the checking of xcomp_bv in xstate.
Mingwei Zhang (4): KVM: selftests: x86: Fix an error in comment of amx_test KVM: selftests: x86: Add check of IA32_XFD in amx_test KVM: selftests: x86: Enable checking on xcomp_bv in amx_test KVM: selftests: x86: Repeat the checking of xheader when IA32_XFD[18] is set in amx_test
tools/testing/selftests/kvm/x86_64/amx_test.c | 43 +++++++++++++++++-- 1 file changed, 40 insertions(+), 3 deletions(-)
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.
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 | 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 bd72c6eb3b67..16533949a189 100644 --- a/tools/testing/selftests/kvm/x86_64/amx_test.c +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c @@ -204,7 +204,7 @@ 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() */ + /* bit 18 not in the XSTATE_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);
On Tue, Jan 10, 2023, Mingwei Zhang wrote:
After the execution of __tilerelease(), AMX component will be in INIT state. Therefore, execution of xsavec saving the AMX state into memory will
s/xsavec/XSAVEC
cause the XSTATE_BV[18] cleared in xheader. However, the XCOMP_BV[18] will remain set. Fix the error in comment.
Cc: Jim Mattson jmattson@google.com Cc: Venkatesh Srinivas venkateshs@google.com Cc: Aaron Lewis aaronlewis@google.com
No need for a blank line.
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 bd72c6eb3b67..16533949a189 100644 --- a/tools/testing/selftests/kvm/x86_64/amx_test.c +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c @@ -204,7 +204,7 @@ 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() */
- /* bit 18 not in the XSTATE_BV after xsavec() */
I would rather overhaul the entire comment, e.g.
/* Verify XTILEDATA is not set in XSTATE_BV after XSAVEC */
I had to look at the definition of XFEATURE_XTILEDATA to verify that yes, indeed, bit 18 is XTILEDATA.
As for xsavec() vs. XSAVE, IIUC the clearing of XCOMP_BV[18] is a side effect of XSAVEC the instruction, not something extra done by xsavec() the function.
set_xstatebv(xsave_data, XFEATURE_MASK_XTILEDATA); __xsavec(xsave_data, XFEATURE_MASK_XTILEDATA); GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILEDATA) == 0); -- 2.39.0.314.g84b9a713c41-goog
On Tue, Feb 7, 2023 at 5:13 PM Sean Christopherson seanjc@google.com wrote:
On Tue, Jan 10, 2023, Mingwei Zhang wrote:
After the execution of __tilerelease(), AMX component will be in INIT state. Therefore, execution of xsavec saving the AMX state into memory will
s/xsavec/XSAVEC
cause the XSTATE_BV[18] cleared in xheader. However, the XCOMP_BV[18] will remain set. Fix the error in comment.
Cc: Jim Mattson jmattson@google.com Cc: Venkatesh Srinivas venkateshs@google.com Cc: Aaron Lewis aaronlewis@google.com
No need for a blank line.
ack.
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 bd72c6eb3b67..16533949a189 100644 --- a/tools/testing/selftests/kvm/x86_64/amx_test.c +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c @@ -204,7 +204,7 @@ 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() */
/* bit 18 not in the XSTATE_BV after xsavec() */I would rather overhaul the entire comment, e.g.
/* Verify XTILEDATA is not set in XSTATE_BV after XSAVEC */I had to look at the definition of XFEATURE_XTILEDATA to verify that yes, indeed, bit 18 is XTILEDATA.
As for xsavec() vs. XSAVE, IIUC the clearing of XCOMP_BV[18] is a side effect of XSAVEC the instruction, not something extra done by xsavec() the function.
I see, that's why you asked me to use capital words.
set_xstatebv(xsave_data, XFEATURE_MASK_XTILEDATA); __xsavec(xsave_data, XFEATURE_MASK_XTILEDATA); GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILEDATA) == 0);-- 2.39.0.314.g84b9a713c41-goog
When #NM is triggered, the handler needs to ensure the exception is triggered by AMX by checking IA32_XFD_ERR and not because of CR0.TS[bit 3] is 1. Note that the value of IA32_XFD_ERR comes from "the logical AND 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 before checking the value of IA32_XFD_ERR. In addition, 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 | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c index 16533949a189..b2369f956fea 100644 --- a/tools/testing/selftests/kvm/x86_64/amx_test.c +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c @@ -226,9 +226,12 @@ 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) & XFEATURE_MASK_XTILEDATA) == 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) == XFEATURE_MASK_XTILEDATA); /* Clear xfd_err */ wrmsr(MSR_IA32_XFD_ERR, 0); /* xfd=0, enable amx */
On Tue, Jan 10, 2023, Mingwei Zhang wrote:
When #NM is triggered, the handler needs to ensure the exception is
State what the patch does (and explain why), don't say ABC needs/should do XYZ. The #NM handler doesn't _need_ to ensure the #NM wasn't due to CR0.TS
triggered by AMX by checking IA32_XFD_ERR and not because of CR0.TS[bit 3]
CR0.TS is a single bit, using square braces makes it look like an index into CR0.TS. I would drop the "bit 3" part altogether, it's not relevant
is 1. Note that the value of IA32_XFD_ERR comes from "the logical AND 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 before checking the value of IA32_XFD_ERR. In addition, 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.
Split the MSR_IA32_XFD checks to a separate patch. Or I guess given the shortlog is about IA32_XFD, split the CR0.TS check to a separate patch.
Signed-off-by: Mingwei Zhang mizhang@google.com
tools/testing/selftests/kvm/x86_64/amx_test.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c index 16533949a189..b2369f956fea 100644 --- a/tools/testing/selftests/kvm/x86_64/amx_test.c +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c @@ -226,9 +226,12 @@ 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(!(get_cr0() & X86_CR0_TS));
GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
- GUEST_ASSERT((rdmsr(MSR_IA32_XFD) & XFEATURE_MASK_XTILEDATA) == XFEATURE_MASK_XTILEDATA);
Isn't this just
GUEST_ASSERT(rdmsr(MSR_IA32_XFD) & XFEATURE_MASK_XTILEDATA);
or am I horribly misreading the code?
GUEST_SYNC(8); GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
- GUEST_ASSERT((rdmsr(MSR_IA32_XFD) & XFEATURE_MASK_XTILEDATA) == XFEATURE_MASK_XTILEDATA);
Same here.
/* Clear xfd_err */ wrmsr(MSR_IA32_XFD_ERR, 0); /* xfd=0, enable amx */ -- 2.39.0.314.g84b9a713c41-goog
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 | 30 +++++++++++++++++-- 1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c index b2369f956fea..18203e399e9d 100644 --- a/tools/testing/selftests/kvm/x86_64/amx_test.c +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c @@ -41,6 +41,12 @@
#define XSAVE_HDR_OFFSET 512
+struct xstate_header { + u64 xfeatures; + u64 xcomp_bv; + u64 reserved[6]; +} __packed; + struct xsave_data { u8 area[XSAVE_SIZE]; } __aligned(64); @@ -160,12 +166,26 @@ static void set_tilecfg(struct tile_config *cfg)
static void set_xstatebv(void *data, uint64_t bv) { - *(uint64_t *)(data + XSAVE_HDR_OFFSET) = bv; + struct xstate_header *header = + (struct xstate_header *)(data + XSAVE_HDR_OFFSET); + + header->xfeatures = bv; }
static u64 get_xstatebv(void *data) { - return *(u64 *)(data + XSAVE_HDR_OFFSET); + struct xstate_header *header = + (struct xstate_header *)(data + XSAVE_HDR_OFFSET); + + return header->xfeatures; +} + +static u64 get_xcompbv(void *data) +{ + struct xstate_header *header = + (struct xstate_header *)(data + XSAVE_HDR_OFFSET); + + return header->xcomp_bv; }
static void init_regs(void) @@ -204,10 +224,14 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg, GUEST_SYNC(4); __tilerelease(); GUEST_SYNC(5); - /* bit 18 not in the XSTATE_BV after xsavec() */ + /* + * After xsavec() bit 18 is cleared in the XSTATE_BV but is set in + * the XCOMP_BV. + */ set_xstatebv(xsave_data, XFEATURE_MASK_XTILEDATA); __xsavec(xsave_data, XFEATURE_MASK_XTILEDATA); GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILEDATA) == 0); + GUEST_ASSERT((get_xcompbv(xsave_data) & XFEATURE_MASK_XTILEDATA) == XFEATURE_MASK_XTILEDATA);
/* xfd=0x40000, disable amx tiledata */ wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA);
On Tue, Jan 10, 2023, Mingwei Zhang wrote:
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 | 30 +++++++++++++++++-- 1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c index b2369f956fea..18203e399e9d 100644 --- a/tools/testing/selftests/kvm/x86_64/amx_test.c +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c @@ -41,6 +41,12 @@ #define XSAVE_HDR_OFFSET 512 +struct xstate_header {
- u64 xfeatures;
- u64 xcomp_bv;
- u64 reserved[6];
+} __packed;
I definitely like the idea of using a struct, but let's go all the way, i.e. add a mostly-functional "struct xstate" too so that we don't to do pointer arithmetic. I don't think it makes sense to copy+paste from the kernel since I highly doubt anyone is going to write an x87 test, so maybe this?
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)));
struct xsave_data { u8 area[XSAVE_SIZE];
Ewww. Not your code. The existing code declares XSAVE_SIZE bytes, but allocates 3 4KiB pages. It took me a bit of starting to realize TILE_SIZE is 1KiB, not 4KiB. Can you tack on a patch do something like:
@@ -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, DIV_ROUND_UP(XSAVE_SIZE, PAGE_SIZE)); + vcpu_args_set(vcpu, 3, amx_cfg, tiledata, xstate);
for (stage = 1; ; stage++) { vcpu_run(vcpu);
} __aligned(64); @@ -160,12 +166,26 @@ static void set_tilecfg(struct tile_config *cfg) static void set_xstatebv(void *data, uint64_t bv) {
- *(uint64_t *)(data + XSAVE_HDR_OFFSET) = bv;
- struct xstate_header *header =
(struct xstate_header *)(data + XSAVE_HDR_OFFSET);- header->xfeatures = bv;
} static u64 get_xstatebv(void *data) {
- return *(u64 *)(data + XSAVE_HDR_OFFSET);
- struct xstate_header *header =
(struct xstate_header *)(data + XSAVE_HDR_OFFSET);- return header->xfeatures;
+}
+static u64 get_xcompbv(void *data) +{
- struct xstate_header *header =
(struct xstate_header *)(data + XSAVE_HDR_OFFSET);- return header->xcomp_bv;
}
If we add a "full" struct, these ugly wrappers go away, e.g. as untested patches that you can claim as your own if you test 'em and write changelogs :-)
--- .../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 53ffa43c90db..a7ce1fe8d70f 100644 --- a/tools/testing/selftests/kvm/include/x86_64/processor.h +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h @@ -48,6 +48,18 @@ extern bool host_cpu_is_amd; #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..d506821a5a26 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, DIV_ROUND_UP(XSAVE_SIZE, PAGE_SIZE)); + vcpu_args_set(vcpu, 3, amx_cfg, tiledata, xstate);
for (stage = 1; ; stage++) { vcpu_run(vcpu);
base-commit: 78332517a5dab54514ae719805eec218715de1fc
Repeat the checking of AMX component in xheader after xsavec when IA32_XFD[18] 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 XCR[18:17] are set.
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 18203e399e9d..9a80a59b64e6 100644 --- a/tools/testing/selftests/kvm/x86_64/amx_test.c +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c @@ -235,6 +235,16 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
/* xfd=0x40000, disable amx tiledata */ wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA); + + /* + * Bit 18 is cleared in XSTATE_BV but set in XCOMP_BV, this property + * remains the same even when IA32_XFD disables amx tiledata. + */ + set_xstatebv(xsave_data, XFEATURE_MASK_XTILEDATA); + __xsavec(xsave_data, XFEATURE_MASK_XTILEDATA); + GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILEDATA) == 0); + GUEST_ASSERT((get_xcompbv(xsave_data) & XFEATURE_MASK_XTILEDATA) == XFEATURE_MASK_XTILEDATA); + GUEST_SYNC(6); GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILEDATA); set_tilecfg(amx_cfg);
On Tue, Jan 10, 2023, Mingwei Zhang wrote:
Repeat the checking of AMX component in xheader after xsavec when
s/xsavec/XSAVEC. Not sure about xheader, though it seems like that should be capitalized too.
IA32_XFD[18] is set. This check calibrates the functionality scope of
s/18/XTILEDATA
IA32_XFD: it does not intercept the XSAVE state management.
I didn't follow this. Is this basically saying "Verify XTILEDATA is saved by XSAVEC even when disabled via IA32_XFD"?
Regardless of the values in IA32_XFD, AMX component state will still be managed by XSAVE* and XRSTOR* as long as XCR[18:17] are set.
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 18203e399e9d..9a80a59b64e6 100644 --- a/tools/testing/selftests/kvm/x86_64/amx_test.c +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c @@ -235,6 +235,16 @@ static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg, /* xfd=0x40000, disable amx tiledata */ wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA);
- /*
* Bit 18 is cleared in XSTATE_BV but set in XCOMP_BV, this property
s/Bit 18/XTILEDATA
* remains the same even when IA32_XFD disables amx tiledata.
I would phrase this as "even when AMX tile data is disabled via IA32_XFD". Software disables AMX, IA32_XFD is the mechanic by which that is done.
*/- set_xstatebv(xsave_data, XFEATURE_MASK_XTILEDATA);
- __xsavec(xsave_data, XFEATURE_MASK_XTILEDATA);
- GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILEDATA) == 0);
- GUEST_ASSERT((get_xcompbv(xsave_data) & XFEATURE_MASK_XTILEDATA) == XFEATURE_MASK_XTILEDATA);
Same feedback about using !(...) and unnecessary "== x".
- GUEST_SYNC(6); GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILEDATA); set_tilecfg(amx_cfg);
-- 2.39.0.314.g84b9a713c41-goog
linux-kselftest-mirror@lists.linaro.org
-
Mingwei Zhang -
Sean Christopherson