[PATCH 0/3] AMX KVM selftest

[PATCH 0/3] AMX KVM selftest

[Yang Zhong]

Hi Paolo,

Please help review this patchset, which is based on Jing's AMX v2.
https://lore.kernel.org/all/20211217153003.1719189-1-jing2.liu@intel.com/

Hope this patchset will be merged into Jing's v3 for further review. 

About this selftest requirement, please check below link:
https://lore.kernel.org/all/85401305-2c71-e57f-a01e-4850060d300a@redhat.com/

By the way, this amx_test.c file referenced Chang's older test code:
https://lore.kernel.org/lkml/20210221185637.19281-21-chang.seok.bae@intel.com/


Thanks!

Yang



Paolo Bonzini (1):
  selftest: kvm: Reorder vcpu_load_state steps for AMX

Yang Zhong (2):
  selftest: Move struct kvm_x86_state to header
  selftest: Support amx selftest

 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/include/x86_64/processor.h  |  16 +-
 .../selftests/kvm/lib/x86_64/processor.c      |  44 +--
 tools/testing/selftests/kvm/x86_64/amx_test.c | 370 ++++++++++++++++++
 4 files changed, 401 insertions(+), 30 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/amx_test.c

[PATCH 1/3] selftest: kvm: Reorder vcpu_load_state steps for AMX

[Yang Zhong]

From: Paolo Bonzini <pbonzini@redhat.com>

For AMX support it is recommended to load XCR0 after XFD, so
that KVM does not see XFD=0, XCR=1 for a save state that will
eventually be disabled (which would lead to premature allocation
of the space required for that save state).

It is also required to load XSAVE data after XCR0 and XFD, so
that KVM can trigger allocation of the extra space required to
store AMX state.

Adjust vcpu_load_state to obey these new requirements.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 .../selftests/kvm/lib/x86_64/processor.c      | 29 ++++++++++---------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 00324d73c687..9b5abf488211 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1192,9 +1192,14 @@ void vcpu_load_state(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_x86_state *s
 	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
 	int r;
 
-	r = ioctl(vcpu->fd, KVM_SET_XSAVE, &state->xsave);
-        TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_XSAVE, r: %i",
-                r);
+	r = ioctl(vcpu->fd, KVM_SET_SREGS, &state->sregs);
+	TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_SREGS, r: %i",
+		r);
+
+	r = ioctl(vcpu->fd, KVM_SET_MSRS, &state->msrs);
+	TEST_ASSERT(r == state->msrs.nmsrs,
+		"Unexpected result from KVM_SET_MSRS,r: %i (failed at %x)",
+		r, r == state->msrs.nmsrs ? -1 : state->msrs.entries[r].index);
 
 	if (kvm_check_cap(KVM_CAP_XCRS)) {
 		r = ioctl(vcpu->fd, KVM_SET_XCRS, &state->xcrs);
@@ -1202,17 +1207,13 @@ void vcpu_load_state(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_x86_state *s
 			    r);
 	}
 
-	r = ioctl(vcpu->fd, KVM_SET_SREGS, &state->sregs);
-        TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_SREGS, r: %i",
-                r);
-
-	r = ioctl(vcpu->fd, KVM_SET_MSRS, &state->msrs);
-        TEST_ASSERT(r == state->msrs.nmsrs, "Unexpected result from KVM_SET_MSRS, r: %i (failed at %x)",
-                r, r == state->msrs.nmsrs ? -1 : state->msrs.entries[r].index);
+	r = ioctl(vcpu->fd, KVM_SET_XSAVE, &state->xsave);
+	TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_XSAVE, r: %i",
+		r);
 
 	r = ioctl(vcpu->fd, KVM_SET_VCPU_EVENTS, &state->events);
-        TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_VCPU_EVENTS, r: %i",
-                r);
+	TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_VCPU_EVENTS, r: %i",
+		r);
 
 	r = ioctl(vcpu->fd, KVM_SET_MP_STATE, &state->mp_state);
         TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_MP_STATE, r: %i",
@@ -1223,8 +1224,8 @@ void vcpu_load_state(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_x86_state *s
                 r);
 
 	r = ioctl(vcpu->fd, KVM_SET_REGS, &state->regs);
