[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
s/identifical/identical/
ok
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.
s/requirments/requirements/
ok
+ */
+
+#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
+ * 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 {
+ /* device which represents this IOMMU H/W */
Did you intend to say "this device's IOMMU"?
Exactly)
--
Regards,
Oleksandr Tyshchenko
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
