From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [hybrid]: code review for function mapping pfn to foreign mfn Date: Thu, 19 Apr 2012 08:22:12 +0100 Message-ID: <1334820132.11493.51.camel@dagon.hellion.org.uk> References: <20120413182952.504e2775@mantra.us.oracle.com> <1334584402.14560.184.camel@zakaz.uk.xensource.com> <20120416185340.72ef5566@mantra.us.oracle.com> <1334653528.14560.261.camel@zakaz.uk.xensource.com> <20120418162908.2b790fae@mantra.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20120418162908.2b790fae@mantra.us.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 Cc: "Xen-devel@lists.xensource.com" , "Tim (Xen.org)" , Keir Fraser , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On Thu, 2012-04-19 at 00:29 +0100, Mukesh Rathor wrote: > On Tue, 17 Apr 2012 10:05:28 +0100 > Ian Campbell wrote: > > > On Tue, 2012-04-17 at 02:53 +0100, Mukesh Rathor wrote: > > > On Mon, 16 Apr 2012 14:53:22 +0100 > > > Ian Campbell wrote: > > > > Sorry, I meant why a whole new subcall instead of a new > > XENMAPSPACE ;-) > > > > e.g. On ARM I did: > > > > diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h > > index 86d02c8..b2adfbe 100644 > > --- a/xen/include/public/memory.h > > +++ b/xen/include/public/memory.h > > @@ -212,11 +212,13 @@ struct xen_add_to_physmap { > > uint16_t size; > > > > /* Source mapping space. */ > > -#define XENMAPSPACE_shared_info 0 /* shared info page */ > > -#define XENMAPSPACE_grant_table 1 /* grant table page */ > > -#define XENMAPSPACE_gmfn 2 /* GMFN */ > > -#define XENMAPSPACE_gmfn_range 3 /* GMFN range */ > > - unsigned int space; > > +#define XENMAPSPACE_shared_info 0 /* shared info page */ > > +#define XENMAPSPACE_grant_table 1 /* grant table page */ > > +#define XENMAPSPACE_gmfn 2 /* GMFN */ > > +#define XENMAPSPACE_gmfn_range 3 /* GMFN range */ > > +#define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another guest */ > > + uint16_t space; > > + domid_t foreign_domid; /* IFF gmfn_foreign */ > > > Well, for several reasons, I didn't use it, mainly, it doesn't allow > for count. So requests have to come in one frame at a time. I've got it that way on ARM too at the moment but I was planning to make it behave like gmfn_range and use the size parameter to map multiple at a time (unless I've misunderstood what the gmfn_range variant does). > Second, none of the common code can be used by my new request, I don't think that's an impediment to the API, XENMAPSPACE_gmfn_foreign is in pretty much the same position. > because: > - frame is not removed from foreign domain in my case > - i don't want to update the m2p with new info. > > Anyways, I put it there for now. With ballooning change in dom0, I'm > now doing the hcall one frame at a time anyways. We can always enhance > in the future. > > case XENMAPSPACE_gmfn_foreign: > { > rc = _add_foreign_to_pmap_batch(&xatp); > rcu_unlock_domain(d); > return rc; > } > > > >> static long noinline _rem_foreign_pmap_batch(XEN_GUEST_HANDLE(void) > >> arg) > > >Can't XENMEM_remove_from_physmap be used here? > > Well, that calls guest_physmap_remove_page() which calls > p2m_remove_page which updates the M2P with INVALID_M2P_ENTRY. Whereas, > i just need to remove from the dom0 p2m and leave M2P as is (mfn to > domU gmfn). I could add a flag to the struct causing it to just call > set_p2m_entry() directly? Would it be useful to track the fact that a p2m entry is foreign somewhere? That would let you do the appropriate type of teardown etc without relying on the tools to get it right. Are there s/w bits available in the p2m entry itself on x86 or do we use them all already? Ian.