Re: [hybrid]: code review for function mapping pfn to foreign mfn

[Ian Campbell <Ian.Campbell@citrix.com>]

On Tue, 2012-04-17 at 02:53 +0100, Mukesh Rathor wrote:
> On Mon, 16 Apr 2012 14:53:22 +0100
> Ian Campbell <Ian.Campbell@citrix.com> 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.