diff mbox series

[V2,1/6] iommu/arm: Add iommu_helpers.c file to keep common for IOMMUs stuff

Message ID 1564763985-20312-2-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. 2, 2019, 4:39 p.m. UTC 
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Introduce a separate file to keep various helpers which could be used
by more than one IOMMU driver in order not to duplicate code.

The first condidates to be moved to the new file are SMMU driver's
"map_page/unmap_page" callbacks. There callbacks neither contain any
SMMU specific info nor perform any SMMU specific actions and are going
to be the same across all IOMMU drivers which H/W IP shares P2M
with the CPU like SMMU does.

So, move callbacks to iommu_helpers.c for the upcoming IPMMU driver
to be able to re-use them.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 xen/drivers/passthrough/arm/Makefile        |  2 +-
 xen/drivers/passthrough/arm/iommu_helpers.c | 78 +++++++++++++++++++++++++++++
 xen/drivers/passthrough/arm/smmu.c          | 48 +-----------------
 xen/include/asm-arm/iommu.h                 |  7 +++
 4 files changed, 88 insertions(+), 47 deletions(-)
 create mode 100644 xen/drivers/passthrough/arm/iommu_helpers.c

Comments

Julien Grall Aug. 9, 2019, 5:35 p.m. UTC | #1 
Hi Oleksandr,

On 02/08/2019 17:39, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Introduce a separate file to keep various helpers which could be used
> by more than one IOMMU driver in order not to duplicate code.
> 
> The first condidates to be moved to the new file are SMMU driver's

NIT: s/condidates/candidates/

> "map_page/unmap_page" callbacks. There callbacks neither contain any
> SMMU specific info nor perform any SMMU specific actions and are going
> to be the same across all IOMMU drivers which H/W IP shares P2M
> with the CPU like SMMU does.
> 
> So, move callbacks to iommu_helpers.c for the upcoming IPMMU driver
> to be able to re-use them.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>   xen/drivers/passthrough/arm/Makefile        |  2 +-
>   xen/drivers/passthrough/arm/iommu_helpers.c | 78 +++++++++++++++++++++++++++++
>   xen/drivers/passthrough/arm/smmu.c          | 48 +-----------------
>   xen/include/asm-arm/iommu.h                 |  7 +++
>   4 files changed, 88 insertions(+), 47 deletions(-)
>   create mode 100644 xen/drivers/passthrough/arm/iommu_helpers.c
> 
> diff --git a/xen/drivers/passthrough/arm/Makefile b/xen/drivers/passthrough/arm/Makefile
> index b3efcfd..4abb87a 100644
> --- a/xen/drivers/passthrough/arm/Makefile
> +++ b/xen/drivers/passthrough/arm/Makefile
> @@ -1,2 +1,2 @@
> -obj-y += iommu.o
> +obj-y += iommu.o iommu_helpers.o
>   obj-$(CONFIG_ARM_SMMU) += smmu.o
> diff --git a/xen/drivers/passthrough/arm/iommu_helpers.c b/xen/drivers/passthrough/arm/iommu_helpers.c
> new file mode 100644
> index 0000000..53e8daa
> --- /dev/null
> +++ b/xen/drivers/passthrough/arm/iommu_helpers.c
> @@ -0,0 +1,78 @@
> +/*
> + * xen/drivers/passthrough/arm/iommu_helpers.c
> + *
> + * Contains various helpers to be used by IOMMU drivers.
> + *
> + * Copyright (C) 2019 EPAM Systems Inc.

You mostly moved the code from the SMMU code, so the copyright there should be 
retain. As this is Xen modification the copyright here should be:

  * Copyright (C) 2014 Linaro Limited.

> + *
> + * 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/lib.h>
> +#include <xen/sched.h>
> +#include <xen/iommu.h>

Could you order the headers above alphabetical please?

And also as a extra NIT a newline before 'xen' headers and 'asm' one :).

> +#include <asm/device.h>

The rest of the good looks good to me.

Cheers,
Oleksandr Tyshchenko Aug. 9, 2019, 6:10 p.m. UTC | #2 
Hi, Julien


>
> On 02/08/2019 17:39, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Introduce a separate file to keep various helpers which could be used
>> by more than one IOMMU driver in order not to duplicate code.
>>
>> The first condidates to be moved to the new file are SMMU driver's
>
> NIT: s/condidates/candidates/

ok


>
>> "map_page/unmap_page" callbacks. There callbacks neither contain any
>> SMMU specific info nor perform any SMMU specific actions and are going
>> to be the same across all IOMMU drivers which H/W IP shares P2M
>> with the CPU like SMMU does.
>>
>> So, move callbacks to iommu_helpers.c for the upcoming IPMMU driver
>> to be able to re-use them.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>   xen/drivers/passthrough/arm/Makefile        |  2 +-
>>   xen/drivers/passthrough/arm/iommu_helpers.c | 78 
>> +++++++++++++++++++++++++++++
>>   xen/drivers/passthrough/arm/smmu.c          | 48 +-----------------
>>   xen/include/asm-arm/iommu.h                 |  7 +++
>>   4 files changed, 88 insertions(+), 47 deletions(-)
>>   create mode 100644 xen/drivers/passthrough/arm/iommu_helpers.c
>>
>> diff --git a/xen/drivers/passthrough/arm/Makefile 
>> b/xen/drivers/passthrough/arm/Makefile
>> index b3efcfd..4abb87a 100644
>> --- a/xen/drivers/passthrough/arm/Makefile
>> +++ b/xen/drivers/passthrough/arm/Makefile
>> @@ -1,2 +1,2 @@
>> -obj-y += iommu.o
>> +obj-y += iommu.o iommu_helpers.o
>>   obj-$(CONFIG_ARM_SMMU) += smmu.o
>> diff --git a/xen/drivers/passthrough/arm/iommu_helpers.c 
>> b/xen/drivers/passthrough/arm/iommu_helpers.c
>> new file mode 100644
>> index 0000000..53e8daa
>> --- /dev/null
>> +++ b/xen/drivers/passthrough/arm/iommu_helpers.c
>> @@ -0,0 +1,78 @@
>> +/*
>> + * xen/drivers/passthrough/arm/iommu_helpers.c
>> + *
>> + * Contains various helpers to be used by IOMMU drivers.
>> + *
>> + * Copyright (C) 2019 EPAM Systems Inc.
>
> You mostly moved the code from the SMMU code, so the copyright there 
> should be retain. As this is Xen modification the copyright here 
> should be:
>
>  * Copyright (C) 2014 Linaro Limited.

Oh, yes. Sorry, forgot about it. Will add.


>
>> + *
>> + * 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/lib.h>
>> +#include <xen/sched.h>
>> +#include <xen/iommu.h>
>
> Could you order the headers above alphabetical please?
>
> And also as a extra NIT a newline before 'xen' headers and 'asm' one :).

Yes, will follow this rule.


>
>> +#include <asm/device.h>
>
> The rest of the good looks good to me.
>
> Cheers,
>
diff mbox series

Patch

diff --git a/xen/drivers/passthrough/arm/Makefile b/xen/drivers/passthrough/arm/Makefile
index b3efcfd..4abb87a 100644
--- a/xen/drivers/passthrough/arm/Makefile
+++ b/xen/drivers/passthrough/arm/Makefile
@@ -1,2 +1,2 @@ 
-obj-y += iommu.o
+obj-y += iommu.o iommu_helpers.o
 obj-$(CONFIG_ARM_SMMU) += smmu.o
diff --git a/xen/drivers/passthrough/arm/iommu_helpers.c b/xen/drivers/passthrough/arm/iommu_helpers.c
new file mode 100644
index 0000000..53e8daa
--- /dev/null
+++ b/xen/drivers/passthrough/arm/iommu_helpers.c
@@ -0,0 +1,78 @@ 
+/*
+ * xen/drivers/passthrough/arm/iommu_helpers.c
+ *
+ * Contains various helpers to be used by IOMMU drivers.
+ *
+ * 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/lib.h>
+#include <xen/sched.h>
+#include <xen/iommu.h>
+#include <asm/device.h>
+
+/* Should only be used if P2M Table is shared between the CPU and the IOMMU. */
+int __must_check arm_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
+                                    unsigned int flags,
+                                    unsigned int *flush_flags)
+{
+    p2m_type_t t;
+
+    /*
+     * Grant mappings can be used for DMA requests. The dev_bus_addr
+     * returned by the hypercall is the MFN (not the IPA). For device
+     * protected by an IOMMU, Xen needs to add a 1:1 mapping in the domain
+     * p2m to allow DMA request to work.
+     * This is only valid when the domain is directed mapped. Hence this
+     * function should only be used by gnttab code with gfn == mfn == dfn.
+     */
+    BUG_ON(!is_domain_direct_mapped(d));
+    BUG_ON(mfn_x(mfn) != dfn_x(dfn));
+
+    /* We only support readable and writable flags */
+    if ( !(flags & (IOMMUF_readable | IOMMUF_writable)) )
+        return -EINVAL;
+
+    t = (flags & IOMMUF_writable) ? p2m_iommu_map_rw : p2m_iommu_map_ro;
+
+    /*
+     * The function guest_physmap_add_entry replaces the current mapping
+     * if there is already one...
+     */
+    return guest_physmap_add_entry(d, _gfn(dfn_x(dfn)), _mfn(dfn_x(dfn)), 0, t);
+}
+
+/* Should only be used if P2M Table is shared between the CPU and the IOMMU. */
+int __must_check arm_iommu_unmap_page(struct domain *d, dfn_t dfn,
+                                      unsigned int *flush_flags)
+{
+    /*
+     * This function should only be used by gnttab code when the domain
+     * is direct mapped (i.e. gfn == mfn == dfn).
+     */
+    if ( !is_domain_direct_mapped(d) )
+        return -EINVAL;
+
+    return guest_physmap_remove_page(d, _gfn(dfn_x(dfn)), _mfn(dfn_x(dfn)), 0);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index f151b9f..8ae986a 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2734,50 +2734,6 @@  static void arm_smmu_iommu_domain_teardown(struct domain *d)
 	xfree(xen_domain);
 }
 
