[PATCH v2 1/2] kvm/kvm_util: add _vm_ioctl

[PATCH v2 1/2] kvm/kvm_util: add _vm_ioctl

[Emanuele Giuseppe Esposito]

As in kvm_ioctl and _kvm_ioctl, add
the respective _vm_ioctl for vm_ioctl.

_vm_ioctl invokes an ioctl using the vm fd,
leaving the caller to test the result.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 tools/testing/selftests/kvm/include/kvm_util.h | 1 +
 tools/testing/selftests/kvm/lib/kvm_util.c     | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index 2d7eb6989e83..d53a5f7cad61 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -133,6 +133,7 @@ void vcpu_ioctl(struct kvm_vm *vm, uint32_t vcpuid, unsigned long ioctl,
 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);
+int _vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg);
 void kvm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
 int _kvm_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);
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index e5fbf16f725b..b8849a1aca79 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1697,11 +1697,16 @@ void vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
 {
 	int ret;
 
-	ret = ioctl(vm->fd, cmd, arg);
+	ret = _vm_ioctl(vm, cmd, arg);
 	TEST_ASSERT(ret == 0, "vm ioctl %lu failed, rc: %i errno: %i (%s)",
 		cmd, ret, errno, strerror(errno));
 }
 
+int _vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
+{
+	return ioctl(vm->fd, cmd, arg);
+}
+
 /*
  * KVM system ioctl
  *
-- 
2.29.2

[PATCH v2 2/2] selftests/kvm: add set_boot_cpu_id test

[Emanuele Giuseppe Esposito]

Test for the KVM_SET_BOOT_CPU_ID ioctl.
Check that it correctly allows to change the BSP vcpu.

v1 -> v2:
- remove unnecessary printf
- move stage for loop inside run_vcpu
- test EBUSY when calling KVM_SET_BOOT_CPU_ID after vcpu
  creation and execution
- introduce _vm_ioctl

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/x86_64/set_boot_cpu_id.c    | 166 ++++++++++++++++++
 3 files changed, 168 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index 32b87cc77c8e..43b8aa82aefe 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -5,6 +5,7 @@
 /s390x/resets
 /s390x/sync_regs_test
 /x86_64/cr4_cpuid_sync_test
+/x86_64/set_boot_cpu_id
 /x86_64/debug_regs
 /x86_64/evmcs_test
 /x86_64/get_cpuid_test
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index a6d61f451f88..e7b62666e06e 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -39,6 +39,7 @@ LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c
 LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
 
 TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
+TEST_GEN_PROGS_x86_64 += x86_64/set_boot_cpu_id
 TEST_GEN_PROGS_x86_64 += x86_64/evmcs_test
 TEST_GEN_PROGS_x86_64 += x86_64/get_cpuid_test
 TEST_GEN_PROGS_x86_64 += x86_64/hyperv_cpuid
diff --git a/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c b/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
new file mode 100644
index 000000000000..12c558fc8074
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
@@ -0,0 +1,166 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test that KVM_SET_BOOT_CPU_ID works as intended
+ *
+ * Copyright (C) 2020, Red Hat, Inc.
+ */
+#define _GNU_SOURCE /* for program_invocation_name */
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+
+#include "test_util.h"
+#include "kvm_util.h"
+#include "processor.h"
+
+#define N_VCPU 2
+#define VCPU_ID0 0
+#define VCPU_ID1 1
+
+static uint32_t get_bsp_flag(void)
+{
+	return rdmsr(MSR_IA32_APICBASE) & MSR_IA32_APICBASE_BSP;
+}
+
+static void guest_bsp_vcpu(void *arg)
+{
+	GUEST_SYNC(1);
+
+	GUEST_ASSERT(get_bsp_flag() != 0);
+
+	GUEST_DONE();
+}
+
+static void guest_not_bsp_vcpu(void *arg)
+{
+	GUEST_SYNC(1);
+
+	GUEST_ASSERT(get_bsp_flag() == 0);
+
+	GUEST_DONE();
+}
+
+static void test_set_boot_busy(struct kvm_vm *vm)
+{
+	int res;
+
+	res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID0);
+	TEST_ASSERT(res == -1 && errno == EBUSY,
+			"KVM_SET_BOOT_CPU_ID set while running vm");
+}
+
+static void run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
+{
+	struct ucall uc;
+	int stage;
+
+	for (stage = 0; stage < 2; stage++) {
+
+		vcpu_run(vm, vcpuid);
+
+		switch (get_ucall(vm, vcpuid, &uc)) {
+		case UCALL_SYNC:
+			TEST_ASSERT(!strcmp((const char *)uc.args[0], "hello") &&
+					uc.args[1] == stage + 1,
+					"Stage %d: Unexpected register values vmexit, got %lx",
+					stage + 1, (ulong)uc.args[1]);
+			test_set_boot_busy(vm);
+			break;
+		case UCALL_DONE:
+			TEST_ASSERT(stage == 1,
+					"Expected GUEST_DONE in stage 2, got stage %d",
+					stage);
+			break;
+		case UCALL_ABORT:
+			TEST_ASSERT(false, "%s at %s:%ld\n\tvalues: %#lx, %#lx",
+						(const char *)uc.args[0], __FILE__,
+						uc.args[1], uc.args[2], uc.args[3]);
+		default:
+			TEST_ASSERT(false, "Unexpected exit: %s",
+					exit_reason_str(vcpu_state(vm, vcpuid)->exit_reason));
+		}
+	}
+}
+
+static struct kvm_vm *create_vm(void)
+{
+	struct kvm_vm *vm;
+	uint64_t vcpu_pages = (DEFAULT_STACK_PGS) * 2;
+	uint64_t extra_pg_pages = vcpu_pages / PTES_PER_MIN_PAGE * N_VCPU;
+	uint64_t pages = DEFAULT_GUEST_PHY_PAGES + vcpu_pages + extra_pg_pages;
+
+	pages = vm_adjust_num_guest_pages(VM_MODE_DEFAULT, pages);
+	vm = vm_create(VM_MODE_DEFAULT, pages, O_RDWR);
+
+	kvm_vm_elf_load(vm, program_invocation_name, 0, 0);
+	vm_create_irqchip(vm);
+
+	return vm;
+}
+
+static void add_x86_vcpu(struct kvm_vm *vm, uint32_t vcpuid, bool bsp_code)
+{
+	if (bsp_code)
+		vm_vcpu_add_default(vm, vcpuid, guest_bsp_vcpu);
+	else
+		vm_vcpu_add_default(vm, vcpuid, guest_not_bsp_vcpu);
+
+	vcpu_set_cpuid(vm, vcpuid, kvm_get_supported_cpuid());
+}
+
+static void run_vm_bsp(uint32_t bsp_vcpu)
+{
+	struct kvm_vm *vm;
+	bool is_bsp_vcpu1 = bsp_vcpu == VCPU_ID1;
+
+	vm = create_vm();
+
+	if (is_bsp_vcpu1)
+		vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
+
+	add_x86_vcpu(vm, VCPU_ID0, !is_bsp_vcpu1);
+	add_x86_vcpu(vm, VCPU_ID1, is_bsp_vcpu1);
+
+	run_vcpu(vm, VCPU_ID0);
+	run_vcpu(vm, VCPU_ID1);
+
+	kvm_vm_free(vm);
+}
+
+static void check_set_bsp_busy(void)
+{
+	struct kvm_vm *vm;
+	int res;
+
+	vm = create_vm();
+
+	add_x86_vcpu(vm, VCPU_ID0, true);
+	add_x86_vcpu(vm, VCPU_ID1, false);
+
+	res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
+	TEST_ASSERT(res == -1 && errno == EBUSY, "KVM_SET_BOOT_CPU_ID set after adding vcpu");
+
+	run_vcpu(vm, VCPU_ID0);
+	run_vcpu(vm, VCPU_ID1);
+
+	res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
+	TEST_ASSERT(res == -1 && errno == EBUSY, "KVM_SET_BOOT_CPU_ID set to a terminated vcpu");
+
+	kvm_vm_free(vm);
+}
+
+int main(int argc, char *argv[])
+{
+	if (!kvm_check_cap(KVM_CAP_SET_BOOT_CPU_ID)) {
+		print_skip("set_boot_cpu_id not available");
+		return 0;
+	}
+
+	run_vm_bsp(VCPU_ID0);
+	run_vm_bsp(VCPU_ID1);
+	run_vm_bsp(VCPU_ID0);
+
+	check_set_bsp_busy();
+}
-- 
2.29.2

