[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Jan,

On 25/05/16 14:12, 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.

I will use -EOPNOTSUPP.


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

Ok.


@@ -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.

I will give a look to use an union.

Regards,
--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.