Messages in this thread | | | Date | Thu, 26 Oct 2023 19:54:18 +0800 | From | Fang Xiang <> | Subject | Re: [PATCH] irqchip/gic-v3-its: Fix the coherent issue in its_setup_baser() when shr = 0. |
| |
On Thu, Oct 26, 2023 at 10:48:05AM +0100, Marc Zyngier wrote: > On Thu, 26 Oct 2023 09:48:13 +0100, > Fang Xiang <fangxiang3@xiaomi.com> wrote: > > > > On Thu, Oct 26, 2023 at 09:01:17AM +0100, Marc Zyngier wrote: > > > On Thu, 26 Oct 2023 03:01:16 +0100, > > > Fang Xiang <fangxiang3@xiaomi.com> wrote: > > > > > > > > The table would not be flushed if the input parameter shr = 0 in > > > > its_setup_baser() and it would cause a coherent problem. > > > > > > Would? Or does? I'm asking, as you have previously indicated that this > > > workaround was working for you. > > > > > > Have you actually observed a problem? Or is that by inspection? > > > > > I actually observed this problem on my device. GIC get a dirty table > > because CPU did not flush the clean one to memory. > > So how comes you previously reported that it was working for you? >
It is a complicated situation. Just the Virtual CPUs table is dirty on my device, so the physical LPI works well.
> > > > > > > > Signed-off-by: Fang Xiang <fangxiang3@xiaomi.com> > > > > --- > > > > drivers/irqchip/irq-gic-v3-its.c | 8 +++++--- > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > > > > index 75a2dd550625..58a9f24ccfa7 100644 > > > > --- a/drivers/irqchip/irq-gic-v3-its.c > > > > +++ b/drivers/irqchip/irq-gic-v3-its.c > > > > @@ -2394,13 +2394,15 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, > > > > * non-cacheable as well. > > > > */ > > > > shr = tmp & GITS_BASER_SHAREABILITY_MASK; > > > > - if (!shr) { > > > > + if (!shr) > > > > cache = GITS_BASER_nC; > > > > - gic_flush_dcache_to_poc(base, PAGE_ORDER_TO_SIZE(order)); > > > > - } > > > > + > > > > goto retry_baser; > > > > } > > > > > > > > + if (!shr) > > > > + gic_flush_dcache_to_poc(base, PAGE_ORDER_TO_SIZE(order)); > > > > + > > > > > > This is wrong. You're doing the cache clean *after* the register has > > > been programmed with its final value, and the ITS is perfectly allowed > > > to prefetch anything it wants as soon as you program the register. The > > > clean must thus happen before the write. Yes, it was wrong before, but > > > you are now making it wrong always. > > Sorry for that. But on my device, GIC would not read the table before > > ITS enable(GITS_CTLR.Enabled == 1). When ITS is disabled, the prefetch > > happens ever in other platforms? > > GITS_CTLR.Enabled == 1 controls the *translation* (i.e. whether a > write to GITS_TRANSLATER gets processed or not). It doesn't say > anything of the tables that are pointed to by the ITS. > > If you care to read the spec, you will find this (Arm IHI 0069H, page > 5-95, "Software access to the private ITS tables"): > > <quote> > * For a table that is pointed to by a GITS_BASER<n> register for which > GITS_BASER<n>.Valid == 1 and GITS_BASER<n>.Indirect == 0, behavior is > UNPREDICTABLE if the table is written by software. > </quote> > > and a cache clean definitely counts as a write from the PoV of the > ITS. What your device does is pretty much irrelevant, as the > architecture allows any sort of access as soon as the Valid bit is > set. >
Yes, I see it. The tables should be flushed before we write the GITS_BASER[n] registers.
> > > > > > > if (val != tmp) { > > > > pr_err("ITS@%pa: %s doesn't stick: %llx %llx\n", > > > > &its->phys_base, its_base_type_string[type], > > > > > > Overall, I think we need a slightly better fix. Since non-coherent > > > ITSs are quickly becoming the common case, we can save ourselves some > > > effort and hoist the quirked attributes early. > > > > > > Does the hack below work for you? > > > > > > M. > > > > > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > > > index 75a2dd550625..d76d44ea2de1 100644 > > > --- a/drivers/irqchip/irq-gic-v3-its.c > > > +++ b/drivers/irqchip/irq-gic-v3-its.c > > > @@ -2379,12 +2379,12 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, > > > break; > > > } > > > > > > + if (!shr) > > > + gic_flush_dcache_to_poc(base, PAGE_ORDER_TO_SIZE(order)); > > > + > > > its_write_baser(its, baser, val); > > > tmp = baser->val; > > > > > > - if (its->flags & ITS_FLAGS_FORCE_NON_SHAREABLE) > > > - tmp &= ~GITS_BASER_SHAREABILITY_MASK; > > > - > > > if ((val ^ tmp) & GITS_BASER_SHAREABILITY_MASK) { > > > /* > > > * Shareability didn't stick. Just use > > > @@ -2394,10 +2394,9 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, > > > * non-cacheable as well. > > > */ > > > shr = tmp & GITS_BASER_SHAREABILITY_MASK; > > > - if (!shr) { > > > + if (!shr) > > > cache = GITS_BASER_nC; > > > - gic_flush_dcache_to_poc(base, PAGE_ORDER_TO_SIZE(order)); > > > - } > > > + > > > goto retry_baser; > > > } > > > > > > @@ -2609,6 +2608,11 @@ static int its_alloc_tables(struct its_node *its) > > > /* erratum 24313: ignore memory access type */ > > > cache = GITS_BASER_nCnB; > > > > > > + if (its->flags & ITS_FLAGS_FORCE_NON_SHAREABLE) { > > > + cache = GITS_BASER_nC; > > > + shr = 0; > > > + } > > > + > > > for (i = 0; i < GITS_BASER_NR_REGS; i++) { > > > struct its_baser *baser = its->tables + i; > > > u64 val = its_read_baser(its, baser); > > > > > There maybe a risk in this patch above when non-shareable attibute indicated > > by hardware, the table would not be flushed ever. > > How? If the HW rejects the shareability attribute, we set shr to 0 and > hit the retry path. At this point, we will clean the page to the PoC > before writing the register again. > > What am I missing? >
Sorry, It is a good fix. I will test it on my device and give a feedback soon.
> M. > > -- > Without deviation from the norm, progress is not possible.
| |