Re: [PATCH] KVM: selftests: vgic_init kvm selftests fixup

2021-04-07 Thread Marc Zyngier 
On Wed, 7 Apr 2021 15:59:37 +0200, Eric Auger wrote:
> Bring some improvements/rationalization over the first version
> of the vgic_init selftests:
> 
> - ucall_init is moved in run_cpu()
> - vcpu_args_set is not called as not needed
> - whenever a helper is supposed to succeed, call the non "_" version
> - helpers do not return -errno, instead errno is checked by the caller
> - vm_gic struct is used whenever possible, as well as vm_gic_destroy
> - _kvm_create_device takes an addition fd parameter

Applied to kvm-arm64/vgic-5.13, thanks!

[1/1] KVM: selftests: vgic_init kvm selftests fixup
  commit: 4cffb2df4260ed38c7ae4105f6913ad2d71a16ec

Cheers,

M.
-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] KVM: selftests: vgic_init kvm selftests fixup

2021-04-07 Thread Andrew Jones 


Hi Eric,

If $SUBJECT started with 'fixup!' and otherwise matched the original
subject then this could be automagically squashed in wherever the
original commit already is.

Anyway,

Reviewed-by: Andrew Jones 

Thanks,
drew

On Wed, Apr 07, 2021 at 03:59:37PM +0200, Eric Auger wrote:
> Bring some improvements/rationalization over the first version
> of the vgic_init selftests:
> 
> - ucall_init is moved in run_cpu()
> - vcpu_args_set is not called as not needed
> - whenever a helper is supposed to succeed, call the non "_" version
> - helpers do not return -errno, instead errno is checked by the caller
> - vm_gic struct is used whenever possible, as well as vm_gic_destroy
> - _kvm_create_device takes an addition fd parameter
> 
> Signed-off-by: Eric Auger 
> Suggested-by: Andrew Jones 
> 
> ---
> 
> Applies on top of [PATCH v6 0/9] KVM/ARM: Some vgic fixes
> and init sequence KVM selftests, put on kvm-arm64/vgic-5.13
> 
> The whole patchset can be found at
> https://github.com/eauger/linux/tree/vgic_kvmselftests_v7
> ---
>  .../testing/selftests/kvm/aarch64/vgic_init.c | 275 --
>  .../testing/selftests/kvm/include/kvm_util.h  |   2 +-
>  tools/testing/selftests/kvm/lib/kvm_util.c|  30 +-
>  3 files changed, 136 insertions(+), 171 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c 
> b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> index be1a7c0d0527..682e753fdc59 100644
> --- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
> +++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
> @@ -27,7 +27,7 @@ struct vm_gic {
>   int gic_fd;
>  };
>  
> -int max_ipa_bits;
> +static int max_ipa_bits;
>  
>  /* helper to access a redistributor register */
>  static int access_redist_reg(int gicv3_fd, int vcpu, int offset,
> @@ -51,12 +51,8 @@ static void guest_code(void)
>  /* we don't want to assert on run execution, hence that helper */
>  static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
>  {
> - int ret;
> -
> - vcpu_args_set(vm, vcpuid, 1);
> - ret = _vcpu_ioctl(vm, vcpuid, KVM_RUN, NULL);
> - get_ucall(vm, vcpuid, NULL);
> -
> + ucall_init(vm, NULL);
> + int ret = _vcpu_ioctl(vm, vcpuid, KVM_RUN, NULL);
>   if (ret)
>   return -errno;
>   return 0;
> @@ -68,7 +64,6 @@ static struct vm_gic vm_gic_create(void)
>  
>   v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
>   v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
> - TEST_ASSERT(v.gic_fd > 0, "GICv3 device created");
>  
>   return v;
>  }
> @@ -91,66 +86,62 @@ static void subtest_dist_rdist(struct vm_gic *v)
>   uint64_t addr;
>  
>   /* Check existing group/attributes */
> - ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> - KVM_VGIC_V3_ADDR_TYPE_DIST);
> - TEST_ASSERT(!ret, "KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_DIST 
> supported");
> + kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +   KVM_VGIC_V3_ADDR_TYPE_DIST);
>  
> - ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> - KVM_VGIC_V3_ADDR_TYPE_REDIST);
> - TEST_ASSERT(!ret, 
> "KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_REDIST supported");
> + kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
> +   KVM_VGIC_V3_ADDR_TYPE_REDIST);
>  
>   /* check non existing attribute */
>   ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, 0);
> - TEST_ASSERT(ret == -ENXIO, "attribute not supported");
> + TEST_ASSERT(ret && errno == ENXIO, "attribute not supported");
>  
>   /* misaligned DIST and REDIST address settings */
>   addr = 0x1000;
>   ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>KVM_VGIC_V3_ADDR_TYPE_DIST, , true);
> - TEST_ASSERT(ret == -EINVAL, "GICv3 dist base not 64kB aligned");
> + TEST_ASSERT(ret && errno == EINVAL, "GICv3 dist base not 64kB aligned");
>  
>   ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>KVM_VGIC_V3_ADDR_TYPE_REDIST, , true);
> - TEST_ASSERT(ret == -EINVAL, "GICv3 redist base not 64kB aligned");
> + TEST_ASSERT(ret && errno == EINVAL, "GICv3 redist base not 64kB 
> aligned");
>  
>   /* out of range address */
>   if (max_ipa_bits) {
>   addr = 1ULL << max_ipa_bits;
>   ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>KVM_VGIC_V3_ADDR_TYPE_DIST, , 
> true);
> - TEST_ASSERT(ret == -E2BIG, "dist address beyond IPA limit");
> + TEST_ASSERT(ret && errno == E2BIG, "dist address beyond IPA 
> limit");
>  
>   ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
>

[PATCH] KVM: selftests: vgic_init kvm selftests fixup

2021-04-07 Thread Eric Auger 
Bring some improvements/rationalization over the first version
of the vgic_init selftests:

- ucall_init is moved in run_cpu()
- vcpu_args_set is not called as not needed
- whenever a helper is supposed to succeed, call the non "_" version
- helpers do not return -errno, instead errno is checked by the caller
- vm_gic struct is used whenever possible, as well as vm_gic_destroy
- _kvm_create_device takes an addition fd parameter

Signed-off-by: Eric Auger 
Suggested-by: Andrew Jones 

---

Applies on top of [PATCH v6 0/9] KVM/ARM: Some vgic fixes
and init sequence KVM selftests, put on kvm-arm64/vgic-5.13

The whole patchset can be found at
https://github.com/eauger/linux/tree/vgic_kvmselftests_v7
---
 .../testing/selftests/kvm/aarch64/vgic_init.c | 275 --
 .../testing/selftests/kvm/include/kvm_util.h  |   2 +-
 tools/testing/selftests/kvm/lib/kvm_util.c|  30 +-
 3 files changed, 136 insertions(+), 171 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/vgic_init.c 
b/tools/testing/selftests/kvm/aarch64/vgic_init.c
index be1a7c0d0527..682e753fdc59 100644
--- a/tools/testing/selftests/kvm/aarch64/vgic_init.c
+++ b/tools/testing/selftests/kvm/aarch64/vgic_init.c
@@ -27,7 +27,7 @@ struct vm_gic {
int gic_fd;
 };
 
-int max_ipa_bits;
+static int max_ipa_bits;
 
 /* helper to access a redistributor register */
 static int access_redist_reg(int gicv3_fd, int vcpu, int offset,
@@ -51,12 +51,8 @@ static void guest_code(void)
 /* we don't want to assert on run execution, hence that helper */
 static int run_vcpu(struct kvm_vm *vm, uint32_t vcpuid)
 {
-   int ret;
-
-   vcpu_args_set(vm, vcpuid, 1);
-   ret = _vcpu_ioctl(vm, vcpuid, KVM_RUN, NULL);
-   get_ucall(vm, vcpuid, NULL);
-
+   ucall_init(vm, NULL);
+   int ret = _vcpu_ioctl(vm, vcpuid, KVM_RUN, NULL);
if (ret)
return -errno;
return 0;
@@ -68,7 +64,6 @@ static struct vm_gic vm_gic_create(void)
 
v.vm = vm_create_default_with_vcpus(NR_VCPUS, 0, 0, guest_code, NULL);
v.gic_fd = kvm_create_device(v.vm, KVM_DEV_TYPE_ARM_VGIC_V3, false);
-   TEST_ASSERT(v.gic_fd > 0, "GICv3 device created");
 
return v;
 }
@@ -91,66 +86,62 @@ static void subtest_dist_rdist(struct vm_gic *v)
uint64_t addr;
 
/* Check existing group/attributes */
-   ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
-   KVM_VGIC_V3_ADDR_TYPE_DIST);
-   TEST_ASSERT(!ret, "KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_DIST 
supported");
+   kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+ KVM_VGIC_V3_ADDR_TYPE_DIST);
 
-   ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
-   KVM_VGIC_V3_ADDR_TYPE_REDIST);
-   TEST_ASSERT(!ret, 
"KVM_DEV_ARM_VGIC_GRP_ADDR/KVM_VGIC_V3_ADDR_TYPE_REDIST supported");
+   kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
+ KVM_VGIC_V3_ADDR_TYPE_REDIST);
 
/* check non existing attribute */
ret = _kvm_device_check_attr(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR, 0);
-   TEST_ASSERT(ret == -ENXIO, "attribute not supported");
+   TEST_ASSERT(ret && errno == ENXIO, "attribute not supported");
 
/* misaligned DIST and REDIST address settings */
addr = 0x1000;
ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
 KVM_VGIC_V3_ADDR_TYPE_DIST, , true);
-   TEST_ASSERT(ret == -EINVAL, "GICv3 dist base not 64kB aligned");
+   TEST_ASSERT(ret && errno == EINVAL, "GICv3 dist base not 64kB aligned");
 
ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
 KVM_VGIC_V3_ADDR_TYPE_REDIST, , true);
-   TEST_ASSERT(ret == -EINVAL, "GICv3 redist base not 64kB aligned");
+   TEST_ASSERT(ret && errno == EINVAL, "GICv3 redist base not 64kB 
aligned");
 
/* out of range address */
if (max_ipa_bits) {
addr = 1ULL << max_ipa_bits;
ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
 KVM_VGIC_V3_ADDR_TYPE_DIST, , 
true);
-   TEST_ASSERT(ret == -E2BIG, "dist address beyond IPA limit");
+   TEST_ASSERT(ret && errno == E2BIG, "dist address beyond IPA 
limit");
 
ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
 KVM_VGIC_V3_ADDR_TYPE_REDIST, , 
true);
-   TEST_ASSERT(ret == -E2BIG, "redist address beyond IPA limit");
+   TEST_ASSERT(ret && errno == E2BIG, "redist address beyond IPA 
limit");
}
 
/* set REDIST base address @0x0*/
addr = 0x0;
-   ret = _kvm_device_access(v->gic_fd, KVM_DEV_ARM_VGIC_GRP_ADDR,
-KVM_VGIC_V3_ADDR_TYPE_REDIST, ,