From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Li, Liang Z" Subject: Re: [PATCH 3/3] vVMX: use latched VMCS machine address Date: Tue, 23 Feb 2016 10:48:10 +0000 Message-ID: References: <5625242002000078000AC73A@prv-mh.provo.novell.com> <562526F702000078000AC76A@prv-mh.provo.novell.com> <56CC430102000078000D51C6@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56CC430102000078000D51C6@prv-mh.provo.novell.com> Content-Language: en-US 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: "Tian, Kevin" , "xen-devel@lists.xensource.com" , Ian Campbell , AndrewCooper , Ian Jackson , "Wang, Yong Y" , osstest service owner , xen-devel , "Nakajima, Jun" , "Jin, Gordon" , "Hu, Robert" List-Id: xen-devel@lists.xenproject.org > Thanks for getting back on this. > > >> --- a/xen/arch/x86/hvm/vmx/vmcs.c > >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c > >> @@ -932,37 +932,36 @@ void vmx_vmcs_switch(paddr_t from, paddr > >> spin_unlock(&vmx->vmcs_lock); > >> } > >> > >> -void virtual_vmcs_enter(void *vvmcs) > >> +void virtual_vmcs_enter(const struct vcpu *v) > >> { > >> - __vmptrld(pfn_to_paddr(domain_page_map_to_mfn(vvmcs))); > >> + __vmptrld(v->arch.hvm_vmx.vmcs_shadow_maddr); > > > > Debug shows v->arch.hvm_vmx.vmcs_shadow_maddr will equal to 0 at > > this point, this will crash the system. > > > >> } > >> > >> -void virtual_vmcs_exit(void *vvmcs) > >> +void virtual_vmcs_exit(const struct vcpu *v) > >> { > >> paddr_t cur = this_cpu(current_vmcs); > >> > >> - __vmpclear(pfn_to_paddr(domain_page_map_to_mfn(vvmcs))); > >> + __vmpclear(v->arch.hvm_vmx.vmcs_shadow_maddr); > > > > Debug shows v->arch.hvm_vmx.vmcs_shadow_maddr will equal to 0 at > > this point, this will crash the system. > > For both of these you provide too little context. In particular ... > > > Maybe we should use pfn_to_paddr(domain_page_map_to_mfn(vvmcs))) > here. > > ... this shouldn't be necessary, since the whole purpose of the patch is to > avoid this, making sure > v->arch.hvm_vmx.vmcs_shadow_maddr always represents > domain_page_map_to_mfn(vvmcs). Hence if you find the latched field to be > zero, we'll need to understand _why_ this is so, i.e. > what code path cleared the field (perhaps prematurely). Yes, it's better to find out the reason for this. > >> @@ -1694,10 +1657,10 @@ int nvmx_handle_vmclear(struct cpu_user_ > >> rc = VMFAIL_INVALID; > >> else if ( gpa == nvcpu->nv_vvmcxaddr ) > >> { > >> - if ( cpu_has_vmx_vmcs_shadowing ) > >> - nvmx_clear_vmcs_pointer(v, nvcpu->nv_vvmcx); > >> - clear_vvmcs_launched(&nvmx->launched_list, > >> - domain_page_map_to_mfn(nvcpu->nv_vvmcx)); > >> + unsigned long mfn = > >> + PFN_DOWN(v->arch.hvm_vmx.vmcs_shadow_maddr); > >> + > >> + nvmx_clear_vmcs_pointer(v, nvcpu->nv_vvmcx); > >> + clear_vvmcs_launched(&nvmx->launched_list, mfn); > > > > v->arch.hvm_vmx.vmcs_shadow_maddr will be set to 0 in > > nvmx_clear_vmcs_pointer() > > so mfn will be 0 at this point, it's incorrect. > > How that? mfn gets latched before calling nvmx_clear_vmcs_pointer(), > precisely because that function would clear > v->arch.hvm_vmx.vmcs_shadow_maddr. If mfn was zero here, > v->arch.hvm_vmx.vmcs_shadow_maddr would need to have been > zero already before the call. > > Jan You are right, I confused the code, mfn is set before nvmx_clear_vmcs_pointer(). Indeed, v->arch.hvm_vmx.vmcs_shadow_maddr may equal to 0 at this point, it will cause clear_vvmcs_launched() failed to remove the vvmcs from the list. Liang