This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


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

To: Keir Fraser <keir.fraser@xxxxxxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxxxx>
Subject: RE: [Xen-devel] Re: xen crash in tmem: checking a xen pfn for domain ownership
From: Dan Magenheimer <dan.magenheimer@xxxxxxxxxx>
Date: Mon, 20 Sep 2010 16:03:52 -0700 (PDT)
Cc: Xen-devel <Xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Mon, 20 Sep 2010 16:06:57 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <5c35f0dd-2f25-4843-8b8b-f6c596e7d2c3@default>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <4C9726890200007800017A42@xxxxxxxxxxxxxxxxxx C8BCCFFD.23806%keir.fraser@xxxxxxxxxxxxx 5c35f0dd-2f25-4843-8b8b-f6c596e7d2c3@default>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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;
        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);
        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 )
    put_page((struct page_info *)cli_pfp);

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