Re: [PATCH v3] VMX: Eliminate cr3 save/loading exiting when UG enabled

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 08/01/2014 01:13, Zhang, Yang Z wrote:
> Zhang, Yang Z wrote on 2013-12-19:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>
>> With the feature of unrestricted guest, there should be no vmexit be
>> triggered when guest accesses the cr3 in non-paging mode. This patch
>> will clear the cr3 save/loading bit in vmcs control filed to eliminate
>> cr3 access vmexit on UG avaliable hardware.
>>
>> The previous patch (commit c9efe34c119418a5ac776e5d91aeefcce4576518)
>> did the same thing compare to this one. But it will cause guest fail
>> to boot up on non-UG hardware which is repoted by Jan and it has been
>> reverted (commit 1e2bf05ec37cf04b0e01585eae524509179f165e).
>>
> Hi, Jun.
>
> Can you help to review this patch?

This got committed earlier today
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=f4fed540e78ac8a2bd3b1dee53a5206dde25f613)
as you are a maintainer, and I (as an independent party as far as the
patch goes) reviewed it.

>
>> This patch incorporate the fixing and guest are working well both in
>> UG and non-UG platform with this patch.
>>
>> Reported-by: Jan Beulich <jbeulich@suse.com>
>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>> ---
>> changes in v3:
>> Revise the patch description according Jan's suggestion
>>
>> changes in v2:
>> Fix the guest boot failure on non-UG platform.
>>
>> There are some discussions around the first patch, please see the
>> following link: http://www.gossamer-threads.com/lists/xen/devel/302810
>>
>> ---
>>  xen/arch/x86/hvm/vmx/vmx.c |    9 +++++----
>>  1 files changed, 5 insertions(+), 4 deletions(-)
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index dfff628..f6409d6 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++
>> b/xen/arch/x86/hvm/vmx/vmx.c @@ -1157,7 +1157,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. */
>> @@ -1231,7 +1231,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);
>> @@ -2487,10 +2487,11 @@ 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];
>> +        if ( vmx_unrestricted_guest(v) || hvm_paging_enabled(v) )
>> +            v->arch.hvm_vcpu.guest_cr[3] = v->arch.hvm_vcpu.hw_cr[3];
>>      }
>>      
>>      __vmread(VM_EXIT_REASON, &exit_reason);
>
> Best regards,
> Yang
>
>