Re: [Xen-devel] [RFC v4 6/8] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver
Hi,
I am slightly confused. I receive your answer on this e-mail after you
resend a version. So were the comments on RFC v4 was addressed?
On 02/09/2018 05:56 PM, Sameer Goel wrote:
/* Request interrupt lines */
irq = smmu->evtq.q.irq;
@@ -2316,9 +2782,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
+ */
Above you did implemented a dummy implementation of
devm_request_threaded_irq(...). So why did you replace the code here?
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.
I don't understand what you mean here. Would it be possible to give a
concrete example?
+ 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);
if (ret < 0)
@@ -2542,8 +3012,14 @@ static int arm_smmu_device_hw_probe(struct
arm_smmu_device *smmu)
smmu->features |= ARM_SMMU_FEAT_STALLS;
}
+/*
+ * Xen: Block stage 1 translations. By doing this here we do not need to set
the
+ * domain->stage explicitly.
+ */
+#if 0
if (reg & IDR0_S1P)
smmu->features |= ARM_SMMU_FEAT_TRANS_S1;
+#endif
if (reg & IDR0_S2P)
smmu->features |= ARM_SMMU_FEAT_TRANS_S2;
@@ -2616,10 +3092,12 @@ static int arm_smmu_device_hw_probe(struct
arm_smmu_device *smmu)
if (reg & IDR5_GRAN4K)
smmu->pgsize_bitmap |= SZ_4K | SZ_2M | SZ_1G;
+#if 0 /* Xen: SMMU ops do not have a pgsize_bitmap member for Xen */
if (arm_smmu_ops.pgsize_bitmap == -1UL)
arm_smmu_ops.pgsize_bitmap = smmu->pgsize_bitmap;
else
arm_smmu_ops.pgsize_bitmap |= smmu->pgsize_bitmap;
+#endif
/* Output address size */
switch (reg & IDR5_OAS_MASK << IDR5_OAS_SHIFT) {
@@ -2680,7 +3158,8 @@ static int arm_smmu_device_acpi_probe(struct
platform_device *pdev,
struct device *dev = smmu->dev;
struct acpi_iort_node *node;
- node = *(struct acpi_iort_node **)dev_get_platdata(dev);
+ /* Xen: Modification to get iort_node */
+ node = (struct acpi_iort_node *)dev->acpi_node;
/* Retrieve SMMUv3 specific data */
iort_smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
@@ -2703,7 +3182,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 +3195,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 */
I think this comment matters more on top of the stub. I would also add an BUG()
in the stub to catch it.
I have just returned a 0 here. Putting a BUG in might not have the desired
impact, since, this will execute every time.
How about a WARN_ON_ONCE in the implementation?
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v4 6/8] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver
On 1/23/2018 8:18 AM, Julien Grall wrote:
> Hi Sameer,
>
> On 19/12/17 03:17, Sameer Goel wrote:
>> + if (!dtprop)
>> + return -EINVAL;
>> +
>> + if (!dtprop->value)
>> + return -ENODATA;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/*
>> + * Xen: Helpers for DMA allocation. Just the function name is reused for
>> + * porting code these allocation are not managed allocations
>> + */
>> +
>> +void *dmam_alloc_coherent(struct device *dev, size_t size,
>
> This should be static I think.
>
>> + 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;
>> +}
>> +
>> +
>> +void dmam_free_coherent(struct device *dev, size_t size, void *vaddr,
>
> Ditto.
>
>> + 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)
>> +#define of_device_id dt_device_match
>> +
>> +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? */
>> +};
>> +
>> +
>> +/* Xen: Type definitions for iommu_domain */
>> +#define IOMMU_DOMAIN_UNMANAGED 0
>> +#define IOMMU_DOMAIN_DMA 1
>> +#define IOMMU_DOMAIN_IDENTITY 2
>> +
>> +/* Xen: Dummy iommu_domain */
>> +struct iommu_domain {
>> + /* Runtime SMMU configuration for this iommu_domain */
>> + struct arm_smmu_domain *priv;
>> + unsigned int type;
>> +
>> + /* Dummy compatibility defines */
>> + unsigned long pgsize_bitmap;
>> + struct iommu_domain_geometry geometry;
>> +
>> + atomic_t ref;
>> + /*
>> + * Used to link iommu_domain contexts for a same domain.
>> + * There is at least one per-SMMU to used by the domain.
>> + */
>> + struct list_head list;
>> +};
>> +
>> +
>
> No need for 2 newlines. Can you drop one?
>
>> +/* Xen: Describes information required for a Xen domain */
>> +struct arm_smmu_xen_domain {
>> + spinlock_t lock;
>> + /* List of iommu domains associated to this domain */
>> + struct list_head iommu_domains;
>> +};
>> +
>> +/*
>> + * Xen: Information about each device stored in dev->archdata.iommu
>> + *
>> + * The dev->archdata.iommu stores the iommu_domain (runtime configuration of
>> + * the SMMU).
>> */
>> +struct arm_smmu_xen_device {
>> + struct iommu_domain *domain;
>> +};
>> +
>> +/*
>> + * Xen: io_pgtable compatibility defines.
>> + * Most of these are to port in the S1 translation code as is.
>> + */
>> +struct io_pgtable_ops {
>> +};
>> +
>> +struct iommu_gather_ops {
>> + void (*tlb_flush_all)(void *cookie);
>> + void (*tlb_add_flush)(unsigned long iova, size_t size, size_t granule,
>> + bool leaf, void *cookie);
>> + void (*tlb_sync)(void *cookie);
>> +};
>> +
>> +struct io_pgtable_cfg {
>> + /*
>> + * IO_PGTABLE_QUIRK_ARM_NS: (ARM formats) Set NS and NSTABLE bits in
>> + * stage 1 PTEs, for hardware which insists on validating them
>> + * even in non-secure state where they should normally be ignored.
>> + *
>> + * IO_PGTABLE_QUIRK_NO_PERMS: Ignore the IOMMU_READ, IOMMU_WRITE and
>> + * IOMMU_NOEXEC
Re: [Xen-devel] [RFC v4 6/8] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver
Hi Sameer,
On 19/12/17 03:17, 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 */
Signed-off-by: Sameer Goel
---
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 | 880 --
5 files changed, 861 insertions(+), 31 deletions(-)
create mode 100644 xen/drivers/passthrough/arm/Kconfig
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 22165ae376..6aa24cae48 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 00..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
+ 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..3488184ad4 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -18,28 +18,402 @@
* Author: Will Deacon
*
* 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
+ * Copyright (C) 2017, The Linux Foundation, All rights reserved.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+
No need for 2 newlines. Can you drop one?
+/* Xen: Helpers to get device MMIO and IRQs */
+struct resource {
+ u64 addr;
+ u64 size;
+ unsigned int type;
+};
+
+#define resource_size(res) ((res)->size)
+
+#define platform_device device
+
+#define IORESOURCE_MEM 0
+#define IORESOURCE_IRQ 1
+
+static struct resource *platform_get_resource(struct platform_device *pdev,
+ unsigned int type,
+ unsigned int num)
+{
+ /*
+* The resource is only used between 2 calls of platform_get_resource.
+* It's quite ugly but it's avoid to add too much code in the part
+* imported from Linux
+*/
+ static struct resource res;
+ struct acpi_iort_node *iort_node;
+ struct acpi_iort_smmu_v3 *node_smmu_data;
+ int ret = 0;
+
+ res.type = type;
+
+ switch (type) {
+ case IORESOURCE_MEM:
+ if (pdev->type == DEV_ACPI) {
+ ret = 1;
+ iort_node = pdev->acpi_node;
+ node_smmu_data =
+ (struct acpi_iort_smmu_v3
*)iort_node->node_data;
+
+ if (node_smmu_data != NULL) {
+ res.addr = node_smmu_data->base_address;
+ res.size = SZ_128K;
+ ret = 0;
+ }
+ } else {
+ ret = dt_device_get_address(dev_to_dt(pdev), num,
+ &res.addr, &res.size);
+ }
+
+ return ((ret) ? NULL : &res);
+
+ case IORESOURCE_IRQ:
+ /* ACPI case not implemented as there is no use case for it */
+ ret = platform_get_irq(dev_to_dt(pdev), num);
+
+ if (ret < 0)
+ return NULL;
+
+ res.addr = ret;
+ res.size = 1;
+
+ return &res;
+
+ default:
+ return NULL;
+ }
+
Re: [Xen-devel] [RFC v4 6/8] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver
Hi,
On 16/01/18 12:37, Manish Jaggi wrote:
On 01/16/2018 02:04 AM, Julien Grall wrote:
On 01/03/2018 05:47 AM, Manish Jaggi wrote:
+int devm_request_threaded_irq(struct device *dev, unsigned int irq,
irq_handler_t handler,
+ irq_handler_t thread_fn, unsigned long irqflags,
+ const char *devname, void *dev_id)
+{
+ return devm_request_irq(dev, irq, thread_fn, irqflags, devname,
dev_id);
+}
Is it possible to change the name from threaded to something more
meaningful as IIUC in xen we dont have threaded irqs.
Though the code is coming from linux, but it has to be called/named
in the place it is intended to be used
What do you mean? This is a wrapper for Linux. So we should keep the
name as it is.
It creates confusion as xne doesnt use threads. So we should rename it.
There is no reason why we cannot rename a function used in linux.
Read my previous e-mail until the end to understand the reason...
[...]
@@ -433,6 +807,7 @@ enum pri_resp {
PRI_RESP_SUCC,
};
+#if 0 /* Xen: No MSI support in this iteration */
enum arm_smmu_msi_index {
EVTQ_MSI_INDEX,
GERROR_MSI_INDEX,
@@ -457,6 +832,7 @@ static phys_addr_t
arm_smmu_msi_cfg[ARM_SMMU_MAX_MSIS][3] = {
ARM_SMMU_PRIQ_IRQ_CFG2,
},
};
+#endif
IMHO can we avoid #if 0 from the code, unless we intend to use the
code in future.
In the past, I made the mistake to remove all unecessary code from
SMMUv2. Few months after, we decided to delete it and import directly
from Linux with limited modifications. This was the best choice
because it is easier to track difference.
We are in the same situation here. We want to stay as close as Linux.
This means no renaming, no code removal, and very limited change in
the code to accommodate Xen.
... particularly this paragraph.
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v4 6/8] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver
Hi Julien,
On 01/16/2018 02:04 AM, Julien Grall wrote:
On 01/03/2018 05:47 AM, Manish Jaggi wrote:
Hi Sameer,
Hi Manish,
+
+/* Xen: Type definitions for iommu_domain */
+#define IOMMU_DOMAIN_UNMANAGED 0
+#define IOMMU_DOMAIN_DMA 1
+#define IOMMU_DOMAIN_IDENTITY 2
+
+/* Xen: Dummy iommu_domain */
+struct iommu_domain {
+ /* Runtime SMMU configuration for this iommu_domain */
+ struct arm_smmu_domain *priv;
Can we use a more meaningful name in place of priv.
I believe this was taken from the iommu_domain structure in SMMUv2
which was based on an ancient version of Linux. It looks like recent
Linux will not use priv, so I am wondering why it is added here?
+ unsigned int type;
+
+ /* Dummy compatibility defines */
+ unsigned long pgsize_bitmap;
+ struct iommu_domain_geometry geometry;
+
+ atomic_t ref;
+ /*
+ * Used to link iommu_domain contexts for a same domain.
+ * There is at least one per-SMMU to used by the domain.
+ */
+ struct list_head list;
+};
+
+
+/* Xen: Describes information required for a Xen domain */
+struct arm_smmu_xen_domain {
+ spinlock_t lock;
+ /* List of iommu domains associated to this domain */
+ struct list_head iommu_domains;
+};
+
+/*
+ * Xen: Information about each device stored in dev->archdata.iommu
+ *
+ * The dev->archdata.iommu stores the iommu_domain (runtime
configuration of
+ * the SMMU).
*/
+struct arm_smmu_xen_device {
+ struct iommu_domain *domain;
domain name is confusing, if you read just the variable name it is
not easy to understand that it is a struct domain pointer or few
other structures which have _domain in their names.
Same comment for all usages of variables with just the name domain.
If this is used by Xen only code, then it should be alright. Which
name do you suggest? iommu_domain?
Looks ok.
[...]
+/*
+ * 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
*)0xDEADBEEF)
+#define free_io_pgtable_ops(o) (o = 0)
+
+/* Xen: Define wrapper for requesting IRQs */
+#define IRQF_ONESHOT 0
+
+typedef void (*irq_handler_t)(int, void *, struct cpu_user_regs *);
+
+static inline int devm_request_irq(struct device *dev, unsigned int
irq,
+ irq_handler_t handler, unsigned long irqflags,
+ const char *devname, void *dev_id)
+{
+ /*TODO: Check if we really need to set a type */
+ irq_set_type(irq, IRQ_TYPE_EDGE_BOTH);
+ return request_irq(irq, irqflags, handler, devname, dev_id);
+
+}
+
+int devm_request_threaded_irq(struct device *dev, unsigned int irq,
irq_handler_t handler,
+ irq_handler_t thread_fn, unsigned long irqflags,
+ const char *devname, void *dev_id)
+{
+ return devm_request_irq(dev, irq, thread_fn, irqflags, devname,
dev_id);
+}
Is it possible to change the name from threaded to something more
meaningful as IIUC in xen we dont have threaded irqs.
Though the code is coming from linux, but it has to be called/named
in the place it is intended to be used
What do you mean? This is a wrapper for Linux. So we should keep the
name as it is.
It creates confusion as xne doesnt use threads. So we should rename it.
There is no reason why we cannot rename a function used in linux.
[...]
@@ -433,6 +807,7 @@ enum pri_resp {
PRI_RESP_SUCC,
};
+#if 0 /* Xen: No MSI support in this iteration */
enum arm_smmu_msi_index {
EVTQ_MSI_INDEX,
GERROR_MSI_INDEX,
@@ -457,6 +832,7 @@ static phys_addr_t
arm_smmu_msi_cfg[ARM_SMMU_MAX_MSIS][3] = {
ARM_SMMU_PRIQ_IRQ_CFG2,
},
};
+#endif
IMHO can we avoid #if 0 from the code, unless we intend to use the
code in future.
In the past, I made the mistake to remove all unecessary code from
SMMUv2. Few months after, we decided to delete it and import directly
from Linux with limited modifications. This was the best choice
because it is easier to track difference.
We are in the same situation here. We want to stay as close as Linux.
This means no renaming, no code removal, and very limited change in
the code to accommodate Xen.
In that particular case, we likely going to want to support MSIs
because an implementation may only support that.
ok for this case, but the code looks clumsy.
Cheers,
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v4 6/8] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver
On 01/03/2018 05:47 AM, Manish Jaggi wrote:
Hi Sameer,
Hi Manish,
+
+/* Xen: Type definitions for iommu_domain */
+#define IOMMU_DOMAIN_UNMANAGED 0
+#define IOMMU_DOMAIN_DMA 1
+#define IOMMU_DOMAIN_IDENTITY 2
+
+/* Xen: Dummy iommu_domain */
+struct iommu_domain {
+ /* Runtime SMMU configuration for this iommu_domain */
+ struct arm_smmu_domain *priv;
Can we use a more meaningful name in place of priv.
I believe this was taken from the iommu_domain structure in SMMUv2 which
was based on an ancient version of Linux. It looks like recent Linux
will not use priv, so I am wondering why it is added here?
+ unsigned int type;
+
+ /* Dummy compatibility defines */
+ unsigned long pgsize_bitmap;
+ struct iommu_domain_geometry geometry;
+
+ atomic_t ref;
+ /*
+ * Used to link iommu_domain contexts for a same domain.
+ * There is at least one per-SMMU to used by the domain.
+ */
+ struct list_head list;
+};
+
+
+/* Xen: Describes information required for a Xen domain */
+struct arm_smmu_xen_domain {
+ spinlock_t lock;
+ /* List of iommu domains associated to this domain */
+ struct list_head iommu_domains;
+};
+
+/*
+ * Xen: Information about each device stored in dev->archdata.iommu
+ *
+ * The dev->archdata.iommu stores the iommu_domain (runtime
configuration of
+ * the SMMU).
*/
+struct arm_smmu_xen_device {
+ struct iommu_domain *domain;
domain name is confusing, if you read just the variable name it is not
easy to understand that it is a struct domain pointer or few other
structures which have _domain in their names.
Same comment for all usages of variables with just the name domain.
If this is used by Xen only code, then it should be alright. Which name
do you suggest? iommu_domain?
[...]
+/*
+ * 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
*)0xDEADBEEF)
+#define free_io_pgtable_ops(o) (o = 0)
+
+/* Xen: Define wrapper for requesting IRQs */
+#define IRQF_ONESHOT 0
+
+typedef void (*irq_handler_t)(int, void *, struct cpu_user_regs *);
+
+static inline int devm_request_irq(struct device *dev, unsigned int irq,
+ irq_handler_t handler, unsigned long irqflags,
+ const char *devname, void *dev_id)
+{
+ /*TODO: Check if we really need to set a type */
+ irq_set_type(irq, IRQ_TYPE_EDGE_BOTH);
+ return request_irq(irq, irqflags, handler, devname, dev_id);
+
+}
+
+int devm_request_threaded_irq(struct device *dev, unsigned int irq,
irq_handler_t handler,
+ irq_handler_t thread_fn, unsigned long irqflags,
+ const char *devname, void *dev_id)
+{
+ return devm_request_irq(dev, irq, thread_fn, irqflags, devname,
dev_id);
+}
Is it possible to change the name from threaded to something more
meaningful as IIUC in xen we dont have threaded irqs.
Though the code is coming from linux, but it has to be called/named in
the place it is intended to be used
What do you mean? This is a wrapper for Linux. So we should keep the
name as it is.
[...]
@@ -433,6 +807,7 @@ enum pri_resp {
PRI_RESP_SUCC,
};
+#if 0 /* Xen: No MSI support in this iteration */
enum arm_smmu_msi_index {
EVTQ_MSI_INDEX,
GERROR_MSI_INDEX,
@@ -457,6 +832,7 @@ static phys_addr_t
arm_smmu_msi_cfg[ARM_SMMU_MAX_MSIS][3] = {
ARM_SMMU_PRIQ_IRQ_CFG2,
},
};
+#endif
IMHO can we avoid #if 0 from the code, unless we intend to use the code
in future.
In the past, I made the mistake to remove all unecessary code from
SMMUv2. Few months after, we decided to delete it and import directly
from Linux with limited modifications. This was the best choice because
it is easier to track difference.
We are in the same situation here. We want to stay as close as Linux.
This means no renaming, no code removal, and very limited change in the
code to accommodate Xen.
In that particular case, we likely going to want to support MSIs because
an implementation may only support that.
Cheers,
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC v4 6/8] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver
Hi Sameer,
Comments on this patch are only for nomenclature...
On 12/19/2017 08:47 AM, 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 */
Signed-off-by: Sameer Goel
---
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 | 880 --
5 files changed, 861 insertions(+), 31 deletions(-)
create mode 100644 xen/drivers/passthrough/arm/Kconfig
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 22165ae376..6aa24cae48 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 00..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
+ 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..3488184ad4 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -18,28 +18,402 @@
* Author: Will Deacon
*
* 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
+ * Copyright (C) 2017, The Linux Foundation, All rights reserved.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+
+/* Xen: Helpers to get device MMIO and IRQs */
+struct resource {
+ u64 addr;
+ u64 size;
+ unsigned int type;
+};
+
+#define resource_size(res) ((res)->size)
+
+#define platform_device device
+
+#define IORESOURCE_MEM 0
+#define IORESOURCE_IRQ 1
+
+static struct resource *platform_get_resource(struct platform_device *pdev,
+ unsigned int type,
+ unsigned int num)
+{
+ /*
+* The resource is only used between 2 calls of platform_get_resource.
+* It's quite ugly but it's avoid to add too much code in the part
+* imported from Linux
+*/
+ static struct resource res;
+ struct acpi_iort_node *iort_node;
+ struct acpi_iort_smmu_v3 *node_smmu_data;
+ int ret = 0;
+
+ res.type = type;
+
+ switch (type) {
+ case IORESOURCE_MEM:
+ if (pdev->type == DEV_ACPI) {
+ ret = 1;
+ iort_node = pdev->acpi_node;
+ node_smmu_data =
+ (struct acpi_iort_smmu_v3
*)iort_node->node_data;
+
+ if (node_smmu_data != NULL) {
+ res.addr = node_smmu_data->base_address;
+ res.size = SZ_128K;
+ ret = 0;
+ }
+ } else {
+ ret = dt_device_get_address(dev_to_dt(pdev), num,
+ &res.addr, &res.size);
+ }
+
+ return ((ret) ? NULL : &res);
+
+ case IORESOURCE_IRQ:
+ /* ACPI case not implemented as there is no use case for it */
+ ret = platform_get_irq(dev_to_dt(pdev), num);
+
+ if (ret < 0)
+ return NULL;
+
+ res.addr = ret;
+ res.size = 1;
+
+ return &res;
+
+ default:
+ return NULL
