| Message ID | 20170814142418.13267-26-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Memory subsystem clean-up | expand |
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 >
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,
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.
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,
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
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(-)