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

[RFC for-4.8 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 MEMORY, cached and with RWX perms.

Dom0 can then choose to further restrict these mappings if needed.
We only look at the outer mmio-sram region. The sub-area nodes that
describe allocations within the mmio-sram region are only meaningful
to the guest AFAICT.

In an attempt to avoid growing the already fairly large domain_build.c
file, I've tried to implement a distributed way to deal with these kind
of special/custom mapping needs. These can live outside of domain_build.c
and are registerd by means of a .map method in the device_spec.

If this approach is not good, I'm happy to bin it and try something else.

Comments welcome!

Best regards,
Edgar

Edgar E. Iglesias (6):
  xen/arm: Add device_get_desc()
  xen/arm: Add an optional map function to the device descriptor
  xen/arm: Add a DEVICE_MEMORY class
  xen/arm: Add helper functions to map RWX memory regions
  xen/arm: Add an mmio-sram device
  xen/arm: Avoid multiple dev class lookups in handle_node

 xen/arch/arm/Makefile        |  1 +
 xen/arch/arm/device.c        | 15 ++++++++
 xen/arch/arm/domain_build.c  | 19 +++++++--
 xen/arch/arm/mmio-sram.c     | 92 ++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/p2m.c           | 26 +++++++++++++
 xen/include/asm-arm/device.h | 12 ++++++
 xen/include/asm-arm/p2m.h    | 10 +++++
 7 files changed, 172 insertions(+), 3 deletions(-)
 create mode 100644 xen/arch/arm/mmio-sram.c

-- 
2.5.0


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

[RFC for-4.8 1/6] xen/arm: Add device_get_desc()

[Edgar E. Iglesias]

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

Add device_get_desc, a function to lookup the device descriptor
for a DT node. This is in preparation for adding per device
mapping implementations.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/arch/arm/device.c        | 15 +++++++++++++++
 xen/include/asm-arm/device.h |  1 +
 2 files changed, 16 insertions(+)

diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index a0072c1..1b934b9 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -83,6 +83,21 @@ enum device_class device_get_class(const struct dt_device_node *dev)
     return DEVICE_UNKNOWN;
 }
 
+const struct device_desc *device_get_desc(const struct dt_device_node *dev)
+{
+    const struct device_desc *desc;
+
+    ASSERT(dev != NULL);
+
+    for ( desc = _sdevice; desc != _edevice; desc++ )
+    {
+        if ( dt_match_node(desc->dt_match, dev) )
+            return desc;
+    }
+
+    return NULL;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index 6734ae8..1a40a02 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -89,6 +89,7 @@ int __init device_init(struct dt_device_node *dev, enum device_class class,
  * Return the device type on success or DEVICE_ANY on failure
  */
 enum device_class device_get_class(const struct dt_device_node *dev);
+const struct device_desc *device_get_desc(const struct dt_device_node *dev);
 
 #define DT_DEVICE_START(_name, _namestr, _class)                    \
 static const struct device_desc __dev_desc_##_name __used           \
-- 
2.5.0


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

[RFC for-4.8 2/6] xen/arm: Add an optional map function to the device descriptor

[Edgar E. Iglesias]

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

Add an optional map function to the device descriptor. If
registered, the map function will be called to do custom
device specific mappings of the device. If not registered,
the generic DT version (handle_device) will be used.

This is in preparation for adding support for "mmio-sram"
memory that needs to be mapped as MEMORY and not DEVICE.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/arch/arm/domain_build.c  | 13 ++++++++++++-
 xen/include/asm-arm/device.h | 10 ++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 00dc07a..15b6dbe 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1212,6 +1212,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         DT_MATCH_PATH("/hypervisor"),
         { /* sentinel */ },
     };
+    const struct device_desc *desc;
     struct dt_device_node *child;
     int res;
     const char *name;
@@ -1233,6 +1234,8 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         return 0;
     }
 
+    desc = device_get_desc(node);
+
     /*
      * Replace these nodes with our own. Note that the original may be
      * used_by DOMID_XEN so this check comes first.
@@ -1268,7 +1271,15 @@ 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);
+    if ( desc && desc->map )
+    {
+        res = desc->map(d, node);
+    }
+    else
+    {
+        res = handle_device(d, node);
+    }
+
     if ( res)
         return res;
 
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index 1a40a02..98b9fe1 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -48,6 +48,16 @@ struct device_desc {
     const struct dt_device_match *dt_match;
     /* Device initialization */
     int (*init)(struct dt_device_node *dev, const void *data);
+
+    /**
+     *  map - Custom map function to map a devices memory regions and IRQs
+     *  @d: Domain to map device into
+     *  @dev: Device tree node representing the device
+     *
+     *  OPTIONAL: If not set the generic DT code will take care of creating
+     *  the mappings.
+     */
+    int (*map)(struct domain *d, struct dt_device_node *dev);
 };
 
 struct acpi_device_desc {
-- 
2.5.0


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

[RFC for-4.8 3/6] xen/arm: Add a DEVICE_MEMORY class

[Edgar E. Iglesias]

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

Add a DEVICE_MEMORY class to be used for things like "mmio-sram"
or memory controllers.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/include/asm-arm/device.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index 98b9fe1..99073bf 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -35,6 +35,7 @@ enum device_class
     DEVICE_SERIAL,
     DEVICE_IOMMU,
     DEVICE_GIC,
+    DEVICE_MEMORY,
     /* Use for error */
     DEVICE_UNKNOWN,
 };
-- 
2.5.0


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

[RFC for-4.8 4/6] xen/arm: Add helper functions to map RWX memory regions

[Edgar E. Iglesias]

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

Create a helper function to map regions as MEMORY with
cached attributes and read-write-execute permissions.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/arch/arm/p2m.c        | 26 ++++++++++++++++++++++++++
 xen/include/asm-arm/p2m.h | 10 ++++++++++
 2 files changed, 36 insertions(+)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index db21433..7e788f9 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1219,6 +1219,32 @@ int p2m_populate_ram(struct domain *d,
                              d->arch.p2m.default_access);
 }
 
+int map_regions_rwx_cache(struct domain *d,
+                         unsigned long start_gfn,
+                         unsigned long nr,
+                         unsigned long mfn)
+{
+    return apply_p2m_changes(d, INSERT,
+                             pfn_to_paddr(start_gfn),
+                             pfn_to_paddr(start_gfn + nr),
+                             pfn_to_paddr(mfn),
+                             MATTR_MEM, 0, p2m_ram_rw,
+                             p2m_access_rwx);
+}
+
+int unmap_regions_rwx_cache(struct domain *d,
+                           unsigned long start_gfn,
+                           unsigned long nr,
+                           unsigned long mfn)
+{
+    return apply_p2m_changes(d, REMOVE,
+                             pfn_to_paddr(start_gfn),
+                             pfn_to_paddr(start_gfn + nr),
+                             pfn_to_paddr(mfn),
+                             MATTR_MEM, 0, p2m_invalid,
+                             p2m_access_rwx);
+}
+
 int map_regions_rw_cache(struct domain *d,
                          unsigned long start_gfn,
                          unsigned long nr,
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index d240d1e..294050e 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -144,6 +144,16 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn);
 /* Setup p2m RAM mapping for domain d from start-end. */
 int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
 
+int map_regions_rwx_cache(struct domain *d,
+                          unsigned long start_gfn,
+                          unsigned long nr_mfns,
+                          unsigned long mfn);
+
+int unmap_regions_rwx_cache(struct domain *d,
+                            unsigned long start_gfn,
+                            unsigned long nr_mfns,
+                            unsigned long mfn);
+
 int map_regions_rw_cache(struct domain *d,
                          unsigned long start_gfn,
                          unsigned long nr_mfns,
-- 
2.5.0


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

[RFC for-4.8 5/6] xen/arm: Add an mmio-sram device

[Edgar E. Iglesias]

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

Add an mmio-sram device. This device takes care of mapping
mmio-sram regions as MEMORY as opposed to the generic DT
mappings as DEVICE.

We only map the outer regions as children of mmio-sram nodes
describe allocations within it that really only mean something
to the guest.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 xen/arch/arm/Makefile    |  1 +
 xen/arch/arm/mmio-sram.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)
 create mode 100644 xen/arch/arm/mmio-sram.c

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index ead0cc0..224c9ae 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -18,6 +18,7 @@ obj-y += io.o
 obj-y += irq.o
 obj-y += kernel.o
 obj-y += mm.o
+obj-y += mmio-sram.o
 obj-y += p2m.o
 obj-y += percpu.o
 obj-y += guestcopy.o
diff --git a/xen/arch/arm/mmio-sram.c b/xen/arch/arm/mmio-sram.c
new file mode 100644
index 0000000..e0dbf7c
--- /dev/null
+++ b/xen/arch/arm/mmio-sram.c
@@ -0,0 +1,92 @@
+/*
+ * xen/arch/arm/mmio-sram.c
+ *
+ * MMIO-SRAM
+ *
+ * Edgar E. Iglesias <edgar.iglesias@xilinx.com>
+ * Copyright (c) 2016 Xilinx Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <xen/config.h>
+#include <xen/lib.h>
+#include <xen/init.h>
+#include <xen/mm.h>
+#include <xen/iocap.h>
+#include <xen/sched.h>
+#include <xen/errno.h>
+#include <xen/list.h>
+#include <xen/device_tree.h>
+#include <xen/libfdt/libfdt.h>
+#include <xen/sizes.h>
+#include <asm/domain.h>
+#include <asm/platform.h>
+#include <asm/device.h>
+
+// #define DEBUG_DT
+
+#ifdef DEBUG_DT
+# define DPRINT(fmt, args...) printk(XENLOG_DEBUG fmt, ##args)
+#else
+# define DPRINT(fmt, args...) do {} while ( 0 )
+#endif
+
+static int sram_map(struct domain *d, struct dt_device_node *dev)
+{
+	unsigned int naddr;
+	u64 addr, size;
+	int res = 0;
+	int i;
+
+	naddr = dt_number_of_address(dev);
+
+	/* Give perms to access the region.  */
+	res = iomem_permit_access(d, paddr_to_pfn(addr),
+				  paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
+
+	/*
+	 * Map the memory regions.
+	 *
+	 * We only map the outer regions. Child regions represent
+	 * sub allocations that really are the guest's business.
+	 */
+	for (i = 0; i < naddr; i++)
+	{
+		res = dt_device_get_address(dev, i, &addr, &size);
+		if (res) {
+			printk(XENLOG_ERR
+				"Unable to retrieve address %u for %s\n",
+				i, dt_node_full_name(dev));
+			return res;
+		}
+
+		DPRINT("  - MEMORY: %s %010"PRIx64" - %010"PRIx64"\n",
+		       dt_node_full_name(dev), addr, addr + size);
+		res = map_regions_rwx_cache(d, paddr_to_pfn(addr & PAGE_MASK),
+					    DIV_ROUND_UP(size, PAGE_SIZE),
+					    paddr_to_pfn(addr & PAGE_MASK));
+		if (res)
+			return res;
+	}
+	return res;
+}
+
+static const struct dt_device_match sram_dt_match[] __initconst =
+{
+	DT_MATCH_COMPATIBLE("mmio-sram"),
+	{ /* sentinel */ },
+};
+
+DT_DEVICE_START(sram, "MMIO-SRAM", DEVICE_MEMORY)
+	.dt_match = sram_dt_match,
+	.map = sram_map,
+DT_DEVICE_END
-- 
2.5.0


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

[RFC for-4.8 6/6] xen/arm: Avoid multiple dev class lookups in handle_node

[Edgar E. Iglesias]

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

Avoid looking up the device class multiple times in handle_node().
This optimization should not have any functional change.

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

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 15b6dbe..65c2df7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1213,6 +1213,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
         { /* sentinel */ },
     };
     const struct device_desc *desc;
+    enum device_class dev_class;
     struct dt_device_node *child;
     int res;
     const char *name;
@@ -1235,12 +1236,13 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
     }
 
     desc = device_get_desc(node);
+    dev_class = desc ? desc->class : DEVICE_UNKNOWN;
 
     /*
      * Replace these nodes with our own. Note that the original may be
      * used_by DOMID_XEN so this check comes first.
      */
-    if ( device_get_class(node) == DEVICE_GIC )
+    if ( dev_class == DEVICE_GIC )
         return make_gic_node(d, kinfo->fdt, node);
     if ( dt_match_node(timer_matches, node) )
         return make_timer_node(d, kinfo->fdt, node);
@@ -1256,7 +1258,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
      * Even if the IOMMU device is not used by Xen, it should not be
      * passthrough to DOM0
      */
