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

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

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.


> 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



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