Re: [PATCH v2 1/2] kvm/kvm_util: add _vm_ioctl

[Paolo Bonzini]

On 18/03/21 16:16, Emanuele Giuseppe Esposito wrote:
> As in kvm_ioctl and _kvm_ioctl, add
> the respective _vm_ioctl for vm_ioctl.
> 
> _vm_ioctl invokes an ioctl using the vm fd,
> leaving the caller to test the result.

Slightly better subject: "selftests/kvm: add _vm_ioctl".

Queued both, but next time please include a cover letter too.

Thanks!

Paolo

> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tools/testing/selftests/kvm/include/kvm_util.h | 1 +
>   tools/testing/selftests/kvm/lib/kvm_util.c     | 7 ++++++-
>   2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index 2d7eb6989e83..d53a5f7cad61 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -133,6 +133,7 @@ void vcpu_ioctl(struct kvm_vm *vm, uint32_t vcpuid, unsigned long ioctl,
>   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);
> +int _vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg);
>   void kvm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
>   int _kvm_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);
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index e5fbf16f725b..b8849a1aca79 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -1697,11 +1697,16 @@ void vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
>   {
>   	int ret;
>   
> -	ret = ioctl(vm->fd, cmd, arg);
> +	ret = _vm_ioctl(vm, cmd, arg);
>   	TEST_ASSERT(ret == 0, "vm ioctl %lu failed, rc: %i errno: %i (%s)",
>   		cmd, ret, errno, strerror(errno));
>   }
>   
> +int _vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
> +{
> +	return ioctl(vm->fd, cmd, arg);
> +}
> +
>   /*
>    * KVM system ioctl
>    *
> 

Re: [PATCH v2 2/2] selftests/kvm: add set_boot_cpu_id test

[Andrew Jones]

On Thu, Mar 18, 2021 at 04:16:24PM +0100, Emanuele Giuseppe Esposito wrote:
> Test for the KVM_SET_BOOT_CPU_ID ioctl.
> Check that it correctly allows to change the BSP vcpu.
> 
> v1 -> v2:
> - remove unnecessary printf
> - move stage for loop inside run_vcpu
> - test EBUSY when calling KVM_SET_BOOT_CPU_ID after vcpu
>   creation and execution
> - introduce _vm_ioctl

This information should be in the cover-letter. Or, for a single patch (no
cover-letter needed submission), then it should go below the '---' under
your s-o-b.

> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  tools/testing/selftests/kvm/.gitignore        |   1 +
>  tools/testing/selftests/kvm/Makefile          |   1 +
>  .../selftests/kvm/x86_64/set_boot_cpu_id.c    | 166 ++++++++++++++++++
>  3 files changed, 168 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
> 
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index 32b87cc77c8e..43b8aa82aefe 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -5,6 +5,7 @@
>  /s390x/resets
>  /s390x/sync_regs_test
>  /x86_64/cr4_cpuid_sync_test
> +/x86_64/set_boot_cpu_id
>  /x86_64/debug_regs
>  /x86_64/evmcs_test
>  /x86_64/get_cpuid_test
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index a6d61f451f88..e7b62666e06e 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -39,6 +39,7 @@ LIBKVM_aarch64 = lib/aarch64/processor.c lib/aarch64/ucall.c
>  LIBKVM_s390x = lib/s390x/processor.c lib/s390x/ucall.c lib/s390x/diag318_test_handler.c
>  
>  TEST_GEN_PROGS_x86_64 = x86_64/cr4_cpuid_sync_test
> +TEST_GEN_PROGS_x86_64 += x86_64/set_boot_cpu_id

We like the above two test lists (Makefile and .gitignore) to be in
alphabetical order.

>  TEST_GEN_PROGS_x86_64 += x86_64/evmcs_test
>  TEST_GEN_PROGS_x86_64 += x86_64/get_cpuid_test
>  TEST_GEN_PROGS_x86_64 += x86_64/hyperv_cpuid
> diff --git a/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c b/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
> new file mode 100644
> index 000000000000..12c558fc8074
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
> @@ -0,0 +1,166 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Test that KVM_SET_BOOT_CPU_ID works as intended
> + *
> + * Copyright (C) 2020, Red Hat, Inc.
> + */
> +#define _GNU_SOURCE /* for program_invocation_name */
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +
> +#include "test_util.h"
> +#include "kvm_util.h"
> +#include "processor.h"
> +
> +#define N_VCPU 2
> +#define VCPU_ID0 0
> +#define VCPU_ID1 1
> +
> +static uint32_t get_bsp_flag(void)
> +{
> +	return rdmsr(MSR_IA32_APICBASE) & MSR_IA32_APICBASE_BSP;
> +}
> +
> +static void guest_bsp_vcpu(void *arg)
> +{
> +	GUEST_SYNC(1);
> +
> +	GUEST_ASSERT(get_bsp_flag() != 0);
> +
> +	GUEST_DONE();
> +}
> +
> +static void guest_not_bsp_vcpu(void *arg)
> +{
> +	GUEST_SYNC(1);
> +
> +	GUEST_ASSERT(get_bsp_flag() == 0);
> +
> +	GUEST_DONE();
> +}
> +
> +static void test_set_boot_busy(struct kvm_vm *vm)
> +{
> +	int res;
> +
> +	res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID0);
> +	TEST_ASSERT(res == -1 && errno == EBUSY,
> +			"KVM_SET_BOOT_CPU_ID set while running vm");
> +}
> +
> +static void run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
> +{
> +	struct ucall uc;
> +	int stage;
> +
> +	for (stage = 0; stage < 2; stage++) {
> +
> +		vcpu_run(vm, vcpuid);
> +
> +		switch (get_ucall(vm, vcpuid, &uc)) {
> +		case UCALL_SYNC:
> +			TEST_ASSERT(!strcmp((const char *)uc.args[0], "hello") &&
> +					uc.args[1] == stage + 1,
> +					"Stage %d: Unexpected register values vmexit, got %lx",
> +					stage + 1, (ulong)uc.args[1]);
> +			test_set_boot_busy(vm);
> +			break;
> +		case UCALL_DONE:
> +			TEST_ASSERT(stage == 1,
> +					"Expected GUEST_DONE in stage 2, got stage %d",
> +					stage);
> +			break;
> +		case UCALL_ABORT:
> +			TEST_ASSERT(false, "%s at %s:%ld\n\tvalues: %#lx, %#lx",
> +						(const char *)uc.args[0], __FILE__,
> +						uc.args[1], uc.args[2], uc.args[3]);
> +		default:
> +			TEST_ASSERT(false, "Unexpected exit: %s",
> +					exit_reason_str(vcpu_state(vm, vcpuid)->exit_reason));
> +		}
> +	}
> +}
> +
> +static struct kvm_vm *create_vm(void)
> +{
> +	struct kvm_vm *vm;
> +	uint64_t vcpu_pages = (DEFAULT_STACK_PGS) * 2;
                                                    ^ should be N_VCPU

> +	uint64_t extra_pg_pages = vcpu_pages / PTES_PER_MIN_PAGE * N_VCPU;
                                                                   ^
                                                                   should be 2
> +	uint64_t pages = DEFAULT_GUEST_PHY_PAGES + vcpu_pages + extra_pg_pages;
> +
> +	pages = vm_adjust_num_guest_pages(VM_MODE_DEFAULT, pages);
> +	vm = vm_create(VM_MODE_DEFAULT, pages, O_RDWR);
> +
> +	kvm_vm_elf_load(vm, program_invocation_name, 0, 0);
> +	vm_create_irqchip(vm);
> +
> +	return vm;
> +}
> +
> +static void add_x86_vcpu(struct kvm_vm *vm, uint32_t vcpuid, bool bsp_code)
> +{
> +	if (bsp_code)
> +		vm_vcpu_add_default(vm, vcpuid, guest_bsp_vcpu);
> +	else
> +		vm_vcpu_add_default(vm, vcpuid, guest_not_bsp_vcpu);
> +
> +	vcpu_set_cpuid(vm, vcpuid, kvm_get_supported_cpuid());
> +}
> +
> +static void run_vm_bsp(uint32_t bsp_vcpu)

