Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 27 Oct 2022 15:27:21 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     "Wang, Wei W" <wei.w.wang@intel.com>
Cc:     "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "dmatlack@google.com" <dmatlack@google.com>,
        "vipinsh@google.com" <vipinsh@google.com>,
        "ajones@ventanamicro.com" <ajones@ventanamicro.com>,
        "eric.auger@redhat.com" <eric.auger@redhat.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 01/18] KVM: selftests/kvm_util: use array of pointers
 to maintain vcpus in kvm_vm
Message-ID: <Y1qjWZ8Lpveyky9m@google.com>
References: <20221024113445.1022147-1-wei.w.wang@intel.com>
 <20221024113445.1022147-2-wei.w.wang@intel.com>
 <Y1nHL5BUoWPqUtt9@google.com>
 <DS0PR11MB6373E471C1378CBEEBD40A82DC339@DS0PR11MB6373.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <DS0PR11MB6373E471C1378CBEEBD40A82DC339@DS0PR11MB6373.namprd11.prod.outlook.com>
Precedence: bulk

On Thu, Oct 27, 2022, Wang, Wei W wrote:
> 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]) \

Needs to be bounded by the size of the array.
 
> > >  #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.

Hrm, right, the number of runnable CPUs isn't a hard upper bound.  Ideally it
would be, as the number of pages required for guest memory will fail to account
for the "extra" vcpus.  E.g. that test should really do vm_create(2) and then
manually add each vCPU.

> 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.

Yeah, just allocate the max for now, though the array still needs to be dynamically
allocated based on the actual maximum number of vCPUs.  Oh, duh, we can do the
easy thing and just bump KVM_MAX_VCPUS to 1024 to match KVM.  And then assert that
kvm_check_cap(KVM_CAP_MAX_VCPUS) == KVM_CAP_MAX_VCPUS in kvm_create_max_vcpus.c.