Re: [V5 PATCH 6/7] pvh dom0: Add and remove foreign pages

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Fri, 6 Dec 2013 10:18:32 +0000
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Thu, 2013-12-05 at 17:32 -0800, Mukesh Rathor wrote:
> > > > Why is page NULL in this case? I'd have thought that
> > > > get_page_from_gfn could handle the p2m_foreign case internally
> > > > and still return the page, with the ref count taken too.
> > > > 
> > > > Doing that would cause a lot of the magic, and in particular the
> > > > ifdef, in the following code to disappear.
> > > 
> > > I had brought this up earlier this year (that's how old this patch
> > > is). get_page_from_gfn can't be used because the mfn owner is
> > > foreign domain and not domain "d", and get_page() will barf.
> > 
> > Rephrase: get_page_from_gfn can't be changed to handle p2m_foreign
> > because...
> 
> Even with that my original reply stands. In the case of a p2m_foreign
> get_page_from_gfn shouldn't be calling get_page, but should be doing
> the same dance as you are currently doing in this function to get at
> the page's original owner and take whatever ref is needed etc etc
> instead.

Wish we were having this discussion 8 months ago when I brought
this up:

http://lists.xen.org/archives/html/xen-devel/2013-04/msg00492.html


Anyways, one of the issues doing what you say is get_page_from_gfn() 
refcnts the page, where as in my path I'm not refcounting since the 
domain is already holding one from xenmem_add_foreign_to_p2m(). It 
would be confusing for it to be doing different things for different
types. So, it should refcnt foreign also, and that would mean a direct
call to page_get_owner_and_reference() in get_page_from_gfn_p2m. However,
the "if ( p2m_is_foreign()) put_page()" will still have to be done
in the "case XENMEM_remove_from_physmap" caller path.

In any case, IMO, any changes around get_page* are too intrusive at 
this point, and we should come back to it start of 4.5.

I made the code symmetrical like you said in different thread, take
a look at my patch in the V6 thread. I think you'll find that acceptable.

thanks
mukesh