[for-4.7 0/2] xen:

[for-4.7 0/2] xen:

[Julien Grall]

Hello all,

This series is based on the thread [1]. To allow future extension, the new
space dev_mmio should have all unused fields marked as reserved.

The first patch enforces the value of the field 'foreign_id'. The second
document the behavior of dev_mmio for ARM.

This series is candidate for Xen 4.7, the first patch ensure a clean ABI
for the new space which will allow future extension.

Regards,

[1] http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg02328.html

Julien Grall (2):
  xen: XENMEM_add_physmap_batch: Mark 'foreign_id' as reserved for
    dev_mmio
  xen/arm: Document the behavior of XENMAPSPACE_dev_mmio

 xen/arch/arm/mm.c           | 4 ++++
 xen/common/memory.c         | 6 ++++--
 xen/include/public/memory.h | 7 +++++--
 3 files changed, 13 insertions(+), 4 deletions(-)

-- 
1.9.1


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

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

[Julien Grall]

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(-)

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;
-- 
1.9.1


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

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

[Jan Beulich]

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


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

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

[Wei Liu]

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
> 

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

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

[Julien Grall]

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,
-- 
Julien Grall

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

[for-4.7 2/2] xen/arm: Document the behavior of XENMAPSPACE_dev_mmio

[Julien Grall]

Currently, XENMAPSPACE_dev_mmio maps MMIO regions using one of the most
restrictive memory attribute (Device_nGnRE).

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/public/memory.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index b023046..ece72d2 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -220,7 +220,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
 #define XENMAPSPACE_gmfn_range   3 /* GMFN range, XENMEM_add_to_physmap only. */
 #define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another dom,
                                     * XENMEM_add_to_physmap_batch only. */
-#define XENMAPSPACE_dev_mmio     5 /* device mmio region */
+#define XENMAPSPACE_dev_mmio     5 /* device mmio region
+                                      On ARM, the region will be mapped
+                                      in stage-2 using the memory attribute
+                                      Device_nGnRE. */
 /* ` } */
 
 /*
-- 
1.9.1


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

Re: [for-4.7 2/2] xen/arm: Document the behavior of XENMAPSPACE_dev_mmio

[Jan Beulich]

>>> On 25.05.16 at 13:41, <julien.grall@arm.com> wrote:
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -220,7 +220,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
>  #define XENMAPSPACE_gmfn_range   3 /* GMFN range, XENMEM_add_to_physmap only. */
>  #define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another dom,
>                                      * XENMEM_add_to_physmap_batch only. */
> -#define XENMAPSPACE_dev_mmio     5 /* device mmio region */
> +#define XENMAPSPACE_dev_mmio     5 /* device mmio region
> +                                      On ARM, the region will be mapped
> +                                      in stage-2 using the memory attribute
> +                                      Device_nGnRE. */

Since this is unimplemented on x86, may I suggest to make this "ARM
only; the region will be ..."?

Also - is Device_nGnRE a term uniform between ARM32 and ARM64?

Jan


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

Re: [for-4.7 2/2] xen/arm: Document the behavior of XENMAPSPACE_dev_mmio

[Julien Grall]

Hi Jan,

On 25/05/16 14:14, Jan Beulich wrote:
>>>> On 25.05.16 at 13:41, <julien.grall@arm.com> wrote:
>> --- a/xen/include/public/memory.h
>> +++ b/xen/include/public/memory.h
>> @@ -220,7 +220,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
>>   #define XENMAPSPACE_gmfn_range   3 /* GMFN range, XENMEM_add_to_physmap only. */
>>   #define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another dom,
>>                                       * XENMEM_add_to_physmap_batch only. */
>> -#define XENMAPSPACE_dev_mmio     5 /* device mmio region */
>> +#define XENMAPSPACE_dev_mmio     5 /* device mmio region
>> +                                      On ARM, the region will be mapped
>> +                                      in stage-2 using the memory attribute
>> +                                      Device_nGnRE. */
>
> Since this is unimplemented on x86, may I suggest to make this "ARM
> only; the region will be ..."?

Sounds good.

>
> Also - is Device_nGnRE a term uniform between ARM32 and ARM64?

Actually it should be Device-nGnRE to match with the spec. This has been 
introduced on Armv8. Previously for Armv7, the name was "Device", 
although the behavior is exactly the same.

I could say:

"ARM only; the region will be mapped in Stage-2 using the memory 
attribute 'Device-nGnRE' (previously named 'Device' on ARMv7)".

Regards,

-- 
Julien Grall

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

Re: [for-4.7 2/2] xen/arm: Document the behavior of XENMAPSPACE_dev_mmio

[Jan Beulich]

>>> On 25.05.16 at 16:43, <julien.grall@arm.com> wrote:
> I could say:
> 
> "ARM only; the region will be mapped in Stage-2 using the memory 
> attribute 'Device-nGnRE' (previously named 'Device' on ARMv7)".

SGTM

Jan


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

Re: [for-4.7 2/2] xen/arm: Document the behavior of XENMAPSPACE_dev_mmio

[Stefano Stabellini]

On Wed, 25 May 2016, Julien Grall wrote:
> Hi Jan,
> 
> On 25/05/16 14:14, Jan Beulich wrote:
> > > > > On 25.05.16 at 13:41, <julien.grall@arm.com> wrote:
> > > --- a/xen/include/public/memory.h
> > > +++ b/xen/include/public/memory.h
> > > @@ -220,7 +220,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
> > >   #define XENMAPSPACE_gmfn_range   3 /* GMFN range, XENMEM_add_to_physmap
> > > only. */
> > >   #define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another dom,
> > >                                       * XENMEM_add_to_physmap_batch only.
> > > */
> > > -#define XENMAPSPACE_dev_mmio     5 /* device mmio region */
> > > +#define XENMAPSPACE_dev_mmio     5 /* device mmio region
> > > +                                      On ARM, the region will be mapped
> > > +                                      in stage-2 using the memory
> > > attribute
> > > +                                      Device_nGnRE. */
> > 
> > Since this is unimplemented on x86, may I suggest to make this "ARM
> > only; the region will be ..."?
> 
> Sounds good.
> 
> > 
> > Also - is Device_nGnRE a term uniform between ARM32 and ARM64?
> 
> Actually it should be Device-nGnRE to match with the spec. This has been
> introduced on Armv8. Previously for Armv7, the name was "Device", although the
> behavior is exactly the same.
> 
> I could say:
> 
> "ARM only; the region will be mapped in Stage-2 using the memory attribute
> 'Device-nGnRE' (previously named 'Device' on ARMv7)".

Small nit: I would say "the region gets mapped" or "is mapped" rather
than "will be" because that makes me think that it is a future behavior
of following Xen releases rather than the current behavior.

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

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

Re: [for-4.7 2/2] xen/arm: Document the behavior of XENMAPSPACE_dev_mmio

[Julien Grall]

Hi Stefano,

On 26/05/2016 10:18, Stefano Stabellini wrote:
> On Wed, 25 May 2016, Julien Grall wrote:
>> On 25/05/16 14:14, Jan Beulich wrote:
>>> Also - is Device_nGnRE a term uniform between ARM32 and ARM64?
>>
>> Actually it should be Device-nGnRE to match with the spec. This has been
>> introduced on Armv8. Previously for Armv7, the name was "Device", although the
>> behavior is exactly the same.
>>
>> I could say:
>>
>> "ARM only; the region will be mapped in Stage-2 using the memory attribute
>> 'Device-nGnRE' (previously named 'Device' on ARMv7)".
>
> Small nit: I would say "the region gets mapped" or "is mapped" rather
> than "will be" because that makes me think that it is a future behavior
> of following Xen releases rather than the current behavior.

Good idea. I sent a v2 yesterday night [1]. So I will fix in the v3.

> Acked-by: Stefano Stabellini <sstabellini@kernel.org>

[1] http://lists.xen.org/archives/html/xen-devel/2016-05/msg02490.html

-- 
Julien Grall

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

xen: XENMEM_add_physmap_batch: Mark 'foreign_is' as reserved for dev_mmio (WAS Re: [for-4.7 0/2] xen:)

[Julien Grall]

Sorry I forgot to put a proper title.

On 25/05/16 12:41, Julien Grall wrote:
> Hello all,
>
> This series is based on the thread [1]. To allow future extension, the new
> space dev_mmio should have all unused fields marked as reserved.
>
> The first patch enforces the value of the field 'foreign_id'. The second
> document the behavior of dev_mmio for ARM.
>
> This series is candidate for Xen 4.7, the first patch ensure a clean ABI
> for the new space which will allow future extension.
>
> Regards,
>
> [1] http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg02328.html
>
> Julien Grall (2):
>    xen: XENMEM_add_physmap_batch: Mark 'foreign_id' as reserved for
>      dev_mmio
>    xen/arm: Document the behavior of XENMAPSPACE_dev_mmio
>
>   xen/arch/arm/mm.c           | 4 ++++
>   xen/common/memory.c         | 6 ++++--
>   xen/include/public/memory.h | 7 +++++--
>   3 files changed, 13 insertions(+), 4 deletions(-)
>

-- 
Julien Grall

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