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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

Jan


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