Re: [Xen-devel] [PATCH 2/5] ARM: ITS: Populate host_its_list from ACPI MADT Table

[Julien Grall <julien.grall@xxxxxxx>]

Hello,

On 13/08/17 22:30, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
> From: Manish Jaggi <mjaggi@xxxxxxxxxx>
>
> Added gicv3_its_acpi_init to update host_its_list from MADT table.
> For ACPI, host_its sturcture  stores dt_node as NULL.
s/sturcture/structure/

> Future TOD0:
> Cleanup :(1) Remove from host_its dt_node as it is required only for ACPI
> Enhancement :(2) Provide a method to access translation_id and
> other fields of madt generic translator.
I don't get those TODOs. This is not related to this patch and does not 
really hence the commit message.
> Signed-off-by: Manish Jaggi <mjaggi@xxxxxxxxxx>
> ---
>  xen/arch/arm/gic-v3-its.c        | 14 ++++++++++++++
>  xen/arch/arm/gic-v3.c            |  8 ++++++++
>  xen/include/asm-arm/gic_v3_its.h | 13 +++++++++++++
>  3 files changed, 35 insertions(+)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index f844a0d..c4f1288 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -32,6 +32,7 @@
>  #include <asm/page.h>
>
>  #define ITS_CMD_QUEUE_SZ                SZ_1M
> +#define ACPI_GICV3_ITS_MEM_SIZE        (SZ_64K)
The (...) are not necessary.

>  /*
>   * No lock here, as this list gets only populated upon boot while scanning
> @@ -1020,6 +1021,19 @@ void gicv3_its_dt_init(const struct dt_device_node *node)
>      }
>  }
>
> +#ifdef CONFIG_ACPI
> +int gicv3_its_acpi_init(struct acpi_subtable_header *header,
> +                        const unsigned long end)
I don't much like the idea of providing a callback that will be called 
by gic-v3.c. I would much prefer to follow the same pattern as the DT 
where gic-v3.c will call a function in the gic-v3-its.c that will 
iterate on all possible ITS.
This will make a more sane interface.

Also, it would make sense to call it gicv3_its_acpi_probe.

> +{
> +    struct acpi_madt_generic_translator *its;
> +
> +    its = (struct acpi_madt_generic_translator *)header;

You probably want to add check BAD_MADT_ENTRY(its, end).

> +
> +    return add_to_host_its_list(its->base_address,
> +                        ACPI_GICV3_ITS_MEM_SIZE, NULL);
If you follow my suggestion in patch #1 regarding the return of 
add_to_host_its_list, then you would need to check true/false and return 
a correct errno.
Even if you don't follow it, please return an appropriate errno rather 
than -1.
> +}
> +#endif
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index f990eae..0be8942 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1567,6 +1567,14 @@ static void __init gicv3_acpi_init(void)
>
>      gicv3.rdist_stride = 0;
>
> +    /* Parse ITS information */
> +    count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
> +                                  gicv3_its_acpi_init, 0);
See my comment above.

> +
> +    if ( count <= 0 )
Hardware without ITS support will return 0 and therefore panic. You 
don't want this to happen.
> +        panic("GICv3: Can't get ITS entry");
> +
> +
>      /*
>       * In ACPI, 0 is considered as the invalid address. However the rest
>       * of the initialization rely on the invalid address to be
> diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
> index 1fac1c7..2b7493d 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -105,6 +105,7 @@
>
>  #include <xen/device_tree.h>
>  #include <xen/rbtree.h>
> +#include <xen/acpi.h>
With the suggestion suggested above, you will not need to include 
<xen/acpi.h> in gic_v3_its.h.
>  #define HOST_ITS_FLUSH_CMD_QUEUE        (1U << 0)
>  #define HOST_ITS_USES_PTA               (1U << 1)
> @@ -135,6 +136,10 @@ extern struct list_head host_its_list;
>  /* Parse the host DT and pick up all host ITSes. */
>  void gicv3_its_dt_init(const struct dt_device_node *node);
>
> +#ifdef CONFIG_ACPI
> +int gicv3_its_acpi_init(struct acpi_subtable_header *header,
> +                                    const unsigned long end);
> +#endif
Newline here please.

>  bool gicv3_its_host_has_its(void);
>
>  unsigned int vgic_v3_its_count(const struct domain *d);
> @@ -196,6 +201,14 @@ static inline void gicv3_its_dt_init(const struct 
> dt_device_node *node)
>  {
>  }
>
> +#ifdef CONFIG_ACPI
> +static inline int gicv3_its_acpi_init(struct acpi_subtable_header *header,
> +                                    const unsigned long end)
> +{
> +    return false;
gicv3_its_acpi_init return an int not a bool. Please modify this 
accordingly.
> +}
> +#endif
> +
>  static inline bool gicv3_its_host_has_its(void)
>  {
>      return false;
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel