Re: [Xen-devel] [PATCH 25/27] xen/arm: mm: Embed permission in the flags
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, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 25/27] xen/arm: mm: Embed permission in the flags
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. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 25/27] xen/arm: mm: Embed permission in the flags
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, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 25/27] xen/arm: mm: Embed permission in the flags
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 Cheers, Andre. > > A follow-up patch will change modify_xen_mappings to use the new flags. > > Signed-off-by: Julien Grall > --- > 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_BIT3 > +#define _PAGE_RO_BIT4 > +#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_NORMALMT_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 > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 25/27] xen/arm: mm: Embed permission in the flags
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 --- 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_BIT3 +#define _PAGE_RO_BIT4 +#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_NORMALMT_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 -- 2.11.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
