| Message ID | 1464176479-14669-2-git-send-email-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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. > --- 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
On Wed, May 25, 2016 at 07:12:30AM -0600, 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. > > > --- 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 >
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(-)