[PATCH 0/3] some nested vmx fixing to hyper-v

[PATCH 0/3] some nested vmx fixing to hyper-v

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

This series patches fix some issues which i encountered when boot L1 hyper-v.

This patch fixed RHEL6 guest installation problem with L1 hyper-v:
	Nested VMX: update nested paging mode when vmswitch is in progress

The two fixing SMP hyper-v boot issue:
	VMX,apicv: Set "NMI-window exiting" for NMI
	Nested VMX: Setup the virtual NMI exiting info:

Yang Zhang (3):
  Nested VMX: update nested paging mode when vmswitch is in progress
  VMX,apicv: Set "NMI-window exiting" for NMI
  Nested VMX: Setup the virtual NMI exiting info

 xen/arch/x86/hvm/hvm.c      |    4 ++--
 xen/arch/x86/hvm/vmx/intr.c |    7 ++++---
 xen/arch/x86/hvm/vmx/vvmx.c |    6 ++++++
 3 files changed, 12 insertions(+), 5 deletions(-)

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

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

virtual vmentry will change paging related stucture, so corrensponding
nested mode need to be updated which is missing currently.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/hvm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

This patch fixed RHEL6 guest installation problem with L1 hyper-v.

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 69f7e74..1f62e00 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1925,7 +1925,7 @@ int hvm_set_cr0(unsigned long value)
     hvm_update_cr(v, 0, value);
 
     if ( (value ^ old_value) & X86_CR0_PG ) {
-        if ( !nestedhvm_vmswitch_in_progress(v) && nestedhvm_vcpu_in_guestmode(v) )
+        if ( nestedhvm_vcpu_in_guestmode(v) )
             paging_update_nestedmode(v);
         else
             paging_update_paging_modes(v);
@@ -2014,7 +2014,7 @@ int hvm_set_cr4(unsigned long value)
           (X86_CR4_PSE | X86_CR4_PGE | X86_CR4_PAE | X86_CR4_SMEP)) ||
          (!(value & X86_CR4_PCIDE) && (old_cr & X86_CR4_PCIDE)) )
     {
-        if ( !nestedhvm_vmswitch_in_progress(v) && nestedhvm_vcpu_in_guestmode(v) )
+        if ( nestedhvm_vcpu_in_guestmode(v) )
             paging_update_nestedmode(v);
         else
             paging_update_paging_modes(v);
-- 
1.7.1

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

[Egger, Christoph]

On 12.12.13 03:06, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> virtual vmentry will change paging related stucture, so corrensponding
> nested mode need to be updated which is missing currently.
> 
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>

I weakly remember the "!nestedhvm_vmswitch_in_progress" is needed
to avoid a nested pagefault loop on AMD. I do not remember the
actual reproduction case. Unfortunately, I do not have a setup
to verify.

Christoph

> ---
>  xen/arch/x86/hvm/hvm.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> This patch fixed RHEL6 guest installation problem with L1 hyper-v.
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 69f7e74..1f62e00 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1925,7 +1925,7 @@ int hvm_set_cr0(unsigned long value)
>      hvm_update_cr(v, 0, value);
>  
>      if ( (value ^ old_value) & X86_CR0_PG ) {
> -        if ( !nestedhvm_vmswitch_in_progress(v) && nestedhvm_vcpu_in_guestmode(v) )
> +        if ( nestedhvm_vcpu_in_guestmode(v) )
>              paging_update_nestedmode(v);
>          else
>              paging_update_paging_modes(v);
> @@ -2014,7 +2014,7 @@ int hvm_set_cr4(unsigned long value)
>            (X86_CR4_PSE | X86_CR4_PGE | X86_CR4_PAE | X86_CR4_SMEP)) ||
>           (!(value & X86_CR4_PCIDE) && (old_cr & X86_CR4_PCIDE)) )
>      {
> -        if ( !nestedhvm_vmswitch_in_progress(v) && nestedhvm_vcpu_in_guestmode(v) )
> +        if ( nestedhvm_vcpu_in_guestmode(v) )
>              paging_update_nestedmode(v);
>          else
>              paging_update_paging_modes(v);
> 

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

[Zhang, Yang Z]

Egger, Christoph wrote on 2013-12-12:
> On 12.12.13 03:06, Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> virtual vmentry will change paging related stucture, so corrensponding
>> nested mode need to be updated which is missing currently.
>> 
>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> 
> I weakly remember the "!nestedhvm_vmswitch_in_progress" is needed
> to avoid a nested pagefault loop on AMD. I do not remember the
> actual reproduction case. Unfortunately, I do not have a setup
> to verify.

So you mean if CR0.PG bit is changed during vmentry, there is no need to update nested paging mode on AMD? 
Consider that, if the nested VCPU is scheduled from ETP to shadow and you don't update the nested paging mode, then it will cause problem, and vice versa.

Best regards,
Yang

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

[Zhang, Yang Z]

Zhang, Yang Z wrote on 2013-12-13:
> Egger, Christoph wrote on 2013-12-12:
>> On 12.12.13 03:06, Yang Zhang wrote:
>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>> 
>>> virtual vmentry will change paging related stucture, so
>>> corrensponding nested mode need to be updated which is missing currently.
>>> 
>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> I weakly remember the "!nestedhvm_vmswitch_in_progress" is needed to
>> avoid a nested pagefault loop on AMD. I do not remember the actual
>> reproduction case. Unfortunately, I do not have a setup to verify.
> 
> So you mean if CR0.PG bit is changed during vmentry, there is no need
> to update nested paging mode on AMD?
> Consider that, if the nested VCPU is scheduled from ETP to shadow and
> you don't update the nested paging mode, then it will cause problem,
> and vice versa.

Hi, Christoph,

Any concern with this patch?

Best regards,
Yang

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

[Dong, Eddie]

Acked by Eddie Dong <eddie.dong@intel.com>

-----Original Message-----
From: Zhang, Yang Z 
Sent: Thursday, December 12, 2013 10:07 AM
To: xen-devel@lists.xen.org
Cc: JBeulich@suse.com; Dong, Eddie; Zhang, Yang Z
Subject: [PATCH 1/3] Nested VMX: update nested paging mode when vmswitch is in progress

From: Yang Zhang <yang.z.zhang@Intel.com>

virtual vmentry will change paging related stucture, so corrensponding
nested mode need to be updated which is missing currently.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/hvm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

This patch fixed RHEL6 guest installation problem with L1 hyper-v.

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 69f7e74..1f62e00 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1925,7 +1925,7 @@ int hvm_set_cr0(unsigned long value)
     hvm_update_cr(v, 0, value);
 
     if ( (value ^ old_value) & X86_CR0_PG ) {
-        if ( !nestedhvm_vmswitch_in_progress(v) && nestedhvm_vcpu_in_guestmode(v) )
+        if ( nestedhvm_vcpu_in_guestmode(v) )
             paging_update_nestedmode(v);
         else
             paging_update_paging_modes(v);
@@ -2014,7 +2014,7 @@ int hvm_set_cr4(unsigned long value)
           (X86_CR4_PSE | X86_CR4_PGE | X86_CR4_PAE | X86_CR4_SMEP)) ||
          (!(value & X86_CR4_PCIDE) && (old_cr & X86_CR4_PCIDE)) )
     {
-        if ( !nestedhvm_vmswitch_in_progress(v) && nestedhvm_vcpu_in_guestmode(v) )
+        if ( nestedhvm_vcpu_in_guestmode(v) )
             paging_update_nestedmode(v);
         else
             paging_update_paging_modes(v);
-- 
1.7.1

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

[Jan Beulich]

>>> On 18.12.13 at 09:58, "Dong, Eddie" <eddie.dong@intel.com> wrote:
> Acked by Eddie Dong <eddie.dong@intel.com>

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.

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).

Jan

> -----Original Message-----
> From: Zhang, Yang Z 
> Sent: Thursday, December 12, 2013 10:07 AM
> To: xen-devel@lists.xen.org 
> Cc: JBeulich@suse.com; Dong, Eddie; Zhang, Yang Z
> Subject: [PATCH 1/3] Nested VMX: update nested paging mode when vmswitch is 
> in progress
> 
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> virtual vmentry will change paging related stucture, so corrensponding
> nested mode need to be updated which is missing currently.
> 
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  xen/arch/x86/hvm/hvm.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> This patch fixed RHEL6 guest installation problem with L1 hyper-v.
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 69f7e74..1f62e00 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1925,7 +1925,7 @@ int hvm_set_cr0(unsigned long value)
>      hvm_update_cr(v, 0, value);
>  
>      if ( (value ^ old_value) & X86_CR0_PG ) {
> -        if ( !nestedhvm_vmswitch_in_progress(v) && 
> nestedhvm_vcpu_in_guestmode(v) )
> +        if ( nestedhvm_vcpu_in_guestmode(v) )
>              paging_update_nestedmode(v);
>          else
>              paging_update_paging_modes(v);
> @@ -2014,7 +2014,7 @@ int hvm_set_cr4(unsigned long value)
>            (X86_CR4_PSE | X86_CR4_PGE | X86_CR4_PAE | X86_CR4_SMEP)) ||
>           (!(value & X86_CR4_PCIDE) && (old_cr & X86_CR4_PCIDE)) )
>      {
> -        if ( !nestedhvm_vmswitch_in_progress(v) && 
> nestedhvm_vcpu_in_guestmode(v) )
> +        if ( nestedhvm_vcpu_in_guestmode(v) )
>              paging_update_nestedmode(v);
>          else
>              paging_update_paging_modes(v);
> -- 
> 1.7.1

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

[Zhang, Yang Z]

Jan Beulich wrote on 2013-12-18:
>>>> On 18.12.13 at 09:58, "Dong, Eddie" <eddie.dong@intel.com> wrote:
>> Acked by Eddie Dong <eddie.dong@intel.com>
> 
> 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.

> 
> Jan
> 
> Zhang, Yang Z wrote on 2013-12-12:
>> vmswitch is in progress
>> 
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> virtual vmentry will change paging related stucture, so
>> corrensponding nested mode need to be updated which is missing currently.
>> 
>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>> ---
>>  xen/arch/x86/hvm/hvm.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>> This patch fixed RHEL6 guest installation problem with L1 hyper-v.
>> 
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index
>> 69f7e74..1f62e00 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -1925,7 +1925,7 @@ int hvm_set_cr0(unsigned long value)
>>      hvm_update_cr(v, 0, value);
>>      
>>      if ( (value ^ old_value) & X86_CR0_PG ) {
>> -        if ( !nestedhvm_vmswitch_in_progress(v) &&
>> nestedhvm_vcpu_in_guestmode(v) )
>> +        if ( nestedhvm_vcpu_in_guestmode(v) )
>>              paging_update_nestedmode(v); else
>>              paging_update_paging_modes(v); @@ -2014,7 +2014,7
> @@ int
>> hvm_set_cr4(unsigned long value)
>>            (X86_CR4_PSE | X86_CR4_PGE | X86_CR4_PAE |
> X86_CR4_SMEP)) ||
>>           (!(value & X86_CR4_PCIDE) && (old_cr & X86_CR4_PCIDE)) )
>>      {
>> -        if ( !nestedhvm_vmswitch_in_progress(v) &&
>> nestedhvm_vcpu_in_guestmode(v) )
>> +        if ( nestedhvm_vcpu_in_guestmode(v) )
>>              paging_update_nestedmode(v); else
>>              paging_update_paging_modes(v);
>> --
>> 1.7.1
> 
>


Best regards,
Yang

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

[Egger, Christoph]

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@intel.com> wrote:
>>> Acked by Eddie Dong <eddie.dong@intel.com>
>>
>> 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).

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.

This is just from reading the code. As I said, I do not have
a setup to verify this, unfortunately.

Christoph


>>
>> Jan
>>
>> Zhang, Yang Z wrote on 2013-12-12:
>>> vmswitch is in progress
>>>
>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>
>>> virtual vmentry will change paging related stucture, so
>>> corrensponding nested mode need to be updated which is missing currently.
>>>
>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>>> ---
>>>  xen/arch/x86/hvm/hvm.c |    4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>> This patch fixed RHEL6 guest installation problem with L1 hyper-v.
>>>
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index
>>> 69f7e74..1f62e00 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -1925,7 +1925,7 @@ int hvm_set_cr0(unsigned long value)
>>>      hvm_update_cr(v, 0, value);
>>>      
>>>      if ( (value ^ old_value) & X86_CR0_PG ) {
>>> -        if ( !nestedhvm_vmswitch_in_progress(v) &&
>>> nestedhvm_vcpu_in_guestmode(v) )
>>> +        if ( nestedhvm_vcpu_in_guestmode(v) )
>>>              paging_update_nestedmode(v); else
>>>              paging_update_paging_modes(v); @@ -2014,7 +2014,7
>> @@ int
>>> hvm_set_cr4(unsigned long value)
>>>            (X86_CR4_PSE | X86_CR4_PGE | X86_CR4_PAE |
>> X86_CR4_SMEP)) ||
>>>           (!(value & X86_CR4_PCIDE) && (old_cr & X86_CR4_PCIDE)) )
>>>      {
>>> -        if ( !nestedhvm_vmswitch_in_progress(v) &&
>>> nestedhvm_vcpu_in_guestmode(v) )
>>> +        if ( nestedhvm_vcpu_in_guestmode(v) )
>>>              paging_update_nestedmode(v); else
>>>              paging_update_paging_modes(v);
>>> --
>>> 1.7.1
>>
>>
> 
> 
> Best regards,
> Yang
> 
> 

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

[Zhang, Yang Z]

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@intel.com> wrote:
>>>> Acked by Eddie Dong <eddie.dong@intel.com>
>>> 
>>> 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

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

[Zhang, Yang Z]

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@intel.com> wrote:
>>>>> Acked by Eddie Dong <eddie.dong@intel.com>
>>>> 
>>>> 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

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

[Zhang, Yang Z]

Zhang, Yang Z wrote on 2013-12-24:
> 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@intel.com> wrote:
>>>>>> Acked by Eddie Dong <eddie.dong@intel.com>
>>>>> 
>>>>> 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
>

Any comments ?

Best regards,
Yang

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

[Jan Beulich]

>>> On 14.01.14 at 03:33, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
> Zhang, Yang Z wrote on 2013-12-24:
>> 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@intel.com> wrote:
>>>>>>> Acked by Eddie Dong <eddie.dong@intel.com>
>>>>>> 
>>>>>> 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.
> 
> Any comments ?

Considering Christoph's comments and reservations, if you can't
alone deal with this I think you should work with the AMD people
to eliminate or address his concerns.

Jan

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

[Zhang, Yang Z]

Jan Beulich wrote on 2014-01-14:
>>>> On 14.01.14 at 03:33, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>> Zhang, Yang Z wrote on 2013-12-24:
>> 
>> Any comments ?
> 
> Considering Christoph's comments and reservations, if you can't alone
> deal with this I think you should work with the AMD people to
> eliminate or address his concerns.
> 

Yes. But the problem puzzled me is that Christoph said nested SVM works well w/o my patch which I cannot understand. Because according my analysis in previous thread, it also buggy in AMD side. But if they really solved the issue in their side, I wonder how they fix it. Perhaps I can use the same solution in VMX side without touch the common code.

Christoph, can you help to check it? thanks.

> Jan


Best regards,
Yang

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

[Egger, Christoph]

On 14.01.14 08:38, Zhang, Yang Z wrote:
> Jan Beulich wrote on 2014-01-14:
>>>>> On 14.01.14 at 03:33, "Zhang, Yang Z" <yang.z.zhang@intel.com> wrote:
>>> Zhang, Yang Z wrote on 2013-12-24:
>>>
>>> Any comments ?
>>
>> Considering Christoph's comments and reservations, if you can't alone
>> deal with this I think you should work with the AMD people to
>> eliminate or address his concerns.
>>
> 
> Yes. But the problem puzzled me is that Christoph said nested SVM works
> well w/o my patch which I cannot understand. Because according my analysis
> in previous thread, it also buggy in AMD side. But if they really solved
> the issue in their side, I wonder how they fix it. Perhaps I can use the
> same solution in VMX side without touch the common code.
> 
> Christoph, can you help to check it? thanks.

The fix I mentioned solves the vmswitch problem on AMD side.
The page mode problem you discovered is a seperate issue for
both SVM and VMX that needs to be addressed.

Christoph

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

[Zhang, Yang Z]

Egger, Christoph wrote on 2014-01-20:
> On 14.01.14 08:38, Zhang, Yang Z wrote:
>> Jan Beulich wrote on 2014-01-14:
>>>>>> On 14.01.14 at 03:33, "Zhang, Yang Z" <yang.z.zhang@intel.com>
> wrote:
>>>> Zhang, Yang Z wrote on 2013-12-24:
>>>> 
>>>> Any comments ?
>>> 
>>> Considering Christoph's comments and reservations, if you can't
>>> alone deal with this I think you should work with the AMD people to
>>> eliminate or address his concerns.
>>> 
>> 
>> Yes. But the problem puzzled me is that Christoph said nested SVM works
>> well w/o my patch which I cannot understand. Because according my
>> analysis in previous thread, it also buggy in AMD side. But if they
>> really solved the issue in their side, I wonder how they fix it.
>> Perhaps I can use the same solution in VMX side without touch the
>> common code.
>> 
>> Christoph, can you help to check it? thanks.
> 
> The fix I mentioned solves the vmswitch problem on AMD side.

But the current code is buggy with your fixing even in AMD side, see below scenario:
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.

I cannot understand why you said it is working.

> The page mode problem you discovered is a seperate issue for both SVM
> and VMX that needs to be addressed.
> 
> Christoph


Best regards,
Yang

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

[Egger, Christoph]

On 21.01.14 09:49, Zhang, Yang Z wrote:
> Egger, Christoph wrote on 2014-01-20:
>> On 14.01.14 08:38, Zhang, Yang Z wrote:
>>> Jan Beulich wrote on 2014-01-14:
>>>>>>> On 14.01.14 at 03:33, "Zhang, Yang Z" <yang.z.zhang@intel.com>
>> wrote:
>>>>> Zhang, Yang Z wrote on 2013-12-24:
>>>>>
>>>>> Any comments ?
>>>>
>>>> Considering Christoph's comments and reservations, if you can't
>>>> alone deal with this I think you should work with the AMD people to
>>>> eliminate or address his concerns.
>>>>
>>>
>>> Yes. But the problem puzzled me is that Christoph said nested SVM works
>>> well w/o my patch which I cannot understand. Because according my
>>> analysis in previous thread, it also buggy in AMD side. But if they
>>> really solved the issue in their side, I wonder how they fix it.
>>> Perhaps I can use the same solution in VMX side without touch the
>>> common code.
>>>
>>> Christoph, can you help to check it? thanks.
>>
>> The fix I mentioned solves the vmswitch problem on AMD side.
> 
> But the current code is buggy with your fixing even in AMD side, see below scenario:
> 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.
> 
> I cannot understand why you said it is working.

This page mode problem you discovered that needs to be addressed for
both SVM and VMX.

The fix I am saying is working solves the problem of emulating an MMIO
instruction which is finished in the softirq while an interrupt
can cause a vmswitch during instruction emulation.

We are talking about two different issues. One is fixed on AMD side and
one not.

Christoph

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

[Dong, Eddie]

Hi Jan:
	Usually I ack one for a series of patch. This one is probably different: It appears as a series of patch, but it seems they look like quit independent. But you are correct, the 1st one is not VMX only stuff. The 3rd one appears to be harmless to me, that you may take seperately.
Thx Eddie

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@suse.com] 
Sent: Wednesday, December 18, 2013 6:08 PM
To: Dong, Eddie; Zhang, Yang Z
Cc: Christoph Egger; xen-devel
Subject: RE: [PATCH 1/3] Nested VMX: update nested paging mode when vmswitch is in progress

>>> On 18.12.13 at 09:58, "Dong, Eddie" <eddie.dong@intel.com> wrote:
> Acked by Eddie Dong <eddie.dong@intel.com>

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.

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).

Jan

[PATCH 2/3] VMX, apicv: Set "NMI-window exiting" for NMI

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

Enable NMI-window exiting if interrupt is blocked by NMI under apicv enabled
platform.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/vmx/intr.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 7757910..8507432 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -252,10 +252,11 @@ void vmx_intr_assist(void)
         intblk = hvm_interrupt_blocked(v, intack);
         if ( cpu_has_vmx_virtual_intr_delivery )
         {
-            /* Set "Interrupt-window exiting" for ExtINT */
+            /* Set "Interrupt-window exiting" for ExtINT and NMI. */
             if ( (intblk != hvm_intblk_none) &&
-                 ( (intack.source == hvm_intsrc_pic) ||
-                 ( intack.source == hvm_intsrc_vector) ) )
+                 (intack.source == hvm_intsrc_pic ||
+                  intack.source == hvm_intsrc_vector ||
+                  intack.source == hvm_intsrc_nmi) )
             {
                 enable_intr_window(v, intack);
                 goto out;
-- 
1.7.1

Re: [PATCH 2/3] VMX, apicv: Set "NMI-window exiting" for NMI

[Zhang, Yang Z]

Zhang, Yang Z wrote on 2013-12-12:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> Enable NMI-window exiting if interrupt is blocked by NMI under apicv
> enabled platform.
> 

Hi Jun,

Can you help to review this patch? Thanks.

> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  xen/arch/x86/hvm/vmx/intr.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> index 7757910..8507432 100644 --- a/xen/arch/x86/hvm/vmx/intr.c +++
> b/xen/arch/x86/hvm/vmx/intr.c @@ -252,10 +252,11 @@ void
> vmx_intr_assist(void)
>          intblk = hvm_interrupt_blocked(v, intack);
>          if ( cpu_has_vmx_virtual_intr_delivery )
>          {
> -            /* Set "Interrupt-window exiting" for ExtINT */
> +            /* Set "Interrupt-window exiting" for ExtINT and NMI. */
>              if ( (intblk != hvm_intblk_none) &&
> -                 ( (intack.source == hvm_intsrc_pic) ||
> -                 ( intack.source == hvm_intsrc_vector) ) )
> +                 (intack.source == hvm_intsrc_pic ||
> +                  intack.source == hvm_intsrc_vector ||
> +                  intack.source == hvm_intsrc_nmi) )
>              {
>                  enable_intr_window(v, intack);
>                  goto out;


Best regards,
Yang

Re: [PATCH 2/3] VMX, apicv: Set "NMI-window exiting" for NMI

[Andrew Cooper]

On 08/01/2014 01:23, Zhang, Yang Z wrote:
> Zhang, Yang Z wrote on 2013-12-12:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>
>> Enable NMI-window exiting if interrupt is blocked by NMI under apicv
>> enabled platform.
>>
> Hi Jun,
>
> Can you help to review this patch? Thanks.

Also committed earlier today
(http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=185e83591ce420e0b004646b55c5e4783e388531)

~Andrew

Re: [PATCH 2/3] VMX, apicv: Set "NMI-window exiting" for NMI

[Zhang, Yang Z]

Andrew Cooper wrote on 2014-01-08:
> On 08/01/2014 01:23, Zhang, Yang Z wrote:
>> Zhang, Yang Z wrote on 2013-12-12:
>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>> 
>>> Enable NMI-window exiting if interrupt is blocked by NMI under
>>> apicv enabled platform.
>>> 
>> Hi Jun,
>> 
>> Can you help to review this patch? Thanks.
> 
> Also committed earlier today
> (http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=185e83591ce42
> 0e0 b004646b55c5e4783e388531)
> 
> ~Andrew

I see it. Thanks for your remainder.

Best regards,
Yang

Re: [PATCH 2/3] VMX, apicv: Set "NMI-window exiting" for NMI

[Zhang, Yang Z]

Zhang, Yang Z wrote on 2014-01-08:
> Zhang, Yang Z wrote on 2013-12-12:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> Enable NMI-window exiting if interrupt is blocked by NMI under apicv
>> enabled platform.
>> 
> 
> Hi Jun,
> 
> Can you help to review this patch? Thanks.

Sorry. It already been committed earlier today. 

> 
>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>> ---
>>  xen/arch/x86/hvm/vmx/intr.c |    7 ++++---
>>  1 files changed, 4 insertions(+), 3 deletions(-) diff --git
>> a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c index
>> 7757910..8507432 100644 --- a/xen/arch/x86/hvm/vmx/intr.c +++
>> b/xen/arch/x86/hvm/vmx/intr.c @@ -252,10 +252,11 @@ void
>> vmx_intr_assist(void)
>>          intblk = hvm_interrupt_blocked(v, intack);
>>          if ( cpu_has_vmx_virtual_intr_delivery )
>>          {
>> -            /* Set "Interrupt-window exiting" for ExtINT */
>> +            /* Set "Interrupt-window exiting" for ExtINT and NMI.
>> + */
>>              if ( (intblk != hvm_intblk_none) &&
>> -                 ( (intack.source == hvm_intsrc_pic) ||
>> -                 ( intack.source == hvm_intsrc_vector) ) )
>> +                 (intack.source == hvm_intsrc_pic ||
>> +                  intack.source == hvm_intsrc_vector ||
>> +                  intack.source == hvm_intsrc_nmi) )
>>              {
>>                  enable_intr_window(v, intack);
>>                  goto out;
> 
> 
> Best regards,
> Yang
>


Best regards,
Yang

[PATCH 3/3] Nested VMX: Setup the virtual NMI exiting info

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

When inject a virtual nmi exit to L1, hypervisor need to set the
virtual vmcs with right vaule which is missing in current Xen.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 0daad79..41db52b 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1293,6 +1293,12 @@ static void sync_exception_state(struct vcpu *v)
                     nvmx->intr.error_code);
         break;
     case X86_EVENTTYPE_NMI:
+        __set_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_REASON,
+                    EXIT_REASON_EXCEPTION_NMI);
+        __set_vvmcs(nvcpu->nv_vvmcx, EXIT_QUALIFICATION, 0);
+        __set_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_INTR_INFO,
+                    nvmx->intr.intr_info);
+        break;
     default:
         gdprintk(XENLOG_ERR, "Exception state %lx not handled\n",
                nvmx->intr.intr_info); 
