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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.03.14 at 02:39, <mukesh.rathor@xxxxxxxxxx> wrote:
> +static inline void atomic_write_ept_entry(ept_entry_t *entryptr,
> +                                          const ept_entry_t *new)
> +{
> +    unsigned long oldmfn = 0;
> +
> +    if ( p2m_is_foreign(new->sa_p2mt) )
> +    {
> +        struct page_info *page;
> +        struct domain *fdom;
> +
> +        ASSERT(mfn_valid(new->mfn));
> +        page = mfn_to_page(new->mfn);
> +        fdom = page_get_owner(page);
> +        get_page(page, fdom);

I'm afraid the checking here is too weak, or at least inconsistent
(considering the ASSERT()): mfn_valid() isn't a sufficient condition
for page_get_owner() to return non-NULL or get_page() to
succeed. If all callers are supposed to guarantee this, then further
ASSERT()s should be added here. If not, error checking is going to
be necessary (and possibly the ASSERT() then also needs to be
converted to an error check).

I also wonder whether page_get_owner_and_reference() wouldn't
be more appropriate to be used here.

> @@ -275,14 +276,19 @@ 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))
> +        if ( (p2m_is_ram(*t) || p2m_is_grant(*t) || p2m_is_foreign(*t) )

Would this perhaps better become something like p2m_is_any_ram()?
(I'm in fact surprised this is only needed in exactly one place.)

> +int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
> +                    unsigned long gpfn, domid_t fdid)
> +{
> +    int rc = -ESRCH;
> +    p2m_type_t p2mt, p2mt_prev;
> +    unsigned long prev_mfn, mfn;
> +    struct page_info *page = NULL;
> +    struct domain *fdom = NULL;
> +
> +    /* xentrace mapping pages from xen heap are owned by DOMID_XEN */
> +    if ( (fdid == DOMID_XEN && (fdom = rcu_lock_domain(dom_xen)) == NULL) ||
> +         (fdid != DOMID_XEN && (fdom = rcu_lock_domain_by_id(fdid)) == NULL) 
> )
> +        goto out;

Didn't you mean to call get_pg_owner() here, at once taking care of
DOMID_IO too? Also I don't think the reference to xentrace is really
useful here - DOMID_XEN owned pages certainly exist elsewhere too.

Of course the question is whether for the purposes you have here
DOMID_XEN/DOMID_IO owned pages are actually validly making it
here.

> +
> +    if ( (rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom)) )
> +        goto out;
> +
> +    rc = -EINVAL;
> +    if ( tdom == fdom || !tdom || !fdom || !is_pvh_domain(tdom) )
> +        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) )
> +    {
> +        if ( page )
> +            put_page(page);
> +        goto out;
> +    }
> +    mfn = mfn_x(page_to_mfn(page));
> +
> +    /* Remove previously mapped page if it is present. */
> +    prev_mfn = mfn_x(get_gfn(tdom, gpfn, &p2mt_prev));
> +    if ( mfn_valid(_mfn(prev_mfn)) )
> +    {
> +        if ( is_xen_heap_mfn(prev_mfn) )
> +            /* Xen heap frames are simply unhooked from this phys slot */
> +            guest_physmap_remove_page(tdom, gpfn, prev_mfn, 0);
> +        else
> +            /* Normal domain memory is freed, to avoid leaking memory. */
> +            guest_remove_page(tdom, gpfn);
> +    }
> +    /*
> +     * Create the new mapping. Can't use guest_physmap_add_page() because it
> +     * will update the m2p table which will result in  mfn -> gpfn of dom0
> +     * and not fgfn of domU.
> +     */
> +    if ( set_foreign_p2m_entry(tdom, gpfn, _mfn(mfn)) == 0 )
> +        gdprintk(XENLOG_WARNING, "set_foreign_p2m_entry failed. "
> +                 "gpfn:%lx mfn:%lx fgfn:%lx td:%d fd:%d\n",
> +                 gpfn, mfn, fgfn, tdom->domain_id, fdom->domain_id);
> +    else
> +        rc = 0;

Can't you make set_foreign_p2m_entry() return a proper error code
(if it doesn't already) and use that here instead of the relatively
meaningless -EINVAL inherited from above (I suppose the function
could e.g. also return -ENOMEM)?

> +
> +    put_page(page);
> +
> +    /*
> +     * We must do this put_gfn after set_foreign_p2m_entry so another cpu
> +     * doesn't populate the gpfn before us.
> +     */
> +    put_gfn(tdom, gpfn);

Might be worth mentioning in the comment which get_...() this pairs
with, as that's not obvious from the code elsewhere in the function
(or the patch as a whole).

Jan

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