[2/2] arm/xen: Don't use xen DMA ops when the device is protected by an IOMMU

Message ID 1392913301-25524-1-git-send-email-julien.grall@linaro.org
State New
Headers show

Commit Message

Julien Grall Feb. 20, 2014, 4:21 p.m.
Only Xen is able to know if a device can safely avoid to use xen-swiotlb.
This patch introduce a new property "protected-devices" for the hypervisor
node which list device which the IOMMU are been correctly programmed by Xen.

During Linux boot, Xen specific code will create an hash table which
contains all these devices. The hash table will be used in need_xen_dma_ops
to check if the Xen DMA ops needs to be used for the current device.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: Rob Landley <rob@landley.net>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: devicetree@vger.kernel.org
---
 Documentation/devicetree/bindings/arm/xen.txt |    2 +
 arch/arm/include/asm/xen/dma-mapping.h        |   11 +++-
 arch/arm/xen/enlighten.c                      |   75 +++++++++++++++++++++++++
 3 files changed, 87 insertions(+), 1 deletion(-)

Comments

Ian Campbell Feb. 20, 2014, 4:35 p.m. | #1
On Thu, 2014-02-20 at 16:21 +0000, Julien Grall wrote:
> Only Xen is able to know if a device can safely avoid to use xen-swiotlb.
> This patch introduce a new property "protected-devices" for the hypervisor
> node which list device which the IOMMU are been correctly programmed by Xen.
> 
> During Linux boot, Xen specific code will create an hash table which
> contains all these devices. The hash table will be used in need_xen_dma_ops
> to check if the Xen DMA ops needs to be used for the current device.

Is it out of the question to find a field within struct device itself to
store this e.g. in struct device_dma_parameters perhaps and avoid the
need for a hashtable lookup.

device->iommu_group might be another option, if we can create our own
group?

Ian.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ian Campbell Feb. 20, 2014, 5:13 p.m. | #2
Adding the -spec list for the generic IOMMU binding question.

On Thu, 2014-02-20 at 16:21 +0000, Julien Grall wrote:
> diff --git a/Documentation/devicetree/bindings/arm/xen.txt b/Documentation/devicetree/bindings/arm/xen.txt
> index 0f7b9c2..ee25a57 100644
> --- a/Documentation/devicetree/bindings/arm/xen.txt
> +++ b/Documentation/devicetree/bindings/arm/xen.txt
> @@ -15,6 +15,8 @@ the following properties:
>  - interrupts: the interrupt used by Xen to inject event notifications.
>    A GIC node is also required.
>  
> +- protected-devices: (optional) List of phandles to device node where the
> +IOMMU has been programmed by Xen. 

Is there some common/generic IOMMU binding which we can reuse here?
Although this isn't exactly an IOMMU it certainly has some similarities
-- it is providing IOMMU like functionality (albeit a very inflexible
IOMMU which you don't need to/can't actually program).

I'd far rather we followed existing patterns rather than invent our own
things.

I'm wondering if perhaps we didn't ought to integrate this as an actual
IOMMU driver, although I'm not convinced this would make sense.

I'm also not sure about shovelling everything as properties under a
single Xen node, should this not be its own node with compatible =
"xen,(pv)iommu"?

Ian.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefano Stabellini Feb. 24, 2014, 12:19 p.m. | #3
CC'ing Greg.

On Thu, 20 Feb 2014, Ian Campbell wrote:
> On Thu, 2014-02-20 at 16:21 +0000, Julien Grall wrote:
> > Only Xen is able to know if a device can safely avoid to use xen-swiotlb.
> > This patch introduce a new property "protected-devices" for the hypervisor
> > node which list device which the IOMMU are been correctly programmed by Xen.
> > 
> > During Linux boot, Xen specific code will create an hash table which
> > contains all these devices. The hash table will be used in need_xen_dma_ops
> > to check if the Xen DMA ops needs to be used for the current device.
> 
> Is it out of the question to find a field within struct device itself to
> store this e.g. in struct device_dma_parameters perhaps and avoid the
> need for a hashtable lookup.
> 
> device->iommu_group might be another option, if we can create our own
> group?

