From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kai Huang Subject: Re: [v2 06/11] vmx: add help functions to support PML Date: Fri, 17 Apr 2015 11:10:04 +0800 Message-ID: <5530798C.2060108@linux.intel.com> References: <1429081433-9600-1-git-send-email-kai.huang@linux.intel.com> <1429081433-9600-7-git-send-email-kai.huang@linux.intel.com> <552FF47B0200007800072E23@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <552FF47B0200007800072E23@mail.emea.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: andrew.cooper3@citrix.com, kevin.tian@intel.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 04/16/2015 11:42 PM, Jan Beulich wrote: >>>> On 15.04.15 at 09:03, wrote: >> +int vmx_vcpu_enable_pml(struct vcpu *v) >> +{ >> + struct domain *d = v->domain; >> + >> + if ( vmx_vcpu_pml_enabled(v) ) >> + return 0; >> + >> + v->arch.hvm_vmx.pml_pg = d->arch.paging.alloc_page(d); > So you latch v->domain into d for this invocation, ... > >> +void vmx_vcpu_disable_pml(struct vcpu *v) >> +{ >> + if ( !vmx_vcpu_pml_enabled(v) ) >> + return; >> + >> + /* Make sure we don't lose any logged GPAs */ >> + vmx_vcpu_flush_pml_buffer(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); > ... but not for this one. Please be consistent. Hmm. My bad. I'll use v->domain in both function. > >> +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); > Don't you require the vCPU to be non-running or current when you > get here? If so, perhaps add a respective ASSERT()? Yes an ASSERT would be better. v->pause_count will be increased if vcpu is kicked out by domain_pause explicitly, but looks the same thing won't be done if vcpu is kicked out by PML buffer full VMEXIT. So should the ASSERT be done like below? ASSERT(atomic_read(&v->pause_count) || (v == current)); > >> + >> + /* Do nothing if PML buffer is empty */ >> + if ( pml_idx == (NR_PML_ENTRIES - 1) ) >> + goto out; >> + >> + pml_buf = __map_domain_page(v->arch.hvm_vmx.pml_pg); >> + >> + /* >> + * 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 >= NR_PML_ENTRIES) >> + pml_idx = 0; >> + else >> + pml_idx++; >> + >> + for ( ; pml_idx < NR_PML_ENTRIES; pml_idx++ ) >> + { >> + unsigned long gfn = pml_buf[pml_idx] >> PAGE_SHIFT; > Blank line here please. Will do. > >> + /* >> + * 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) ) > Indentation. To be where exactly? Sorry I didn't find an example to refer in such case. > >> + paging_mark_gfn_dirty(v->domain, gfn); >> + } >> + >> + unmap_domain_page(pml_buf); >> + >> + /* Reset PML index */ >> + __vmwrite(GUEST_PML_INDEX, NR_PML_ENTRIES - 1); >> + >> +out: > Labels indented by at least one space please. OK. I'll put one space before the "out:" label. Thanks, -Kai > > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel