Xen devel - Oct 2013 - [PATCH] VMX: Eliminate cr3 store/load vmexit when UG enabled

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

With the feature of unrestricted guest, there should no vmexit
be triggered when guest accesses the cr3 in non-paging mode.

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

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 9ca8632..b9b5e74 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1086,7 +1086,7 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned
int cr)
             uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
                                  CPU_BASED_CR3_STORE_EXITING);
             v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
-            if ( !hvm_paging_enabled(v) )
+            if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
                 v->arch.hvm_vmx.exec_control |= cr3_ctls;
 
             /* Trap CR3 updates if CR3 memory events are enabled. */
@@ -1156,7 +1156,7 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned
int cr)
     case 3:
         if ( paging_mode_hap(v->domain) )
         {
-            if ( !hvm_paging_enabled(v) )
+            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);
@@ -2408,7 +2408,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
 
     hvm_invalidate_regs_fields(regs);
 
-    if ( paging_mode_hap(v->domain) && hvm_paging_enabled(v) )
+    if ( paging_mode_hap(v->domain) )
     {
         __vmread(GUEST_CR3, &v->arch.hvm_vcpu.hw_cr[3]);
         v->arch.hvm_vcpu.guest_cr[3] = v->arch.hvm_vcpu.hw_cr[3];
-- 
1.7.1

On 23/10/13 08:39, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
>
> With the feature of unrestricted guest, there should no vmexit
> be triggered when guest accesses the cr3 in non-paging mode.
>
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
You English here confused me for a bit.  I presume you mean "Xen should
not cause vmexits for cr3 accesses in unrestricted guests", whereas the
current meaning implies that hardware wont generate a vmexit for cr3
accesses for unrestricted guests (which is not correct according to the
SDM).
> ---
>  xen/arch/x86/hvm/vmx/vmx.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 9ca8632..b9b5e74 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1086,7 +1086,7 @@ static void vmx_update_guest_cr(struct vcpu *v,
unsigned int cr)
>              uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
>                                   CPU_BASED_CR3_STORE_EXITING);
>              v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
> -            if ( !hvm_paging_enabled(v) )
> +            if ( !hvm_paging_enabled(v) &&
!vmx_unrestricted_guest(v) )
From my (brief, so please correct me if I am wrong) reading of the spec,
guest_CR0.PG is enforced if SECONDARY_EXEC_UNRESTRICTED_GUEST is clear. 
Therefore, !hvm_paging_enable() implies
SECONDARY_EXEC_UNRESTRICTED_GUEST is set (or we will incur a vmentry
failure)
>                  v->arch.hvm_vmx.exec_control |= cr3_ctls;
>  
>              /* Trap CR3 updates if CR3 memory events are enabled. */
> @@ -1156,7 +1156,7 @@ static void vmx_update_guest_cr(struct vcpu *v,
unsigned int cr)
>      case 3:
>          if ( paging_mode_hap(v->domain) )
>          {
> -            if ( !hvm_paging_enabled(v) )
> +            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);
Why does unrestricted mode affect which pagetables we use when moving
back into Xen?
> @@ -2408,7 +2408,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>  
>      hvm_invalidate_regs_fields(regs);
>  
> -    if ( paging_mode_hap(v->domain) && hvm_paging_enabled(v) )
> +    if ( paging_mode_hap(v->domain) )
>      {
>          __vmread(GUEST_CR3, &v->arch.hvm_vcpu.hw_cr[3]);
>          v->arch.hvm_vcpu.guest_cr[3] = v->arch.hvm_vcpu.hw_cr[3];
~Andrew

Andrew Cooper wrote on 2013-10-23:
> On 23/10/13 08:39, Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> With the feature of unrestricted guest, there should no vmexit be
>> triggered when guest accesses the cr3 in non-paging mode.
>> 
>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> 
> You English here confused me for a bit.  I presume you mean "Xen
> should not cause vmexits for cr3 accesses in unrestricted guests",
> whereas the current meaning implies that hardware wont generate a
> vmexit for cr3 accesses for unrestricted guests (which is not correct
according to the SDM).
> 
Apology for my poor English. Yes, your understanding is right.
>> ---
>>  xen/arch/x86/hvm/vmx/vmx.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index 9ca8632..b9b5e74 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -1086,7 +1086,7 @@ static void vmx_update_guest_cr(struct vcpu
>> *v,
> unsigned int cr)
>>              uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
> CPU_BASED_CR3_STORE_EXITING);
>>              v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
>> -            if ( !hvm_paging_enabled(v) )
>> +            if ( !hvm_paging_enabled(v) &&
>> + !vmx_unrestricted_guest(v)
>> + )
> 
> From my (brief, so please correct me if I am wrong) reading of the spec,
> guest_CR0.PG is enforced if SECONDARY_EXEC_UNRESTRICTED_GUEST is clear.
> Therefore, !hvm_paging_enable() implies
> SECONDARY_EXEC_UNRESTRICTED_GUEST is set (or we will incur a vmentry
> failure)
> 
GUEST_CR0 from SPEC means hw_cr in Xen not guest_cr. hvm_paging_enable() checks
the paging status that as guest saw them not the real status in hardware.
>>                  v->arch.hvm_vmx.exec_control |= cr3_ctls;
>>              /* Trap CR3 updates if CR3 memory events are enabled.
>> */ @@ -1156,7 +1156,7 @@ static void vmx_update_guest_cr(struct vcpu
>> *v,
> unsigned int cr)
>>      case 3:
>>          if ( paging_mode_hap(v->domain) )
>>          {
>> -            if ( !hvm_paging_enabled(v) )
>> +            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);
> 
> Why does unrestricted mode affect which pagetables we use when moving
> back into Xen?
With unrestricted guest, the CR3 of hardware is owned by guest itself.
Hypervisor should not touch the CR3. I don''t why current logic tries to
modify the CR3 as long as guest in non-paging mode which totally ignore the UG.
Normally, it will work well since we trap all guests'' modifications to
CR3 and CR0.PG. But in nested case, we saw an issue that L2 running in
non-paging mode with UG enabled, and L1 set CR3 for L2 to run. With currently
logic, L0 will rewrite the CR3 with its value and cause the problem.

> 
>> @@ -2408,7 +2408,7 @@ void vmx_vmexit_handler(struct cpu_user_regs
>> *regs)
>> 
>>      hvm_invalidate_regs_fields(regs);
>> -    if ( paging_mode_hap(v->domain) &&
hvm_paging_enabled(v) )
>> +    if ( paging_mode_hap(v->domain) )
>>      {
>>          __vmread(GUEST_CR3, &v->arch.hvm_vcpu.hw_cr[3]);
>>          v->arch.hvm_vcpu.guest_cr[3] =
v->arch.hvm_vcpu.hw_cr[3];
> 
> ~Andrew

Best regards,
Yang

>>> On 24.10.13 at 06:41, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
> Andrew Cooper wrote on 2013-10-23:
>> On 23/10/13 08:39, Yang Zhang wrote:
>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>> 
>>> With the feature of unrestricted guest, there should no vmexit be
>>> triggered when guest accesses the cr3 in non-paging mode.
>>> 
>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> You English here confused me for a bit.  I presume you mean "Xen
>> should not cause vmexits for cr3 accesses in unrestricted guests",
>> whereas the current meaning implies that hardware wont generate a
>> vmexit for cr3 accesses for unrestricted guests (which is not correct 
> according to the SDM).
>> 
> 
> Apology for my poor English. Yes, your understanding is right.
So assuming we''ll get an ack from one of the VMX maintainers,
should this then be committed with the suggested change to the
description? Also, Andrew, any more concerns regarding this
change (IOW did Yang address your earlier questions)?

Jan

On 28/10/13 13:22, Jan Beulich wrote:
>>>> On 24.10.13 at 06:41, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
>> Andrew Cooper wrote on 2013-10-23:
>>> On 23/10/13 08:39, Yang Zhang wrote:
>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>
>>>> With the feature of unrestricted guest, there should no vmexit
be
>>>> triggered when guest accesses the cr3 in non-paging mode.
>>>>
>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>>> You English here confused me for a bit.  I presume you mean
"Xen
>>> should not cause vmexits for cr3 accesses in unrestricted
guests",
>>> whereas the current meaning implies that hardware wont generate a
>>> vmexit for cr3 accesses for unrestricted guests (which is not
correct
>> according to the SDM).
>> Apology for my poor English. Yes, your understanding is right.
> So assuming we''ll get an ack from one of the VMX maintainers,
> should this then be committed with the suggested change to the
> description? Also, Andrew, any more concerns regarding this
> change (IOW did Yang address your earlier questions)?
>
> Jan
>
I think so, but I really don''t think I know the implications of the
changes well enough to be happy giving it a Reviewed-by tag.

Given the clarification regarding the commit message, I shall defer to
the maintainers for the correctness of the change.

~Andrew

Jan Beulich wrote on 2013-10-28:
>>>> On 24.10.13 at 06:41, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
>> Andrew Cooper wrote on 2013-10-23:
>>> On 23/10/13 08:39, Yang Zhang wrote:
>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>> 
>>>> With the feature of unrestricted guest, there should no vmexit
be
>>>> triggered when guest accesses the cr3 in non-paging mode.
>>>> 
>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>>> 
>>> You English here confused me for a bit.  I presume you mean
"Xen
>>> should not cause vmexits for cr3 accesses in unrestricted
guests",
>>> whereas the current meaning implies that hardware wont generate a
>>> vmexit for cr3 accesses for unrestricted guests (which is not
correct
>>> according to the SDM).
>>> 
>> 
>> Apology for my poor English. Yes, your understanding is right.
> 
> So assuming we''ll get an ack from one of the VMX maintainers,
should
> this then be committed with the suggested change to the description?
Sure. If no need to revise the current patch, please help to commit it with the
comments Paolo suggested.
> Also, Andrew, any more concerns regarding this change (IOW did Yang
> address your earlier questions)?
> 
> Jan

Best regards,
Yang

Andrew Cooper wrote on 2013-10-28:
> On 28/10/13 13:22, Jan Beulich wrote:
>>>>> On 24.10.13 at 06:41, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
>>> Andrew Cooper wrote on 2013-10-23:
>>>> On 23/10/13 08:39, Yang Zhang wrote:
>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>> 
>>>>> With the feature of unrestricted guest, there should no
vmexit be
>>>>> triggered when guest accesses the cr3 in non-paging mode.
>>>>> 
>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>>>> You English here confused me for a bit.  I presume you mean
"Xen
>>>> should not cause vmexits for cr3 accesses in unrestricted
guests",
>>>> whereas the current meaning implies that hardware wont generate
a
>>>> vmexit for cr3 accesses for unrestricted guests (which is not
>>>> correct
>>> according to the SDM).
>>> Apology for my poor English. Yes, your understanding is right.
>> So assuming we''ll get an ack from one of the VMX maintainers,
should
>> this then be committed with the suggested change to the description?
>> Also, Andrew, any more concerns regarding this change (IOW did Yang
>> address your earlier questions)?
>> 
>> Jan
>> 
> 
> I think so, but I really don''t think I know the implications of
the
> changes well enough to be happy giving it a Reviewed-by tag.
I will ask our QA to do a full testing against this patch to see whether it
introduces any regression.
> 
> Given the clarification regarding the commit message, I shall defer to
> the maintainers for the correctness of the change.
> 
> ~Andrew

Best regards,
Yang

On Mon, Oct 28, 2013 at 6:52 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 28/10/13 13:22, Jan Beulich wrote:
>>>>> On 24.10.13 at 06:41, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
>>> Andrew Cooper wrote on 2013-10-23:
>>>> On 23/10/13 08:39, Yang Zhang wrote:
>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>
>>>>> With the feature of unrestricted guest, there should no
vmexit be
>>>>> triggered when guest accesses the cr3 in non-paging mode.
>>>>>
>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>>>> You English here confused me for a bit.  I presume you mean
"Xen
>>>> should not cause vmexits for cr3 accesses in unrestricted
guests",
>>>> whereas the current meaning implies that hardware wont generate
a
>>>> vmexit for cr3 accesses for unrestricted guests (which is not
correct
>>> according to the SDM).
>>> Apology for my poor English. Yes, your understanding is right.
>> So assuming we''ll get an ack from one of the VMX maintainers,
>> should this then be committed with the suggested change to the
>> description? Also, Andrew, any more concerns regarding this
>> change (IOW did Yang address your earlier questions)?
>>
>> Jan
>>
>
> I think so, but I really don''t think I know the implications of
the
> changes well enough to be happy giving it a Reviewed-by tag.
>
> Given the clarification regarding the commit message, I shall defer to
> the maintainers for the correctness of the change.
>
> ~Andrew
The patch seems doing the right thing.

Acked-by: Jun Nakajima <jun.nakajima@intel.com>

-- 
Jun
Intel Open Source Technology Center

>>> On 29.10.13 at 02:02, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
> Jan Beulich wrote on 2013-10-28:
>>>>> On 24.10.13 at 06:41, "Zhang, Yang Z"
<yang.z.zhang@intel.com> wrote:
>>> Andrew Cooper wrote on 2013-10-23:
>>>> On 23/10/13 08:39, Yang Zhang wrote:
>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>> 
>>>>> With the feature of unrestricted guest, there should no
vmexit be
>>>>> triggered when guest accesses the cr3 in non-paging mode.
>>>>> 
>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>>>> 
>>>> You English here confused me for a bit.  I presume you mean
"Xen
>>>> should not cause vmexits for cr3 accesses in unrestricted
guests",
>>>> whereas the current meaning implies that hardware wont generate
a
>>>> vmexit for cr3 accesses for unrestricted guests (which is not
correct
>>>> according to the SDM).
>>>> 
>>> 
>>> Apology for my poor English. Yes, your understanding is right.
>> 
>> So assuming we''ll get an ack from one of the VMX maintainers,
should
>> this then be committed with the suggested change to the description?
> 
> Sure. If no need to revise the current patch, please help to commit it with
> the comments Paolo suggested.
Andrew you mean?

Jan