Re: [PATCH v3 05/10] KVM: arm64: selftests: Make vgic_init gic version agnostic

[Ricardo Koller <ricarkol@google.com>]

On Wed, Sep 29, 2021 at 07:12:59PM +0200, Eric Auger wrote:
> 
> 
> On 9/28/21 8:47 PM, Ricardo Koller wrote:
> > As a preparation for the next commits which will add some tests for
> > GICv2, make aarch64/vgic_init GIC version agnostic. Add a new generic
> > run_tests function(gic_dev_type) that starts all applicable tests using
> > GICv3 or GICv2. GICv2 tests are attempted if GICv3 is not available in
> > the system. There are currently no GICv2 tests, but the test passes now
> > in GICv2 systems.
> >
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  .../testing/selftests/kvm/aarch64/vgic_init.c | 156 +++++++++++-------
> >  1 file changed, 95 insertions(+), 61 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > index 623f31a14326..896a29f2503d 100644
> > --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> > @@ -22,6 +22,9 @@
> >  
> >  #define GICR_TYPER 0x8
> >  
> > +#define VGIC_DEV_IS_V2(_d) ((_d) == KVM_DEV_TYPE_ARM_VGIC_V2)
> > +#define VGIC_DEV_IS_V3(_d) ((_d) == KVM_DEV_TYPE_ARM_VGIC_V3)
> > +
> >  struct vm_gic {
> >  	struct kvm_vm *vm;
> >  	int gic_fd;
> > @@ -30,8 +33,8 @@ struct vm_gic {
> >  static int max_ipa_bits;
> >  
> >  /* helper to access a redistributor register */
> > -static int access_redist_reg(int gicv3_fd, int vcpu, int offset,
> > -			     uint32_t *val, bool write)
> > +static int access_v3_redist_reg(int gicv3_fd, int vcpu, int offset,
> > +				uint32_t *val, bool write)
> >  {
> >  	uint64_t attr = REG_OFFSET(vcpu, offset);
> >  
> > @@ -58,7 +61,7 @@ static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
> >  	return 0;
> >  }
> >  
> > -static struct vm_gic vm_gic_create(void)
> > +static struct vm_gic vm_gic_v3_create(void)
> >  {
> >  	struct vm_gic v;
> >  
> > @@ -80,7 +83,7 @@ static void vm_gic_destroy(struct vm_gic *v)
> >   * device gets created, a legacy RDIST region is set at @0x0
> >   * and a DIST region is set @0x60000
> >   */
> > -static void subtest_dist_rdist(struct vm_gic *v)
> > +static void subtest_v3_dist_rdist(struct vm_gic *v)
> >  {
> >  	int ret;
> >  	uint64_t addr;
> > @@ -145,7 +148,7 @@ static void subtest_dist_rdist(struct vm_gic *v)
> >  }
> >  
> >  /* Test the new REDIST region API */
> > -static void subtest_redist_regions(struct vm_gic *v)
> > +static void subtest_v3_redist_regions(struct vm_gic *v)
> >  {
> >  	uint64_t addr, expected_addr;
> >  	int ret;
> > @@ -249,7 +252,7 @@ static void subtest_redist_regions(struct vm_gic *v)
> >   * VGIC KVM device is created and initialized before the secondary CPUs
> >   * get created
> >   */
> > -static void test_vgic_then_vcpus(void)
> > +static void test_v3_vgic_then_vcpus(uint32_t gic_dev_type)
> >  {
> >  	struct vm_gic v;
> >  	int ret, i;
> > @@ -257,7 +260,7 @@ static void test_vgic_then_vcpus(void)
> >  	v.vm = vm_create_default(0, 0, guest_code);
> >  	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> >  
> > -	subtest_dist_rdist(&v);
> > +	subtest_v3_dist_rdist(&v);
> >  
> >  	/* Add the rest of the VCPUs */
> >  	for (i = 1; i < NR_VCPUS; ++i)
> > @@ -270,14 +273,14 @@ static void test_vgic_then_vcpus(void)
> >  }
> >  
> >  /* All the VCPUs are created before the VGIC KVM device gets initialized */
> > -static void test_vcpus_then_vgic(void)
> > +static void test_v3_vcpus_then_vgic(uint32_t gic_dev_type)
> >  {
> >  	struct vm_gic v;
> >  	int ret;
> >  
> > -	v = vm_gic_create();
> > +	v = vm_gic_v3_create();
> >  
> > -	subtest_dist_rdist(&v);
> > +	subtest_v3_dist_rdist(&v);
> >  
> >  	ret = run_vcpu(v.vm, 3);
> >  	TEST_ASSERT(ret == -EINVAL, "dist/rdist overlap detected on 1st vcpu run");
> > @@ -285,15 +288,15 @@ static void test_vcpus_then_vgic(void)
> >  	vm_gic_destroy(&v);
> >  }
> >  
> > -static void test_new_redist_regions(void)
> > +static void test_v3_new_redist_regions(void)
> >  {
> >  	void *dummy = NULL;
> >  	struct vm_gic v;
> >  	uint64_t addr;
> >  	int ret;
> >  
> > -	v = vm_gic_create();
> > -	subtest_redist_regions(&v);
> > +	v = vm_gic_v3_create();
> > +	subtest_v3_redist_regions(&v);
> >  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_CTRL,
> >  			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> >  
> > @@ -303,8 +306,8 @@ static void test_new_redist_regions(void)
> >  
> >  	/* step2 */
> >  
> > -	v = vm_gic_create();
> > -	subtest_redist_regions(&v);
> > +	v = vm_gic_v3_create();
> > +	subtest_v3_redist_regions(&v);
> >  
> >  	addr = REDIST_REGION_ATTR_ADDR(1, 0x280000, 0, 2);
> >  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> > @@ -317,8 +320,8 @@ static void test_new_redist_regions(void)
> >  
> >  	/* step 3 */
> >  
> > -	v = vm_gic_create();
> > -	subtest_redist_regions(&v);
> > +	v = vm_gic_v3_create();
> > +	subtest_v3_redist_regions(&v);
> >  
> >  	_kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> >  			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, dummy, true);
> > @@ -338,7 +341,7 @@ static void test_new_redist_regions(void)
> >  	vm_gic_destroy(&v);
> >  }
> >  
> > -static void test_typer_accesses(void)
> > +static void test_v3_typer_accesses(void)
> >  {
> >  	struct vm_gic v;
> >  	uint64_t addr;
> > @@ -351,12 +354,12 @@ static void test_typer_accesses(void)
> >  
> >  	vm_vcpu_add_default(v.vm, 3, guest_code);
> >  
> > -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(ret && errno == EINVAL, "attempting to read GICR_TYPER of non created vcpu");
> >  
> >  	vm_vcpu_add_default(v.vm, 1, guest_code);
> >  
> > -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(ret && errno == EBUSY, "read GICR_TYPER before GIC initialized");
> >  
> >  	vm_vcpu_add_default(v.vm, 2, guest_code);
> > @@ -365,7 +368,7 @@ static void test_typer_accesses(void)
> >  			  KVM_DEV_ARM_VGIC_CTRL_INIT, NULL, true);
> >  
> >  	for (i = 0; i < NR_VCPUS ; i++) {
> > -		ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> > +		ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> >  		TEST_ASSERT(!ret && !val, "read GICR_TYPER before rdist region setting");
> >  	}
> >  
> > @@ -374,10 +377,10 @@ static void test_typer_accesses(void)
> >  			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> >  
> >  	/* The 2 first rdists should be put there (vcpu 0 and 3) */
> > -	ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && !val, "read typer of rdist #0");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x310, "read typer of rdist #1");
> >  
> >  	addr = REDIST_REGION_ATTR_ADDR(10, 0x100000, 0, 1);
> > @@ -385,11 +388,11 @@ static void test_typer_accesses(void)
> >  				 KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> >  	TEST_ASSERT(ret && errno == EINVAL, "collision with previous rdist region");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x100,
> >  		    "no redist region attached to vcpu #1 yet, last cannot be returned");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x200,
> >  		    "no redist region attached to vcpu #2, last cannot be returned");
> >  
> > @@ -397,10 +400,10 @@ static void test_typer_accesses(void)
> >  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> >  			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> >  
> > -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #1");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x210,
> >  		    "read typer of rdist #1, last properly returned");
> >  
> > @@ -417,7 +420,7 @@ static void test_typer_accesses(void)
> >   * rdist region #2 @0x200000 2 rdist capacity
> >   *     rdists: 1, 2
> >   */
> > -static void test_last_bit_redist_regions(void)
> > +static void test_v3_last_bit_redist_regions(void)
> >  {
> >  	uint32_t vcpuids[] = { 0, 3, 5, 4, 1, 2 };
> >  	struct vm_gic v;
> > @@ -444,29 +447,29 @@ static void test_last_bit_redist_regions(void)
> >  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> >  			  KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION, &addr, true);
> >  
> > -	ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x000, "read typer of rdist #0");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #1");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x200, "read typer of rdist #2");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x310, "read typer of rdist #3");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x500, "read typer of rdist #5");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 4, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 4, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x410, "read typer of rdist #4");
> >  
> >  	vm_gic_destroy(&v);
> >  }
> >  
> >  /* Test last bit with legacy region */
> > -static void test_last_bit_single_rdist(void)
> > +static void test_v3_last_bit_single_rdist(void)
> >  {
> >  	uint32_t vcpuids[] = { 0, 3, 5, 4, 1, 2 };
> >  	struct vm_gic v;
> > @@ -485,28 +488,32 @@ static void test_last_bit_single_rdist(void)
> >  	kvm_device_access(v.gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> >  			  KVM_VGIC_V3_ADDR_TYPE_REDIST, &addr, true);
> >  
> > -	ret = access_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 0, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x000, "read typer of rdist #0");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 3, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x300, "read typer of rdist #1");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 5, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x500, "read typer of rdist #2");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 1, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x100, "read typer of rdist #3");
> >  
> > -	ret = access_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> > +	ret = access_v3_redist_reg(v.gic_fd, 2, GICR_TYPER, &val, false);
> >  	TEST_ASSERT(!ret && val == 0x210, "read typer of rdist #3");
> >  
> >  	vm_gic_destroy(&v);
> >  }
> >  
> > -void test_kvm_device(void)
> > +/*
> > + * Returns 0 if it's possible to create GIC device of a given type (V2 or V3).
> > + */
> > +int test_kvm_device(uint32_t gic_dev_type)
> >  {
> >  	struct vm_gic v;
> >  	int ret, fd;
> > +	uint32_t other;
> >  
> >  	v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
> >  
> > @@ -514,38 +521,65 @@ void test_kvm_device(void)
> >  	ret = _kvm_create_device(v.vm, 0, true, &fd);
> >  	TEST_ASSERT(ret && errno == ENODEV, "unsupported device");
> >  
> > -	/* trial mode with VGIC_V3 device */
> > -	ret = _kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, true, &fd);
> > -	if (ret) {
> > -		print_skip("GICv3 not supported");
> > -		exit(KSFT_SKIP);
> > -	}
> > -	v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> > +	/* trial mode */
> > +	ret = _kvm_create_device(v.vm, gic_dev_type, true, &fd);
> > +	if (ret)
> > +		return ret;
> > +	v.gic_fd = kvm_create_device(v.vm, gic_dev_type, false);
> > +
> > +	ret = _kvm_create_device(v.vm, gic_dev_type, false, &fd);
> > +	TEST_ASSERT(ret && errno == EEXIST, "create GIC device twice");
> >  
> > -	ret = _kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false, &fd);
> > -	TEST_ASSERT(ret && errno == EEXIST, "create GICv3 device twice");
> > +	kvm_create_device(v.vm, gic_dev_type, true);
> >  
> > -	kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, true);
> > +	/* try to create the other gic_dev_type */
> > +	other = VGIC_DEV_IS_V2(gic_dev_type) ? KVM_DEV_TYPE_ARM_VGIC_V3
> > +					     : KVM_DEV_TYPE_ARM_VGIC_V2;
> >  
> > -	if (!_kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V2, true, &fd)) {
> > -		ret = _kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V2, false, &fd);
> > -		TEST_ASSERT(ret && errno == EINVAL, "create GICv2 while v3 exists");
> > +	if (!_kvm_create_device(v.vm, other, true, &fd)) {
> > +		ret = _kvm_create_device(v.vm, other, false, &fd);
> > +		TEST_ASSERT(ret && errno == EINVAL,
> > +				"create GIC device while other version exists");
> >  	}
> >  
> >  	vm_gic_destroy(&v);
> > +
> > +	return 0;
> > +}
> > +
> > +void run_tests(uint32_t gic_dev_type)
> > +{
> > +	if (VGIC_DEV_IS_V3(gic_dev_type)) {
> > +		test_v3_vcpus_then_vgic(gic_dev_type);
> > +		test_v3_vgic_then_vcpus(gic_dev_type);
> > +		test_v3_new_redist_regions();
> > +		test_v3_typer_accesses();
> > +		test_v3_last_bit_redist_regions();
> > +		test_v3_last_bit_single_rdist();
> > +	}
> >  }
> >  
> >  int main(int ac, char **av)
> >  {
> > +	int ret;
> > +
> >  	max_ipa_bits = kvm_check_cap(KVM_CAP_ARM_VM_IPA_SIZE);
> >  
> > -	test_kvm_device();
> > -	test_vcpus_then_vgic();
> > -	test_vgic_then_vcpus();
> > -	test_new_redist_regions();
> > -	test_typer_accesses();
> > -	test_last_bit_redist_regions();
> > -	test_last_bit_single_rdist();
> > +	ret = test_kvm_device(KVM_DEV_TYPE_ARM_VGIC_V3);
> > +	if (!ret) {
> > +		pr_info("Running GIC_v3 tests.\n");
> > +		run_tests(KVM_DEV_TYPE_ARM_VGIC_V3);
> > +		return 0;
> If the GICv3 supports compat with GICv2, I think you could be able to
> run both tests consecutively. So maybe don't return?
> 

Good point, will try that (not returning).

Thanks,
Ricardo

> Besides looks good
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 
> Eric
> 
> > +	}
> > +
> > +	ret = test_kvm_device(KVM_DEV_TYPE_ARM_VGIC_V2);
> > +	if (!ret) {
> > +		pr_info("Running GIC_v2 tests.\n");
> > +		run_tests(KVM_DEV_TYPE_ARM_VGIC_V2);
> > +		return 0;
> > +	}
> 
> >  
> > +	print_skip("No GICv2 nor GICv3 support");
> > +	exit(KSFT_SKIP);
> >  	return 0;
> >  }
>