[Xen-devel,25/27] xen/arm: mm: Embed permission in the flags

Message ID 20170814142418.13267-26-julien.grall@arm.com
State Superseded
Headers show
Series
  • xen/arm: Memory subsystem clean-up
Related show

Commit Message

Julien Grall Aug. 14, 2017, 2:24 p.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.

Add introduce a couple of new flags for the permissions (Non-eXecutable,
Read-Only) and also provides define that combine the memory attribute
and permission for common combination.

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

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/page.h | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Comments

Andre Przywara Aug. 23, 2017, 2:08 p.m. | #1
Hi,

On 14/08/17 15:24, 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.
> 
> Add introduce a couple of new flags for the permissions (Non-eXecutable,
> Read-Only) and also provides define that combine the memory attribute
> and permission for common combination.

If I haven't been lost in the definitions, this now adds "not
executable" to the existing definitions, which seems to make sense, but
is a change that might trigger regressions (especially for
PAGE_HYPERVISOR). So I wonder if that should be mentioned in the commit
message then?

The actual patch looks OK though, so:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> 
> A follow-up patch will change modify_xen_mappings to use the new flags.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  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 1bf8e9d012..047220f86b 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -67,12 +67,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
>
Julien Grall Aug. 23, 2017, 2:26 p.m. | #2
On 08/23/2017 03:08 PM, Andre Przywara wrote:
> Hi,

Hi,

> On 14/08/17 15:24, 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.
>>
>> Add introduce a couple of new flags for the permissions (Non-eXecutable,
>> Read-Only) and also provides define that combine the memory attribute
>> and permission for common combination.
> 
> If I haven't been lost in the definitions, this now adds "not
> executable" to the existing definitions, which seems to make sense, but
> is a change that might trigger regressions (especially for
> PAGE_HYPERVISOR). So I wonder if that should be mentioned in the commit
> message then?

It will not trigger regression because mfn_to_xen_entry is setting xn to 
1 by default. So all the mapping will be execute never when using 
PAGE_HYPERVISOR.

Cheers,
Andre Przywara Aug. 23, 2017, 2:37 p.m. | #3
Hi,

On 23/08/17 15:26, Julien Grall wrote:
> On 08/23/2017 03:08 PM, Andre Przywara wrote:
>> Hi,
> 
> Hi,
> 
>> On 14/08/17 15:24, 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.
>>>

Just saw that I forgot the typos here:

>>> Add introduce a couple of new flags for the permissions (Non-eXecutable,

Either "add" or "introduce", I guess.

>>> Read-Only) and also provides define that combine the memory attribute
>>> and permission for common combination.

Somehow the plural/singular is messed up here, I needed to read that
sentence multiple times.

>>
>> If I haven't been lost in the definitions, this now adds "not
>> executable" to the existing definitions, which seems to make sense, but
>> is a change that might trigger regressions (especially for
>> PAGE_HYPERVISOR). So I wonder if that should be mentioned in the commit
>> message then?
> 
> It will not trigger regression because mfn_to_xen_entry is setting xn to
> 1 by default. So all the mapping will be execute never when using
> PAGE_HYPERVISOR.

Ah right, I missed that. Might still be worth to mention in the commit
message, as this isn't obvious from just that patch.

Cheers,
Andre.
Julien Grall Aug. 23, 2017, 7:03 p.m. | #4
On 08/23/2017 03:37 PM, Andre Przywara wrote:
> Hi,
Hi Andre,

> On 23/08/17 15:26, Julien Grall wrote:
>> On 08/23/2017 03:08 PM, Andre Przywara wrote:
>>> Hi,
>>
>> Hi,
>>
>>> On 14/08/17 15:24, 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.
>>>>
> 
> Just saw that I forgot the typos here:
> 
>>>> Add introduce a couple of new flags for the permissions (Non-eXecutable,
> 
> Either "add" or "introduce", I guess.

I guess my mind disagree with my hands :). I will use "introduce" here.

> 
>>>> Read-Only) and also provides define that combine the memory attribute
>>>> and permission for common combination.
> 
> Somehow the plural/singular is messed up here, I needed to read that
> sentence multiple times.
> 
>>>
>>> If I haven't been lost in the definitions, this now adds "not
>>> executable" to the existing definitions, which seems to make sense, but
>>> is a change that might trigger regressions (especially for
>>> PAGE_HYPERVISOR). So I wonder if that should be mentioned in the commit
>>> message then?
>>
>> It will not trigger regression because mfn_to_xen_entry is setting xn to
>> 1 by default. So all the mapping will be execute never when using
>> PAGE_HYPERVISOR.
> 
> Ah right, I missed that. Might still be worth to mention in the commit
> message, as this isn't obvious from just that patch.

I can do that. Below the suggested commit message:

"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 define that combine the memory attribute
and permission for common combination.

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 this

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

Can I keep your reviewed-by?

Cheers,

Patch

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 1bf8e9d012..047220f86b 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -67,12 +67,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