Re: [PATCH v5 2/2] KVM: arm64: selftests: Introduce vcpu_width_config

[Oliver Upton <oupton@google.com>]

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel