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

[Julien Grall <julien.grall@xxxxxxx>]

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

--
Julien Grall

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