Re: [PATCH v8 2/4] selftests: KVM: Add fpu and one reg set/get library functions

Andrew Jones
2020-01-30T14:30:22Z

On Thu, Jan 30, 2020 at 03:10:55PM +0100, Janosch Frank wrote:
> On 1/30/20 2:55 PM, Andrew Jones wrote:
> > On Thu, Jan 30, 2020 at 11:36:21AM +0100, Thomas Huth wrote:
> >> On 29/01/2020 21.03, Janosch Frank wrote:
> >>> Add library access to more registers.
> >>>
> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >>> ---
> >>> .../testing/selftests/kvm/include/kvm_util.h | 6 +++
> >>> tools/testing/selftests/kvm/lib/kvm_util.c | 48 +++++++++++++++++++
> >>> 2 files changed, 54 insertions(+)
> >>>
> >>> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> >>> index 29cccaf96baf..ae0d14c2540a 100644
> >>> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> >>> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> >>> @@ -125,6 +125,12 @@ void vcpu_sregs_set(struct kvm_vm *vm, uint32_t vcpuid,
> >>> struct kvm_sregs *sregs);
> >>> int _vcpu_sregs_set(struct kvm_vm *vm, uint32_t vcpuid,
> >>> struct kvm_sregs *sregs);
> >>> +void vcpu_fpu_get(struct kvm_vm *vm, uint32_t vcpuid,
> >>> + struct kvm_fpu *fpu);
> >>> +void vcpu_fpu_set(struct kvm_vm *vm, uint32_t vcpuid,
> >>> + struct kvm_fpu *fpu);

nit: no need for the above line breaks. We don't even get to 80 char.

> >>> +void vcpu_get_reg(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_one_reg *reg);
> >>> +void vcpu_set_reg(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_one_reg *reg);
> >>> #ifdef __KVM_HAVE_VCPU_EVENTS
> >>> void vcpu_events_get(struct kvm_vm *vm, uint32_t vcpuid,
> >>> struct kvm_vcpu_events *events);
> >>> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> >>> index 41cf45416060..dae117728ec6 100644
> >>> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> >>> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> >>> @@ -1373,6 +1373,54 @@ int _vcpu_sregs_set(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_sregs *sregs)
> >>> return ioctl(vcpu->fd, KVM_SET_SREGS, sregs);
> >>> }
> >>>
> >>> +void vcpu_fpu_get(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_fpu *fpu)
> >>> +{
> >>> + struct vcpu *vcpu = vcpu_find(vm, vcpuid);
> >>> + int ret;
> >>> +
> >>> + TEST_ASSERT(vcpu != NULL, "vcpu not found, vcpuid: %u", vcpuid);
> >>> +
> >>> + ret = ioctl(vcpu->fd, KVM_GET_FPU, fpu);
> >>> + TEST_ASSERT(ret == 0, "KVM_GET_FPU failed, rc: %i errno: %i",
> >>> + ret, errno);
> >>> +}
> >>> +
> >>> +void vcpu_fpu_set(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_fpu *fpu)
> >>> +{
> >>> + struct vcpu *vcpu = vcpu_find(vm, vcpuid);
> >>> + int ret;
> >>> +
> >>> + TEST_ASSERT(vcpu != NULL, "vcpu not found, vcpuid: %u", vcpuid);
> >>> +
> >>> + ret = ioctl(vcpu->fd, KVM_SET_FPU, fpu);
> >>> + TEST_ASSERT(ret == 0, "KVM_SET_FPU failed, rc: %i errno: %i",
> >>> + ret, errno);
> >>> +}
> >>> +
> >>> +void vcpu_get_reg(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_one_reg *reg)
> >>> +{
> >>> + struct vcpu *vcpu = vcpu_find(vm, vcpuid);
> >>> + int ret;
> >>> +
> >>> + TEST_ASSERT(vcpu != NULL, "vcpu not found, vcpuid: %u", vcpuid);
> >>> +
> >>> + ret = ioctl(vcpu->fd, KVM_GET_ONE_REG, reg);
> >>> + TEST_ASSERT(ret == 0, "KVM_GET_ONE_REG failed, rc: %i errno: %i",
> >>> + ret, errno);
> >>> +}
> >>> +
> >>> +void vcpu_set_reg(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_one_reg *reg)
> >>> +{
> >>> + struct vcpu *vcpu = vcpu_find(vm, vcpuid);
> >>> + int ret;
> >>> +
> >>> + TEST_ASSERT(vcpu != NULL, "vcpu not found, vcpuid: %u", vcpuid);
> >>> +
> >>> + ret = ioctl(vcpu->fd, KVM_SET_ONE_REG, reg);
> >>> + TEST_ASSERT(ret == 0, "KVM_SET_ONE_REG failed, rc: %i errno: %i",
> >>> + ret, errno);
> >>> +}
> >>> +
> >>> /*
> >>> * VCPU Ioctl
> >>> *
> >>>
> >>
> >> Reviewed-by: Thomas Huth <thuth@redhat.com>
> >>
> >
> > How about what's below instead. It should be equivalent.
>
> With your proposed changes we loose a bit verbosity in the error
> messages. I need to think about which I like more.

Looks like both error messages are missing something. The ones above are
missing the string version of errno. The ones below are missing the string
version of cmd. It's easy to add the string version of errno, which is
an argument for keeping the functions above (but we could at least use
_vcpu_ioctl to avoid duplicating the vcpu_find and vcpu!=NULL assert).
Or, we could consider adding a kvm_ioctl_cmd_to_string() function,
which might be nice for other ioctl wrappers now and in the future.
It shouldn't be too bad to generate a string table from kvm.h, but of
course we'd have to keep it maintained.

Thanks,
drew

>
> >
> > Thanks,
> > drew
> >
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> > index 29cccaf96baf..d96a072e69bf 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> > @@ -125,6 +125,31 @@ void vcpu_sregs_set(struct kvm_vm *vm, uint32_t vcpuid,
> > struct kvm_sregs *sregs);
> > int _vcpu_sregs_set(struct kvm_vm *vm, uint32_t vcpuid,
> > struct kvm_sregs *sregs);
> > +
> > +static inline void
> > +vcpu_fpu_get(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_fpu *fpu)
> > +{
> > + vcpu_ioctl(vm, vcpuid, KVM_GET_FPU, fpu);
> > +}
> > +
> > +static inline void
> > +vcpu_fpu_set(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_fpu *fpu)
> > +{
> > + vcpu_ioctl(vm, vcpuid, KVM_SET_FPU, fpu);
> > +}
> > +
> > +static inline void
> > +vcpu_get_reg(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_one_reg *reg)
> > +{
> > + vcpu_ioctl(vm, vcpuid, KVM_GET_ONE_REG, reg);
> > +}
> > +
> > +static inline void
> > +vcpu_set_reg(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_one_reg *reg)
> > +{
> > + vcpu_ioctl(vm, vcpuid, KVM_SET_ONE_REG, reg);
> > +}
> > +
> > #ifdef __KVM_HAVE_VCPU_EVENTS
> > void vcpu_events_get(struct kvm_vm *vm, uint32_t vcpuid,
> > struct kvm_vcpu_events *events);
> >
>
>