Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Thu, 26 Oct 2023 10:48:05 +0100
Message-ID: <86bkcl4sve.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Fang Xiang <fangxiang3@xiaomi.com>
Cc:     <tglx@linutronix.de>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH] irqchip/gic-v3-its: Fix the coherent issue in its_setup_baser() when shr = 0.
In-Reply-To: <ZTonzQvHedqa7Uj9@oa-fangxiang3.localdomain>
References: <20231026020116.4238-1-fangxiang3@xiaomi.com>
        <87sf5x6cdu.wl-maz@kernel.org>
        <ZTonzQvHedqa7Uj9@oa-fangxiang3.localdomain>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.1
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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?

> > > 
> > > 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.

> > 
> > >  	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?

	M.

-- 
Without deviation from the norm, progress is not possible.