Hi,
On 09/02/18 03:10, Sameer Goel wrote:
This driver follows an approach similar to smmu driver. The intent here
is to reuse as much Linux code as possible.
- Glue code has been introduced to bridge the API calls.
- Called Linux functions from the Xen IOMMU function calls.
- Xen modifications are preceded by /*Xen: comment */
- xen/linux_compat: Add a Linux compat header
For porting files directly from Linux it is useful to have a function mapping
definitions from Linux to Xen. This file adds common API functions and
other defines that are needed for porting arm SMMU drivers.
I understand Roger asked for it, but that was not a really wise choice
given the size of this patch. Anyway, let's keep it like that.
Signed-off-by: Sameer Goel <sameer.g...@linaro.org>
---
xen/arch/arm/p2m.c | 1 +
xen/drivers/Kconfig | 2 +
xen/drivers/passthrough/arm/Kconfig | 8 +
xen/drivers/passthrough/arm/Makefile | 1 +
xen/drivers/passthrough/arm/smmu-v3.c | 892 ++++++++++++++++++++++++++++++++--
xen/include/xen/linux_compat.h | 84 ++++
You need to CC the REST maintainers for that.
6 files changed, 959 insertions(+), 29 deletions(-)
create mode 100644 xen/drivers/passthrough/arm/Kconfig
create mode 100644 xen/include/xen/linux_compat.h
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 65e8b9c6ea..fef7605fd6 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1460,6 +1460,7 @@ err:
static void __init setup_virt_paging_one(void *data)
{
unsigned long val = (unsigned long)data;
+ /* SMMUv3 S2 cfg vtcr reuses the following value */
WRITE_SYSREG32(val, VTCR_EL2);
isb();
}
diff --git a/xen/drivers/Kconfig b/xen/drivers/Kconfig
index bc3a54f0ea..612655386d 100644
--- a/xen/drivers/Kconfig
+++ b/xen/drivers/Kconfig
@@ -12,4 +12,6 @@ source "drivers/pci/Kconfig"
source "drivers/video/Kconfig"
+source "drivers/passthrough/arm/Kconfig"
+
endmenu
diff --git a/xen/drivers/passthrough/arm/Kconfig
b/xen/drivers/passthrough/arm/Kconfig
new file mode 100644
index 0000000000..cda899f608
--- /dev/null
+++ b/xen/drivers/passthrough/arm/Kconfig
@@ -0,0 +1,8 @@
+
+config ARM_SMMU_v3
+ bool "ARM SMMUv3 Support"
+ depends on ARM_64
Why the dependency on Arm64 here?
+ help
+ Support for implementations of the ARM System MMU architecture
+ version 3.
+
diff --git a/xen/drivers/passthrough/arm/Makefile
b/xen/drivers/passthrough/arm/Makefile
index f4cd26e15d..e14732b55c 100644
--- a/xen/drivers/passthrough/arm/Makefile
+++ b/xen/drivers/passthrough/arm/Makefile
@@ -1,2 +1,3 @@
obj-y += iommu.o
obj-y += smmu.o
+obj-$(CONFIG_ARM_SMMU_v3) += smmu-v3.o
diff --git a/xen/drivers/passthrough/arm/smmu-v3.c
b/xen/drivers/passthrough/arm/smmu-v3.c
index e67ba6c40f..f43485fe6e 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -18,28 +18,414 @@
* Author: Will Deacon <will.dea...@arm.com>
*
* This driver is powered by bad coffee and bombay mix.
+ *
+ *
+ * Based on Linux drivers/iommu/arm-smmu-v3.c
+ * => commit 7aa8619a66aea52b145e04cbab4f8d6a4e5f3f3b
+ *
+ * Xen modifications:
+ * Sameer Goel <sameer.g...@linaro.org>
+ * Copyright (C) 2017, The Linux Foundation, All rights reserved.
+ *
+ */
[...]
+static void *dmam_alloc_coherent(struct device *dev, size_t size,
+ dma_addr_t *dma_handle, gfp_t gfp)
+{
+ void *vaddr;
+ unsigned long alignment = size;
+
+ /*
+ * _xzalloc requires that the (align & (align -1)) = 0. Most of the
+ * allocations in SMMU code should send the right value for size. In
+ * case this is not true print a warning and align to the size of a
+ * (void *)
+ */
+ if (size & (size - 1)) {
+ dev_warn(dev, "Fixing alignment for the DMA buffer\n");
+ alignment = sizeof(void *);
+ }
+
+ vaddr = _xzalloc(size, alignment);
+ if (!vaddr) {
+ dev_err(dev, "DMA allocation failed\n");
+ return NULL;
+ }
+
+ *dma_handle = virt_to_maddr(vaddr);
+
+ return vaddr;
+}
+
+
One newline should be enough.
+static void dmam_free_coherent(struct device *dev, size_t size, void *vaddr,
+ dma_addr_t dma_handle)
+{
+ xfree(vaddr);
+}
+
+/* Xen: Stub out DMA domain related functions */
+#define iommu_get_dma_cookie(dom) 0
+#define iommu_put_dma_cookie(dom)
+
+/* Xen: Stub out module param related function */
+#define module_param_named(a, b, c, d)
+#define MODULE_PARM_DESC(a, b)
+
+#define dma_set_mask_and_coherent(d, b) 0
+
+#define of_dma_is_coherent(n) 0
+
+#define MODULE_DEVICE_TABLE(type, name)
+
+static void __iomem *devm_ioremap_resource(struct device *dev,
+ struct resource *res)
+{
+ void __iomem *ptr;
+
+ if (!res || res->type != IORESOURCE_MEM) {
+ dev_err(dev, "Invalid resource\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ ptr = ioremap_nocache(res->addr, res->size);
+ if (!ptr) {
+ dev_err(dev,
+ "ioremap failed (addr 0x%"PRIx64" size 0x%"PRIx64")\n",
+ res->addr, res->size);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ return ptr;
+}
+
+/* Xen: Compatibility define for iommu_domain_geometry.*/
+struct iommu_domain_geometry {
+ dma_addr_t aperture_start; /* First address that can be mapped */
+ dma_addr_t aperture_end; /* Last address that can be mapped */
+ bool force_aperture; /* DMA only allowed in mappable range? */
+};
+
+
Same here.
[...]
+
+/*
+ * Xen: The pgtable_ops are used by the S1 translations, so return the dummy
+ * address.
+ */
+#define alloc_io_pgtable_ops(f, c, o) ((struct io_pgtable_ops *)0x0)
I am slightly confused, on a previous e-mail you suggested that 0x0 is
not possible to use. The comment in arm_smmu-domain_finalise seems to
confirm that. So why the 0x0 here?
+#define free_io_pgtable_ops(o)
Please use do { } while (0)
[...]
@@ -1232,7 +1634,7 @@ static void arm_smmu_handle_ppr(struct arm_smmu_device
*smmu, u64 *evt)
dev_info(smmu->dev, "unexpected PRI request received:\n");
dev_info(smmu->dev,
- "\tsid 0x%08x.0x%05x: [%u%s] %sprivileged %s%s%s access at iova
0x%016llx\n",
+ "\tsid 0x%08x.0x%05x: [%u%s] %sprivileged %s%s%s access at iova %#"
PRIx64 "\n",
sid, ssid, grpid, last ? "L" : "",
evt[0] & PRIQ_0_PERM_PRIV ? "" : "un",
evt[0] & PRIQ_0_PERM_READ ? "R" : "",
@@ -1346,6 +1748,8 @@ static irqreturn_t arm_smmu_combined_irq_handler(int irq,
void *dev)
{
arm_smmu_gerror_handler(irq, dev);
arm_smmu_cmdq_sync_handler(irq, dev);
+ /*Xen: No threaded irq. So call the required function from here */
NIT: /* Xen: ... */
+ arm_smmu_combined_irq_thread(irq, dev);
return IRQ_WAKE_THREAD;
}
[...]
@@ -1783,7 +2239,14 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device
*smmu, u32 sid)
return sid < limit;
}
+/* Xen: Unused */
+#if 0
static struct iommu_ops arm_smmu_ops;
+#endif
+
+/* Xen: Redefine arm_smmu_ops to what fwspec should evaluate */
+static const struct iommu_ops arm_smmu_iommu_ops;
+#define arm_smmu_ops arm_smmu_iommu_ops
Hmmmmm. Why is that necessary? arm_smmu_iommu_ops is added in this
patch. So can't you just name the structure arm_smmu_ops?
Furthermore, I would be ok to leave to remove the const as Linux does
not do it.
static int arm_smmu_add_device(struct device *dev)
{
@@ -1791,8 +2254,11 @@ static int arm_smmu_add_device(struct device *dev)
struct arm_smmu_device *smmu;
struct arm_smmu_master_data *master;
struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+#if 0 /*Xen: iommu_group is not needed */
struct iommu_group *group;
+#endif
+ /* Xen: fwspec->ops are not needed */
You don't change this code. So why this comment?
if (!fwspec || fwspec->ops != &arm_smmu_ops)
return -ENODEV;
/*
@@ -1830,6 +2296,11 @@ static int arm_smmu_add_device(struct device *dev)
}
}
+/*
+ * Xen: Do not need an iommu group as the stream data is carried by the SMMU
NIT: "We don't need...".
+ * master device object
NIT: Missing full stop.
+ */
+#if 0
group = iommu_group_get_for_dev(dev);
if (!IS_ERR(group)) {
iommu_group_put(group);
[...]
@@ -2316,9 +2800,13 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device
*smmu)
* Cavium ThunderX2 implementation doesn't not support unique
* irq lines. Use single irq line for all the SMMUv3 interrupts.
*/
- ret = devm_request_threaded_irq(smmu->dev, irq,
+ /*
+ * Xen: Does not support threaded irqs, so serialise the setup.
+ * This is the same for pris and event interrupt lines on other
+ * systems
+ */
+ ret = devm_request_irq(smmu->dev, irq,
arm_smmu_combined_irq_handler,
- arm_smmu_combined_irq_thread,
IRQF_ONESHOT,
"arm-smmu-v3-combined-irq", smmu);
On "RFC v4", I asked a question which was left unanswered. Here the
conversation:
Me: Above you did implemented a dummy implementation of
devm_request_threaded_irq(...). So why did you replace the code here?
You: The replacement worked well for other functions, where the handler
was not defined. So, the wrapper function calls devm_request_irq with
the argument passed in as thread. In this case really the handler hits
first and it calls the thread in response. I can modify the code to make
this fit into the api but in that case I will need to swap around the
functions so number of line changes will stay the same. Tell me your
preference.
Me: I don't understand what you mean here. Would it be possible to give
a concrete example?
if (ret < 0)
[...]
@@ -2703,7 +3200,7 @@ static inline int arm_smmu_device_acpi_probe(struct
platform_device *pdev,
static int arm_smmu_device_dt_probe(struct platform_device *pdev,
struct arm_smmu_device *smmu)
{
- struct device *dev = &pdev->dev;
+ struct device *dev = pdev;
u32 cells;
int ret = -EINVAL;
@@ -2716,6 +3213,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
parse_driver_options(smmu);
+ /* Xen: of_dma_is_coherent is a stub till dt support is introduced */
if (of_dma_is_coherent(dev->of_node))
On RFC v4, I requested to move the message on top of of_dma_is_coherent
stub and add a WARN/WARN_ON(). Please address it.
smmu->features |= ARM_SMMU_FEAT_COHERENCY;
[...]
@@ -2844,9 +3351,20 @@ static int arm_smmu_device_probe(struct platform_device
*pdev)
if (ret)
return ret;
}
+#endif
+ /*
+ * Xen: Keep a list of all probed devices. This will be used to query
+ * the smmu devices based on the fwnode.
+ */
+ INIT_LIST_HEAD(&smmu->devices);
+ spin_lock(&arm_smmu_devices_lock);
+ list_add(&smmu->devices, &arm_smmu_devices);
+ spin_unlock(&arm_smmu_devices_lock);
return 0;
}
+/* Xen: Unused function */
+#if 0
static int arm_smmu_device_remove(struct platform_device *pdev)
{
struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
@@ -2860,6 +3378,8 @@ static void arm_smmu_device_shutdown(struct
platform_device *pdev)
{
arm_smmu_device_remove(pdev);
}
+#endif
+
Newline not necessary.
> static const struct of_device_id arm_smmu_of_match[] = {
{ .compatible = "arm,smmu-v3", },
[...]
diff --git a/xen/include/xen/linux_compat.h b/xen/include/xen/linux_compat.h
new file mode 100644
index 0000000000..8037be0a3e
--- /dev/null
+++ b/xen/include/xen/linux_compat.h
@@ -0,0 +1,84 @@
+/******************************************************************************
+ * include/xen/linux_compat.h
+ *
+ * Compatibility defines for porting code from Linux to Xen
+ *
+ * Copyright (c) 2017 Linaro Limited
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __XEN_LINUX_COMPAT_H__
+#define __XEN_LINUX_COMPAT_H__
+
+#include <asm/types.h>
+
+typedef paddr_t phys_addr_t;
+typedef paddr_t dma_addr_t;
+
+typedef unsigned int gfp_t;
+#define GFP_KERNEL 0
+#define __GFP_ZERO 0x01U
No need to the hexa here. 1U is much clearer.
+
+/* Helpers for IRQ functions */
+#define free_irq release_irq
+
+enum irqreturn {
+ IRQ_NONE,
+ IRQ_HANDLED,
+ IRQ_WAKE_THREAD,
+};
+
+typedef enum irqreturn irqreturn_t;
+
+/* Device logger functions */
+#define dev_dbg(dev, fmt, ...) printk(XENLOG_DEBUG fmt, ## __VA_ARGS__)
+#define dev_notice(dev, fmt, ...) printk(XENLOG_INFO fmt, ## __VA_ARGS__)
+#define dev_warn(dev, fmt, ...) printk(XENLOG_WARNING fmt, ## __VA_ARGS__)
+#define dev_err(dev, fmt, ...) printk(XENLOG_ERR fmt, ## __VA_ARGS__)
+#define dev_info(dev, fmt, ...) printk(XENLOG_INFO fmt, ## __VA_ARGS__)
+
+#define dev_err_ratelimited(dev, fmt, ...) \
+ printk(XENLOG_ERR fmt, ## __VA_ARGS__)
+
+#define dev_name(dev) dt_node_full_name(dev_to_dt(dev))
+
+/* Alias to Xen allocation helpers */
+#define kfree xfree
+#define kmalloc(size, flags) ({\
+ void *__ret_alloc = NULL; \
+ if (flags & __GFP_ZERO) \
+ __ret_alloc = _xzalloc(size, sizeof(void *)); \
That's Xen code, so please avoid using hard tabs.
+ else \
+ __ret_alloc = _xmalloc(size, sizeof(void *)); \
+ __ret_alloc; \
+})
Could we make at least kmalloc and kmalloc_array static inline? This
will add safety and make easier to read (the \ are not indented at all).
+#define kzalloc(size, flags) _xzalloc(size, sizeof(void *))
+#define devm_kzalloc(dev, size, flags) _xzalloc(size, sizeof(void *))
+#define kmalloc_array(size, n, flags) ({\
+ void *__ret_alloc = NULL; \
+ if (flags & __GFP_ZERO) \
+ __ret_alloc = _xzalloc_array(size, sizeof(void *), n); \
+ else \
+ __ret_alloc = _xmalloc_array(size, sizeof(void *), n); \
+ __ret_alloc; \
+})
+
+/* Alias to Xen time functions */
+#define ktime_t s_time_t
+#define ktime_get() (NOW())
+#define ktime_add_us(t,i) (t + MICROSECS(i))
+#define ktime_compare(t,i) (t > (i))
+
+#endif /* __XEN_LINUX_COMPAT_H__ */
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel