Re: [PATCH v0] irqchip/gic-v3: Avoid check of lpi configuration for non existent cpu

[Gaurav Kohli]

On 12/5/2019 6:17 PM, Marc Zyngier wrote:
> Hi Gaurav,
>
> On 2019-12-05 10:55, Gaurav Kohli wrote:
> > As per GIC specification, we can configure gic for more no of cpus
> > then the available cpus in the soc, But this can cause mem abort
> > while iterating lpi region for non existent cpu as we don't map
>
> Which LPI region? We're talking about RDs, right... Or does LPI mean
> something other than GIC LPIs for you?

Yes RDs only.
> > redistrubutor region for non-existent cpu.
> >
> > To avoid this issue, put one more check of valid mpidr.
>
> Sorry, but I'm not sure I grasp your problem. Let me try and rephrase it:
>
> - Your GIC is configured for (let's say) 8 CPUs, and your SoC has only 4.
Yes, suppose gic is configured for 8 cpus but soc has only 4 cpus. Then 
in this case gic_iterate will iterate till it get TYPER_LAST.

But as gic is configured for 8, So last bit sets in eight redistributor 
regions only.
> - As part of the probing, the driver iterates on the RD regions and 
> explodes
>  Â because something isn't mapped?
>
> That'd be a grave bug, but I believe the issue is somewhere else.

There are 4 cpus present, that's why we have mapped 4 redistributor 
only, but during probe below function keeps iterating and give mem abort 
for 5th cpu.

static void gic_update_vlpi_properties(void)
{
        gic_iterate_rdists(__gic_update_vlpi_properties);

}

We can solve this problem by mapping all eight redistributor in dt, but 
ideally code should also able to handle this and we can avoid mappin?
> > Signed-off-by: Gaurav Kohli <gkohli@xxxxxxxxxxxxxx>
> >
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 1edc993..adc9186 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -766,6 +766,7 @@ static int gic_iterate_rdists(int (*fn)(struct
> > redist_region *, void __iomem *))
> > Â{
> > ÂÂÂÂ int ret = -ENODEV;
> > ÂÂÂÂ int i;
> > +ÂÂÂ int cpu = 0;
> >
> > ÂÂÂÂ for (i = 0; i < gic_data.nr_redist_regions; i++) {
> > ÂÂÂÂÂÂÂÂ void __iomem *ptr = gic_data.redist_regions[i].redist_base;
> > @@ -780,6 +781,7 @@ static int gic_iterate_rdists(int (*fn)(struct
> > redist_region *, void __iomem *))
> > ÂÂÂÂÂÂÂÂ }
> >
> > ÂÂÂÂÂÂÂÂ do {
> > +ÂÂÂÂÂÂÂÂÂÂÂ cpu++;
> > ÂÂÂÂÂÂÂÂÂÂÂÂ typer = gic_read_typer(ptr + GICR_TYPER);
> > ÂÂÂÂÂÂÂÂÂÂÂÂ ret = fn(gic_data.redist_regions + i, ptr);
> > ÂÂÂÂÂÂÂÂÂÂÂÂ if (!ret)
> > @@ -795,7 +797,8 @@ static int gic_iterate_rdists(int (*fn)(struct
> > redist_region *, void __iomem *))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (typer & GICR_TYPER_VLPIS)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ptr += SZ_64K * 2; /* Skip VLPI_base + reserved 
> > page */
> > ÂÂÂÂÂÂÂÂÂÂÂÂ }
> > -ÂÂÂÂÂÂÂ } while (!(typer & GICR_TYPER_LAST));
> > +ÂÂÂÂÂÂÂ } while (!(typer & GICR_TYPER_LAST) &&
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ cpu_logical_map(cpu) != INVALID_HWID);
> > ÂÂÂÂ }
> >
> > ÂÂÂÂ return ret ? -ENODEV : 0;
>
> This makes little sense. A redistributor region contains a bunch of RDs,
> each of which maps onto a given CPU. We iterate on the RDs, and not on the
> CPUs, as it is the RD that tells us which CPU it is affine with, not the
> other way around.
>
> If a RD is for some reason unavailable, then it shouldn't be described in
> the firmware the first place. If you end-up exposing RD regions that do
> not have the last RD having GICR_TYPER.Last set, then your SoC is broken,
> and this needs yet another quirk.
>
>  ÂÂÂÂÂÂÂ M.

--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.