From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [V5 PATCH 6/7] pvh dom0: Add and remove foreign pages Date: Fri, 6 Dec 2013 18:07:07 -0800 Message-ID: <20131206180707.219ff6bd@mantra.us.oracle.com> References: <1386209128-5261-1-git-send-email-mukesh.rathor@oracle.com> <1386209128-5261-7-git-send-email-mukesh.rathor@oracle.com> <1386244824.20047.41.camel@kazak.uk.xensource.com> <20131205171504.033f3604@mantra.us.oracle.com> <20131205173206.5a047de5@mantra.us.oracle.com> <1386325112.6672.15.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1386325112.6672.15.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: george.dunlap@eu.citrix.com, Xen-devel@lists.xensource.com, tim@xen.org, keir.xen@gmail.com, JBeulich@suse.com List-Id: xen-devel@lists.xenproject.org On Fri, 6 Dec 2013 10:18:32 +0000 Ian Campbell 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