Re: [V11 PATCH 3/4] pvh dom0: Add and remove foreign pages

["Roger Pau Monné" <roger.pau@citrix.com>]

On 02/05/14 03:55, Mukesh Rathor wrote:
> +int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
> +                    unsigned long gpfn, domid_t foreigndom)
> +{
> +    p2m_type_t p2mt, p2mt_prev;
> +    unsigned long prev_mfn, mfn;
> +    struct page_info *page;
> +    int rc = -EINVAL;
> +    struct domain *fdom = NULL;
> +
> +    ASSERT(tdom);
> +    if ( foreigndom == DOMID_SELF || !is_pvh_domain(tdom) )
> +        return -EINVAL;
> +
> +    /*
> +     * pvh fixme: until support is added to p2m teardown code to cleanup any
> +     * foreign entries, limit this to hardware domain only.
> +     */
> +    if ( !is_hardware_domain(tdom) )
> +        return -EPERM;
> +
> +    fdom = get_pg_owner(foreigndom);
> +    if ( fdom == NULL )
> +        return -ESRCH;
> +
> +    if ( tdom == fdom )
> +        goto out;
> +
> +    rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom);
> +    if ( rc )
> +        goto out;
> +
> +    /*
> +     * Take a refcnt on the mfn. NB: following supported for foreign mapping:
> +     *     ram_rw | ram_logdirty | ram_ro | paging_out.
> +     */
> +    page = get_page_from_gfn(fdom, fgfn, &p2mt, P2M_ALLOC);
> +    if ( !page ||
> +         !p2m_is_ram(p2mt) || p2m_is_shared(p2mt) || p2m_is_hole(p2mt) )
> +    {
> +        if ( page )
> +            put_page(page);

This is missing setting rc to an error value, something like the
following is needed:

rc = -EINVAL;

Roger.

> +        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.
> +     */
> +    rc = set_foreign_p2m_entry(tdom, gpfn, _mfn(mfn));
> +    if ( rc )
> +        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);
> +
> +    put_page(page);
> +
> +    /*
> +     * This put_gfn for the above get_gfn for prev_mfn.  We must do this
> +     * after set_foreign_p2m_entry so another cpu doesn't populate the gpfn
> +     * before us.
> +     */
> +    put_gfn(tdom, gpfn);
> +
> +out:
> +    if ( fdom )
> +        put_pg_owner(fdom);
> +    return rc;
> +}