Re: [PATCH 4/5] ARM: Introduce get_hwdom_madt_size in gic_hw_operations

[Julien Grall <julien.grall@arm.com>]

Hello,

On 13/08/17 22:30, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@cavium.com>
>
> estimate_acpi_efi_size needs to be updated to provide correct size of
> hardware domains MADT, which now adds ITS information as well.
>
> Introducing gic_get_hwdom_madt_size.
>
> Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
> ---
>  xen/arch/arm/domain_build.c      |  7 +------
>  xen/arch/arm/gic-v2.c            |  6 ++++++
>  xen/arch/arm/gic-v3-its.c        | 12 ++++++++++++
>  xen/arch/arm/gic-v3.c            | 17 +++++++++++++++++
>  xen/arch/arm/gic.c               | 11 +++++++++++
>  xen/include/asm-arm/gic.h        |  3 +++
>  xen/include/asm-arm/gic_v3_its.h |  6 ++++++
>  7 files changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 1bec4fa..5739ea4 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1806,12 +1806,7 @@ static int estimate_acpi_efi_size(struct domain *d, struct kernel_info *kinfo)
>      acpi_size = ROUNDUP(sizeof(struct acpi_table_fadt), 8);
>      acpi_size += ROUNDUP(sizeof(struct acpi_table_stao), 8);
>
> -    madt_size = sizeof(struct acpi_table_madt)
> -                + sizeof(struct acpi_madt_generic_interrupt) * d->max_vcpus
> -                + sizeof(struct acpi_madt_generic_distributor);
> -    if ( d->arch.vgic.version == GIC_V3 )
> -        madt_size += sizeof(struct acpi_madt_generic_redistributor)
> -                     * d->arch.vgic.nr_regions;
> +    madt_size = gic_get_hwdom_madt_size(d);
>      acpi_size += ROUNDUP(madt_size, 8);
>
>      addr = acpi_os_get_root_pointer();
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index cbe71a9..f5ca227 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -1012,6 +1012,11 @@ static int gicv2_iomem_deny_access(const struct domain *d)
>      return iomem_deny_access(d, mfn, mfn + nr);
>  }
>
> +static u32 gicv2_get_hwdom_madt_size(const struct domain *d)

Please no more use of u32, use uint32_t. But, the return is a bit weird. 
Why 32-bit when the code is using size_t.

I think this should return unsigned long given you use them in 
combination with _xmalloc.

> +{
> +    return 0;
> +}
> +
>  #ifdef CONFIG_ACPI
>  static int gicv2_make_hwdom_madt(const struct domain *d, u32 offset)
>  {
> @@ -1248,6 +1253,7 @@ const static struct gic_hw_operations gicv2_ops = {
>      .read_apr            = gicv2_read_apr,
>      .make_hwdom_dt_node  = gicv2_make_hwdom_dt_node,
>      .make_hwdom_madt     = gicv2_make_hwdom_madt,
> +    .get_hwdom_madt_size = gicv2_get_hwdom_madt_size,
>      .map_hwdom_extra_mappings = gicv2_map_hwdown_extra_mappings,
>      .iomem_deny_access   = gicv2_iomem_deny_access,
>      .do_LPI              = gicv2_do_LPI,
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index f584d33..82e025e 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -924,6 +924,18 @@ int gicv3_its_deny_access(const struct domain *d)
>      return rc;
>  }
>
> +#ifdef CONFIG_ACPI
> +u32 gicv3_its_madt_generic_translator_size(void)

See my comment above.

> +{
> +    const struct host_its *its_data;
> +    u32 size = 0;

Same here.

> +
> +    list_for_each_entry(its_data, &host_its_list, entry)
> +        size += sizeof(struct acpi_madt_generic_translator);
> +
> +    return size;
> +}

Overall, I don't think this function is necessary. Instead you should 
use vgic_v3_its_count given that the ACPI table will for the hardware 
domain.

> +#endif
>  /*
>   * Create the respective guest DT nodes from a list of host ITSes.
>   * This copies the reg property, so the guest sees the ITS at the same address
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 045d20d..6c2b562 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1410,6 +1410,17 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>      return table_len;
>  }
>
> +static u32 gicv3_get_hwdom_madt_size(const struct domain *d)

Ditto

> +{
> +    u32 size;

Ditto + newline here.

> +    size  = sizeof(struct acpi_madt_generic_redistributor)
> +                     * d->arch.vgic.nr_regions;
> +    if ( gicv3_its_host_has_its() )
> +        size  += gicv3_its_madt_generic_translator_size();

size += vgic_v3_its_count(d);

> +
> +    return size;
> +}
> +
>  static int __init
>  gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>                          const unsigned long end)
> @@ -1607,6 +1618,11 @@ static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>  {
>      return 0;
>  }
> +
> +static u32 gicv3_get_hwdom_madt_size(const struct domain *d)
> +{
> +    return 0;
> +}
>  #endif
>
>  /* Set up the GIC */
> @@ -1708,6 +1724,7 @@ static const struct gic_hw_operations gicv3_ops = {
>      .secondary_init      = gicv3_secondary_cpu_init,
>      .make_hwdom_dt_node  = gicv3_make_hwdom_dt_node,
>      .make_hwdom_madt     = gicv3_make_hwdom_madt,
> +    .get_hwdom_madt_size = gicv3_get_hwdom_madt_size,
>      .iomem_deny_access   = gicv3_iomem_deny_access,
>      .do_LPI              = gicv3_do_LPI,
>  };
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 6c803bf..7bdb603 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -851,6 +851,17 @@ int gic_make_hwdom_madt(const struct domain *d, u32 offset)
>      return gic_hw_ops->make_hwdom_madt(d, offset);
>  }
>
> +u32 gic_get_hwdom_madt_size(const struct domain *d)

See above for u32.

> +{
> +    u32 madt_size;

Ditto + newline.

> +    madt_size = sizeof(struct acpi_table_madt)
> +                + sizeof(struct acpi_madt_generic_interrupt) * d->max_vcpus
> +                + sizeof(struct acpi_madt_generic_distributor)
> +                + gic_hw_ops->get_hwdom_madt_size(d);
> +
> +    return madt_size;
> +}
> +
>  int gic_iomem_deny_access(const struct domain *d)
>  {
>      return gic_hw_ops->iomem_deny_access(d);
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 6203dc5..a766e42 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -365,6 +365,8 @@ struct gic_hw_operations {
>      int (*make_hwdom_madt)(const struct domain *d, u32 offset);
>      /* Map extra GIC MMIO, irqs and other hw stuffs to the hardware domain. */
>      int (*map_hwdom_extra_mappings)(struct domain *d);
> +    /* Query the size of hardware domain madt table */
> +    u32 (*get_hwdom_madt_size)(const struct domain *d);

See above for u32.

>      /* Deny access to GIC regions */
>      int (*iomem_deny_access)(const struct domain *d);
>      /* Handle LPIs, which require special handling */
> @@ -376,6 +378,7 @@ int gic_make_hwdom_dt_node(const struct domain *d,
>                             const struct dt_device_node *gic,
>                             void *fdt);
>  int gic_make_hwdom_madt(const struct domain *d, u32 offset);
> +u32 gic_get_hwdom_madt_size(const struct domain *d);
>  int gic_map_hwdom_extra_mappings(struct domain *d);
>  int gic_iomem_deny_access(const struct domain *d);
>
> diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h
> index a673fba..b849b16 100644
> --- a/xen/include/asm-arm/gic_v3_its.h
> +++ b/xen/include/asm-arm/gic_v3_its.h
> @@ -138,6 +138,7 @@ 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);
> +u32 gicv3_its_madt_generic_translator_size(void);

Ditto.

>  #endif
>  /* Deny iomem access for its */
>  int gicv3_its_deny_access(const struct domain *d);
> @@ -209,6 +210,11 @@ static inline int gicv3_its_acpi_init(struct acpi_subtable_header *header,
>  {
>      return false;
>  }
> +
> +static inline u32 gicv3_its_madt_generic_translator_size(void)
> +{
> +    return 0;
> +}
>  #endif
>
>  static inline int gicv3_its_deny_access(const struct domain *d)
>

Cheers,

-- 
Julien Grall

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