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:32:12 +0800 Message-ID: <55307EBC.30901@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> <20150417001035.GA82377@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150417001035.GA82377@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan , "Tian, Kevin" Cc: "andrew.cooper3@citrix.com" , "jbeulich@suse.com" , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 04/17/2015 08:10 AM, Tim Deegan wrote: > At 22:57 +0000 on 16 Apr (1429225024), Tian, Kevin wrote: > >>> From: Kai Huang [mailto:kai.huang@linux.intel.com] >>> + if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty, >>> + p2m_ram_rw) ) >>> + paging_mark_gfn_dirty(v->domain, gfn); >> Should we handle error from p2m_change_type_one and consequently >> making this flush function non-void? > I don't think we need to return an error, but we should probably > call mark_dirty here for anything except -EBUSY. Hi Kevin, Tim, My intention here is to rule out the GFN with original type that is not p2m_ram_logdirty, though with patch 11 it's not likely have such GFN logged. Looks -EBUSY returns exactly when original type is not p2m_ram_logdirty, so I think it might be OK to do as Tim suggested. But given the same thing has already been done in hap_track_dirty_vram (hap_track_dirty_vram->paging_log_dirty_range, in which gfn is set in bitmap with !p2m_change_type_one is true), and in EPT violation (p2m_change_type_one is called unconditionally without checking return value), I think it should be safe to do the current code here. What's your comments? Thanks, -Kai > >>> + d->arch.hvm_domain.vmx.status |= VMX_DOMAIN_PML_ENABLED; >> I didn't see how this domain-wise flag is useful. Or if we really >> want to go this way, you also need to clear this flag if vcpu pml >> enable is failed in vcpu hotplug phase, since the flag itself means >> all vcpus of the domain so we must keep this intention in all >> places. > IIUC we need this flag so that we know whether to enable PML on any > new vcpus. If we fail to enable PML on hotplug, we fail the hotplug > (like for other vcpu bringup errors) so the invariant still holds. > > Cheers, > > Tim. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel