Re: [PATCH v1 03/28] KVM: VMX: Generalize VPID allocation to be vendor-neutral

[Yosry Ahmed]

> @@ -8711,8 +8685,6 @@ __init int vmx_hardware_setup(void)
>         kvm_caps.has_bus_lock_exit = cpu_has_vmx_bus_lock_detection();
>         kvm_caps.has_notify_vmexit = cpu_has_notify_vmexit();
>
> -       set_bit(0, vmx_vpid_bitmap); /* 0 is reserved for host */
> -
>         if (enable_ept)
>                 kvm_mmu_set_ept_masks(enable_ept_ad_bits);
>         else
> @@ -8777,6 +8749,10 @@ __init int vmx_hardware_setup(void)
>
>         vmx_set_cpu_caps();
>
> +       r = init_vpids();
> +       if (r)
> +               return r;
> +

>From internal Sashiko:
---
If tdx_hardware_setup() fails after vmx_hardware_setup() succeeds, will
this leak the dynamically allocated tlb_tags.bitmap?

Looking at vt_hardware_setup() in arch/x86/kvm/vmx/main.c:

arch/x86/kvm/vmx/main.c:vt_hardware_setup() {
        ...
        ret = vmx_hardware_setup();
        if (ret)
                return ret;
        return enable_tdx ? tdx_hardware_setup() : 0;
}

When tdx_hardware_setup() fails, it returns the error code directly without
calling vmx_hardware_unsetup(). KVM's core setup (kvm_x86_vendor_init)
expects vendor routines to handle their own partial initialization failures
and skips hardware_unsetup() if hardware_setup() fails.

Since this patch changes the VPID bitmap from a static array to a dynamic
allocation in init_vpids(), this error path will now leave tlb_tags.bitmap
permanently leaked on module initialization failure.

Does vt_hardware_setup() need to call vmx_hardware_unsetup() when TDX
setup fails to ensure the new dynamic allocations are freed?
---

I think this is a pre-existing issue. The allocations made by
nested_vmx_hardware_setup() are also not properly cleaned up.

>         /*
>          * Configure nested capabilities after core CPU capabilities so that
>          * nested support can be conditional on base support, e.g. so that KVM
> @@ -8784,8 +8760,10 @@ __init int vmx_hardware_setup(void)
>          */
>         if (nested) {
>                 r = nested_vmx_hardware_setup(kvm_vmx_exit_handlers);
> -               if (r)
> +               if (r) {
> +                       destroy_vpids();
>                         return r;
> +               }
>         }
>         vmx_nested_ops.enabled = nested;
>