[PATCH 0/4] Add extra checkings to amx_test

[PATCH 0/4] Add extra checkings to amx_test

[Mingwei Zhang]

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

-- 
2.39.0.314.g84b9a713c41-goog

[PATCH 1/4] 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.

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);
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH 2/4] KVM: selftests: x86: Add check of IA32_XFD in amx_test

[Mingwei Zhang]

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 */
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH 3/4] 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 | 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);
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH 4/4] KVM: selftests: x86: Repeat the checking of xheader when IA32_XFD[18] is set in amx_test

[Mingwei Zhang]

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);
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH 1/4] KVM: selftests: x86: Fix an error in comment of amx_test

[Sean Christopherson]

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
> 

Re: [PATCH 2/4] KVM: selftests: x86: Add check of IA32_XFD in amx_test

[Sean Christopherson]

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
> 

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

[Sean Christopherson]

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

Re: [PATCH 4/4] KVM: selftests: x86: Repeat the checking of xheader when IA32_XFD[18] is set in amx_test

[Sean Christopherson]

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
> 

Re: [PATCH 1/4] KVM: selftests: x86: Fix an error in comment of amx_test

[Mingwei Zhang]

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