-        TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_REGS, r: %i",
-                r);
+	TEST_ASSERT(r == 0, "Unexpected result from KVM_SET_REGS, r: %i",
+		r);
 
 	if (state->nested.size) {
 		r = ioctl(vcpu->fd, KVM_SET_NESTED_STATE, &state->nested);

[PATCH 2/3] selftest: Move struct kvm_x86_state to header

[Yang Zhong]

Those changes can avoid dereferencing pointer compile issue
when amx_test.c reference state->xsave.

Move struct kvm_x86_state definition to processor.h.

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 .../selftests/kvm/include/x86_64/processor.h     | 16 +++++++++++++++-
 .../testing/selftests/kvm/lib/x86_64/processor.c | 15 ---------------
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 0546173ab628..28f8fa78a47b 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -94,6 +94,21 @@ struct desc_ptr {
 	uint64_t address;
 } __attribute__((packed));
 
+struct kvm_x86_state {
+	struct kvm_xsave *xsave;
+	struct kvm_vcpu_events events;
+	struct kvm_mp_state mp_state;
+	struct kvm_regs regs;
+	struct kvm_xcrs xcrs;
+	struct kvm_sregs sregs;
+	struct kvm_debugregs debugregs;
+	union {
+		struct kvm_nested_state nested;
+		char nested_[16384];
+	};
+	struct kvm_msrs msrs;
+};
+
 static inline uint64_t get_desc64_base(const struct desc64 *desc)
 {
 	return ((uint64_t)desc->base3 << 32) |
@@ -350,7 +365,6 @@ static inline unsigned long get_xmm(int n)
 
 bool is_intel_cpu(void);
 
-struct kvm_x86_state;
 struct kvm_x86_state *vcpu_save_state(struct kvm_vm *vm, uint32_t vcpuid);
 void vcpu_load_state(struct kvm_vm *vm, uint32_t vcpuid,
 		     struct kvm_x86_state *state);
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 9b5abf488211..126f8e743bc2 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -1036,21 +1036,6 @@ void vcpu_dump(FILE *stream, struct kvm_vm *vm, uint32_t vcpuid, uint8_t indent)
 	sregs_dump(stream, &sregs, indent + 4);
 }
 
-struct kvm_x86_state {
-	struct kvm_xsave *xsave;
-	struct kvm_vcpu_events events;
-	struct kvm_mp_state mp_state;
-	struct kvm_regs regs;
-	struct kvm_xcrs xcrs;
-	struct kvm_sregs sregs;
-	struct kvm_debugregs debugregs;
-	union {
-		struct kvm_nested_state nested;
-		char nested_[16384];
-	};
-	struct kvm_msrs msrs;
-};
-
 static int kvm_get_num_msrs_fd(int kvm_fd)
 {
 	struct kvm_msr_list nmsrs;

[PATCH 3/3] selftest: Support amx selftest

[Yang Zhong]

This selftest do two test cases, one is to trigger #NM
exception and check MSR XFD_ERR value. Another case is
guest load tile data into tmm0 registers and trap to host
side to check memory data after save/restore.

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 tools/testing/selftests/kvm/x86_64/amx_test.c | 370 ++++++++++++++++++
 2 files changed, 371 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/amx_test.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index c4e34717826a..ecb1fe022111 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -75,6 +75,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/xen_shinfo_test
 TEST_GEN_PROGS_x86_64 += x86_64/xen_vmcall_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_pi_mmio_test
 TEST_GEN_PROGS_x86_64 += x86_64/sev_migrate_tests
+TEST_GEN_PROGS_x86_64 += x86_64/amx_test
 TEST_GEN_PROGS_x86_64 += demand_paging_test
 TEST_GEN_PROGS_x86_64 += dirty_log_test
 TEST_GEN_PROGS_x86_64 += dirty_log_perf_test
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
new file mode 100644
index 000000000000..76ae44622923
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -0,0 +1,370 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * amx tests
+ *
+ * Copyright (C) 2021, Intel, Inc.
+ *
+ * Tests for amx #NM exception and save/restore.
+ */
+
+#define _GNU_SOURCE /* for program_invocation_short_name */
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/syscall.h>
+
+#include "test_util.h"
+
+#include "kvm_util.h"
+#include "processor.h"
+#include "vmx.h"
+
+#ifndef __x86_64__
+# error This test is 64-bit only
+#endif
+
+#define VCPU_ID				0
+#define X86_FEATURE_XSAVE		(1 << 26)
+#define X86_FEATURE_OSXSAVE		(1 << 27)
+
+#define PAGE_SIZE			(1 << 12)
+#define NUM_TILES			8
+#define TILE_SIZE			1024
+#define XSAVE_SIZE			((NUM_TILES * TILE_SIZE) + PAGE_SIZE)
+
+/* Tile configuration associated: */
+#define MAX_TILES			16
+#define RESERVED_BYTES			14
+
+#define XFEATURE_XTILECFG		17
+#define XFEATURE_XTILEDATA		18
+#define XFEATURE_MASK_XTILECFG		(1 << XFEATURE_XTILECFG)
+#define XFEATURE_MASK_XTILEDATA		(1 << XFEATURE_XTILEDATA)
+#define XFEATURE_MASK_XTILE		(XFEATURE_MASK_XTILECFG | XFEATURE_MASK_XTILEDATA)
+
+#define TILE_CPUID			0x1d
+#define XSTATE_CPUID			0xd
+#define TILE_PALETTE_CPUID_SUBLEAVE	0x1
+#define XSTATE_USER_STATE_SUBLEAVE	0x0
+
+struct tile_config {
+	u8  palette_id;
+	u8  start_row;
+	u8  reserved[RESERVED_BYTES];
+	u16 colsb[MAX_TILES];
+	u8  rows[MAX_TILES];
+};
+
+struct tile_data {
+	u8 data[NUM_TILES * TILE_SIZE];
+};
+
+struct xtile_info {
+	u16 bytes_per_tile;
+	u16 bytes_per_row;
+	u16 max_names;
+	u16 max_rows;
+	u32 xsave_offset;
+	u32 xsave_size;
+};
+
+static struct xtile_info xtile;
+
+static inline u64 __xgetbv(u32 index)
+{
+	u32 eax, edx;
+
+	asm volatile("xgetbv;"
+		     : "=a" (eax), "=d" (edx)
+		     : "c" (index));
+	return eax + ((u64)edx << 32);
+}
+
+static inline void __xsetbv(u32 index, u64 value)
+{
+	u32 eax = value;
+	u32 edx = value >> 32;
+
+	asm volatile("xsetbv" :: "a" (eax), "d" (edx), "c" (index));
+}
+
+static inline void __ldtilecfg(void *cfg)
+{
+	asm volatile(".byte 0xc4,0xe2,0x78,0x49,0x00"
+		     : : "a"(cfg));
+}
+
+static inline void __tileloadd(void *tile)
+{
+	asm volatile(".byte 0xc4,0xe2,0x7b,0x4b,0x04,0x10"
+		     : : "a"(tile), "d"(0));
+}
+
+static inline void __tilerelease(void)
+{
+	asm volatile(".byte 0xc4, 0xe2, 0x78, 0x49, 0xc0" ::);
+}
+
+static inline void check_cpuid_xsave(void)
+{
+	uint32_t eax, ebx, ecx, edx;
+
+	eax = 1;
+	ecx = 0;
+	cpuid(&eax, &ebx, &ecx, &edx);
+	if (!(ecx & X86_FEATURE_XSAVE))
+		GUEST_ASSERT(!"cpuid: no CPU xsave support!");
+	if (!(ecx & X86_FEATURE_OSXSAVE))
+		GUEST_ASSERT(!"cpuid: no OS xsave support!");
+}
+
+static bool check_xsave_supports_xtile(void)
+{
+	return __xgetbv(0) & XFEATURE_MASK_XTILE;
+}
+
+static bool enum_xtile_config(void)
+{
+	u32 eax, ebx, ecx, edx;
+
+	eax = TILE_CPUID;
+	ecx = TILE_PALETTE_CPUID_SUBLEAVE;
+
+	cpuid(&eax, &ebx, &ecx, &edx);
+	if (!eax || !ebx || !ecx)
+		return false;
+
+	xtile.max_names = ebx >> 16;
+	if (xtile.max_names < NUM_TILES)
+		return false;
+
+	xtile.bytes_per_tile = eax >> 16;
+	if (xtile.bytes_per_tile < TILE_SIZE)
+		return false;
+
+	xtile.bytes_per_row = ebx;
+	xtile.max_rows = ecx;
+
+	return true;
+}
+
+static bool enum_xsave_tile(void)
+{
+	u32 eax, ebx, ecx, edx;
+
+	eax = XSTATE_CPUID;
+	ecx = XFEATURE_XTILEDATA;
+
+	cpuid(&eax, &ebx, &ecx, &edx);
+	if (!eax || !ebx)
+		return false;
+
+	xtile.xsave_offset = ebx;
+	xtile.xsave_size = eax;
+
+	return true;
+}
+
+static bool check_xsave_size(void)
+{
+	u32 eax, ebx, ecx, edx;
+	bool valid = false;
+
+	eax = XSTATE_CPUID;
+	ecx = XSTATE_USER_STATE_SUBLEAVE;
+
+	cpuid(&eax, &ebx, &ecx, &edx);
+	if (ebx && ebx <= XSAVE_SIZE)
+		valid = true;
+
+	return valid;
+}
+
+static bool check_xtile_info(void)
+{
+	bool ret = false;
+
+	if (!check_xsave_size())
+		return ret;
+
+	if (!enum_xsave_tile())
+		return ret;
+
+	if (!enum_xtile_config())
+		return ret;
+
+	if (sizeof(struct tile_data) >= xtile.xsave_size)
+		ret = true;
+
+	return ret;
+}
+
+static void set_tilecfg(struct tile_config *cfg)
+{
+	int i;
+
+	/* Only palette id 1 */
+	cfg->palette_id = 1;
+	for (i = 0; i < xtile.max_names; i++) {
+		cfg->colsb[i] = xtile.bytes_per_row;
+		cfg->rows[i] = xtile.max_rows;
+	}
+}
+
+static void init_regs(void)
+{
+	uint64_t cr4, xcr0;
+
+	/* turn on CR4.OSXSAVE */
+	cr4 = get_cr4();
+	cr4 |= X86_CR4_OSXSAVE;
+	set_cr4(cr4);
+
+	xcr0 = __xgetbv(0);
+	xcr0 |= XFEATURE_MASK_XTILE;
+	__xsetbv(0x0, xcr0);
+}
+
+static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
+						    struct tile_data *tiledata)
+{
+	init_regs();
+	check_cpuid_xsave();
+	GUEST_ASSERT(check_xsave_supports_xtile());
+	GUEST_ASSERT(check_xtile_info());
+
+	/* check xtile configs */
+	GUEST_ASSERT(xtile.xsave_offset == 2816);
+	GUEST_ASSERT(xtile.xsave_size == 8192);
+	GUEST_ASSERT(xtile.max_names == 8);
+	GUEST_ASSERT(xtile.bytes_per_tile == 1024);
+	GUEST_ASSERT(xtile.bytes_per_row == 64);
+	GUEST_ASSERT(xtile.max_rows == 16);
+
+	/* xfd=0, enable amx */
+	wrmsr(MSR_IA32_XFD, 0);
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == 0);
+	set_tilecfg(amx_cfg);
+	__ldtilecfg(amx_cfg);
+	/* Check save/restore when trap to userspace */
+	__tileloadd(tiledata);
+	GUEST_SYNC(1);
+
+	/* xfd=0x40000, disable amx tiledata */
+	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILEDATA);
+	set_tilecfg(amx_cfg);
+	__ldtilecfg(amx_cfg);
+	/* Trigger #NM exception */
+	__tileloadd(tiledata);
+
+	GUEST_DONE();
+}
+
+void guest_nm_handler(struct ex_regs *regs)
+{
+	/* Check if #NM is triggered by XFEATURE_MASK_XTILEDATA */
+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
+	/* Clear xfd_err */
+	wrmsr(MSR_IA32_XFD_ERR, 0);
+	GUEST_SYNC(2);
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_cpuid_entry2 *entry;
+	struct kvm_regs regs1;
+	bool amx_supported = false;
+	struct kvm_vm *vm;
+	struct kvm_run *run;
+	struct kvm_x86_state *state;
+	int xsave_restore_size = 0;
+	vm_vaddr_t amx_cfg, tiledata;
+	struct ucall uc;
+	u32 amx_offset;
+	int stage, ret;
+
+	/* Create VM */
+	vm = vm_create_default(VCPU_ID, 0, guest_code);
+
+	entry = kvm_get_supported_cpuid_entry(1);
+	if (!(entry->ecx & X86_FEATURE_XSAVE)) {
+		print_skip("XSAVE feature not supported");
+		return 0;
+	}
+
+	if (kvm_get_cpuid_max_basic() >= 0xd) {
+		entry = kvm_get_supported_cpuid_index(0xd, 0);
+		amx_supported = entry && !!(entry->eax & XFEATURE_MASK_XTILE);
+		if (!amx_supported) {
+			print_skip("AMX is not supported by the vCPU and eax=0x%x", entry->eax);
+			return 0;
+		}
+		/* Get xsave/restore max size */
+		xsave_restore_size = entry->ecx;
+	}
+
+	run = vcpu_state(vm, VCPU_ID);
+	vcpu_regs_get(vm, VCPU_ID, &regs1);
+
+	/* Register #NM handler */
+	vm_init_descriptor_tables(vm);
+	vcpu_init_descriptor_tables(vm, VCPU_ID);
+	vm_install_exception_handler(vm, NM_VECTOR, guest_nm_handler);
+
+	/* amx cfg for guest_code */
+	amx_cfg = vm_vaddr_alloc_page(vm);
+	memset(addr_gva2hva(vm, amx_cfg), 0x0, getpagesize());
+
+	/* amx tiledatas for guest_code */
+	tiledata = vm_vaddr_alloc_pages(vm, 2);
+	memset(addr_gva2hva(vm, tiledata), rand() | 1, 2 * getpagesize());
+	vcpu_args_set(vm, VCPU_ID, 2, amx_cfg, tiledata);
+
+	for (stage = 1; ; stage++) {
+		_vcpu_run(vm, VCPU_ID);
+		TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
+			    "Stage %d: unexpected exit reason: %u (%s),\n",
+			    stage, run->exit_reason,
+			    exit_reason_str(run->exit_reason));
+
+		switch (get_ucall(vm, VCPU_ID, &uc)) {
+		case UCALL_ABORT:
+			TEST_FAIL("%s at %s:%ld", (const char *)uc.args[0],
+				  __FILE__, uc.args[1]);
+			/* NOT REACHED */
+		case UCALL_SYNC:
+			switch (uc.args[1]) {
+			case 1:
+				fprintf(stderr,
+					"Exit VM by GUEST_SYNC(1), check save/restore.\n");
+
+				/* Compacted mode, get amx offset by xsave area
+				 * size subtract 8K amx size.
+				 */
+				amx_offset = xsave_restore_size - NUM_TILES*TILE_SIZE;
+				state = vcpu_save_state(vm, VCPU_ID);
+				void *amx_start = (void *)state->xsave + amx_offset;
+				void *tiles_data = (void *)addr_gva2hva(vm, tiledata);
+				/* Only check TMM0 register, 1 tile */
+				ret = memcmp(amx_start, tiles_data, TILE_SIZE);
+				TEST_ASSERT(ret == 0, "memcmp failed, ret=%d\n", ret);
+				kvm_x86_state_cleanup(state);
+				break;
+			case 2:
+				fprintf(stderr,
+					"Exit VM by GUEST_SYNC(2), generate #NM exception.\n");
+				goto done;
+			}
+			break;
+		case UCALL_DONE:
+			goto done;
+		default:
+			TEST_FAIL("Unknown ucall %lu", uc.cmd);
+		}
+	}
+done:
+	kvm_vm_free(vm);
+}

Re: [PATCH 3/3] selftest: Support amx selftest

[Paolo Bonzini]

On 12/22/21 00:15, Yang Zhong wrote:
> This selftest do two test cases, one is to trigger #NM
> exception and check MSR XFD_ERR value. Another case is
> guest load tile data into tmm0 registers and trap to host
> side to check memory data after save/restore.
> 
> Signed-off-by: Yang Zhong <yang.zhong@intel.com>

This is a great start, mainly I'd add a lot more GUEST_SYNCs.

Basically any instruction after the initial GUEST_ASSERTs are a 
potential point for GUEST_SYNC, except right after the call to set_tilecfg:

GUEST_SYNC(1)

> +	/* xfd=0, enable amx */
> +	wrmsr(MSR_IA32_XFD, 0);

GUEST_SYNC(2)

> +	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == 0);
> +	set_tilecfg(amx_cfg);
> +	__ldtilecfg(amx_cfg);

GUEST_SYNC(3)

> +	/* Check save/restore when trap to userspace */
> +	__tileloadd(tiledata);
> +	GUEST_SYNC(1);

This would become 4; here add tilerelease+GUEST_SYNC(5)+XSAVEC, and 
check that state 18 is not included in XCOMP_BV.

> +	/* xfd=0x40000, disable amx tiledata */
> +	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA);

GUEST_SYNC(6)

> +	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILEDATA);
> +	set_tilecfg(amx_cfg);
> +	__ldtilecfg(amx_cfg);
> +	/* Trigger #NM exception */
> +	__tileloadd(tiledata);

GUEST_SYNC(10); this final GUEST_SYNC should also check TMM0 in the host.

> +	GUEST_DONE();
> +}
> +
> +void guest_nm_handler(struct ex_regs *regs)
> +{
> +	/* Check if #NM is triggered by XFEATURE_MASK_XTILEDATA */

GUEST_SYNC(7)

> +	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
> +	/* Clear xfd_err */

Same here, I'd do a GUEST_SYNC(8) and re-read MSR_IA32_XFD_ERR.

> +	wrmsr(MSR_IA32_XFD_ERR, 0);
> +	GUEST_SYNC(2);

This becomes GUEST_SYNC(9).

> +}


> +		case UCALL_SYNC:
> +			switch (uc.args[1]) {
> +			case 1:
> +				fprintf(stderr,
> +					"Exit VM by GUEST_SYNC(1), check save/restore.\n");
> +
> +				/* Compacted mode, get amx offset by xsave area
> +				 * size subtract 8K amx size.
> +				 */
> +				amx_offset = xsave_restore_size - NUM_TILES*TILE_SIZE;
> +				state = vcpu_save_state(vm, VCPU_ID);
> +				void *amx_start = (void *)state->xsave + amx_offset;
> +				void *tiles_data = (void *)addr_gva2hva(vm, tiledata);
> +				/* Only check TMM0 register, 1 tile */
> +				ret = memcmp(amx_start, tiles_data, TILE_SIZE);
> +				TEST_ASSERT(ret == 0, "memcmp failed, ret=%d\n", ret);
> +				kvm_x86_state_cleanup(state);
> +				break;

All GUEST_SYNCs should do save_state/load_state like state_test.c.  Then 
of course you can *also* check TMM0 after __tileloadd, which would be 
cases 4 and 10.

Thanks,

Paolo

> +			case 2:
> +				fprintf(stderr,
> +					"Exit VM by GUEST_SYNC(2), generate #NM exception.\n");
> +				goto done;
> +			}
> +			break;
> +		case UCALL_DONE:
> +			goto done;
> +		default:
> +			TEST_FAIL("Unknown ucall %lu", uc.cmd);
> +		}
> +	}
> +done:
> +	kvm_vm_free(vm);
> +}
> 

