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: Tue, 17 Apr 2012 10:05:28 +0100 Message-ID: <1334653528.14560.261.camel@zakaz.uk.xensource.com> References: <20120413182952.504e2775@mantra.us.oracle.com> <1334584402.14560.184.camel@zakaz.uk.xensource.com> <20120416185340.72ef5566@mantra.us.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20120416185340.72ef5566@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 Tue, 2012-04-17 at 02:53 +0100, Mukesh Rathor wrote: > On Mon, 16 Apr 2012 14:53:22 +0100 > Ian Campbell wrote: > > > > Similar to > > > * XENMEM_add_to_physmap > > > > Why a whole new hypercall rather than a new XENMAPSPACE for the > > exiting XENMEM_add_to_physmap. > > > Ideally we'd have the definition of this (or the equivalent mod to the > > XENMEM_add_to_physmap associated struct) for context, but I can > > probably guess what the content looks like. > > Not a new hcall, just a new subcall. 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 */ #define XENMAPIDX_grant_table_status 0x80000000 Effectively stealing the (currently always zero) top 16 bits of the space member for the foreign domid. > Forgot to include the struct: > > #define XENMEM_add_foreign_to_pmap_batch 19 > struct xen_add_to_foreign_pmap_batch { > domid_t foreign_domid; /* IN: gmfn belongs to this domain */ > int count; /* IN/OUT: number of contigous > frames */ unsigned long gpfn; /* IN: pfn in the current > domain */ unsigned long gmfn; /* IN: from foreign domain */ > int fpmap_flags; /* future use */ > }; > typedef struct xen_add_to_foreign_pmap_batch > xen_add_to_foreign_pmap_batch_t; > DEFINE_GUEST_HANDLE_STRUCT(xen_add_to_foreign_pmap_batch_t); > > > > > rc = set_mmio_p2m_entry(p2m_get_hostp2m(currd), gpfn, mfn); > > > > This ends up setting the page type to p2m_mmio_direct, which doesn't > > seem likely to be correct. Perhaps you should be calling > > set_p2m_entry()? Or adding a set_ram_p2m_entry which does similar > > checks etc to set_mmio_p2m_entry (or maybe you could abstract out > > some generic bits there)? > > well, set_mmio_p2m_entry() calls set_p2m_entry() with a couple checks. > I can add those to my function and just call set_p2m_entry too. It says > mmio, but doesn't seem to do anything mmio specific. If that's the case perhaps you could rename it and add the type as a parameter? If not then I wouldn't add those checks to your function -- create a new wrapper (set_ram_p2m_entry or whatever) with the checks. Ian.