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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel