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 09:01:17 +0100
Message-ID: <87sf5x6cdu.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: <20231026020116.4238-1-fangxiang3@xiaomi.com>
References: <20231026020116.4238-1-fangxiang3@xiaomi.com>
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/28.2
 (x86_64-pc-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 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?

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

>  	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);

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