RE: [PATCH v13 002/113] KVM: x86/vmx: Refactor KVM VMX module init/exit functions

[Wang, Wei W]

On Tuesday, March 14, 2023 2:40 AM, Isaku Yamahata wrote:
> > I had a patch to fix a bug here, maybe you can take it:
> >
> > kvm_x86_vendor_init copies vt_x86_ops to kvm_x86_ops.
> > vt_x86_ops.vm_size needs to be updated before calling
> > kvm_x86_vendor_init so that kvm_x86_ops can get the correct vm_size.
> 
> Thanks for catching it.  With your patch, vm_size is always max(sizeof struct
> kvm_vmx, sizeof strut kvm_tdx) even when the admin sets kvm_intel.tdx=true
> and tdx is disabled by error.

This seems to be another bug, where enable_tdx should be set to false if tdx
fails to be enabled:

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index d6a9f705a2a1..ef1e794efb97 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -4552,6 +4552,7 @@ static int tdx_module_setup(void)

        ret = tdx_enable();
        if (ret) {
+               enable_tdx = false;
                pr_info("Failed to initialize TDX module.\n");
                return ret;
        }

> 
> option 1: Ignore such waste. Your patch. The difference is small and it's only
>           the error case. Locally I have the following values.
>           sizeof(struct kvm_vmx) = 44576
>           sizeof(struct vcpu_vmx) = 10432
>           sizeof(struct kvm_tdx)= 44632
>           sizeof(struct vcpu_tdx) = 8192
>           I suspect the actual allocation size for struct kvm is same.  That's
>           the reason why I didn't hit problem.

No, the actual allocation size isn't same.
You didn’t get error notices because the kvm_tdx fields are still located
in the same page as that allocated for kvm_vmx, though that part of memory
isn’t explicitly allocated. If kvm_tdx size grows and crosses the page boundary,
you will see unexpected faults. Or if this part of memory (illegally used by
kvm_tdx) later happens to be given to other kernel components to use, you
would see random errors somewhere.

> 
> option 2: Explicitly update vm_size after kvm_x86_vendor_init()
>           struct kvm_x86_ops isn't exported.  It would be ugly.
> 
> option 3: Allow setup_hardware() to update vm_size.
>           setup_hardware(void) => setup_hardware(unsigned int *vm_size)
>           It's confusing because kvm_x86_ops.vm_size is already initialized.
> 
> Let's go with option 1(your patch).

Sounds good.