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

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

Jan Beulich wrote on 2013-10-08:
>>>> 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?
Sure.

> 
>> --- 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).
Yes, this looks much tidily.

> 
> 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.
It should be ok. Guest_cr[] is same to this VMCS. Hypervisor will use it 
instead touch the VMCS when he wants to check guest's CR value. So here just 
change the order of writes VMCS should be ok. 

> 
>> --- 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?
> 
Not sure whether I am understand your meaning. There are two places that L0 
need to update CR_READ_SHADOW for L2 and this patch already handled the two 
cases:
1. The first place is during virtual vmentry. If L2 writing the CR causes 
vmexit, and L0 inject it to L1. Then L1 will write the CR_READ_SHADOW in 
virtual vmcs. So during virtual vmentry, L0 need to read it from virtual vmcs 
and set it in hardware. Here is handling this case.
2. The second place is that when L2 writing the CR directly and it didn't 
change any L1's shadow bits. Then L0 will not inject it to L1. Instead, L0 will 
handle the vmexit by itself. First chunk of this patch did this.

>> --- 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];
> 
> 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.
> 
> Jan


Best regards,
Yang



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