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