diff mbox

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

Message ID 1464176479-14669-3-git-send-email-julien.grall@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Julien Grall May 25, 2016, 11:41 a.m. UTC
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(-)

Comments

Jan Beulich May 25, 2016, 1:14 p.m. UTC | #1
>>> 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
Julien Grall May 25, 2016, 2:43 p.m. UTC | #2
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,
Jan Beulich May 25, 2016, 2:53 p.m. UTC | #3
>>> 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
Stefano Stabellini May 26, 2016, 9:18 a.m. UTC | #4
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>
Julien Grall May 26, 2016, 9:28 a.m. UTC | #5
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
diff mbox

Patch

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. */
 /* ` } */
 
 /*