From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 350FAC25B48 for ; Thu, 26 Oct 2023 09:48:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=fyVib7RFb9MAo2obutFUtp6yt43zvZyd3WResMpLBV8=; b=nEqTm1beQhoUWJ ASGCw7MY28KfhXGZxrrw3uesmjn12CDTQk2P0J/ns8iTGxpDVWnfV6VTmMIqLqJk/ajSIMqKJyS/P Hq9W1v7zNNqPwiOcJ2TPm4MsXVZygi6mvkYxVeheab2HbHt0yhDb6p/PiMJrk+450tdXeKfxcKd4N glddk/rHMctoYHgVQW3O8/hw2sQbnzrN0Odqugcq+aE7mBo9d4OnTHS7NjGeFvM0j8tkVQPjq9YPg Z42GYNcEJMni+Lbj4aJLBl4mD9Ei465x/hZ+AF/orKjye9gOd6Tf69SKYY3yQx/1LbpIJ0EfFk/c1 SAwh6cAT8CUzYZCfHWkA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qvwyN-00E6cf-2l; Thu, 26 Oct 2023 09:48:15 +0000 Received: from ams.source.kernel.org ([145.40.68.75]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qvwyL-00E6bX-0C for linux-arm-kernel@lists.infradead.org; Thu, 26 Oct 2023 09:48:15 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by ams.source.kernel.org (Postfix) with ESMTP id C6172B834C7; Thu, 26 Oct 2023 09:48:09 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 28491C433C7; Thu, 26 Oct 2023 09:48:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1698313689; bh=m4K6I24gIzB9BEoM41jPYXXjvqkA7hNcqLdkKLZniac=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=Qh1MXEyMRsZLlL1JUNqEdadQCjdW8famLwX+f0ofG+gkNFPp+UA71u57siF3UTuDQ NJ67HK70KU1xhb30RaOmdFULUZ9mvhrMn6Ig6b7jROVFvrmO1cgVpq16kF9X7KBOVz R0pS8fbJdWzlW97rdhe7YxutiK2GMKgAF2cCXH9XGXRyvHayVdw1rou+AnZrPFeF3p h0nTsoMxUAyXpjLfxUhsJkiNUuG25Xf7gB+Ai4a6pj2I+iVd9Ma6i7BenSzPc5nvp8 tRhJJugR+HBJv19ociccQ/o+YN/K+5g21PxsunazoiKmdJa1UJCfgHlp7wyc7EFc5w RGZ6tjisAXosw== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1qvwyD-007qfE-Ex; Thu, 26 Oct 2023 10:48:06 +0100 Date: Thu, 26 Oct 2023 10:48:05 +0100 Message-ID: <86bkcl4sve.wl-maz@kernel.org> From: Marc Zyngier To: Fang Xiang Cc: , , Subject: Re: [PATCH] irqchip/gic-v3-its: Fix the coherent issue in its_setup_baser() when shr = 0. In-Reply-To: References: <20231026020116.4238-1-fangxiang3@xiaomi.com> <87sf5x6cdu.wl-maz@kernel.org> 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") X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: fangxiang3@xiaomi.com, tglx@linutronix.de, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231026_024813_412304_0F63B278 X-CRM114-Status: GOOD ( 42.64 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, 26 Oct 2023 09:48:13 +0100, Fang Xiang 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 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 > > > --- > > > 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"): * For a table that is pointed to by a GITS_BASER register for which GITS_BASER.Valid == 1 and GITS_BASER.Indirect == 0, behavior is UNPREDICTABLE if the table is written by software. 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel