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

[Shanker Donthineni <shankerd@codeaurora.org>]

On 06/21/2016 05:16 AM, Julien Grall wrote:
> 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.
>
Still learning XEN coding style, I'll fix it.
>> +{
>> +    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.
>
  I'll do.
>> +        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.
>
I'll 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,
>

-- 
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project


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