Re: [PATCH v3 2/3] KVM: x86: Use kvzalloc() to allocate VM struct

[Paolo Bonzini <pbonzini@redhat.com>]

On 5/16/25 23:54, Sean Christopherson wrote:
> 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.  For non-debug builds, The SVM and VMX
> (and TDX) structures are now just below 7000 bytes in the worst case
> scenario (see below), 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 of the
> structures, sans the memslos hash tables, are order-0 allocations, i.e.
> are less than 4KiB.  There's nothing fundamentally wrong with a larger
> kvm_{svm,vmx,tdx} size, but given that the size of the structure (without
> the memslots hash tables) is below 2KiB after 18+ years of existence,
> more than doubling the size would be quite notable.
> 
> Add sanity checks on the memslot hash table sizes, partly to ensure they
> aren't resized without accounting for the impact on VM structure size, and
> partly to document that the majority of the size of VM structures comes
> from the memslots.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/include/asm/kvm_host.h |  2 +-
>   arch/x86/kvm/svm/svm.c          |  2 ++
>   arch/x86/kvm/vmx/main.c         |  2 ++
>   arch/x86/kvm/vmx/vmx.c          |  2 ++
>   arch/x86/kvm/x86.h              | 22 ++++++++++++++++++++++
>   5 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 9667d6b929ee..3a985825a945 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1961,7 +1961,7 @@ void kvm_x86_vendor_exit(void);
>   #define __KVM_HAVE_ARCH_VM_ALLOC
>   static inline struct kvm *kvm_arch_alloc_vm(void)
>   {
> -	return __vmalloc(kvm_x86_ops.vm_size, GFP_KERNEL_ACCOUNT | __GFP_ZERO);
> +	return kvzalloc(kvm_x86_ops.vm_size, GFP_KERNEL_ACCOUNT);
>   }
>   
>   #define __KVM_HAVE_ARCH_VM_FREE
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 0ad1a6d4fb6d..d13e475c3407 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -5675,6 +5675,8 @@ static int __init svm_init(void)
>   {
>   	int r;
>   
> +	KVM_SANITY_CHECK_VM_STRUCT_SIZE(kvm_svm);
> +
>   	__unused_size_checks();
>   
>   	if (!kvm_is_svm_supported())
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index d1e02e567b57..e18dfada2e90 100644
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -64,6 +64,8 @@ static __init int vt_hardware_setup(void)
>   		vt_x86_ops.protected_apic_has_interrupt = tdx_protected_apic_has_interrupt;
>   	}
>   
> +	KVM_SANITY_CHECK_VM_STRUCT_SIZE(kvm_tdx);

I would put either both or no checks in main.c.

Or if you use static_assert, you can also place the macro invocation 
close to the struct definition.

Paolo

> + */
> +#define KVM_SANITY_CHECK_VM_STRUCT_SIZE(x)						\
> +do {											\
> +	BUILD_BUG_ON(get_order(sizeof(struct x) - SIZE_OF_MEMSLOTS_HASHTABLE) &&	\
> +		     !IS_ENABLED(CONFIG_DEBUG_KERNEL) && !IS_ENABLED(CONFIG_KASAN));	\
> +	BUILD_BUG_ON(get_order(sizeof(struct x)) < 2 &&					\
> +		     !IS_ENABLED(CONFIG_DEBUG_KERNEL) && !IS_ENABLED(CONFIG_KASAN));	\
> +} while (0)
>   #define KVM_NESTED_VMENTER_CONSISTENCY_CHECK(consistency_check)		\
>   ({									\
>   	bool failed = (consistency_check);				\