Re: [PATCH 3/3] selftest: Support amx selftest

[Yang Zhong]

On Tue, Dec 21, 2021 at 06:36:17PM +0100, Paolo Bonzini wrote:
> On 12/22/21 00:15, Yang Zhong wrote:
> >This selftest do two test cases, one is to trigger #NM
> >exception and check MSR XFD_ERR value. Another case is
> >guest load tile data into tmm0 registers and trap to host
> >side to check memory data after save/restore.
> >
> >Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> 
> This is a great start, mainly I'd add a lot more GUEST_SYNCs.
> 
> Basically any instruction after the initial GUEST_ASSERTs are a
> potential point for GUEST_SYNC, except right after the call to
> set_tilecfg:
> 
> GUEST_SYNC(1)
> 
> >+	/* xfd=0, enable amx */
> >+	wrmsr(MSR_IA32_XFD, 0);
> 
> GUEST_SYNC(2)
> 
> >+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == 0);
> >+	set_tilecfg(amx_cfg);
> >+	__ldtilecfg(amx_cfg);
> 
> GUEST_SYNC(3)
 
   Paolo, with this GUEST_SYNC(3) between __ldtilecfg and __tileloadd
   instructions, the guest code generate (vector:0x6, Invalid Opcode) 
   exception, which is a strange issue. thanks!

   Yang   


> 
> >+	/* Check save/restore when trap to userspace */
> >+	__tileloadd(tiledata);
> >+	GUEST_SYNC(1);
> 
> This would become 4; here add tilerelease+GUEST_SYNC(5)+XSAVEC, and
> check that state 18 is not included in XCOMP_BV.
>
     
   Thanks Paolo, the new code like below:

    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)


    Yang

 
> >+	/* xfd=0x40000, disable amx tiledata */
> >+	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA);
> 
> GUEST_SYNC(6)
> 
> >+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILEDATA);
> >+	set_tilecfg(amx_cfg);
> >+	__ldtilecfg(amx_cfg);
> >+	/* Trigger #NM exception */
> >+	__tileloadd(tiledata);
> 
> GUEST_SYNC(10); this final GUEST_SYNC should also check TMM0 in the host.
> 
> >+	GUEST_DONE();
> >+}
> >+
> >+void guest_nm_handler(struct ex_regs *regs)
> >+{
> >+	/* Check if #NM is triggered by XFEATURE_MASK_XTILEDATA */
> 
> GUEST_SYNC(7)
> 
> >+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
> >+	/* Clear xfd_err */
> 
> Same here, I'd do a GUEST_SYNC(8) and re-read MSR_IA32_XFD_ERR.
> 
> >+	wrmsr(MSR_IA32_XFD_ERR, 0);
> >+	GUEST_SYNC(2);
> 
   
    This need add regs->rip +=3 to skip __tileloadd() instruction(generate #NM) 
    when VM resume from GUEST_SYNC(9).

    Thanks!

    Yang


> This becomes GUEST_SYNC(9).
> 
> >+}
> 
> 
> >+		case UCALL_SYNC:
> >+			switch (uc.args[1]) {
> >+			case 1:
> >+				fprintf(stderr,
> >+					"Exit VM by GUEST_SYNC(1), check save/restore.\n");
> >+
> >+				/* Compacted mode, get amx offset by xsave area
> >+				 * size subtract 8K amx size.
> >+				 */
> >+				amx_offset = xsave_restore_size - NUM_TILES*TILE_SIZE;
> >+				state = vcpu_save_state(vm, VCPU_ID);
> >+				void *amx_start = (void *)state->xsave + amx_offset;
> >+				void *tiles_data = (void *)addr_gva2hva(vm, tiledata);
> >+				/* Only check TMM0 register, 1 tile */
> >+				ret = memcmp(amx_start, tiles_data, TILE_SIZE);
> >+				TEST_ASSERT(ret == 0, "memcmp failed, ret=%d\n", ret);
> >+				kvm_x86_state_cleanup(state);
> >+				break;
> 
> All GUEST_SYNCs should do save_state/load_state like state_test.c.
> Then of course you can *also* check TMM0 after __tileloadd, which
> would be cases 4 and 10.
> 

  Yes, the new code will add save_state/load_state as in the state_test.c file.

  Thanks!

  Yang



> Thanks,
> 
> Paolo
> 
> >+			case 2:
> >+				fprintf(stderr,
> >+					"Exit VM by GUEST_SYNC(2), generate #NM exception.\n");
> >+				goto done;
> >+			}
> >+			break;
> >+		case UCALL_DONE:
> >+			goto done;
> >+		default:
> >+			TEST_FAIL("Unknown ucall %lu", uc.cmd);
> >+		}
> >+	}
> >+done:
> >+	kvm_vm_free(vm);
> >+}
> >