Re: [hybrid]: code review for function mapping pfn to foreign mfn

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

On Thu, 19 Apr 2012 15:15:27 +0100
Tim Deegan <tim@xen.org> wrote:

> At 18:29 -0700 on 13 Apr (1334341792), Mukesh Rathor wrote:
>  - AFAICT you're using set_mmio_p2m_entry and adding a new unmap
>    operation just to avoid having the m2p updated.  Since you can't
> rely on the unmap always happening through the new call (and you don't
>    enforce it anywhere), it would be better to add a new p2m_type
>    just for non-grant foreign mappings.  Then you can gate the m2p
>    updates in the existing code on the map being normal RAM, as is
>    already done for p2m_is_grant().
 
Hi Tim,

The variants of get_page* are confusing me, so wanna double check with 
you.  I should be able to do something like following, right?


/* add frames from foreign domain to current domain physmap. Similar to 
 * XENMEM_add_to_physmap but the frame is foreign being mapped into current,
 * and is not removed from foreign domain.  */
static long noinline _add_foreign_to_pmap_batch(struct xen_add_to_physmap *xpmp)
{
    unsigned long rc, prev_mfn, mfn = 0;
    struct domain *fdom, *currd = current->domain;
    unsigned long fgmfn = xpmp->idx, gpfn = xpmp->gpfn;
    p2m_type_t p2mt;

    if ( (fdom = get_pg_owner(xpmp->foreign_domid)) == NULL ) {
        return -EPERM;
    }

    mfn = mfn_x(gfn_to_mfn_query(p2m_get_hostp2m(fdom), fgmfn, &p2mt));
    if ( !p2m_is_valid(p2mt) ) {
        put_pg_owner(fdom);
        return -EINVAL;
    }
    if ( (rc=get_page_and_type_from_pagenr(mfn, PGT_writable_page,fdom,0,0)) ) {
        put_pg_owner(fdom);
        return rc;
    }

    /* Remove previously mapped page if it was present. */
    prev_mfn = gmfn_to_mfn(currd, gpfn);
    if ( mfn_valid(prev_mfn) )
    {
        if ( is_xen_heap_mfn(prev_mfn) )
            /* Xen heap frames are simply unhooked from this phys slot */
            guest_physmap_remove_page(currd, gpfn, prev_mfn, 0);
        else
            /* Normal domain memory is freed, to avoid leaking memory. */
            guest_remove_page(currd, gpfn);
    }
    
    if (set_foreign_p2m_entry(p2m_get_hostp2m(currd), gpfn, mfn) == 0) {
        printk("guest_physmap_add_page failed. gpfn:%lx mfn:%lx fgmfn:%lx\n", 
             gpfn, mfn, fgmfn);
        rc = -EINVAL;      ??????????????
    }

    put_pg_owner(fdom);
    return rc;
}

/* Returns: True for success. 0 for failure */
int
set_foreign_p2m_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn)
{
    int rc = 0;
    p2m_type_t ot;
    mfn_t omfn;

    if ( !paging_mode_translate(p2m->domain) )
        return 0;

    omfn = gfn_to_mfn_query(p2m, gfn, &ot);
    if (mfn_valid(omfn)) {
        gdprintk(XENLOG_ERR, "Already mapped mfn %lx at gfn:%lx\n", omfn, gfn);
        set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY);
    }

    P2M_DEBUG("set foreign %lx %lx\n", gfn, mfn_x(mfn));
    p2m_lock(p2m);
    rc = set_p2m_entry(p2m, gfn, mfn, 0, p2m_map_foreign, p2m->default_access);
    audit_p2m(p2m, 1);
    p2m_unlock(p2m);
    if ( rc == 0 )
        gdprintk(XENLOG_ERR,
            "set_foreign_p2m_entry: set_p2m_entry failed! gfn:%lx mfn=%08lx\n",
            gfn, mfn_x(gfn_to_mfn(p2m, gfn, &ot)));
    return rc;
}



thanks,
Mukesh