Re: [V13 PATCH 1/2] pvh dom0: Add and remove foreign pages

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Thu, 22 May 2014 08:33:47 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 22.05.14 at 03:15, <mukesh.rathor@oracle.com> wrote:
> > On Wed, 21 May 2014 08:59:27 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> >> >>> On 21.05.14 at 01:46, <mukesh.rathor@oracle.com> wrote:
> >> > On Tue, 20 May 2014 11:33:11 +0100
> >> > "Jan Beulich" <JBeulich@suse.com> wrote:
> >> >> >>> On 20.05.14 at 01:51, <mukesh.rathor@oracle.com> 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