[Xen-devel,v2,22/24] xen/arm: mm: Embed permission in the flags

Message ID 20170912100330.2168-23-julien.grall@arm.com
State New
Headers show
Series
  • xen/arm: Memory subsystem clean-up
Related show

Commit Message

Julien Grall Sept. 12, 2017, 10:03 a.m.
Currently, it is not possible to specify the permission of a new
mapping. It would be necessary to use the function modify_xen_mappings
with a different set of flags.

Introduce a couple of new flags for the permissions (Non-eXecutable,
Read-Only) and also provides definition that combine the memory attribute
and permission for common combinations.

PAGE_HYPERVISOR is now an alias to PAGE_HYPERVISOR_RW (read-write,
non-executable mappings). This does not affect the current mapping using
PAGE_HYPERVISOR because Xen is currently forcing all the mapping to be
non-executable by default (see mfn_to_xen_entry).

A follow-up patch will change modify_xen_mappings to use the new flags.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

    Changes in v2:
        - Update the commit message
---
 xen/include/asm-arm/page.h | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Comments

Stefano Stabellini Sept. 19, 2017, 11:59 p.m. | #1
On Tue, 12 Sep 2017, Julien Grall wrote:
> Currently, it is not possible to specify the permission of a new
> mapping. It would be necessary to use the function modify_xen_mappings
> with a different set of flags.
> 
> Introduce a couple of new flags for the permissions (Non-eXecutable,
> Read-Only) and also provides definition that combine the memory attribute
> and permission for common combinations.
> 
> PAGE_HYPERVISOR is now an alias to PAGE_HYPERVISOR_RW (read-write,
> non-executable mappings). This does not affect the current mapping using
> PAGE_HYPERVISOR because Xen is currently forcing all the mapping to be
> non-executable by default (see mfn_to_xen_entry).
> 
> A follow-up patch will change modify_xen_mappings to use the new flags.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> 
>     Changes in v2:
>         - Update the commit message
> ---
>  xen/include/asm-arm/page.h | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 4022b7dc33..814ed126ec 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -66,12 +66,28 @@
>   * Layout of the flags used for updating the hypervisor page tables
>   *
>   * [0:2] Memory Attribute Index
> + * [3:4] Permission flags
>   */
>  #define PAGE_AI_MASK(x) ((x) & 0x7U)
>  
> -#define PAGE_HYPERVISOR         (MT_NORMAL)
> -#define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
> -#define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
> +#define _PAGE_XN_BIT    3
> +#define _PAGE_RO_BIT    4
> +#define _PAGE_XN    (1U << _PAGE_XN_BIT)
> +#define _PAGE_RO    (1U << _PAGE_RO_BIT)
> +#define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
> +#define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)
> +
> +/* Device memory will always be mapped read-write non-executable. */
> +#define _PAGE_DEVICE    _PAGE_XN
> +#define _PAGE_NORMAL    MT_NORMAL

I think I understand the intent behind these two definitions, but I find
them more confusing then useful. Specifically, I find confusing that
_PAGE_DEVICE specifies permissions but not memory attributes, while
_PAGE_NORMAL specifies memory attributes but not permissions.

I would probably remove the two definitions completely and only retain
the useful comment above them. The patch looks good aside from this
nit.



