[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH V2 4/6] iommu/arm: Add lightweight iommu_fwspec support

On 13.08.19 15:39, Julien Grall wrote:
Hi Oleksandr,

Hi Julien.

On 8/2/19 5:39 PM, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

We need to have some abstract way to add new device to the IOMMU
based on the generic IOMMU DT binding [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 identifical 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 requirments.



+ */
+#include <xen/lib.h>
+#include <xen/iommu.h>

Please order the headers alphabetically.

NIT: Can you a newline between xen and asm headers?

Will do

+#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 )
+        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);
+    if ( fwspec )

xfree is able to deal with NULL pointer, so the check is not necessary.

Yes, the reason I left this check is to not perform an extra operation (dev_iommu_fwspec_set). Shall I drop this check anyway?

+    {
+        xfree(fwspec);
+        dev_iommu_fwspec_set(dev, NULL);
+    }
+int iommu_fwspec_add_ids(struct device *dev, uint32_t *ids, int num_ids)

While I realize the prototype is coming from Linux, num_ids cannot be negative (the code below would not work properly). So the parameter should be unsigned.

Agree, will use unsigned.

+    struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+    size_t size;
+    int i;

Any variable that can't be negative should be unsigned.

Yes, will follow.

diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
index 20d865e..1853bd9 100644
--- a/xen/include/asm-arm/iommu.h
+++ b/xen/include/asm-arm/iommu.h
@@ -14,6 +14,8 @@
  #ifndef __ARCH_ARM_IOMMU_H__
  #define __ARCH_ARM_IOMMU_H__
  +#include <asm/iommu_fwspec.h>

iommu.h does not seem to use anything defined in iommu_fwspec.h. So why do you include it here?

I was thinking that every source which includes iommu.h will get iommu_fwspec.h included indirectly. No need to include iommu_fwspec.h in many sources.

This was a reason. Shall I included it directly where needed?

  struct arch_iommu
      /* Private information for the IOMMU drivers */
diff --git a/xen/include/asm-arm/iommu_fwspec.h b/xen/include/asm-arm/iommu_fwspec.h
new file mode 100644
index 0000000..0676285
--- /dev/null
+++ b/xen/include/asm-arm/iommu_fwspec.h
@@ -0,0 +1,65 @@
+ * 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) 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
+ * 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/>.
+ */
+/* per-device IOMMU instance data */
+struct iommu_fwspec {
+    /* device which represents this IOMMU H/W */

Did you intend to say "this device's IOMMU"?



Oleksandr Tyshchenko

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.