Re: [RFC 09/11] Xen IORT Changes

[Julien Grall <julien.grall@linaro.org>]

Hi Manish,

On 02/01/18 09:28, manish.jaggi@linaro.org wrote:
> From: Manish Jaggi <manish.jaggi@linaro.org>
> 
> This patch adds xen specific changes to iort.c

When I see the diff below, it is not changed but fully rewrite.

The original file contains 1279 lines, but you remove 978 lines and add 
71. In that context, I see no value to re-use Linux code.

> 
> Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
> ---
>   xen/arch/arm/setup.c          |    2 +
>   xen/drivers/acpi/arm/Makefile |    1 +
>   xen/drivers/acpi/arm/iort.c   | 1040 +++--------------------------------------
>   xen/include/acpi/acpi_iort.h  |    6 +-
>   4 files changed, 71 insertions(+), 978 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 16a3b1be8e..7ada48920f 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -37,6 +37,7 @@
>   #include <xen/vmap.h>
>   #include <xen/libfdt/libfdt.h>
>   #include <xen/acpi.h>
> +#include <acpi/acpi_iort.h>
>   #include <asm/alternative.h>
>   #include <asm/page.h>
>   #include <asm/current.h>
> @@ -800,6 +801,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>   
>       tasklet_subsys_init();
>   
> +    acpi_iort_init();

Please no. I can't see any reason to call acpi_iort_init() in common 
code. Instead this should be an initcall().

If you think differently, please explain why.

>   
>       xsm_dt_init();
>   
> diff --git a/xen/drivers/acpi/arm/Makefile b/xen/drivers/acpi/arm/Makefile
> index 13f1a9159f..5d16161016 100644
> --- a/xen/drivers/acpi/arm/Makefile
> +++ b/xen/drivers/acpi/arm/Makefile
> @@ -1,2 +1,3 @@
>   obj-y = ridmap.o
>   obj-y += gen-iort.o
> +obj-y += iort.o
> diff --git a/xen/drivers/acpi/arm/iort.c b/xen/drivers/acpi/arm/iort.c
> index de56394dd1..a47ee2df4c 100644
> --- a/xen/drivers/acpi/arm/iort.c
> +++ b/xen/drivers/acpi/arm/iort.c
> @@ -14,17 +14,20 @@
>    * This file implements early detection/parsing of I/O mapping
>    * reported to OS through firmware via I/O Remapping Table (IORT)
>    * IORT document number: ARM DEN 0049A
> + *
> + * Imported from Linux 4.14.0
> + * Xen Modifications : Manish Jaggi <manish.jaggi@linaro.org>
>    */
>   
>   #define pr_fmt(fmt)	"ACPI: IORT: " fmt
>   
> -#include <linux/acpi_iort.h>
> -#include <linux/iommu.h>
> -#include <linux/kernel.h>
> -#include <linux/list.h>
> -#include <linux/pci.h>
> -#include <linux/platform_device.h>
> -#include <linux/slab.h>
> +#include <acpi/acpi_iort.h>
> +#include <acpi/ridmap.h>
> +#include <xen/iommu.h>
> +#include <xen/kernel.h>
> +#include <xen/list.h>
> +#include <xen/lib.h>
> +#include <xen/pci.h>
>   
>   #define IORT_TYPE_MASK(type)	(1 << (type))
>   #define IORT_MSI_TYPE		(1 << ACPI_IORT_NODE_ITS_GROUP)
> @@ -36,6 +39,22 @@
>   #define ACPI_IORT_SMMU_V3_CAVIUM_CN99XX		0x2
>   #endif
>   
> +/* Redefine WARN macros */
> +#undef WARN
> +#undef WARN_ON
> +#define WARN(condition, format...) ({					\
> +	int __ret_warn_on = !!(condition);				\
> +	if (unlikely(__ret_warn_on))					\
> +		printk(format);						\
> +	unlikely(__ret_warn_on);					\
> +})
> +#define WARN_TAINT(cond, taint, format...) WARN(cond, format)
> +#define WARN_ON(cond)                      (!!cond)

Again, I would have appreciated if you look at the comment I made on 
Sameer series.

> +
> +
> +#define MAX_ERRNO	4095
> +#define IS_ERR_VALUE(x) unlikely((unsigned long)(void *)(x) >= (unsigned long)-MAX_ERRNO)

IS_ERR_VALUE already exists in Xen (see include/xen/err.h).

For the rest of the code, I will wait your answer on my first comment in 
that e-mail.

Cheers,

-- 
Julien Grall

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