Re: [PATCH V2 03/10] arm/gic-v3: Fold GICR subtable parsing into a new function

[Shanker Donthineni <shankerd@codeaurora.org>]

On 06/27/2016 06:26 AM, Julien Grall wrote:
> Hi Shanker,
>
> Title: I think you want to say "Move GICR..." rather than "Fold GICR...".
>
> On 26/06/16 18:48, Shanker Donthineni wrote:
>> Add a new function for parsing GICR subtable and move the code
>
> Add a new function to parse GICR...
>
>> that is specific to GICR table to new function without changing
>
> to a new function
>
>> the function gicv3_acpi_init() behavior.
>>
>> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
>> ---
>> Changes since v1:
>>    Removed the unnecessary GICR ioremap operation inside GICR table
> parse code.
>>
>>   xen/arch/arm/gic-v3.c | 61
> ++++++++++++++++++++++++++++++++-------------------
>>   1 file changed, 39 insertions(+), 22 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 542c4f3..0471fea 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -1282,6 +1282,14 @@ 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)
>
> Please use paddr_t for both parameter. Also the suffix _addr is 
> pointless.
>
>> +{
>> +    unsigned int idx = gicv3.rdist_count++;
>> +
>> +    gicv3.rdist_regions[idx].base = base_addr;
>> +    gicv3.rdist_regions[idx].size = size;
>> +}
>> +
>>   static int gicv3_make_hwdom_madt(const struct domain *d, u32 offset)
>>   {
>>       struct acpi_subtable_header *header;
>> @@ -1387,6 +1395,25 @@ gic_acpi_parse_madt_distributor(struct
> acpi_subtable_header *header,
>>
>>       return 0;
>>   }
>> +
>> +static int __init
>> +gic_acpi_parse_madt_redistributor(struct acpi_subtable_header *header,
>> +                                  const unsigned long end)
>> +{
>> +    struct acpi_madt_generic_redistributor *rdist;
>> +
>> +    rdist = (struct acpi_madt_generic_redistributor *)header;
>> +    if ( BAD_MADT_ENTRY(rdist, end) )
>> +        return -EINVAL;
>> +
>> +    if ( !rdist->base_address || !rdist->length )
>> +        return -EINVAL;
>
> In the commit message you said that the behavior is unchanged, however 
> this check is not part of the previous code.
>
> Anyway, I don't think this check is necessary.
>
>> +
>> +    gic_acpi_add_rdist_region(rdist->base_address, rdist->length);
>> +
>> +    return 0;
>> +}
>> +
>>   static int __init
>>   gic_acpi_get_madt_redistributor_num(struct acpi_subtable_header
> *header,
>> const unsigned long end)
>> @@ -1402,7 +1429,7 @@ static void __init gicv3_acpi_init(void)
>>       struct acpi_table_header *table;
>>       struct rdist_region *rdist_regs;
>>       acpi_status status;
>> -    int count, i;
>> +    int count;
>>
>>       status = acpi_get_table(ACPI_SIG_MADT, 0, &table);
>>
>> @@ -1433,37 +1460,27 @@ static void __init gicv3_acpi_init(void)
>>       if ( count <= 0 )
>>           panic("GICv3: No valid GICR entries exists");
>>
>> -    gicv3.rdist_count = count;
>> -
>> -    if ( gicv3.rdist_count > MAX_RDIST_COUNT )
>> +    if ( count > MAX_RDIST_COUNT )
>>           panic("GICv3: Number of redistributor regions is more than"
>>                 "%d (Increase MAX_RDIST_COUNT!!)\n", MAX_RDIST_COUNT);
>>
>> -    rdist_regs = xzalloc_array(struct rdist_region, gicv3.rdist_count);
>> +    rdist_regs = xzalloc_array(struct rdist_region, count);
>>       if ( !rdist_regs )
>>           panic("GICv3: Failed to allocate memory for rdist regions\n");
>>
>> -    for ( i = 0; i < gicv3.rdist_count; i++ )
>> -    {
>> -        struct acpi_subtable_header *header;
>> -        struct acpi_madt_generic_redistributor *gic_rdist;
>> -
>> -        header =
> acpi_table_get_entry_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
>> - i);
>> -        if ( !header )
>> -            panic("GICv3: Can't get GICR entry");
>> -
>> -        gic_rdist =
>> -           container_of(header, struct acpi_madt_generic_redistributor,
> header);
>> -        rdist_regs[i].base = gic_rdist->base_address;
>> -        rdist_regs[i].size = gic_rdist->length;
>> -    }
>> +    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);
>
> Please use acpi_table_parse_madt here.
>

How do we pass MADT table pointer that we are using to a function 
acpi_table_parse_madt()?  We have already obtained  the MADT table 
address by calling  acpi_get_table() at the beginning of the function 
gicv3_acpi_init().


>> +    if ( count <= 0 )
>> +        panic("GICv3: Can't get Redistributor entry");
>>
>>       /* The vGIC code requires the region to be sorted */
>>       sort(rdist_regs, gicv3.rdist_count, sizeof(*rdist_regs),
> cmp_rdist, NULL);
>>
>> -    gicv3.rdist_regions= rdist_regs;
>> -
>>       /* Collect CPU base addresses */
>>       count = acpi_parse_entries(ACPI_SIG_MADT, sizeof(struct
> acpi_table_madt),
>> gic_acpi_parse_madt_cpu, table,
>>
>
> 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