-- 
1.7.1

Re: [PATCH 3/3] Nested VMX: Setup the virtual NMI exiting info

[Dong, Eddie]

Acked by Eddie Dong <eddie.dong@intel.com>

-----Original Message-----
From: Zhang, Yang Z 
Sent: Thursday, December 12, 2013 10:07 AM
To: xen-devel@lists.xen.org
Cc: JBeulich@suse.com; Dong, Eddie; Zhang, Yang Z
Subject: [PATCH 3/3] Nested VMX: Setup the virtual NMI exiting info

From: Yang Zhang <yang.z.zhang@Intel.com>

When inject a virtual nmi exit to L1, hypervisor need to set the
virtual vmcs with right vaule which is missing in current Xen.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 0daad79..41db52b 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1293,6 +1293,12 @@ static void sync_exception_state(struct vcpu *v)
                     nvmx->intr.error_code);
         break;
     case X86_EVENTTYPE_NMI:
+        __set_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_REASON,
+                    EXIT_REASON_EXCEPTION_NMI);
+        __set_vvmcs(nvcpu->nv_vvmcx, EXIT_QUALIFICATION, 0);
+        __set_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_INTR_INFO,
+                    nvmx->intr.intr_info);
+        break;
     default:
         gdprintk(XENLOG_ERR, "Exception state %lx not handled\n",
                nvmx->intr.intr_info); 
-- 
1.7.1

Re: [PATCH 0/3] some nested vmx fixing to hyper-v

[Zhang, Yang Z]

[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]

Hi, eddie,

Can you help to review the patches?

Best regards,
Yang


> -----Original Message-----
> From: Zhang, Yang Z
> Sent: Thursday, December 12, 2013 10:07 AM
> To: xen-devel@lists.xen.org
> Cc: JBeulich@suse.com; Dong, Eddie; Zhang, Yang Z
> Subject: [PATCH 0/3] some nested vmx fixing to hyper-v
> 
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> This series patches fix some issues which i encountered when boot L1 hyper-v.
> 
> This patch fixed RHEL6 guest installation problem with L1 hyper-v:
> 	Nested VMX: update nested paging mode when vmswitch is in progress
> 
> The two fixing SMP hyper-v boot issue:
> 	VMX,apicv: Set "NMI-window exiting" for NMI
> 	Nested VMX: Setup the virtual NMI exiting info:
> 
> Yang Zhang (3):
>   Nested VMX: update nested paging mode when vmswitch is in progress
>   VMX,apicv: Set "NMI-window exiting" for NMI
>   Nested VMX: Setup the virtual NMI exiting info
> 
>  xen/arch/x86/hvm/hvm.c      |    4 ++--
>  xen/arch/x86/hvm/vmx/intr.c |    7 ++++---
>  xen/arch/x86/hvm/vmx/vvmx.c |    6 ++++++
>  3 files changed, 12 insertions(+), 5 deletions(-)


[-- Attachment #2: Type: message/rfc822, Size: 2926 bytes --]

From: "Zhang, Yang Z" <yang.z.zhang@intel.com>
To: "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Cc: "JBeulich@suse.com" <JBeulich@suse.com>, "Dong, Eddie" <eddie.dong@intel.com>, "Zhang, Yang Z" <yang.z.zhang@intel.com>
Subject: [PATCH 1/3] Nested VMX: update nested paging mode when vmswitch is in progress
Date: Thu, 12 Dec 2013 02:06:42 +0000
Message-ID: <1386814004-5574-2-git-send-email-yang.z.zhang@intel.com>

From: Yang Zhang <yang.z.zhang@Intel.com>

virtual vmentry will change paging related stucture, so corrensponding
nested mode need to be updated which is missing currently.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/hvm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

This patch fixed RHEL6 guest installation problem with L1 hyper-v.

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 69f7e74..1f62e00 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1925,7 +1925,7 @@ int hvm_set_cr0(unsigned long value)
     hvm_update_cr(v, 0, value);

     if ( (value ^ old_value) & X86_CR0_PG ) {
-        if ( !nestedhvm_vmswitch_in_progress(v) && nestedhvm_vcpu_in_guestmode(v) )
+        if ( nestedhvm_vcpu_in_guestmode(v) )
             paging_update_nestedmode(v);
         else
             paging_update_paging_modes(v);
@@ -2014,7 +2014,7 @@ int hvm_set_cr4(unsigned long value)
           (X86_CR4_PSE | X86_CR4_PGE | X86_CR4_PAE | X86_CR4_SMEP)) ||
          (!(value & X86_CR4_PCIDE) && (old_cr & X86_CR4_PCIDE)) )
     {
-        if ( !nestedhvm_vmswitch_in_progress(v) && nestedhvm_vcpu_in_guestmode(v) )
+        if ( nestedhvm_vcpu_in_guestmode(v) )
             paging_update_nestedmode(v);
         else
             paging_update_paging_modes(v);
--
1.7.1


[-- Attachment #3: Type: message/rfc822, Size: 2723 bytes --]

From: "Zhang, Yang Z" <yang.z.zhang@intel.com>
To: "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Cc: "JBeulich@suse.com" <JBeulich@suse.com>, "Dong, Eddie" <eddie.dong@intel.com>, "Zhang, Yang Z" <yang.z.zhang@intel.com>
Subject: [PATCH 2/3] VMX,apicv: Set "NMI-window exiting" for NMI
Date: Thu, 12 Dec 2013 02:06:43 +0000
Message-ID: <1386814004-5574-3-git-send-email-yang.z.zhang@intel.com>

From: Yang Zhang <yang.z.zhang@Intel.com>

Enable NMI-window exiting if interrupt is blocked by NMI under apicv enabled
platform.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/vmx/intr.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 7757910..8507432 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -252,10 +252,11 @@ void vmx_intr_assist(void)
         intblk = hvm_interrupt_blocked(v, intack);
         if ( cpu_has_vmx_virtual_intr_delivery )
         {
-            /* Set "Interrupt-window exiting" for ExtINT */
+            /* Set "Interrupt-window exiting" for ExtINT and NMI. */
             if ( (intblk != hvm_intblk_none) &&
-                 ( (intack.source == hvm_intsrc_pic) ||
-                 ( intack.source == hvm_intsrc_vector) ) )
+                 (intack.source == hvm_intsrc_pic ||
+                  intack.source == hvm_intsrc_vector ||
+                  intack.source == hvm_intsrc_nmi) )
             {
                 enable_intr_window(v, intack);
                 goto out;
--
1.7.1


[-- Attachment #4: Type: message/rfc822, Size: 2592 bytes --]

From: "Zhang, Yang Z" <yang.z.zhang@intel.com>
To: "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Cc: "JBeulich@suse.com" <JBeulich@suse.com>, "Dong, Eddie" <eddie.dong@intel.com>, "Zhang, Yang Z" <yang.z.zhang@intel.com>
Subject: [PATCH 3/3] Nested VMX: Setup the virtual NMI exiting info
Date: Thu, 12 Dec 2013 02:06:44 +0000
Message-ID: <1386814004-5574-4-git-send-email-yang.z.zhang@intel.com>

From: Yang Zhang <yang.z.zhang@Intel.com>

When inject a virtual nmi exit to L1, hypervisor need to set the
virtual vmcs with right vaule which is missing in current Xen.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 0daad79..41db52b 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1293,6 +1293,12 @@ static void sync_exception_state(struct vcpu *v)
                     nvmx->intr.error_code);
         break;
     case X86_EVENTTYPE_NMI:
+        __set_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_REASON,
+                    EXIT_REASON_EXCEPTION_NMI);
+        __set_vvmcs(nvcpu->nv_vvmcx, EXIT_QUALIFICATION, 0);
+        __set_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_INTR_INFO,
+                    nvmx->intr.intr_info);
+        break;
     default:
         gdprintk(XENLOG_ERR, "Exception state %lx not handled\n",
                nvmx->intr.intr_info);
--
1.7.1


[-- Attachment #5: Type: text/plain, Size: 126 bytes --]

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