[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] Nested VMX: CR emulation fix up
Boris Ostrovsky wrote on 2013-10-08: > On 10/08/2013 04:31 AM, Jan Beulich wrote: >>>>> 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]; > > This should be prefixed with nv_: all members of this structure are. > In addition, struct hvm_vcpu has exact same member. > >> 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. >> > > I am not sure whether ns_cr0 (replaced with nv_guest_cr[0]) would then > be updated in paths where it currently is not. > > For example in nsvm_vmcb_prepare4vmrun(): > > /* CR0 */ svm->ns_cr0 = v->arch.hvm_vcpu.guest_cr[0]; cr0 = > nestedsvm_fpu_vmentry(svm->ns_cr0, ns_vmcb, n1vmcb, n2vmcb); > v->arch.hvm_vcpu.guest_cr[0] = ns_vmcb->_cr0; rc = > hvm_set_cr0(cr0); <------ nv_guest_cr[0] will get set here. I am not familiar with SVM code. If you think this change may impact the nested SVM. Then I will move the change to VMX specific code. Best regards, Yang _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |