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

[Julien Grall <julien.grall@xxxxxxx>]

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