> +#define PAGE_HYPERVISOR_RO      (_PAGE_NORMAL|_PAGE_RO|_PAGE_XN)
> +#define PAGE_HYPERVISOR_RX      (_PAGE_NORMAL|_PAGE_RO)
> +#define PAGE_HYPERVISOR_RW      (_PAGE_NORMAL|_PAGE_XN)
> +
> +#define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
> +#define PAGE_HYPERVISOR_NOCACHE (_PAGE_DEVICE|MT_DEVICE_nGnRE)
> +#define PAGE_HYPERVISOR_WC      (_PAGE_DEVICE|MT_NORMAL_NC)
>  
>  /*
>   * Defines for changing the hypervisor PTE .ro and .nx bits. This is only to be
Julien Grall Sept. 20, 2017, 6:03 p.m. | #2
Hi,

On 20/09/17 00:59, Stefano Stabellini wrote:
> On Tue, 12 Sep 2017, Julien Grall wrote:
>> Currently, it is not possible to specify the permission of a new
>> mapping. It would be necessary to use the function modify_xen_mappings
>> with a different set of flags.
>>
>> Introduce a couple of new flags for the permissions (Non-eXecutable,
>> Read-Only) and also provides definition that combine the memory attribute
>> and permission for common combinations.
>>
>> PAGE_HYPERVISOR is now an alias to PAGE_HYPERVISOR_RW (read-write,
>> non-executable mappings). This does not affect the current mapping using
>> PAGE_HYPERVISOR because Xen is currently forcing all the mapping to be
>> non-executable by default (see mfn_to_xen_entry).
>>
>> A follow-up patch will change modify_xen_mappings to use the new flags.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>
>>      Changes in v2:
>>          - Update the commit message
>> ---
>>   xen/include/asm-arm/page.h | 22 +++++++++++++++++++---
>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>> index 4022b7dc33..814ed126ec 100644
>> --- a/xen/include/asm-arm/page.h
>> +++ b/xen/include/asm-arm/page.h
>> @@ -66,12 +66,28 @@
>>    * Layout of the flags used for updating the hypervisor page tables
>>    *
>>    * [0:2] Memory Attribute Index
>> + * [3:4] Permission flags
>>    */
>>   #define PAGE_AI_MASK(x) ((x) & 0x7U)
>>   
>> -#define PAGE_HYPERVISOR         (MT_NORMAL)
>> -#define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
>> -#define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
>> +#define _PAGE_XN_BIT    3
>> +#define _PAGE_RO_BIT    4
>> +#define _PAGE_XN    (1U << _PAGE_XN_BIT)
>> +#define _PAGE_RO    (1U << _PAGE_RO_BIT)
>> +#define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
>> +#define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)
>> +
>> +/* Device memory will always be mapped read-write non-executable. */
>> +#define _PAGE_DEVICE    _PAGE_XN
>> +#define _PAGE_NORMAL    MT_NORMAL
> 
> I think I understand the intent behind these two definitions, but I find
> them more confusing then useful. Specifically, I find confusing that
> _PAGE_DEVICE specifies permissions but not memory attributes, while
> _PAGE_NORMAL specifies memory attributes but not permissions.

Well, it is just contain the common bits for normal memory and device 
memory. They are not related and are not meant to be used outside of 
this file except for very specific use case. Such as you want to 
introduce a new device type and you want to default attributes. Hence 
the prefixed underscore.

Furthermore, it is much easier to reason with _PAGE_DEVICE rather than 
_PAGE_XN. At least you have one place explaining why the mapping is 
non-executable. And also it also extending default attribute more easily.

Cheers,
Stefano Stabellini Sept. 20, 2017, 9:07 p.m. | #3
On Wed, 20 Sep 2017, Julien Grall wrote:
> Hi,
> 
> On 20/09/17 00:59, Stefano Stabellini wrote:
> > On Tue, 12 Sep 2017, Julien Grall wrote:
> > > Currently, it is not possible to specify the permission of a new
> > > mapping. It would be necessary to use the function modify_xen_mappings
> > > with a different set of flags.
> > > 
> > > Introduce a couple of new flags for the permissions (Non-eXecutable,
> > > Read-Only) and also provides definition that combine the memory attribute
> > > and permission for common combinations.
> > > 
> > > PAGE_HYPERVISOR is now an alias to PAGE_HYPERVISOR_RW (read-write,
> > > non-executable mappings). This does not affect the current mapping using
> > > PAGE_HYPERVISOR because Xen is currently forcing all the mapping to be
> > > non-executable by default (see mfn_to_xen_entry).
> > > 
> > > A follow-up patch will change modify_xen_mappings to use the new flags.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > 
> > > ---
> > > 
> > >      Changes in v2:
> > >          - Update the commit message
> > > ---
> > >   xen/include/asm-arm/page.h | 22 +++++++++++++++++++---
> > >   1 file changed, 19 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> > > index 4022b7dc33..814ed126ec 100644
> > > --- a/xen/include/asm-arm/page.h
> > > +++ b/xen/include/asm-arm/page.h
> > > @@ -66,12 +66,28 @@
> > >    * Layout of the flags used for updating the hypervisor page tables
> > >    *
> > >    * [0:2] Memory Attribute Index
> > > + * [3:4] Permission flags
> > >    */
> > >   #define PAGE_AI_MASK(x) ((x) & 0x7U)
> > >   -#define PAGE_HYPERVISOR         (MT_NORMAL)
> > > -#define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
> > > -#define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
> > > +#define _PAGE_XN_BIT    3
> > > +#define _PAGE_RO_BIT    4
> > > +#define _PAGE_XN    (1U << _PAGE_XN_BIT)
> > > +#define _PAGE_RO    (1U << _PAGE_RO_BIT)
> > > +#define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
> > > +#define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)
> > > +
> > > +/* Device memory will always be mapped read-write non-executable. */
> > > +#define _PAGE_DEVICE    _PAGE_XN
> > > +#define _PAGE_NORMAL    MT_NORMAL
> > 
> > I think I understand the intent behind these two definitions, but I find
> > them more confusing then useful. Specifically, I find confusing that
> > _PAGE_DEVICE specifies permissions but not memory attributes, while
> > _PAGE_NORMAL specifies memory attributes but not permissions.
> 
> Well, it is just contain the common bits for normal memory and device memory.
> They are not related and are not meant to be used outside of this file except
> for very specific use case. 

Yes, I think that is key. More below.


> Such as you want to introduce a new device type
> and you want to default attributes. Hence the prefixed underscore.
> 
> Furthermore, it is much easier to reason with _PAGE_DEVICE rather than
> _PAGE_XN. At least you have one place explaining why the mapping is
> non-executable. And also it also extending default attribute more easily.

If they are not mean to be used outside of this file, then I am fine with
them. But please write it in the comment explicitly on top for them.

Something like:

* Convenience #defines to build the PAGE_HYPERVISOR* defines below. Not
* meant to be used outside of this file.
Julien Grall Sept. 20, 2017, 10:13 p.m. | #4
On 20/09/2017 22:07, Stefano Stabellini wrote:
> On Wed, 20 Sep 2017, Julien Grall wrote:
>> Hi,
>>
>> On 20/09/17 00:59, Stefano Stabellini wrote:
>>> On Tue, 12 Sep 2017, Julien Grall wrote:
>>>> Currently, it is not possible to specify the permission of a new
>>>> mapping. It would be necessary to use the function modify_xen_mappings
>>>> with a different set of flags.
>>>>
>>>> Introduce a couple of new flags for the permissions (Non-eXecutable,
>>>> Read-Only) and also provides definition that combine the memory attribute
>>>> and permission for common combinations.
>>>>
>>>> PAGE_HYPERVISOR is now an alias to PAGE_HYPERVISOR_RW (read-write,
>>>> non-executable mappings). This does not affect the current mapping using
>>>> PAGE_HYPERVISOR because Xen is currently forcing all the mapping to be
>>>> non-executable by default (see mfn_to_xen_entry).
>>>>
>>>> A follow-up patch will change modify_xen_mappings to use the new flags.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>
>>>> ---
>>>>
>>>>      Changes in v2:
>>>>          - Update the commit message
>>>> ---
>>>>   xen/include/asm-arm/page.h | 22 +++++++++++++++++++---
>>>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>>>> index 4022b7dc33..814ed126ec 100644
>>>> --- a/xen/include/asm-arm/page.h
>>>> +++ b/xen/include/asm-arm/page.h
>>>> @@ -66,12 +66,28 @@
>>>>    * Layout of the flags used for updating the hypervisor page tables
>>>>    *
>>>>    * [0:2] Memory Attribute Index
>>>> + * [3:4] Permission flags
>>>>    */
>>>>   #define PAGE_AI_MASK(x) ((x) & 0x7U)
>>>>   -#define PAGE_HYPERVISOR         (MT_NORMAL)
>>>> -#define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
>>>> -#define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
>>>> +#define _PAGE_XN_BIT    3
>>>> +#define _PAGE_RO_BIT    4
>>>> +#define _PAGE_XN    (1U << _PAGE_XN_BIT)
>>>> +#define _PAGE_RO    (1U << _PAGE_RO_BIT)
>>>> +#define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
>>>> +#define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)
>>>> +
>>>> +/* Device memory will always be mapped read-write non-executable. */
>>>> +#define _PAGE_DEVICE    _PAGE_XN
>>>> +#define _PAGE_NORMAL    MT_NORMAL
>>>
>>> I think I understand the intent behind these two definitions, but I find
>>> them more confusing then useful. Specifically, I find confusing that
>>> _PAGE_DEVICE specifies permissions but not memory attributes, while
>>> _PAGE_NORMAL specifies memory attributes but not permissions.
>>
>> Well, it is just contain the common bits for normal memory and device memory.
>> They are not related and are not meant to be used outside of this file except
>> for very specific use case.
>
> Yes, I think that is key. More below.
>
>
>> Such as you want to introduce a new device type
>> and you want to default attributes. Hence the prefixed underscore.
>>
>> Furthermore, it is much easier to reason with _PAGE_DEVICE rather than
>> _PAGE_XN. At least you have one place explaining why the mapping is
>> non-executable. And also it also extending default attribute more easily.
>
> If they are not mean to be used outside of this file, then I am fine with
> them. But please write it in the comment explicitly on top for them.
>
> Something like:
>
> * Convenience #defines to build the PAGE_HYPERVISOR* defines below. Not
> * meant to be used outside of this file.

I will add that.

Cheers,

Patch

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 4022b7dc33..814ed126ec 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -66,12 +66,28 @@ 
  * Layout of the flags used for updating the hypervisor page tables
  *
  * [0:2] Memory Attribute Index
+ * [3:4] Permission flags
  */
 #define PAGE_AI_MASK(x) ((x) & 0x7U)
 
-#define PAGE_HYPERVISOR         (MT_NORMAL)
-#define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
-#define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
+#define _PAGE_XN_BIT    3
+#define _PAGE_RO_BIT    4
+#define _PAGE_XN    (1U << _PAGE_XN_BIT)
+#define _PAGE_RO    (1U << _PAGE_RO_BIT)
+#define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
+#define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)
+
+/* Device memory will always be mapped read-write non-executable. */
+#define _PAGE_DEVICE    _PAGE_XN
+#define _PAGE_NORMAL    MT_NORMAL
+
+#define PAGE_HYPERVISOR_RO      (_PAGE_NORMAL|_PAGE_RO|_PAGE_XN)
+#define PAGE_HYPERVISOR_RX      (_PAGE_NORMAL|_PAGE_RO)
+#define PAGE_HYPERVISOR_RW      (_PAGE_NORMAL|_PAGE_XN)
+
+#define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
+#define PAGE_HYPERVISOR_NOCACHE (_PAGE_DEVICE|MT_DEVICE_nGnRE)
+#define PAGE_HYPERVISOR_WC      (_PAGE_DEVICE|MT_NORMAL_NC)
 
 /*
  * Defines for changing the hypervisor PTE .ro and .nx bits. This is only to be