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

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

On 27/06/16 15:17, Shanker Donthineni wrote:
>>>   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.

The function acpi_parse_entries (& co) does not do what you think. The 
function will return an error as soon as one call to the handler (here 
gic_acpi_get_madt_cpu_num) return a non-zero value (see the 
implementation of acpi_parse_entries_array).

So if the CPU interface is not valid (i.e gicr_base_address it a 
non-zero value), then it will return an error. Therefore this check is 
pointless.

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

I am not sure to follow here. A usable CPU may have his Redistributor in 
the not always-on power domain. So the issue would be the same, correct?

> Please look at my review comment to your
> KVM-ACPI patch http://www.gossamer-threads.com/lists/linux/kernel/2413670.

Well in this case the check needs to be done in the other function too 
(gic_acpi_parse_madt_cpu).

Regards,

-- 
Julien Grall

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