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

[Shanker Donthineni <shankerd@codeaurora.org>]

On 06/27/2016 06:47 AM, Julien Grall wrote:
> 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.
>
Other function just finds the number of valid cpu interfaces. I would  
prefer to keep the validation check here.

>> +
>> +    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
>

I'll do.

> 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.
>

Disabled GICC entries should be skipped because its Redistributor region 
is not always-on power domain. Please look at my review comment to your 
KVM-ACPI patch http://www.gossamer-threads.com/lists/linux/kernel/2413670.

> 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,
>

-- 
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