Re: [PATCH 13/14] KVM: selftests: Let dirty_log_test async for dirty ring test

From: Peter Xu <peterx@redhat.com>
To: Andrew Jones <drjones@redhat.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	dinechin@redhat.com, sean.j.christopherson@intel.com,
	pbonzini@redhat.com, jasowang@redhat.com, yan.y.zhao@intel.com,
	mst@redhat.com, kevin.tian@intel.com, alex.williamson@redhat.com,
	dgilbert@redhat.com, vkuznets@redhat.com
Subject: Re: [PATCH 13/14] KVM: selftests: Let dirty_log_test async for dirty ring test
Date: Wed, 5 Feb 2020 10:55:51 -0500	[thread overview]
Message-ID: <20200205155551.GB378317@xz-x1> (raw)
In-Reply-To: <20200205094806.dqkzpxhrndocjl6g@kamzik.brq.redhat.com>

On Wed, Feb 05, 2020 at 10:48:06AM +0100, Andrew Jones wrote:
> On Tue, Feb 04, 2020 at 09:58:41PM -0500, Peter Xu wrote:
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> > index 4b78a8d3e773..e64fbfe6bbd5 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> > @@ -115,6 +115,7 @@ vm_paddr_t addr_gva2gpa(struct kvm_vm *vm, vm_vaddr_t gva);
> >  struct kvm_run *vcpu_state(struct kvm_vm *vm, uint32_t vcpuid);
> >  void vcpu_run(struct kvm_vm *vm, uint32_t vcpuid);
> >  int _vcpu_run(struct kvm_vm *vm, uint32_t vcpuid);
> > +int __vcpu_run(struct kvm_vm *vm, uint32_t vcpuid);
> >  void vcpu_run_complete_io(struct kvm_vm *vm, uint32_t vcpuid);
> >  void vcpu_set_mp_state(struct kvm_vm *vm, uint32_t vcpuid,
> >  		       struct kvm_mp_state *mp_state);
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index 25edf20d1962..5137882503bd 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -1203,6 +1203,14 @@ int _vcpu_run(struct kvm_vm *vm, uint32_t vcpuid)
> >  	return rc;
> >  }
> >  
> > +int __vcpu_run(struct kvm_vm *vm, uint32_t vcpuid)
> > +{
> > +	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
> > +
> > +	TEST_ASSERT(vcpu != NULL, "vcpu not found, vcpuid: %u", vcpuid);
> > +	return ioctl(vcpu->fd, KVM_RUN, NULL);
> > +}
> > +
> >  void vcpu_run_complete_io(struct kvm_vm *vm, uint32_t vcpuid)
> >  {
> >  	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
> 
> I think we should add a vcpu_get_fd(vm, vcpuid) function instead, and
> then call ioctl directly from the test.

Currently the vcpu struct is still internal to the lib/ directory (as
defined in lib/kvm_util_internal.h).  Wit that, it seems the vcpu fd
should also be limited to the lib/ as well?

But I feel like I got your point, because when I worked on the
selftests I did notice that in many places it's easier to expose all
these things for test cases (e.g., the struct vcpu).  For me, it's not
only for the vcpu fd, but also for the rest of internal structures to
be able to be accessed from tests directly.  Not sure whether that's
what you thought too.  It's just a separate topic of what this series
was trying to do.

Thanks,

-- 
Peter Xu