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 *
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(); +}
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
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
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
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
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
linux-kselftest-mirror@lists.linaro.org