I think the 'bsp' suffixes and prefixes make the purpose of this function
and its argument more confusing. Just 

 static void run_vm(uint32_t vcpu)

would be more clear to me.

> +{
> +	struct kvm_vm *vm;
> +	bool is_bsp_vcpu1 = bsp_vcpu == VCPU_ID1;

Could add another define

 #define BSP_VCPU VCPU_ID1

And then instead of creating the above bool, just do

 if (vcpu == BSP_VCPU)

> +
> +	vm = create_vm();
> +
> +	if (is_bsp_vcpu1)
> +		vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);

Does this ioctl need to be called before creating the vcpus? The
documentation in Documentation/virt/kvm/api.rst doesn't say that.
If it can be called after creating the vcpus, then
vm_create_default_with_vcpus() can be used and there's no need
for the create_vm() and add_x86_vcpu() functions. Just use the
same guest code for both, but pass the cpu index to the guest
code function allowing something like

   if (cpu == BSP_VCPU)
	GUEST_ASSERT(get_bsp_flag() != 0);
   else
	GUEST_ASSERT(get_bsp_flag() == 0);


> +
> +	add_x86_vcpu(vm, VCPU_ID0, !is_bsp_vcpu1);
> +	add_x86_vcpu(vm, VCPU_ID1, is_bsp_vcpu1);
> +
> +	run_vcpu(vm, VCPU_ID0);
> +	run_vcpu(vm, VCPU_ID1);
> +
> +	kvm_vm_free(vm);
> +}
> +
> +static void check_set_bsp_busy(void)
> +{
> +	struct kvm_vm *vm;
> +	int res;
> +
> +	vm = create_vm();
> +
> +	add_x86_vcpu(vm, VCPU_ID0, true);
> +	add_x86_vcpu(vm, VCPU_ID1, false);
> +
> +	res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
> +	TEST_ASSERT(res == -1 && errno == EBUSY, "KVM_SET_BOOT_CPU_ID set after adding vcpu");
> +
> +	run_vcpu(vm, VCPU_ID0);
> +	run_vcpu(vm, VCPU_ID1);
> +
> +	res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
> +	TEST_ASSERT(res == -1 && errno == EBUSY, "KVM_SET_BOOT_CPU_ID set to a terminated vcpu");
> +
> +	kvm_vm_free(vm);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	if (!kvm_check_cap(KVM_CAP_SET_BOOT_CPU_ID)) {
> +		print_skip("set_boot_cpu_id not available");
> +		return 0;

Should be exit(KSFT_SKIP);

> +	}
> +
> +	run_vm_bsp(VCPU_ID0);
> +	run_vm_bsp(VCPU_ID1);
> +	run_vm_bsp(VCPU_ID0);
> +
> +	check_set_bsp_busy();

Don't you get a compiler warning here saying there's no return from a
function that returns int?

> +}
> -- 
> 2.29.2
> 

Thanks,
drew

Re: [PATCH v2 1/2] kvm/kvm_util: add _vm_ioctl

[Andrew Jones]

On Thu, Mar 18, 2021 at 04:16:23PM +0100, Emanuele Giuseppe Esposito wrote:
> As in kvm_ioctl and _kvm_ioctl, add
> the respective _vm_ioctl for vm_ioctl.
> 
> _vm_ioctl invokes an ioctl using the vm fd,
> leaving the caller to test the result.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  tools/testing/selftests/kvm/include/kvm_util.h | 1 +
>  tools/testing/selftests/kvm/lib/kvm_util.c     | 7 ++++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index 2d7eb6989e83..d53a5f7cad61 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -133,6 +133,7 @@ void vcpu_ioctl(struct kvm_vm *vm, uint32_t vcpuid, unsigned long ioctl,
>  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);
> +int _vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg);
>  void kvm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
>  int _kvm_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);
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index e5fbf16f725b..b8849a1aca79 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -1697,11 +1697,16 @@ void vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
>  {
>  	int ret;
>  
> -	ret = ioctl(vm->fd, cmd, arg);
> +	ret = _vm_ioctl(vm, cmd, arg);
>  	TEST_ASSERT(ret == 0, "vm ioctl %lu failed, rc: %i errno: %i (%s)",
>  		cmd, ret, errno, strerror(errno));
>  }
>  
> +int _vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
> +{
> +	return ioctl(vm->fd, cmd, arg);
> +}
> +
>  /*
>   * KVM system ioctl
>   *
> -- 
> 2.29.2
>

With the summary prefix change suggested by Paolo, or even better
'KVM: selftests:' since that's what the majority of patches in KVM
selftests have

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

Re: [PATCH v2 2/2] selftests/kvm: add set_boot_cpu_id test

[Emanuele Giuseppe Esposito]

On 18/03/2021 17:20, Andrew Jones wrote:
> On Thu, Mar 18, 2021 at 04:16:24PM +0100, Emanuele Giuseppe Esposito wrote:
>> Test for the KVM_SET_BOOT_CPU_ID ioctl.
>> Check that it correctly allows to change the BSP vcpu.
>>
>> v1 -> v2:
>> - remove unnecessary printf
>> - move stage for loop inside run_vcpu
>> - test EBUSY when calling KVM_SET_BOOT_CPU_ID after vcpu
>>    creation and execution
>> - introduce _vm_ioctl
> 
> This information should be in the cover-letter. Or, for a single patch (no
> cover-letter needed submission), then it should go below the '---' under
> your s-o-b.
> 
>>

>> +static void add_x86_vcpu(struct kvm_vm *vm, uint32_t vcpuid, bool bsp_code)
>> +{
>> +	if (bsp_code)
>> +		vm_vcpu_add_default(vm, vcpuid, guest_bsp_vcpu);
>> +	else
>> +		vm_vcpu_add_default(vm, vcpuid, guest_not_bsp_vcpu);
>> +
>> +	vcpu_set_cpuid(vm, vcpuid, kvm_get_supported_cpuid());
>> +}
>> +
>> +static void run_vm_bsp(uint32_t bsp_vcpu)
> 
> I think the 'bsp' suffixes and prefixes make the purpose of this function
> and its argument more confusing. Just
> 
>   static void run_vm(uint32_t vcpu)
> 
> would be more clear to me.

The idea here was "run vm with this vcpu as BSP", implicitly assuming 
that there are alwasy 2 vcpu inside, so we are picking one as BSP.

Maybe

run_vm_2_vcpu(uint32_t bsp_vcpid)

is better?

> 
>> +{
>> +	struct kvm_vm *vm;
>> +	bool is_bsp_vcpu1 = bsp_vcpu == VCPU_ID1;
> 
> Could add another define
> 
>   #define BSP_VCPU VCPU_ID1
> 
> And then instead of creating the above bool, just do
> 
>   if (vcpu == BSP_VCPU)

I think it will be even more confusing to have BSP_VCPU fixed to 
VCPU_ID1, because in the tests before and after I use VCPU_ID0 as BSP.

	run_vm_bsp(VCPU_ID0);
	run_vm_bsp(VCPU_ID1);
	run_vm_bsp(VCPU_ID0);

> 
>> +
>> +	vm = create_vm();
>> +
>> +	if (is_bsp_vcpu1)
>> +		vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
> 
> Does this ioctl need to be called before creating the vcpus? The
> documentation in Documentation/virt/kvm/api.rst doesn't say that.

Yes, it has to be called before creating the vcpus, as also shown in the 
test function "check_set_bsp_busy". KVM checks that created_vcpus is 0 
before setting the bsp field.

arch/x86/kvm/x86.c
		case KVM_SET_BOOT_CPU_ID:
		...
		if (kvm->created_vcpus)
			r = -EBUSY;
		else
			kvm->arch.bsp_vcpu_id = arg;

I will update the documentation to include also this information.

> If it can be called after creating the vcpus, then
> vm_create_default_with_vcpus() can be used and there's no need
> for the create_vm() and add_x86_vcpu() functions.

Just use the
> same guest code for both, but pass the cpu index to the guest
> code function allowing something like
> 
>     if (cpu == BSP_VCPU)
> 	GUEST_ASSERT(get_bsp_flag() != 0);
>     else
> 	GUEST_ASSERT(get_bsp_flag() == 0);
> 
I might be wrong, but there seems not to be an easy way to pass 
arguments to the guest function.

Thank you,
Emanuele
> 
>> +
>> +	add_x86_vcpu(vm, VCPU_ID0, !is_bsp_vcpu1);
>> +	add_x86_vcpu(vm, VCPU_ID1, is_bsp_vcpu1);
>> +
>> +	run_vcpu(vm, VCPU_ID0);
>> +	run_vcpu(vm, VCPU_ID1);
>> +
>> +	kvm_vm_free(vm);
>> +}
>> +
>> +static void check_set_bsp_busy(void)
>> +{
>> +	struct kvm_vm *vm;
>> +	int res;
>> +
>> +	vm = create_vm();
>> +
>> +	add_x86_vcpu(vm, VCPU_ID0, true);
>> +	add_x86_vcpu(vm, VCPU_ID1, false);
>> +
>> +	res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
>> +	TEST_ASSERT(res == -1 && errno == EBUSY, "KVM_SET_BOOT_CPU_ID set after adding vcpu");
>> +
>> +	run_vcpu(vm, VCPU_ID0);
>> +	run_vcpu(vm, VCPU_ID1);
>> +
>> +	res = _vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
>> +	TEST_ASSERT(res == -1 && errno == EBUSY, "KVM_SET_BOOT_CPU_ID set to a terminated vcpu");
>> +
>> +	kvm_vm_free(vm);
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +	if (!kvm_check_cap(KVM_CAP_SET_BOOT_CPU_ID)) {
>> +		print_skip("set_boot_cpu_id not available");
>> +		return 0;
> 
> Should be exit(KSFT_SKIP);
> 
>> +	}
>> +
>> +	run_vm_bsp(VCPU_ID0);
>> +	run_vm_bsp(VCPU_ID1);
>> +	run_vm_bsp(VCPU_ID0);
>> +
>> +	check_set_bsp_busy();
> 
> Don't you get a compiler warning here saying there's no return from a
> function that returns int?
> 
>> +}
>> -- 
>> 2.29.2
>>
> 
> Thanks,
> drew
> 

Re: [PATCH v2 2/2] selftests/kvm: add set_boot_cpu_id test

[Andrew Jones]

On Fri, Mar 19, 2021 at 09:34:40AM +0100, Emanuele Giuseppe Esposito wrote:
> On 18/03/2021 17:20, Andrew Jones wrote:
> > On Thu, Mar 18, 2021 at 04:16:24PM +0100, Emanuele Giuseppe Esposito wrote:
> > > +static void run_vm_bsp(uint32_t bsp_vcpu)
> > 
> > I think the 'bsp' suffixes and prefixes make the purpose of this function
> > and its argument more confusing. Just
> > 
> >   static void run_vm(uint32_t vcpu)
> > 
> > would be more clear to me.
> 
> The idea here was "run vm with this vcpu as BSP", implicitly assuming that
> there are alwasy 2 vcpu inside, so we are picking one as BSP.
> 
> Maybe
> 
> run_vm_2_vcpu(uint32_t bsp_vcpid)
> 
> is better?

run_vm() is probably still sufficient for the function name. I better
understand the naming of the function's argument now, so the bsp prefix
does make sense.

> 
> > 
> > > +{
> > > +	struct kvm_vm *vm;
> > > +	bool is_bsp_vcpu1 = bsp_vcpu == VCPU_ID1;
> > 
> > Could add another define
> > 
> >   #define BSP_VCPU VCPU_ID1
> > 
> > And then instead of creating the above bool, just do
> > 
> >   if (vcpu == BSP_VCPU)
> 
> I think it will be even more confusing to have BSP_VCPU fixed to VCPU_ID1,
> because in the tests before and after I use VCPU_ID0 as BSP.
> 
> 	run_vm_bsp(VCPU_ID0);
> 	run_vm_bsp(VCPU_ID1);
> 	run_vm_bsp(VCPU_ID0);

Agreed. I hadn't read that far down and hadn't grasped the purpose
of run_vm[_bsp]'s argument before. But, can we get rid of the
is_bsp_vcpu1 boolean anyway? 'if (bsp_vcpu == VCPU_ID1)' is terse
enough, and it better exposes the logic.

> 
> > 
> > > +
> > > +	vm = create_vm();
> > > +
> > > +	if (is_bsp_vcpu1)
> > > +		vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *) VCPU_ID1);
> > 
> > Does this ioctl need to be called before creating the vcpus? The
> > documentation in Documentation/virt/kvm/api.rst doesn't say that.
> 
> Yes, it has to be called before creating the vcpus, as also shown in the
> test function "check_set_bsp_busy". KVM checks that created_vcpus is 0
> before setting the bsp field.
> 
> arch/x86/kvm/x86.c
> 		case KVM_SET_BOOT_CPU_ID:
> 		...
> 		if (kvm->created_vcpus)
> 			r = -EBUSY;
> 		else
> 			kvm->arch.bsp_vcpu_id = arg;
> 
> I will update the documentation to include also this information.

Great!

And I'll try to improve the KVM selftests API to better support these
types of tests without having to copy so much code.

> 
> > If it can be called after creating the vcpus, then
> > vm_create_default_with_vcpus() can be used and there's no need
> > for the create_vm() and add_x86_vcpu() functions.
> 
> Just use the
> > same guest code for both, but pass the cpu index to the guest
> > code function allowing something like
> > 
> >     if (cpu == BSP_VCPU)
> > 	GUEST_ASSERT(get_bsp_flag() != 0);
> >     else
> > 	GUEST_ASSERT(get_bsp_flag() == 0);
> > 
> I might be wrong, but there seems not to be an easy way to pass arguments to
> the guest function.

There are several tests that pass the CPU index to the guest function
which you can use as examples. Take a look at e.g. steal_time.c. The
trick is to set the argument(s) with vcpu_args_set().

Thanks,
drew