> 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),
Ignore this part. There IS at least one scenario (persistent
get) where tmem would not be resilient to a failed unshare.
So I think I will skip unshare for now and just submit a patch
for the crash-on-bad-PV-mfn.
> -----Original Message-----
> From: Dan Magenheimer
> Sent: Monday, September 20, 2010 5:04 PM
> To: Keir Fraser; Jan Beulich
> Cc: Xen-devel
> Subject: RE: [Xen-devel] Re: xen crash in tmem: checking a xen pfn for
> domain ownership
>
> 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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home