[PATCH 0/4] KVM: selftests: Introduce VM_MODE_PXXV48_4K

[PATCH 0/4] KVM: selftests: Introduce VM_MODE_PXXV48_4K

[Peter Xu]

The work is based on Thomas's s390 port for dirty_log_test.

This series originates from "[PATCH] KVM: selftests: Detect max PA
width from cpuid" [1] and one of Drew's comments - instead of keeping
the hackish line to overwrite guest_pa_bits all the time, this series
introduced the new mode VM_MODE_PXXV48_4K for x86_64 platform.

The major issue is that even all the x86_64 kvm selftests are
currently using the guest mode VM_MODE_P52V48_4K, many x86_64 hosts
are not using 52 bits PA (and in most cases, far less).  If with luck
we could be having 48 bits hosts, but it's more adhoc (I've observed 3
x86_64 systems, they are having different PA width of 36, 39, 48).  I
am not sure whether this is happening to the other archs as well, but
it probably makes sense to bring the x86_64 tests to the real world on
always using the correct PA bits.

A side effect of this series is that it will also fix the crash we've
encountered on Xeon E3-1220 as mentioned [1] due to the
differenciation of PA width.

With [1], we've observed AMD host issues when with NPT=off.  However a
funny fact is that after I reworked into this series, the tests can
instead pass on both NPT=on/off.  It could be that the series changes
vm->pa_bits or other fields so something was affected.  I didn't dig
more on that though, considering we should not lose anything.

Any kind of smoke test would be greatly welcomed (especially on s390
or ARM).  Same to comments.  Thanks,

[1] https://lkml.org/lkml/2019/8/26/141

Peter Xu (4):
  KVM: selftests: Move vm type into _vm_create() internally
  KVM: selftests: Create VM earlier for dirty log test
  KVM: selftests: Introduce VM_MODE_PXXV48_4K
  KVM: selftests: Remove duplicate guest mode handling

 tools/testing/selftests/kvm/dirty_log_test.c  | 78 +++++--------------
 .../testing/selftests/kvm/include/kvm_util.h  | 17 +++-
 .../selftests/kvm/lib/aarch64/processor.c     |  3 +
 tools/testing/selftests/kvm/lib/kvm_util.c    | 77 ++++++++++++++----
 .../selftests/kvm/lib/x86_64/processor.c      |  8 +-
 5 files changed, 107 insertions(+), 76 deletions(-)

-- 
2.21.0

[PATCH 1/4] KVM: selftests: Move vm type into _vm_create() internally

[Peter Xu]

Rather than passing the vm type from the top level to the end of vm
creation, let's simply keep that as an internal of kvm_vm struct and
decide the type in _vm_create().  Several reasons for doing this:

- The vm type is only decided by physical address width and currently
  only used in aarch64, so we've got enough information as long as
  we're passing vm_guest_mode into _vm_create(),

- This removes a loop dependency between the vm->type and creation of
  vms.  That's why now we need to parse vm_guest_mode twice sometimes,
  once in run_test() and then again in _vm_create().  The follow up
  patches will move on to clean up that as well so we can have a
  single place to decide guest machine types and so.

Note that this patch will slightly change the behavior of aarch64
tests in that previously most vm_create() callers will directly pass
in type==0 into _vm_create() but now the type will depend on
vm_guest_mode, however it shouldn't affect any user because all
vm_create() users of aarch64 will be using VM_MODE_DEFAULT guest
mode (which is VM_MODE_P40V48_4K) so at last type will still be zero.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tools/testing/selftests/kvm/dirty_log_test.c  | 12 +++--------
 .../testing/selftests/kvm/include/kvm_util.h  |  2 +-
 tools/testing/selftests/kvm/lib/kvm_util.c    | 20 ++++++++++++-------
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index dc3346e090f5..424efcf8c734 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -249,14 +249,13 @@ static void vm_dirty_log_verify(unsigned long *bmap)
 }
 
 static struct kvm_vm *create_vm(enum vm_guest_mode mode, uint32_t vcpuid,
-				uint64_t extra_mem_pages, void *guest_code,
-				unsigned long type)
+				uint64_t extra_mem_pages, void *guest_code)
 {
 	struct kvm_vm *vm;
 	uint64_t extra_pg_pages = extra_mem_pages / 512 * 2;
 
 	vm = _vm_create(mode, DEFAULT_GUEST_PHY_PAGES + extra_pg_pages,
-			O_RDWR, type);
+			O_RDWR);
 	kvm_vm_elf_load(vm, program_invocation_name, 0, 0);
 #ifdef __x86_64__
 	vm_create_irqchip(vm);
@@ -273,7 +272,6 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 	struct kvm_vm *vm;
 	uint64_t max_gfn;
 	unsigned long *bmap;
-	unsigned long type = 0;
 
 	switch (mode) {
 	case VM_MODE_P52V48_4K:
@@ -314,10 +312,6 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 	 * bits we can change to 39.
 	 */
 	guest_pa_bits = 39;
-#endif
-#ifdef __aarch64__
-	if (guest_pa_bits != 40)
-		type = KVM_VM_TYPE_ARM_IPA_SIZE(guest_pa_bits);
 #endif
 	max_gfn = (1ul << (guest_pa_bits - guest_page_shift)) - 1;
 	guest_page_size = (1ul << guest_page_shift);
@@ -351,7 +345,7 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 	bmap = bitmap_alloc(host_num_pages);
 	host_bmap_track = bitmap_alloc(host_num_pages);
 
-	vm = create_vm(mode, VCPU_ID, guest_num_pages, guest_code, type);
+	vm = create_vm(mode, VCPU_ID, guest_num_pages, guest_code);
 
 #ifdef USE_CLEAR_DIRTY_LOG
 	struct kvm_enable_cap cap = {};
diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index 5463b7896a0a..cfc079f20815 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -61,7 +61,7 @@ int vm_enable_cap(struct kvm_vm *vm, struct kvm_enable_cap *cap);
 
 struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm);
 struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
-			  int perm, unsigned long type);
+			  int perm);
 void kvm_vm_free(struct kvm_vm *vmp);
 void kvm_vm_restart(struct kvm_vm *vmp, int perm);
 void kvm_vm_release(struct kvm_vm *vmp);
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 6e49bb039376..0c7c4368bc14 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -84,7 +84,7 @@ int vm_enable_cap(struct kvm_vm *vm, struct kvm_enable_cap *cap)
 	return ret;
 }
 
-static void vm_open(struct kvm_vm *vm, int perm, unsigned long type)
+static void vm_open(struct kvm_vm *vm, int perm)
 {
 	vm->kvm_fd = open(KVM_DEV_PATH, perm);
 	if (vm->kvm_fd < 0)
@@ -95,7 +95,7 @@ static void vm_open(struct kvm_vm *vm, int perm, unsigned long type)
 		exit(KSFT_SKIP);
 	}
 
-	vm->fd = ioctl(vm->kvm_fd, KVM_CREATE_VM, type);
+	vm->fd = ioctl(vm->kvm_fd, KVM_CREATE_VM, vm->type);
 	TEST_ASSERT(vm->fd >= 0, "KVM_CREATE_VM ioctl failed, "
 		"rc: %i errno: %i", vm->fd, errno);
 }
@@ -131,7 +131,7 @@ _Static_assert(sizeof(vm_guest_mode_string)/sizeof(char *) == NUM_VM_MODES,
  * given by perm (e.g. O_RDWR).
  */
 struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
-			  int perm, unsigned long type)
+			  int perm)
 {
 	struct kvm_vm *vm;
 
@@ -139,8 +139,7 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
 	TEST_ASSERT(vm != NULL, "Insufficient Memory");
 
 	vm->mode = mode;
-	vm->type = type;
-	vm_open(vm, perm, type);
+	vm->type = 0;
 
 	/* Setup mode specific traits. */
 	switch (vm->mode) {
@@ -190,6 +189,13 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
 		TEST_ASSERT(false, "Unknown guest mode, mode: 0x%x", mode);
 	}
 
+#ifdef __aarch64__
+	if (vm->pa_bits != 40)
+		vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(guest_pa_bits);
+#endif
+
+	vm_open(vm, perm);
+
 	/* Limit to VA-bit canonical virtual addresses. */
 	vm->vpages_valid = sparsebit_alloc();
 	sparsebit_set_num(vm->vpages_valid,
@@ -212,7 +218,7 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
 
 struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm)
 {
-	return _vm_create(mode, phy_pages, perm, 0);
+	return _vm_create(mode, phy_pages, perm);
 }
 
 /*
@@ -232,7 +238,7 @@ void kvm_vm_restart(struct kvm_vm *vmp, int perm)
 {
 	struct userspace_mem_region *region;
 
-	vm_open(vmp, perm, vmp->type);
+	vm_open(vmp, perm);
 	if (vmp->has_irqchip)
 		vm_create_irqchip(vmp);
 
-- 
2.21.0

[PATCH 2/4] KVM: selftests: Create VM earlier for dirty log test

[Peter Xu]

Since we've just removed the dependency of vm type in previous patch,
now we can create the vm much earlier.  Note that to move it earlier
we used an approximation of number of extra pages but it should be
fine.

This prepares for the follow up patches to finally remove the
duplication of guest mode parsings.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tools/testing/selftests/kvm/dirty_log_test.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 424efcf8c734..040952f3d4ad 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -264,6 +264,9 @@ static struct kvm_vm *create_vm(enum vm_guest_mode mode, uint32_t vcpuid,
 	return vm;
 }
 
+#define DIRTY_MEM_BITS 30 /* 1G */
+#define PAGE_SHIFT_4K  12
+
 static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 		     unsigned long interval, uint64_t phys_offset)
 {
@@ -273,6 +276,18 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 	uint64_t max_gfn;
 	unsigned long *bmap;
 
+	/*
+	 * We reserve page table for 2 times of extra dirty mem which
+	 * will definitely cover the original (1G+) test range.  Here
+	 * we do the calculation with 4K page size which is the
+	 * smallest so the page number will be enough for all archs
+	 * (e.g., 64K page size guest will need even less memory for
+	 * page tables).
+	 */
+	vm = create_vm(mode, VCPU_ID,
+		       2ul << (DIRTY_MEM_BITS - PAGE_SHIFT_4K),
+		       guest_code);
+
 	switch (mode) {
 	case VM_MODE_P52V48_4K:
 		guest_pa_bits = 52;
@@ -319,7 +334,7 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 	 * A little more than 1G of guest page sized pages.  Cover the
 	 * case where the size is not aligned to 64 pages.
 	 */
-	guest_num_pages = (1ul << (30 - guest_page_shift)) + 16;
+	guest_num_pages = (1ul << (DIRTY_MEM_BITS - guest_page_shift)) + 16;
 #ifdef __s390x__
 	/* Round up to multiple of 1M (segment size) */
 	guest_num_pages = (guest_num_pages + 0xff) & ~0xffUL;
@@ -345,8 +360,6 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 	bmap = bitmap_alloc(host_num_pages);
 	host_bmap_track = bitmap_alloc(host_num_pages);
 
-	vm = create_vm(mode, VCPU_ID, guest_num_pages, guest_code);
-
 #ifdef USE_CLEAR_DIRTY_LOG
 	struct kvm_enable_cap cap = {};
 
-- 
2.21.0

[PATCH 3/4] KVM: selftests: Introduce VM_MODE_PXXV48_4K

[Peter Xu]

The naming VM_MODE_P52V48_4K is explicit but unclear when used on
x86_64 machines, because x86_64 machines are having various physical
address width rather than some static values.  Here's some examples:

  - Intel Xeon E3-1220:  36 bits
  - Intel Core i7-8650:  39 bits
  - AMD   EPYC 7251:     48 bits

All of them are using 48 bits linear address width but with totally
different physical address width (and most of the old machines should
be less than 52 bits).

Let's create a new guest mode called VM_MODE_PXXV48_4K for current
x86_64 tests and make it as the default to replace the old naming of
VM_MODE_P52V48_4K because it shows more clearly that the PA width is
not really a constant.  Meanwhile we also stop assuming all the x86
machines are having 52 bits PA width but instead we fetch the real
vm->pa_bits from CPUID 0x80000008 during runtime.

We currently make this exclusively used by x86_64 but no other arch.

As a slight touch up, moving DEBUG macro from dirty_log_test.c to
kvm_util.h so lib can use it too.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tools/testing/selftests/kvm/dirty_log_test.c  |  5 +--
 .../testing/selftests/kvm/include/kvm_util.h  | 11 ++++-
 .../selftests/kvm/lib/aarch64/processor.c     |  3 ++
 tools/testing/selftests/kvm/lib/kvm_util.c    | 40 ++++++++++++++++---
 .../selftests/kvm/lib/x86_64/processor.c      |  8 ++--
 5 files changed, 53 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 040952f3d4ad..b2e07a3173b2 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -19,8 +19,6 @@
 #include "kvm_util.h"
 #include "processor.h"
 
-#define DEBUG printf
-
 #define VCPU_ID				1
 
 /* The memory slot index to track dirty pages */
@@ -290,6 +288,7 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 
 	switch (mode) {
 	case VM_MODE_P52V48_4K:
+	case VM_MODE_PXXV48_4K:
 		guest_pa_bits = 52;
 		guest_page_shift = 12;
 		break;
@@ -489,7 +488,7 @@ int main(int argc, char *argv[])
 #endif
 
 #ifdef __x86_64__
-	vm_guest_mode_params_init(VM_MODE_P52V48_4K, true, true);
+	vm_guest_mode_params_init(VM_MODE_PXXV48_4K, true, true);
 #endif
 #ifdef __aarch64__
 	vm_guest_mode_params_init(VM_MODE_P40V48_4K, true, true);
diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index cfc079f20815..1c700c6b31b5 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -24,6 +24,8 @@ struct kvm_vm;
 typedef uint64_t vm_paddr_t; /* Virtual Machine (Guest) physical address */
 typedef uint64_t vm_vaddr_t; /* Virtual Machine (Guest) virtual address */
 
+#define DEBUG printf
+
 /* Minimum allocated guest virtual and physical addresses */
 #define KVM_UTIL_MIN_VADDR		0x2000
 
@@ -38,12 +40,19 @@ enum vm_guest_mode {
 	VM_MODE_P48V48_64K,
 	VM_MODE_P40V48_4K,
 	VM_MODE_P40V48_64K,
+	VM_MODE_PXXV48_4K,	/* For 48bits VA but ANY bits PA */
 	NUM_VM_MODES,
 };
 
 #ifdef __aarch64__
 #define VM_MODE_DEFAULT VM_MODE_P40V48_4K
-#else
+#endif
+
+#ifdef __x86_64__
+#define VM_MODE_DEFAULT VM_MODE_PXXV48_4K
+#endif
+
+#ifndef VM_MODE_DEFAULT
 #define VM_MODE_DEFAULT VM_MODE_P52V48_4K
 #endif
 
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index 486400a97374..86036a59a668 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -264,6 +264,9 @@ void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *ini
 	case VM_MODE_P52V48_4K:
 		TEST_ASSERT(false, "AArch64 does not support 4K sized pages "
 				   "with 52-bit physical address ranges");
+	case VM_MODE_PXXV48_4K:
+		TEST_ASSERT(false, "AArch64 does not support 4K sized pages "
+				   "with ANY-bit physical address ranges");
 	case VM_MODE_P52V48_64K:
 		tcr_el1 |= 1ul << 14; /* TG0 = 64KB */
 		tcr_el1 |= 6ul << 32; /* IPS = 52 bits */
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 0c7c4368bc14..8c6f872a8793 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -8,6 +8,7 @@
 #include "test_util.h"
 #include "kvm_util.h"
 #include "kvm_util_internal.h"
+#include "processor.h"
 
 #include <assert.h>
 #include <sys/mman.h>
@@ -101,12 +102,13 @@ static void vm_open(struct kvm_vm *vm, int perm)
 }
 
 const char * const vm_guest_mode_string[] = {
-	"PA-bits:52, VA-bits:48, 4K pages",
-	"PA-bits:52, VA-bits:48, 64K pages",
-	"PA-bits:48, VA-bits:48, 4K pages",
-	"PA-bits:48, VA-bits:48, 64K pages",
-	"PA-bits:40, VA-bits:48, 4K pages",
-	"PA-bits:40, VA-bits:48, 64K pages",
+	"PA-bits:52,  VA-bits:48, 4K pages",
+	"PA-bits:52,  VA-bits:48, 64K pages",
+	"PA-bits:48,  VA-bits:48, 4K pages",
+	"PA-bits:48,  VA-bits:48, 64K pages",
+	"PA-bits:40,  VA-bits:48, 4K pages",
+	"PA-bits:40,  VA-bits:48, 64K pages",
+	"PA-bits:ANY, VA-bits:48, 4K pages",
 };
 _Static_assert(sizeof(vm_guest_mode_string)/sizeof(char *) == NUM_VM_MODES,
 	       "Missing new mode strings?");
@@ -185,6 +187,32 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
 		vm->page_size = 0x10000;
 		vm->page_shift = 16;
 		break;
+	case VM_MODE_PXXV48_4K:
+#ifdef __x86_64__
+		{
+			struct kvm_cpuid_entry2 *entry;
+
+			/* SDM 4.1.4 */
+			entry = kvm_get_supported_cpuid_entry(0x80000008);
+			if (entry) {
+				vm->pa_bits = entry->eax & 0xff;
+				vm->va_bits = (entry->eax >> 8) & 0xff;
+			} else {
+				vm->pa_bits = vm->va_bits = 32;
+			}
+			TEST_ASSERT(vm->va_bits == 48, "Linear address width "
+				    "(%d bits) not supported", vm->va_bits);
+			vm->pgtable_levels = 4;
+			vm->page_size = 0x1000;
+			vm->page_shift = 12;
+			DEBUG("Guest physical address width detected: %d\n",
+			      vm->pa_bits);
+		}
+#else
+		TEST_ASSERT(false, "VM_MODE_PXXV48_4K not supported on "
+			    "non-x86 platforms");
+#endif
+		break;
 	default:
 		TEST_ASSERT(false, "Unknown guest mode, mode: 0x%x", mode);
 	}
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index 6cb34a0fa200..eb750ee24d2e 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -228,7 +228,7 @@ void sregs_dump(FILE *stream, struct kvm_sregs *sregs,
 
 void virt_pgd_alloc(struct kvm_vm *vm, uint32_t pgd_memslot)
 {
-	TEST_ASSERT(vm->mode == VM_MODE_P52V48_4K, "Attempt to use "
+	TEST_ASSERT(vm->mode == VM_MODE_PXXV48_4K, "Attempt to use "
 		"unknown or unsupported guest mode, mode: 0x%x", vm->mode);
 
 	/* If needed, create page map l4 table. */
@@ -261,7 +261,7 @@ void virt_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
 	uint16_t index[4];
 	struct pageMapL4Entry *pml4e;
 
-	TEST_ASSERT(vm->mode == VM_MODE_P52V48_4K, "Attempt to use "
+	TEST_ASSERT(vm->mode == VM_MODE_PXXV48_4K, "Attempt to use "
 		"unknown or unsupported guest mode, mode: 0x%x", vm->mode);
 
 	TEST_ASSERT((vaddr % vm->page_size) == 0,
@@ -547,7 +547,7 @@ vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva)
 	struct pageDirectoryEntry *pde;
 	struct pageTableEntry *pte;
 
-	TEST_ASSERT(vm->mode == VM_MODE_P52V48_4K, "Attempt to use "
+	TEST_ASSERT(vm->mode == VM_MODE_PXXV48_4K, "Attempt to use "
 		"unknown or unsupported guest mode, mode: 0x%x", vm->mode);
 
 	index[0] = (gva >> 12) & 0x1ffu;
@@ -621,7 +621,7 @@ static void vcpu_setup(struct kvm_vm *vm, int vcpuid, int pgd_memslot, int gdt_m
 	kvm_setup_gdt(vm, &sregs.gdt, gdt_memslot, pgd_memslot);
 
 	switch (vm->mode) {
-	case VM_MODE_P52V48_4K:
+	case VM_MODE_PXXV48_4K:
 		sregs.cr0 = X86_CR0_PE | X86_CR0_NE | X86_CR0_PG;
 		sregs.cr4 |= X86_CR4_PAE | X86_CR4_OSFXSR;
 		sregs.efer |= (EFER_LME | EFER_LMA | EFER_NX);
-- 
2.21.0

[PATCH 4/4] KVM: selftests: Remove duplicate guest mode handling

[Peter Xu]

Remove the duplication code in run_test() of dirty_log_test because
after some reordering of functions now we can directly use the outcome
of vm_create().

Meanwhile, with the new VM_MODE_PXXV48_4K, we can safely revert
b442324b58 too where we stick the x86_64 PA width to 39 bits for
dirty_log_test.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tools/testing/selftests/kvm/dirty_log_test.c  | 52 ++-----------------
 .../testing/selftests/kvm/include/kvm_util.h  |  4 ++
 tools/testing/selftests/kvm/lib/kvm_util.c    | 17 ++++++
 3 files changed, 26 insertions(+), 47 deletions(-)

diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index b2e07a3173b2..73f679bbf082 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -268,10 +268,8 @@ static struct kvm_vm *create_vm(enum vm_guest_mode mode, uint32_t vcpuid,
 static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 		     unsigned long interval, uint64_t phys_offset)
 {
-	unsigned int guest_pa_bits, guest_page_shift;
 	pthread_t vcpu_thread;
 	struct kvm_vm *vm;
-	uint64_t max_gfn;
 	unsigned long *bmap;
 
 	/*
@@ -286,54 +284,13 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 		       2ul << (DIRTY_MEM_BITS - PAGE_SHIFT_4K),
 		       guest_code);
 
-	switch (mode) {
-	case VM_MODE_P52V48_4K:
-	case VM_MODE_PXXV48_4K:
-		guest_pa_bits = 52;
-		guest_page_shift = 12;
-		break;
-	case VM_MODE_P52V48_64K:
-		guest_pa_bits = 52;
-		guest_page_shift = 16;
-		break;
-	case VM_MODE_P48V48_4K:
-		guest_pa_bits = 48;
-		guest_page_shift = 12;
-		break;
-	case VM_MODE_P48V48_64K:
-		guest_pa_bits = 48;
-		guest_page_shift = 16;
-		break;
-	case VM_MODE_P40V48_4K:
-		guest_pa_bits = 40;
-		guest_page_shift = 12;
-		break;
-	case VM_MODE_P40V48_64K:
-		guest_pa_bits = 40;
-		guest_page_shift = 16;
-		break;
-	default:
-		TEST_ASSERT(false, "Unknown guest mode, mode: 0x%x", mode);
-	}
-
-	DEBUG("Testing guest mode: %s\n", vm_guest_mode_string(mode));
-
-#ifdef __x86_64__
-	/*
-	 * FIXME
-	 * The x86_64 kvm selftests framework currently only supports a
-	 * single PML4 which restricts the number of physical address
-	 * bits we can change to 39.
-	 */
-	guest_pa_bits = 39;
-#endif
-	max_gfn = (1ul << (guest_pa_bits - guest_page_shift)) - 1;
-	guest_page_size = (1ul << guest_page_shift);
+	guest_page_size = vm_get_page_size(vm);
 	/*
 	 * A little more than 1G of guest page sized pages.  Cover the
 	 * case where the size is not aligned to 64 pages.
 	 */
-	guest_num_pages = (1ul << (DIRTY_MEM_BITS - guest_page_shift)) + 16;
+	guest_num_pages = (1ul << (DIRTY_MEM_BITS -
+				   vm_get_page_shift(vm))) + 16;
 #ifdef __s390x__
 	/* Round up to multiple of 1M (segment size) */
 	guest_num_pages = (guest_num_pages + 0xff) & ~0xffUL;
@@ -343,7 +300,8 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
 			 !!((guest_num_pages * guest_page_size) % host_page_size);
 
 	if (!phys_offset) {
-		guest_test_phys_mem = (max_gfn - guest_num_pages) * guest_page_size;
+		guest_test_phys_mem = (vm_get_max_gfn(vm) -
+				       guest_num_pages) * guest_page_size;
 		guest_test_phys_mem &= ~(host_page_size - 1);
 	} else {
 		guest_test_phys_mem = phys_offset;
diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index 1c700c6b31b5..0d65fc676182 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -155,6 +155,10 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code);
 
 bool vm_is_unrestricted_guest(struct kvm_vm *vm);
 
+unsigned int vm_get_page_size(struct kvm_vm *vm);
+unsigned int vm_get_page_shift(struct kvm_vm *vm);
+unsigned int vm_get_max_gfn(struct kvm_vm *vm);
+
 struct kvm_userspace_memory_region *
 kvm_userspace_memory_region_find(struct kvm_vm *vm, uint64_t start,
 				 uint64_t end);
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 8c6f872a8793..cf39643ff2c7 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -137,6 +137,8 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
 {
 	struct kvm_vm *vm;
 
+	DEBUG("Testing guest mode: %s\n", vm_guest_mode_string(mode));
+
 	vm = calloc(1, sizeof(*vm));
 	TEST_ASSERT(vm != NULL, "Insufficient Memory");
 
@@ -1662,3 +1664,18 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
 
 	return val == 'Y';
 }
+
+unsigned int vm_get_page_size(struct kvm_vm *vm)
+{
+	return vm->page_size;
+}
+
+unsigned int vm_get_page_shift(struct kvm_vm *vm)
+{
+	return vm->page_shift;
+}
+
+unsigned int vm_get_max_gfn(struct kvm_vm *vm)
+{
+	return vm->max_gfn;
+}
-- 
2.21.0

Re: [PATCH 1/4] KVM: selftests: Move vm type into _vm_create() internally

[Andrew Jones]

On Tue, Aug 27, 2019 at 09:10:12PM +0800, Peter Xu wrote:
> Rather than passing the vm type from the top level to the end of vm
> creation, let's simply keep that as an internal of kvm_vm struct and
> decide the type in _vm_create().  Several reasons for doing this:
> 
> - The vm type is only decided by physical address width and currently
>   only used in aarch64, so we've got enough information as long as
>   we're passing vm_guest_mode into _vm_create(),
> 
> - This removes a loop dependency between the vm->type and creation of
>   vms.  That's why now we need to parse vm_guest_mode twice sometimes,
>   once in run_test() and then again in _vm_create().  The follow up
>   patches will move on to clean up that as well so we can have a
>   single place to decide guest machine types and so.
> 
> Note that this patch will slightly change the behavior of aarch64
> tests in that previously most vm_create() callers will directly pass
> in type==0 into _vm_create() but now the type will depend on
> vm_guest_mode, however it shouldn't affect any user because all
> vm_create() users of aarch64 will be using VM_MODE_DEFAULT guest
> mode (which is VM_MODE_P40V48_4K) so at last type will still be zero.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  tools/testing/selftests/kvm/dirty_log_test.c  | 12 +++--------
>  .../testing/selftests/kvm/include/kvm_util.h  |  2 +-
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 20 ++++++++++++-------
>  3 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index dc3346e090f5..424efcf8c734 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -249,14 +249,13 @@ static void vm_dirty_log_verify(unsigned long *bmap)
>  }
>  
>  static struct kvm_vm *create_vm(enum vm_guest_mode mode, uint32_t vcpuid,
> -				uint64_t extra_mem_pages, void *guest_code,
> -				unsigned long type)
> +				uint64_t extra_mem_pages, void *guest_code)
>  {
>  	struct kvm_vm *vm;
>  	uint64_t extra_pg_pages = extra_mem_pages / 512 * 2;
>  
>  	vm = _vm_create(mode, DEFAULT_GUEST_PHY_PAGES + extra_pg_pages,
> -			O_RDWR, type);
> +			O_RDWR);

nit: after removing type can O_RDWR go up a line?

>  	kvm_vm_elf_load(vm, program_invocation_name, 0, 0);
>  #ifdef __x86_64__
>  	vm_create_irqchip(vm);
> @@ -273,7 +272,6 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
>  	struct kvm_vm *vm;
>  	uint64_t max_gfn;
>  	unsigned long *bmap;
> -	unsigned long type = 0;
>  
>  	switch (mode) {
>  	case VM_MODE_P52V48_4K:
> @@ -314,10 +312,6 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
>  	 * bits we can change to 39.
>  	 */
>  	guest_pa_bits = 39;
> -#endif
> -#ifdef __aarch64__
> -	if (guest_pa_bits != 40)
> -		type = KVM_VM_TYPE_ARM_IPA_SIZE(guest_pa_bits);
>  #endif
>  	max_gfn = (1ul << (guest_pa_bits - guest_page_shift)) - 1;
>  	guest_page_size = (1ul << guest_page_shift);
> @@ -351,7 +345,7 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
>  	bmap = bitmap_alloc(host_num_pages);
>  	host_bmap_track = bitmap_alloc(host_num_pages);
>  
> -	vm = create_vm(mode, VCPU_ID, guest_num_pages, guest_code, type);
> +	vm = create_vm(mode, VCPU_ID, guest_num_pages, guest_code);
>  
>  #ifdef USE_CLEAR_DIRTY_LOG
>  	struct kvm_enable_cap cap = {};
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index 5463b7896a0a..cfc079f20815 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -61,7 +61,7 @@ int vm_enable_cap(struct kvm_vm *vm, struct kvm_enable_cap *cap);
>  
>  struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm);
>  struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
> -			  int perm, unsigned long type);
> +			  int perm);

nit: can perm go up?

>  void kvm_vm_free(struct kvm_vm *vmp);
>  void kvm_vm_restart(struct kvm_vm *vmp, int perm);
>  void kvm_vm_release(struct kvm_vm *vmp);
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 6e49bb039376..0c7c4368bc14 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -84,7 +84,7 @@ int vm_enable_cap(struct kvm_vm *vm, struct kvm_enable_cap *cap)
>  	return ret;
>  }
>  
> -static void vm_open(struct kvm_vm *vm, int perm, unsigned long type)
> +static void vm_open(struct kvm_vm *vm, int perm)
>  {
>  	vm->kvm_fd = open(KVM_DEV_PATH, perm);
>  	if (vm->kvm_fd < 0)
> @@ -95,7 +95,7 @@ static void vm_open(struct kvm_vm *vm, int perm, unsigned long type)
>  		exit(KSFT_SKIP);
>  	}
>  
> -	vm->fd = ioctl(vm->kvm_fd, KVM_CREATE_VM, type);
> +	vm->fd = ioctl(vm->kvm_fd, KVM_CREATE_VM, vm->type);
>  	TEST_ASSERT(vm->fd >= 0, "KVM_CREATE_VM ioctl failed, "
>  		"rc: %i errno: %i", vm->fd, errno);
>  }
> @@ -131,7 +131,7 @@ _Static_assert(sizeof(vm_guest_mode_string)/sizeof(char *) == NUM_VM_MODES,
>   * given by perm (e.g. O_RDWR).
>   */
>  struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
> -			  int perm, unsigned long type)
> +			  int perm)

nit: can perm go up?

>  {
>  	struct kvm_vm *vm;
>  
> @@ -139,8 +139,7 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
>  	TEST_ASSERT(vm != NULL, "Insufficient Memory");
>  
>  	vm->mode = mode;
> -	vm->type = type;
> -	vm_open(vm, perm, type);
> +	vm->type = 0;
>  
>  	/* Setup mode specific traits. */
>  	switch (vm->mode) {
> @@ -190,6 +189,13 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
>  		TEST_ASSERT(false, "Unknown guest mode, mode: 0x%x", mode);
>  	}
>  
> +#ifdef __aarch64__
> +	if (vm->pa_bits != 40)
> +		vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(guest_pa_bits);
                                                    ^^
                                                    should be vm->pa_bits

> +#endif
> +
> +	vm_open(vm, perm);
> +
>  	/* Limit to VA-bit canonical virtual addresses. */
>  	vm->vpages_valid = sparsebit_alloc();
>  	sparsebit_set_num(vm->vpages_valid,
> @@ -212,7 +218,7 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
>  
>  struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm)
>  {
> -	return _vm_create(mode, phy_pages, perm, 0);
> +	return _vm_create(mode, phy_pages, perm);
>  }
>  
>  /*
> @@ -232,7 +238,7 @@ void kvm_vm_restart(struct kvm_vm *vmp, int perm)
>  {
>  	struct userspace_mem_region *region;
>  
> -	vm_open(vmp, perm, vmp->type);
> +	vm_open(vmp, perm);
>  	if (vmp->has_irqchip)
>  		vm_create_irqchip(vmp);
>  
> -- 
> 2.21.0
> 

Thanks,
drew

Re: [PATCH 3/4] KVM: selftests: Introduce VM_MODE_PXXV48_4K

[Andrew Jones]

On Tue, Aug 27, 2019 at 09:10:14PM +0800, Peter Xu wrote:
> The naming VM_MODE_P52V48_4K is explicit but unclear when used on
> x86_64 machines, because x86_64 machines are having various physical
> address width rather than some static values.  Here's some examples:
> 
>   - Intel Xeon E3-1220:  36 bits
>   - Intel Core i7-8650:  39 bits
>   - AMD   EPYC 7251:     48 bits
> 
> All of them are using 48 bits linear address width but with totally
> different physical address width (and most of the old machines should
> be less than 52 bits).
> 
> Let's create a new guest mode called VM_MODE_PXXV48_4K for current
> x86_64 tests and make it as the default to replace the old naming of
> VM_MODE_P52V48_4K because it shows more clearly that the PA width is
> not really a constant.  Meanwhile we also stop assuming all the x86
> machines are having 52 bits PA width but instead we fetch the real
> vm->pa_bits from CPUID 0x80000008 during runtime.
> 
> We currently make this exclusively used by x86_64 but no other arch.
> 
> As a slight touch up, moving DEBUG macro from dirty_log_test.c to
> kvm_util.h so lib can use it too.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  tools/testing/selftests/kvm/dirty_log_test.c  |  5 +--
>  .../testing/selftests/kvm/include/kvm_util.h  | 11 ++++-
>  .../selftests/kvm/lib/aarch64/processor.c     |  3 ++
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 40 ++++++++++++++++---
>  .../selftests/kvm/lib/x86_64/processor.c      |  8 ++--
>  5 files changed, 53 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index 040952f3d4ad..b2e07a3173b2 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -19,8 +19,6 @@
>  #include "kvm_util.h"
>  #include "processor.h"
>  
> -#define DEBUG printf
> -
>  #define VCPU_ID				1
>  
>  /* The memory slot index to track dirty pages */
> @@ -290,6 +288,7 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
>  
>  	switch (mode) {
>  	case VM_MODE_P52V48_4K:
> +	case VM_MODE_PXXV48_4K:
>  		guest_pa_bits = 52;
>  		guest_page_shift = 12;
>  		break;
> @@ -489,7 +488,7 @@ int main(int argc, char *argv[])
>  #endif
>  
>  #ifdef __x86_64__
> -	vm_guest_mode_params_init(VM_MODE_P52V48_4K, true, true);
> +	vm_guest_mode_params_init(VM_MODE_PXXV48_4K, true, true);
>  #endif
>  #ifdef __aarch64__
>  	vm_guest_mode_params_init(VM_MODE_P40V48_4K, true, true);
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index cfc079f20815..1c700c6b31b5 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -24,6 +24,8 @@ struct kvm_vm;
>  typedef uint64_t vm_paddr_t; /* Virtual Machine (Guest) physical address */
>  typedef uint64_t vm_vaddr_t; /* Virtual Machine (Guest) virtual address */
>  
> +#define DEBUG printf

If this is going to be some general thing, then maybe we should do it
like this

#ifndef NDEBUG
#define dprintf printf
#endif


> +
>  /* Minimum allocated guest virtual and physical addresses */
>  #define KVM_UTIL_MIN_VADDR		0x2000
>  
> @@ -38,12 +40,19 @@ enum vm_guest_mode {
>  	VM_MODE_P48V48_64K,
>  	VM_MODE_P40V48_4K,
>  	VM_MODE_P40V48_64K,
> +	VM_MODE_PXXV48_4K,	/* For 48bits VA but ANY bits PA */
>  	NUM_VM_MODES,
>  };
>  
>  #ifdef __aarch64__
>  #define VM_MODE_DEFAULT VM_MODE_P40V48_4K
> -#else
> +#endif
> +
> +#ifdef __x86_64__
> +#define VM_MODE_DEFAULT VM_MODE_PXXV48_4K
> +#endif
> +
> +#ifndef VM_MODE_DEFAULT
>  #define VM_MODE_DEFAULT VM_MODE_P52V48_4K
>  #endif

nit: how about

#if defined(__aarch64__)
...
#elif defined(__x86_64__)
...
#else
...
#endif

>  
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> index 486400a97374..86036a59a668 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> @@ -264,6 +264,9 @@ void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *ini
>  	case VM_MODE_P52V48_4K:
>  		TEST_ASSERT(false, "AArch64 does not support 4K sized pages "
>  				   "with 52-bit physical address ranges");
> +	case VM_MODE_PXXV48_4K:
> +		TEST_ASSERT(false, "AArch64 does not support 4K sized pages "
> +				   "with ANY-bit physical address ranges");
>  	case VM_MODE_P52V48_64K:
>  		tcr_el1 |= 1ul << 14; /* TG0 = 64KB */
>  		tcr_el1 |= 6ul << 32; /* IPS = 52 bits */
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 0c7c4368bc14..8c6f872a8793 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -8,6 +8,7 @@
>  #include "test_util.h"
>  #include "kvm_util.h"
>  #include "kvm_util_internal.h"
> +#include "processor.h"
>  
>  #include <assert.h>
>  #include <sys/mman.h>
> @@ -101,12 +102,13 @@ static void vm_open(struct kvm_vm *vm, int perm)
>  }
>  
>  const char * const vm_guest_mode_string[] = {
> -	"PA-bits:52, VA-bits:48, 4K pages",
> -	"PA-bits:52, VA-bits:48, 64K pages",
> -	"PA-bits:48, VA-bits:48, 4K pages",
> -	"PA-bits:48, VA-bits:48, 64K pages",
> -	"PA-bits:40, VA-bits:48, 4K pages",
> -	"PA-bits:40, VA-bits:48, 64K pages",
> +	"PA-bits:52,  VA-bits:48, 4K pages",
> +	"PA-bits:52,  VA-bits:48, 64K pages",
> +	"PA-bits:48,  VA-bits:48, 4K pages",
> +	"PA-bits:48,  VA-bits:48, 64K pages",
> +	"PA-bits:40,  VA-bits:48, 4K pages",
> +	"PA-bits:40,  VA-bits:48, 64K pages",
> +	"PA-bits:ANY, VA-bits:48, 4K pages",

nit: while formatting we could align the 'K's in the 64/4K column

>  };
>  _Static_assert(sizeof(vm_guest_mode_string)/sizeof(char *) == NUM_VM_MODES,
>  	       "Missing new mode strings?");
> @@ -185,6 +187,32 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
>  		vm->page_size = 0x10000;
>  		vm->page_shift = 16;
>  		break;
> +	case VM_MODE_PXXV48_4K:
> +#ifdef __x86_64__
> +		{
> +			struct kvm_cpuid_entry2 *entry;
> +
> +			/* SDM 4.1.4 */
> +			entry = kvm_get_supported_cpuid_entry(0x80000008);
> +			if (entry) {
> +				vm->pa_bits = entry->eax & 0xff;
> +				vm->va_bits = (entry->eax >> 8) & 0xff;
> +			} else {
> +				vm->pa_bits = vm->va_bits = 32;
> +			}
> +			TEST_ASSERT(vm->va_bits == 48, "Linear address width "
> +				    "(%d bits) not supported", vm->va_bits);
> +			vm->pgtable_levels = 4;
> +			vm->page_size = 0x1000;
> +			vm->page_shift = 12;
> +			DEBUG("Guest physical address width detected: %d\n",
> +			      vm->pa_bits);
> +		}

How about making this a function that lives in x86_64/processor.c?

> +#else
> +		TEST_ASSERT(false, "VM_MODE_PXXV48_4K not supported on "
> +			    "non-x86 platforms");

This should make the above aarch64_vcpu_setup() change unnecessary.

> +#endif
> +		break;
>  	default:
>  		TEST_ASSERT(false, "Unknown guest mode, mode: 0x%x", mode);
>  	}
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 6cb34a0fa200..eb750ee24d2e 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -228,7 +228,7 @@ void sregs_dump(FILE *stream, struct kvm_sregs *sregs,
>  
>  void virt_pgd_alloc(struct kvm_vm *vm, uint32_t pgd_memslot)
>  {
> -	TEST_ASSERT(vm->mode == VM_MODE_P52V48_4K, "Attempt to use "
> +	TEST_ASSERT(vm->mode == VM_MODE_PXXV48_4K, "Attempt to use "
>  		"unknown or unsupported guest mode, mode: 0x%x", vm->mode);
>  
>  	/* If needed, create page map l4 table. */
> @@ -261,7 +261,7 @@ void virt_pg_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
>  	uint16_t index[4];
>  	struct pageMapL4Entry *pml4e;
>  
> -	TEST_ASSERT(vm->mode == VM_MODE_P52V48_4K, "Attempt to use "
> +	TEST_ASSERT(vm->mode == VM_MODE_PXXV48_4K, "Attempt to use "
>  		"unknown or unsupported guest mode, mode: 0x%x", vm->mode);
>  
>  	TEST_ASSERT((vaddr % vm->page_size) == 0,
> @@ -547,7 +547,7 @@ vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva)
>  	struct pageDirectoryEntry *pde;
>  	struct pageTableEntry *pte;
>  
> -	TEST_ASSERT(vm->mode == VM_MODE_P52V48_4K, "Attempt to use "
> +	TEST_ASSERT(vm->mode == VM_MODE_PXXV48_4K, "Attempt to use "
>  		"unknown or unsupported guest mode, mode: 0x%x", vm->mode);
>  
>  	index[0] = (gva >> 12) & 0x1ffu;
> @@ -621,7 +621,7 @@ static void vcpu_setup(struct kvm_vm *vm, int vcpuid, int pgd_memslot, int gdt_m
>  	kvm_setup_gdt(vm, &sregs.gdt, gdt_memslot, pgd_memslot);
>  
>  	switch (vm->mode) {
> -	case VM_MODE_P52V48_4K:
> +	case VM_MODE_PXXV48_4K:
>  		sregs.cr0 = X86_CR0_PE | X86_CR0_NE | X86_CR0_PG;
>  		sregs.cr4 |= X86_CR4_PAE | X86_CR4_OSFXSR;
>  		sregs.efer |= (EFER_LME | EFER_LMA | EFER_NX);
> -- 
> 2.21.0
> 

Thanks,
drew

Re: [PATCH 4/4] KVM: selftests: Remove duplicate guest mode handling

[Andrew Jones]

On Tue, Aug 27, 2019 at 09:10:15PM +0800, Peter Xu wrote:
> Remove the duplication code in run_test() of dirty_log_test because
> after some reordering of functions now we can directly use the outcome
> of vm_create().
> 
> Meanwhile, with the new VM_MODE_PXXV48_4K, we can safely revert
> b442324b58 too where we stick the x86_64 PA width to 39 bits for
> dirty_log_test.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  tools/testing/selftests/kvm/dirty_log_test.c  | 52 ++-----------------
>  .../testing/selftests/kvm/include/kvm_util.h  |  4 ++
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 17 ++++++
>  3 files changed, 26 insertions(+), 47 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index b2e07a3173b2..73f679bbf082 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -268,10 +268,8 @@ static struct kvm_vm *create_vm(enum vm_guest_mode mode, uint32_t vcpuid,
>  static void run_test(enum vm_guest_mode mode, unsigned long iterations,
>  		     unsigned long interval, uint64_t phys_offset)
>  {
> -	unsigned int guest_pa_bits, guest_page_shift;
>  	pthread_t vcpu_thread;
>  	struct kvm_vm *vm;
> -	uint64_t max_gfn;
>  	unsigned long *bmap;
>  
>  	/*
> @@ -286,54 +284,13 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
>  		       2ul << (DIRTY_MEM_BITS - PAGE_SHIFT_4K),
>  		       guest_code);
>  
> -	switch (mode) {
> -	case VM_MODE_P52V48_4K:
> -	case VM_MODE_PXXV48_4K:
> -		guest_pa_bits = 52;
> -		guest_page_shift = 12;
> -		break;
> -	case VM_MODE_P52V48_64K:
> -		guest_pa_bits = 52;
> -		guest_page_shift = 16;
> -		break;
> -	case VM_MODE_P48V48_4K:
> -		guest_pa_bits = 48;
> -		guest_page_shift = 12;
> -		break;
> -	case VM_MODE_P48V48_64K:
> -		guest_pa_bits = 48;
> -		guest_page_shift = 16;
> -		break;
> -	case VM_MODE_P40V48_4K:
> -		guest_pa_bits = 40;
> -		guest_page_shift = 12;
> -		break;
> -	case VM_MODE_P40V48_64K:
> -		guest_pa_bits = 40;
> -		guest_page_shift = 16;
> -		break;
> -	default:
> -		TEST_ASSERT(false, "Unknown guest mode, mode: 0x%x", mode);
> -	}
> -
> -	DEBUG("Testing guest mode: %s\n", vm_guest_mode_string(mode));
> -
> -#ifdef __x86_64__
> -	/*
> -	 * FIXME
> -	 * The x86_64 kvm selftests framework currently only supports a
> -	 * single PML4 which restricts the number of physical address
> -	 * bits we can change to 39.
> -	 */
> -	guest_pa_bits = 39;
> -#endif
> -	max_gfn = (1ul << (guest_pa_bits - guest_page_shift)) - 1;
> -	guest_page_size = (1ul << guest_page_shift);
> +	guest_page_size = vm_get_page_size(vm);
>  	/*
>  	 * A little more than 1G of guest page sized pages.  Cover the
>  	 * case where the size is not aligned to 64 pages.
>  	 */
> -	guest_num_pages = (1ul << (DIRTY_MEM_BITS - guest_page_shift)) + 16;
> +	guest_num_pages = (1ul << (DIRTY_MEM_BITS -
> +				   vm_get_page_shift(vm))) + 16;
>  #ifdef __s390x__
>  	/* Round up to multiple of 1M (segment size) */
>  	guest_num_pages = (guest_num_pages + 0xff) & ~0xffUL;
> @@ -343,7 +300,8 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
>  			 !!((guest_num_pages * guest_page_size) % host_page_size);
>  
>  	if (!phys_offset) {
> -		guest_test_phys_mem = (max_gfn - guest_num_pages) * guest_page_size;
> +		guest_test_phys_mem = (vm_get_max_gfn(vm) -
> +				       guest_num_pages) * guest_page_size;
>  		guest_test_phys_mem &= ~(host_page_size - 1);
>  	} else {
>  		guest_test_phys_mem = phys_offset;
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index 1c700c6b31b5..0d65fc676182 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -155,6 +155,10 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code);
>  
>  bool vm_is_unrestricted_guest(struct kvm_vm *vm);
>  
> +unsigned int vm_get_page_size(struct kvm_vm *vm);
> +unsigned int vm_get_page_shift(struct kvm_vm *vm);
> +unsigned int vm_get_max_gfn(struct kvm_vm *vm);
> +
>  struct kvm_userspace_memory_region *
>  kvm_userspace_memory_region_find(struct kvm_vm *vm, uint64_t start,
>  				 uint64_t end);
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 8c6f872a8793..cf39643ff2c7 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -137,6 +137,8 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
>  {
>  	struct kvm_vm *vm;
>  
> +	DEBUG("Testing guest mode: %s\n", vm_guest_mode_string(mode));
> +
>  	vm = calloc(1, sizeof(*vm));
>  	TEST_ASSERT(vm != NULL, "Insufficient Memory");
>  
> @@ -1662,3 +1664,18 @@ bool vm_is_unrestricted_guest(struct kvm_vm *vm)
>  
>  	return val == 'Y';
>  }
> +
> +unsigned int vm_get_page_size(struct kvm_vm *vm)
> +{
> +	return vm->page_size;
> +}
> +
> +unsigned int vm_get_page_shift(struct kvm_vm *vm)
> +{
> +	return vm->page_shift;
> +}

We could get by with just one of the above two, but whatever
> +
> +unsigned int vm_get_max_gfn(struct kvm_vm *vm)
> +{
> +	return vm->max_gfn;
> +}
> -- 
> 2.21.0
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH 2/4] KVM: selftests: Create VM earlier for dirty log test

[Andrew Jones]

On Tue, Aug 27, 2019 at 09:10:13PM +0800, Peter Xu wrote:
> Since we've just removed the dependency of vm type in previous patch,
> now we can create the vm much earlier.  Note that to move it earlier
> we used an approximation of number of extra pages but it should be
> fine.
> 
> This prepares for the follow up patches to finally remove the
> duplication of guest mode parsings.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  tools/testing/selftests/kvm/dirty_log_test.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
> index 424efcf8c734..040952f3d4ad 100644
> --- a/tools/testing/selftests/kvm/dirty_log_test.c
> +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> @@ -264,6 +264,9 @@ static struct kvm_vm *create_vm(enum vm_guest_mode mode, uint32_t vcpuid,
>  	return vm;
>  }
>  
> +#define DIRTY_MEM_BITS 30 /* 1G */
> +#define PAGE_SHIFT_4K  12
> +
>  static void run_test(enum vm_guest_mode mode, unsigned long iterations,
>  		     unsigned long interval, uint64_t phys_offset)
>  {
> @@ -273,6 +276,18 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
>  	uint64_t max_gfn;
>  	unsigned long *bmap;
>  
> +	/*
> +	 * We reserve page table for 2 times of extra dirty mem which
> +	 * will definitely cover the original (1G+) test range.  Here
> +	 * we do the calculation with 4K page size which is the
> +	 * smallest so the page number will be enough for all archs
> +	 * (e.g., 64K page size guest will need even less memory for
> +	 * page tables).
> +	 */
> +	vm = create_vm(mode, VCPU_ID,
> +		       2ul << (DIRTY_MEM_BITS - PAGE_SHIFT_4K),
> +		       guest_code);
> +
>  	switch (mode) {
>  	case VM_MODE_P52V48_4K:
>  		guest_pa_bits = 52;
> @@ -319,7 +334,7 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
>  	 * A little more than 1G of guest page sized pages.  Cover the
>  	 * case where the size is not aligned to 64 pages.
>  	 */
> -	guest_num_pages = (1ul << (30 - guest_page_shift)) + 16;
> +	guest_num_pages = (1ul << (DIRTY_MEM_BITS - guest_page_shift)) + 16;
>  #ifdef __s390x__
>  	/* Round up to multiple of 1M (segment size) */
>  	guest_num_pages = (guest_num_pages + 0xff) & ~0xffUL;
> @@ -345,8 +360,6 @@ static void run_test(enum vm_guest_mode mode, unsigned long iterations,
>  	bmap = bitmap_alloc(host_num_pages);
>  	host_bmap_track = bitmap_alloc(host_num_pages);
>  
> -	vm = create_vm(mode, VCPU_ID, guest_num_pages, guest_code);
> -
>  #ifdef USE_CLEAR_DIRTY_LOG
>  	struct kvm_enable_cap cap = {};
>  
> -- 
> 2.21.0
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH 0/4] KVM: selftests: Introduce VM_MODE_PXXV48_4K

[Andrew Jones]

On Tue, Aug 27, 2019 at 09:10:11PM +0800, Peter Xu wrote:
> The work is based on Thomas's s390 port for dirty_log_test.
> 
> This series originates from "[PATCH] KVM: selftests: Detect max PA
> width from cpuid" [1] and one of Drew's comments - instead of keeping
> the hackish line to overwrite guest_pa_bits all the time, this series
> introduced the new mode VM_MODE_PXXV48_4K for x86_64 platform.
> 
> The major issue is that even all the x86_64 kvm selftests are
> currently using the guest mode VM_MODE_P52V48_4K, many x86_64 hosts
> are not using 52 bits PA (and in most cases, far less).  If with luck
> we could be having 48 bits hosts, but it's more adhoc (I've observed 3
> x86_64 systems, they are having different PA width of 36, 39, 48).  I
> am not sure whether this is happening to the other archs as well, but
> it probably makes sense to bring the x86_64 tests to the real world on
> always using the correct PA bits.
> 
> A side effect of this series is that it will also fix the crash we've
> encountered on Xeon E3-1220 as mentioned [1] due to the
> differenciation of PA width.
> 
> With [1], we've observed AMD host issues when with NPT=off.  However a
> funny fact is that after I reworked into this series, the tests can
> instead pass on both NPT=on/off.  It could be that the series changes
> vm->pa_bits or other fields so something was affected.  I didn't dig
> more on that though, considering we should not lose anything.
> 
> Any kind of smoke test would be greatly welcomed (especially on s390
> or ARM).  Same to comments.  Thanks,
> 

The patches didn't apply cleanly for me on 9e8312f5e160, but once I got
them applied I was able to run the aarch64 tests.

> [1] https://lkml.org/lkml/2019/8/26/141
> 
> Peter Xu (4):
>   KVM: selftests: Move vm type into _vm_create() internally
>   KVM: selftests: Create VM earlier for dirty log test
>   KVM: selftests: Introduce VM_MODE_PXXV48_4K
>   KVM: selftests: Remove duplicate guest mode handling
> 
>  tools/testing/selftests/kvm/dirty_log_test.c  | 78 +++++--------------
>  .../testing/selftests/kvm/include/kvm_util.h  | 17 +++-
>  .../selftests/kvm/lib/aarch64/processor.c     |  3 +
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 77 ++++++++++++++----
>  .../selftests/kvm/lib/x86_64/processor.c      |  8 +-
>  5 files changed, 107 insertions(+), 76 deletions(-)
> 
> -- 
> 2.21.0
>

Thanks,
drew

Re: [PATCH 0/4] KVM: selftests: Introduce VM_MODE_PXXV48_4K

[Andrew Jones]

On Wed, Aug 28, 2019 at 01:51:06PM +0200, Andrew Jones wrote:
> On Tue, Aug 27, 2019 at 09:10:11PM +0800, Peter Xu wrote:
> > The work is based on Thomas's s390 port for dirty_log_test.
> > 
> > This series originates from "[PATCH] KVM: selftests: Detect max PA
> > width from cpuid" [1] and one of Drew's comments - instead of keeping
> > the hackish line to overwrite guest_pa_bits all the time, this series
> > introduced the new mode VM_MODE_PXXV48_4K for x86_64 platform.
> > 
> > The major issue is that even all the x86_64 kvm selftests are
> > currently using the guest mode VM_MODE_P52V48_4K, many x86_64 hosts
> > are not using 52 bits PA (and in most cases, far less).  If with luck
> > we could be having 48 bits hosts, but it's more adhoc (I've observed 3
> > x86_64 systems, they are having different PA width of 36, 39, 48).  I
> > am not sure whether this is happening to the other archs as well, but
> > it probably makes sense to bring the x86_64 tests to the real world on
> > always using the correct PA bits.
> > 
> > A side effect of this series is that it will also fix the crash we've
> > encountered on Xeon E3-1220 as mentioned [1] due to the
> > differenciation of PA width.
> > 
> > With [1], we've observed AMD host issues when with NPT=off.  However a
> > funny fact is that after I reworked into this series, the tests can
> > instead pass on both NPT=on/off.  It could be that the series changes
> > vm->pa_bits or other fields so something was affected.  I didn't dig
> > more on that though, considering we should not lose anything.
> > 
> > Any kind of smoke test would be greatly welcomed (especially on s390
> > or ARM).  Same to comments.  Thanks,
> > 
> 
> The patches didn't apply cleanly for me on 9e8312f5e160, but once I got
> them applied I was able to run the aarch64 tests.

Oh, and after fixing 2/4 (vm->pa_bits) to fix compilation on aarch64 as
pointed out on that patch.

> 
> > [1] https://lkml.org/lkml/2019/8/26/141
> > 
> > Peter Xu (4):
> >   KVM: selftests: Move vm type into _vm_create() internally
> >   KVM: selftests: Create VM earlier for dirty log test
> >   KVM: selftests: Introduce VM_MODE_PXXV48_4K
> >   KVM: selftests: Remove duplicate guest mode handling
> > 
> >  tools/testing/selftests/kvm/dirty_log_test.c  | 78 +++++--------------
> >  .../testing/selftests/kvm/include/kvm_util.h  | 17 +++-
> >  .../selftests/kvm/lib/aarch64/processor.c     |  3 +
> >  tools/testing/selftests/kvm/lib/kvm_util.c    | 77 ++++++++++++++----
> >  .../selftests/kvm/lib/x86_64/processor.c      |  8 +-
> >  5 files changed, 107 insertions(+), 76 deletions(-)
> > 
> > -- 
> > 2.21.0
> >
> 
> Thanks,
> drew

Re: [PATCH 0/4] KVM: selftests: Introduce VM_MODE_PXXV48_4K

[Peter Xu]

On Wed, Aug 28, 2019 at 01:52:30PM +0200, Andrew Jones wrote:
> On Wed, Aug 28, 2019 at 01:51:06PM +0200, Andrew Jones wrote:
> > On Tue, Aug 27, 2019 at 09:10:11PM +0800, Peter Xu wrote:
> > > The work is based on Thomas's s390 port for dirty_log_test.

[1]

> > > 
> > > This series originates from "[PATCH] KVM: selftests: Detect max PA
> > > width from cpuid" [1] and one of Drew's comments - instead of keeping
> > > the hackish line to overwrite guest_pa_bits all the time, this series
> > > introduced the new mode VM_MODE_PXXV48_4K for x86_64 platform.
> > > 
> > > The major issue is that even all the x86_64 kvm selftests are
> > > currently using the guest mode VM_MODE_P52V48_4K, many x86_64 hosts
> > > are not using 52 bits PA (and in most cases, far less).  If with luck
> > > we could be having 48 bits hosts, but it's more adhoc (I've observed 3
> > > x86_64 systems, they are having different PA width of 36, 39, 48).  I
> > > am not sure whether this is happening to the other archs as well, but
> > > it probably makes sense to bring the x86_64 tests to the real world on
> > > always using the correct PA bits.
> > > 
> > > A side effect of this series is that it will also fix the crash we've
> > > encountered on Xeon E3-1220 as mentioned [1] due to the
> > > differenciation of PA width.
> > > 
> > > With [1], we've observed AMD host issues when with NPT=off.  However a
> > > funny fact is that after I reworked into this series, the tests can
> > > instead pass on both NPT=on/off.  It could be that the series changes
> > > vm->pa_bits or other fields so something was affected.  I didn't dig
> > > more on that though, considering we should not lose anything.
> > > 
> > > Any kind of smoke test would be greatly welcomed (especially on s390
> > > or ARM).  Same to comments.  Thanks,
> > > 
> > 
> > The patches didn't apply cleanly for me on 9e8312f5e160, but once I got
> > them applied I was able to run the aarch64 tests.

Right, because I applied Thomas's s390x port as base [1], considering
that that one should reach kvm/queue earlier (should be in the
submaintainer's tree and waiting for a pull).  Maybe I should post
against the current kvm/queue next time?  After all this series does
not modify anything of the s390x work so the conflict should be
trivial.

> 
> Oh, and after fixing 2/4 (vm->pa_bits) to fix compilation on aarch64 as
> pointed out on that patch.

Thanks for verifying and reviews!  Yes I'll fix that up.

Regards,

-- 
Peter Xu

Re: [PATCH 1/4] KVM: selftests: Move vm type into _vm_create() internally

[Peter Xu]

On Wed, Aug 28, 2019 at 01:23:57PM +0200, Andrew Jones wrote:

[...]

> 
> >  {
> >  	struct kvm_vm *vm;
> >  
> > @@ -139,8 +139,7 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
> >  	TEST_ASSERT(vm != NULL, "Insufficient Memory");
> >  
> >  	vm->mode = mode;
> > -	vm->type = type;
> > -	vm_open(vm, perm, type);
> > +	vm->type = 0;
> >  
> >  	/* Setup mode specific traits. */
> >  	switch (vm->mode) {
> > @@ -190,6 +189,13 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
> >  		TEST_ASSERT(false, "Unknown guest mode, mode: 0x%x", mode);
> >  	}
> >  
> > +#ifdef __aarch64__
> > +	if (vm->pa_bits != 40)
> > +		vm->type = KVM_VM_TYPE_ARM_IPA_SIZE(guest_pa_bits);
>                                                     ^^
>                                                     should be vm->pa_bits

Thanks for spotting it!  Fixed this and also the rest of indent
issues.

Regards,

-- 
Peter Xu

Re: [PATCH 3/4] KVM: selftests: Introduce VM_MODE_PXXV48_4K

[Peter Xu]

On Wed, Aug 28, 2019 at 01:41:04PM +0200, Andrew Jones wrote:

[...]

> > +#define DEBUG printf
> 
> If this is going to be some general thing, then maybe we should do it
> like this
> 
> #ifndef NDEBUG
> #define dprintf printf
> #endif

Ok.

> 
> 
> > +
> >  /* Minimum allocated guest virtual and physical addresses */
> >  #define KVM_UTIL_MIN_VADDR		0x2000
> >  
> > @@ -38,12 +40,19 @@ enum vm_guest_mode {
> >  	VM_MODE_P48V48_64K,
> >  	VM_MODE_P40V48_4K,
> >  	VM_MODE_P40V48_64K,
> > +	VM_MODE_PXXV48_4K,	/* For 48bits VA but ANY bits PA */
> >  	NUM_VM_MODES,
> >  };
> >  
> >  #ifdef __aarch64__
> >  #define VM_MODE_DEFAULT VM_MODE_P40V48_4K
> > -#else
> > +#endif
> > +
> > +#ifdef __x86_64__
> > +#define VM_MODE_DEFAULT VM_MODE_PXXV48_4K
> > +#endif
> > +
> > +#ifndef VM_MODE_DEFAULT
> >  #define VM_MODE_DEFAULT VM_MODE_P52V48_4K
> >  #endif
> 
> nit: how about
> 
> #if defined(__aarch64__)
> ...
> #elif defined(__x86_64__)
> ...
> #else
> ...
> #endif

Yes, better!

> 
> >  
> > diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > index 486400a97374..86036a59a668 100644
> > --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> > @@ -264,6 +264,9 @@ void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *ini
> >  	case VM_MODE_P52V48_4K:
> >  		TEST_ASSERT(false, "AArch64 does not support 4K sized pages "
> >  				   "with 52-bit physical address ranges");
> > +	case VM_MODE_PXXV48_4K:
> > +		TEST_ASSERT(false, "AArch64 does not support 4K sized pages "
> > +				   "with ANY-bit physical address ranges");
> >  	case VM_MODE_P52V48_64K:
> >  		tcr_el1 |= 1ul << 14; /* TG0 = 64KB */
> >  		tcr_el1 |= 6ul << 32; /* IPS = 52 bits */
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index 0c7c4368bc14..8c6f872a8793 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -8,6 +8,7 @@
> >  #include "test_util.h"
> >  #include "kvm_util.h"
> >  #include "kvm_util_internal.h"
> > +#include "processor.h"
> >  
> >  #include <assert.h>
> >  #include <sys/mman.h>
> > @@ -101,12 +102,13 @@ static void vm_open(struct kvm_vm *vm, int perm)
> >  }
> >  
> >  const char * const vm_guest_mode_string[] = {
> > -	"PA-bits:52, VA-bits:48, 4K pages",
> > -	"PA-bits:52, VA-bits:48, 64K pages",
> > -	"PA-bits:48, VA-bits:48, 4K pages",
> > -	"PA-bits:48, VA-bits:48, 64K pages",
> > -	"PA-bits:40, VA-bits:48, 4K pages",
> > -	"PA-bits:40, VA-bits:48, 64K pages",
> > +	"PA-bits:52,  VA-bits:48, 4K pages",
> > +	"PA-bits:52,  VA-bits:48, 64K pages",
> > +	"PA-bits:48,  VA-bits:48, 4K pages",
> > +	"PA-bits:48,  VA-bits:48, 64K pages",
> > +	"PA-bits:40,  VA-bits:48, 4K pages",
> > +	"PA-bits:40,  VA-bits:48, 64K pages",
> > +	"PA-bits:ANY, VA-bits:48, 4K pages",
> 
> nit: while formatting we could align the 'K's in the 64/4K column

Ok.

> 
> >  };
> >  _Static_assert(sizeof(vm_guest_mode_string)/sizeof(char *) == NUM_VM_MODES,
> >  	       "Missing new mode strings?");
> > @@ -185,6 +187,32 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages,
> >  		vm->page_size = 0x10000;
> >  		vm->page_shift = 16;
> >  		break;
> > +	case VM_MODE_PXXV48_4K:
> > +#ifdef __x86_64__
> > +		{
> > +			struct kvm_cpuid_entry2 *entry;
> > +
> > +			/* SDM 4.1.4 */
> > +			entry = kvm_get_supported_cpuid_entry(0x80000008);
> > +			if (entry) {
> > +				vm->pa_bits = entry->eax & 0xff;
> > +				vm->va_bits = (entry->eax >> 8) & 0xff;
> > +			} else {
> > +				vm->pa_bits = vm->va_bits = 32;
> > +			}
> > +			TEST_ASSERT(vm->va_bits == 48, "Linear address width "
> > +				    "(%d bits) not supported", vm->va_bits);
> > +			vm->pgtable_levels = 4;
> > +			vm->page_size = 0x1000;
> > +			vm->page_shift = 12;
> > +			DEBUG("Guest physical address width detected: %d\n",
> > +			      vm->pa_bits);
> > +		}
> 
> How about making this a function that lives in x86_64/processor.c?

Done.  Also I fixed up the overlooked PAE calculation which should be
36 rather than 32 bits PA.

> 
> > +#else
> > +		TEST_ASSERT(false, "VM_MODE_PXXV48_4K not supported on "
> > +			    "non-x86 platforms");
> 
> This should make the above aarch64_vcpu_setup() change unnecessary.

I tend to prefer keeping both because if some non-arm arch removed it
then aarch64 has another chance to assert.

Thanks,

-- 
Peter Xu

Re: [PATCH 4/4] KVM: selftests: Remove duplicate guest mode handling

[Peter Xu]

On Wed, Aug 28, 2019 at 01:46:13PM +0200, Andrew Jones wrote:

[...]

> > +unsigned int vm_get_page_size(struct kvm_vm *vm)
> > +{
> > +	return vm->page_size;
> > +}
> > +
> > +unsigned int vm_get_page_shift(struct kvm_vm *vm)
> > +{
> > +	return vm->page_shift;
> > +}
> 
> We could get by with just one of the above two, but whatever

Right... and imho if we export kvm_vm struct we don't even any
helpers. :) But I didn't touch that.

> > +
> > +unsigned int vm_get_max_gfn(struct kvm_vm *vm)
> > +{
> > +	return vm->max_gfn;
> > +}
> > -- 
> > 2.21.0
> >
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks!

-- 
Peter Xu

Re: [PATCH 4/4] KVM: selftests: Remove duplicate guest mode handling

[Andrew Jones]

On Thu, Aug 29, 2019 at 10:09:35AM +0800, Peter Xu wrote:
> On Wed, Aug 28, 2019 at 01:46:13PM +0200, Andrew Jones wrote:
> 
> [...]
> 
> > > +unsigned int vm_get_page_size(struct kvm_vm *vm)
> > > +{
> > > +	return vm->page_size;
> > > +}
> > > +
> > > +unsigned int vm_get_page_shift(struct kvm_vm *vm)
> > > +{
> > > +	return vm->page_shift;
> > > +}
> > 
> > We could get by with just one of the above two, but whatever
> 
> Right... and imho if we export kvm_vm struct we don't even any
> helpers. :) But I didn't touch that.

yeah, I'm starting to wonder if there's much value in keeping the vm and
vcpu structures private. I've already had a couple cases where I wanted
to write a quick+dirty test that needed the vcpu_fd, so I cheated and
included the internal header to get to it.

Thanks,
drew

> 
> > > +
> > > +unsigned int vm_get_max_gfn(struct kvm_vm *vm)
> > > +{
> > > +	return vm->max_gfn;
> > > +}
> > > -- 
> > > 2.21.0
> > >
> > 
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> Thanks!
> 
> -- 
> Peter Xu