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

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

Andrew Cooper wrote on 2013-10-23:
> On 23/10/13 08:39, Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@xxxxxxxxx>
>> 
>> 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@xxxxxxxxx>
> 
> 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



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