Re: [PATCH 3/4] KVM: selftests: x86: Enable checking on xcomp_bv in amx_test

[Sean Christopherson <seanjc@google.com>]

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