From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kai Huang Subject: Re: [PATCH 05/10] VMX: add help functions to support PML Date: Mon, 30 Mar 2015 14:43:42 +0800 Message-ID: <5518F09E.9070407@linux.intel.com> References: <1427423754-11841-1-git-send-email-kai.huang@linux.intel.com> <1427423754-11841-6-git-send-email-kai.huang@linux.intel.com> <5515C715.3050404@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5515C715.3050404@citrix.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: Andrew Cooper , jbeulich@suse.com, tim@xen.org, kevin.tian@intel.com, yang.z.zhang@intel.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 03/28/2015 05:09 AM, Andrew Cooper wrote: > On 27/03/15 02:35, Kai Huang wrote: >> This patch adds help functions to enable/disable PML, and flush PML buffer for >> single vcpu and particular domain for further use. >> >> Signed-off-by: Kai Huang >> --- >> xen/arch/x86/hvm/vmx/vmcs.c | 190 +++++++++++++++++++++++++++++++++++++ >> xen/include/asm-x86/hvm/vmx/vmcs.h | 9 ++ >> 2 files changed, 199 insertions(+) >> >> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >> index 2798b0b..17cbef4 100644 >> --- a/xen/arch/x86/hvm/vmx/vmcs.c >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >> @@ -1326,6 +1326,196 @@ void vmx_clear_eoi_exit_bitmap(struct vcpu *v, u8 vector) >> &v->arch.hvm_vmx.eoi_exitmap_changed); >> } >> >> +int vmx_vcpu_pml_enabled(struct vcpu *v) > bool_t vmx_vcpu_pml_enabled(const struct vcpu *v) Will do. > >> +{ >> + return (v->arch.hvm_vmx.secondary_exec_control & >> + SECONDARY_EXEC_ENABLE_PML) ? 1 : 0; > This would be slightly shorter as > !!(v->arch.hvm_vmx.secondary_exec_control & SECONDARY_EXEC_ENABLE_PML) Will do. > >> +} >> + >> +int vmx_vcpu_enable_pml(struct vcpu *v) >> +{ >> + struct domain *d = v->domain; >> + >> + ASSERT(!vmx_vcpu_pml_enabled(v)); >> + >> + v->arch.hvm_vmx.pml_pg = d->arch.paging.alloc_page(d); >> + if ( !v->arch.hvm_vmx.pml_pg ) >> + return -ENOMEM; >> + >> + vmx_vmcs_enter(v); >> + >> + __vmwrite(PML_ADDRESS, page_to_mfn(v->arch.hvm_vmx.pml_pg) << PAGE_SHIFT); >> + __vmwrite(GUEST_PML_INDEX, PML_ENTITY_NUM - 1); >> + >> + v->arch.hvm_vmx.secondary_exec_control |= SECONDARY_EXEC_ENABLE_PML; >> + >> + __vmwrite(SECONDARY_VM_EXEC_CONTROL, >> + v->arch.hvm_vmx.secondary_exec_control); > Alignment. Do you mean to put 'v->arch.hvm_vmx.secondary_exec_control' to the same line with '__vmwrite(SECONDARY_VM_EXEC_CONTROL,'? In this case the number of characters will be 81. > >> + >> + vmx_vmcs_exit(v); >> + >> + return 0; >> +} >> + >> +void vmx_vcpu_disable_pml(struct vcpu *v) >> +{ >> + ASSERT(vmx_vcpu_pml_enabled(v)); >> + >> + vmx_vmcs_enter(v); >> + >> + v->arch.hvm_vmx.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML; >> + __vmwrite(SECONDARY_VM_EXEC_CONTROL, >> + v->arch.hvm_vmx.secondary_exec_control); >> + >> + vmx_vmcs_exit(v); >> + >> + v->domain->arch.paging.free_page(v->domain, v->arch.hvm_vmx.pml_pg); >> + v->arch.hvm_vmx.pml_pg = NULL; >> +} >> + >> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v) >> +{ >> + uint64_t *pml_buf; >> + unsigned long pml_idx; >> + >> + ASSERT(vmx_vcpu_pml_enabled(v)); >> + >> + vmx_vmcs_enter(v); >> + >> + __vmread(GUEST_PML_INDEX, &pml_idx); >> + >> + /* Do nothing if PML buffer is empty */ >> + if ( pml_idx == (PML_ENTITY_NUM - 1) ) >> + goto out; >> + >> + pml_buf = map_domain_page(page_to_mfn(v->arch.hvm_vmx.pml_pg)); > __map_domain_page() is a wrapper which takes a struct page_info Will do. > >> + >> + /* >> + * PML index can be either 2^16-1 (buffer is full), or 0~511 (buffer is not >> + * full), and in latter case PML index always points to next available >> + * entity. >> + */ >> + if (pml_idx >= PML_ENTITY_NUM) >> + pml_idx = 0; >> + else >> + pml_idx++; >> + >> + for ( ; pml_idx < PML_ENTITY_NUM; pml_idx++ ) >> + { >> + struct p2m_domain *p2m = p2m_get_hostp2m(v->domain); > This p2m_get_host_p2m() call should be hoisted out of the loop. Will do. > >> + unsigned long gfn; >> + mfn_t mfn; >> + p2m_type_t t; >> + p2m_access_t a; >> + >> + gfn = pml_buf[pml_idx] >> PAGE_SHIFT; >> + mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL); >> + if ( mfn_x(mfn) == INVALID_MFN ) >> + { >> + /* >> + * Either EPT table entry for mapping the GFN has been destroyed, or >> + * there's something wrong with hardware behavior, in both cases we >> + * should report a warning. >> + */ >> + dprintk(XENLOG_WARNING, "PML: vcpu %d: invalid GPA 0x%lx logged\n", >> + v->vcpu_id, pml_buf[pml_idx]); > It would be shorter to log gfn rather than gpa. Will do. And I'd also like to add the domain ID in the warning info. > >> + continue; >> + } >> + >> + /* >> + * Need to change type from log-dirty to normal memory for logged GFN. >> + * hap_track_dirty_vram depends on it to work. And we really only need >> + * to mark GFNs which hve been successfully changed from log-dirty to >> + * normal memory to be dirty. >> + */ >> + if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty, >> + p2m_ram_rw) ) >> + paging_mark_dirty(v->domain, mfn_x(mfn)); >> + } >> + >> + unmap_domain_page(pml_buf); >> + >> + /* Reset PML index */ >> + __vmwrite(GUEST_PML_INDEX, PML_ENTITY_NUM - 1); >> + >> +out: >> + vmx_vmcs_exit(v); >> +} >> + >> +int vmx_domain_pml_enabled(struct domain *d) > bool_t and const as per vcpu variant. Will do. > >> +{ >> + return (d->arch.hvm_domain.vmx.status & VMX_DOMAIN_PML_ENABLED) ? 1 : 0; >> +} >> + >> +/* >> + * This function enables PML for particular domain. It should be called when >> + * domain is paused. > In which case assert that the domain is paused, or call domain_pause() > yourself to take an extra pause refcount. Which function should I use to assert domain is paused? I didn't find a function like "domain_paused". Is below good enough? ASSERT(atomic_read(&d->pause_count)); > >> + * >> + * PML needs to be enabled globally for all vcpus of the domain, as PML buffer >> + * and PML index are pre-vcpu, but EPT table is shared by vcpus, therefore >> + * enabling PML on partial vcpus won't work. >> + */ >> +int vmx_domain_enable_pml(struct domain *d) >> +{ >> + struct vcpu *v; >> + >> + ASSERT(!vmx_domain_pml_enabled(d)); >> + >> + for_each_vcpu( d, v ) >> + { >> + if ( vmx_vcpu_enable_pml(v) ) >> + goto error; > Please catch the actual rc from vmx_vcpu_enable_pml() and propagate out > of this function, rather than clobbering -ENOMEM with -EINVAL. > > Also, per Xen style, you can drop the braces. Will do. And I'll drop the braces in other functions I added in this patch as well. > > ~Andrew > >> + } >> + >> + d->arch.hvm_domain.vmx.status |= VMX_DOMAIN_PML_ENABLED; >> + >> + return 0; >> + >> +error: >> + for_each_vcpu( d, v ) >> + { >> + if ( vmx_vcpu_pml_enabled(v) ) >> + vmx_vcpu_disable_pml(v); >> + } >> + return -EINVAL; >> +} >> + >> +/* >> + * Disable PML for particular domain. Called when domain is paused. >> + * >> + * The same as enabling PML for domain, disabling PML should be done for all >> + * vcpus at once. >> + */ >> +void vmx_domain_disable_pml(struct domain *d) >> +{ >> + struct vcpu *v; >> + >> + ASSERT(vmx_domain_pml_enabled(d)); >> + >> + for_each_vcpu( d, v ) >> + { >> + vmx_vcpu_disable_pml(v); >> + } >> + >> + d->arch.hvm_domain.vmx.status &= ~VMX_DOMAIN_PML_ENABLED; >> +} >> + >> +/* >> + * Flush PML buffer of all vcpus, and update the logged dirty pages to log-dirty >> + * radix tree. Called when domain is paused. >> + */ >> +void vmx_domain_flush_pml_buffers(struct domain *d) >> +{ >> + struct vcpu *v; >> + >> + ASSERT(vmx_domain_pml_enabled(d)); >> + >> + for_each_vcpu( d, v ) >> + { >> + vmx_vcpu_flush_pml_buffer(v); >> + } >> +} >> + >> int vmx_create_vmcs(struct vcpu *v) >> { >> struct arch_vmx_struct *arch_vmx = &v->arch.hvm_vmx; >> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h >> index 8cc1122..939d097 100644 >> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h >> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h >> @@ -499,6 +499,15 @@ static inline int vmx_add_host_load_msr(u32 msr) >> >> DECLARE_PER_CPU(bool_t, vmxon); >> >> +int vmx_vcpu_pml_enabled(struct vcpu *v); >> +int vmx_vcpu_enable_pml(struct vcpu *v); >> +void vmx_vcpu_disable_pml(struct vcpu *v); >> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v); >> +int vmx_domain_pml_enabled(struct domain *d); >> +int vmx_domain_enable_pml(struct domain *d); >> +void vmx_domain_disable_pml(struct domain *d); >> +void vmx_domain_flush_pml_buffers(struct domain *d); >> + >> #endif /* ASM_X86_HVM_VMX_VMCS_H__ */ >> >> /* > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel