From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [V8 PATCH 3/8] pvh dom0: Introduce p2m_map_foreign Date: Thu, 27 Mar 2014 12:29:03 +0000 Message-ID: <5334198F.9050907@eu.citrix.com> References: <1395452357-1598-1-git-send-email-mukesh.rathor@oracle.com> <1395452357-1598-4-git-send-email-mukesh.rathor@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WT9Qx-0008Ku-Li for xen-devel@lists.xenproject.org; Thu, 27 Mar 2014 12:29:23 +0000 In-Reply-To: <1395452357-1598-4-git-send-email-mukesh.rathor@oracle.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: Mukesh Rathor , xen-devel@lists.xenproject.org Cc: keir.xen@gmail.com, tim@xen.org, JBeulich@suse.com List-Id: xen-devel@lists.xenproject.org On 03/22/2014 01:39 AM, Mukesh Rathor wrote: > In this patch, a new type p2m_map_foreign is introduced for pages > that toolstack on an auto translated dom0 or a control domain maps > from foreign domains that its creating or supporting during it's > run time. > > Signed-off-by: Mukesh Rathor > Acked-by: Tim Deegan Overall looks good, just a couple of comments: > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index c0ddef0..7050f6a 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -492,7 +492,7 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn, > for ( i = 0; i < (1UL << page_order); i++ ) > { > mfn_return = p2m->get_entry(p2m, gfn + i, &t, &a, 0, NULL); > - if ( !p2m_is_grant(t) && !p2m_is_shared(t) ) > + if ( !p2m_is_grant(t) && !p2m_is_shared(t) && !p2m_is_foreign(t) ) What's the unifying charateristic of these three types? At some point we should come up with a name for it, and make a function like "p2m_is_ram()", which returns true for all three of them. (Not required for acceptance.) > @@ -751,16 +750,31 @@ set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn) > set_gpfn_from_mfn(mfn_x(omfn), INVALID_M2P_ENTRY); > } > > - P2M_DEBUG("set mmio %lx %lx\n", gfn, mfn_x(mfn)); > - rc = set_p2m_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2m_mmio_direct, p2m->default_access); > + P2M_DEBUG("set %d %lx %lx\n", gfn_p2mt, gfn, mfn_x(mfn)); > + rc = set_p2m_entry(p2m, gfn, mfn, PAGE_ORDER_4K, gfn_p2mt, > + p2m->default_access); > gfn_unlock(p2m, gfn, 0); > if ( 0 == rc ) > gdprintk(XENLOG_ERR, > - "set_mmio_p2m_entry: set_p2m_entry failed! mfn=%08lx\n", > + "%s: set_p2m_entry failed! mfn=%08lx\n", __func__, > mfn_x(get_gfn_query_unlocked(p2m->domain, gfn, &ot))); > return rc; > } > > +/* Set foreign mfn in the given guest's p2m table. > + * Returns: True for success. */ > +static int __attribute__((unused)) > +set_foreign_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn) Why "unused"? It appears that tag remains even at the end of the series. -George