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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> +
> +             /* 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.

Jan


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