-static int __must_check arm_smmu_map_page(struct domain *d, dfn_t dfn,
-					  mfn_t mfn, unsigned int flags,
-					  unsigned int *flush_flags)
-{
-	p2m_type_t t;
-
-	/*
-	 * Grant mappings can be used for DMA requests. The dev_bus_addr
-	 * returned by the hypercall is the MFN (not the IPA). For device
-	 * protected by an IOMMU, Xen needs to add a 1:1 mapping in the domain
-	 * p2m to allow DMA request to work.
-	 * This is only valid when the domain is directed mapped. Hence this
-	 * function should only be used by gnttab code with gfn == mfn == dfn.
-	 */
-	BUG_ON(!is_domain_direct_mapped(d));
-	BUG_ON(mfn_x(mfn) != dfn_x(dfn));
-
-	/* We only support readable and writable flags */
-	if (!(flags & (IOMMUF_readable | IOMMUF_writable)))
-		return -EINVAL;
-
-	t = (flags & IOMMUF_writable) ? p2m_iommu_map_rw : p2m_iommu_map_ro;
-
-	/*
-	 * The function guest_physmap_add_entry replaces the current mapping
-	 * if there is already one...
-	 */
-	return guest_physmap_add_entry(d, _gfn(dfn_x(dfn)), _mfn(dfn_x(dfn)),
-				       0, t);
-}
-
-static int __must_check arm_smmu_unmap_page(struct domain *d, dfn_t dfn,
-                                            unsigned int *flush_flags)
-{
-	/*
-	 * This function should only be used by gnttab code when the domain
-	 * is direct mapped (i.e. gfn == mfn == dfn).
-	 */
-	if ( !is_domain_direct_mapped(d) )
-		return -EINVAL;
-
-	return guest_physmap_remove_page(d, _gfn(dfn_x(dfn)), _mfn(dfn_x(dfn)), 0);
-}
-
 static const struct iommu_ops arm_smmu_iommu_ops = {
     .init = arm_smmu_iommu_domain_init,
     .hwdom_init = arm_smmu_iommu_hwdom_init,
@@ -2786,8 +2742,8 @@  static const struct iommu_ops arm_smmu_iommu_ops = {
     .iotlb_flush_all = arm_smmu_iotlb_flush_all,
     .assign_device = arm_smmu_assign_dev,
     .reassign_device = arm_smmu_reassign_dev,
-    .map_page = arm_smmu_map_page,
-    .unmap_page = arm_smmu_unmap_page,
+    .map_page = arm_iommu_map_page,
+    .unmap_page = arm_iommu_unmap_page,
 };
 
 static __init const struct arm_smmu_device *find_smmu(const struct device *dev)
diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
index 904c9ae..20d865e 100644
--- a/xen/include/asm-arm/iommu.h
+++ b/xen/include/asm-arm/iommu.h
@@ -26,6 +26,13 @@  struct arch_iommu
 const struct iommu_ops *iommu_get_ops(void);
 void iommu_set_ops(const struct iommu_ops *ops);
 
+/* mapping helpers */
+int __must_check arm_iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
+                                    unsigned int flags,
+                                    unsigned int *flush_flags);
+int __must_check arm_iommu_unmap_page(struct domain *d, dfn_t dfn,
+                                      unsigned int *flush_flags);
+
 #endif /* __ARCH_ARM_IOMMU_H__ */
 
 /*