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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 215CCC25B6B for ; Thu, 26 Oct 2023 08:01:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344448AbjJZIB0 (ORCPT ); Thu, 26 Oct 2023 04:01:26 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44040 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229678AbjJZIBY (ORCPT ); Thu, 26 Oct 2023 04:01:24 -0400 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 29B6ACE for ; Thu, 26 Oct 2023 01:01:22 -0700 (PDT) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A866CC433C8; Thu, 26 Oct 2023 08:01:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1698307281; bh=JiNlhVZAz3Pcv0XnmPPg/33soHhY7u9+dEZ3w/gD2/A=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=ZX9CAr/MjdIgT4sjvZWhZnyByRxoMzC4wG1gffReM9OpPQXhxSNFJTUYVqYTX3KAi MgL4ERQQ8yWPkLV0uAUjsjAQ8SyQc92dIp0GTJ5LZTnC+YgiKzzck7/YIxVYHGujCC aHRup2PDH358KAi750RzOEUDe2ctaDrSP9aNxXhNDfWpBINwwtyqcE89bIQ9L4nGSn ACguEsox9Ihx8sPb26GE2W7P9AGGmi04F5TWLHAlFeEvOS2sWjtUoSymddKU7fGeG1 tMbfBiy03GoIij0Q+RLVTd/fFd3oPBdx5aKoL1i5VFXX/B99D96iH4FuaU6Gd6QmgL brCbYiZcvrh6w== Received: from ip-185-104-136-29.ptr.icomera.net ([185.104.136.29] helo=wait-a-minute.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 1qvvIt-007olZ-4e; Thu, 26 Oct 2023 09:01:19 +0100 Date: Thu, 26 Oct 2023 09:01:17 +0100 Message-ID: <87sf5x6cdu.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: <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 X-SA-Exim-Connect-IP: 185.104.136.29 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 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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? > > 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. > 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.