Re: [PATCH] Nested VMX: CR emulation fix up

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 10/08/2013 04:31 AM, Jan Beulich wrote:
>>>> On 08.10.13 at 09:29, Yang Zhang <yang.z.zhang@intel.com> 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.



-boris