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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.05.14 at 01:46, <mukesh.rathor@xxxxxxxxxx> wrote:
> On Tue, 20 May 2014 11:33:11 +0100
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 20.05.14 at 01:51, <mukesh.rathor@xxxxxxxxxx> wrote:
>> > +static int atomic_write_ept_entry(ept_entry_t *entryptr,
>> > ept_entry_t new,
>> > +                                  int level)
>> > +{
>> > +    int rc = 0;
>> > +    unsigned long oldmfn = INVALID_MFN;
>> > +    bool_t skip_foreign = (new.mfn == entryptr->mfn &&
>> > +                           new.sa_p2mt == entryptr->sa_p2mt);
>> > +
>> > +    if ( level )
>> > +    {
>> > +        ASSERT(!is_epte_superpage(&new)
>> > || !p2m_is_foreign(new.sa_p2mt));
>> > +        write_atomic(&entryptr->epte, new.epte);
>> > +        goto out;
>> > +    }
>> > +
>> > +    if ( unlikely(p2m_is_foreign(new.sa_p2mt)) && !skip_foreign )
>> > +    {
>> > +        struct domain *fdom;
>> > +
>> > +        rc = -EINVAL;
>> > +        if ( !mfn_valid(new.mfn) )
>> > +            goto out;
>> > +
>> > +        rc = -ESRCH;
>> > +        fdom = page_get_owner(mfn_to_page(new.mfn));
>> > +        if ( fdom == NULL )
>> > +            goto out;
>> > +
>> > +        /* get refcount on the page */
>> > +        rc = -EBUSY;
>> > +        if ( !get_page(mfn_to_page(new.mfn), fdom) )
>> > +            goto out;
>> > +    }
>> > +
>> > +    if ( unlikely(p2m_is_foreign(entryptr->sa_p2mt))
>> > && !skip_foreign )
>> > +        oldmfn = entryptr->mfn;
>> > +
>> > +    write_atomic(&entryptr->epte, new.epte);
>> > +
>> > +    if ( unlikely(oldmfn != INVALID_MFN) )
>> > +        put_page(mfn_to_page(oldmfn));
>> > +
>> > +    rc = 0;
>> > +
>> > + out:
>> > +    if ( rc )
>> > +        gdprintk(XENLOG_ERR, "epte o:%"PRIx64" n:%"PRIx64"
>> > rc:%d\n",
>> > +                 entryptr->epte, new.epte, rc);
>> > +    return rc;
>> > +}
>> 
>> There's still no sign of any use of is_epte_present() here, and also
>> no mention anywhere that taking refcounts even for inaccessible
>> entries is correct. I think this is actually okay, but the policy
> 
> I thought we now have no paths leading to that!  Can you please point 
> me to a path that this would happen for foreign type?

No I can't - as implied by the rest of the response seen below. _If_
indeed there are no such paths, a respective ASSERT() might be the
way to go.

>> entries is correct. I think this is actually okay, but the policy
>> (refcount taken even for inaccessible pages) should be spelled out
>> somewhere.
> 
> Can you please point to where this is spelled out for grant types?
> They are very similar to foreign types.

Can you please stop using examples of bad situations as justification
for adding further badness? But then again, it's Tim to decide whether
he's fine with all sorts of things being implicit rather than explicit here.

Apart from that the question would need to be raised to those not
having spelled out properly what the rules for grants are -
considering that it's the second switch() in ept_p2m_type_to_flags()
that can actually lead to non-present entries associated with a valid
MFN, it's likely the addition of mem_access to blame here.

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

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel