[PATCH 0/4] kvm: selftests: aarch64: use struct kvm_vcpu_init

[PATCH 0/4] kvm: selftests: aarch64: use struct kvm_vcpu_init

[Andrew Jones]

aarch64 vcpu setup requires a vcpu init step that takes a kvm_vcpu_init
struct. So far we've just hard coded that to be one that requests no
features and always uses KVM_ARM_TARGET_GENERIC_V8 for the target. We
should have used the preferred target from the beginning, so we do that
now, and we also provide an API to unit tests to select a target of their
choosing and/or cpu features.

Switching to the preferred target fixes running on platforms that don't
like KVM_ARM_TARGET_GENERIC_V8. The new API will be made use of with
some coming unit tests.

Andrew Jones (4):
  kvm: selftests: rename vm_vcpu_add to vm_vcpu_add_memslots
  kvm: selftests: introduce vm_vcpu_add
  kvm: selftests: introduce aarch64_vcpu_setup
  kvm: selftests: introduce aarch64_vcpu_add_default

 .../selftests/kvm/include/aarch64/processor.h |  4 +++
 .../testing/selftests/kvm/include/kvm_util.h  |  5 +--
 .../selftests/kvm/lib/aarch64/processor.c     | 33 +++++++++++++++----
 tools/testing/selftests/kvm/lib/kvm_util.c    | 28 +++++++++++++---
 .../selftests/kvm/lib/x86_64/processor.c      |  2 +-
 .../testing/selftests/kvm/x86_64/evmcs_test.c |  2 +-
 .../kvm/x86_64/kvm_create_max_vcpus.c         |  2 +-
 tools/testing/selftests/kvm/x86_64/smm_test.c |  2 +-
 .../testing/selftests/kvm/x86_64/state_test.c |  2 +-
 9 files changed, 62 insertions(+), 18 deletions(-)

-- 
2.20.1

[PATCH 1/4] kvm: selftests: rename vm_vcpu_add to vm_vcpu_add_memslots

[Andrew Jones]

This frees up the name vm_vcpu_add for another use.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 tools/testing/selftests/kvm/include/kvm_util.h       |  4 ++--
 tools/testing/selftests/kvm/lib/aarch64/processor.c  |  2 +-
 tools/testing/selftests/kvm/lib/kvm_util.c           | 12 +++++++-----
 tools/testing/selftests/kvm/lib/x86_64/processor.c   |  2 +-
 tools/testing/selftests/kvm/x86_64/evmcs_test.c      |  2 +-
 .../selftests/kvm/x86_64/kvm_create_max_vcpus.c      |  2 +-
 tools/testing/selftests/kvm/x86_64/smm_test.c        |  2 +-
 tools/testing/selftests/kvm/x86_64/state_test.c      |  2 +-
 8 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index 8c6b9619797d..4e92f34cf46a 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -88,8 +88,8 @@ int _vcpu_ioctl(struct kvm_vm *vm, uint32_t vcpuid, unsigned long ioctl,
 		void *arg);
 void vm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
 void vm_mem_region_set_flags(struct kvm_vm *vm, uint32_t slot, uint32_t flags);
-void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
-		 int gdt_memslot);
+void vm_vcpu_add_memslots(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
+			  int gdt_memslot);
 vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min,
 			  uint32_t data_memslot, uint32_t pgd_memslot);
 void virt_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index 19e667911496..713a0e6b0e08 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -243,7 +243,7 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
 	uint64_t stack_vaddr = vm_vaddr_alloc(vm, stack_size,
 					DEFAULT_ARM64_GUEST_STACK_VADDR_MIN, 0, 0);
 
-	vm_vcpu_add(vm, vcpuid, 0, 0);
+	vm_vcpu_add_memslots(vm, vcpuid, 0, 0);
 
 	set_reg(vm, vcpuid, ARM64_CORE_REG(sp_el1), stack_vaddr + stack_size);
 	set_reg(vm, vcpuid, ARM64_CORE_REG(regs.pc), (uint64_t)guest_code);
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index e9113857f44e..937292dca81b 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -756,21 +756,23 @@ static int vcpu_mmap_sz(void)
 }
 
 /*
- * VM VCPU Add
+ * VM VCPU Add with provided memslots
  *
  * Input Args:
  *   vm - Virtual Machine
  *   vcpuid - VCPU ID
+ *   pgd_memslot - Memory region slot for new virtual translation tables
+ *   gdt_memslot - Memory region slot for data pages
  *
  * Output Args: None
  *
  * Return: None
  *
- * Creates and adds to the VM specified by vm and virtual CPU with
- * the ID given by vcpuid.
+ * Adds a virtual CPU to the VM specified by vm with the ID given by vcpuid
+ * and then sets it up with vcpu_setup() and the provided memslots.
  */
-void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
-		 int gdt_memslot)
+void vm_vcpu_add_memslots(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
+			  int gdt_memslot)
 {
 	struct vcpu *vcpu;
 
diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
index dc7fae9fa424..7779cdcc9159 100644
--- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
+++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
@@ -658,7 +658,7 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
 				     DEFAULT_GUEST_STACK_VADDR_MIN, 0, 0);
 
 	/* Create VCPU */
-	vm_vcpu_add(vm, vcpuid, 0, 0);
+	vm_vcpu_add_memslots(vm, vcpuid, 0, 0);
 
 	/* Setup guest general purpose registers */
 	vcpu_regs_get(vm, vcpuid, &regs);
diff --git a/tools/testing/selftests/kvm/x86_64/evmcs_test.c b/tools/testing/selftests/kvm/x86_64/evmcs_test.c
index 36669684eca5..f4bce50ded95 100644
--- a/tools/testing/selftests/kvm/x86_64/evmcs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/evmcs_test.c
@@ -149,7 +149,7 @@ int main(int argc, char *argv[])
 
 		/* Restore state in a new VM.  */
 		kvm_vm_restart(vm, O_RDWR);
-		vm_vcpu_add(vm, VCPU_ID, 0, 0);
+		vm_vcpu_add_memslots(vm, VCPU_ID, 0, 0);
 		vcpu_set_cpuid(vm, VCPU_ID, kvm_get_supported_cpuid());
 		vcpu_load_state(vm, VCPU_ID, state);
 		run = vcpu_state(vm, VCPU_ID);
diff --git a/tools/testing/selftests/kvm/x86_64/kvm_create_max_vcpus.c b/tools/testing/selftests/kvm/x86_64/kvm_create_max_vcpus.c
index 50e92996f918..ded4e0272f8a 100644
--- a/tools/testing/selftests/kvm/x86_64/kvm_create_max_vcpus.c
+++ b/tools/testing/selftests/kvm/x86_64/kvm_create_max_vcpus.c
@@ -34,7 +34,7 @@ void test_vcpu_creation(int first_vcpu_id, int num_vcpus)
 		int vcpu_id = first_vcpu_id + i;
 
 		/* This asserts that the vCPU was created. */
-		vm_vcpu_add(vm, vcpu_id, 0, 0);
+		vm_vcpu_add_memslots(vm, vcpu_id, 0, 0);
 	}
 
 	kvm_vm_free(vm);
diff --git a/tools/testing/selftests/kvm/x86_64/smm_test.c b/tools/testing/selftests/kvm/x86_64/smm_test.c
index fb8086964d83..b31d8c29b215 100644
--- a/tools/testing/selftests/kvm/x86_64/smm_test.c
+++ b/tools/testing/selftests/kvm/x86_64/smm_test.c
@@ -145,7 +145,7 @@ int main(int argc, char *argv[])
 		state = vcpu_save_state(vm, VCPU_ID);
 		kvm_vm_release(vm);
 		kvm_vm_restart(vm, O_RDWR);
-		vm_vcpu_add(vm, VCPU_ID, 0, 0);
+		vm_vcpu_add_memslots(vm, VCPU_ID, 0, 0);
 		vcpu_set_cpuid(vm, VCPU_ID, kvm_get_supported_cpuid());
 		vcpu_load_state(vm, VCPU_ID, state);
 		run = vcpu_state(vm, VCPU_ID);
diff --git a/tools/testing/selftests/kvm/x86_64/state_test.c b/tools/testing/selftests/kvm/x86_64/state_test.c
index e0a3c0204b7c..b564ebdd34ca 100644
--- a/tools/testing/selftests/kvm/x86_64/state_test.c
+++ b/tools/testing/selftests/kvm/x86_64/state_test.c
@@ -182,7 +182,7 @@ int main(int argc, char *argv[])
 
 		/* Restore state in a new VM.  */
 		kvm_vm_restart(vm, O_RDWR);
-		vm_vcpu_add(vm, VCPU_ID, 0, 0);
+		vm_vcpu_add_memslots(vm, VCPU_ID, 0, 0);
 		vcpu_set_cpuid(vm, VCPU_ID, kvm_get_supported_cpuid());
 		vcpu_load_state(vm, VCPU_ID, state);
 		run = vcpu_state(vm, VCPU_ID);
-- 
2.20.1

[PATCH 2/4] kvm: selftests: introduce vm_vcpu_add

[Andrew Jones]

vm_vcpu_add() just adds a vcpu to the vm, but doesn't do any
additional vcpu setup.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 .../testing/selftests/kvm/include/kvm_util.h  |  1 +
 tools/testing/selftests/kvm/lib/kvm_util.c    | 32 +++++++++++++++----
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index 4e92f34cf46a..32fabbc98803 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -88,6 +88,7 @@ int _vcpu_ioctl(struct kvm_vm *vm, uint32_t vcpuid, unsigned long ioctl,
 		void *arg);
 void vm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
 void vm_mem_region_set_flags(struct kvm_vm *vm, uint32_t slot, uint32_t flags);
+void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid);
 void vm_vcpu_add_memslots(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
 			  int gdt_memslot);
 vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min,
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 937292dca81b..ae6d4b274ddd 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -756,23 +756,20 @@ static int vcpu_mmap_sz(void)
 }
 
 /*
- * VM VCPU Add with provided memslots
+ * VM VCPU Add
  *
  * Input Args:
  *   vm - Virtual Machine
  *   vcpuid - VCPU ID
- *   pgd_memslot - Memory region slot for new virtual translation tables
- *   gdt_memslot - Memory region slot for data pages
  *
  * Output Args: None
  *
  * Return: None
  *
- * Adds a virtual CPU to the VM specified by vm with the ID given by vcpuid
- * and then sets it up with vcpu_setup() and the provided memslots.
+ * Adds a virtual CPU to the VM specified by vm with the ID given by vcpuid.
+ * No additional VCPU setup is done.
  */
-void vm_vcpu_add_memslots(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
-			  int gdt_memslot)
+void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid)
 {
 	struct vcpu *vcpu;
 
@@ -806,7 +803,28 @@ void vm_vcpu_add_memslots(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
 		vm->vcpu_head->prev = vcpu;
 	vcpu->next = vm->vcpu_head;
 	vm->vcpu_head = vcpu;
+}
 
+/*
+ * VM VCPU Add with provided memslots
+ *
+ * Input Args:
+ *   vm - Virtual Machine
+ *   vcpuid - VCPU ID
+ *   pgd_memslot - Memory region slot for new virtual translation tables
+ *   gdt_memslot - Memory region slot for data pages
+ *
+ * Output Args: None
+ *
+ * Return: None
+ *
+ * Adds a virtual CPU the VM specified by vm with the ID given by vcpuid
+ * and then sets it up with vcpu_setup() and the provided memslots.
+ */
+void vm_vcpu_add_memslots(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
+			  int gdt_memslot)
+{
+	vm_vcpu_add(vm, vcpuid);
 	vcpu_setup(vm, vcpuid, pgd_memslot, gdt_memslot);
 }
 
-- 
2.20.1

[PATCH 3/4] kvm: selftests: introduce aarch64_vcpu_setup

[Andrew Jones]

This allows aarch64 tests to run on more targets, such as the Arm
simulator that doesn't like KVM_ARM_TARGET_GENERIC_V8. And it also
allows aarch64 tests to provide vcpu features in struct kvm_vcpu_init.
Additionally it drops the unused memslot parameters.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 .../selftests/kvm/include/aarch64/processor.h |  2 ++
 .../selftests/kvm/lib/aarch64/processor.c     | 22 ++++++++++++++-----
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
index 9ef2ab1a0c08..37f8129e1ea9 100644
--- a/tools/testing/selftests/kvm/include/aarch64/processor.h
+++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
@@ -52,4 +52,6 @@ static inline void set_reg(struct kvm_vm *vm, uint32_t vcpuid, uint64_t id, uint
 	vcpu_ioctl(vm, vcpuid, KVM_SET_ONE_REG, &reg);
 }
 
+void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *init);
+
 #endif /* SELFTEST_KVM_PROCESSOR_H */
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index 713a0e6b0e08..e18da35815f2 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -249,14 +249,21 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
 	set_reg(vm, vcpuid, ARM64_CORE_REG(regs.pc), (uint64_t)guest_code);
 }
 
-void vcpu_setup(struct kvm_vm *vm, int vcpuid, int pgd_memslot, int gdt_memslot)
+void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *init)
 {
-	struct kvm_vcpu_init init;
+	struct kvm_vcpu_init default_init = { .target = -1, };
 	uint64_t sctlr_el1, tcr_el1;
 
-	memset(&init, 0, sizeof(init));
-	init.target = KVM_ARM_TARGET_GENERIC_V8;
-	vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_INIT, &init);
+	if (!init)
+		init = &default_init;
+
+	if (init->target == -1) {
+		struct kvm_vcpu_init preferred;
+		vm_ioctl(vm, KVM_ARM_PREFERRED_TARGET, &preferred);
+		init->target = preferred.target;
+	}
+
+	vcpu_ioctl(vm, vcpuid, KVM_ARM_VCPU_INIT, init);
 
 	/*
 	 * Enable FP/ASIMD to avoid trapping when accessing Q0-Q15
@@ -306,6 +313,11 @@ void vcpu_setup(struct kvm_vm *vm, int vcpuid, int pgd_memslot, int gdt_memslot)
 	set_reg(vm, vcpuid, ARM64_SYS_REG(TTBR0_EL1), vm->pgd);
 }
 
+void vcpu_setup(struct kvm_vm *vm, int vcpuid, int pgd_memslot, int gdt_memslot)
+{
+	aarch64_vcpu_setup(vm, vcpuid, NULL);
+}
+
 void vcpu_dump(FILE *stream, struct kvm_vm *vm, uint32_t vcpuid, uint8_t indent)
 {
 	uint64_t pstate, pc;
-- 
2.20.1

[PATCH 4/4] kvm: selftests: introduce aarch64_vcpu_add_default

[Andrew Jones]

This is the same as vm_vcpu_add_default, but it also takes a
kvm_vcpu_init struct pointer.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 .../testing/selftests/kvm/include/aarch64/processor.h |  2 ++
 tools/testing/selftests/kvm/lib/aarch64/processor.c   | 11 +++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/aarch64/processor.h b/tools/testing/selftests/kvm/include/aarch64/processor.h
index 37f8129e1ea9..b7fa0c8551db 100644
--- a/tools/testing/selftests/kvm/include/aarch64/processor.h
+++ b/tools/testing/selftests/kvm/include/aarch64/processor.h
@@ -53,5 +53,7 @@ static inline void set_reg(struct kvm_vm *vm, uint32_t vcpuid, uint64_t id, uint
 }
 
 void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *init);
+void aarch64_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid,
+			      struct kvm_vcpu_init *init, void *guest_code);
 
 #endif /* SELFTEST_KVM_PROCESSOR_H */
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index e18da35815f2..284dfd63b65b 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -235,7 +235,8 @@ struct kvm_vm *vm_create_default(uint32_t vcpuid, uint64_t extra_mem_pages,
 	return vm;
 }
 
