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

[Andrew Jones]

On Wed, Feb 05, 2020 at 10:55:51AM -0500, Peter Xu wrote:
> 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.

So far I've just wished I could get to the fd, which seems reasonable
since it's an fd. I agree the whole internal thing is probably
unnecessary, but nobody (including me) has complained enough yet to
undo it. For this patch series I'd prefer we start heading in the
expose more direction, than in the yet another variant of vcpu_run
direction though.

Thanks,
drew