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

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

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:
> > +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?

In the interest of saving time, are you looking-for/ok-with 
something like:

    unsigned int operm = entryptr->epte & 0x7, nperm = new.epte & 0x7;
    bool_t skip_foreign = (new.mfn == entryptr->mfn &&
                           new.sa_p2mt == entryptr->sa_p2mt &&
                           operm == nperm);
...
    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) ||  !is_epte_present(&new) ) <====
            goto out;
..

thereby just rejecting non-present for foreign types. 

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


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


thanks
mukesh