Re: [Xen-devel] [PATCH] Nested VMX: CR emulation fix up

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

Boris Ostrovsky wrote on 2013-10-08:
> On 10/08/2013 04:31 AM, Jan Beulich wrote:
>>>>> On 08.10.13 at 09:29, Yang Zhang <yang.z.zhang@xxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -1817,6 +1817,9 @@ int hvm_set_cr0(unsigned long value)
>>>       }
>>>       
>>>       v->arch.hvm_vcpu.guest_cr[0] = value;
>>> +    if ( !nestedhvm_vmswitch_in_progress(v) &&
>>> +         nestedhvm_vcpu_in_guestmode(v) )
>>> +        v->arch.hvm_vcpu.nvcpu.guest_cr[0] = value;
>>>       hvm_update_guest_cr(v, 0);
>>>       
>>>       if ( (value ^ old_value) & X86_CR0_PG ) { @@ -1899,6 +1902,9 @@
>>>       int hvm_set_cr4(unsigned long value) }
>>>       
>>>       v->arch.hvm_vcpu.guest_cr[4] = value;
>>> +    if ( !nestedhvm_vmswitch_in_progress(v) &&
>>> +         nestedhvm_vcpu_in_guestmode(v) )
>>> +        v->arch.hvm_vcpu.nvcpu.guest_cr[4] = value;
>>>       hvm_update_guest_cr(v, 4);
>> Considering the redundancy - wouldn't all of the above now become
>> the body of a rather desirable helper function?
>> 
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -1096,6 +1096,30 @@ static void vmx_update_guest_cr(struct vcpu
>>> *v,
> unsigned int cr)
>>>               vmx_update_cpu_exec_control(v);
>>>           }
>>> +        if ( nestedhvm_vcpu_in_guestmode(v) ) +        { +           
>>> if ( !nestedhvm_vmswitch_in_progress(v) ) +            { +            
>>>    /* +                 * We get here when L2 changed cr0 in a way
>>> that + did not change +                 * any of L1's shadowed bits
>>> (see nvmx_n2_vmexit_handler), +                 * but did change L0
>>> shadowed bits. So we first calculate the +                 * effective
>>> cr0 value that L1 would like to write + into the +                 *
>>> hardware. It consists of the L2-owned bits from + the new +           
>>>      * value combined with the L1-owned bits from L1's guest cr0. +   
>>>              */ +                v->arch.hvm_vcpu.guest_cr[0] &= +    
>>>                ~__get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx,
>>> CR0_GUEST_HOST_MASK); +                v->arch.hvm_vcpu.guest_cr[0] |=
>>> +                    __get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx,
>>> GUEST_CR0) & +                   
>>> __get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx, CR0_GUEST_HOST_MASK); +       
>>>     } +             /* nvcpu.guest_cr[0] is what L2 write to cr0
>>> actually. */ +            __vmwrite(CR0_READ_SHADOW,
>>> v->arch.hvm_vcpu.nvcpu.guest_cr[0]); +        } +        else +       
>>>     __vmwrite(CR0_READ_SHADOW, + v->arch.hvm_vcpu.guest_cr[0]);
>> Please re-phrase this into
>> 
>>          if ( !nestedhvm_vcpu_in_guestmode(v) )
>>          ...
>>          else if ( !nestedhvm_vmswitch_in_progress(v) )
>> For one that'll put the "normal" (non-nested) case first. And second
>> it'll reduce indentation on the main portion of your additions (at
>> once taking care of the otherwise over-long lines in there).
>> 
>> I'm btw also mildly concerned that the moving ahead of this VMCS
>> write might have other side effects. I did check that we don't read
>> the shadow value other than in debugging and nested code, but I'm
>> nevertheless not quite certain that this is indeed benign.
>> 
>>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>>> @@ -1042,6 +1042,8 @@ static void load_shadow_guest_state(struct
>>> vcpu
> *v)
>>>       vvmcs_to_shadow_bulk(v, ARRAY_SIZE(vmcs_gstate_field),
>>>                            vmcs_gstate_field);
>>> +    nvcpu->guest_cr[0] = __get_vvmcs(vvmcs, CR0_READ_SHADOW);
>>> +    nvcpu->guest_cr[4] = __get_vvmcs(vvmcs, CR4_READ_SHADOW);
>> Given that the only time where these get read is in
>> vmx_update_guest_cr() (for writing CR<n>_READ_SHADOW), are the
>> writes above really needed? And if they are, aren't there other
>> updates to these two fields still missing?
>> 
>>> --- a/xen/include/asm-x86/hvm/vcpu.h
>>> +++ b/xen/include/asm-x86/hvm/vcpu.h
>>> @@ -100,6 +100,9 @@ struct nestedvcpu {
>>>        */
>>>       bool_t nv_ioport80;
>>>       bool_t nv_ioportED;
>>> +
>>> +    /* L2's control-resgister, just as the L2 sees them. */
>>> +    unsigned long       guest_cr[5];
> 
> This should be prefixed with nv_: all members of this structure are.
> In addition, struct hvm_vcpu has exact same member.
> 
>> Considering that this touches code common with nested SVM, I'd
>> expect the SVM maintainers to have to approve of the change in any case.
>> 
>> In particular I wonder whether this addition isn't obsoleting SVM's
>> ns_cr0.
>> 
> 
> I am not sure whether ns_cr0 (replaced with nv_guest_cr[0]) would then
> be updated in paths where it currently is not.
> 
> For example in nsvm_vmcb_prepare4vmrun():
> 
>      /* CR0 */ svm->ns_cr0 = v->arch.hvm_vcpu.guest_cr[0]; cr0 =
>      nestedsvm_fpu_vmentry(svm->ns_cr0, ns_vmcb, n1vmcb, n2vmcb);
>      v->arch.hvm_vcpu.guest_cr[0] = ns_vmcb->_cr0; rc =
>      hvm_set_cr0(cr0);  <------ nv_guest_cr[0] will get set here.
I am not familiar with SVM code. If you think this change may impact the nested 
SVM. Then I will move the change to VMX specific code.

Best regards,
Yang



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