RE: [PATCH v1 01/18] KVM: selftests/kvm_util: use array of pointers to maintain vcpus in kvm_vm

[Wang, Wei W]

On Thursday, October 27, 2022 7:48 AM, Sean Christopherson wrote:
> > +	for (i = 0, vcpu = vm->vcpus[0];				\
> > +		vcpu && i < KVM_MAX_VCPUS; vcpu = vm->vcpus[++i])
> 
> I hate pointer arithmetic more than most people, but in this case it avoids the
> need to pass in 'i', which in turn cuts down on boilerplate and churn.

Hmm, indeed, this can be improved, how about this one:

+#define vm_iterate_over_vcpus(vm, vcpu)                         \
+       for (vcpu = vm->vcpus[0]; vcpu; vcpu = vm->vcpus[vcpu->id + 1]) \


> 
> >  #endif /* SELFTEST_KVM_UTIL_H */
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > index e42a09cd24a0..c90a9609b853 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > @@ -45,7 +45,6 @@ struct userspace_mem_region {  };
> >
> >  struct kvm_vcpu {
> > -	struct list_head list;
> >  	uint32_t id;
> >  	int fd;
> >  	struct kvm_vm *vm;
> > @@ -75,7 +74,6 @@ struct kvm_vm {
> >  	unsigned int pa_bits;
> >  	unsigned int va_bits;
> >  	uint64_t max_gfn;
> > -	struct list_head vcpus;
> >  	struct userspace_mem_regions regions;
> >  	struct sparsebit *vpages_valid;
> >  	struct sparsebit *vpages_mapped;
> > @@ -92,6 +90,7 @@ struct kvm_vm {
> >  	int stats_fd;
> >  	struct kvm_stats_header stats_header;
> >  	struct kvm_stats_desc *stats_desc;
> > +	struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
> 
> We can dynamically allocate the array without too much trouble, though I'm not
> sure it's worth shaving the few KiB of memory.  For __vm_create(), the number
> of vCPUs is known when the VM is created.  For vm_create_barebones(), we
> could do the simple thing of allocating KVM_MAX_VCPU.

The issue with dynamic allocation is that some users start with __vm_create(nr_vcpus), and later
could add more cpus with vm_vcpu_add (e.g. x86_64/xapic_ipi_test.c). To support this we may
need to re-allocate the array for later vm_vcpu_add(), and also need to add nr_vcpus to indicate
the size.
It's userspace memory, and not a problem to use a bit larger virtual memory (memory are not really
allocated until we have that many vcpus to touch the array entries), I think.

> 
> > @@ -534,6 +533,10 @@ __weak void vcpu_arch_free(struct kvm_vcpu *vcpu)
> > static void vm_vcpu_rm(struct kvm_vm *vm, struct kvm_vcpu *vcpu)  {
> >  	int ret;
> > +	uint32_t vcpu_id = vcpu->id;
> > +
> > +	TEST_ASSERT(!!vm->vcpus[vcpu_id], "vCPU%d wasn't added\n", vcpu_id);
> 
> This is unecessary, there's one caller and it's iterating over the array of vCPUs.

That's right, thanks.