Xen devel - Sep 2010 - xen crash in tmem: checking a xen pfn for domain ownership

Does the construct:

  xen_pfn_t gpfn;
  p2m_type_t t;
  unsigned long mfn;

  mfn = mfn_x(gfn_to_mfn(current->domain, gpfn, &t));
  if (t != p2m_ram_rw || cli_mfn == INVALID_MFN)
      return NULL; /* bad */
  return map_domain_page(mfn)

somehow check to ensure that pfn belongs to current->domain?
(See cli_mfn_to_va() in common/tmem_xen.c.)

If not, is there an easy way to perform that check?
(preferably one that works for both HVM and PV guests)

In debugging a tmem Linux-side guest patch, I discovered
that a bad mfn passed by the guest can crash Xen and
I think this assumption might be the problem.

Thanks,
Dan

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

If you could be doing memory sharing then you might need to use
gfn_to_mfn_unshare()? Otherwise it looks pretty plausible, and that one flaw
is pretty minor as you''re probably not using memshr.

 -- Keir

On 17/09/2010 17:29, "Dan Magenheimer"
<dan.magenheimer@oracle.com> wrote:
> Does the construct:
> 
>   xen_pfn_t gpfn;
>   p2m_type_t t;
>   unsigned long mfn;
> 
>   mfn = mfn_x(gfn_to_mfn(current->domain, gpfn, &t));
>   if (t != p2m_ram_rw || cli_mfn == INVALID_MFN)
>       return NULL; /* bad */
>   return map_domain_page(mfn)
> 
> somehow check to ensure that pfn belongs to current->domain?
> (See cli_mfn_to_va() in common/tmem_xen.c.)
> 
> If not, is there an easy way to perform that check?
> (preferably one that works for both HVM and PV guests)
> 
> In debugging a tmem Linux-side guest patch, I discovered
> that a bad mfn passed by the guest can crash Xen and
> I think this assumption might be the problem.
> 
> Thanks,
> Dan


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

Thanks for the reply, but I''m not sure I understand.

Ignore memory sharing for now...

Are you saying, yes, the ownership check IS performed?
E.g. if gpfn is a random number, NULL will always be
returned (unless of course the random number happens
to be a valid gfn for current->domain)?

Or are you saying its plausible that this IS the problem
(that I am not checking for ownership)?

Now bring memory sharing back in...

Since tmem and memory sharing are supposed to be complementary
(though I don''t think anybody has ever tried using both
together), are you saying I should change this one
call from gfn_to_mfn() to gfn_to_mfn_unshare() for
some reason (e.g. maybe to avoid a race)?  Note
that this code is just getting a virtual address
to copy a page to/from the guest.

Thanks,
Dan
> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: Friday, September 17, 2010 10:35 AM
> To: Dan Magenheimer; Jan Beulich
> Cc: Xen-devel
> Subject: Re: xen crash in tmem: checking a xen pfn for domain ownership
> 
> If you could be doing memory sharing then you might need to use
> gfn_to_mfn_unshare()? Otherwise it looks pretty plausible, and that one
> flaw
> is pretty minor as you''re probably not using memshr.
> 
>  -- Keir
> 
> On 17/09/2010 17:29, "Dan Magenheimer"
<dan.magenheimer@oracle.com>
> wrote:
> 
> > Does the construct:
> >
> >   xen_pfn_t gpfn;
> >   p2m_type_t t;
> >   unsigned long mfn;
> >
> >   mfn = mfn_x(gfn_to_mfn(current->domain, gpfn, &t));
> >   if (t != p2m_ram_rw || cli_mfn == INVALID_MFN)
> >       return NULL; /* bad */
> >   return map_domain_page(mfn)
> >
> > somehow check to ensure that pfn belongs to current->domain?
> > (See cli_mfn_to_va() in common/tmem_xen.c.)
> >
> > If not, is there an easy way to perform that check?
> > (preferably one that works for both HVM and PV guests)
> >
> > In debugging a tmem Linux-side guest patch, I discovered
> > that a bad mfn passed by the guest can crash Xen and
> > I think this assumption might be the problem.
> >
> > Thanks,
> > Dan
> 
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Gfn_to_mfn() takes a domain as a parameter. It looks up gfn in that
domain''s
p2m. The only RAM-typed pfns that can be present in a domain''s p2m, if
it is
not sharing pages via memshr, are the domain''s own pages. As far as I
know,
at least. It does no harm for you to switch to gfn_to_mfn_unshare(), but I
doubt this is the fix for your current problem.

 -- Keir

On 17/09/2010 17:48, "Dan Magenheimer"
<dan.magenheimer@oracle.com> wrote:
> Thanks for the reply, but I''m not sure I understand.
> 
> Ignore memory sharing for now...
> 
> Are you saying, yes, the ownership check IS performed?
> E.g. if gpfn is a random number, NULL will always be
> returned (unless of course the random number happens
> to be a valid gfn for current->domain)?
> 
> Or are you saying its plausible that this IS the problem
> (that I am not checking for ownership)?
> 
> Now bring memory sharing back in...
> 
> Since tmem and memory sharing are supposed to be complementary
> (though I don''t think anybody has ever tried using both
> together), are you saying I should change this one
> call from gfn_to_mfn() to gfn_to_mfn_unshare() for
> some reason (e.g. maybe to avoid a race)?  Note
> that this code is just getting a virtual address
> to copy a page to/from the guest.
> 
> Thanks,
> Dan
> 
>> -----Original Message-----
>> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>> Sent: Friday, September 17, 2010 10:35 AM
>> To: Dan Magenheimer; Jan Beulich
>> Cc: Xen-devel
>> Subject: Re: xen crash in tmem: checking a xen pfn for domain ownership
>> 
>> If you could be doing memory sharing then you might need to use
>> gfn_to_mfn_unshare()? Otherwise it looks pretty plausible, and that one
>> flaw
>> is pretty minor as you''re probably not using memshr.
>> 
>>  -- Keir
>> 
>> On 17/09/2010 17:29, "Dan Magenheimer"
<dan.magenheimer@oracle.com>
>> wrote:
>> 
>>> Does the construct:
>>> 
>>>   xen_pfn_t gpfn;
>>>   p2m_type_t t;
>>>   unsigned long mfn;
>>> 
>>>   mfn = mfn_x(gfn_to_mfn(current->domain, gpfn, &t));
>>>   if (t != p2m_ram_rw || cli_mfn == INVALID_MFN)
>>>       return NULL; /* bad */
>>>   return map_domain_page(mfn)
>>> 
>>> somehow check to ensure that pfn belongs to current->domain?
>>> (See cli_mfn_to_va() in common/tmem_xen.c.)
>>> 
>>> If not, is there an easy way to perform that check?
>>> (preferably one that works for both HVM and PV guests)
>>> 
>>> In debugging a tmem Linux-side guest patch, I discovered
>>> that a bad mfn passed by the guest can crash Xen and
>>> I think this assumption might be the problem.
>>> 
>>> Thanks,
>>> Dan
>> 
>> 


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

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

 -- Keir


On 17/09/2010 18:25, "Keir Fraser" <keir.fraser@eu.citrix.com>
wrote:
> Gfn_to_mfn() takes a domain as a parameter. It looks up gfn in that
domain''s
> p2m. The only RAM-typed pfns that can be present in a domain''s
p2m, if it is
> not sharing pages via memshr, are the domain''s own pages. As far
as I know,
> at least. It does no harm for you to switch to gfn_to_mfn_unshare(), but I
> doubt this is the fix for your current problem.
> 
>  -- Keir
> 
> On 17/09/2010 17:48, "Dan Magenheimer"
<dan.magenheimer@oracle.com> wrote:
> 
>> Thanks for the reply, but I''m not sure I understand.
>> 
>> Ignore memory sharing for now...
>> 
>> Are you saying, yes, the ownership check IS performed?
>> E.g. if gpfn is a random number, NULL will always be
>> returned (unless of course the random number happens
>> to be a valid gfn for current->domain)?
>> 
>> Or are you saying its plausible that this IS the problem
>> (that I am not checking for ownership)?
>> 
>> Now bring memory sharing back in...
>> 
>> Since tmem and memory sharing are supposed to be complementary
>> (though I don''t think anybody has ever tried using both
>> together), are you saying I should change this one
>> call from gfn_to_mfn() to gfn_to_mfn_unshare() for
>> some reason (e.g. maybe to avoid a race)?  Note
>> that this code is just getting a virtual address
>> to copy a page to/from the guest.
>> 
>> Thanks,
>> Dan
>> 
>>> -----Original Message-----
>>> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>>> Sent: Friday, September 17, 2010 10:35 AM
>>> To: Dan Magenheimer; Jan Beulich
>>> Cc: Xen-devel
>>> Subject: Re: xen crash in tmem: checking a xen pfn for domain
ownership
>>> 
>>> If you could be doing memory sharing then you might need to use
>>> gfn_to_mfn_unshare()? Otherwise it looks pretty plausible, and that
one
>>> flaw
>>> is pretty minor as you''re probably not using memshr.
>>> 
>>>  -- Keir
>>> 
>>> On 17/09/2010 17:29, "Dan Magenheimer"
<dan.magenheimer@oracle.com>
>>> wrote:
>>> 
>>>> Does the construct:
>>>> 
>>>>   xen_pfn_t gpfn;
>>>>   p2m_type_t t;
>>>>   unsigned long mfn;
>>>> 
>>>>   mfn = mfn_x(gfn_to_mfn(current->domain, gpfn, &t));
>>>>   if (t != p2m_ram_rw || cli_mfn == INVALID_MFN)
>>>>       return NULL; /* bad */
>>>>   return map_domain_page(mfn)
>>>> 
>>>> somehow check to ensure that pfn belongs to current->domain?
>>>> (See cli_mfn_to_va() in common/tmem_xen.c.)
>>>> 
>>>> If not, is there an easy way to perform that check?
>>>> (preferably one that works for both HVM and PV guests)
>>>> 
>>>> In debugging a tmem Linux-side guest patch, I discovered
>>>> that a bad mfn passed by the guest can crash Xen and
>>>> I think this assumption might be the problem.
>>>> 
>>>> Thanks,
>>>> Dan
>>> 
>>> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


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

>>> On 17.09.10 at 21:32, Keir Fraser <keir.fraser@eu.citrix.com>
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.

Jan


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

On 20/09/2010 08:16, "Jan Beulich" <JBeulich@novell.com> wrote:
>>>> On 17.09.10 at 21:32, Keir Fraser
<keir.fraser@eu.citrix.com> 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.
Yes, I actually forgot about the PV case, and I think that''s the only
case
that matters for tmem. :-)

 -- Keir



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

> On 20/09/2010 08:16, "Jan Beulich" <JBeulich@novell.com>
wrote:
> 
> >>>> On 17.09.10 at 21:32, Keir Fraser
<keir.fraser@eu.citrix.com>
> 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@lists.xensource.com
http://lists.xensource.com/xen-devel

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@novell.com> wrote:
> >
> > >>>> On 17.09.10 at 21:32, Keir Fraser
<keir.fraser@eu.citrix.com>
> > 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@lists.xensource.com
http://lists.xensource.com/xen-devel

> 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@novell.com> wrote:
> > >
> > > >>>> On 17.09.10 at 21:32, Keir Fraser
<keir.fraser@eu.citrix.com>
> > > 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@lists.xensource.com
http://lists.xensource.com/xen-devel

On 21/09/2010 00:03, "Dan Magenheimer"
<dan.magenheimer@oracle.com> 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@lists.xensource.com
http://lists.xensource.com/xen-devel

> 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@eu.citrix.com]
> 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@oracle.com>
> 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@lists.xensource.com
http://lists.xensource.com/xen-devel