I agree that a field in struct device would be ideal.
Greg, get_maintainer.pl points at you as main maintainer of device.h, do
you have an opinion on this?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefano Stabellini Feb. 24, 2014, 8:49 p.m. | #4
On Mon, 24 Feb 2014, gregkh@linuxfoundation.org wrote:
> On Mon, Feb 24, 2014 at 12:19:11PM +0000, Stefano Stabellini wrote:
> > CC'ing Greg.
> > 
> > On Thu, 20 Feb 2014, Ian Campbell wrote:
> > > On Thu, 2014-02-20 at 16:21 +0000, Julien Grall wrote:
> > > > Only Xen is able to know if a device can safely avoid to use xen-swiotlb.
> > > > This patch introduce a new property "protected-devices" for the hypervisor
> > > > node which list device which the IOMMU are been correctly programmed by Xen.
> > > > 
> > > > During Linux boot, Xen specific code will create an hash table which
> > > > contains all these devices. The hash table will be used in need_xen_dma_ops
> > > > to check if the Xen DMA ops needs to be used for the current device.
> > > 
> > > Is it out of the question to find a field within struct device itself to
> > > store this e.g. in struct device_dma_parameters perhaps and avoid the
> > > need for a hashtable lookup.
> > > 
> > > device->iommu_group might be another option, if we can create our own
> > > group?
> > 
> > I agree that a field in struct device would be ideal.
> > Greg, get_maintainer.pl points at you as main maintainer of device.h, do
> > you have an opinion on this?
> 
> I need a whole lot more context here please.  With a patch would be even
> better so that I know exactly what you are referring to...

The Xen hypervisor tells Linux which devices are protected by an SMMU,
preprogrammed by Xen, so that Linux can avoid the swiotlb and bounce
buffers for DMA requests involving them.
The information is present on device tree and parsed at boot time by
Linux.

