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

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

Jan Beulich wrote on 2013-10-28:
>>>> On 23.10.13 at 09:13, Yang Zhang <yang.z.zhang@xxxxxxxxx> wrote:
>> From: Yang Zhang <yang.z.zhang@xxxxxxxxx>
>> 
>> This patch fixs two issues:
>> 1. The CR_READ_SHADOW should only cover the value that L2 wirtes to
>> CR when L2 is running. But currently, L0 wirtes wrong value to it
>> during virtual vmentry and L2's CR access emualtion.
>> 
>> 2. L2 changed cr[0/4] in a way that did not change any of L1's
>> shadowed bits, but did change L0 shadowed bits. In this case, the
>> effective cr[0/4] value that L1 would like to write into the
>> hardware is consist of the L2-owned bits from the new value combined
>> with the L1-owned bits from L1's guest cr[0/4].
>> 
>> Signed-off-by: Yang Zhang <yang.z.zhang@xxxxxxxxx>
>> Acked-by: "Dong, Eddie" <eddie.dong@xxxxxxxxx>
> 
> So apart from wanting to see a proper ack on this patch (or a
> description of the changes in v2 that makes clear that a previous ack did not 
> get invalidated) ...
> 
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -1096,6 +1096,29 @@ static void vmx_update_guest_cr(struct vcpu
>> *v,
> unsigned int cr)
>>              vmx_update_cpu_exec_control(v);
>>          }
>> +        if ( !nestedhvm_vcpu_in_guestmode(v) ) +           
>> __vmwrite(CR0_READ_SHADOW, v->arch.hvm_vcpu.guest_cr[0]); +        else
>> 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);
> 
> ... I'd also like to see clarified whether __get_vvmcs() really is
> cheap enough to get invoked twice for the same register here instead
> of the result latched into a local variable and re-used.

Actually, this code is executed rarely. But you are right, using a local 
variable is a better choice.
        
> 
>> + +             /* nvcpu.guest_cr[0] is what L2 write to cr0 actually.
>> */ +            __vmwrite(CR0_READ_SHADOW,
>> v->arch.hvm_vcpu.nvcpu.guest_cr[0]); +        } else
> 
> And should the above result in the patch to be rev'ed again, please
> also correct the coding style violation here.

Current code seems a little redundant. I revised it to put the redundant code 
into nvmx_set_cr_read_shadow(). Please review it.

void nvmx_set_cr_read_shadow(struct vcpu *v, unsigned int cr)
{
    unsigned long cr_field, read_shadow_field, mask_field;

    switch ( cr )
    {
    case 0: 
        cr_field = GUEST_CR0;
        read_shadow_field = CR0_READ_SHADOW;
        mask_field = CR0_GUEST_HOST_MASK;
        break;
    case 4:
        cr_field = GUEST_CR4;
        read_shadow_field = CR4_READ_SHADOW;
        mask_field = CR4_GUEST_HOST_MASK;
        break;
    default:
        gdprintk(XENLOG_WARNING, "Set read shadow for CR%d.\n", cr);
        return;
    }

    if ( !nestedhvm_vmswitch_in_progress(v) )
    {
        unsigned long virtual_cr_mask = 
            __get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx, mask_field);

        /*
         * We get here when L2 changed cr 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 cr 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 cr.
         */
        v->arch.hvm_vcpu.guest_cr[cr] &= ~virtual_cr_mask;
        v->arch.hvm_vcpu.guest_cr[cr] |= virtual_cr_mask &
            __get_vvmcs(vcpu_nestedhvm(v).nv_vvmcx, cr_field);
    }

    /* nvcpu.guest_cr is what L2 write to cr actually. */
    __vmwrite(read_shadow_field, v->arch.hvm_vcpu.nvcpu.guest_cr[cr]);
}

> 
> Jan

Best regards,
Yang



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