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"?
>
>
> > What should the developer do? Is it a hard requirement that the size should
> > never go beyond 4K? Or, should the assert of order 0 allocation be changed to
> > the assert of order 1 allocation?
>
> It depends. Now that Vipin has corrected my math, the assertion will be that the
> VM struct is order-1 or smaller, i.e. <= 8KiB. That gives us a _lot_ of room to
> grow. E.g. KVM has existed for ~18 years and is barely about 4KiB, so for organic
> growth (small additions here and there), I don't expect to hit the 8KiB limit in
> the next decade (famous last words). And the memory landscape will likely be
> quite different 10+ years from now, i.e. the assertion may be completely unnecessary
> by the time it fires.
>
> What I'm most interested in detecting and prevent is things like mmu_page_hash,
> where a massive field is embedded in struct kvm for an *optional* feature. I.e.
> if a new feature adds a massive field, then it should probably be placed in a
> separate, dynamically allocated structure. And for those, it should be quite
> obvious that a separate allocation is the way to go.
Agreed. Thanks for explaining.