-void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
+void aarch64_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid,
+			      struct kvm_vcpu_init *init, void *guest_code)
 {
 	size_t stack_size = vm->page_size == 4096 ?
 					DEFAULT_STACK_PGS * vm->page_size :
@@ -243,12 +244,18 @@ void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
 	uint64_t stack_vaddr = vm_vaddr_alloc(vm, stack_size,
 					DEFAULT_ARM64_GUEST_STACK_VADDR_MIN, 0, 0);
 
-	vm_vcpu_add_memslots(vm, vcpuid, 0, 0);
+	vm_vcpu_add(vm, vcpuid);
+	aarch64_vcpu_setup(vm, vcpuid, init);
 
 	set_reg(vm, vcpuid, ARM64_CORE_REG(sp_el1), stack_vaddr + stack_size);
 	set_reg(vm, vcpuid, ARM64_CORE_REG(regs.pc), (uint64_t)guest_code);
 }
 
+void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code)
+{
+	aarch64_vcpu_add_default(vm, vcpuid, NULL, guest_code);
+}
+
 void aarch64_vcpu_setup(struct kvm_vm *vm, int vcpuid, struct kvm_vcpu_init *init)
 {
 	struct kvm_vcpu_init default_init = { .target = -1, };
-- 
2.20.1

Re: [PATCH 2/4] kvm: selftests: introduce vm_vcpu_add

[Peter Xu]

On Thu, May 23, 2019 at 02:57:54PM +0200, Andrew Jones wrote:
> vm_vcpu_add() just adds a vcpu to the vm, but doesn't do any
> additional vcpu setup.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  .../testing/selftests/kvm/include/kvm_util.h  |  1 +
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 32 +++++++++++++++----
>  2 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index 4e92f34cf46a..32fabbc98803 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -88,6 +88,7 @@ int _vcpu_ioctl(struct kvm_vm *vm, uint32_t vcpuid, unsigned long ioctl,
>  		void *arg);
>  void vm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
>  void vm_mem_region_set_flags(struct kvm_vm *vm, uint32_t slot, uint32_t flags);
> +void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid);
>  void vm_vcpu_add_memslots(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
>  			  int gdt_memslot);
>  vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min,
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 937292dca81b..ae6d4b274ddd 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -756,23 +756,20 @@ static int vcpu_mmap_sz(void)
>  }
>  
>  /*
> - * VM VCPU Add with provided memslots
> + * VM VCPU Add
>   *
>   * Input Args:
>   *   vm - Virtual Machine
>   *   vcpuid - VCPU ID
> - *   pgd_memslot - Memory region slot for new virtual translation tables
> - *   gdt_memslot - Memory region slot for data pages

Would it make sense to squash the first two patches together?  They
are somehow related, and also no lines will be added and quickly removed.

Nitpicking on the name: vm_vcpu_add_memslots() makes me feel like
"vcpu is adding memslots" rather than adding vcpu itself.  How about
vm_vcpu_add_with_memslots()?

Thanks,

>   *
>   * Output Args: None
>   *
>   * Return: None
>   *
> - * Adds a virtual CPU to the VM specified by vm with the ID given by vcpuid
> - * and then sets it up with vcpu_setup() and the provided memslots.
> + * Adds a virtual CPU to the VM specified by vm with the ID given by vcpuid.
> + * No additional VCPU setup is done.
>   */
> -void vm_vcpu_add_memslots(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
> -			  int gdt_memslot)
> +void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid)
>  {
>  	struct vcpu *vcpu;
>  
> @@ -806,7 +803,28 @@ void vm_vcpu_add_memslots(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
>  		vm->vcpu_head->prev = vcpu;
>  	vcpu->next = vm->vcpu_head;
>  	vm->vcpu_head = vcpu;
> +}
>  
> +/*
> + * VM VCPU Add with provided memslots
> + *
> + * Input Args:
> + *   vm - Virtual Machine
> + *   vcpuid - VCPU ID
> + *   pgd_memslot - Memory region slot for new virtual translation tables
> + *   gdt_memslot - Memory region slot for data pages
> + *
> + * Output Args: None
> + *
> + * Return: None
> + *
> + * Adds a virtual CPU the VM specified by vm with the ID given by vcpuid
> + * and then sets it up with vcpu_setup() and the provided memslots.
> + */
> +void vm_vcpu_add_memslots(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
> +			  int gdt_memslot)
> +{
> +	vm_vcpu_add(vm, vcpuid);
>  	vcpu_setup(vm, vcpuid, pgd_memslot, gdt_memslot);
>  }
>  
> -- 
> 2.20.1
> 

Regards,

-- 
Peter Xu

Re: [PATCH 2/4] kvm: selftests: introduce vm_vcpu_add

[Andrew Jones]

On Mon, May 27, 2019 at 02:35:52PM +0800, Peter Xu wrote:
> On Thu, May 23, 2019 at 02:57:54PM +0200, Andrew Jones wrote:
> > vm_vcpu_add() just adds a vcpu to the vm, but doesn't do any
> > additional vcpu setup.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  .../testing/selftests/kvm/include/kvm_util.h  |  1 +
> >  tools/testing/selftests/kvm/lib/kvm_util.c    | 32 +++++++++++++++----
> >  2 files changed, 26 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> > index 4e92f34cf46a..32fabbc98803 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> > @@ -88,6 +88,7 @@ int _vcpu_ioctl(struct kvm_vm *vm, uint32_t vcpuid, unsigned long ioctl,
> >  		void *arg);
> >  void vm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
> >  void vm_mem_region_set_flags(struct kvm_vm *vm, uint32_t slot, uint32_t flags);
> > +void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid);
> >  void vm_vcpu_add_memslots(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
> >  			  int gdt_memslot);
> >  vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min,
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index 937292dca81b..ae6d4b274ddd 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -756,23 +756,20 @@ static int vcpu_mmap_sz(void)
> >  }
> >  
> >  /*
> > - * VM VCPU Add with provided memslots
> > + * VM VCPU Add
> >   *
> >   * Input Args:
> >   *   vm - Virtual Machine
> >   *   vcpuid - VCPU ID
> > - *   pgd_memslot - Memory region slot for new virtual translation tables
> > - *   gdt_memslot - Memory region slot for data pages
> 
> Would it make sense to squash the first two patches together?  They
> are somehow related, and also no lines will be added and quickly removed.

I had them separated for easier review. If Paolo wants to squash on merge,
I've got no problem with that.

> 
> Nitpicking on the name: vm_vcpu_add_memslots() makes me feel like
> "vcpu is adding memslots" rather than adding vcpu itself.  How about
> vm_vcpu_add_with_memslots()?

I can do that, although it's getting pretty verbose. Anybody else want
to vote on the name?

Thanks,
drew

Re: [PATCH 1/4] kvm: selftests: rename vm_vcpu_add to vm_vcpu_add_memslots

[Thomas Huth]

On 23/05/2019 14.57, Andrew Jones wrote:
> This frees up the name vm_vcpu_add for another use.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  tools/testing/selftests/kvm/include/kvm_util.h       |  4 ++--
>  tools/testing/selftests/kvm/lib/aarch64/processor.c  |  2 +-
>  tools/testing/selftests/kvm/lib/kvm_util.c           | 12 +++++++-----
>  tools/testing/selftests/kvm/lib/x86_64/processor.c   |  2 +-
>  tools/testing/selftests/kvm/x86_64/evmcs_test.c      |  2 +-
>  .../selftests/kvm/x86_64/kvm_create_max_vcpus.c      |  2 +-
>  tools/testing/selftests/kvm/x86_64/smm_test.c        |  2 +-
>  tools/testing/selftests/kvm/x86_64/state_test.c      |  2 +-
>  8 files changed, 15 insertions(+), 13 deletions(-)
[...]
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index e9113857f44e..937292dca81b 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -756,21 +756,23 @@ static int vcpu_mmap_sz(void)
>  }
>  
>  /*
> - * VM VCPU Add
> + * VM VCPU Add with provided memslots
>   *
>   * Input Args:
>   *   vm - Virtual Machine
>   *   vcpuid - VCPU ID
> + *   pgd_memslot - Memory region slot for new virtual translation tables
> + *   gdt_memslot - Memory region slot for data pages
>   *
>   * Output Args: None
>   *
>   * Return: None
>   *
> - * Creates and adds to the VM specified by vm and virtual CPU with
> - * the ID given by vcpuid.
> + * Adds a virtual CPU to the VM specified by vm with the ID given by vcpuid
> + * and then sets it up with vcpu_setup() and the provided memslots.
>   */
> -void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
> -		 int gdt_memslot)
> +void vm_vcpu_add_memslots(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
> +			  int gdt_memslot)

I think the naming and description of the function is somewhat
unfortunate now. The function is not really about memslots, but about
setting up some MMU tables in the memory (and for this you need a
memslot). So maybe rather name it vm_vcpu_add_with_mmu() or something
similar? Also it would be nice to give the reason for the memslots in
the comment before the function.

 Thomas

Re: [PATCH 1/4] kvm: selftests: rename vm_vcpu_add to vm_vcpu_add_memslots

[Andrew Jones]

On Mon, May 27, 2019 at 09:27:56AM +0200, Thomas Huth wrote:
> On 23/05/2019 14.57, Andrew Jones wrote:
> > This frees up the name vm_vcpu_add for another use.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  tools/testing/selftests/kvm/include/kvm_util.h       |  4 ++--
> >  tools/testing/selftests/kvm/lib/aarch64/processor.c  |  2 +-
> >  tools/testing/selftests/kvm/lib/kvm_util.c           | 12 +++++++-----
> >  tools/testing/selftests/kvm/lib/x86_64/processor.c   |  2 +-
> >  tools/testing/selftests/kvm/x86_64/evmcs_test.c      |  2 +-
> >  .../selftests/kvm/x86_64/kvm_create_max_vcpus.c      |  2 +-
> >  tools/testing/selftests/kvm/x86_64/smm_test.c        |  2 +-
> >  tools/testing/selftests/kvm/x86_64/state_test.c      |  2 +-
> >  8 files changed, 15 insertions(+), 13 deletions(-)
> [...]
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index e9113857f44e..937292dca81b 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -756,21 +756,23 @@ static int vcpu_mmap_sz(void)
> >  }
> >  
> >  /*
> > - * VM VCPU Add
> > + * VM VCPU Add with provided memslots
> >   *
> >   * Input Args:
> >   *   vm - Virtual Machine
> >   *   vcpuid - VCPU ID
> > + *   pgd_memslot - Memory region slot for new virtual translation tables
> > + *   gdt_memslot - Memory region slot for data pages
> >   *
> >   * Output Args: None
> >   *
> >   * Return: None
> >   *
> > - * Creates and adds to the VM specified by vm and virtual CPU with
> > - * the ID given by vcpuid.
> > + * Adds a virtual CPU to the VM specified by vm with the ID given by vcpuid
> > + * and then sets it up with vcpu_setup() and the provided memslots.
> >   */
> > -void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
> > -		 int gdt_memslot)
> > +void vm_vcpu_add_memslots(struct kvm_vm *vm, uint32_t vcpuid, int pgd_memslot,
> > +			  int gdt_memslot)
> 
> I think the naming and description of the function is somewhat
> unfortunate now. The function is not really about memslots, but about
> setting up some MMU tables in the memory (and for this you need a
> memslot). So maybe rather name it vm_vcpu_add_with_mmu() or something
> similar? Also it would be nice to give the reason for the memslots in
> the comment before the function.
>

Peter Xu suggested almost the same name, so I'll do that for a v2.
I'll add a couple more words to the comment too.

Thanks,
drew