Re: [PATCH 0/5] KVM: SVM: nSVM: Implement Enlightened MSR-Bitmap for Hyper-V-on-KVM and fix it for KVM-on-Hyper-V

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 2/1/22 15:31, Vitaly Kuznetsov wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> On 12/20/21 16:21, Vitaly Kuznetsov wrote:
>>>> Enlightened MSR-Bitmap feature implements a PV protocol for L0 and L1
>>>> hypervisors to collaborate and skip unneeded updates to MSR-Bitmap.
>>>> KVM implements the feature for KVM-on-Hyper-V but it seems there was
>>>> a flaw in the implementation and the feature may not be fully functional.
>>>> PATCHes 1-2 fix the problem. The rest of the series implements the same
>>>> feature for Hyper-V-on-KVM.
>>>>
>>>> Vitaly Kuznetsov (5):
>>>>     KVM: SVM: Drop stale comment from
>>>>       svm_hv_vmcb_dirty_nested_enlightenments()
>>>>     KVM: SVM: hyper-v: Enable Enlightened MSR-Bitmap support for real
>>>>     KVM: nSVM: Track whether changes in L0 require MSR bitmap for L2 to be
>>>>       rebuilt
>>>>     KVM: x86: Make kvm_hv_hypercall_enabled() static inline
>>>>     KVM: nSVM: Implement Enlightened MSR-Bitmap feature
>>>>
>>>>    arch/x86/kvm/hyperv.c           | 12 +--------
>>>>    arch/x86/kvm/hyperv.h           |  6 ++++-
>>>>    arch/x86/kvm/svm/nested.c       | 47 ++++++++++++++++++++++++++++-----
>>>>    arch/x86/kvm/svm/svm.c          |  3 ++-
>>>>    arch/x86/kvm/svm/svm.h          | 16 +++++++----
>>>>    arch/x86/kvm/svm/svm_onhyperv.h | 12 +++------
>>>>    6 files changed, 63 insertions(+), 33 deletions(-)
>>>>
>>>
>>> Queued 3-5 now, but it would be nice to have some testcases.
>
> Hmm, it fails to compile with CONFIG_HYPERV disabled, and a trivial
> #if also fails due to an unused goto label.  Does this look good to you?
>

Hm, it does but honestly I did not anticipate this dependency --
CONFIG_HYPERV is needed for KVM-on-Hyper-V but this feature is for
Hyper-V-on-KVM. Let me take a look tomorrow.

> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index e3759a79d39a..a2b5267b3e73 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -173,9 +173,16 @@ void recalc_intercepts(struct vcpu_svm *svm)
>    */
>   static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
>   {
> +	int i;
> +
> +	if (!(vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
> +		return true;
> +
> +	svm->vmcb->control.msrpm_base_pa = __sme_set(__pa(svm->nested.msrpm));
> +
> +#if IS_ENABLED(CONFIG_HYPERV)
>   	struct hv_enlightenments *hve =
>   		(struct hv_enlightenments *)svm->nested.ctl.reserved_sw;
> -	int i;
>   
>   	/*
>   	 * MSR bitmap update can be skipped when:
> @@ -185,10 +192,8 @@ static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
>   	    kvm_hv_hypercall_enabled(&svm->vcpu) &&
>   	    hve->hv_enlightenments_control.msr_bitmap &&
>   	    (svm->nested.ctl.clean & VMCB_HV_NESTED_ENLIGHTENMENTS))
> -		goto set_msrpm_base_pa;
> -
> -	if (!(vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
>   		return true;
> +#endif
>   
>   	for (i = 0; i < MSRPM_OFFSETS; i++) {
>   		u32 value, p;
> @@ -213,10 +216,6 @@ static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
>   	}
>   
>   	svm->nested.force_msr_bitmap_recalc = false;
> -
> -set_msrpm_base_pa:
> -	svm->vmcb->control.msrpm_base_pa = __sme_set(__pa(svm->nested.msrpm));
> -
>   	return true;
>   }
>   
>
>
> Thanks,
>
> Paolo
>

-- 
Vitaly