On Fri, Apr 12, 2013 at 01:44:14PM +0300, Abel Gordon wrote: > > > 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 ? Yes. -- Gleb. -- 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