-    if ( device_get_class(node) == DEVICE_IOMMU )
+    if ( dev_class == DEVICE_IOMMU )
     {
         DPRINT(" IOMMU, skip it\n");
         return 0;
-- 
2.5.0


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

Re: [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Julien Grall]

Hello Edgar,

I have CCed a couple of people from ARM to get more input on it.

On 20/05/16 16:51, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> This series adds support for mapping mmio-sram nodes into dom0
> as MEMORY, cached and with RWX perms.

Can you explain why you chose to map those nodes as MEMORY, cached and 
with RWX perms?

>
> Dom0 can then choose to further restrict these mappings if needed.
> We only look at the outer mmio-sram region. The sub-area nodes that
> describe allocations within the mmio-sram region are only meaningful
> to the guest AFAICT.
>
> In an attempt to avoid growing the already fairly large domain_build.c
> file, I've tried to implement a distributed way to deal with these kind
> of special/custom mapping needs. These can live outside of domain_build.c
> and are registerd by means of a .map method in the device_spec.
>
> If this approach is not good, I'm happy to bin it and try something else.

We will have a similar problem when using ACPI for DOM0 or mapping a 
such MMIO to the guest. The hypercalls XENMAPSPACE_dev_mmio and 
XEN_DOMCTL_memory_mapping do not provide enough information to know the 
attribute to be used for mapping.

MMIO are always mapped in Stage-2 with Device_nGnRE, which is quite 
restrictive. This would also impact any MMIO regions, such as the video 
RAM buffer, that could be mapped write-combine.

After reading the ARM ARM (B2.8.2 ARM DII 0486.i), I think we could 
relax the stage-2 mapping by using Device_GRE for all the device MMIOs 
but the GIC.

We have to keep the GIC MMIO with the most restrictive memory attribute 
to avoid potential side-effect when Xen is switching between multiple 
vCPUs. All the other devices will be exclusive to a specific guest, so 
the guest can handle the device the way it wants. This may require some 
extra-care when reassigning the device to another domain.

Edgar, would Device_GRE be fine for you?

Note, that XENMAPSPACE_dev_mmio has been introduced in Xen 4.7 (which is 
due in a couple of weeks) and part of the stable ABI. So if it is not 
possible to relax the memory attribute, it might be worth to think 
fixing/reverting the hypercall for 4.7. Otherwise we would have to 
introduce a new one in the next release.

Regards,

-- 
Julien Grall

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

Re: [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Edgar E. Iglesias]

On Mon, May 23, 2016 at 11:29:31AM +0100, Julien Grall wrote:
> Hello Edgar,
> 
> I have CCed a couple of people from ARM to get more input on it.

Thanks Julien,


> On 20/05/16 16:51, Edgar E. Iglesias wrote:
> >From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> >This series adds support for mapping mmio-sram nodes into dom0
> >as MEMORY, cached and with RWX perms.
> 
> Can you explain why you chose to map those nodes as MEMORY, cached and with
> RWX perms?

My understanding is that these mmio-sram nodes are allowed to be treated as
Normal memory by the guest OS.
Guests could potentially do any kind of memory like operations on them.

In our specific case, dom0 won't execute code from these regions but
Linux/dom0 ends up using standard memcpy/memset/x functions (not
memcpy_fromio and friends) on the regions.

We saw issues with memset doing cache operations on DEVICE memory in
the past (external data aborts). I can't find the text in the ARM ARM
regarding this at the moment but IIRC, dc ops on device mem are not
allowed.

We could also see alignment problems, as the alignment checks for
ARMv8 differ somewhat between normal memory and device memory.

A third reason is performance. The rules for speculative accesses
and caching are different between device and normal memory.

So I opted for the most permissive attributes thinking that dom0 can
apply further restrictions if it needs to do so.


> >Dom0 can then choose to further restrict these mappings if needed.
> >We only look at the outer mmio-sram region. The sub-area nodes that
> >describe allocations within the mmio-sram region are only meaningful
> >to the guest AFAICT.
> >
> >In an attempt to avoid growing the already fairly large domain_build.c
> >file, I've tried to implement a distributed way to deal with these kind
> >of special/custom mapping needs. These can live outside of domain_build.c
> >and are registerd by means of a .map method in the device_spec.
> >
> >If this approach is not good, I'm happy to bin it and try something else.
> 
> We will have a similar problem when using ACPI for DOM0 or mapping a such
> MMIO to the guest. The hypercalls XENMAPSPACE_dev_mmio and
> XEN_DOMCTL_memory_mapping do not provide enough information to know the
> attribute to be used for mapping.
> 
> MMIO are always mapped in Stage-2 with Device_nGnRE, which is quite
> restrictive. This would also impact any MMIO regions, such as the video RAM
> buffer, that could be mapped write-combine.
> 
> After reading the ARM ARM (B2.8.2 ARM DII 0486.i), I think we could relax
> the stage-2 mapping by using Device_GRE for all the device MMIOs but the
> GIC.
> 
> We have to keep the GIC MMIO with the most restrictive memory attribute to
> avoid potential side-effect when Xen is switching between multiple vCPUs.

I see, that makes sense.


> All the other devices will be exclusive to a specific guest, so the guest
> can handle the device the way it wants. This may require some extra-care
> when reassigning the device to another domain.
> 
> Edgar, would Device_GRE be fine for you?

Sorry, but I don't think so. We can live with performance impacts but it's
harder with the external data aborts and potential alignment checks.


> Note, that XENMAPSPACE_dev_mmio has been introduced in Xen 4.7 (which is due
> in a couple of weeks) and part of the stable ABI. So if it is not possible
> to relax the memory attribute, it might be worth to think fixing/reverting
> the hypercall for 4.7. Otherwise we would have to introduce a new one in the
> next release.

Yes, maybe we could add something along the lines of the pgprot arg
that Linux/arm64 has in it's __ioremap call. Even if we only support
PROT_DEVICE_nGnRE (or GRE) in 4.7 at the least we can reuse the ABI
to add more modes in 4.8?

Best regards,
Edgar

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

Re: [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Julien Grall]

(CC Wei Liu)

On 23/05/16 12:56, Edgar E. Iglesias wrote:
> On Mon, May 23, 2016 at 11:29:31AM +0100, Julien Grall wrote:
>> On 20/05/16 16:51, Edgar E. Iglesias wrote:
>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>
>>> This series adds support for mapping mmio-sram nodes into dom0
>>> as MEMORY, cached and with RWX perms.
>>
>> Can you explain why you chose to map those nodes as MEMORY, cached and with
>> RWX perms?
>
> My understanding is that these mmio-sram nodes are allowed to be treated as
> Normal memory by the guest OS.
> Guests could potentially do any kind of memory like operations on them.
>
> In our specific case, dom0 won't execute code from these regions but
> Linux/dom0 ends up using standard memcpy/memset/x functions (not
> memcpy_fromio and friends) on the regions.

I looked at the generic sram driver in Linux (drivers/misc/sram.c) which 
actually use memcpy_{to,from}io. So how you driver differs from the 
generic one? What the SRAM will contain?

>
> We saw issues with memset doing cache operations on DEVICE memory in
> the past (external data aborts). I can't find the text in the ARM ARM
> regarding this at the moment but IIRC, dc ops on device mem are not
> allowed.

The ARM ARM (D3-1632 in ARM DDI 0487A.i) states "that the effects of the 
cache maintenance instructions can apply regardless of:
Whether the address accessed:
	* Is Normal memory or Device memory.
	* Has the Cacheable attribute or the Non-cacheable attribute.
"

So I am not sure why you would get an external data aborts when 
executing dc ops on device memory.

>
> We could also see alignment problems, as the alignment checks for
> ARMv8 differ somewhat between normal memory and device memory.
>
> A third reason is performance. The rules for speculative accesses
> and caching are different between device and normal memory.
>
> So I opted for the most permissive attributes thinking that dom0 can
> apply further restrictions if it needs to do so.
>
>
>>> Dom0 can then choose to further restrict these mappings if needed.
>>> We only look at the outer mmio-sram region. The sub-area nodes that
>>> describe allocations within the mmio-sram region are only meaningful
>>> to the guest AFAICT.
>>>
>>> In an attempt to avoid growing the already fairly large domain_build.c
>>> file, I've tried to implement a distributed way to deal with these kind
>>> of special/custom mapping needs. These can live outside of domain_build.c
>>> and are registerd by means of a .map method in the device_spec.
>>>
>>> If this approach is not good, I'm happy to bin it and try something else.
>>
>> We will have a similar problem when using ACPI for DOM0 or mapping a such
>> MMIO to the guest. The hypercalls XENMAPSPACE_dev_mmio and
>> XEN_DOMCTL_memory_mapping do not provide enough information to know the
>> attribute to be used for mapping.
>>
>> MMIO are always mapped in Stage-2 with Device_nGnRE, which is quite
>> restrictive. This would also impact any MMIO regions, such as the video RAM
>> buffer, that could be mapped write-combine.
>>
>> After reading the ARM ARM (B2.8.2 ARM DII 0486.i), I think we could relax
>> the stage-2 mapping by using Device_GRE for all the device MMIOs but the
>> GIC.
>>
>> We have to keep the GIC MMIO with the most restrictive memory attribute to
>> avoid potential side-effect when Xen is switching between multiple vCPUs.
>
> I see, that makes sense.
>
>
>> All the other devices will be exclusive to a specific guest, so the guest
>> can handle the device the way it wants. This may require some extra-care
>> when reassigning the device to another domain.
>>
>> Edgar, would Device_GRE be fine for you?
>
> Sorry, but I don't think so. We can live with performance impacts but it's
> harder with the external data aborts and potential alignment checks.
>
>
>> Note, that XENMAPSPACE_dev_mmio has been introduced in Xen 4.7 (which is due
>> in a couple of weeks) and part of the stable ABI. So if it is not possible
>> to relax the memory attribute, it might be worth to think fixing/reverting
>> the hypercall for 4.7. Otherwise we would have to introduce a new one in the
>> next release.
>
> Yes, maybe we could add something along the lines of the pgprot arg
> that Linux/arm64 has in it's __ioremap call. Even if we only support
> PROT_DEVICE_nGnRE (or GRE) in 4.7 at the least we can reuse the ABI
> to add more modes in 4.8?

I will bring it on a separate subject with the REST of the maintainers 
when we find out what are the possible memory attributes.

Depending on the result, this might be considered as a blocker as I do 
not think we should avoid to  introduce a new hypercall 
(XENMAPSPACE_dev_mmio) which is known to not fit for every case.

Regards,

-- 
Julien Grall

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

Re: [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Edgar E. Iglesias]

On Mon, May 23, 2016 at 02:02:39PM +0100, Julien Grall wrote:
> (CC Wei Liu)
> 
> On 23/05/16 12:56, Edgar E. Iglesias wrote:
> >On Mon, May 23, 2016 at 11:29:31AM +0100, Julien Grall wrote:
> >>On 20/05/16 16:51, Edgar E. Iglesias wrote:
> >>>From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >>>
> >>>This series adds support for mapping mmio-sram nodes into dom0
> >>>as MEMORY, cached and with RWX perms.
> >>
> >>Can you explain why you chose to map those nodes as MEMORY, cached and with
> >>RWX perms?
> >
> >My understanding is that these mmio-sram nodes are allowed to be treated as
> >Normal memory by the guest OS.
> >Guests could potentially do any kind of memory like operations on them.
> >
> >In our specific case, dom0 won't execute code from these regions but
> >Linux/dom0 ends up using standard memcpy/memset/x functions (not
> >memcpy_fromio and friends) on the regions.
> 
> I looked at the generic sram driver in Linux (drivers/misc/sram.c) which
> actually use memcpy_{to,from}io. So how you driver differs from the generic
> one? What the SRAM will contain?

We intend to use that same driver to map the memory but mmio-sram
nodes allow you to assign the regions to other device-drivers.

Some examples:
Documentation/devicetree/bindings/crypto/marvell-cesa.txt
arch/arm/boot/dts/orion5x.dtsi
drivers/crypto/mv_cesa.c

The cover letter for the sram driver had an example aswell allthough
the function names have changed since (it's of_gen_pool_get now):
https://lwn.net/Articles/537728/

Nothing explicitely says that the regions can be assumed to be mapped
as Normal memory, but on Linux they do get mapped as Mormal WC mem
(unless the no-memory-wc prop is set on the node).

The marvell-cesa example also uses plain memset on the sram.


> >We saw issues with memset doing cache operations on DEVICE memory in
> >the past (external data aborts). I can't find the text in the ARM ARM
> >regarding this at the moment but IIRC, dc ops on device mem are not
> >allowed.
> 
> The ARM ARM (D3-1632 in ARM DDI 0487A.i) states "that the effects of the
> cache maintenance instructions can apply regardless of:
> Whether the address accessed:
> 	* Is Normal memory or Device memory.
> 	* Has the Cacheable attribute or the Non-cacheable attribute.
> "
> 
> So I am not sure why you would get an external data aborts when executing dc
> ops on device memory.


OK, I found a reference to the issue we were seeing. If you look at ARM ARM
D3.4.9 Data cache zero instruction, you'll see that the DC ZVA insn always
generates an alignment fault if used on device memory.

I must have remembered incorrectly about the external abort, it's an
alignment fault.


> >We could also see alignment problems, as the alignment checks for
> >ARMv8 differ somewhat between normal memory and device memory.
> >
> >A third reason is performance. The rules for speculative accesses
> >and caching are different between device and normal memory.
> >
> >So I opted for the most permissive attributes thinking that dom0 can
> >apply further restrictions if it needs to do so.
> >
> >
> >>>Dom0 can then choose to further restrict these mappings if needed.
> >>>We only look at the outer mmio-sram region. The sub-area nodes that
> >>>describe allocations within the mmio-sram region are only meaningful
> >>>to the guest AFAICT.
> >>>
> >>>In an attempt to avoid growing the already fairly large domain_build.c
> >>>file, I've tried to implement a distributed way to deal with these kind
> >>>of special/custom mapping needs. These can live outside of domain_build.c
> >>>and are registerd by means of a .map method in the device_spec.
> >>>
> >>>If this approach is not good, I'm happy to bin it and try something else.
> >>
> >>We will have a similar problem when using ACPI for DOM0 or mapping a such
> >>MMIO to the guest. The hypercalls XENMAPSPACE_dev_mmio and
> >>XEN_DOMCTL_memory_mapping do not provide enough information to know the
> >>attribute to be used for mapping.
> >>
> >>MMIO are always mapped in Stage-2 with Device_nGnRE, which is quite
> >>restrictive. This would also impact any MMIO regions, such as the video RAM
> >>buffer, that could be mapped write-combine.
> >>
> >>After reading the ARM ARM (B2.8.2 ARM DII 0486.i), I think we could relax
> >>the stage-2 mapping by using Device_GRE for all the device MMIOs but the
> >>GIC.
> >>
> >>We have to keep the GIC MMIO with the most restrictive memory attribute to
> >>avoid potential side-effect when Xen is switching between multiple vCPUs.
> >
> >I see, that makes sense.
> >
> >
> >>All the other devices will be exclusive to a specific guest, so the guest
> >>can handle the device the way it wants. This may require some extra-care
> >>when reassigning the device to another domain.
> >>
> >>Edgar, would Device_GRE be fine for you?
> >
> >Sorry, but I don't think so. We can live with performance impacts but it's
> >harder with the external data aborts and potential alignment checks.
> >
> >
> >>Note, that XENMAPSPACE_dev_mmio has been introduced in Xen 4.7 (which is due
> >>in a couple of weeks) and part of the stable ABI. So if it is not possible
> >>to relax the memory attribute, it might be worth to think fixing/reverting
> >>the hypercall for 4.7. Otherwise we would have to introduce a new one in the
> >>next release.
> >
> >Yes, maybe we could add something along the lines of the pgprot arg
> >that Linux/arm64 has in it's __ioremap call. Even if we only support
> >PROT_DEVICE_nGnRE (or GRE) in 4.7 at the least we can reuse the ABI
> >to add more modes in 4.8?
> 
> I will bring it on a separate subject with the REST of the maintainers when
> we find out what are the possible memory attributes.
> 
> Depending on the result, this might be considered as a blocker as I do not
> think we should avoid to  introduce a new hypercall (XENMAPSPACE_dev_mmio)
> which is known to not fit for every case.

Thanks!

Best regards,
Edgar

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

Re: [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Julien Grall]

Hi Edgar,

On 23/05/16 15:02, Edgar E. Iglesias wrote:
> On Mon, May 23, 2016 at 02:02:39PM +0100, Julien Grall wrote:
>> (CC Wei Liu)
>>
>> On 23/05/16 12:56, Edgar E. Iglesias wrote:
>>> On Mon, May 23, 2016 at 11:29:31AM +0100, Julien Grall wrote:
>>>> On 20/05/16 16:51, Edgar E. Iglesias wrote:
>>>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>>>
>>>>> This series adds support for mapping mmio-sram nodes into dom0
>>>>> as MEMORY, cached and with RWX perms.
>>>>
>>>> Can you explain why you chose to map those nodes as MEMORY, cached and with
>>>> RWX perms?
>>>
>>> My understanding is that these mmio-sram nodes are allowed to be treated as
>>> Normal memory by the guest OS.
>>> Guests could potentially do any kind of memory like operations on them.
>>>
>>> In our specific case, dom0 won't execute code from these regions but
>>> Linux/dom0 ends up using standard memcpy/memset/x functions (not
>>> memcpy_fromio and friends) on the regions.
>>
>> I looked at the generic sram driver in Linux (drivers/misc/sram.c) which
>> actually use memcpy_{to,from}io. So how you driver differs from the generic
>> one? What the SRAM will contain?
>
> We intend to use that same driver to map the memory but mmio-sram
> nodes allow you to assign the regions to other device-drivers.
>
> Some examples:
> Documentation/devicetree/bindings/crypto/marvell-cesa.txt
> arch/arm/boot/dts/orion5x.dtsi
> drivers/crypto/mv_cesa.c
>
> The cover letter for the sram driver had an example aswell allthough
> the function names have changed since (it's of_gen_pool_get now):
> https://lwn.net/Articles/537728/
>
> Nothing explicitely says that the regions can be assumed to be mapped
> as Normal memory, but on Linux they do get mapped as Mormal WC mem
> (unless the no-memory-wc prop is set on the node).
> The marvell-cesa example also uses plain memset on the sram.

I am a bit confused with this example. From my understanding of 
mv_cesa_get_ram, cp->sram can point either to a normal memory (?) area 
(see gen_pool_dma_alloc) or a Device_nGnRE area (see devm_ioremap_resource).

However, memcpy_{from,to}io should be used when dealing with MMIO (the 
field sram has the __iomem attribute). See the commit 0f3304dc from 
Russel King related to marvell/cesa.

>
>>> We saw issues with memset doing cache operations on DEVICE memory in
>>> the past (external data aborts). I can't find the text in the ARM ARM
>>> regarding this at the moment but IIRC, dc ops on device mem are not
>>> allowed.
>>
>> The ARM ARM (D3-1632 in ARM DDI 0487A.i) states "that the effects of the
>> cache maintenance instructions can apply regardless of:
>> Whether the address accessed:
>> 	* Is Normal memory or Device memory.
>> 	* Has the Cacheable attribute or the Non-cacheable attribute.
>> "
>>
>> So I am not sure why you would get an external data aborts when executing dc
>> ops on device memory.
>
>
> OK, I found a reference to the issue we were seeing. If you look at ARM ARM
> D3.4.9 Data cache zero instruction, you'll see that the DC ZVA insn always
> generates an alignment fault if used on device memory.

Thinking a bit more, I find weird to use cache instructions on the SRAM 
given the region will be mapped uncacheable by Linux. Note that SRAM is 
usually very-fast so using the cache may not improve that much the 
performance.

How bad would the performance be if the processor cannot speculate 
access to the SRAM?

Regards,

-- 
Julien Grall

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

Re: [RFC for-4.8 4/6] xen/arm: Add helper functions to map RWX memory regions

[Julien Grall]

Hi Edgar,

On 20/05/16 16:51, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Create a helper function to map regions as MEMORY with
> cached attributes and read-write-execute permissions.

Providing setting the execute bit is useful, I would try to rationalize 
the helpers by expanding map_regions_rw_cache (and maybe rename it).

> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>   xen/arch/arm/p2m.c        | 26 ++++++++++++++++++++++++++
>   xen/include/asm-arm/p2m.h | 10 ++++++++++
>   2 files changed, 36 insertions(+)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index db21433..7e788f9 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1219,6 +1219,32 @@ int p2m_populate_ram(struct domain *d,
>                                d->arch.p2m.default_access);
>   }
>
> +int map_regions_rwx_cache(struct domain *d,
> +                         unsigned long start_gfn,
> +                         unsigned long nr,
> +                         unsigned long mfn)
> +{
> +    return apply_p2m_changes(d, INSERT,
> +                             pfn_to_paddr(start_gfn),
> +                             pfn_to_paddr(start_gfn + nr),
> +                             pfn_to_paddr(mfn),
> +                             MATTR_MEM, 0, p2m_ram_rw,

We should not use p2m_ram_rw for other mapping than DRAM. It could be 
used by Xen to differentiate MMIO vs RAM.

> +                             p2m_access_rwx);
> +}
> +
> +int unmap_regions_rwx_cache(struct domain *d,
> +                           unsigned long start_gfn,
> +                           unsigned long nr,
> +                           unsigned long mfn)
> +{
> +    return apply_p2m_changes(d, REMOVE,
> +                             pfn_to_paddr(start_gfn),
> +                             pfn_to_paddr(start_gfn + nr),
> +                             pfn_to_paddr(mfn),
> +                             MATTR_MEM, 0, p2m_invalid,
> +                             p2m_access_rwx);
> +}
> +
>   int map_regions_rw_cache(struct domain *d,
>                            unsigned long start_gfn,
>                            unsigned long nr,
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index d240d1e..294050e 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -144,6 +144,16 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn);
>   /* Setup p2m RAM mapping for domain d from start-end. */
>   int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
>
> +int map_regions_rwx_cache(struct domain *d,
> +                          unsigned long start_gfn,
> +                          unsigned long nr_mfns,
> +                          unsigned long mfn);
> +
> +int unmap_regions_rwx_cache(struct domain *d,
> +                            unsigned long start_gfn,
> +                            unsigned long nr_mfns,
> +                            unsigned long mfn);
> +
>   int map_regions_rw_cache(struct domain *d,
>                            unsigned long start_gfn,
>                            unsigned long nr_mfns,
>

Regards,

-- 
Julien Grall

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

Re: [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Edgar E. Iglesias]

On Mon, May 23, 2016 at 04:13:53PM +0100, Julien Grall wrote:
> Hi Edgar,
> 
> On 23/05/16 15:02, Edgar E. Iglesias wrote:
> >On Mon, May 23, 2016 at 02:02:39PM +0100, Julien Grall wrote:
> >>(CC Wei Liu)
> >>
> >>On 23/05/16 12:56, Edgar E. Iglesias wrote:
> >>>On Mon, May 23, 2016 at 11:29:31AM +0100, Julien Grall wrote:
> >>>>On 20/05/16 16:51, Edgar E. Iglesias wrote:
> >>>>>From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >>>>>
> >>>>>This series adds support for mapping mmio-sram nodes into dom0
> >>>>>as MEMORY, cached and with RWX perms.
> >>>>
> >>>>Can you explain why you chose to map those nodes as MEMORY, cached and with
> >>>>RWX perms?
> >>>
> >>>My understanding is that these mmio-sram nodes are allowed to be treated as
> >>>Normal memory by the guest OS.
> >>>Guests could potentially do any kind of memory like operations on them.
> >>>
> >>>In our specific case, dom0 won't execute code from these regions but
> >>>Linux/dom0 ends up using standard memcpy/memset/x functions (not
> >>>memcpy_fromio and friends) on the regions.
> >>
> >>I looked at the generic sram driver in Linux (drivers/misc/sram.c) which
> >>actually use memcpy_{to,from}io. So how you driver differs from the generic
> >>one? What the SRAM will contain?
> >
> >We intend to use that same driver to map the memory but mmio-sram
> >nodes allow you to assign the regions to other device-drivers.
> >
> >Some examples:
> >Documentation/devicetree/bindings/crypto/marvell-cesa.txt
> >arch/arm/boot/dts/orion5x.dtsi
> >drivers/crypto/mv_cesa.c
> >
> >The cover letter for the sram driver had an example aswell allthough
> >the function names have changed since (it's of_gen_pool_get now):
> >https://lwn.net/Articles/537728/
> >
> >Nothing explicitely says that the regions can be assumed to be mapped
> >as Normal memory, but on Linux they do get mapped as Mormal WC mem
> >(unless the no-memory-wc prop is set on the node).
> >The marvell-cesa example also uses plain memset on the sram.
> 
> I am a bit confused with this example. From my understanding of
> mv_cesa_get_ram, cp->sram can point either to a normal memory (?) area (see
> gen_pool_dma_alloc) or a Device_nGnRE area (see devm_ioremap_resource).
> 
> However, memcpy_{from,to}io should be used when dealing with MMIO (the field
> sram has the __iomem attribute). See the commit 0f3304dc from Russel King
> related to marvell/cesa.


Yeah, I'm started to get confused too. Maybe they just forgot the memset
in drivers/crypto/mv_cesa.c.

There are other examples though, that don't do fromio/toio at all.
Documentation/devicetree/bindings/media/coda.txt
drivers/media/platform/coda/coda-common.c

Allthough ofcourse, these could also be wrong. Maybe I've missunderstood how
mmio-sram is supposed to be used.


> >>>We saw issues with memset doing cache operations on DEVICE memory in
> >>>the past (external data aborts). I can't find the text in the ARM ARM
> >>>regarding this at the moment but IIRC, dc ops on device mem are not
> >>>allowed.
> >>
> >>The ARM ARM (D3-1632 in ARM DDI 0487A.i) states "that the effects of the
> >>cache maintenance instructions can apply regardless of:
> >>Whether the address accessed:
> >>	* Is Normal memory or Device memory.
> >>	* Has the Cacheable attribute or the Non-cacheable attribute.
> >>"
> >>
> >>So I am not sure why you would get an external data aborts when executing dc
> >>ops on device memory.
> >
> >
> >OK, I found a reference to the issue we were seeing. If you look at ARM ARM
> >D3.4.9 Data cache zero instruction, you'll see that the DC ZVA insn always
> >generates an alignment fault if used on device memory.
> 
> Thinking a bit more, I find weird to use cache instructions on the SRAM
> given the region will be mapped uncacheable by Linux. Note that SRAM is
> usually very-fast so using the cache may not improve that much the
> performance.
> 
> How bad would the performance be if the processor cannot speculate access to
> the SRAM?

I don't have the setup available right now to try it out but I wouldn't
expect it to be very signifcant for our apps. In our case it had more to
do with the ability to use the remote-proc drivers as they are and really
any linux driver that potentially can take an sram as a memory region for
local use (DMA buffers or whatever), without changing the memory ops to
the _fromio/_toio versions.

Best regards,
Edgar

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

Re: [RFC for-4.8 4/6] xen/arm: Add helper functions to map RWX memory regions

[Edgar E. Iglesias]

On Mon, May 23, 2016 at 04:36:03PM +0100, Julien Grall wrote:
> Hi Edgar,
> 
> On 20/05/16 16:51, Edgar E. Iglesias wrote:
> >From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> >Create a helper function to map regions as MEMORY with
> >cached attributes and read-write-execute permissions.
> 
> Providing setting the execute bit is useful, I would try to rationalize the
> helpers by expanding map_regions_rw_cache (and maybe rename it).

Thanks, I'll have change the code around to do that.

> 
> >Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> >---
> >  xen/arch/arm/p2m.c        | 26 ++++++++++++++++++++++++++
> >  xen/include/asm-arm/p2m.h | 10 ++++++++++
> >  2 files changed, 36 insertions(+)
> >
> >diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> >index db21433..7e788f9 100644
> >--- a/xen/arch/arm/p2m.c
> >+++ b/xen/arch/arm/p2m.c
> >@@ -1219,6 +1219,32 @@ int p2m_populate_ram(struct domain *d,
> >                               d->arch.p2m.default_access);
> >  }
> >
> >+int map_regions_rwx_cache(struct domain *d,
> >+                         unsigned long start_gfn,
> >+                         unsigned long nr,
> >+                         unsigned long mfn)
> >+{
> >+    return apply_p2m_changes(d, INSERT,
> >+                             pfn_to_paddr(start_gfn),
> >+                             pfn_to_paddr(start_gfn + nr),
> >+                             pfn_to_paddr(mfn),
> >+                             MATTR_MEM, 0, p2m_ram_rw,
> 
> We should not use p2m_ram_rw for other mapping than DRAM. It could be used
> by Xen to differentiate MMIO vs RAM.


OK, I see. I'll fix that.

Cheers,
Edgar

> 
> >+                             p2m_access_rwx);
> >+}
> >+
> >+int unmap_regions_rwx_cache(struct domain *d,
> >+                           unsigned long start_gfn,
> >+                           unsigned long nr,
> >+                           unsigned long mfn)
> >+{
> >+    return apply_p2m_changes(d, REMOVE,
> >+                             pfn_to_paddr(start_gfn),
> >+                             pfn_to_paddr(start_gfn + nr),
> >+                             pfn_to_paddr(mfn),
> >+                             MATTR_MEM, 0, p2m_invalid,
> >+                             p2m_access_rwx);
> >+}
> >+
> >  int map_regions_rw_cache(struct domain *d,
> >                           unsigned long start_gfn,
> >                           unsigned long nr,
> >diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> >index d240d1e..294050e 100644
> >--- a/xen/include/asm-arm/p2m.h
> >+++ b/xen/include/asm-arm/p2m.h
> >@@ -144,6 +144,16 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn);
> >  /* Setup p2m RAM mapping for domain d from start-end. */
> >  int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
> >
> >+int map_regions_rwx_cache(struct domain *d,
> >+                          unsigned long start_gfn,
> >+                          unsigned long nr_mfns,
> >+                          unsigned long mfn);
> >+
> >+int unmap_regions_rwx_cache(struct domain *d,
> >+                            unsigned long start_gfn,
> >+                            unsigned long nr_mfns,
> >+                            unsigned long mfn);
> >+
> >  int map_regions_rw_cache(struct domain *d,
> >                           unsigned long start_gfn,
> >                           unsigned long nr_mfns,
> >
> 
> Regards,
> 
> -- 
> Julien Grall

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

Re: [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Julien Grall]

Hi Edgar,

On 23/05/2016 16:42, Edgar E. Iglesias wrote:
> On Mon, May 23, 2016 at 04:13:53PM +0100, Julien Grall wrote:
>> On 23/05/16 15:02, Edgar E. Iglesias wrote:
>>> On Mon, May 23, 2016 at 02:02:39PM +0100, Julien Grall wrote:
>>>> (CC Wei Liu)
>>>>
>>>> On 23/05/16 12:56, Edgar E. Iglesias wrote:
>>>>> On Mon, May 23, 2016 at 11:29:31AM +0100, Julien Grall wrote:
>>>>>> On 20/05/16 16:51, Edgar E. Iglesias wrote:
>>>>>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>>>>>
>>>>>>> This series adds support for mapping mmio-sram nodes into dom0
>>>>>>> as MEMORY, cached and with RWX perms.
>>>>>>
>>>>>> Can you explain why you chose to map those nodes as MEMORY, cached and with
>>>>>> RWX perms?
>>>>>
>>>>> My understanding is that these mmio-sram nodes are allowed to be treated as
>>>>> Normal memory by the guest OS.
>>>>> Guests could potentially do any kind of memory like operations on them.
>>>>>
>>>>> In our specific case, dom0 won't execute code from these regions but
>>>>> Linux/dom0 ends up using standard memcpy/memset/x functions (not
>>>>> memcpy_fromio and friends) on the regions.
>>>>
>>>> I looked at the generic sram driver in Linux (drivers/misc/sram.c) which
>>>> actually use memcpy_{to,from}io. So how you driver differs from the generic
>>>> one? What the SRAM will contain?
>>>
>>> We intend to use that same driver to map the memory but mmio-sram
>>> nodes allow you to assign the regions to other device-drivers.
>>>
>>> Some examples:
>>> Documentation/devicetree/bindings/crypto/marvell-cesa.txt
>>> arch/arm/boot/dts/orion5x.dtsi
>>> drivers/crypto/mv_cesa.c
>>>
>>> The cover letter for the sram driver had an example aswell allthough
>>> the function names have changed since (it's of_gen_pool_get now):
>>> https://lwn.net/Articles/537728/
>>>
>>> Nothing explicitely says that the regions can be assumed to be mapped
>>> as Normal memory, but on Linux they do get mapped as Mormal WC mem
>>> (unless the no-memory-wc prop is set on the node).
>>> The marvell-cesa example also uses plain memset on the sram.
>>
>> I am a bit confused with this example. From my understanding of
>> mv_cesa_get_ram, cp->sram can point either to a normal memory (?) area (see
>> gen_pool_dma_alloc) or a Device_nGnRE area (see devm_ioremap_resource).
>>
>> However, memcpy_{from,to}io should be used when dealing with MMIO (the field
>> sram has the __iomem attribute). See the commit 0f3304dc from Russel King
>> related to marvell/cesa.
>
>
> Yeah, I'm started to get confused too. Maybe they just forgot the memset
> in drivers/crypto/mv_cesa.c.
>
> There are other examples though, that don't do fromio/toio at all.
> Documentation/devicetree/bindings/media/coda.txt
> drivers/media/platform/coda/coda-common.c
>
> Allthough ofcourse, these could also be wrong. Maybe I've missunderstood how
> mmio-sram is supposed to be used.

I have talked about the memory attribute around me and the consensus is 
we should use the most relaxed mode that does not have any security 
implication or undefined behavior for a given device.

For SRAM it would be normal memory uncached (?) when the property 
"no-memory-wc" is not present, else TBD.

I suspect we would have to relax more MMIOs in the future. Rather than 
providing a function to map, the code is very similar except the memory 
attribute, I suggest to provide a list of compatible with the memory 
attribute to use.

All the children node would inherit the memory attribute of the parent.

What do you think?

Regards,

-- 
Julien Grall

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

Re: [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Stefano Stabellini]

On Mon, 23 May 2016, Julien Grall wrote:
> Note, that XENMAPSPACE_dev_mmio has been introduced in Xen 4.7 (which is due
> in a couple of weeks) and part of the stable ABI. So if it is not possible to
> relax the memory attribute, it might be worth to think fixing/reverting the
> hypercall for 4.7. Otherwise we would have to introduce a new one in the next
> release.

FYI the Linux side missed the Linux 4.7 merge window and it is now
queued for 4.8. Therefore theoretically could still be changed, but I
would be careful with any doing major changes at this point.

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

Re: [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Stefano Stabellini]

On Tue, 24 May 2016, Julien Grall wrote:
> On 23/05/2016 16:42, Edgar E. Iglesias wrote:
> > On Mon, May 23, 2016 at 04:13:53PM +0100, Julien Grall wrote:
> > > On 23/05/16 15:02, Edgar E. Iglesias wrote:
> > > > On Mon, May 23, 2016 at 02:02:39PM +0100, Julien Grall wrote:
> > > > > (CC Wei Liu)
> > > > > 
> > > > > On 23/05/16 12:56, Edgar E. Iglesias wrote:
> > > > > > On Mon, May 23, 2016 at 11:29:31AM +0100, Julien Grall wrote:
> > > > > > > On 20/05/16 16:51, Edgar E. Iglesias wrote:
> > > > > > > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> > > > > > > > 
> > > > > > > > This series adds support for mapping mmio-sram nodes into dom0
> > > > > > > > as MEMORY, cached and with RWX perms.
> > > > > > > 
> > > > > > > Can you explain why you chose to map those nodes as MEMORY, cached
> > > > > > > and with
> > > > > > > RWX perms?
> > > > > > 
> > > > > > My understanding is that these mmio-sram nodes are allowed to be
> > > > > > treated as
> > > > > > Normal memory by the guest OS.
> > > > > > Guests could potentially do any kind of memory like operations on
> > > > > > them.
> > > > > > 
> > > > > > In our specific case, dom0 won't execute code from these regions but
> > > > > > Linux/dom0 ends up using standard memcpy/memset/x functions (not
> > > > > > memcpy_fromio and friends) on the regions.
> > > > > 
> > > > > I looked at the generic sram driver in Linux (drivers/misc/sram.c)
> > > > > which
> > > > > actually use memcpy_{to,from}io. So how you driver differs from the
> > > > > generic
> > > > > one? What the SRAM will contain?
> > > > 
> > > > We intend to use that same driver to map the memory but mmio-sram
> > > > nodes allow you to assign the regions to other device-drivers.
> > > > 
> > > > Some examples:
> > > > Documentation/devicetree/bindings/crypto/marvell-cesa.txt
> > > > arch/arm/boot/dts/orion5x.dtsi
> > > > drivers/crypto/mv_cesa.c
> > > > 
> > > > The cover letter for the sram driver had an example aswell allthough
> > > > the function names have changed since (it's of_gen_pool_get now):
> > > > https://lwn.net/Articles/537728/
> > > > 
> > > > Nothing explicitely says that the regions can be assumed to be mapped
> > > > as Normal memory, but on Linux they do get mapped as Mormal WC mem
> > > > (unless the no-memory-wc prop is set on the node).
> > > > The marvell-cesa example also uses plain memset on the sram.
> > > 
> > > I am a bit confused with this example. From my understanding of
> > > mv_cesa_get_ram, cp->sram can point either to a normal memory (?) area
> > > (see
> > > gen_pool_dma_alloc) or a Device_nGnRE area (see devm_ioremap_resource).
> > > 
> > > However, memcpy_{from,to}io should be used when dealing with MMIO (the
> > > field
> > > sram has the __iomem attribute). See the commit 0f3304dc from Russel King
> > > related to marvell/cesa.
> > 
> > 
> > Yeah, I'm started to get confused too. Maybe they just forgot the memset
> > in drivers/crypto/mv_cesa.c.
> > 
> > There are other examples though, that don't do fromio/toio at all.
> > Documentation/devicetree/bindings/media/coda.txt
> > drivers/media/platform/coda/coda-common.c
> > 
> > Allthough ofcourse, these could also be wrong. Maybe I've missunderstood how
> > mmio-sram is supposed to be used.
> 
> I have talked about the memory attribute around me and the consensus is we
> should use the most relaxed mode that does not have any security implication
> or undefined behavior for a given device.

I agree and it has always been the intention.


> For SRAM it would be normal memory uncached (?) when the property
> "no-memory-wc" is not present, else TBD.
> 
> I suspect we would have to relax more MMIOs in the future. Rather than
> providing a function to map, the code is very similar except the memory
> attribute, I suggest to provide a list of compatible with the memory attribute
> to use.
> 
> All the children node would inherit the memory attribute of the parent.
> 
> What do you think?

That would work for device tree, but we still need to rely on the
hypercall for ACPI systems.

Given that it is not easy to add an additional parameter to
XENMEM_add_to_physmap_range, I think we'll have to provide a new
hypercall to allow setting attributes other than the Xen default. That
could be done in Xen 4.8 and Linux >= 4.9.

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

Re: [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Julien Grall]

Hi Stefano,

On 25/05/16 10:43, Stefano Stabellini wrote:
>> For SRAM it would be normal memory uncached (?) when the property
>> "no-memory-wc" is not present, else TBD.
>>
>> I suspect we would have to relax more MMIOs in the future. Rather than
>> providing a function to map, the code is very similar except the memory
>> attribute, I suggest to provide a list of compatible with the memory attribute
>> to use.
>>
>> All the children node would inherit the memory attribute of the parent.
>>
>> What do you think?
>
> That would work for device tree, but we still need to rely on the
> hypercall for ACPI systems.
>
> Given that it is not easy to add an additional parameter to
> XENMEM_add_to_physmap_range, I think we'll have to provide a new
> hypercall to allow setting attributes other than the Xen default. That
> could be done in Xen 4.8 and Linux >= 4.9.

There is no need to introduce a new hypercall. The 
XENMEM_add_to_physmap_batch contains an unused field ('foreign_id', to 
be renamed) for mapping device MMIOs (see Jan's mail [1]).

XENMEM_add_to_physmap will always map with the default memory attribute 
(Device_nGnRnE) and if the kernel want to use another memory attribute, 
it will have to use XENMEM_add_to_physmap_batch.

With the plan suggested in [2], there are no modifications required in 
Linux for the moment.

Regards,

[1] 
http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg02341.html
[2] 
http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg02347.html

-- 
Julien Grall

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

Re: [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Stefano Stabellini]

On Wed, 25 May 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 25/05/16 10:43, Stefano Stabellini wrote:
> > > For SRAM it would be normal memory uncached (?) when the property
> > > "no-memory-wc" is not present, else TBD.
> > > 
> > > I suspect we would have to relax more MMIOs in the future. Rather than
> > > providing a function to map, the code is very similar except the memory
> > > attribute, I suggest to provide a list of compatible with the memory
> > > attribute
> > > to use.
> > > 
> > > All the children node would inherit the memory attribute of the parent.
> > > 
> > > What do you think?
> > 
> > That would work for device tree, but we still need to rely on the
> > hypercall for ACPI systems.
> > 
> > Given that it is not easy to add an additional parameter to
> > XENMEM_add_to_physmap_range, I think we'll have to provide a new
> > hypercall to allow setting attributes other than the Xen default. That
> > could be done in Xen 4.8 and Linux >= 4.9.
> 
> There is no need to introduce a new hypercall. The XENMEM_add_to_physmap_batch
> contains an unused field ('foreign_id', to be renamed) for mapping device
> MMIOs (see Jan's mail [1]).
> 
> XENMEM_add_to_physmap will always map with the default memory attribute
> (Device_nGnRnE) and if the kernel want to use another memory attribute, it
> will have to use XENMEM_add_to_physmap_batch.
> 
> With the plan suggested in [2], there are no modifications required in Linux
> for the moment.
> 
> Regards,
> 
> [1] http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg02341.html
> [2] http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg02347.html

I read the separate thread. Sounds good.

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

Re: [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Edgar E. Iglesias]

On Tue, May 24, 2016 at 08:44:41PM +0100, Julien Grall wrote:
> Hi Edgar,

Hi Julien,

> 
> On 23/05/2016 16:42, Edgar E. Iglesias wrote:
> >On Mon, May 23, 2016 at 04:13:53PM +0100, Julien Grall wrote:
> >>On 23/05/16 15:02, Edgar E. Iglesias wrote:
> >>>On Mon, May 23, 2016 at 02:02:39PM +0100, Julien Grall wrote:
> >>>>(CC Wei Liu)
> >>>>
> >>>>On 23/05/16 12:56, Edgar E. Iglesias wrote:
> >>>>>On Mon, May 23, 2016 at 11:29:31AM +0100, Julien Grall wrote:
> >>>>>>On 20/05/16 16:51, Edgar E. Iglesias wrote:
> >>>>>>>From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >>>>>>>
> >>>>>>>This series adds support for mapping mmio-sram nodes into dom0
> >>>>>>>as MEMORY, cached and with RWX perms.
> >>>>>>
> >>>>>>Can you explain why you chose to map those nodes as MEMORY, cached and with
> >>>>>>RWX perms?
> >>>>>
> >>>>>My understanding is that these mmio-sram nodes are allowed to be treated as
> >>>>>Normal memory by the guest OS.
> >>>>>Guests could potentially do any kind of memory like operations on them.
> >>>>>
> >>>>>In our specific case, dom0 won't execute code from these regions but
> >>>>>Linux/dom0 ends up using standard memcpy/memset/x functions (not
> >>>>>memcpy_fromio and friends) on the regions.
> >>>>
> >>>>I looked at the generic sram driver in Linux (drivers/misc/sram.c) which
> >>>>actually use memcpy_{to,from}io. So how you driver differs from the generic
> >>>>one? What the SRAM will contain?
> >>>
> >>>We intend to use that same driver to map the memory but mmio-sram
> >>>nodes allow you to assign the regions to other device-drivers.
> >>>
> >>>Some examples:
> >>>Documentation/devicetree/bindings/crypto/marvell-cesa.txt
> >>>arch/arm/boot/dts/orion5x.dtsi
> >>>drivers/crypto/mv_cesa.c
> >>>
> >>>The cover letter for the sram driver had an example aswell allthough
> >>>the function names have changed since (it's of_gen_pool_get now):
> >>>https://lwn.net/Articles/537728/
> >>>
> >>>Nothing explicitely says that the regions can be assumed to be mapped
> >>>as Normal memory, but on Linux they do get mapped as Mormal WC mem
> >>>(unless the no-memory-wc prop is set on the node).
> >>>The marvell-cesa example also uses plain memset on the sram.
> >>
> >>I am a bit confused with this example. From my understanding of
> >>mv_cesa_get_ram, cp->sram can point either to a normal memory (?) area (see
> >>gen_pool_dma_alloc) or a Device_nGnRE area (see devm_ioremap_resource).
> >>
> >>However, memcpy_{from,to}io should be used when dealing with MMIO (the field
> >>sram has the __iomem attribute). See the commit 0f3304dc from Russel King
> >>related to marvell/cesa.
> >
> >
> >Yeah, I'm started to get confused too. Maybe they just forgot the memset
> >in drivers/crypto/mv_cesa.c.
> >
> >There are other examples though, that don't do fromio/toio at all.
> >Documentation/devicetree/bindings/media/coda.txt
> >drivers/media/platform/coda/coda-common.c
> >
> >Allthough ofcourse, these could also be wrong. Maybe I've missunderstood how
> >mmio-sram is supposed to be used.
> 
> I have talked about the memory attribute around me and the consensus is we
> should use the most relaxed mode that does not have any security implication
> or undefined behavior for a given device.

Yes, I agree with the principle.
The questionable part is what attributes are considered safe for specific
nodes.

> 
> For SRAM it would be normal memory uncached (?) when the property
> "no-memory-wc" is not present, else TBD.

That sounds good, it would solve our problems.


> I suspect we would have to relax more MMIOs in the future. Rather than
> providing a function to map, the code is very similar except the memory
> attribute, I suggest to provide a list of compatible with the memory
> attribute to use.
> 
> All the children node would inherit the memory attribute of the parent.
> 
> What do you think?

That sounds doable. I'll put something together addressing all your comments
and post new RFC.

Best regards,
Edgar

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

Re: [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Edgar E. Iglesias]

On Tue, May 24, 2016 at 08:44:41PM +0100, Julien Grall wrote:
> Hi Edgar,
> 
> On 23/05/2016 16:42, Edgar E. Iglesias wrote:
> >On Mon, May 23, 2016 at 04:13:53PM +0100, Julien Grall wrote:
> >>On 23/05/16 15:02, Edgar E. Iglesias wrote:
> >>>On Mon, May 23, 2016 at 02:02:39PM +0100, Julien Grall wrote:
> >>>>(CC Wei Liu)
> >>>>
> >>>>On 23/05/16 12:56, Edgar E. Iglesias wrote:
> >>>>>On Mon, May 23, 2016 at 11:29:31AM +0100, Julien Grall wrote:
> >>>>>>On 20/05/16 16:51, Edgar E. Iglesias wrote:
> >>>>>>>From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >>>>>>>
> >>>>>>>This series adds support for mapping mmio-sram nodes into dom0
> >>>>>>>as MEMORY, cached and with RWX perms.
> >>>>>>
> >>>>>>Can you explain why you chose to map those nodes as MEMORY, cached and with
> >>>>>>RWX perms?
> >>>>>
> >>>>>My understanding is that these mmio-sram nodes are allowed to be treated as
> >>>>>Normal memory by the guest OS.
> >>>>>Guests could potentially do any kind of memory like operations on them.
> >>>>>
> >>>>>In our specific case, dom0 won't execute code from these regions but
> >>>>>Linux/dom0 ends up using standard memcpy/memset/x functions (not
> >>>>>memcpy_fromio and friends) on the regions.
> >>>>
> >>>>I looked at the generic sram driver in Linux (drivers/misc/sram.c) which
> >>>>actually use memcpy_{to,from}io. So how you driver differs from the generic
> >>>>one? What the SRAM will contain?
> >>>
> >>>We intend to use that same driver to map the memory but mmio-sram
> >>>nodes allow you to assign the regions to other device-drivers.
> >>>
> >>>Some examples:
> >>>Documentation/devicetree/bindings/crypto/marvell-cesa.txt
> >>>arch/arm/boot/dts/orion5x.dtsi
> >>>drivers/crypto/mv_cesa.c
> >>>
> >>>The cover letter for the sram driver had an example aswell allthough
> >>>the function names have changed since (it's of_gen_pool_get now):
> >>>https://lwn.net/Articles/537728/
> >>>
> >>>Nothing explicitely says that the regions can be assumed to be mapped
> >>>as Normal memory, but on Linux they do get mapped as Mormal WC mem
> >>>(unless the no-memory-wc prop is set on the node).
> >>>The marvell-cesa example also uses plain memset on the sram.
> >>
> >>I am a bit confused with this example. From my understanding of
> >>mv_cesa_get_ram, cp->sram can point either to a normal memory (?) area (see
> >>gen_pool_dma_alloc) or a Device_nGnRE area (see devm_ioremap_resource).
> >>
> >>However, memcpy_{from,to}io should be used when dealing with MMIO (the field
> >>sram has the __iomem attribute). See the commit 0f3304dc from Russel King
> >>related to marvell/cesa.
> >
> >
> >Yeah, I'm started to get confused too. Maybe they just forgot the memset
> >in drivers/crypto/mv_cesa.c.
> >
> >There are other examples though, that don't do fromio/toio at all.
> >Documentation/devicetree/bindings/media/coda.txt
> >drivers/media/platform/coda/coda-common.c
> >
> >Allthough ofcourse, these could also be wrong. Maybe I've missunderstood how
> >mmio-sram is supposed to be used.
> 
> I have talked about the memory attribute around me and the consensus is we
> should use the most relaxed mode that does not have any security implication
> or undefined behavior for a given device.
> 
> For SRAM it would be normal memory uncached (?) when the property
> "no-memory-wc" is not present, else TBD.
> 
> I suspect we would have to relax more MMIOs in the future. Rather than
> providing a function to map, the code is very similar except the memory
> attribute, I suggest to provide a list of compatible with the memory
> attribute to use.
> 
> All the children node would inherit the memory attribute of the parent.
> 
> What do you think?

Hi again,

Looking a little closer, the place where the generic list of matches and
attributes doesn't work well is when trying to deal with the no-memory-wc
property available only in mmio-sram nodes.

We'd really need an mmio-sram specific check in that case. Either
explicitely open coded in domain_build.c or something along the lines
f the .map method. Or did you have other ideas in mind?

Best regards,
Edgar

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

Re: [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Julien Grall]

Hi Edgar,

On 25/05/16 14:29, Edgar E. Iglesias wrote:
> On Tue, May 24, 2016 at 08:44:41PM +0100, Julien Grall wrote:
> Looking a little closer, the place where the generic list of matches and
> attributes doesn't work well is when trying to deal with the no-memory-wc
> property available only in mmio-sram nodes.
>
> We'd really need an mmio-sram specific check in that case. Either
> explicitely open coded in domain_build.c or something along the lines
> f the .map method. Or did you have other ideas in mind?

How about extending the function dt_match_node and the structure 
dt_device_match to check the existence (or not) of a property?

Regards,

-- 
Julien Grall

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

Re: [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Edgar E. Iglesias]

On Wed, May 25, 2016 at 03:24:39PM +0100, Julien Grall wrote:
> Hi Edgar,
> 
> On 25/05/16 14:29, Edgar E. Iglesias wrote:
> >On Tue, May 24, 2016 at 08:44:41PM +0100, Julien Grall wrote:
> >Looking a little closer, the place where the generic list of matches and
> >attributes doesn't work well is when trying to deal with the no-memory-wc
> >property available only in mmio-sram nodes.
> >
> >We'd really need an mmio-sram specific check in that case. Either
> >explicitely open coded in domain_build.c or something along the lines
> >f the .map method. Or did you have other ideas in mind?
> 
> How about extending the function dt_match_node and the structure
> dt_device_match to check the existence (or not) of a property?


Thanks Julien,

I had some time to get back to this and have created a new series
that I think is more along the lines with your suggestions.
I'll be posting an RFC soon.

Best regards,
Edgar

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