Re: [PATCH V2 04/10] arm/gic-v3: Parse per-cpu redistributor entry in GICC subtable

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

Hi Shanker,

On 26/06/16 18:48, 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.
>
> The per CPU Redistributor size is not defined in ACPI specification.
> Set it's size to SZ_256K if the GIC hardware is capable of Direct

s/it's/its/, although I would use "the".

> Virtual LPI Injection feature otherwise SZ_128K.

"Set the size to SZ_256K if the GIC hardware is supporting Direct 
Virtual LPI injection, SZ_128K otherwise".

>
> This patch adds necessary code to handle both types of Redistributors
> base addresses.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> ---
> Changes since v1:
>    Edited commit text and fixed white spaces.
>    Added a new function for parsing per CPU Redistributor entry.
>
>   xen/arch/arm/gic-v3.c             | 84 ++++++++++++++++++++++++++++++++++-----
>   xen/include/asm-arm/gic.h         |  1 +
>   xen/include/asm-arm/gic_v3_defs.h |  1 +
>   3 files changed, 77 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 0471fea..3977244 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,14 +1286,21 @@ static int gicv3_iomem_deny_access(const struct domain *d)
>   }
>
>   #ifdef CONFIG_ACPI
> -static void __init gic_acpi_add_rdist_region(u64 base_addr, u32 size)
> +static void __init
> +gic_acpi_add_rdist_region(u64 base_addr, u32 size, bool single_rdist)
>   {
>       unsigned int idx = gicv3.rdist_count++;
>
> +    gicv3.rdist_regions[idx].single_rdist = single_rdist;
>       gicv3.rdist_regions[idx].base = base_addr;
>       gicv3.rdist_regions[idx].size = size;
>   }
>
> +static inline bool gic_dist_supports_dvis(void)
> +{
> +    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;
> @@ -1397,6 +1408,42 @@ gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header,
>   }
>
>   static int __init
> +gic_acpi_parse_cpu_redistributor(struct acpi_subtable_header *header,
> +                                  const unsigned long end)
> +{
> +    struct acpi_madt_generic_interrupt *processor;
> +    u32 size;
> +
> +    processor = (struct acpi_madt_generic_interrupt *)header;
> +    if ( BAD_MADT_ENTRY(processor, end) )
> +        return -EINVAL;
> +
> +    if ( !processor->gicr_base_address )
> +        return -EINVAL;

You already check it in gic_acpi_get_madt_cpu_num, so there is no reason 
to do it again.

> +
> +    if ( processor->flags & ACPI_MADT_ENABLED )
> +    {
> +        size = gic_dist_supports_dvis() ? 4 * SZ_64K : 2 * SZ_64K;
> +        gic_acpi_add_rdist_region(processor->gicr_base_address, size, true);
> +    }

I would revert the condition to avoid one level of indentation. I.e

if ( !(processor->flags & ACPI_MADT_ENABLED) )
   return 0;

size = ....
gic_acpi_add...

return 0;

However, it looks like that the other function that parses GICC within 
gic-v3.c (see gic_acpi_parse_madt_cpu) does not check if the CPU is usable.

I think we need to have the same parsing behavior on every function.

> +
> +    return 0;
> +}
> +
> +static int __init
> +gic_acpi_get_madt_cpu_num(struct acpi_subtable_header *header,
> +                                    const unsigned long end)
> +{
> +    struct acpi_madt_generic_interrupt *cpuif;
> +
> +    cpuif = (struct acpi_madt_generic_interrupt *)header;
> +    if ( BAD_MADT_ENTRY(cpuif, end) || !cpuif->gicr_base_address )
> +        return -EINVAL;
> +
> +    return 0;
> +}
> +
> +static int __init
>   gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header,
>                                     const unsigned long end)
>   {
> @@ -1409,7 +1456,7 @@ gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header,
>       if ( !rdist->base_address || !rdist->length )
>           return -EINVAL;
>
> -    gic_acpi_add_rdist_region(rdist->base_address, rdist->length);
> +    gic_acpi_add_rdist_region(rdist->base_address, rdist->length, false);
>
>       return 0;
>   }
> @@ -1428,6 +1475,7 @@ static void __init gicv3_acpi_init(void)
>   {
>       struct acpi_table_header *table;
>       struct rdist_region *rdist_regs;
> +    bool gicr_table = true;
>       acpi_status status;
>       int count;
>
> @@ -1457,8 +1505,18 @@ static void __init gicv3_acpi_init(void)
>       count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
>                                  gic_acpi_get_madt_redistributor_num, table,
>                                  ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, 0);
> -    if ( count <= 0 )
> -        panic("GICv3: No valid GICR entries exists");
> +
> +    /* Count the total number of CPU interface entries */
> +    if (count <= 0) {

Coding style:

if ( count <= 0 )
{

> +        count = acpi_parse_entries(ACPI_SIG_MADT,
> +                                   sizeof(struct acpi_table_madt),
> +                                   gic_acpi_get_madt_cpu_num,
> +                                   table, ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0);

Please use acpi_table_parse_madt.

> +        if (count <= 0)

Coding style:

if ( count <= 0 )

> +            panic("GICv3: No valid GICR entries exists");
> +
> +        gicr_table = false;
> +    }
>
>       if ( count > MAX_RDIST_COUNT )
>           panic("GICv3: Number of redistributor regions is more than"
> @@ -1470,11 +1528,19 @@ static void __init gicv3_acpi_init(void)
>
>       gicv3.rdist_regions = rdist_regs;
>
> -    /* Parse always-on power domain Re-distributor entries */
> -    count = acpi_parse_entries(ACPI_SIG_MADT,
> -                               sizeof(struct acpi_table_madt),
> -                               gic_acpi_parse_madt_redistributor, table,
> -                               ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count);
> +    if ( gicr_table )
> +        /* Parse always-on power domain Re-distributor entries */
> +        count = acpi_parse_entries(ACPI_SIG_MADT,
> +                                   sizeof(struct acpi_table_madt),
> +                                   gic_acpi_parse_madt_redistributor, table,
> +                                   ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, count);

Please use acpi_table_parse_madt.

> +    else
> +        /* Parse Re-distributor entries described in CPU interface table */
> +        count = acpi_parse_entries(ACPI_SIG_MADT,
> +                                   sizeof(struct acpi_table_madt),
> +                                   gic_acpi_parse_cpu_redistributor, table,
> +                                   ACPI_MADT_TYPE_GENERIC_INTERRUPT, count);

Ditto.

> +
>       if ( count <= 0 )
>           panic("GICv3: Can't get Redistributor entry");
>
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 44b9ef6..fedf1fa 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -101,6 +101,7 @@
>   #define GICD_TYPE_CPUS_SHIFT 5
>   #define GICD_TYPE_CPUS  0x0e0
>   #define GICD_TYPE_SEC   0x400
> +#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