Re: [Xen-devel] [V9 PATCH 6/8] pvh dom0: Add and remove foreign pages

[Mukesh Rathor <mukesh.rathor@xxxxxxxxxx>]

On Wed, 16 Apr 2014 17:00:35 +0100
"Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> >>> On 16.04.14 at 02:12, <mukesh.rathor@xxxxxxxxxx> wrote:
> > In this patch, a new function, p2m_add_foreign(), is added
> > to map pages from foreign guest into dom0 for domU creation.
> > Such pages are typed p2m_map_foreign.  Note, it is the nature
> > of such pages that a refcnt is held during their stay in the p2m.
> > The refcnt is added and released in the low level ept function
> > atomic_write_ept_entry for convenience.
> > Also, p2m_teardown is changed to add a call to allow for the
> > cleanup of foreign pages during it's destruction.
> > Finally, get_pg_owner is changed to allow pvh to map foreign
> > mappings, and is made public to be used by p2m_add_foreign().
> 
> Do you really need to do this at this low a layer? I'm afraid that's
> going to be rather limiting when wanting to use the bits used for the
> p2m type for different purposes in intermediate table entries. I.e. I
> think this special casing should only be done for leaf entries, and
> hence at least one layer further up, or introduce something named
> e.g. atomic_write_epte_leaf().

Well, Tim and I went back and forth several times on this over the
last several months (you were cc'd :) ). 

Since a refcnt absolutely needs to be taken and released for such 
pages while they are mapped, the best way to ensure that was to do
it at the lowest level. This was Tim's suggestion, my earlier patches
did at higher levels. At present, there is a single purpose for foreign
types. But future uses, unlikely at this point, can make any relevant
enhancements.

> > @@ -4583,6 +4583,11 @@ int xenmem_add_to_physmap_one(
> >              page = mfn_to_page(mfn);
> >              break;
> >          }
> > +        case XENMAPSPACE_gmfn_foreign:
> > +        {
> > +            rc = p2m_add_foreign(d, idx, gpfn, foreign_domid);
> > +            return rc;
> > +        }
> 
> No pointless braces please.

Often, case statement get sooo long that having braces makes it easier
to find matching code, specially when indentation is weird for case 
statements, and uglier still, switch statements within switch statements 
are allowed. 

But I'll remove it here anyways rather than argue, even tho the prev
case has {}.

 
> > @@ -46,6 +44,33 @@ static inline bool_t is_epte_valid(ept_entry_t
> > *e) return (e->epte != 0 && e->sa_p2mt != p2m_invalid);
> >  }
> >  
> > +static inline void atomic_write_ept_entry(ept_entry_t *entryptr,
> > +                                          const ept_entry_t *new)
> 
> I don't see a reason not to pass new by value. Also I think you would
> want to drop the "inline" considering the size of the function.

ok. I guess entries are unlikely to change u64 size.

> > +{
> > +    unsigned long oldmfn = 0;
> > +
> > +    if ( p2m_is_foreign(new->sa_p2mt) )
> 
> And you clearly want to wrap this in unlikely().

Ok.

> > +    {
> > +        int rc;
> > +        struct page_info *page;
> > +        struct domain *fdom;
> > +
> > +        ASSERT(mfn_valid(new->mfn));
> > +        page = mfn_to_page(new->mfn);
> > +        fdom = page_get_owner(page);
> > +        ASSERT(fdom);
> > +        rc = get_page(page, fdom);
> > +        ASSERT(rc);
> 
> I'm afraid you can't rely on this to succeed: A PV guest could have
> mapped the page so many times that you can't get another ref.

foreign pages are valid for PVH/HVM only, PV would never get here.
I can return rc instead of ASSERT. In the prev version review, you
had asked for an ASSERT here.


> > +    }
> > +    if ( p2m_is_foreign(entryptr->sa_p2mt) )
> > +        oldmfn = entryptr->mfn;
> > +
> > +    write_atomic(&entryptr->epte, new->epte);
> > +
> > +    if ( oldmfn )
> > +        put_page(mfn_to_page(oldmfn));
> 
> Is it perhaps worthwhile avoiding the get/put pair if neither the MFN
> nor the type change?

Such a change is so not allowed at present. 


> > @@ -275,14 +276,18 @@ struct page_info *get_page_from_gfn_p2m(
> >          /* Fast path: look up and get out */
> >          p2m_read_lock(p2m);
> >          mfn = __get_gfn_type_access(p2m, gfn, t, a, 0, NULL, 0);
> > -        if ( (p2m_is_ram(*t) || p2m_is_grant(*t))
> > -             && mfn_valid(mfn)
> > +        if ( p2m_is_any_ram(*t) && mfn_valid(mfn)
> >               && !((q & P2M_UNSHARE) && p2m_is_shared(*t)) )
> >          {
> >              page = mfn_to_page(mfn);
> > -            if ( !get_page(page, d)
> > -                 /* Page could be shared */
> > -                 && !get_page(page, dom_cow) )
> > +            if ( p2m_is_foreign(*t) )
> > +            {
> > +                struct domain *fdom =
> > page_get_owner_and_reference(page);
> > +                ASSERT(fdom && fdom != d);
> 
> Same here regarding the assumption of guaranteed success.

Sure, can change that to return NULL in case of failure.

> > @@ -451,6 +456,10 @@ void p2m_teardown(struct p2m_domain *p2m)
> >      d = p2m->domain;
> >  
> >      p2m_lock(p2m);
> > +    /* pvh: we must release refcnt on all foreign pages */
> > +    if ( is_pvh_domain(d) )
> > +        p2m_change_entry_type_global(d, p2m_map_foreign,
> > p2m_invalid);
> 
> Ah, here it comes. But no, this is a no-go. The function currently is
> running for unbounded time, which I'm about to change (pending
> some more testing; seems to generally work). I can't, however, see

I believe Tim has a patch to make this pre-emptible and OK'd this hook
here. This path for foreign pages is only for destroy of p2m in case 
of domain crash, which at present would only be dom0 (or controlling
domains in future). Normally, these pages are un-mapped by guest via the 
remove from physmap hcall. 

Moreover, the number of foreign pages is relatively small.


> > +int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
> > +                    unsigned long gpfn, domid_t fdid)
> 
> Please call this foreigndom, just like other code does, allowing for
> it to be found by grep-ing the tree.
> 
> > +{
> > +    p2m_type_t p2mt, p2mt_prev;
> > +    unsigned long prev_mfn, mfn;
> > +    struct page_info *page;
> > +    int rc = -EINVAL;
> > +    struct domain *fdom = NULL;
> > +
> > +    if ( fdid == DOMID_SELF )
> > +        goto out;
> > +
> > +    rc = -ESRCH;
> > +    fdom = get_pg_owner(fdid);
> > +    if ( fdom == NULL )
> > +        goto out;
> > +
> > +    rc = -EINVAL;
> > +    if ( tdom == NULL || tdom == fdom || !is_pvh_domain(tdom) )
> 
> Is tdom == NULL a reasonable thing to expect here. I.e. can't you
> rather ASSERT(tdom)?

Could. Why is it a big deal to check for NULL rather than ASSERT?


> > +        goto out;
> > +
> > +    rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom);
> > +    if ( rc )
> > +        goto out;
> > +
> > +    /* following will take a refcnt on the mfn */
> > +    page = get_page_from_gfn(fdom, fgfn, &p2mt, P2M_ALLOC);
> > +    if ( !page || !p2m_is_valid(p2mt) )
> 
> Is this really p2m_is_valid() (i.e. including various types apart from
> p2m_ram_rw)?

Hmm.. let me check again in case of HVM guest coming up on PVH dom0, if
there are use cases of non-ram types. Otherwise, it could be just
p2m_is_ram.

thanks
mukesh


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