| Message ID | 20170912100330.2168-23-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@arm.com> > > --- > > 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. I would probably remove the two definitions completely and only retain the useful comment above them. The patch looks good aside from this nit. > +#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
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@arm.com> >> >> --- >> >> 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,
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@arm.com> > > > > > > --- > > > > > > 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.
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@arm.com> >>>> >>>> --- >>>> >>>> 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,
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 + +#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. 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@arm.com> --- Changes in v2: - Update the commit message --- xen/include/asm-arm/page.h | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)