| Message ID | 1473231377-7800-2-git-send-email-edgar.iglesias@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Edgar, On 07/09/2016 08:56, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Add support for describing normal non-cacheable memory. > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > xen/arch/arm/p2m.c | 18 ++++++++++++++++++ > xen/include/asm-arm/p2m.h | 1 + > xen/include/asm-arm/page.h | 1 + > 3 files changed, 20 insertions(+) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index b648a9d..bfef77b 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -282,6 +282,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a) > /* First apply type permissions */ > switch ( t ) > { > + case p2m_mem_nc: > case p2m_ram_rw: > e->p2m.xn = 0; > e->p2m.write = 1; > @@ -376,6 +377,23 @@ static lpae_t mfn_to_p2m_entry(mfn_t mfn, p2m_type_t t, p2m_access_t a) > e.p2m.sh = LPAE_SH_OUTER; > break; > > + /* > + * ARM ARM: Overlaying the shareability attribute (DDI > + * 0406C.b B3-1376 to 1377) > + * > + * A memory region with a resultant memory type attribute of Normal, > + * and a resultant cacheability attribute of Inner Non-cacheable, > + * Outer Non-cacheable, must have a resultant shareability attribute > + * of Outer Shareable, otherwise shareability is UNPREDICTABLE. > + * > + * On ARMv8 sharability is ignored and explicitly treated as Outer I know you copied it from mfn_to_xen_entry, but can we fixed the copy with: s/sharability/shareability/ > + * Shareable for Normal Inner Non_cacheable, Outer Non-cacheable. s/_/-/ Also I would like to see a spec reference for the ARMv8. I think it is the note in D4-1788 ARM DDI 0487A.j. > + */ > + case p2m_mem_nc: > + e.p2m.mattr = MATTR_MEM_NC; > + e.p2m.sh = LPAE_SH_OUTER; > + break; > + > default: > e.p2m.mattr = MATTR_MEM; > e.p2m.sh = LPAE_SH_INNER; > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index 53c4d78..b012d50 100644 > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h > @@ -93,6 +93,7 @@ typedef enum { > p2m_ram_ro, /* Read-only; writes are silently dropped */ > p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */ > p2m_mmio_direct_c, /* Read/write mapping of genuine MMIO area cacheable */ > + p2m_mem_nc, /* Read/write mapping of Non-cacheable Memory */ I find the name a bit confusing. Technically p2m_mem_nc is p2m_mmio_direct_c version but non-cacheable. I have got the feeling that the naming I used on a recent patch is not correct. Because p2m_mmio_direct_nc is not doing what is expect (i.e mapping non-cacheable). It maps with the device memory attribute. Maybe we should rename p2m_mmio_direct_nc to p2m_mmio_direct_dev (because it will use the device memory attribute) and then use p2m_mmio_direct_nc for your purpose. Any opinions? > p2m_map_foreign, /* Ram pages from foreign domain */ > p2m_grant_map_rw, /* Read/write grant mapping */ > p2m_grant_map_ro, /* Read-only grant mapping */ > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h > index 05d9f82..9adc973 100644 > --- a/xen/include/asm-arm/page.h > +++ b/xen/include/asm-arm/page.h > @@ -73,6 +73,7 @@ > * > */ > #define MATTR_DEV 0x1 > +#define MATTR_MEM_NC 0x5 > #define MATTR_MEM 0xf > > /* Flags for get_page_from_gva, gvirt_to_maddr etc */ > Regards,
Hi Edgar, On 07/09/2016 08:56, Edgar E. Iglesias wrote: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Add support for describing normal non-cacheable memory. I am a bit confused, you introduced this new p2m type but I don't see any usage of it in this series. Is it expected? Regards, > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > xen/arch/arm/p2m.c | 18 ++++++++++++++++++ > xen/include/asm-arm/p2m.h | 1 + > xen/include/asm-arm/page.h | 1 + > 3 files changed, 20 insertions(+) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index b648a9d..bfef77b 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -282,6 +282,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a) > /* First apply type permissions */ > switch ( t ) > { > + case p2m_mem_nc: > case p2m_ram_rw: > e->p2m.xn = 0; > e->p2m.write = 1; > @@ -376,6 +377,23 @@ static lpae_t mfn_to_p2m_entry(mfn_t mfn, p2m_type_t t, p2m_access_t a) > e.p2m.sh = LPAE_SH_OUTER; > break; > > + /* > + * ARM ARM: Overlaying the shareability attribute (DDI > + * 0406C.b B3-1376 to 1377) > + * > + * A memory region with a resultant memory type attribute of Normal, > + * and a resultant cacheability attribute of Inner Non-cacheable, > + * Outer Non-cacheable, must have a resultant shareability attribute > + * of Outer Shareable, otherwise shareability is UNPREDICTABLE. > + * > + * On ARMv8 sharability is ignored and explicitly treated as Outer > + * Shareable for Normal Inner Non_cacheable, Outer Non-cacheable. > + */ > + case p2m_mem_nc: > + e.p2m.mattr = MATTR_MEM_NC; > + e.p2m.sh = LPAE_SH_OUTER; > + break; > + > default: > e.p2m.mattr = MATTR_MEM; > e.p2m.sh = LPAE_SH_INNER; > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index 53c4d78..b012d50 100644 > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h > @@ -93,6 +93,7 @@ typedef enum { > p2m_ram_ro, /* Read-only; writes are silently dropped */ > p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */ > p2m_mmio_direct_c, /* Read/write mapping of genuine MMIO area cacheable */ > + p2m_mem_nc, /* Read/write mapping of Non-cacheable Memory */ > p2m_map_foreign, /* Ram pages from foreign domain */ > p2m_grant_map_rw, /* Read/write grant mapping */ > p2m_grant_map_ro, /* Read-only grant mapping */ > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h > index 05d9f82..9adc973 100644 > --- a/xen/include/asm-arm/page.h > +++ b/xen/include/asm-arm/page.h > @@ -73,6 +73,7 @@ > * > */ > #define MATTR_DEV 0x1 > +#define MATTR_MEM_NC 0x5 > #define MATTR_MEM 0xf > > /* Flags for get_page_from_gva, gvirt_to_maddr etc */ >
On Fri, Sep 16, 2016 at 04:21:12PM +0200, Julien Grall wrote: > Hi Edgar, Hi Julien, Thanks for the review! > On 07/09/2016 08:56, Edgar E. Iglesias wrote: > >From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > >Add support for describing normal non-cacheable memory. > > > >Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > >--- > > xen/arch/arm/p2m.c | 18 ++++++++++++++++++ > > xen/include/asm-arm/p2m.h | 1 + > > xen/include/asm-arm/page.h | 1 + > > 3 files changed, 20 insertions(+) > > > >diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > >index b648a9d..bfef77b 100644 > >--- a/xen/arch/arm/p2m.c > >+++ b/xen/arch/arm/p2m.c > >@@ -282,6 +282,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a) > > /* First apply type permissions */ > > switch ( t ) > > { > >+ case p2m_mem_nc: > > case p2m_ram_rw: > > e->p2m.xn = 0; > > e->p2m.write = 1; > >@@ -376,6 +377,23 @@ static lpae_t mfn_to_p2m_entry(mfn_t mfn, p2m_type_t t, p2m_access_t a) > > e.p2m.sh = LPAE_SH_OUTER; > > break; > > > >+ /* > >+ * ARM ARM: Overlaying the shareability attribute (DDI > >+ * 0406C.b B3-1376 to 1377) > >+ * > >+ * A memory region with a resultant memory type attribute of Normal, > >+ * and a resultant cacheability attribute of Inner Non-cacheable, > >+ * Outer Non-cacheable, must have a resultant shareability attribute > >+ * of Outer Shareable, otherwise shareability is UNPREDICTABLE. > >+ * > >+ * On ARMv8 sharability is ignored and explicitly treated as Outer > > I know you copied it from mfn_to_xen_entry, but can we fixed the copy with: > > s/sharability/shareability/ Fixed for v4. > > >+ * Shareable for Normal Inner Non_cacheable, Outer Non-cacheable. > > s/_/-/ > > Also I would like to see a spec reference for the ARMv8. I think it is the > note in D4-1788 ARM DDI 0487A.j. Fixed for v4. > > >+ */ > >+ case p2m_mem_nc: > >+ e.p2m.mattr = MATTR_MEM_NC; > >+ e.p2m.sh = LPAE_SH_OUTER; > >+ break; > >+ > > default: > > e.p2m.mattr = MATTR_MEM; > > e.p2m.sh = LPAE_SH_INNER; > >diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > >index 53c4d78..b012d50 100644 > >--- a/xen/include/asm-arm/p2m.h > >+++ b/xen/include/asm-arm/p2m.h > >@@ -93,6 +93,7 @@ typedef enum { > > p2m_ram_ro, /* Read-only; writes are silently dropped */ > > p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */ > > p2m_mmio_direct_c, /* Read/write mapping of genuine MMIO area cacheable */ > >+ p2m_mem_nc, /* Read/write mapping of Non-cacheable Memory */ > > I find the name a bit confusing. Technically p2m_mem_nc is p2m_mmio_direct_c > version but non-cacheable. > > I have got the feeling that the naming I used on a recent patch is not > correct. Because p2m_mmio_direct_nc is not doing what is expect (i.e mapping > non-cacheable). It maps with the device memory attribute. > > Maybe we should rename p2m_mmio_direct_nc to p2m_mmio_direct_dev (because it > will use the device memory attribute) and then use p2m_mmio_direct_nc for > your purpose. > > Any opinions? Sounds good, I'll do that. To keep the patches more readable, I thought i'd first do the rename in a first patch without any functional changes. Then I'll follow up with re-adding the p2m_mmio_direct_nc for my purposes. Let me know if you prefer something different. Thanks, Edgar > > > > p2m_map_foreign, /* Ram pages from foreign domain */ > > p2m_grant_map_rw, /* Read/write grant mapping */ > > p2m_grant_map_ro, /* Read-only grant mapping */ > >diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h > >index 05d9f82..9adc973 100644 > >--- a/xen/include/asm-arm/page.h > >+++ b/xen/include/asm-arm/page.h > >@@ -73,6 +73,7 @@ > > * > > */ > > #define MATTR_DEV 0x1 > >+#define MATTR_MEM_NC 0x5 > > #define MATTR_MEM 0xf > > > > /* Flags for get_page_from_gva, gvirt_to_maddr etc */ > > > > Regards, > > -- > Julien Grall
On Fri, Sep 16, 2016 at 04:34:36PM +0200, Julien Grall wrote: > Hi Edgar, > > On 07/09/2016 08:56, Edgar E. Iglesias wrote: > >From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > >Add support for describing normal non-cacheable memory. > > I am a bit confused, you introduced this new p2m type but I don't see any > usage of it in this series. Is it expected? Hi Julien, It's supposed to be used in patch 6 xen/arm: Map mmio-sram nodes as un-cached memory But there's a typo using p2m_mmio_direct_c instead of p2m_mem_nc :-) I'll fix that in v4. Cheers, Edgar > Regards, > > > > >Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > >--- > > xen/arch/arm/p2m.c | 18 ++++++++++++++++++ > > xen/include/asm-arm/p2m.h | 1 + > > xen/include/asm-arm/page.h | 1 + > > 3 files changed, 20 insertions(+) > > > >diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > >index b648a9d..bfef77b 100644 > >--- a/xen/arch/arm/p2m.c > >+++ b/xen/arch/arm/p2m.c > >@@ -282,6 +282,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a) > > /* First apply type permissions */ > > switch ( t ) > > { > >+ case p2m_mem_nc: > > case p2m_ram_rw: > > e->p2m.xn = 0; > > e->p2m.write = 1; > >@@ -376,6 +377,23 @@ static lpae_t mfn_to_p2m_entry(mfn_t mfn, p2m_type_t t, p2m_access_t a) > > e.p2m.sh = LPAE_SH_OUTER; > > break; > > > >+ /* > >+ * ARM ARM: Overlaying the shareability attribute (DDI > >+ * 0406C.b B3-1376 to 1377) > >+ * > >+ * A memory region with a resultant memory type attribute of Normal, > >+ * and a resultant cacheability attribute of Inner Non-cacheable, > >+ * Outer Non-cacheable, must have a resultant shareability attribute > >+ * of Outer Shareable, otherwise shareability is UNPREDICTABLE. > >+ * > >+ * On ARMv8 sharability is ignored and explicitly treated as Outer > >+ * Shareable for Normal Inner Non_cacheable, Outer Non-cacheable. > >+ */ > >+ case p2m_mem_nc: > >+ e.p2m.mattr = MATTR_MEM_NC; > >+ e.p2m.sh = LPAE_SH_OUTER; > >+ break; > >+ > > default: > > e.p2m.mattr = MATTR_MEM; > > e.p2m.sh = LPAE_SH_INNER; > >diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > >index 53c4d78..b012d50 100644 > >--- a/xen/include/asm-arm/p2m.h > >+++ b/xen/include/asm-arm/p2m.h > >@@ -93,6 +93,7 @@ typedef enum { > > p2m_ram_ro, /* Read-only; writes are silently dropped */ > > p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */ > > p2m_mmio_direct_c, /* Read/write mapping of genuine MMIO area cacheable */ > >+ p2m_mem_nc, /* Read/write mapping of Non-cacheable Memory */ > > p2m_map_foreign, /* Ram pages from foreign domain */ > > p2m_grant_map_rw, /* Read/write grant mapping */ > > p2m_grant_map_ro, /* Read-only grant mapping */ > >diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h > >index 05d9f82..9adc973 100644 > >--- a/xen/include/asm-arm/page.h > >+++ b/xen/include/asm-arm/page.h > >@@ -73,6 +73,7 @@ > > * > > */ > > #define MATTR_DEV 0x1 > >+#define MATTR_MEM_NC 0x5 > > #define MATTR_MEM 0xf > > > > /* Flags for get_page_from_gva, gvirt_to_maddr etc */ > > > > -- > Julien Grall
On Fri, Sep 16, 2016 at 04:21:12PM +0200, Julien Grall wrote: > Hi Edgar, > > On 07/09/2016 08:56, Edgar E. Iglesias wrote: > >From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > >Add support for describing normal non-cacheable memory. > > > >Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > >--- > > xen/arch/arm/p2m.c | 18 ++++++++++++++++++ > > xen/include/asm-arm/p2m.h | 1 + > > xen/include/asm-arm/page.h | 1 + > > 3 files changed, 20 insertions(+) > > > >diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > >index b648a9d..bfef77b 100644 > >--- a/xen/arch/arm/p2m.c > >+++ b/xen/arch/arm/p2m.c > >@@ -282,6 +282,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a) > > /* First apply type permissions */ > > switch ( t ) > > { > >+ case p2m_mem_nc: > > case p2m_ram_rw: > > e->p2m.xn = 0; > > e->p2m.write = 1; > >@@ -376,6 +377,23 @@ static lpae_t mfn_to_p2m_entry(mfn_t mfn, p2m_type_t t, p2m_access_t a) > > e.p2m.sh = LPAE_SH_OUTER; > > break; > > > >+ /* > >+ * ARM ARM: Overlaying the shareability attribute (DDI > >+ * 0406C.b B3-1376 to 1377) > >+ * > >+ * A memory region with a resultant memory type attribute of Normal, > >+ * and a resultant cacheability attribute of Inner Non-cacheable, > >+ * Outer Non-cacheable, must have a resultant shareability attribute > >+ * of Outer Shareable, otherwise shareability is UNPREDICTABLE. > >+ * > >+ * On ARMv8 sharability is ignored and explicitly treated as Outer > > I know you copied it from mfn_to_xen_entry, but can we fixed the copy with: > > s/sharability/shareability/ > > >+ * Shareable for Normal Inner Non_cacheable, Outer Non-cacheable. > > s/_/-/ > > Also I would like to see a spec reference for the ARMv8. I think it is the > note in D4-1788 ARM DDI 0487A.j. > > >+ */ > >+ case p2m_mem_nc: > >+ e.p2m.mattr = MATTR_MEM_NC; > >+ e.p2m.sh = LPAE_SH_OUTER; > >+ break; > >+ > > default: > > e.p2m.mattr = MATTR_MEM; > > e.p2m.sh = LPAE_SH_INNER; > >diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > >index 53c4d78..b012d50 100644 > >--- a/xen/include/asm-arm/p2m.h > >+++ b/xen/include/asm-arm/p2m.h > >@@ -93,6 +93,7 @@ typedef enum { > > p2m_ram_ro, /* Read-only; writes are silently dropped */ > > p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */ > > p2m_mmio_direct_c, /* Read/write mapping of genuine MMIO area cacheable */ > >+ p2m_mem_nc, /* Read/write mapping of Non-cacheable Memory */ > > I find the name a bit confusing. Technically p2m_mem_nc is p2m_mmio_direct_c > version but non-cacheable. > > I have got the feeling that the naming I used on a recent patch is not > correct. Because p2m_mmio_direct_nc is not doing what is expect (i.e mapping > non-cacheable). It maps with the device memory attribute. > > Maybe we should rename p2m_mmio_direct_nc to p2m_mmio_direct_dev (because it > will use the device memory attribute) and then use p2m_mmio_direct_nc for > your purpose. > > Any opinions? Something that shows up after doing the rename and otherwise keeping the patch is that we treat the XN bit differently for p2m_mmio_direct_nc and p2m_mmio_direct_c. Is there any reason why we can't allow execution for p2m_mmio_direct_c mappings? If so, perhaps that same reason also applies to p2m_mmio_direct_nc and both should be non-executable. I think there are use-cases for running guests that will want to execute from on chip memories. But we can cross that bridge when we come to it... Cheers, Edgar > > > > p2m_map_foreign, /* Ram pages from foreign domain */ > > p2m_grant_map_rw, /* Read/write grant mapping */ > > p2m_grant_map_ro, /* Read-only grant mapping */ > >diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h > >index 05d9f82..9adc973 100644 > >--- a/xen/include/asm-arm/page.h > >+++ b/xen/include/asm-arm/page.h > >@@ -73,6 +73,7 @@ > > * > > */ > > #define MATTR_DEV 0x1 > >+#define MATTR_MEM_NC 0x5 > > #define MATTR_MEM 0xf > > > > /* Flags for get_page_from_gva, gvirt_to_maddr etc */ > > > > Regards, > > -- > Julien Grall
Hi Edgar, On 16/09/2016 18:17, Edgar E. Iglesias wrote: > On Fri, Sep 16, 2016 at 04:21:12PM +0200, Julien Grall wrote: >> Hi Edgar, >> >> On 07/09/2016 08:56, Edgar E. Iglesias wrote: >>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> >>> >>> Add support for describing normal non-cacheable memory. >>> >>> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> >>> --- >>> xen/arch/arm/p2m.c | 18 ++++++++++++++++++ >>> xen/include/asm-arm/p2m.h | 1 + >>> xen/include/asm-arm/page.h | 1 + >>> 3 files changed, 20 insertions(+) >>> >>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>> index b648a9d..bfef77b 100644 >>> --- a/xen/arch/arm/p2m.c >>> +++ b/xen/arch/arm/p2m.c >>> @@ -282,6 +282,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a) >>> /* First apply type permissions */ >>> switch ( t ) >>> { >>> + case p2m_mem_nc: >>> case p2m_ram_rw: >>> e->p2m.xn = 0; >>> e->p2m.write = 1; >>> @@ -376,6 +377,23 @@ static lpae_t mfn_to_p2m_entry(mfn_t mfn, p2m_type_t t, p2m_access_t a) >>> e.p2m.sh = LPAE_SH_OUTER; >>> break; >>> >>> + /* >>> + * ARM ARM: Overlaying the shareability attribute (DDI >>> + * 0406C.b B3-1376 to 1377) >>> + * >>> + * A memory region with a resultant memory type attribute of Normal, >>> + * and a resultant cacheability attribute of Inner Non-cacheable, >>> + * Outer Non-cacheable, must have a resultant shareability attribute >>> + * of Outer Shareable, otherwise shareability is UNPREDICTABLE. >>> + * >>> + * On ARMv8 sharability is ignored and explicitly treated as Outer >> >> I know you copied it from mfn_to_xen_entry, but can we fixed the copy with: >> >> s/sharability/shareability/ >> >>> + * Shareable for Normal Inner Non_cacheable, Outer Non-cacheable. >> >> s/_/-/ >> >> Also I would like to see a spec reference for the ARMv8. I think it is the >> note in D4-1788 ARM DDI 0487A.j. >> >>> + */ >>> + case p2m_mem_nc: >>> + e.p2m.mattr = MATTR_MEM_NC; >>> + e.p2m.sh = LPAE_SH_OUTER; >>> + break; >>> + >>> default: >>> e.p2m.mattr = MATTR_MEM; >>> e.p2m.sh = LPAE_SH_INNER; >>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h >>> index 53c4d78..b012d50 100644 >>> --- a/xen/include/asm-arm/p2m.h >>> +++ b/xen/include/asm-arm/p2m.h >>> @@ -93,6 +93,7 @@ typedef enum { >>> p2m_ram_ro, /* Read-only; writes are silently dropped */ >>> p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */ >>> p2m_mmio_direct_c, /* Read/write mapping of genuine MMIO area cacheable */ >>> + p2m_mem_nc, /* Read/write mapping of Non-cacheable Memory */ >> >> I find the name a bit confusing. Technically p2m_mem_nc is p2m_mmio_direct_c >> version but non-cacheable. >> >> I have got the feeling that the naming I used on a recent patch is not >> correct. Because p2m_mmio_direct_nc is not doing what is expect (i.e mapping >> non-cacheable). It maps with the device memory attribute. >> >> Maybe we should rename p2m_mmio_direct_nc to p2m_mmio_direct_dev (because it >> will use the device memory attribute) and then use p2m_mmio_direct_nc for >> your purpose. >> >> Any opinions? > > > Something that shows up after doing the rename and otherwise keeping the > patch is that we treat the XN bit differently for p2m_mmio_direct_nc > and p2m_mmio_direct_c. > > Is there any reason why we can't allow execution for p2m_mmio_direct_c > mappings? > If so, perhaps that same reason also applies to p2m_mmio_direct_nc and > both should be non-executable. I guess you mean your new p2m_mmio_direct_nc and not the current one. If so, I think it is more a safety. Until now, the p2m type was used to share ACPI tables which should not be executable. I am not sure what would be the implication to unset the NX bit by default. The question to answer before any relaxation is could a potential misbehave guest harm the platform? Regards,
On Mon, Sep 19, 2016 at 05:22:27PM +0200, Julien Grall wrote: > Hi Edgar, Hi Julien, Sorry for the late reply! > > On 16/09/2016 18:17, Edgar E. Iglesias wrote: > >On Fri, Sep 16, 2016 at 04:21:12PM +0200, Julien Grall wrote: > >>Hi Edgar, > >> > >>On 07/09/2016 08:56, Edgar E. Iglesias wrote: > >>>From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > >>> > >>>Add support for describing normal non-cacheable memory. > >>> > >>>Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > >>>--- > >>>xen/arch/arm/p2m.c | 18 ++++++++++++++++++ > >>>xen/include/asm-arm/p2m.h | 1 + > >>>xen/include/asm-arm/page.h | 1 + > >>>3 files changed, 20 insertions(+) > >>> > >>>diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > >>>index b648a9d..bfef77b 100644 > >>>--- a/xen/arch/arm/p2m.c > >>>+++ b/xen/arch/arm/p2m.c > >>>@@ -282,6 +282,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a) > >>> /* First apply type permissions */ > >>> switch ( t ) > >>> { > >>>+ case p2m_mem_nc: > >>> case p2m_ram_rw: > >>> e->p2m.xn = 0; > >>> e->p2m.write = 1; > >>>@@ -376,6 +377,23 @@ static lpae_t mfn_to_p2m_entry(mfn_t mfn, p2m_type_t t, p2m_access_t a) > >>> e.p2m.sh = LPAE_SH_OUTER; > >>> break; > >>> > >>>+ /* > >>>+ * ARM ARM: Overlaying the shareability attribute (DDI > >>>+ * 0406C.b B3-1376 to 1377) > >>>+ * > >>>+ * A memory region with a resultant memory type attribute of Normal, > >>>+ * and a resultant cacheability attribute of Inner Non-cacheable, > >>>+ * Outer Non-cacheable, must have a resultant shareability attribute > >>>+ * of Outer Shareable, otherwise shareability is UNPREDICTABLE. > >>>+ * > >>>+ * On ARMv8 sharability is ignored and explicitly treated as Outer > >> > >>I know you copied it from mfn_to_xen_entry, but can we fixed the copy with: > >> > >>s/sharability/shareability/ > >> > >>>+ * Shareable for Normal Inner Non_cacheable, Outer Non-cacheable. > >> > >>s/_/-/ > >> > >>Also I would like to see a spec reference for the ARMv8. I think it is the > >>note in D4-1788 ARM DDI 0487A.j. > >> > >>>+ */ > >>>+ case p2m_mem_nc: > >>>+ e.p2m.mattr = MATTR_MEM_NC; > >>>+ e.p2m.sh = LPAE_SH_OUTER; > >>>+ break; > >>>+ > >>> default: > >>> e.p2m.mattr = MATTR_MEM; > >>> e.p2m.sh = LPAE_SH_INNER; > >>>diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > >>>index 53c4d78..b012d50 100644 > >>>--- a/xen/include/asm-arm/p2m.h > >>>+++ b/xen/include/asm-arm/p2m.h > >>>@@ -93,6 +93,7 @@ typedef enum { > >>> p2m_ram_ro, /* Read-only; writes are silently dropped */ > >>> p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */ > >>> p2m_mmio_direct_c, /* Read/write mapping of genuine MMIO area cacheable */ > >>>+ p2m_mem_nc, /* Read/write mapping of Non-cacheable Memory */ > >> > >>I find the name a bit confusing. Technically p2m_mem_nc is p2m_mmio_direct_c > >>version but non-cacheable. > >> > >>I have got the feeling that the naming I used on a recent patch is not > >>correct. Because p2m_mmio_direct_nc is not doing what is expect (i.e mapping > >>non-cacheable). It maps with the device memory attribute. > >> > >>Maybe we should rename p2m_mmio_direct_nc to p2m_mmio_direct_dev (because it > >>will use the device memory attribute) and then use p2m_mmio_direct_nc for > >>your purpose. > >> > >>Any opinions? > > > > > >Something that shows up after doing the rename and otherwise keeping the > >patch is that we treat the XN bit differently for p2m_mmio_direct_nc > >and p2m_mmio_direct_c. > > > >Is there any reason why we can't allow execution for p2m_mmio_direct_c > >mappings? > >If so, perhaps that same reason also applies to p2m_mmio_direct_nc and > >both should be non-executable. > > I guess you mean your new p2m_mmio_direct_nc and not the current one. If so, > I think it is more a safety. Until now, the p2m type was used to share ACPI > tables which should not be executable. Yes, I meant that it's a little strange to have them differ, e.g: p2m_mmio_direct_nc xn = 0 p2m_mmio_direct_c xn = 1 Because intuitively, when looking at those types I would expect them to be identical except for the cacheability... > I am not sure what would be the implication to unset the NX bit by default. > The question to answer before any relaxation is could a potential misbehave > guest harm the platform? Agreed. I personally don't see a reason why normal memory would cause problems but it may be a little late in the release cycle to risk anything. I can make the new p2m_mmio_direct_nc XN=1 and we can discuss a relaxation for the next release? Cheers, Edgar
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index b648a9d..bfef77b 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -282,6 +282,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a) /* First apply type permissions */ switch ( t ) { + case p2m_mem_nc: case p2m_ram_rw: e->p2m.xn = 0; e->p2m.write = 1; @@ -376,6 +377,23 @@ static lpae_t mfn_to_p2m_entry(mfn_t mfn, p2m_type_t t, p2m_access_t a) e.p2m.sh = LPAE_SH_OUTER; break; + /* + * ARM ARM: Overlaying the shareability attribute (DDI + * 0406C.b B3-1376 to 1377) + * + * A memory region with a resultant memory type attribute of Normal, + * and a resultant cacheability attribute of Inner Non-cacheable, + * Outer Non-cacheable, must have a resultant shareability attribute + * of Outer Shareable, otherwise shareability is UNPREDICTABLE. + * + * On ARMv8 sharability is ignored and explicitly treated as Outer + * Shareable for Normal Inner Non_cacheable, Outer Non-cacheable. + */ + case p2m_mem_nc: + e.p2m.mattr = MATTR_MEM_NC; + e.p2m.sh = LPAE_SH_OUTER; + break; + default: e.p2m.mattr = MATTR_MEM; e.p2m.sh = LPAE_SH_INNER; diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 53c4d78..b012d50 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -93,6 +93,7 @@ typedef enum { p2m_ram_ro, /* Read-only; writes are silently dropped */ p2m_mmio_direct_nc, /* Read/write mapping of genuine MMIO area non-cacheable */ p2m_mmio_direct_c, /* Read/write mapping of genuine MMIO area cacheable */ + p2m_mem_nc, /* Read/write mapping of Non-cacheable Memory */ p2m_map_foreign, /* Ram pages from foreign domain */ p2m_grant_map_rw, /* Read/write grant mapping */ p2m_grant_map_ro, /* Read-only grant mapping */ diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h index 05d9f82..9adc973 100644 --- a/xen/include/asm-arm/page.h +++ b/xen/include/asm-arm/page.h @@ -73,6 +73,7 @@ * */ #define MATTR_DEV 0x1 +#define MATTR_MEM_NC 0x5 #define MATTR_MEM 0xf /* Flags for get_page_from_gva, gvirt_to_maddr etc */