Re: [PATCH v4 1/6] xen: improve changes to xen_add_to_physmap

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

> >          mfn = maddr >> PAGE_SHIFT;
> > -	if (x && x--) {
> > -            printk("Mapping dom%d mfn 0x%lx to dom%d mfn 0x%"PRIpaddr"\n",
> > -                   od->domain_id, mfn, d->domain_id, xatp->gpfn);
> > -        }
> 
> why remove the printk here?

It's debug junk which crept in from somewhere else, I'll clean it up in
the original patch which added it before posting this stuff properly.

> > diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> > index b2adfbe..1cc7a80 100644
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -198,6 +198,15 @@ struct xen_machphys_mapping {
> >  typedef struct xen_machphys_mapping xen_machphys_mapping_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
> >  
> > +/* Source mapping space. */
> > +/* ` enum phys_map_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 dom */
> > +/* ` } */
> >  /*
> >   * Sets the GPFN at which a particular page appears in the specified guest's
> >   * pseudophysical address space.
> > @@ -211,26 +220,40 @@ struct xen_add_to_physmap {
> >      /* Number of pages to go through for gmfn_range */
> >      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 */
> > -#define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another guest */
> > -    uint16_t space;
> > +    uint16_t space; /* => enum phys_map_space */
> >      domid_t foreign_domid; /* IFF gmfn_foreign */
> 
> I don't like very much that you have a uint16_t in the struct that
> actually represents a union

Actually, the foreign_domid can go from here, since it is entirely
replaced with the xatp_range stuff. IOW XENMAPSPACE_gmfn_foreign will
only be valid with the add_to_physmap_range version of this interface.

This is just a left over (and it's convenient during the transition
period).

> 
> 
> >  #define XENMAPIDX_grant_table_status 0x80000000
> >  
> > -    /* Index into source mapping space. */
> > +    /* Index into space being mapped. */
> >      xen_ulong_t idx;
> >  
> > -    /* GPFN where the source mapping page should appear. */
> > +    /* GPFN in domid where the source mapping page should appear. */
> >      xen_pfn_t     gpfn;
> >  };
> >  typedef struct xen_add_to_physmap xen_add_to_physmap_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_add_to_physmap_t);
> >  
> > +/* A batched version of add_to_physmap. */
> > +#define XENMEM_add_to_physmap_range 23
> > +struct xen_add_to_physmap_range {
> > +    /* Which domain to change the mapping for. */
> > +    domid_t domid;
> > +    uint16_t space; /* => enum phys_map_space */
> > +
> > +    /* Number of pages to go through */
> > +    uint16_t size;
> > +    domid_t foreign_domid; /* IFF gmfn_foreign */
> > +
> > +    /* Indexes into space being mapped. */
> > +    XEN_GUEST_HANDLE(xen_ulong_t) idxs;
> > +
> > +    /* GPFN in domdwhere the source mapping page should appear. */
>                      ^ domid where

Thanks.

> 
> > +    XEN_GUEST_HANDLE(xen_pfn_t) gpfns;
> > +};
> > +typedef struct xen_add_to_physmap_range xen_add_to_physmap_range_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_add_to_physmap_range_t);
> > +
> >  /*
> >   * Unmaps the page appearing at a particular GPFN from the specified guest's
> >   * pseudophysical address space.
> > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> > index b2f6c50..9425520 100644
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -51,6 +51,7 @@ DEFINE_XEN_GUEST_HANDLE(void);
> >  
> >  DEFINE_XEN_GUEST_HANDLE(uint64_t);
> >  DEFINE_XEN_GUEST_HANDLE(xen_pfn_t);
> > +DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
> >  #endif
> 
> You shouldn't need that: xen_ulong_t is already defined in arch-arm.h
> (and arch-x86/xen.h)

The type is, but the guest handle is not.

Ian.