Re: [Xen-devel] [PATCH V2 1/6] iommu/arm: Add iommu_helpers.c file to keep common for IOMMUs stuff

[Julien Grall <julien.grall@xxxxxxx>]

Hi Oleksandr,

On 02/08/2019 17:39, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>
> 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@xxxxxxxx>
> ---
>   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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel