Messages in this thread Patch in this message | | | Subject | Re: [PATCH v0] irqchip/gic-v3: Avoid check of lpi configuration for non existent cpu | From | Gaurav Kohli <> | Date | Thu, 5 Dec 2019 13:01:34 +0000 |
| |
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@codeaurora.org> >> >> 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.
| |