Re: [Xen-devel] [PATCH v3 1/6] xen/arm: p2m: Add support for normal non-cacheable memory

["Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxxx>]

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@xxxxxxxxxx>
> >>>
> >>>Add support for describing normal non-cacheable memory.
> >>>
> >>>Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxxxxx>
> >>>---
> >>>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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel