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

[Fang Xiang]

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