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 15:45:42 +0800 Message-ID: <5530BA26.1060906@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> <5530798C.2060108@linux.intel.com> <5530C30D02000078000730D4@mail.emea.novell.com> <5530AD7D.4020101@linux.intel.com> <5530CB4A0200007800073125@mail.emea.novell.com> <5530B4E6.1020907@linux.intel.com> <5530D471020000780007319D@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: <5530D471020000780007319D@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/17/2015 03:37 PM, Jan Beulich wrote: >>>> On 17.04.15 at 09:23, wrote: >> On 04/17/2015 02:58 PM, Jan Beulich wrote: >>>>>> On 17.04.15 at 08:51, wrote: >>>> On 04/17/2015 02:23 PM, Jan Beulich wrote: >>>>>>>> On 17.04.15 at 05:10, wrote: >>>>>> On 04/16/2015 11:42 PM, Jan Beulich wrote: >>>>>>>>>> On 15.04.15 at 09:03, wrote: >>>>>>>> +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)); >>>>> For one I'd reverse the two parts. And then I think pause count >>>>> being non-zero is not a sufficient condition - if a non-synchronous >>>>> pause was issued against the vCPU it may still be running. I'd >>>>> suggest !vcpu_runnable(v) && !v->is_running, possibly with the >>>>> pause count check instead of the runnable one if the only >>>>> permitted case where v != current requires the vCPU to be >>>>> paused. >>>> The vmx_vcpu_flush_pml_buffer is only supposed to be called in below cases: >>>> >>>> - When PML full VMEXIT happens >>>> - In paging_log_dirty_op & hap_track_dirty_vram, before reporting >>>> dirty pages to userspace. >>>> - In vmx_vcpu_disable_pml, called from vmx_vcpu_destroy, or when >>>> log-dirty mode is disabled. >>>> >>>> In the latter two cases, domain_pause is guaranteed to be called before >>>> vmx_vcpu_flush_pml_buffer is called, therefore looks there's no >>>> possibility of non-synchronous pause of the vcpu. >>>> >>>> Or are you suggesting we should suppose this function can be called from >>>> any caller, and meanwhile is able to act reasonably? >>> No. All I'm saying is in order to protect against eventual undue >>> future callers, it should assert that its preconditions are met. I.e. >>> if the vCPU is expected to be paused, check that the pause >>> count in non-zero _and_ that the pause actually took effect. >> I see. I will do as you suggested: >> >> ASSERT((v == current) || (!vcpu_runnable(v) && !v->is_running)); >> >> And v != current should be the only case requires the vcpu to be paused. > But if you require (or at least expect) the vCPU to be paused, this > isn't what I suggested. Afaict > > ASSERT((v == current) || (!v->is_running && atomic_read(v->pause_count))); > > would then be the right check (and, while not be a full guarantee that > things wouldn't change behind your back, would at least increase > chances that the vCPU's runnable state won't change, as the vCPU > could have been non-runnable for reasons other than having been > paused). You are right. Your last sentence convinced me. I didn't go that far. I will use atomic_read(&v->pause_count) instead of !vcpu_runnable(v). Thanks, -Kai > > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel