[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] Nested VMX: CR emulation fix up
>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |