Re: [Xen-devel] [PATCH 5/5] ARM: ITS: Pass ITS in Hardware Domain MADT

[Julien Grall <julien.grall@xxxxxxx>]

Hello,

Title: xen/arm: ITS: Expose ITS in the MADT table

On 13/08/17 22:30, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
> From: Manish Jaggi <mjaggi@xxxxxxxxxx>
>
> Adds gicv3_its_make_hwdom_madt to update hwdom MADT ITS information.
s/Adds/Add/

> Signed-off-by: Manish Jaggi <mjaggi@xxxxxxxxxx>
> ---
>  xen/arch/arm/gic-v3-its.c        | 24 ++++++++++++++++++++++++
>  xen/arch/arm/gic-v3.c            |  1 +
>  xen/include/asm-arm/gic_v3_its.h |  1 +
>  3 files changed, 26 insertions(+)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 82e025e..6e0a701 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -935,6 +935,30 @@ u32 gicv3_its_madt_generic_translator_size(void)
>
>      return size;
>  }
> +
> +u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset)
Pretty much all my remarks in the previous patch for u* are valid here 
too. I will not repeat them and for the whole the patch.
> +{
> +    const struct host_its *its_data;
> +    u32 table_len = offset, i = 0, size;
> +    struct acpi_madt_generic_translator *fw_its;
> +    struct acpi_madt_generic_translator *hwdom_its;
> +
> +    size = sizeof(struct acpi_madt_generic_translator);
> +
> +    /* Update GIC ITS information in hardware domain's MADT */
> +    list_for_each_entry(its_data, &host_its_list, entry)
Please look at vgic_v3_its_count to avoid introduce dummy variables:

for ( i = 0; i < vgic_v3_its_count(...); i++ )

> +    {
> +        hwdom_its = (struct acpi_madt_generic_translator *)(base_ptr
> +                   + table_len);
> +        fw_its = (struct acpi_madt_generic_translator *)
> +                    acpi_table_get_entry_madt(
> +                        ACPI_MADT_TYPE_GENERIC_TRANSLATOR, i++);
Please check the return here + panic if it does not work.

> +        memcpy(hwdom_its, fw_its, size);
> +        table_len +=  size;
This code is too complicate for not much reason. If you do:

hwdom_its = (struct acpi_madt_generic_translator)(base_ptr + offset);

for ( i = 0; i < vgic_v3_its_count(...); i++ )
{
        fw_its = (struct .... *)acpi_table_get_...

        hwdom_its++;
}

return (offset + sizeof(...) * vgic_v3_its_count());

That would be much clear.

> +    }
> +
> +    return table_len;
> +}
>  #endif
>  /*
>   * Create the respective guest DT nodes from a list of host ITSes.
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 6c2b562..30b29c9 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1407,6 +1407,7 @@ static int gicv3_make_hwdom_madt(const struct domain *d, 
> u32 offset)
>          table_len += size;
>      }
>
> +    table_len = gicv3_its_make_hwdom_madt(base_ptr, table_len);
Newline here.

>      return table_len;
>  }
>
> diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
> index b849b16..8955451 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -139,6 +139,7 @@ void gicv3_its_dt_init(const struct dt_device_node *node);
>  int gicv3_its_acpi_init(struct acpi_subtable_header *header,
>                                      const unsigned long end);
>  u32 gicv3_its_madt_generic_translator_size(void);
> +u32 gicv3_its_make_hwdom_madt(u8 *base_ptr, u32 offset);
>  #endif
>  /* Deny iomem access for its */
>  int gicv3_its_deny_access(const struct domain *d);
Cheers,

--
Julien Grall

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