From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH] Nested VMX: CR emulation fix up Date: Tue, 08 Oct 2013 11:46:16 -0400 Message-ID: <525428C8.1050502@oracle.com> References: <1381217398-16449-1-git-send-email-yang.z.zhang@intel.com> <5253DEFB02000078000F97A4@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VTZRt-0003tZ-6b for xen-devel@lists.xenproject.org; Tue, 08 Oct 2013 15:43:49 +0000 In-Reply-To: <5253DEFB02000078000F97A4@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Yang Zhang , eddie.dong@intel.com, Jacob Shin , suravee.suthikulpanit@amd.com, xen-devel List-Id: xen-devel@lists.xenproject.org On 10/08/2013 04:31 AM, Jan Beulich wrote: >>>> On 08.10.13 at 09:29, Yang Zhang 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_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. -boris