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.
