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

[Dan Magenheimer <dan.magenheimer@xxxxxxxxxx>]

> Rest of the code looks fine.

Except for a couple of bugs that took awhile to track down :-(

First (stupid one), get_page returns 1 for success and 0 for failure
and I was testing for failure by <= 0 (the Linux convention, oops).

Second, and much more subtle, apparently get_page_and_type
must always be bracketed with a put_page_and_type, and get_page
must always be bracketed with a put_page, i.e. these can't
be mixed.  It appears that the "and_type" versions take
two references, not one so mixing them results in bad
non-obvious BUGs when pages are cycled through the heap.

> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@xxxxxxxxxxxxx]
> Sent: Tuesday, September 21, 2010 1:44 AM
> To: Dan Magenheimer; Jan Beulich
> Cc: Xen-devel
> Subject: Re: [Xen-devel] Re: xen crash in tmem: checking a xen pfn for
> domain ownership
> 
> On 21/09/2010 00:03, "Dan Magenheimer" <dan.magenheimer@xxxxxxxxxx>
> wrote:
> 
> >     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;
> >     }
> 
> This is needlessly cumbersome. You can do it without the if(is_hvm):
> 
>     xmfn = mfn_x(gfn_to_mfn(current->domain, cmfn, &t);
>     if ((t != p2m_ram_rw) || !mfn_valid(xmfn))
>         return NULL;
> 
> (Didn't use hfn_to_mfn_unshare as you have decided against it.)
> 
> Rest of the code looks fine.
> 
>  -- Keir
> 
> 

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