Julien is proposing to store the list of "safe" devices on an hash table
in the Xen specific code (in arch/arm/xen/enlighten.c, see
http://marc.info/?l=linux-kernel&m=139291370526082&w=2).
Whenever Linux is about to do DMA, we would check in the hashtable to
figure out whether we need to go through the swiotlb or we can simply
use the native dma_ops.

Ian and I were thinking that it would be much easier and faster to have
a "xen_safe_device" parameter in struct device and just check for that.
It doesn't actually need to be in struct device, it could simply be a
flag in struct device_dma_parameters as Ian was suggesting.

Julien, could you please come up with a simple patch to demonstrate the
concept?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julien Grall March 1, 2014, 3:33 p.m. | #5
Hi Stefano,

On 25/02/14 04:49, Stefano Stabellini wrote:
> Julien, could you please come up with a simple patch to demonstrate the
> concept?
>

Sure. I won't have time to write the patch next week. I will try to send 
it as soon as possible.

Cheers,

Patch

diff --git a/Documentation/devicetree/bindings/arm/xen.txt b/Documentation/devicetree/bindings/arm/xen.txt
index 0f7b9c2..ee25a57 100644
--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt
@@ -15,6 +15,8 @@  the following properties:
 - interrupts: the interrupt used by Xen to inject event notifications.
   A GIC node is also required.
 
+- protected-devices: (optional) List of phandles to device node where the
+IOMMU has been programmed by Xen.
 
 Example (assuming #address-cells = <2> and #size-cells = <2>):
 
diff --git a/arch/arm/include/asm/xen/dma-mapping.h b/arch/arm/include/asm/xen/dma-mapping.h
index 002fc57..da8e4fe 100644
--- a/arch/arm/include/asm/xen/dma-mapping.h
+++ b/arch/arm/include/asm/xen/dma-mapping.h
@@ -5,9 +5,18 @@ 
 
 extern struct dma_map_ops *xen_dma_ops;
 
+#ifdef CONFIG_XEN
+bool xen_is_protected_device(const struct device *dev);
+#else
+static inline bool xen_is_protected_device(const struct device *dev)
+{
+	return 0;
+}
+#endif
+
 static inline bool need_xen_dma_ops(struct device *dev)
 {
-	return xen_initial_domain();
+	return xen_initial_domain() && !xen_is_protected_device(dev);
 }
 
 #endif /* _ASM_ARM_XEN_DMA_MAPPING_H */
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index b96723e..f124c8c 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -24,6 +24,7 @@ 
 #include <linux/cpuidle.h>
 #include <linux/cpufreq.h>
 #include <linux/cpu.h>
+#include <linux/hashtable.h>
 
 #include <linux/mm.h>
 
@@ -53,6 +54,42 @@  EXPORT_SYMBOL_GPL(xen_platform_pci_unplug);
 
 static __read_mostly int xen_events_irq = -1;
 
+/* Hash table for list of devices protected by an IOMMU in Xen */
+#define DEV_HASH_BITS	4
+#define DEV_HASH_SIZE	(1 << DEV_HASH_BITS)
+
+static struct hlist_head *protected_devices;
+
+struct protected_device
+{
+	struct hlist_node hlist;
+	struct device_node *node;
+};
+
+static unsigned long pdev_hash(const struct device_node *node)
+{
+	return (node->phandle % DEV_HASH_SIZE);
+}
+
+bool xen_is_protected_device(const struct device *dev)
+{
+	const struct device_node *node = dev->of_node;
+	unsigned long hash;
+	const struct protected_device *pdev;
+
+	if (!node->phandle)
+		return 0;
+
+	hash = pdev_hash(node);
+
+	hlist_for_each_entry(pdev, &protected_devices[hash], hlist) {
+		if (node == pdev->node)
+			return 1;
+	}
+
+	return 0;
+}
+
 /* map fgmfn of domid to lpfn in the current domain */
 static int map_foreign_page(unsigned long lpfn, unsigned long fgmfn,
 			    unsigned int domid)
@@ -235,6 +272,8 @@  static int __init xen_guest_init(void)
 	const char *xen_prefix = "xen,xen-";
 	struct resource res;
 	phys_addr_t grant_frames;
+	int i = 0;
+	struct device_node *dev;
 
 	node = of_find_compatible_node(NULL, NULL, "xen,xen");
 	if (!node) {
@@ -259,6 +298,31 @@  static int __init xen_guest_init(void)
 	if (xen_events_irq < 0)
 		return -ENODEV;
 
+	protected_devices = kmalloc(DEV_HASH_SIZE * sizeof (*protected_devices),
+				    GFP_KERNEL);
+	if (!protected_devices)
+		return -ENOMEM;
+
+	for (i = 0; i < DEV_HASH_SIZE; i++)
+		INIT_HLIST_HEAD(&protected_devices[i]);
+
+	pr_info("List of protected devices:\n");
+	i = 0;
+	while ((dev = of_parse_phandle(node, "protected-devices", i))) {
+		struct protected_device *pdev;
+		unsigned long hash;
+
+		pr_info(" - %s\n", of_node_full_name(dev));
+		pdev = kmalloc(sizeof (*pdev), GFP_KERNEL);
+		if (!pdev)
+			goto free_hash;
+
+		pdev->node = dev;
+		hash = pdev_hash(dev);
+		hlist_add_head(&pdev->hlist, &protected_devices[hash]);
+		i++;
+	}
+
 	xen_domain_type = XEN_HVM_DOMAIN;
 
 	xen_setup_features();
@@ -324,6 +388,17 @@  static int __init xen_guest_init(void)
 	register_cpu_notifier(&xen_cpu_notifier);
 
 	return 0;
+free_hash:
+	for (i = 0; i < DEV_HASH_SIZE; i++) {
+		struct protected_device *pdev;
+		struct hlist_node *next;
+
+		hlist_for_each_entry_safe(pdev, next, &protected_devices[i],
+					  hlist)
+			kfree(pdev);
+	}
+	kfree(protected_devices);
+	return -ENOMEM;
 }
 early_initcall(xen_guest_init);