| Message ID | 20210318151624.490861-1-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] kvm/kvm_util: add _vm_ioctl | expand |
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 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>
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
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 *
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(-)