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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 05 Nov 2021 13:08:51 +0100
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Maxim Levitsky <mlevitsk@...hat.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/4] KVM: nVMX: Implement Enlightened MSR Bitmap feature

Sean Christopherson <seanjc@...gle.com> writes:

> 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@...hat.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
>

>From my side I can suggest to combine these two series in v4)

>>  
>>  	/* 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?
>

No need indeed, will drop in v4.

>> +
>>  	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
>> 
>

-- 
Vitaly