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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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