Re: [PATCH 3/8] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable

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

Hello Shanker,

On 19/06/16 00:45, Shanker Donthineni wrote:
> The redistributor address can be specified either as part of GICC or
> GICR subtable depending on the power domain. The current driver
> doesn't support parsing redistributor entry that is defined in GICC
> subtable. The GIC CPU subtable entry holds the associated Redistributor
> base address if it is not on always-on power domain.
>
> This patch adds necessary code to handle both types of Redistributors
> base addresses.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> ---
>   xen/arch/arm/gic-v3.c             | 97 ++++++++++++++++++++++++++++++++-------
>   xen/include/asm-arm/gic.h         |  2 +
>   xen/include/asm-arm/gic_v3_defs.h |  1 +
>   3 files changed, 83 insertions(+), 17 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index af12ebc..42cf848 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -659,6 +659,10 @@ static int __init gicv3_populate_rdist(void)
>                           smp_processor_id(), i, ptr);
>                   return 0;
>               }
> +
> +            if ( gicv3.rdist_regions[i].single_rdist )
> +                break;
> +
>               if ( gicv3.rdist_stride )
>                   ptr += gicv3.rdist_stride;
>               else
> @@ -1282,6 +1286,11 @@ static int gicv3_iomem_deny_access(const struct domain *d)
>   }
>
>   #ifdef CONFIG_ACPI
> +static bool gic_dist_supports_dvis(void)

static inline and please use bool_t here.

> +{
> +    return !!(readl_relaxed(GICD + GICD_TYPER) & GICD_TYPER_DVIS);
> +}
> +
>   static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>   {
>       struct acpi_subtable_header *header;
> @@ -1393,18 +1402,39 @@ gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header,
>                                     const unsigned long end)
>   {
>       struct acpi_madt_generic_redistributor *rdist;
> +    struct acpi_madt_generic_interrupt *processor;
>       struct rdist_region *region;
>
>       region = gicv3.rdist_regions + gicv3.rdist_count;
> -    rdist = (struct acpi_madt_generic_redistributor *)header;
> -    if ( BAD_MADT_ENTRY(rdist, end) )
> -        return -EINVAL;
> +    if ( header->type == ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR )
> +    {
> +        rdist = (struct acpi_madt_generic_redistributor *)header;
> +        if ( BAD_MADT_ENTRY(rdist, end) )
> +            return -EINVAL;
>
> -    if ( !rdist->base_address || !rdist->length )
> -        return -EINVAL;
> +        if ( !rdist->base_address || !rdist->length )
> +            return -EINVAL;
> +
> +        region->base = rdist->base_address;
> +        region->size = rdist->length;
> +    }
> +    else if ( header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT )
> +    {

Parsing the GICC and the redistributor is quite different. I would much 
prefer a function for parsing each table and an helper to add a new 
redistributor.

> +        processor = (struct acpi_madt_generic_interrupt *)header;
> +        if ( BAD_MADT_ENTRY(processor, end) )
> +            return -EINVAL;
> +
> +        if ( !(processor->flags & ACPI_MADT_ENABLED) )
> +            return 0;
> +
> +        if ( !processor->gicr_base_address )
> +            return -EINVAL;
> +
> +        region->base = processor->gicr_base_address;
> +        region->size = gic_dist_supports_dvis() ? SZ_256K : SZ_128K;

Please explain in the commit message how you find the size. I would also 
prefer if you use (4 x SZ_64K) and (2 * SZ_64K) as we do in populate_rdist.

> +	region->single_rdist = true;

The indentation looks wrong.

> +   }


>
> -    region->base = rdist->base_address;
> -    region->size = rdist->length;
>
>       region->map_base = ioremap_nocache(region->base, region->size);
>       if ( !region->map_base )
> @@ -1412,6 +1442,7 @@ gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header,
>           printk("Unable to map GICR registers\n");
>           return -ENOMEM;
>       }
> +

Spurious change.

>       gicv3.rdist_count++;
>
>       return 0;
> @@ -1421,9 +1452,22 @@ static int __init
>   gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header *header,
>                                       const unsigned long end)
>   {
> -    /* Nothing to do here since it only wants to get the number of GIC
> -     * redistributors.
> -     */
> +    struct acpi_madt_generic_redistributor *rdist;
> +    struct acpi_madt_generic_interrupt *cpuif;
> +
> +    if ( header->type == ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR )
> +    {
> +	 rdist = (struct acpi_madt_generic_redistributor *)header;
> +	 if ( BAD_MADT_ENTRY(rdist, end) || !rdist->base_address )
> +	     return -EINVAL;
> +    }
> +    else if ( header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT )
> +    {
> +	 cpuif = (struct acpi_madt_generic_interrupt *)header;
> +	 if ( BAD_MADT_ENTRY(cpuif, end) || !cpuif->gicr_base_address )
> +	     return -EINVAL;
> +    }
> +

Ditto for the parsing.

>       return 0;
>   }
>

[...]

> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 44b9ef6..7f9ad86 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -101,6 +101,8 @@
>   #define GICD_TYPE_CPUS_SHIFT 5
>   #define GICD_TYPE_CPUS  0x0e0
>   #define GICD_TYPE_SEC   0x400
> +#define GICD_TYPER_LPIS (1U << 17)

This is not used by this patch. So please drop it.

> +#define GICD_TYPER_DVIS (1U << 18)
>
>   #define GICC_CTL_ENABLE 0x1
>   #define GICC_CTL_EOI    (0x1 << 9)
> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index 6d98491..6bd25a5 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -141,6 +141,7 @@ struct rdist_region {
>       paddr_t base;
>       paddr_t size;
>       void __iomem *map_base;
> +    bool single_rdist;
>   };
>
>   #endif /* __ASM_ARM_GIC_V3_DEFS_H__ */
>

Regards,

-- 
Julien Grall

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