From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [V13 PATCH 1/2] pvh dom0: Add and remove foreign pages Date: Thu, 22 May 2014 16:20:29 -0700 Message-ID: <20140522162029.281bfdee@mantra.us.oracle.com> References: <1400543499-20328-1-git-send-email-mukesh.rathor@oracle.com> <1400543499-20328-2-git-send-email-mukesh.rathor@oracle.com> <537B4B870200007800014060@mail.emea.novell.com> <20140520164604.07912a6e@mantra.us.oracle.com> <537C78FF02000078000145F5@mail.emea.novell.com> <20140521181523.49dde97c@mantra.us.oracle.com> <537DC47B0200007800014CA3@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WncI2-0002Z8-QQ for xen-devel@lists.xenproject.org; Thu, 22 May 2014 23:20:47 +0000 In-Reply-To: <537DC47B0200007800014CA3@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: George.Dunlap@eu.citrix.com, tim@xen.org, eddie.dong@intel.com, keir.xen@gmail.com, jun.nakajima@intel.com, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On Thu, 22 May 2014 08:33:47 +0100 "Jan Beulich" wrote: > >>> On 22.05.14 at 03:15, wrote: > > On Wed, 21 May 2014 08:59:27 +0100 > > "Jan Beulich" wrote: > >> >>> On 21.05.14 at 01:46, wrote: > >> > On Tue, 20 May 2014 11:33:11 +0100 > >> > "Jan Beulich" wrote: > >> >> >>> On 20.05.14 at 01:51, wrote: > >> >> > @@ -688,10 +748,10 @@ ept_set_entry(struct p2m_domain *p2m, > >> >> > unsigned long gfn, mfn_t mfn, > >> >> > ept_p2m_type_to_flags(&new_entry, p2mt, p2ma); } > >> >> > > >> >> > - atomic_write_ept_entry(ept_entry, new_entry); > >> >> > + rc = atomic_write_ept_entry(ept_entry, new_entry, > >> >> > target); > >> >> > >> >> To me it would seem cleaner to clear old_entry here right away, > >> >> so there's no confusion about it needing freeing on eventual > >> >> new error paths getting added in the future. > >> > > >> > Not sure I understand why. If the error happens only before the > >> > entry is ever written, leaving the old entry seems reasonable. > >> > IOW, if going from A to B, if there's an error, nothing is > >> > changed, A is still around. Clearing the old entry may make > >> > things worse, specially if clearing the entry needs any special > >> > handling, like clearing old refcnt etc.. Having an api change > >> > state from A to C when failing to set to B seems odd to me. > >> > >> You're right with what you state, yet you apparently didn't spot > >> that I talked about "old_entry", since all your response refers to > >> "old entry". I.e. I was asking for the local variable to be > >> cleared right away, not an entry in the active table. > > > > Sorry, my eyes missed the underscore between old and entry... > > According to the comment, it needs to be done after sync domain... > > I'd rather just leave it as is for now, I'm unable to anticipate > > how the function might change in future. > > I think you still didn't understand what I would like to be done: > Rather than adding an rc check to where old_entry is being > checked for presence (to determine whether to call > ept_free_entry() - that check is pointless anyway) I'd like you to > zap old_entry (read: old_entry.epte = 0) right away when the > above atomic_write_ept_entry() fails. > > Jan > Got it. V15 with that change. thanks, mukesh