[PATCH v3 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[PATCH v3 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

This series adds support for mapping mmio-sram nodes into dom0
as normal uncached MEMORY with RWX perms.

If the no-memory-wc prop is present in the DTB node, we create
DEVICE RW mappings instead.

Julien, I've scratched the map_regions_mattr name since we are now
passing p2m_type_t's instead. I borrowed the short p2mt name from
existing code.

Comments welcome!

Best regards,
Edgar

ChangeLog:

v2 -> v3:
* Drop RFC
* Add comment on outer-shareable choice for non-cached mem
* Spellfix existance -> existence
* Add comment on props usage
* Props matching now only supports a single property
* Dropped p2m_access in plumbing for mapping attributes
* Rename un/map_regions to un/map_regions_p2mt
* Add path to mmio-sram device-tree bindings docs in commit msg
* Add a comment on inheriting parent mem attributes
* Dropped the mmio-sram bus

v1 -> v2
* Replace the .map method with a list of match -> map attributes
* Handle no-memory-wc by mapping as DEVICE RW
* Generalize map_regions_rw_cache instead of adding new functions

Edgar E. Iglesias (6):
  xen/arm: p2m: Add support for normal non-cacheable memory
  xen/arm: Rename and generalize un/map_regions_rw_cache
  xen/device-tree: Add __DT_MATCH macros without braces
  xen/device-tree: Make dt_match_node match props
  xen/arm: domain_build: Plumb for different mapping attributes
  xen/arm: Map mmio-sram nodes as un-cached memory

 xen/arch/arm/domain_build.c   | 90 +++++++++++++++++++++++++++++++++----------
 xen/arch/arm/p2m.c            | 37 +++++++++++++-----
 xen/common/device_tree.c      |  5 ++-
 xen/include/asm-arm/p2m.h     | 18 +++++----
 xen/include/asm-arm/page.h    |  1 +
 xen/include/xen/device_tree.h | 20 ++++++++--
 6 files changed, 128 insertions(+), 43 deletions(-)

-- 
1.9.1


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

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

[Edgar E. Iglesias]

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
+     * 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 */
-- 
1.9.1


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

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

[Julien Grall]

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,

-- 
Julien Grall

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

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

[Edgar E. Iglesias]

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

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

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

[Edgar E. Iglesias]

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

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

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

[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,

-- 
Julien Grall

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

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

[Edgar E. Iglesias]

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

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

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

[Julien Grall]

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 */
>

-- 
Julien Grall

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

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

[Edgar E. Iglesias]

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

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

[PATCH v3 2/6] xen/arm: Rename and generalize un/map_regions_rw_cache

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Rename and generalize un/map_regions_rw_cache into
un/map_regions_p2mt. The new functions take the mapping
attributes as an argument.

No functional change.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/arch/arm/domain_build.c | 18 ++++++++++--------
 xen/arch/arm/p2m.c          | 19 ++++++++++---------
 xen/include/asm-arm/p2m.h   | 19 ++++++++++---------
 3 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 35ab08d..f022342 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1518,10 +1518,11 @@ static void acpi_map_other_tables(struct domain *d)
     {
         addr = acpi_gbl_root_table_list.tables[i].address;
         size = acpi_gbl_root_table_list.tables[i].length;
-        res = map_regions_rw_cache(d,
-                                   _gfn(paddr_to_pfn(addr)),
-                                   DIV_ROUND_UP(size, PAGE_SIZE),
-                                   _mfn(paddr_to_pfn(addr)));
+        res = map_regions_p2mt(d,
+                               _gfn(paddr_to_pfn(addr)),
+                               DIV_ROUND_UP(size, PAGE_SIZE),
+                               _mfn(paddr_to_pfn(addr)),
+                               p2m_mmio_direct_c);
         if ( res )
         {
              panic(XENLOG_ERR "Unable to map ACPI region 0x%"PRIx64
@@ -1874,10 +1875,11 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
     acpi_create_efi_mmap_table(d, &kinfo->mem, tbl_add);
 
     /* Map the EFI and ACPI tables to Dom0 */
-    rc = map_regions_rw_cache(d,
-                              _gfn(paddr_to_pfn(d->arch.efi_acpi_gpa)),
-                              PFN_UP(d->arch.efi_acpi_len),
-                              _mfn(paddr_to_pfn(virt_to_maddr(d->arch.efi_acpi_table))));
+    rc = map_regions_p2mt(d,
+                          _gfn(paddr_to_pfn(d->arch.efi_acpi_gpa)),
+                          PFN_UP(d->arch.efi_acpi_len),
+                          _mfn(paddr_to_pfn(virt_to_maddr(d->arch.efi_acpi_table))),
+                          p2m_mmio_direct_c);
     if ( rc != 0 )
     {
         printk(XENLOG_ERR "Unable to map EFI/ACPI table 0x%"PRIx64
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index bfef77b..58d4940 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1234,18 +1234,19 @@ static inline int p2m_remove_mapping(struct domain *d,
                              0, p2m_invalid, d->arch.p2m.default_access);
 }
 
-int map_regions_rw_cache(struct domain *d,
-                         gfn_t gfn,
-                         unsigned long nr,
-                         mfn_t mfn)
+int map_regions_p2mt(struct domain *d,
+                     gfn_t gfn,
+                     unsigned long nr,
+                     mfn_t mfn,
+                     p2m_type_t p2mt)
 {
-    return p2m_insert_mapping(d, gfn, nr, mfn, p2m_mmio_direct_c);
+    return p2m_insert_mapping(d, gfn, nr, mfn, p2mt);
 }
 
-int unmap_regions_rw_cache(struct domain *d,
-                           gfn_t gfn,
-                           unsigned long nr,
-                           mfn_t mfn)
+int unmap_regions_p2mt(struct domain *d,
+                       gfn_t gfn,
+                       unsigned long nr,
+                       mfn_t mfn)
 {
     return p2m_remove_mapping(d, gfn, nr, mfn);
 }
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index b012d50..f2bd16c 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -166,15 +166,16 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
 /* Clean & invalidate caches corresponding to a region of guest address space */
 int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr);
 
-int map_regions_rw_cache(struct domain *d,
-                         gfn_t gfn,
-                         unsigned long nr,
-                         mfn_t mfn);
-
-int unmap_regions_rw_cache(struct domain *d,
-                           gfn_t gfn,
-                           unsigned long nr,
-                           mfn_t mfn);
+int map_regions_p2mt(struct domain *d,
+                     gfn_t gfn,
+                     unsigned long nr,
+                     mfn_t mfn,
+                     p2m_type_t p2mt);
+
+int unmap_regions_p2mt(struct domain *d,
+                       gfn_t gfn,
+                       unsigned long nr,
+                       mfn_t mfn);
 
 int map_dev_mmio_region(struct domain *d,
                         gfn_t gfn,
-- 
1.9.1


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

Re: [PATCH v3 2/6] xen/arm: Rename and generalize un/map_regions_rw_cache

[Julien Grall]

Hi Edgar,

On 07/09/2016 08:56, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Rename and generalize un/map_regions_rw_cache into
> un/map_regions_p2mt. The new functions take the mapping
> attributes as an argument.
>
> No functional change.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  xen/arch/arm/domain_build.c | 18 ++++++++++--------
>  xen/arch/arm/p2m.c          | 19 ++++++++++---------
>  xen/include/asm-arm/p2m.h   | 19 ++++++++++---------
>  3 files changed, 30 insertions(+), 26 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 35ab08d..f022342 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1518,10 +1518,11 @@ static void acpi_map_other_tables(struct domain *d)
>      {
>          addr = acpi_gbl_root_table_list.tables[i].address;
>          size = acpi_gbl_root_table_list.tables[i].length;
> -        res = map_regions_rw_cache(d,
> -                                   _gfn(paddr_to_pfn(addr)),
> -                                   DIV_ROUND_UP(size, PAGE_SIZE),
> -                                   _mfn(paddr_to_pfn(addr)));
> +        res = map_regions_p2mt(d,
> +                               _gfn(paddr_to_pfn(addr)),
> +                               DIV_ROUND_UP(size, PAGE_SIZE),
> +                               _mfn(paddr_to_pfn(addr)),
> +                               p2m_mmio_direct_c);
>          if ( res )
>          {
>               panic(XENLOG_ERR "Unable to map ACPI region 0x%"PRIx64
> @@ -1874,10 +1875,11 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
>      acpi_create_efi_mmap_table(d, &kinfo->mem, tbl_add);
>
>      /* Map the EFI and ACPI tables to Dom0 */
> -    rc = map_regions_rw_cache(d,
> -                              _gfn(paddr_to_pfn(d->arch.efi_acpi_gpa)),
> -                              PFN_UP(d->arch.efi_acpi_len),
> -                              _mfn(paddr_to_pfn(virt_to_maddr(d->arch.efi_acpi_table))));
> +    rc = map_regions_p2mt(d,
> +                          _gfn(paddr_to_pfn(d->arch.efi_acpi_gpa)),
> +                          PFN_UP(d->arch.efi_acpi_len),
> +                          _mfn(paddr_to_pfn(virt_to_maddr(d->arch.efi_acpi_table))),
> +                          p2m_mmio_direct_c);
>      if ( rc != 0 )
>      {
>          printk(XENLOG_ERR "Unable to map EFI/ACPI table 0x%"PRIx64
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index bfef77b..58d4940 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1234,18 +1234,19 @@ static inline int p2m_remove_mapping(struct domain *d,
>                               0, p2m_invalid, d->arch.p2m.default_access);
>  }
>
> -int map_regions_rw_cache(struct domain *d,
> -                         gfn_t gfn,
> -                         unsigned long nr,
> -                         mfn_t mfn)
> +int map_regions_p2mt(struct domain *d,
> +                     gfn_t gfn,
> +                     unsigned long nr,
> +                     mfn_t mfn,
> +                     p2m_type_t p2mt)
>  {
> -    return p2m_insert_mapping(d, gfn, nr, mfn, p2m_mmio_direct_c);
> +    return p2m_insert_mapping(d, gfn, nr, mfn, p2mt);
>  }
>
> -int unmap_regions_rw_cache(struct domain *d,
> -                           gfn_t gfn,
> -                           unsigned long nr,
> -                           mfn_t mfn)
> +int unmap_regions_p2mt(struct domain *d,
> +                       gfn_t gfn,
> +                       unsigned long nr,
> +                       mfn_t mfn)
>  {
>      return p2m_remove_mapping(d, gfn, nr, mfn);
>  }
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index b012d50..f2bd16c 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -166,15 +166,16 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
>  /* Clean & invalidate caches corresponding to a region of guest address space */
>  int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr);
>
> -int map_regions_rw_cache(struct domain *d,
> -                         gfn_t gfn,
> -                         unsigned long nr,
> -                         mfn_t mfn);
> -
> -int unmap_regions_rw_cache(struct domain *d,
> -                           gfn_t gfn,
> -                           unsigned long nr,
> -                           mfn_t mfn);
> +int map_regions_p2mt(struct domain *d,
> +                     gfn_t gfn,
> +                     unsigned long nr,
> +                     mfn_t mfn,
> +                     p2m_type_t p2mt);

Can you document the purpose of this function in the code? Something 
like: "Map the region in the guest p2m with a specific type (will affect 
the attributes of the region).".

> +
> +int unmap_regions_p2mt(struct domain *d,
> +                       gfn_t gfn,
> +                       unsigned long nr,
> +                       mfn_t mfn);
>
>  int map_dev_mmio_region(struct domain *d,
>                          gfn_t gfn,
>

Regards,

-- 
Julien Grall

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

Re: [PATCH v3 2/6] xen/arm: Rename and generalize un/map_regions_rw_cache

[Edgar E. Iglesias]

On Fri, Sep 16, 2016 at 04:24:17PM +0200, Julien Grall wrote:
> Hi Edgar,
> 
> On 07/09/2016 08:56, Edgar E. Iglesias wrote:
> >From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> >Rename and generalize un/map_regions_rw_cache into
> >un/map_regions_p2mt. The new functions take the mapping
> >attributes as an argument.
> >
> >No functional change.
> >
> >Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> >---
> > xen/arch/arm/domain_build.c | 18 ++++++++++--------
> > xen/arch/arm/p2m.c          | 19 ++++++++++---------
> > xen/include/asm-arm/p2m.h   | 19 ++++++++++---------
> > 3 files changed, 30 insertions(+), 26 deletions(-)
> >
> >diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >index 35ab08d..f022342 100644
> >--- a/xen/arch/arm/domain_build.c
> >+++ b/xen/arch/arm/domain_build.c
> >@@ -1518,10 +1518,11 @@ static void acpi_map_other_tables(struct domain *d)
> >     {
> >         addr = acpi_gbl_root_table_list.tables[i].address;
> >         size = acpi_gbl_root_table_list.tables[i].length;
> >-        res = map_regions_rw_cache(d,
> >-                                   _gfn(paddr_to_pfn(addr)),
> >-                                   DIV_ROUND_UP(size, PAGE_SIZE),
> >-                                   _mfn(paddr_to_pfn(addr)));
> >+        res = map_regions_p2mt(d,
> >+                               _gfn(paddr_to_pfn(addr)),
> >+                               DIV_ROUND_UP(size, PAGE_SIZE),
> >+                               _mfn(paddr_to_pfn(addr)),
> >+                               p2m_mmio_direct_c);
> >         if ( res )
> >         {
> >              panic(XENLOG_ERR "Unable to map ACPI region 0x%"PRIx64
> >@@ -1874,10 +1875,11 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
> >     acpi_create_efi_mmap_table(d, &kinfo->mem, tbl_add);
> >
> >     /* Map the EFI and ACPI tables to Dom0 */
> >-    rc = map_regions_rw_cache(d,
> >-                              _gfn(paddr_to_pfn(d->arch.efi_acpi_gpa)),
> >-                              PFN_UP(d->arch.efi_acpi_len),
> >-                              _mfn(paddr_to_pfn(virt_to_maddr(d->arch.efi_acpi_table))));
> >+    rc = map_regions_p2mt(d,
> >+                          _gfn(paddr_to_pfn(d->arch.efi_acpi_gpa)),
> >+                          PFN_UP(d->arch.efi_acpi_len),
> >+                          _mfn(paddr_to_pfn(virt_to_maddr(d->arch.efi_acpi_table))),
> >+                          p2m_mmio_direct_c);
> >     if ( rc != 0 )
> >     {
> >         printk(XENLOG_ERR "Unable to map EFI/ACPI table 0x%"PRIx64
> >diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> >index bfef77b..58d4940 100644
> >--- a/xen/arch/arm/p2m.c
> >+++ b/xen/arch/arm/p2m.c
> >@@ -1234,18 +1234,19 @@ static inline int p2m_remove_mapping(struct domain *d,
> >                              0, p2m_invalid, d->arch.p2m.default_access);
> > }
> >
> >-int map_regions_rw_cache(struct domain *d,
> >-                         gfn_t gfn,
> >-                         unsigned long nr,
> >-                         mfn_t mfn)
> >+int map_regions_p2mt(struct domain *d,
> >+                     gfn_t gfn,
> >+                     unsigned long nr,
> >+                     mfn_t mfn,
> >+                     p2m_type_t p2mt)
> > {
> >-    return p2m_insert_mapping(d, gfn, nr, mfn, p2m_mmio_direct_c);
> >+    return p2m_insert_mapping(d, gfn, nr, mfn, p2mt);
> > }
> >
> >-int unmap_regions_rw_cache(struct domain *d,
> >-                           gfn_t gfn,
> >-                           unsigned long nr,
> >-                           mfn_t mfn)
> >+int unmap_regions_p2mt(struct domain *d,
> >+                       gfn_t gfn,
> >+                       unsigned long nr,
> >+                       mfn_t mfn)
> > {
> >     return p2m_remove_mapping(d, gfn, nr, mfn);
> > }
> >diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> >index b012d50..f2bd16c 100644
> >--- a/xen/include/asm-arm/p2m.h
> >+++ b/xen/include/asm-arm/p2m.h
> >@@ -166,15 +166,16 @@ mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
> > /* Clean & invalidate caches corresponding to a region of guest address space */
> > int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr);
> >
> >-int map_regions_rw_cache(struct domain *d,
> >-                         gfn_t gfn,
> >-                         unsigned long nr,
> >-                         mfn_t mfn);
> >-
> >-int unmap_regions_rw_cache(struct domain *d,
> >-                           gfn_t gfn,
> >-                           unsigned long nr,
> >-                           mfn_t mfn);
> >+int map_regions_p2mt(struct domain *d,
> >+                     gfn_t gfn,
> >+                     unsigned long nr,
> >+                     mfn_t mfn,
> >+                     p2m_type_t p2mt);
> 
> Can you document the purpose of this function in the code? Something like:
> "Map the region in the guest p2m with a specific type (will affect the
> attributes of the region).".

Yes, I've added the following for v4:

/*
 * Map a region in the guest p2m with a specific p2m type.
 * The memory attributes will be derived from the p2m type.
 */

Thanks,
Edgar


> 
> >+
> >+int unmap_regions_p2mt(struct domain *d,
> >+                       gfn_t gfn,
> >+                       unsigned long nr,
> >+                       mfn_t mfn);
> >
> > int map_dev_mmio_region(struct domain *d,
> >                         gfn_t gfn,
> >
> 
> Regards,
> 
> -- 
> Julien Grall

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

[PATCH v3 3/6] xen/device-tree: Add __DT_MATCH macros without braces

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add __DT_MATCH macros without braces to allow the creation
of match descriptors with multiple combined match options.

Acked-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/include/xen/device_tree.h | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 3657ac2..da153a5 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -32,10 +32,15 @@ struct dt_device_match {
     const void *data;
 };
 
-#define DT_MATCH_PATH(p)                { .path = p }
-#define DT_MATCH_TYPE(typ)              { .type = typ }
-#define DT_MATCH_COMPATIBLE(compat)     { .compatible = compat }
-#define DT_MATCH_NOT_AVAILABLE()        { .not_available = 1 }
+#define __DT_MATCH_PATH(p)              .path = p
+#define __DT_MATCH_TYPE(typ)            .type = typ
+#define __DT_MATCH_COMPATIBLE(compat)   .compatible = compat
+#define __DT_MATCH_NOT_AVAILABLE()      .not_available = 1
+
+#define DT_MATCH_PATH(p)                { __DT_MATCH_PATH(p) }
+#define DT_MATCH_TYPE(typ)              { __DT_MATCH_TYPE(typ) }
+#define DT_MATCH_COMPATIBLE(compat)     { __DT_MATCH_COMPATIBLE(compat) }
+#define DT_MATCH_NOT_AVAILABLE()        { __DT_MATCH_NOT_AVAILABLE() }
 
 typedef u32 dt_phandle;
 
-- 
1.9.1


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

[PATCH v3 4/6] xen/device-tree: Make dt_match_node match props

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Make dt_match_node match for existing properties.
We only search for the existence of the properties, not
for specific values.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/common/device_tree.c      | 5 ++++-
 xen/include/xen/device_tree.h | 7 +++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index b39c8ca..1be074b 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -319,7 +319,7 @@ dt_match_node(const struct dt_device_match *matches,
         return NULL;
 
     while ( matches->path || matches->type ||
-            matches->compatible || matches->not_available )
+            matches->compatible || matches->not_available || matches->prop)
     {
         bool_t match = 1;
 
@@ -335,6 +335,9 @@ dt_match_node(const struct dt_device_match *matches,
         if ( matches->not_available )
             match &= !dt_device_is_available(node);
 
+        if ( matches->prop )
+            match &= dt_find_property(node, matches->prop, NULL) != NULL;
+
         if ( match )
             return matches;
         matches++;
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index da153a5..c71ddb9 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -29,6 +29,11 @@ struct dt_device_match {
     const char *type;
     const char *compatible;
     const bool_t not_available;
+    /*
+     * Property name to search for. We only search for the properties
+     * existence.
+     */
+    const char *prop;
     const void *data;
 };
 
@@ -36,11 +41,13 @@ struct dt_device_match {
 #define __DT_MATCH_TYPE(typ)            .type = typ
 #define __DT_MATCH_COMPATIBLE(compat)   .compatible = compat
 #define __DT_MATCH_NOT_AVAILABLE()      .not_available = 1
+#define __DT_MATCH_PROP(p)              .prop = p
 
 #define DT_MATCH_PATH(p)                { __DT_MATCH_PATH(p) }
 #define DT_MATCH_TYPE(typ)              { __DT_MATCH_TYPE(typ) }
 #define DT_MATCH_COMPATIBLE(compat)     { __DT_MATCH_COMPATIBLE(compat) }
 #define DT_MATCH_NOT_AVAILABLE()        { __DT_MATCH_NOT_AVAILABLE() }
+#define DT_MATCH_PROP(p)                { __DT_MATCH_PROP(p) }
 
 typedef u32 dt_phandle;
 
-- 
1.9.1


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

Re: [PATCH v3 4/6] xen/device-tree: Make dt_match_node match props

[Julien Grall]

Hi Edgar,

On 07/09/2016 08:56, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Make dt_match_node match for existing properties.
> We only search for the existence of the properties, not
> for specific values.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  xen/common/device_tree.c      | 5 ++++-
>  xen/include/xen/device_tree.h | 7 +++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index b39c8ca..1be074b 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -319,7 +319,7 @@ dt_match_node(const struct dt_device_match *matches,
>          return NULL;
>
>      while ( matches->path || matches->type ||
> -            matches->compatible || matches->not_available )
> +            matches->compatible || matches->not_available || matches->prop)
>      {
>          bool_t match = 1;
>
> @@ -335,6 +335,9 @@ dt_match_node(const struct dt_device_match *matches,
>          if ( matches->not_available )
>              match &= !dt_device_is_available(node);
>
> +        if ( matches->prop )
> +            match &= dt_find_property(node, matches->prop, NULL) != NULL;
> +
>          if ( match )
>              return matches;
>          matches++;
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index da153a5..c71ddb9 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -29,6 +29,11 @@ struct dt_device_match {
>      const char *type;
>      const char *compatible;
>      const bool_t not_available;
> +    /*
> +     * Property name to search for. We only search for the properties

NIT: s/the properties/the property/ I think same.
I think this also apply for the commit message?

The rest of the patch looks good to me:

Acked-by: Julien Grall <julien.grall@arm.com>

Regards,

> +     * existence.
> +     */
> +    const char *prop;
>      const void *data;
>  };
>
> @@ -36,11 +41,13 @@ struct dt_device_match {
>  #define __DT_MATCH_TYPE(typ)            .type = typ
>  #define __DT_MATCH_COMPATIBLE(compat)   .compatible = compat
>  #define __DT_MATCH_NOT_AVAILABLE()      .not_available = 1
> +#define __DT_MATCH_PROP(p)              .prop = p
>
>  #define DT_MATCH_PATH(p)                { __DT_MATCH_PATH(p) }
>  #define DT_MATCH_TYPE(typ)              { __DT_MATCH_TYPE(typ) }
>  #define DT_MATCH_COMPATIBLE(compat)     { __DT_MATCH_COMPATIBLE(compat) }
>  #define DT_MATCH_NOT_AVAILABLE()        { __DT_MATCH_NOT_AVAILABLE() }
> +#define DT_MATCH_PROP(p)                { __DT_MATCH_PROP(p) }
>
>  typedef u32 dt_phandle;
>
>

-- 
Julien Grall

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

Re: [PATCH v3 4/6] xen/device-tree: Make dt_match_node match props

[Edgar E. Iglesias]

On Fri, Sep 16, 2016 at 04:28:20PM +0200, Julien Grall wrote:
> Hi Edgar,
> 
> On 07/09/2016 08:56, Edgar E. Iglesias wrote:
> >From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> >Make dt_match_node match for existing properties.
> >We only search for the existence of the properties, not
> >for specific values.
> >
> >Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> >---
> > xen/common/device_tree.c      | 5 ++++-
> > xen/include/xen/device_tree.h | 7 +++++++
> > 2 files changed, 11 insertions(+), 1 deletion(-)
> >
> >diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> >index b39c8ca..1be074b 100644
> >--- a/xen/common/device_tree.c
> >+++ b/xen/common/device_tree.c
> >@@ -319,7 +319,7 @@ dt_match_node(const struct dt_device_match *matches,
> >         return NULL;
> >
> >     while ( matches->path || matches->type ||
> >-            matches->compatible || matches->not_available )
> >+            matches->compatible || matches->not_available || matches->prop)
> >     {
> >         bool_t match = 1;
> >
> >@@ -335,6 +335,9 @@ dt_match_node(const struct dt_device_match *matches,
> >         if ( matches->not_available )
> >             match &= !dt_device_is_available(node);
> >
> >+        if ( matches->prop )
> >+            match &= dt_find_property(node, matches->prop, NULL) != NULL;
> >+
> >         if ( match )
> >             return matches;
> >         matches++;
> >diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> >index da153a5..c71ddb9 100644
> >--- a/xen/include/xen/device_tree.h
> >+++ b/xen/include/xen/device_tree.h
> >@@ -29,6 +29,11 @@ struct dt_device_match {
> >     const char *type;
> >     const char *compatible;
> >     const bool_t not_available;
> >+    /*
> >+     * Property name to search for. We only search for the properties
> 
> NIT: s/the properties/the property/ I think same.
> I think this also apply for the commit message?

OK, I've changed the commit message to:
xen/device-tree: Make dt_match_node match props

Make dt_match_node match for a single existing property.
We only search for the existence of the property, not
for specific values.

And I've changed the comment to say "the property's existence"
which I think is correct but I'm not a native English speaker..

Cheers,
Edgar

> 
> The rest of the patch looks good to me:
> 
> Acked-by: Julien Grall <julien.grall@arm.com>
> 
> Regards,
> 
> >+     * existence.
> >+     */
> >+    const char *prop;
> >     const void *data;
> > };
> >
> >@@ -36,11 +41,13 @@ struct dt_device_match {
> > #define __DT_MATCH_TYPE(typ)            .type = typ
> > #define __DT_MATCH_COMPATIBLE(compat)   .compatible = compat
> > #define __DT_MATCH_NOT_AVAILABLE()      .not_available = 1
> >+#define __DT_MATCH_PROP(p)              .prop = p
> >
> > #define DT_MATCH_PATH(p)                { __DT_MATCH_PATH(p) }
> > #define DT_MATCH_TYPE(typ)              { __DT_MATCH_TYPE(typ) }
> > #define DT_MATCH_COMPATIBLE(compat)     { __DT_MATCH_COMPATIBLE(compat) }
> > #define DT_MATCH_NOT_AVAILABLE()        { __DT_MATCH_NOT_AVAILABLE() }
> >+#define DT_MATCH_PROP(p)                { __DT_MATCH_PROP(p) }
> >
> > typedef u32 dt_phandle;
> >
> >
> 
> -- 
> Julien Grall

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

[PATCH v3 5/6] xen/arm: domain_build: Plumb for different mapping attributes

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add plumbing for passing around mapping attributes. This
is in preparation to allow us to differentiate the attributes
for specific device nodes.

We still use the same DEVICE mappings for all nodes so this
patch has no functional change.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/arch/arm/domain_build.c | 42 +++++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f022342..bbe4895 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -42,6 +42,12 @@ static void __init parse_dom0_mem(const char *s)
 }
 custom_param("dom0_mem", parse_dom0_mem);
 
+struct map_range_data
+{
+    struct domain *d;
+    p2m_type_t p2mt;
+};
+
 //#define DEBUG_11_ALLOCATION
 #ifdef DEBUG_11_ALLOCATION
 # define D11PRINT(fmt, args...) printk(XENLOG_DEBUG fmt, ##args)
@@ -974,7 +980,8 @@ static int map_range_to_domain(const struct dt_device_node *dev,
                                u64 addr, u64 len,
                                void *data)
 {
-    struct domain *d = data;
+    struct map_range_data *mr_data = data;
+    struct domain *d = mr_data->d;
     bool_t need_mapping = !dt_device_for_passthrough(dev);
     int res;
 
@@ -991,10 +998,12 @@ static int map_range_to_domain(const struct dt_device_node *dev,
 
     if ( need_mapping )
     {
-        res = map_mmio_regions(d,
+        res = map_regions_p2mt(d,
                                _gfn(paddr_to_pfn(addr)),
                                DIV_ROUND_UP(len, PAGE_SIZE),
-                               _mfn(paddr_to_pfn(addr)));
+                               _mfn(paddr_to_pfn(addr)),
+                               mr_data->p2mt);
+
         if ( res < 0 )
         {
             printk(XENLOG_ERR "Unable to map 0x%"PRIx64
@@ -1005,7 +1014,8 @@ static int map_range_to_domain(const struct dt_device_node *dev,
         }
     }
 
-    dt_dprintk("  - MMIO: %010"PRIx64" - %010"PRIx64"\n", addr, addr + len);
+    dt_dprintk("  - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
+               addr, addr + len, mr_data->p2mt);
 
     return 0;
 }
@@ -1016,8 +1026,10 @@ static int map_range_to_domain(const struct dt_device_node *dev,
  * the child resources available to domain 0.
  */
 static int map_device_children(struct domain *d,
-                               const struct dt_device_node *dev)
+                               const struct dt_device_node *dev,
+                               p2m_type_t p2mt)
 {
+    struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
     int ret;
 
     if ( dt_device_type_is_equal(dev, "pci") )
@@ -1029,7 +1041,7 @@ static int map_device_children(struct domain *d,
         if ( ret < 0 )
             return ret;
 
-        ret = dt_for_each_range(dev, &map_range_to_domain, d);
+        ret = dt_for_each_range(dev, &map_range_to_domain, &mr_data);
         if ( ret < 0 )
             return ret;
     }
@@ -1045,7 +1057,8 @@ static int map_device_children(struct domain *d,
  *  - Assign the device to the guest if it's protected by an IOMMU
  *  - Map the IRQs and iomem regions to DOM0
  */
-static int handle_device(struct domain *d, struct dt_device_node *dev)
+static int handle_device(struct domain *d, struct dt_device_node *dev,
+                         p2m_type_t p2mt)
 {
     unsigned int nirq;
     unsigned int naddr;
@@ -1111,6 +1124,7 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
     /* Give permission and map MMIOs */
     for ( i = 0; i < naddr; i++ )
     {
+        struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
         res = dt_device_get_address(dev, i, &addr, &size);
         if ( res )
         {
@@ -1119,12 +1133,12 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
             return res;
         }
 
-        res = map_range_to_domain(dev, addr, size, d);
+        res = map_range_to_domain(dev, addr, size, &mr_data);
         if ( res )
             return res;
     }
 
-    res = map_device_children(d, dev);
+    res = map_device_children(d, dev, p2mt);
     if ( res )
         return res;
 
@@ -1132,7 +1146,8 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
 }
 
 static int handle_node(struct domain *d, struct kernel_info *kinfo,
-                       struct dt_device_node *node)
+                       struct dt_device_node *node,
+                       p2m_type_t p2mt)
 {
     static const struct dt_device_match skip_matches[] __initconst =
     {
@@ -1219,7 +1234,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
                "WARNING: Path %s is reserved, skip the node as we may re-use the path.\n",
                path);
 
-    res = handle_device(d, node);
+    res = handle_device(d, node, p2mt);
     if ( res)
         return res;
 
@@ -1241,7 +1256,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
 
     for ( child = node->child; child != NULL; child = child->sibling )
     {
-        res = handle_node(d, kinfo, child);
+        res = handle_node(d, kinfo, child, p2mt);
         if ( res )
             return res;
     }
@@ -1273,6 +1288,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
 
 static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
 {
+    const p2m_type_t default_p2mt = p2m_mmio_direct_nc;
     const void *fdt;
     int new_size;
     int ret;
@@ -1292,7 +1308,7 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
 
     fdt_finish_reservemap(kinfo->fdt);
 
-    ret = handle_node(d, kinfo, dt_host);
+    ret = handle_node(d, kinfo, dt_host, default_p2mt);
     if ( ret )
         goto err;
 
-- 
1.9.1


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

Re: [PATCH v3 5/6] xen/arm: domain_build: Plumb for different mapping attributes

[Julien Grall]

Hi Edgar,

On 07/09/2016 08:56, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Add plumbing for passing around mapping attributes. This
> is in preparation to allow us to differentiate the attributes
> for specific device nodes.
>
> We still use the same DEVICE mappings for all nodes so this
> patch has no functional change.

I would mention somewhere (commit message, code) that unless stated, the 
children nodes inherit of the p2m type of the parent.

With that:

Acked-by: Julien Grall <julien.grall@arm.com>

Regards,

>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  xen/arch/arm/domain_build.c | 42 +++++++++++++++++++++++++++++-------------
>  1 file changed, 29 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index f022342..bbe4895 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -42,6 +42,12 @@ static void __init parse_dom0_mem(const char *s)
>  }
>  custom_param("dom0_mem", parse_dom0_mem);
>
> +struct map_range_data
> +{
> +    struct domain *d;
> +    p2m_type_t p2mt;
> +};
> +
>  //#define DEBUG_11_ALLOCATION
>  #ifdef DEBUG_11_ALLOCATION
>  # define D11PRINT(fmt, args...) printk(XENLOG_DEBUG fmt, ##args)
> @@ -974,7 +980,8 @@ static int map_range_to_domain(const struct dt_device_node *dev,
>                                 u64 addr, u64 len,
>                                 void *data)
>  {
> -    struct domain *d = data;
> +    struct map_range_data *mr_data = data;
> +    struct domain *d = mr_data->d;
>      bool_t need_mapping = !dt_device_for_passthrough(dev);
>      int res;
>
> @@ -991,10 +998,12 @@ static int map_range_to_domain(const struct dt_device_node *dev,
>
>      if ( need_mapping )
>      {
> -        res = map_mmio_regions(d,
> +        res = map_regions_p2mt(d,
>                                 _gfn(paddr_to_pfn(addr)),
>                                 DIV_ROUND_UP(len, PAGE_SIZE),
> -                               _mfn(paddr_to_pfn(addr)));
> +                               _mfn(paddr_to_pfn(addr)),
> +                               mr_data->p2mt);
> +
>          if ( res < 0 )
>          {
>              printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> @@ -1005,7 +1014,8 @@ static int map_range_to_domain(const struct dt_device_node *dev,
>          }
>      }
>
> -    dt_dprintk("  - MMIO: %010"PRIx64" - %010"PRIx64"\n", addr, addr + len);
> +    dt_dprintk("  - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
> +               addr, addr + len, mr_data->p2mt);
>
>      return 0;
>  }
> @@ -1016,8 +1026,10 @@ static int map_range_to_domain(const struct dt_device_node *dev,
>   * the child resources available to domain 0.
>   */
>  static int map_device_children(struct domain *d,
> -                               const struct dt_device_node *dev)
> +                               const struct dt_device_node *dev,
> +                               p2m_type_t p2mt)
>  {
> +    struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
>      int ret;
>
>      if ( dt_device_type_is_equal(dev, "pci") )
> @@ -1029,7 +1041,7 @@ static int map_device_children(struct domain *d,
>          if ( ret < 0 )
>              return ret;
>
> -        ret = dt_for_each_range(dev, &map_range_to_domain, d);
> +        ret = dt_for_each_range(dev, &map_range_to_domain, &mr_data);
>          if ( ret < 0 )
>              return ret;
>      }
> @@ -1045,7 +1057,8 @@ static int map_device_children(struct domain *d,
>   *  - Assign the device to the guest if it's protected by an IOMMU
>   *  - Map the IRQs and iomem regions to DOM0
>   */
> -static int handle_device(struct domain *d, struct dt_device_node *dev)
> +static int handle_device(struct domain *d, struct dt_device_node *dev,
> +                         p2m_type_t p2mt)
>  {
>      unsigned int nirq;
>      unsigned int naddr;
> @@ -1111,6 +1124,7 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
>      /* Give permission and map MMIOs */
>      for ( i = 0; i < naddr; i++ )
>      {
> +        struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
>          res = dt_device_get_address(dev, i, &addr, &size);
>          if ( res )
>          {
> @@ -1119,12 +1133,12 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
>              return res;
>          }
>
> -        res = map_range_to_domain(dev, addr, size, d);
> +        res = map_range_to_domain(dev, addr, size, &mr_data);
>          if ( res )
>              return res;
>      }
>
> -    res = map_device_children(d, dev);
> +    res = map_device_children(d, dev, p2mt);
>      if ( res )
>          return res;
>
> @@ -1132,7 +1146,8 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
>  }
>
>  static int handle_node(struct domain *d, struct kernel_info *kinfo,
> -                       struct dt_device_node *node)
> +                       struct dt_device_node *node,
> +                       p2m_type_t p2mt)
>  {
>      static const struct dt_device_match skip_matches[] __initconst =
>      {
> @@ -1219,7 +1234,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>                 "WARNING: Path %s is reserved, skip the node as we may re-use the path.\n",
>                 path);
>
> -    res = handle_device(d, node);
> +    res = handle_device(d, node, p2mt);
>      if ( res)
>          return res;
>
> @@ -1241,7 +1256,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>
>      for ( child = node->child; child != NULL; child = child->sibling )
>      {
> -        res = handle_node(d, kinfo, child);
> +        res = handle_node(d, kinfo, child, p2mt);
>          if ( res )
>              return res;
>      }
> @@ -1273,6 +1288,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
>
>  static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>  {
> +    const p2m_type_t default_p2mt = p2m_mmio_direct_nc;
>      const void *fdt;
>      int new_size;
>      int ret;
> @@ -1292,7 +1308,7 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
>
>      fdt_finish_reservemap(kinfo->fdt);
>
> -    ret = handle_node(d, kinfo, dt_host);
> +    ret = handle_node(d, kinfo, dt_host, default_p2mt);
>      if ( ret )
>          goto err;
>
>

-- 
Julien Grall

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

Re: [PATCH v3 5/6] xen/arm: domain_build: Plumb for different mapping attributes

[Edgar E. Iglesias]

On Fri, Sep 16, 2016 at 04:31:20PM +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 plumbing for passing around mapping attributes. This
> >is in preparation to allow us to differentiate the attributes
> >for specific device nodes.
> >
> >We still use the same DEVICE mappings for all nodes so this
> >patch has no functional change.
> 
> I would mention somewhere (commit message, code) that unless stated, the
> children nodes inherit of the p2m type of the parent.

Yes, agreed. I've added a comment in the commit message.
Patch nr 6, when we actually start overriding types adds
a comment about the inheritance in the code that matches
nodes and provides specific types.

Thanks,
Edgar


> 
> With that:
> 
> Acked-by: Julien Grall <julien.grall@arm.com>
> 
> Regards,
> 
> >
> >Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> >---
> > xen/arch/arm/domain_build.c | 42 +++++++++++++++++++++++++++++-------------
> > 1 file changed, 29 insertions(+), 13 deletions(-)
> >
> >diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >index f022342..bbe4895 100644
> >--- a/xen/arch/arm/domain_build.c
> >+++ b/xen/arch/arm/domain_build.c
> >@@ -42,6 +42,12 @@ static void __init parse_dom0_mem(const char *s)
> > }
> > custom_param("dom0_mem", parse_dom0_mem);
> >
> >+struct map_range_data
> >+{
> >+    struct domain *d;
> >+    p2m_type_t p2mt;
> >+};
> >+
> > //#define DEBUG_11_ALLOCATION
> > #ifdef DEBUG_11_ALLOCATION
> > # define D11PRINT(fmt, args...) printk(XENLOG_DEBUG fmt, ##args)
> >@@ -974,7 +980,8 @@ static int map_range_to_domain(const struct dt_device_node *dev,
> >                                u64 addr, u64 len,
> >                                void *data)
> > {
> >-    struct domain *d = data;
> >+    struct map_range_data *mr_data = data;
> >+    struct domain *d = mr_data->d;
> >     bool_t need_mapping = !dt_device_for_passthrough(dev);
> >     int res;
> >
> >@@ -991,10 +998,12 @@ static int map_range_to_domain(const struct dt_device_node *dev,
> >
> >     if ( need_mapping )
> >     {
> >-        res = map_mmio_regions(d,
> >+        res = map_regions_p2mt(d,
> >                                _gfn(paddr_to_pfn(addr)),
> >                                DIV_ROUND_UP(len, PAGE_SIZE),
> >-                               _mfn(paddr_to_pfn(addr)));
> >+                               _mfn(paddr_to_pfn(addr)),
> >+                               mr_data->p2mt);
> >+
> >         if ( res < 0 )
> >         {
> >             printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> >@@ -1005,7 +1014,8 @@ static int map_range_to_domain(const struct dt_device_node *dev,
> >         }
> >     }
> >
> >-    dt_dprintk("  - MMIO: %010"PRIx64" - %010"PRIx64"\n", addr, addr + len);
> >+    dt_dprintk("  - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
> >+               addr, addr + len, mr_data->p2mt);
> >
> >     return 0;
> > }
> >@@ -1016,8 +1026,10 @@ static int map_range_to_domain(const struct dt_device_node *dev,
> >  * the child resources available to domain 0.
> >  */
> > static int map_device_children(struct domain *d,
> >-                               const struct dt_device_node *dev)
> >+                               const struct dt_device_node *dev,
> >+                               p2m_type_t p2mt)
> > {
> >+    struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
> >     int ret;
> >
> >     if ( dt_device_type_is_equal(dev, "pci") )
> >@@ -1029,7 +1041,7 @@ static int map_device_children(struct domain *d,
> >         if ( ret < 0 )
> >             return ret;
> >
> >-        ret = dt_for_each_range(dev, &map_range_to_domain, d);
> >+        ret = dt_for_each_range(dev, &map_range_to_domain, &mr_data);
> >         if ( ret < 0 )
> >             return ret;
> >     }
> >@@ -1045,7 +1057,8 @@ static int map_device_children(struct domain *d,
> >  *  - Assign the device to the guest if it's protected by an IOMMU
> >  *  - Map the IRQs and iomem regions to DOM0
> >  */
> >-static int handle_device(struct domain *d, struct dt_device_node *dev)
> >+static int handle_device(struct domain *d, struct dt_device_node *dev,
> >+                         p2m_type_t p2mt)
> > {
> >     unsigned int nirq;
> >     unsigned int naddr;
> >@@ -1111,6 +1124,7 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
> >     /* Give permission and map MMIOs */
> >     for ( i = 0; i < naddr; i++ )
> >     {
> >+        struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
> >         res = dt_device_get_address(dev, i, &addr, &size);
> >         if ( res )
> >         {
> >@@ -1119,12 +1133,12 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
> >             return res;
> >         }
> >
> >-        res = map_range_to_domain(dev, addr, size, d);
> >+        res = map_range_to_domain(dev, addr, size, &mr_data);
> >         if ( res )
> >             return res;
> >     }
> >
> >-    res = map_device_children(d, dev);
> >+    res = map_device_children(d, dev, p2mt);
> >     if ( res )
> >         return res;
> >
> >@@ -1132,7 +1146,8 @@ static int handle_device(struct domain *d, struct dt_device_node *dev)
> > }
> >
> > static int handle_node(struct domain *d, struct kernel_info *kinfo,
> >-                       struct dt_device_node *node)
> >+                       struct dt_device_node *node,
> >+                       p2m_type_t p2mt)
> > {
> >     static const struct dt_device_match skip_matches[] __initconst =
> >     {
> >@@ -1219,7 +1234,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
> >                "WARNING: Path %s is reserved, skip the node as we may re-use the path.\n",
> >                path);
> >
> >-    res = handle_device(d, node);
> >+    res = handle_device(d, node, p2mt);
> >     if ( res)
> >         return res;
> >
> >@@ -1241,7 +1256,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
> >
> >     for ( child = node->child; child != NULL; child = child->sibling )
> >     {
> >-        res = handle_node(d, kinfo, child);
> >+        res = handle_node(d, kinfo, child, p2mt);
> >         if ( res )
> >             return res;
> >     }
> >@@ -1273,6 +1288,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
> >
> > static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
> > {
> >+    const p2m_type_t default_p2mt = p2m_mmio_direct_nc;
> >     const void *fdt;
> >     int new_size;
> >     int ret;
> >@@ -1292,7 +1308,7 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
> >
> >     fdt_finish_reservemap(kinfo->fdt);
> >
> >-    ret = handle_node(d, kinfo, dt_host);
> >+    ret = handle_node(d, kinfo, dt_host, default_p2mt);
> >     if ( ret )
> >         goto err;
> >
> >
> 
> -- 
> Julien Grall

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

[PATCH v3 6/6] xen/arm: Map mmio-sram nodes as un-cached memory

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Map mmio-sram nodes as un-cached memory. If the node
has set the no-memory-wc property, we map it as device.

The DTS bindings for mmio-sram nodes can be found in the
Linux tree under Documentation/devicetree/bindings/sram/sram.txt.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/arch/arm/domain_build.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index bbe4895..f55d67f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -48,6 +48,20 @@ struct map_range_data
     p2m_type_t p2mt;
 };
 
+static const struct dt_device_match dev_map_attrs[] __initconst =
+{
+    {
+        __DT_MATCH_COMPATIBLE("mmio-sram"),
+        __DT_MATCH_PROP("no-memory-wc"),
+        .data = (void *) (uintptr_t) p2m_mmio_direct_nc,
+    },
+    {
+        __DT_MATCH_COMPATIBLE("mmio-sram"),
+        .data = (void *) (uintptr_t) p2m_mmio_direct_c,
+    },
+    { /* sentinel */ },
+};
+
 //#define DEBUG_11_ALLOCATION
 #ifdef DEBUG_11_ALLOCATION
 # define D11PRINT(fmt, args...) printk(XENLOG_DEBUG fmt, ##args)
@@ -1145,6 +1159,21 @@ static int handle_device(struct domain *d, struct dt_device_node *dev,
     return 0;
 }
 
+static p2m_type_t lookup_map_attr(struct dt_device_node *node,
+                                  p2m_type_t parent_p2mt)
+{
+    const struct dt_device_match *r;
+
+    /* Search and if nothing matches, use the parent's attributes.  */
+    r = dt_match_node(dev_map_attrs, node);
+
+    /*
+     * If this node does not dictate specific mapping attributes,
+     * it inherits its parent's attributes.
+     */
+    return r ? (uintptr_t) r->data : parent_p2mt;
+}
+
 static int handle_node(struct domain *d, struct kernel_info *kinfo,
                        struct dt_device_node *node,
                        p2m_type_t p2mt)
@@ -1234,6 +1263,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
                "WARNING: Path %s is reserved, skip the node as we may re-use the path.\n",
                path);
 
+    p2mt = lookup_map_attr(node, p2mt);
     res = handle_device(d, node, p2mt);
     if ( res)
         return res;
-- 
1.9.1


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