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

["Jan Beulich" <JBeulich@suse.com>]

>>> 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