Re: [Xen-devel] [for-4.7 1/2] xen: XENMEM_add_physmap_batch: Mark 'foreign_id' as reserved for dev_mmio

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, May 25, 2016 at 07:12:30AM -0600, Jan Beulich wrote:
> >>> On 25.05.16 at 13:41, <julien.grall@xxxxxxx> wrote:
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -1143,6 +1143,10 @@ int xenmem_add_to_physmap_one(
> >          break;
> >      }
> >      case XENMAPSPACE_dev_mmio:
> > +        /* The field 'foreign_domid' is reserved for future use */
> > +        if ( foreign_domid )
> > +            return -ENOSYS;
> 
> This should return -EINVAL or maybe -EOPNOTSUPP, but
> definitely not -ENOSYS.
> 
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -639,9 +639,11 @@ static int xenmem_add_to_physmap(struct domain *d,
> >  {
> >      unsigned int done = 0;
> >      long rc = 0;
> > +    /* The field 'foreign_id' should be 0 when mapping MMIO. */
> > +    domid_t inv = (xatp->space != XENMAPSPACE_dev_mmio) ? DOMID_INVALID : 
> > 0;
> 
> This is a bad type for something that now isn't a domain ID anymore.
> Please use u16 or even better unsigned int. Eventually we should
> fix xenmem_add_to_physmap_one()'s respective parameter type
> accordingly.
> 
> Also I think the condition would better be space == gmfn_foreign.
> 
> > @@ -658,7 +660,7 @@ static int xenmem_add_to_physmap(struct domain *d,
> >  
> >      while ( xatp->size > done )
> >      {
> > -        rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID,
> > +        rc = xenmem_add_to_physmap_one(d, xatp->space, inv,
> >                                         xatp->idx, xatp->gpfn);
> 
> This instance you could actually leave alone (as it's dealing with
> XENMAPSPACE_gmfn_range only).
> 
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -259,7 +259,7 @@ struct xen_add_to_physmap_batch {
> >  
> >      /* Number of pages to go through */
> >      uint16_t size;
> > -    domid_t foreign_domid; /* IFF gmfn_foreign */
> > +    domid_t foreign_domid; /* IFF gmfn_foreign. Should be 0 for other 
> > spaces. */
> 
> I wonder whether we shouldn't fix up the structure here right away,
> instead of deferring that to after 4.7. After all, as above, we don't
> really want a domain ID here generally anymore, so this should
> either become "u16 aux" (or some such) or a union (all of course only
> for new enough __XEN_INTERFACE_VERSION__).
> 
> Plus I think we will want this to be IN/OUT, such that if the
> implementation, rather than failing, uses a replacement attribute,
> that could be communicated back. Of course that would matter
> only if we don't go the union route mentioned above.
> 
> Wei, would that be still acceptable for 4.7?
> 

Sure. It's a simple in term of code change and should be fairly easy to
review.

Wei.

> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel