Re: [PATCH 1/2] KVM: selftests: Add get featured msrs test case

[Peter Xu]

On Mon, Oct 26, 2020 at 09:58:54AM +0100, Vitaly Kuznetsov wrote:
> Peter Xu <peterx@xxxxxxxxxx> writes:
> 
> > Try to fetch any supported featured msr.  Currently it won't fail, so at least
> > we can check against valid ones (which should be >0).
> >
> > This reproduces [1] too by trying to fetch one invalid msr there.
> >
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=209845
> >
> > Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> > ---
> >  .../testing/selftests/kvm/include/kvm_util.h  |  3 +
> >  tools/testing/selftests/kvm/lib/kvm_util.c    | 14 +++++
> >  .../testing/selftests/kvm/x86_64/state_test.c | 58 +++++++++++++++++++
> >  3 files changed, 75 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> > index 919e161dd289..e34cf263b20a 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> > @@ -66,6 +66,9 @@ int vm_enable_cap(struct kvm_vm *vm, struct kvm_enable_cap *cap);
> >  
> >  struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm);
> >  struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm);
> > +void kvm_vm_get_msr_feature_index_list(struct kvm_vm *vm,
> > +				       struct kvm_msr_list *list);
> > +int kvm_vm_get_feature_msrs(struct kvm_vm *vm, struct kvm_msrs *msrs);
> >  void kvm_vm_free(struct kvm_vm *vmp);
> >  void kvm_vm_restart(struct kvm_vm *vmp, int perm);
> >  void kvm_vm_release(struct kvm_vm *vmp);
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index 74776ee228f2..3c16fa044335 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -132,6 +132,20 @@ static const struct vm_guest_mode_params vm_guest_mode_params[] = {
> >  _Static_assert(sizeof(vm_guest_mode_params)/sizeof(struct vm_guest_mode_params) == NUM_VM_MODES,
> >  	       "Missing new mode params?");
> >  
> > +void kvm_vm_get_msr_feature_index_list(struct kvm_vm *vm,
> > +				       struct kvm_msr_list *list)
> > +{
> > +	int r = ioctl(vm->kvm_fd, KVM_GET_MSR_FEATURE_INDEX_LIST, list);
> > +
> > +	TEST_ASSERT(r == 0, "KVM_GET_MSR_FEATURE_INDEX_LIST failed: %d\n",
> > +		    -errno);
> > +}
> > +
> > +int kvm_vm_get_feature_msrs(struct kvm_vm *vm, struct kvm_msrs *msrs)
> > +{
> > +	return ioctl(vm->kvm_fd, KVM_GET_MSRS, msrs);
> > +}
> 
> I *think* that the non-written rule for kvm selftests is that functions
> without '_' prefix check ioctl return value with TEST_ASSERT() and
> functions with it don't (e.g. _vcpu_run()/vcpu_run()) but maybe it's
> just me.

Sure, will fix it up.

> 
> > +
> >  /*
> >   * VM Create
> >   *
> > diff --git a/tools/testing/selftests/kvm/x86_64/state_test.c b/tools/testing/selftests/kvm/x86_64/state_test.c
> > index f6c8b9042f8a..7ce9920e526a 100644
> > --- a/tools/testing/selftests/kvm/x86_64/state_test.c
> > +++ b/tools/testing/selftests/kvm/x86_64/state_test.c
> 
> I would not overload state_test with this new check and create a new
> one. The benefit is that when one of these tests fail we still get the
> result of the other one so it's not 'everything works' vs 'everything is
> broken' type of log.

IMHO it's not extremely important on knowingg which binary failed - afaiu,
kernel selftests are really for an "all pass", so if anything fails, we dig.

Another thing, state_test.c has a comment (at the top):

/*
 * KVM_GET/SET_* tests
 *
 * Copyright (C) 2018, Red Hat, Inc.
 ...

Shouldn't KVM_GET_MSRS suites here? :)

Thanks,

-- 
Peter Xu