Re: [PATCH v2 06/12] selftests: KVM: Add helpers for vCPU device attributes

From: Andrew Jones <drjones@redhat.com>
To: Oliver Upton <oupton@google.com>
Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
	Marc Zyngier <maz@kernel.org>,
	Raghavendra Rao Anata <rananta@google.com>,
	Peter Shier <pshier@google.com>,
	Sean Christopherson <seanjc@google.com>,
	David Matlack <dmatlack@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	linux-arm-kernel@lists.infradead.org,
	Jim Mattson <jmattson@google.com>
Subject: Re: [PATCH v2 06/12] selftests: KVM: Add helpers for vCPU device attributes
Date: Wed, 21 Jul 2021 17:14:58 +0200	[thread overview]
Message-ID: <20210721151458.bsfbjtk7rmlupfve@gator> (raw)
In-Reply-To: <20210716212629.2232756-7-oupton@google.com>

On Fri, Jul 16, 2021 at 09:26:23PM +0000, Oliver Upton wrote:
> vCPU file descriptors are abstracted away from test code in KVM
> selftests, meaning that tests cannot directly access a vCPU's device
> attributes. Add helpers that tests can use to get at vCPU device
> attributes.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  .../testing/selftests/kvm/include/kvm_util.h  |  9 +++++
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 38 +++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index a8ac5d52e17b..1b3ef5757819 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -240,6 +240,15 @@ int _kvm_device_access(int dev_fd, uint32_t group, uint64_t attr,
>  int kvm_device_access(int dev_fd, uint32_t group, uint64_t attr,
>  		      void *val, bool write);
>  
> +int _vcpu_has_device_attr(struct kvm_vm *vm, uint32_t vcpuid, uint32_t group,
> +			  uint64_t attr);
> +int vcpu_has_device_attr(struct kvm_vm *vm, uint32_t vcpuid, uint32_t group,
> +			 uint64_t attr);
> +int _vcpu_access_device_attr(struct kvm_vm *vm, uint32_t vcpuid, uint32_t group,
> +			  uint64_t attr, void *val, bool write);
> +int vcpu_access_device_attr(struct kvm_vm *vm, uint32_t vcpuid, uint32_t group,
> +			 uint64_t attr, void *val, bool write);
> +
>  const char *exit_reason_str(unsigned int exit_reason);
>  
>  void virt_pgd_alloc(struct kvm_vm *vm);
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 10a8ed691c66..b595e7dc3fc5 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -2040,6 +2040,44 @@ int kvm_device_access(int dev_fd, uint32_t group, uint64_t attr,
>  	return ret;
>  }
>  
> +int _vcpu_has_device_attr(struct kvm_vm *vm, uint32_t vcpuid, uint32_t group,
> +			  uint64_t attr)
> +{
> +	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
> +
> +	TEST_ASSERT(vcpu, "nonexistent vcpu id: %d", vcpuid);
> +
> +	return _kvm_device_check_attr(vcpu->fd, group, attr);
> +}
> +
> +int vcpu_has_device_attr(struct kvm_vm *vm, uint32_t vcpuid, uint32_t group,
> +				 uint64_t attr)
> +{
> +	int ret = _vcpu_has_device_attr(vm, vcpuid, group, attr);
> +
> +	TEST_ASSERT(!ret, "KVM_HAS_DEVICE_ATTR IOCTL failed, rc: %i errno: %i", ret, errno);
> +	return ret;
> +}
> +
> +int _vcpu_access_device_attr(struct kvm_vm *vm, uint32_t vcpuid, uint32_t group,
> +			     uint64_t attr, void *val, bool write)
> +{
> +	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
> +
> +	TEST_ASSERT(vcpu, "nonexistent vcpu id: %d", vcpuid);
> +
> +	return _kvm_device_access(vcpu->fd, group, attr, val, write);
> +}
> +
> +int vcpu_access_device_attr(struct kvm_vm *vm, uint32_t vcpuid, uint32_t group,
> +			    uint64_t attr, void *val, bool write)
> +{
> +	int ret = _vcpu_access_device_attr(vm, vcpuid, group, attr, val, write);
> +
> +	TEST_ASSERT(!ret, "KVM_SET|GET_DEVICE_ATTR IOCTL failed, rc: %i errno: %i", ret, errno);
> +	return ret;
> +}

Reviewed-by: Andrew Jones <drjones@redhat.com>

The 'assert !ret's are correct here. I see they are not correct in 

 kvm_device_check_attr
 kvm_create_device
 kvm_device_access

though, as they are 'assert ret >= 0', but the documentation says 0 on
success. It'd be nice to get that fixed before we build more API on top
of it.

Thanks,
drew

> +
>  /*
>   * VM Dump
>   *
> -- 
> 2.32.0.402.g57bb445576-goog
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 