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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

> +{
> +    unsigned long oldmfn = 0;
> +
> +    if ( p2m_is_foreign(new->sa_p2mt) )

And you clearly want to wrap this in unlikely().

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

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

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

> @@ -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
an immediate way to integrate this into the new model (where type
changes get propagated down the page table hierarchy upon access
rather than when the type change is requested), largely because I'm
afraid the number of foreign mappings that a domain may want to
establish needs to be pretty much unbounded (i.e. can't be tracked
in a range set, linked list, or some such).

Why can't this be done as pages get de-allocated (which is a
preemptable process already)?

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

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

Jan

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