Re: [PATCH v2 2/3] KVM: x86: Allocate kvm_vmx/kvm_svm structures using kzalloc()

[Sean Christopherson <seanjc@google.com>]

On Wed, Apr 23, 2025, Kai Huang wrote:
> On Wed, 2025-04-23 at 10:07 -0700, Sean Christopherson wrote:
> > On Tue, Apr 22, 2025, Kai Huang wrote:
> > > On Wed, 2025-04-16 at 12:57 -0700, Sean Christopherson wrote:
> > > > On Wed, Apr 16, 2025, Vipin Sharma wrote:
> > > > > Checked via pahole, sizes of struct have reduced but still not under 4k.
> > > > > After applying the patch:
> > > > > 
> > > > > struct kvm{} - 4104
> > > > > struct kvm_svm{} - 4320
> > > > > struct kvm_vmx{} - 4128
> > > > > 
> > > > > Also, this BUILD_BUG_ON() might not be reliable unless all of the ifdefs
> > > > > under kvm_[vmx|svm] and its children are enabled. Won't that be an
> > > > > issue?
> > > > 
> > > > That's what build bots (and to a lesser extent, maintainers) are for.  An individual
> > > > developer might miss a particular config, but the build bots that run allyesconfig
> > > > will very quickly detect the issue, and then we fix it.
> > > > 
> > > > I also build what is effectively an "allkvmconfig" before officially applying
> > > > anything, so in general things like this shouldn't even make it to the bots.
> > > > 
> > > 
> > > Just want to understand the intention here:
> > > 
> > > What if someday a developer really needs to add some new field(s) to, lets say
> > > 'struct kvm_vmx', and that makes the size exceed 4K?
> > 
> > If it helps, here's the changelog I plan on posting for v3:
> >     
> >     Allocate VM structs via kvzalloc(), i.e. try to use a contiguous physical
> >     allocation before falling back to __vmalloc(), to avoid the overhead of
> >     establishing the virtual mappings.  The SVM and VMX (and TDX) structures
> >     are now just above 4096 bytes, i.e. are order-1 allocations, and will
> >     likely remain that way for quite some time.
> >     
> >     Add compile-time assertions in vendor code to ensure the size is an
> >     order-0 or order-1 allocation, i.e. to prevent unknowingly letting the
> >     size balloon in the future.  There's nothing fundamentally wrong with a
> >     larger kvm_{svm,vmx,tdx} size, but given that the size is barely above
> >     4096 after 18+ years of existence, exceeding exceed 8192 bytes would be
> >     quite notable.
> 
> Yeah looks reasonable.
> 
> Nit: I am not quite following "falling back to __vmalloc()" part.  We are
> replacing __vmalloc() with kzalloc() AFAICT, therefore there should be no
> "falling back"?

Correct, not in this version.  In the next version, my plan is to use kvzalloc()
(though honestly, I'm not sure that's worth doing; it'll be an order-1 allocation,
and if that fails...).