kvm-owner@xxxxxxxxxxxxxxx wrote on 12/04/2013 01:31:17 PM: > From: Gleb Natapov <gleb@xxxxxxxxxx> > To: Abel Gordon/Haifa/IBM@IBMIL, > Cc: dongxiao.xu@xxxxxxxxx, jun.nakajima@xxxxxxxxx, > kvm@xxxxxxxxxxxxxxx, "Nadav Har'El" <nyh@xxxxxxxxxxxxxxxxxxx>, > owasserm@xxxxxxxxxx > Date: 12/04/2013 01:31 PM > Subject: Re: [PATCH 10/11] KVM: nVMX: Synchronize VMCS12 content > with the shadow vmcs > Sent by: kvm-owner@xxxxxxxxxxxxxxx > > On Fri, Apr 12, 2013 at 01:26:32PM +0300, Abel Gordon wrote: > > > > > > Gleb Natapov <gleb@xxxxxxxxxx> wrote on 11/04/2013 09:54:11 AM: > > > > > On Wed, Apr 10, 2013 at 10:15:37PM +0300, Abel Gordon wrote: > > > > > > > > > > > > Gleb Natapov <gleb@xxxxxxxxxx> wrote on 09/04/2013 04:14:35 PM: > > > > > I think the patch already miss some fields. What if nested_vmx_run() > > > > > fails and calls nested_vmx_entry_failure(). nested_vmx_entry_failure > > () > > > > > sets vmcs12->vm_exit_reason and vmcs12->exit_qualification, but where > > do > > > > > we copy them back to shadow before going back to L1? > > > > > > > > Good catch! :) > > > > > > > > Note that the entry path is easy to handle because we copy the fields > > > > as part of nested_vmx_entry. This is not like exit path where > > > > KVM(L0) code can modify fields after nested_vmx_vmexit is called. > > > > > > > > So here, we could simple call copy_vmcs12_to_shadow if the entry fails > > > > (as part of nested_vmx_entry_failure or nested_vmx). We could optimize > > > > the code by updating these specific fields directly, but I don't think > > > > we really need to optimize code that is part of the error path. > > > > > > > We needn't. > > > > > > > > May be we need to introduce vmcs12 accessors to track what is changes > > > > > and if something need to be copied to shadow before going back to L1. > > > > > > > > That means we will need to modify all the lines of code that uses > > > > "vmcs12->" with an inline nested_vmcs_read or nested_vmcs_write > > function. > > > > Inside these inline functions we could access the shadow vmcs directly. > > > > However, to access the shadow vmcs we need to vmptrld first and this > > will > > > > force > > > > unnecessary vmptrlds (between shadow vmcs 12 and current vmcs 01) each > > time > > > > the code accesses a vmcs12 field. Alternatively, if we want to avoid > > > > unnecessary vmptrlds each time we access vmcs12 we could simple set a > > > > flag that indicates when a shadow field was changed. In this case, we > > will > > > > need to find all the places to check the flag and copy the fields, > > > > considering both success and error paths. > > > That's not how I see it. nested_vmcs_write() will set a request bit in > > > vcpu (this is the flag you mention above). The bit will be checked during > > > a guest entry and vmcs12 will be synced to shadow at this point. Later > > > we can track what fields were written and sync only them. > > > > XXX > > > > > Finally, I am afraid that these directions will introduce new issues, > > > > will force us to modify too many lines and they may create a > > merge/rebase > > > > mess... > > > Conflicts should be trivial and I do not expect many iterations for the > > > patch series. I like the approach you take to use vmcs shadowing and most > > > comment are nitpicks. I promise to review the next version as soon as > > > it is posted :) > > > > > > > > > > > Maybe we should simple fix nested_vmx_entry_failure (as well as the > > > > other fixes you suggested in other patches) and apply the code. > > > > Do you agree ? > > > > > > > The approach is error prone. Even if we will fix all bugs in the current > > > code each new nested vmx modification will have to be reviewed with > > shadowing > > > in mind and sometimes it is hard to see global picture just from a > > > patch. It is better to hide shadow details from occasional nested vmx > > hacker. > > > > But how ? I suggested 2 alternatives with pros/cons. > I outlined how above (search for XXX). Use accessors, set request bit, > sync during guest entry if request bit is set. Ok, so then you prefer to add the inline functions to read/write to the vmcs12 fields, (to set the request bit if shadowed field changed) and you are not concerned about any merge/rebase mess. I will work on this direction. I'll first send an independent patch to introduce the accessors. Once you apply this patch, I'll continue and send you v2 patches for shadow vmcs. Do you agree ? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
