Re: [Xen-devel] [PATCH 1/3] Nested VMX: update nested paging mode when vmswitch is in progress

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Zhang, Yang Z wrote on 2013-12-23:
> Egger, Christoph wrote on 2013-12-18:
>> On 18.12.13 11:24, Zhang, Yang Z wrote:
>>> Jan Beulich wrote on 2013-12-18:
>>>>>>> On 18.12.13 at 09:58, "Dong, Eddie" <eddie.dong@xxxxxxxxx> wrote:
>>>>> Acked by Eddie Dong <eddie.dong@xxxxxxxxx>
>>>> 
>>>> As long as Christoph's reservations wrt SVM aren't being
>>>> addressed/ eliminated, I don't think we can apply this patch.
>>>> 
>>>> Furthermore, while you ack-ed this patch (which isn't really VMX
>>>> specific) and patch 3, you didn't ack patch 2, but you also didn't
>>>> indicate anything that's possibly wrong with it.
>>> 
>>> Actually, I asked him help to review the first patch. Since
>>> Christoph thought
>> the first patch may break AMD. So I hope he can help to review the
>> first patch to see whether I am wrong.
>>> 
>>>> 
>>>> And finally, with patch 1 needing to be left out for the moment,
>>>> I'd like to have confirmation that all three patches can be
>>>> applied independently (i.e. with the current state of things only
>>>> patch 3 is ready to
>> go in).
>>> 
>>> Yes, the three patches are independent.
>> 
>> I have looked through code.
>> 
>> vcpu is in guestmode till the vmentry/vmexit emulation is done.
>> In SVM the vcpu guestmode changes right before setting
>> nv_vmswitch_in_progress to 0 when the vmentry/vmexit emulation was
>> successfull (there is a bunch of error-checking).
>> 
> 
> After checking the SVM logic, I find the basic usage of
> vcpu_in/exitk_guestmode of VMX side is different from that of SVM side:
> VMX side:
>     virtual vmentry: set vcpu in guestmode after vmcs switched to vmcs02.
> This happed at the beginning of vmentry which means the whole vmentry
> emulation code is executed when vcpu is in guest mode.
>     virtual vmexit: set vcpu exit guestmode after vmcs switched to vmcs01.
> Also, this action occurred just after sync vmcs02 to vmcs12 and before
> the vmcs01's state restored (like set_cr), which means the vmcs01's
> state restored when vcpu is not in guest mode.
> SVM side:
>     virtual vmentry: set vcpu in guest mode after vmentry emulation is
>     done, which means the emulation code is executed when vcpu is not in
>     guest mode. virtual vmexit: set vcpu exit guest mode after vmexit
>     emulation is
> done, which means the emulation code is executed when vcpu is in guest mode.
> 
> Ok, now let us take a look at current implementation, take hvm_set_cr0
> for example: Update nested mode when (vcpu_in_guestmode &&
> !vmswitch_in_progress). Otherwise, update L1's paging mode:
>         if ( !nestedhvm_vmswitch_in_progress(v) &&
> nestedhvm_vcpu_in_guestmode(v) )
>             paging_update_nestedmode(v); else
>             paging_update_paging_modes(v);
> virtual vmentry:
>     Expected result: nested mode is being updated.
>     Current result in SVM:
>           !vcpu_in_guestmode and vmswitch_in_progress:  L1's paging
> mode is updated.  Wrong.
>     Current result in VMX:
>           vcpu_in_guestmode and vmswitch_in_progress :  L1's paging
> mode is updated.  Wrong.
> 
> Virtual vmexit:
>       Expected result: L1's paging mode is updated.
>       Current result in SVM:
>           vcpu_in_guestmode and vmswitch_in_progress:  L1's paging
> mode is updated.   Correct.
>     Current result in VMX:
>           !vcpu_in_guestmode and vmswitch_in_progress:  L1's paging
> mod is updated.    Correct
> 
> From the above result, we can see the vmswitch_in_progress is actually
> take effect not vcpu_guest_mode. The original code doesn't consider
> the paging mode changed during vmentry/vmexit emulation. This seems
> wrong to me, because paging mode changing happens in real world.
> 
> Here is the result with my patch: Update nested mode when
> vcpu_in_guestmode. Otherwise, update L1's paging mode:
>         if (nestedhvm_vcpu_in_guestmode(v) )
>             paging_update_nestedmode(v); else
>             paging_update_paging_modes(v);
> virtual vmentry:
>     Expected result: nested mode is being updated.
>     Current result in SVM:
>           !vcpu_in_guestmode:  L1's paging mode is updated.    Wrong.
>           Current result in VMX: vcpu_in_guestmode :  Nested paging mode
>           is updated.  Correct.
> Virtual vmexit:
>       Expected result: L1's paging mode is updated.
>       Current result in SVM:
>           vcpu_in_guestmode:  Nested paging mode is updated.   Wrong.
>           Current result in VMX: !vcpu_in_guestmode:  L1's paging mod is
>           updated.      Correct
> From the above, we can see the problem is that the way to distinguish
> the vmentry and vmexit in SVM and VMX side is different. Since I am
> not familiar the SVM, so i am not sure whether the usage of
> vcpu_in_guestmode in SVM is right or wrong. But in VMX side, once the
> vmcs is changed, then the vcpu_in_guestmode is changed too which we
> think is following hardware's behavior.
> 
> Also, I think I found another issue that the paging mode cannot be
> tracked correctly with current way or with my patch. Need more time to 
> investigate.

Ok. The issue is that if the paging state is changed via vmwrite (L1 writes 
L2's vmcs to change paging mode directly) which is unaware to L0. And 
hypervisor cannot set the right nested paging mode during virtual vmentry. So 
we need to update nest mode unconditionally for each virtual vmentry.

> 
> 
>> This patch breaks both vmentry and vmexit emulation for SVM.
>> The vmentry breakage comes with l1-hypervisor using shadow-paging.
>> 
>> During vmexit emulation hvm_set_cr0 and hvm_set_cr4 are called to
>> restore cr0 and cr4 for the l1 guest. With this patch the paging
>> mode for the l2 guest is updated rather for the l1 guest.
>> 
>> I think this patch also breaks the case where l2 guest wants to set
>> cr0 or cr4 and l1-hypervisor does not intercept cr0/cr4 and
>> l1-hypervisor uses shadow-paging. This may also count for VMX.
> 
> For this case, am I missing something? If yes, please correct me.
> 
>> 
>> This is just from reading the code. As I said, I do not have a setup
>> to verify this, unfortunately.
>> 
> 
> 
> Best regards,
> Yang
>


Best regards,
Yang



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel