[PATCH 0/3] x86/vmx: Minor improvements to vmx_update_guest_cr()

[PATCH 0/3] x86/vmx: Minor improvements to vmx_update_guest_cr()

[Andrew Cooper]

This series is mostly cleanup, base on code observations when working on my
comprehensive XTF pagetable test, running in unpaged modes.  I've double
checked the behaviour HAP and Shadow modes, with and without the unrestricted
guest feature, on Haswell and Skylake-S hardware.

Andrew Cooper (3):
  x86/vmx: Misc cleanup to vmx_update_guest_cr()
  x86/vmx: Don't self-recurse in vmx_update_guest_cr()
  x86/vmx: Better description of CR4 settings outside of paged mode

 xen/arch/x86/hvm/vmx/vmx.c | 94 ++++++++++++++++++++++++++++------------------
 1 file changed, 57 insertions(+), 37 deletions(-)

-- 
2.1.4


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

[PATCH 1/3] x86/vmx: Misc cleanup to vmx_update_guest_cr()

[Andrew Cooper]

 * Drop trailing whitespace
 * Fix indendation and newlines
 * Use bool where appropriate

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 9cfa9b6..61047e0 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1547,15 +1547,16 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
 
     switch ( cr )
     {
-    case 0: {
-        int realmode;
+    case 0:
+    {
+        bool realmode;
         unsigned long hw_cr0_mask = X86_CR0_NE;
 
         if ( !vmx_unrestricted_guest(v) )
             hw_cr0_mask |= X86_CR0_PG | X86_CR0_PE;
 
         if ( paging_mode_shadow(v->domain) )
-           hw_cr0_mask |= X86_CR0_WP;
+            hw_cr0_mask |= X86_CR0_WP;
 
         if ( paging_mode_hap(v->domain) )
         {
@@ -1590,12 +1591,12 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
                 vmx_fpu_enter(v);
         }
 
-        realmode = !(v->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE); 
+        realmode = !(v->arch.hvm_vcpu.guest_cr[0] & X86_CR0_PE);
 
-        if ( (!vmx_unrestricted_guest(v)) &&
+        if ( !vmx_unrestricted_guest(v) &&
              (realmode != v->arch.hvm_vmx.vmx_realmode) )
         {
-            enum x86_segment s; 
+            enum x86_segment s;
             struct segment_register reg[x86_seg_tr + 1];
 
             BUILD_BUG_ON(x86_seg_tr != x86_seg_gs + 1);
@@ -1606,13 +1607,13 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
             for ( s = 0; s < ARRAY_SIZE(reg); s++ )
                 hvm_get_segment_register(v, s, &reg[s]);
             v->arch.hvm_vmx.vmx_realmode = realmode;
-            
+
             if ( realmode )
             {
                 for ( s = 0; s < ARRAY_SIZE(reg); s++ )
                     hvm_set_segment_register(v, s, &reg[s]);
             }
-            else 
+            else
             {
                 for ( s = 0; s < ARRAY_SIZE(reg); s++ )
                     if ( !(v->arch.hvm_vmx.vm86_segment_mask & (1<<s)) )
@@ -1631,9 +1632,11 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
         vmx_update_guest_cr(v, 4);
         break;
     }
+
     case 2:
         /* CR2 is updated in exit stub. */
         break;
+
     case 3:
         if ( paging_mode_hap(v->domain) )
         {
@@ -1642,10 +1645,11 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
                     v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT];
             vmx_load_pdptrs(v);
         }
- 
+
         __vmwrite(GUEST_CR3, v->arch.hvm_vcpu.hw_cr[3]);
         hvm_asid_flush_vcpu(v);
         break;
+
     case 4:
         v->arch.hvm_vcpu.hw_cr[4] = HVM_CR4_HOST_MASK;
         if ( paging_mode_hap(v->domain) )
@@ -1657,7 +1661,7 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
             nvmx_set_cr_read_shadow(v, 4);
 
         v->arch.hvm_vcpu.hw_cr[4] |= v->arch.hvm_vcpu.guest_cr[4];
-        if ( v->arch.hvm_vmx.vmx_realmode ) 
+        if ( v->arch.hvm_vmx.vmx_realmode )
             v->arch.hvm_vcpu.hw_cr[4] |= X86_CR4_VME;
         if ( paging_mode_hap(v->domain) && !hvm_paging_enabled(v) )
         {
@@ -1676,6 +1680,7 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
         }
         __vmwrite(GUEST_CR4, v->arch.hvm_vcpu.hw_cr[4]);
         break;
+
     default:
         BUG();
     }
-- 
2.1.4


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

Re: [PATCH 1/3] x86/vmx: Misc cleanup to vmx_update_guest_cr()

[Roger Pau Monné]

On Fri, Sep 29, 2017 at 06:31:01PM +0000, Andrew Cooper wrote:
>  * Drop trailing whitespace
>  * Fix indendation and newlines
>  * Use bool where appropriate
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

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

Re: [PATCH 1/3] x86/vmx: Misc cleanup to vmx_update_guest_cr()

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Saturday, September 30, 2017 2:31 AM
> 
>  * Drop trailing whitespace
>  * Fix indendation and newlines
>  * Use bool where appropriate
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

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

[PATCH 2/3] x86/vmx: Don't self-recurse in vmx_update_guest_cr()

[Andrew Cooper]

An update to CR4 following a CR0 update can be done easily by falling
through into the CR4 case.  This avoids unnecessary passes through
vmx_vmcs_{enter,exit}() and unnecessary stack usage (as the compiler
cannot optimise this use to a tailcall).

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 61047e0..5b943d4 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1627,29 +1627,8 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
         v->arch.hvm_vcpu.hw_cr[0] =
             v->arch.hvm_vcpu.guest_cr[0] | hw_cr0_mask;
         __vmwrite(GUEST_CR0, v->arch.hvm_vcpu.hw_cr[0]);
-
-        /* Changing CR0 can change some bits in real CR4. */
-        vmx_update_guest_cr(v, 4);
-        break;
     }
-
-    case 2:
-        /* CR2 is updated in exit stub. */
-        break;
-
-    case 3:
-        if ( paging_mode_hap(v->domain) )
-        {
-            if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
-                v->arch.hvm_vcpu.hw_cr[3] =
-                    v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT];
-            vmx_load_pdptrs(v);
-        }
-
-        __vmwrite(GUEST_CR3, v->arch.hvm_vcpu.hw_cr[3]);
-        hvm_asid_flush_vcpu(v);
-        break;
-
+        /* Fallthrough: Changing CR0 can change some bits in real CR4. */
     case 4:
         v->arch.hvm_vcpu.hw_cr[4] = HVM_CR4_HOST_MASK;
         if ( paging_mode_hap(v->domain) )
@@ -1681,6 +1660,23 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
         __vmwrite(GUEST_CR4, v->arch.hvm_vcpu.hw_cr[4]);
         break;
 
+    case 2:
+        /* CR2 is updated in exit stub. */
+        break;
+
+    case 3:
+        if ( paging_mode_hap(v->domain) )
+        {
+            if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
+                v->arch.hvm_vcpu.hw_cr[3] =
+                    v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT];
+            vmx_load_pdptrs(v);
+        }
+
+        __vmwrite(GUEST_CR3, v->arch.hvm_vcpu.hw_cr[3]);
+        hvm_asid_flush_vcpu(v);
+        break;
+
     default:
         BUG();
     }
-- 
2.1.4


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

Re: [PATCH 2/3] x86/vmx: Don't self-recurse in vmx_update_guest_cr()

[Roger Pau Monné]

On Fri, Sep 29, 2017 at 06:31:02PM +0000, Andrew Cooper wrote:
> An update to CR4 following a CR0 update can be done easily by falling
> through into the CR4 case.  This avoids unnecessary passes through
> vmx_vmcs_{enter,exit}() and unnecessary stack usage (as the compiler
> cannot optimise this use to a tailcall).
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

LGTM:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Maybe you should add to the commit message that this is not a
functional change?

Thanks, Roger.

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

Re: [PATCH 2/3] x86/vmx: Don't self-recurse in vmx_update_guest_cr()

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Saturday, September 30, 2017 2:31 AM
> 
> An update to CR4 following a CR0 update can be done easily by falling
> through into the CR4 case.  This avoids unnecessary passes through
> vmx_vmcs_{enter,exit}() and unnecessary stack usage (as the compiler
> cannot optimise this use to a tailcall).
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

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

[PATCH 3/3] x86/vmx: Better description of CR4 settings outside of paged mode

[Andrew Cooper]

This rearanges the logic to avoid the double !hvm_paging_enabled(v) check, but
is otherwise identical.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 5b943d4..5b9b074 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1642,21 +1642,40 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
         v->arch.hvm_vcpu.hw_cr[4] |= v->arch.hvm_vcpu.guest_cr[4];
         if ( v->arch.hvm_vmx.vmx_realmode )
             v->arch.hvm_vcpu.hw_cr[4] |= X86_CR4_VME;
-        if ( paging_mode_hap(v->domain) && !hvm_paging_enabled(v) )
-        {
-            v->arch.hvm_vcpu.hw_cr[4] |= X86_CR4_PSE;
-            v->arch.hvm_vcpu.hw_cr[4] &= ~X86_CR4_PAE;
-        }
+
         if ( !hvm_paging_enabled(v) )
         {
             /*
-             * SMEP/SMAP is disabled if CPU is in non-paging mode in hardware.
-             * However Xen always uses paging mode to emulate guest non-paging
-             * mode. To emulate this behavior, SMEP/SMAP needs to be manually
-             * disabled when guest VCPU is in non-paging mode.
+             * When the guest thinks paging is disabled, Xen may need to hide
+             * the effects of running with CR0.PG actually enabled.  There are
+             * two subtly complicated cases.
+             */
+
+            if ( paging_mode_hap(v->domain) )
+            {
+                /*
+                 * On hardware lacking the Unrestricted Guest feature (or with
+                 * it disabled in the VMCS), we may not enter the guest with
+                 * CR0.PG actually disabled.  When EPT is enabled, we run with
+                 * guest paging settings, but with CR3 pointing at
+                 * HVM_PARAM_IDENT_PT which is a 32bit pagetable using 4M
+                 * superpages.  Override the guests paging settings to match.
+                 */
+                v->arch.hvm_vcpu.hw_cr[4] |= X86_CR4_PSE;
+                v->arch.hvm_vcpu.hw_cr[4] &= ~X86_CR4_PAE;
+            }
+
+            /*
+             * Without CR0.PG, all memory accesses are user mode, so
+             * _PAGE_USER must be set in the pagetables for guest userspace to
+             * function.  This in turn trips up guest supervisor mode if
+             * SMEP/SMAP are left active in context.  They wouldn't have any
+             * effect if paging was actually disabled, so hide them behind the
+             * back of the guest.
              */
             v->arch.hvm_vcpu.hw_cr[4] &= ~(X86_CR4_SMEP | X86_CR4_SMAP);
         }
+
         __vmwrite(GUEST_CR4, v->arch.hvm_vcpu.hw_cr[4]);
         break;
 
-- 
2.1.4


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

Re: [PATCH 3/3] x86/vmx: Better description of CR4 settings outside of paged mode

[Roger Pau Monné]

On Fri, Sep 29, 2017 at 06:31:03PM +0000, Andrew Cooper wrote:
> This rearanges the logic to avoid the double !hvm_paging_enabled(v) check, but
> is otherwise identical.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> ---
>  xen/arch/x86/hvm/vmx/vmx.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 5b943d4..5b9b074 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1642,21 +1642,40 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
>          v->arch.hvm_vcpu.hw_cr[4] |= v->arch.hvm_vcpu.guest_cr[4];
>          if ( v->arch.hvm_vmx.vmx_realmode )
>              v->arch.hvm_vcpu.hw_cr[4] |= X86_CR4_VME;
> -        if ( paging_mode_hap(v->domain) && !hvm_paging_enabled(v) )
> -        {
> -            v->arch.hvm_vcpu.hw_cr[4] |= X86_CR4_PSE;
> -            v->arch.hvm_vcpu.hw_cr[4] &= ~X86_CR4_PAE;
> -        }
> +
>          if ( !hvm_paging_enabled(v) )
>          {
>              /*
> -             * SMEP/SMAP is disabled if CPU is in non-paging mode in hardware.
> -             * However Xen always uses paging mode to emulate guest non-paging
> -             * mode. To emulate this behavior, SMEP/SMAP needs to be manually
> -             * disabled when guest VCPU is in non-paging mode.
> +             * When the guest thinks paging is disabled, Xen may need to hide
> +             * the effects of running with CR0.PG actually enabled.  There are
> +             * two subtly complicated cases.
> +             */
> +
> +            if ( paging_mode_hap(v->domain) )
> +            {
> +                /*
> +                 * On hardware lacking the Unrestricted Guest feature (or with
> +                 * it disabled in the VMCS), we may not enter the guest with

Shouldn't this be paging_mode_hap && vmx_unrestricted_guest?

From the code below I think it's harmless what we do with CR4 if
CR0.PG is disabled, but in any case it would be good to mention it in
the comment IMHO.

> +                 * CR0.PG actually disabled.  When EPT is enabled, we run with
> +                 * guest paging settings, but with CR3 pointing at
> +                 * HVM_PARAM_IDENT_PT which is a 32bit pagetable using 4M
> +                 * superpages.  Override the guests paging settings to match.
> +                 */
> +                v->arch.hvm_vcpu.hw_cr[4] |= X86_CR4_PSE;
> +                v->arch.hvm_vcpu.hw_cr[4] &= ~X86_CR4_PAE;

Thanks, Roger.

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

Re: [PATCH 3/3] x86/vmx: Better description of CR4 settings outside of paged mode

[Andrew Cooper]

On 03/10/17 15:04, Roger Pau Monné wrote:
> On Fri, Sep 29, 2017 at 06:31:03PM +0000, Andrew Cooper wrote:
>> This rearanges the logic to avoid the double !hvm_paging_enabled(v) check, but
>> is otherwise identical.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Jun Nakajima <jun.nakajima@intel.com>
>> CC: Kevin Tian <kevin.tian@intel.com>
>> ---
>>  xen/arch/x86/hvm/vmx/vmx.c | 37 ++++++++++++++++++++++++++++---------
>>  1 file changed, 28 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 5b943d4..5b9b074 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -1642,21 +1642,40 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
>>          v->arch.hvm_vcpu.hw_cr[4] |= v->arch.hvm_vcpu.guest_cr[4];
>>          if ( v->arch.hvm_vmx.vmx_realmode )
>>              v->arch.hvm_vcpu.hw_cr[4] |= X86_CR4_VME;
>> -        if ( paging_mode_hap(v->domain) && !hvm_paging_enabled(v) )
>> -        {
>> -            v->arch.hvm_vcpu.hw_cr[4] |= X86_CR4_PSE;
>> -            v->arch.hvm_vcpu.hw_cr[4] &= ~X86_CR4_PAE;
>> -        }
>> +
>>          if ( !hvm_paging_enabled(v) )
>>          {
>>              /*
>> -             * SMEP/SMAP is disabled if CPU is in non-paging mode in hardware.
>> -             * However Xen always uses paging mode to emulate guest non-paging
>> -             * mode. To emulate this behavior, SMEP/SMAP needs to be manually
>> -             * disabled when guest VCPU is in non-paging mode.
>> +             * When the guest thinks paging is disabled, Xen may need to hide
>> +             * the effects of running with CR0.PG actually enabled.  There are
>> +             * two subtly complicated cases.
>> +             */
>> +
>> +            if ( paging_mode_hap(v->domain) )
>> +            {
>> +                /*
>> +                 * On hardware lacking the Unrestricted Guest feature (or with
>> +                 * it disabled in the VMCS), we may not enter the guest with
> Shouldn't this be paging_mode_hap && vmx_unrestricted_guest?

ITYM paging_mode_hap && !vmx_unrestricted_guest

>
> From the code below I think it's harmless what we do with CR4 if
> CR0.PG is disabled, but in any case it would be good to mention it in
> the comment IMHO.

Indeed it is harmless, which is why I didn't include the extra
conditional.  Including it would invalidate my statement of "otherwise
identical".

~Andrew

>
>> +                 * CR0.PG actually disabled.  When EPT is enabled, we run with
>> +                 * guest paging settings, but with CR3 pointing at
>> +                 * HVM_PARAM_IDENT_PT which is a 32bit pagetable using 4M
>> +                 * superpages.  Override the guests paging settings to match.
>> +                 */
>> +                v->arch.hvm_vcpu.hw_cr[4] |= X86_CR4_PSE;
>> +                v->arch.hvm_vcpu.hw_cr[4] &= ~X86_CR4_PAE;
> Thanks, Roger.


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

Re: [PATCH 3/3] x86/vmx: Better description of CR4 settings outside of paged mode

[Roger Pau Monné]

On Tue, Oct 03, 2017 at 02:12:22PM +0000, Andrew Cooper wrote:
> On 03/10/17 15:04, Roger Pau Monné wrote:
> > On Fri, Sep 29, 2017 at 06:31:03PM +0000, Andrew Cooper wrote:
> >> This rearanges the logic to avoid the double !hvm_paging_enabled(v) check, but
> >> is otherwise identical.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Jun Nakajima <jun.nakajima@intel.com>
> >> CC: Kevin Tian <kevin.tian@intel.com>
> >> ---
> >>  xen/arch/x86/hvm/vmx/vmx.c | 37 ++++++++++++++++++++++++++++---------
> >>  1 file changed, 28 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> >> index 5b943d4..5b9b074 100644
> >> --- a/xen/arch/x86/hvm/vmx/vmx.c
> >> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >> @@ -1642,21 +1642,40 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
> >>          v->arch.hvm_vcpu.hw_cr[4] |= v->arch.hvm_vcpu.guest_cr[4];
> >>          if ( v->arch.hvm_vmx.vmx_realmode )
> >>              v->arch.hvm_vcpu.hw_cr[4] |= X86_CR4_VME;
> >> -        if ( paging_mode_hap(v->domain) && !hvm_paging_enabled(v) )
> >> -        {
> >> -            v->arch.hvm_vcpu.hw_cr[4] |= X86_CR4_PSE;
> >> -            v->arch.hvm_vcpu.hw_cr[4] &= ~X86_CR4_PAE;
> >> -        }
> >> +
> >>          if ( !hvm_paging_enabled(v) )
> >>          {
> >>              /*
> >> -             * SMEP/SMAP is disabled if CPU is in non-paging mode in hardware.
> >> -             * However Xen always uses paging mode to emulate guest non-paging
> >> -             * mode. To emulate this behavior, SMEP/SMAP needs to be manually
> >> -             * disabled when guest VCPU is in non-paging mode.
> >> +             * When the guest thinks paging is disabled, Xen may need to hide
> >> +             * the effects of running with CR0.PG actually enabled.  There are
> >> +             * two subtly complicated cases.
> >> +             */
> >> +
> >> +            if ( paging_mode_hap(v->domain) )
> >> +            {
> >> +                /*
> >> +                 * On hardware lacking the Unrestricted Guest feature (or with
> >> +                 * it disabled in the VMCS), we may not enter the guest with
> > Shouldn't this be paging_mode_hap && vmx_unrestricted_guest?
> 
> ITYM paging_mode_hap && !vmx_unrestricted_guest
> 
> >
> > From the code below I think it's harmless what we do with CR4 if
> > CR0.PG is disabled, but in any case it would be good to mention it in
> > the comment IMHO.
> 
> Indeed it is harmless, which is why I didn't include the extra
> conditional.  Including it would invalidate my statement of "otherwise
> identical".

Right.

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

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

Re: [PATCH 3/3] x86/vmx: Better description of CR4 settings outside of paged mode

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Saturday, September 30, 2017 2:31 AM
> 
> This rearanges the logic to avoid the double !hvm_paging_enabled(v) check,
> but
> is otherwise identical.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

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