RE: [Xen-devel] Re: xen crash in tmem: checking a xen pfn for domain ownership

[Dan Magenheimer <dan.magenheimer@xxxxxxxxxx>]

Wow, gfn_to_mfn() really is a no-op for the pv case.  I thought
you meant that it didn't do any translation, but it appears it
also doesn't even set the return value to INVALID_MFN, so for pv
I have to add a separate check for mfn_valid() to see if it
is even a valid Xen mfn!  THAT was what was causing the crash.

Also, it appears that gfn_to_mfn_unshare is not suitable for
tmem... it appears the existing code checks for "must_succeed"
but gives no indication of success if must_succeed is NOT set
but mem_sharing_unshare_page fails, correct?  If I do plan to
write to the page but I am willing to accept failure if the
page is shared (since tmem is resilient to this scenario),
it appears I will have to add a new variation of gfn_to_mfn_unshare.
Or maybe I should just overload must_succeed, and if must_succeed
is 2 and unsharing fails, return INVALID_MFN?

P.S. Here's what the relevant tmem code looks like now (below
and attached).  Quick review appreciated.

static inline void *cli_get_page(tmem_cli_mfn_t cmfn, unsigned long *pcli_mfn,
                                 pfp_t **pcli_pfp, bool_t cli_write)
{
    unsigned long xmfn;
    p2m_type_t t;
    struct page_info *page;
    int ret;

    if ( is_hvm_domain(current->domain) )
    {
        xmfn = mfn_x(gfn_to_mfn_unshare(current->domain, cmfn, &t, 2));
        if (t != p2m_ram_rw || xmfn == INVALID_MFN)
            return NULL;
    }
    else
    {
        xmfn = cmfn;
        if (!mfn_valid(xmfn))
            return NULL;
    }
    page = mfn_to_page(xmfn);
    if ( cli_write )
        ret = get_page_and_type(page, current->domain, PGT_writable_page);
    else
        ret = get_page(page, current->domain);
    if ( ret <= 0 )
        return NULL;
    *pcli_mfn = xmfn;
    *pcli_pfp = (pfp_t *)page;
    return map_domain_page(xmfn);
}

static inline void cli_put_page(void *cli_va, pfp_t *cli_pfp,
                                unsigned long cli_mfn, bool_t mark_dirty)
{
    if ( mark_dirty )
        paging_mark_dirty(current->domain,cli_mfn);
    put_page((struct page_info *)cli_pfp);
    unmap_domain_page(cli_va);
}


> -----Original Message-----
> From: Dan Magenheimer
> Sent: Monday, September 20, 2010 8:13 AM
> To: Keir Fraser; Jan Beulich
> Cc: Xen-devel
> Subject: RE: [Xen-devel] Re: xen crash in tmem: checking a xen pfn for
> domain ownership
> 
> > On 20/09/2010 08:16, "Jan Beulich" <JBeulich@xxxxxxxxxx> wrote:
> >
> > >>>> On 17.09.10 at 21:32, Keir Fraser <keir.fraser@xxxxxxxxxxxxx>
> > wrote:
> > >> Oh, depending on what you want to do with the page you may well
> want
> > to
> > >> get_page(current->domain, page). You don't hold a lock on the
> > domain's p2m,
> > >> so page ownerships can change under your feet, and hence getting a
> > reference
> > >> to the page, and checking the page's ownership at the same time,
> > might be
> > >> wise. And if you want to modify the page you should probably use
> > >> get_page_and_type(..., PGT_writable_page).
> > >
> > > That's particularly important for the pv case, where gfn_to_mfn()
> is
> > > a no-op.
> 
> Thanks, Jan and Keir, I'll take a look at it.
> 
> > Yes, I actually forgot about the PV case, and I think that's the only
> > case
> > that matters for tmem. :-)
> 
> Tmem works for HVM guests now too (on top of Stefano's PV-on-HVM
> Linux kernel patches), though I haven't tested it in awhile.

Attachment: tmem_xen.c
Description: Binary data

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel