Re: [PATCH v3 4/4] KVM: nVMX: Implement Enlightened MSR Bitmap feature

Sean Christopherson
2021-11-05T01:10:00Z

On Wed, Oct 13, 2021, Vitaly Kuznetsov wrote:
> Updating MSR bitmap for L2 is not cheap and rearly needed. TLFS for Hyper-V
> offers 'Enlightened MSR Bitmap' feature which allows L1 hypervisor to
> inform L0 when it changes MSR bitmap, this eliminates the need to examine
> L1's MSR bitmap for L2 every time when 'real' MSR bitmap for L2 gets
> constructed.
>
> Use 'vmx->nested.msr_bitmap_changed' flag to implement the feature.
>
> Note, KVM already uses 'Enlightened MSR bitmap' feature when it runs as a
> nested hypervisor on top of Hyper-V. The newly introduced feature is going
> to be used by Hyper-V guests on KVM.
>
> When the feature is enabled for Win10+WSL2, it shaves off around 700 CPU
> cycles from a nested vmexit cost (tight cpuid loop test).
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> arch/x86/kvm/hyperv.c | 2 ++
> arch/x86/kvm/vmx/nested.c | 20 ++++++++++++++++++--
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 6f11cda2bfa4..a00de1dbec57 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -2516,6 +2516,8 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>
> case HYPERV_CPUID_NESTED_FEATURES:
> ent->eax = evmcs_ver;
> + if (evmcs_ver)
> + ent->eax |= HV_X64_NESTED_MSR_BITMAP;
>
> break;
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index bf4fa63ed371..7cd0c20d4557 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -608,15 +608,30 @@ static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
> struct vmcs12 *vmcs12)
> {
> int msr;
> + struct vcpu_vmx *vmx = to_vmx(vcpu);
> unsigned long *msr_bitmap_l1;
> - unsigned long *msr_bitmap_l0 = to_vmx(vcpu)->nested.vmcs02.msr_bitmap;
> - struct kvm_host_map *map = &to_vmx(vcpu)->nested.msr_bitmap_map;
> + unsigned long *msr_bitmap_l0 = vmx->nested.vmcs02.msr_bitmap;
> + struct hv_enlightened_vmcs *evmcs = vmx->nested.hv_evmcs;
> + struct kvm_host_map *map = &vmx->nested.msr_bitmap_map;

That reminds me, can my nested bitmap fixes get merged? Superficial conflicts,
but still conflicts that I'd rather not have to resolve :-)

https://lkml.kernel.org/r/20210924204907.1111817-1-seanjc@google.com

>
> /* Nothing to do if the MSR bitmap is not in use. */
> if (!cpu_has_vmx_msr_bitmap() ||
> !nested_cpu_has(vmcs12, CPU_BASED_USE_MSR_BITMAPS))
> return false;
>
> + /*
> + * MSR bitmap update can be skipped when:
> + * - MSR bitmap for L1 hasn't changed.
> + * - Nested hypervisor (L1) is attempting to launch the same L2 as
> + * before.
> + * - Nested hypervisor (L1) has enabled 'Enlightened MSR Bitmap' feature
> + * and tells KVM (L0) there were no changes in MSR bitmap for L2.
> + */
> + if (!vmx->nested.msr_bitmap_force_recalc && evmcs &&
> + evmcs->hv_enlightenments_control.msr_bitmap &&
> + evmcs->hv_clean_fields & HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP)
> + goto out_clear_msr_bitmap_force_recalc;

Huh? Why clear it, it's already clear. Any reason not to simply return true?

> +
> if (kvm_vcpu_map(vcpu, gpa_to_gfn(vmcs12->msr_bitmap), map))
> return false;
>
> @@ -700,6 +715,7 @@ static inline bool nested_vmx_prepare_msr_bitmap(struct kvm_vcpu *vcpu,
>
> kvm_vcpu_unmap(vcpu, &to_vmx(vcpu)->nested.msr_bitmap_map, false);
>
> +out_clear_msr_bitmap_force_recalc:
> vmx->nested.msr_bitmap_force_recalc = false;
>
> return true;
> --
> 2.31.1
>