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

[Dan Magenheimer <dan.magenheimer@xxxxxxxxxx>]

> 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