[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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

---

     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,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.