diff mbox series

[V3,5/8] iommu/arm: Add lightweight iommu_fwspec support

Message ID 1566324587-3442-6-git-send-email-olekstysh@gmail.com (mailing list archive)
State Superseded
Headers show
Series iommu/arm: Add Renesas IPMMU-VMSA support + Linux's iommu_fwspec | expand

Commit Message

Oleksandr Tyshchenko Aug. 20, 2019, 6:09 p.m. UTC
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

We need to have some abstract way to add new device to the IOMMU
based on the generic IOMMU DT bindings [1] which can be used for
both DT (right now) and ACPI (in future).

For that reason we can borrow the idea used in Linux these days
called "iommu_fwspec". Having this in, it will be possible
to configure IOMMU master interfaces of the device (device IDs)
from a single common place and avoid keeping almost identical look-up
implementations in each IOMMU driver.

There is no need to port the whole implementation of "iommu_fwspec"
to Xen, we could, probably, end up with a much simpler solution,
some "stripped down" version which fits our requirements.

So, this patch adds the following:
1. A common structure "iommu_fwspec" to hold the the per-device
   firmware data
2. New member "iommu_fwspec" of struct device
3. Functions/helpers to deal with "dev->iommu_fwspec"

It should be noted that in comparing with original "iommu_fwspec"
Xen's variant doesn't contain some fields, which are not really
needed at the moment (ops, flag) and "iommu_fwnode" field was replaced
by "iommu_dev" to avoid porting a lot of code (to support "fwnode_handle")
with little benefit.

Subsequent patches will use of that support.

[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/iommu/iommu.txt

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
CC: Julien Grall <julien.grall@arm.com>

---
Changes V2 -> V3:
    - added Copyright from Linux
    - ordered the headers alphabetically
    - removed check for not a NULL before calling xfree()
    - used unsigned for variables which can't be negative
    - removed #include <asm/iommu_fwspec.h> from iommu.h
    - added check to iommu_fwspec_init() to not allow overriding
    - clarified comments in code
    - modified iommu_fwspec_add_ids() to use type-safe xrealloc_flex_struct()
---
 xen/drivers/passthrough/arm/Makefile       |  2 +-
 xen/drivers/passthrough/arm/iommu_fwspec.c | 94 ++++++++++++++++++++++++++++++
 xen/include/asm-arm/device.h               |  1 +
 xen/include/asm-arm/iommu_fwspec.h         | 68 +++++++++++++++++++++
 4 files changed, 164 insertions(+), 1 deletion(-)
 create mode 100644 xen/drivers/passthrough/arm/iommu_fwspec.c
 create mode 100644 xen/include/asm-arm/iommu_fwspec.h

Comments

Julien Grall Sept. 9, 2019, 2:36 p.m. UTC | #1
Hi Oleksandr,

On 8/20/19 7:09 PM, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> We need to have some abstract way to add new device to the IOMMU
> based on the generic IOMMU DT bindings [1] which can be used for
> both DT (right now) and ACPI (in future).
> 
> For that reason we can borrow the idea used in Linux these days
> called "iommu_fwspec". Having this in, it will be possible
> to configure IOMMU master interfaces of the device (device IDs)
> from a single common place and avoid keeping almost identical look-up
> implementations in each IOMMU driver.
> 
> There is no need to port the whole implementation of "iommu_fwspec"
> to Xen, we could, probably, end up with a much simpler solution,
> some "stripped down" version which fits our requirements.
> 
> So, this patch adds the following:
> 1. A common structure "iommu_fwspec" to hold the the per-device
>     firmware data
> 2. New member "iommu_fwspec" of struct device
> 3. Functions/helpers to deal with "dev->iommu_fwspec"
> 
> It should be noted that in comparing with original "iommu_fwspec"

s/comparing with/comparison of/
s/original/the Linux/ or "the original".

> Xen's variant doesn't contain some fields, which are not really
> needed at the moment (ops, flag) and "iommu_fwnode" field was replaced
> by "iommu_dev" to avoid porting a lot of code (to support "fwnode_handle")
> with little benefit.


It would be good to mention which version of Linux this is based on. So 
if there is a critical bug in Linux we can port it.

The rest of the patch looks good to me.

Cheers,
Oleksandr Tyshchenko Sept. 9, 2019, 2:41 p.m. UTC | #2
On 09.09.19 17:36, Julien Grall wrote:
> Hi Oleksandr,

Hi, Julien


>
> On 8/20/19 7:09 PM, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> We need to have some abstract way to add new device to the IOMMU
>> based on the generic IOMMU DT bindings [1] which can be used for
>> both DT (right now) and ACPI (in future).
>>
>> For that reason we can borrow the idea used in Linux these days
>> called "iommu_fwspec". Having this in, it will be possible
>> to configure IOMMU master interfaces of the device (device IDs)
>> from a single common place and avoid keeping almost identical look-up
>> implementations in each IOMMU driver.
>>
>> There is no need to port the whole implementation of "iommu_fwspec"
>> to Xen, we could, probably, end up with a much simpler solution,
>> some "stripped down" version which fits our requirements.
>>
>> So, this patch adds the following:
>> 1. A common structure "iommu_fwspec" to hold the the per-device
>>     firmware data
>> 2. New member "iommu_fwspec" of struct device
>> 3. Functions/helpers to deal with "dev->iommu_fwspec"
>>
>> It should be noted that in comparing with original "iommu_fwspec"
>
> s/comparing with/comparison of/
> s/original/the Linux/ or "the original".

ok


>
>> Xen's variant doesn't contain some fields, which are not really
>> needed at the moment (ops, flag) and "iommu_fwnode" field was replaced
>> by "iommu_dev" to avoid porting a lot of code (to support 
>> "fwnode_handle")
>> with little benefit.
>
>
> It would be good to mention which version of Linux this is based on. 
> So if there is a critical bug in Linux we can port it.

It was based on current Linux master. I will mention exact version.
diff mbox series

Patch

diff --git a/xen/drivers/passthrough/arm/Makefile b/xen/drivers/passthrough/arm/Makefile
index 4abb87a..5fbad45 100644
--- a/xen/drivers/passthrough/arm/Makefile
+++ b/xen/drivers/passthrough/arm/Makefile
@@ -1,2 +1,2 @@ 
-obj-y += iommu.o iommu_helpers.o
+obj-y += iommu.o iommu_helpers.o iommu_fwspec.o
 obj-$(CONFIG_ARM_SMMU) += smmu.o
diff --git a/xen/drivers/passthrough/arm/iommu_fwspec.c b/xen/drivers/passthrough/arm/iommu_fwspec.c
new file mode 100644
index 0000000..12b8bae
--- /dev/null
+++ b/xen/drivers/passthrough/arm/iommu_fwspec.c
@@ -0,0 +1,94 @@ 
+/*
+ * xen/drivers/passthrough/arm/iommu_fwspec.c
+ *
+ * Contains functions to maintain per-device firmware data
+ *
+ * Based on Linux's iommu_fwspec support you can find at:
+ *    drivers/iommu/iommu.c
+ *
+ * Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
+ *
+ * Copyright (C) 2019 EPAM Systems Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * 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/>.
+ */
+
+#include <xen/iommu.h>
+#include <xen/lib.h>
+
+#include <asm/device.h>
+#include <asm/iommu_fwspec.h>
+
+int iommu_fwspec_init(struct device *dev, struct device *iommu_dev)
+{
+    struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+
+    if ( fwspec )
+    {
+        /* We expect the device to be protected by only one IOMMU. */
+        if ( fwspec->iommu_dev != iommu_dev )
+            return -EINVAL;
+
+        return 0;
+    }
+
+    fwspec = xzalloc(struct iommu_fwspec);
+    if ( !fwspec )
+        return -ENOMEM;
+
+    fwspec->iommu_dev = iommu_dev;
+    dev_iommu_fwspec_set(dev, fwspec);
+
+    return 0;
+}
+
+void iommu_fwspec_free(struct device *dev)
+{
+    struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+
+    xfree(fwspec);
+    dev_iommu_fwspec_set(dev, NULL);
+}
+
+int iommu_fwspec_add_ids(struct device *dev, uint32_t *ids,
+                         unsigned int num_ids)
+{
+    struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+    unsigned int i;
+
+    if ( !fwspec )
+        return -EINVAL;
+
+    fwspec = xrealloc_flex_struct(fwspec, struct iommu_fwspec, ids,
+                                  fwspec->num_ids + num_ids);
+    if ( !fwspec )
+        return -ENOMEM;
+
+    dev_iommu_fwspec_set(dev, fwspec);
+
+    for ( i = 0; i < num_ids; i++ )
+        fwspec->ids[fwspec->num_ids + i] = ids[i];
+
+    fwspec->num_ids += num_ids;
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index ee1c3bc..ee7cff2 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -18,6 +18,7 @@  struct device
     struct dt_device_node *of_node; /* Used by drivers imported from Linux */
 #endif
     struct dev_archdata archdata;
+    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */
 };
 
 typedef struct device device_t;
diff --git a/xen/include/asm-arm/iommu_fwspec.h b/xen/include/asm-arm/iommu_fwspec.h
new file mode 100644
index 0000000..8ce4da1
--- /dev/null
+++ b/xen/include/asm-arm/iommu_fwspec.h
@@ -0,0 +1,68 @@ 
+/*
+ * xen/include/asm-arm/iommu_fwspec.h
+ *
+ * Contains a common structure to hold the per-device firmware data and
+ * declaration of functions used to maintain that data
+ *
+ * Based on Linux's iommu_fwspec support you can find at:
+ *    include/linux/iommu.h
+ *
+ * Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
+ *
+ * Copyright (C) 2019 EPAM Systems Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * 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 __ARCH_ARM_IOMMU_FWSPEC_H__
+#define __ARCH_ARM_IOMMU_FWSPEC_H__
+
+/* per-device IOMMU instance data */
+struct iommu_fwspec {
+    /* this device's IOMMU */
+    struct device *iommu_dev;
+    /* IOMMU driver private data for this device */
+    void *iommu_priv;
+    /* number of associated device IDs */
+    unsigned int num_ids;
+    /* IDs which this device may present to the IOMMU */
+    uint32_t ids[1];
+};
+
+int iommu_fwspec_init(struct device *dev, struct device *iommu_dev);
+void iommu_fwspec_free(struct device *dev);
+int iommu_fwspec_add_ids(struct device *dev, uint32_t *ids,
+                         unsigned int num_ids);
+
+static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev)
+{
+    return dev->iommu_fwspec;
+}
+
+static inline void dev_iommu_fwspec_set(struct device *dev,
+                                        struct iommu_fwspec *fwspec)
+{
+    dev->iommu_fwspec = fwspec;
+}
+
+#endif /* __ARCH_ARM_IOMMU_FWSPEC_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */