Re: [PATCH] irqchip/gic-v3-its: Fix the coherent issue in its_setup_baser() when shr = 0.

Fang Xiang
2023-10-26T11:54:40Z

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.