| Message ID | 1464176479-14669-2-git-send-email-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
Hi Jan, On 25/05/16 14:12, Jan Beulich wrote: >>>> On 25.05.16 at 13:41, <julien.grall@arm.com> 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,
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index b46e23e..83edfa1 100644 --- 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; + rc = map_dev_mmio_region(d, gpfn, 1, idx); return rc; diff --git a/xen/common/memory.c b/xen/common/memory.c index 644f81a..6bc52ac 100644 --- 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; if ( xatp->space != XENMAPSPACE_gmfn_range ) - return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, + return xenmem_add_to_physmap_one(d, xatp->space, inv, xatp->idx, xatp->gpfn); if ( xatp->size < start ) @@ -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); if ( rc < 0 ) break; diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h index fe52ee1..b023046 100644 --- 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. */ /* Indexes into space being mapped. */ XEN_GUEST_HANDLE(xen_ulong_t) idxs;
The field 'foreign_id' is not used when the space is dev_mmio. As the space is not yet part of the stable ABI, the field is marked as reserved for future use. The value should always be 0, other values will return -ENOSYS. Note that the code would need some rework (such as renaming the field 'foreign_id' to a generic name), however the release of Xen 4.7 is really close. The rework will be done for the next release. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/mm.c | 4 ++++ xen/common/memory.c | 6 ++++-- xen/include/public/memory.h | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-)