From mboxrd@z Thu Jan  1 00:00:00 1970
From: Vitaly Kuznetsov <vkuznets@redhat.com>
Subject: Re: [PATCH v2 6/6] KVM: nVMX: optimize prepare_vmcs02{,_full} for Enlightened VMCS case
Date: Wed, 25 Jul 2018 14:50:56 +0200
Message-ID: <87in53pjgv.fsf@vitty.brq.redhat.com>
References: <20180621123046.29606-1-vkuznets@redhat.com>
        <20180621123046.29606-7-vkuznets@redhat.com>
        <a826fd88-0199-8e7b-ebfc-290e936b5efc@redhat.com>
        <87va93pv6w.fsf@vitty.brq.redhat.com>
        <46052d1e-9ee1-8cee-3f7c-cf27b1cd0373@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain
Cc: kvm@vger.kernel.org,
        Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        Roman Kagan <rkagan@virtuozzo.com>,
        "K. Y. Srinivasan" <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Stephen Hemminger <sthemmin@microsoft.com>,
        "Michael Kelley \(EOSG\)" <Michael.H.Kelley@microsoft.com>,
        Mohammed Gamal <mmorsy@redhat.com>,
        Cathy Avery <cavery@redhat.com>, linux-kernel@vger.kernel.org,
        Jim Mattson <jmattson@google.com>,
        Liran Alon <liran.alon@oracle.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Return-path: <linux-kernel-owner@vger.kernel.org>
In-Reply-To: <46052d1e-9ee1-8cee-3f7c-cf27b1cd0373@redhat.com> (Paolo
        Bonzini's message of "Wed, 25 Jul 2018 14:14:49 +0200")
Sender: linux-kernel-owner@vger.kernel.org
List-Id: kvm.vger.kernel.org

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 25/07/2018 10:37, Vitaly Kuznetsov wrote:
>>> Why is this needed?  If it weren't for it, you could pass hv_evmcs
>>> directly to evmcs_needs_write, which would simplify the code a bit in
>>> the caller.
>> This is an equivalent of prepare_vmcs02()/prepare_vmcs02_full() split
>> for eVMCS case: when we switch from L2 guest A to L2 guest B we need to
>> write the whole VMCS so evmcs_needs_write() needs to return true.
>
> Right, I missed the dirty_vmcs12 assignment in patch 5.
>
> But is L0 allowed to write to hv_clean_fields?  

It is kinda expected to: currently I reset it in vmx_vcpu_run() and (if
I remember correctly) L1 Hyper-V only clears bits in this mask when it
touches certain fields so if we don't set it to 'all clean' it stays
zeroed forever. So nothing stops us from doing 

       if (hv_evmcs && vmx->nested.dirty_vmcs12)
               hv_evmcs->hv_clean_fields &=
                       ~HV_VMX_ENLIGHTENED_CLEAN_FIELD_ALL;

in prepare_vmcs02() I guess.

> One possibility is to
> add a dirty_evmcs field to struct nested_vmx, and "OR" ~hv_clean_fields
> into it at the beginning of prepare_vmcs02.
>
> Something like
>
> 	if (vmx->nested.hv_evmcs) {
> 		vmx->nested.dirty_evmcs |=
> 			~vmx->nested.hv_evmcs->hv_clean_fields;
> 		prepare_vmcs02_full(vcpu, vmcs12,
> 				    vmx->nested.dirty_evmcs);
> 	} else if (vmx->nested.dirty_vmcs12) {
> 		prepare_vmcs02_full(vcpu, vmcs12, ~0);
> 	}
>
> 	...
> 	vmx->nested.dirty_evmcs = 0;
> 	vmx->nested.dirty_vmcs12 = false;
>
> ?
>

I think we can even get away with a local variable in prepare_vmcs02()
and pass it to prepare_vmcs02_full(), no need to have it in struct
nested_vmx. But I would slightly prefer to just reset
hv_evmcs->hv_clean_fields when vmcs12 is dirty.

Thanks,

-- 
  Vitaly