| Message ID | 20220321050804.2701035-3-reijiw@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: mixed-width check should be skipped for uninitialized vCPUs | expand |
Hi Reiji, On Sun, Mar 20, 2022 at 10:08:04PM -0700, Reiji Watanabe wrote: > Introduce a test for aarch64 that ensures non-mixed-width vCPUs > (all 64bit vCPUs or all 32bit vcPUs) can be configured, and > mixed-width vCPUs cannot be configured. > > Reviewed-by: Andrew Jones <drjones@redhat.com> > Signed-off-by: Reiji Watanabe <reijiw@google.com> Tiny nits, but looks fine to me. Only bother addressing if you do another spin, otherwise: Reviewed-by: Oliver Upton <oupton@google.com> > --- > tools/testing/selftests/kvm/.gitignore | 1 + > tools/testing/selftests/kvm/Makefile | 1 + > .../selftests/kvm/aarch64/vcpu_width_config.c | 125 ++++++++++++++++++ > 3 files changed, 127 insertions(+) > create mode 100644 tools/testing/selftests/kvm/aarch64/vcpu_width_config.c > > diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore > index dce7de7755e6..4e884e29b2a8 100644 > --- a/tools/testing/selftests/kvm/.gitignore > +++ b/tools/testing/selftests/kvm/.gitignore > @@ -3,6 +3,7 @@ > /aarch64/debug-exceptions > /aarch64/get-reg-list > /aarch64/psci_cpu_on_test > +/aarch64/vcpu_width_config > /aarch64/vgic_init > /aarch64/vgic_irq > /s390x/memop > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile > index 0e4926bc9a58..06a5a982123e 100644 > --- a/tools/testing/selftests/kvm/Makefile > +++ b/tools/testing/selftests/kvm/Makefile > @@ -104,6 +104,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/arch_timer > TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions > TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list > TEST_GEN_PROGS_aarch64 += aarch64/psci_cpu_on_test > +TEST_GEN_PROGS_aarch64 += aarch64/vcpu_width_config > TEST_GEN_PROGS_aarch64 += aarch64/vgic_init > TEST_GEN_PROGS_aarch64 += aarch64/vgic_irq > TEST_GEN_PROGS_aarch64 += demand_paging_test > diff --git a/tools/testing/selftests/kvm/aarch64/vcpu_width_config.c b/tools/testing/selftests/kvm/aarch64/vcpu_width_config.c > new file mode 100644 > index 000000000000..6e6e6a9f69e3 > --- /dev/null > +++ b/tools/testing/selftests/kvm/aarch64/vcpu_width_config.c > @@ -0,0 +1,125 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * vcpu_width_config - Test KVM_ARM_VCPU_INIT() with KVM_ARM_VCPU_EL1_32BIT. > + * > + * Copyright (c) 2022 Google LLC. > + * > + * This is a test that ensures that non-mixed-width vCPUs (all 64bit vCPUs > + * or all 32bit vcPUs) can be configured and mixed-width vCPUs cannot be > + * configured. > + */ > + > +#define _GNU_SOURCE In other instances where we define _GNU_SOURCE, it is said we do it for program_invocation_short_name. Nonetheless, I cannot find anywhere that the symbol is actually being used. This looks to be some leftover crud from our internal test library before we upstreamed KVM selftests a few years ago. > +#include "kvm_util.h" > +#include "processor.h" > +#include "test_util.h" > + > + > +/* > + * Add a vCPU, run KVM_ARM_VCPU_INIT with @init1, and then > + * add another vCPU, and run KVM_ARM_VCPU_INIT with @init2. > + */ > +static int add_init_2vcpus(struct kvm_vcpu_init *init1, > + struct kvm_vcpu_init *init2) > +{ > + struct kvm_vm *vm; > + int ret; > + > + vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR); > + > + vm_vcpu_add(vm, 0); > + ret = _vcpu_ioctl(vm, 0, KVM_ARM_VCPU_INIT, init1); > + if (ret) > + goto free_exit; > + > + vm_vcpu_add(vm, 1); > + ret = _vcpu_ioctl(vm, 1, KVM_ARM_VCPU_INIT, init2); > + > +free_exit: > + kvm_vm_free(vm); > + return ret; > +} > + > +/* > + * Add two vCPUs, then run KVM_ARM_VCPU_INIT for one vCPU with @init1, > + * and run KVM_ARM_VCPU_INIT for another vCPU with @init2. > + */ > +static int add_2vcpus_init_2vcpus(struct kvm_vcpu_init *init1, > + struct kvm_vcpu_init *init2) > +{ > + struct kvm_vm *vm; > + int ret; > + > + vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR); > + > + vm_vcpu_add(vm, 0); > + vm_vcpu_add(vm, 1); > + > + ret = _vcpu_ioctl(vm, 0, KVM_ARM_VCPU_INIT, init1); > + if (ret) > + goto free_exit; > + > + ret = _vcpu_ioctl(vm, 1, KVM_ARM_VCPU_INIT, init2); > + > +free_exit: > + kvm_vm_free(vm); > + return ret; > +} > + > +/* > + * Tests that two 64bit vCPUs can be configured, two 32bit vCPUs can be > + * configured, and two mixed-witgh vCPUs cannot be configured. mixed-width > + * Each of those three cases, configure vCPUs in two different orders. > + * The one is running KVM_CREATE_VCPU for 2 vCPUs, and then running > + * KVM_ARM_VCPU_INIT for them. > + * The other is running KVM_CREATE_VCPU and KVM_ARM_VCPU_INIT for a vCPU, > + * and then run those commands for another vCPU. > + */ > +int main(void) > +{ > + struct kvm_vcpu_init init1, init2; > + struct kvm_vm *vm; > + int ret; > + > + if (kvm_check_cap(KVM_CAP_ARM_EL1_32BIT) <= 0) { nit: this is fine, but KVM_CHECK_EXTENSION returns exactly 0 if a capability is not supported. > + print_skip("KVM_CAP_ARM_EL1_32BIT is not supported"); > + exit(KSFT_SKIP); > + } > + > + /* Get the preferred target type and copy that to init2 */ > + vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR); > + vm_ioctl(vm, KVM_ARM_PREFERRED_TARGET, &init1); > + kvm_vm_free(vm); > + memcpy(&init2, &init1, sizeof(init2)); nit: I think you can just assign init2 = init1. > + /* Test with 64bit vCPUs */ > + ret = add_init_2vcpus(&init1, &init2); > + TEST_ASSERT(ret == 0, > + "Configuring 64bit EL1 vCPUs failed unexpectedly"); > + ret = add_2vcpus_init_2vcpus(&init1, &init2); nit: for the homogeneous vCPU configuration tests, you could pass two pointers to the same init structure to really drive the point home that the vCPUs are the same. The underlying ioctl does not write back anything to userspace. > + TEST_ASSERT(ret == 0, > + "Configuring 64bit EL1 vCPUs failed unexpectedly"); > + > + /* Test with 32bit vCPUs */ > + init1.features[0] = (1 << KVM_ARM_VCPU_EL1_32BIT); > + init2.features[0] = (1 << KVM_ARM_VCPU_EL1_32BIT); > + ret = add_init_2vcpus(&init1, &init2); > + TEST_ASSERT(ret == 0, > + "Configuring 32bit EL1 vCPUs failed unexpectedly"); > + ret = add_2vcpus_init_2vcpus(&init1, &init2); > + TEST_ASSERT(ret == 0, > + "Configuring 32bit EL1 vCPUs failed unexpectedly"); > + > + /* Test with mixed-width vCPUs */ > + init1.features[0] = 0; > + init2.features[0] = (1 << KVM_ARM_VCPU_EL1_32BIT); > + ret = add_init_2vcpus(&init1, &init2); > + TEST_ASSERT(ret != 0, > + "Configuring mixed-width vCPUs worked unexpectedly"); > + ret = add_2vcpus_init_2vcpus(&init1, &init2); > + TEST_ASSERT(ret != 0, > + "Configuring mixed-width vCPUs worked unexpectedly"); > + > + return 0; > +} > -- > 2.35.1.894.gb6a874cedc-goog >
On Mon, Mar 21, 2022 at 06:17:43AM +0000, Oliver Upton wrote: > Hi Reiji, > > On Sun, Mar 20, 2022 at 10:08:04PM -0700, Reiji Watanabe wrote: [...] > > +#define _GNU_SOURCE > > In other instances where we define _GNU_SOURCE, it is said we do it for > program_invocation_short_name. Nonetheless, I cannot find anywhere that > the symbol is actually being used. > > This looks to be some leftover crud from our internal test library > before we upstreamed KVM selftests a few years ago. > Ah, it's because we're actually using program_invocation_name. This already gets defined in lib/kvm_util.c, so except for a few oddball tests that directly call kvm_vm_elf_load(), this is unnecessary. -- Thanks, Oliver
Hi Oliver, On Sun, Mar 20, 2022 at 11:17 PM Oliver Upton <oupton@google.com> wrote: > > Hi Reiji, > > On Sun, Mar 20, 2022 at 10:08:04PM -0700, Reiji Watanabe wrote: > > Introduce a test for aarch64 that ensures non-mixed-width vCPUs > > (all 64bit vCPUs or all 32bit vcPUs) can be configured, and > > mixed-width vCPUs cannot be configured. > > > > Reviewed-by: Andrew Jones <drjones@redhat.com> > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > Tiny nits, but looks fine to me. Only bother addressing if you do > another spin, otherwise: > > Reviewed-by: Oliver Upton <oupton@google.com> Thank you for the review! Since I would like to fix one of your comments (the typo) at least, I will respin the series (and will address all the comments). Thanks, Reiji
diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore index dce7de7755e6..4e884e29b2a8 100644 --- a/tools/testing/selftests/kvm/.gitignore +++ b/tools/testing/selftests/kvm/.gitignore @@ -3,6 +3,7 @@ /aarch64/debug-exceptions /aarch64/get-reg-list /aarch64/psci_cpu_on_test +/aarch64/vcpu_width_config /aarch64/vgic_init /aarch64/vgic_irq /s390x/memop diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 0e4926bc9a58..06a5a982123e 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -104,6 +104,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/arch_timer TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list TEST_GEN_PROGS_aarch64 += aarch64/psci_cpu_on_test +TEST_GEN_PROGS_aarch64 += aarch64/vcpu_width_config TEST_GEN_PROGS_aarch64 += aarch64/vgic_init TEST_GEN_PROGS_aarch64 += aarch64/vgic_irq TEST_GEN_PROGS_aarch64 += demand_paging_test diff --git a/tools/testing/selftests/kvm/aarch64/vcpu_width_config.c b/tools/testing/selftests/kvm/aarch64/vcpu_width_config.c new file mode 100644 index 000000000000..6e6e6a9f69e3 --- /dev/null +++ b/tools/testing/selftests/kvm/aarch64/vcpu_width_config.c @@ -0,0 +1,125 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * vcpu_width_config - Test KVM_ARM_VCPU_INIT() with KVM_ARM_VCPU_EL1_32BIT. + * + * Copyright (c) 2022 Google LLC. + * + * This is a test that ensures that non-mixed-width vCPUs (all 64bit vCPUs + * or all 32bit vcPUs) can be configured and mixed-width vCPUs cannot be + * configured. + */ + +#define _GNU_SOURCE + +#include "kvm_util.h" +#include "processor.h" +#include "test_util.h" + + +/* + * Add a vCPU, run KVM_ARM_VCPU_INIT with @init1, and then + * add another vCPU, and run KVM_ARM_VCPU_INIT with @init2. + */ +static int add_init_2vcpus(struct kvm_vcpu_init *init1, + struct kvm_vcpu_init *init2) +{ + struct kvm_vm *vm; + int ret; + + vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR); + + vm_vcpu_add(vm, 0); + ret = _vcpu_ioctl(vm, 0, KVM_ARM_VCPU_INIT, init1); + if (ret) + goto free_exit; + + vm_vcpu_add(vm, 1); + ret = _vcpu_ioctl(vm, 1, KVM_ARM_VCPU_INIT, init2); + +free_exit: + kvm_vm_free(vm); + return ret; +} + +/* + * Add two vCPUs, then run KVM_ARM_VCPU_INIT for one vCPU with @init1, + * and run KVM_ARM_VCPU_INIT for another vCPU with @init2. + */ +static int add_2vcpus_init_2vcpus(struct kvm_vcpu_init *init1, + struct kvm_vcpu_init *init2) +{ + struct kvm_vm *vm; + int ret; + + vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR); + + vm_vcpu_add(vm, 0); + vm_vcpu_add(vm, 1); + + ret = _vcpu_ioctl(vm, 0, KVM_ARM_VCPU_INIT, init1); + if (ret) + goto free_exit; + + ret = _vcpu_ioctl(vm, 1, KVM_ARM_VCPU_INIT, init2); + +free_exit: + kvm_vm_free(vm); + return ret; +} + +/* + * Tests that two 64bit vCPUs can be configured, two 32bit vCPUs can be + * configured, and two mixed-witgh vCPUs cannot be configured. + * Each of those three cases, configure vCPUs in two different orders. + * The one is running KVM_CREATE_VCPU for 2 vCPUs, and then running + * KVM_ARM_VCPU_INIT for them. + * The other is running KVM_CREATE_VCPU and KVM_ARM_VCPU_INIT for a vCPU, + * and then run those commands for another vCPU. + */ +int main(void) +{ + struct kvm_vcpu_init init1, init2; + struct kvm_vm *vm; + int ret; + + if (kvm_check_cap(KVM_CAP_ARM_EL1_32BIT) <= 0) { + print_skip("KVM_CAP_ARM_EL1_32BIT is not supported"); + exit(KSFT_SKIP); + } + + /* Get the preferred target type and copy that to init2 */ + vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES, O_RDWR); + vm_ioctl(vm, KVM_ARM_PREFERRED_TARGET, &init1); + kvm_vm_free(vm); + memcpy(&init2, &init1, sizeof(init2)); + + /* Test with 64bit vCPUs */ + ret = add_init_2vcpus(&init1, &init2); + TEST_ASSERT(ret == 0, + "Configuring 64bit EL1 vCPUs failed unexpectedly"); + ret = add_2vcpus_init_2vcpus(&init1, &init2); + TEST_ASSERT(ret == 0, + "Configuring 64bit EL1 vCPUs failed unexpectedly"); + + /* Test with 32bit vCPUs */ + init1.features[0] = (1 << KVM_ARM_VCPU_EL1_32BIT); + init2.features[0] = (1 << KVM_ARM_VCPU_EL1_32BIT); + ret = add_init_2vcpus(&init1, &init2); + TEST_ASSERT(ret == 0, + "Configuring 32bit EL1 vCPUs failed unexpectedly"); + ret = add_2vcpus_init_2vcpus(&init1, &init2); + TEST_ASSERT(ret == 0, + "Configuring 32bit EL1 vCPUs failed unexpectedly"); + + /* Test with mixed-width vCPUs */ + init1.features[0] = 0; + init2.features[0] = (1 << KVM_ARM_VCPU_EL1_32BIT); + ret = add_init_2vcpus(&init1, &init2); + TEST_ASSERT(ret != 0, + "Configuring mixed-width vCPUs worked unexpectedly"); + ret = add_2vcpus_init_2vcpus(&init1, &init2); + TEST_ASSERT(ret != 0, + "Configuring mixed-width vCPUs worked unexpectedly"); + + return 0; +}