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 B3A4EC54EE9 for ; Wed, 7 Sep 2022 16:27:03 +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:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=tsEfn/eoFb5A7Y2Her8vxZhUPyEJkG0fABvLQQybzWI=; b=xvpNk1ckSKbapf v7JTIxUq5W6bz8f6+GxF7zBr89TNQiMf0EdL9J2V6gIlRc91SYj71tQ5QK8odPE6O3Hp7M8vq7kA2 WrCGPLO6YHqPxugkA72V6XEYidETzHHbAwtdmv+YA5Rup/gKCIDhRY3cXNeD5GWWjNbRFNgZOI4Tb ZlR4VhazcW3pzHzZrC7kpzvreFwF2mrB68XJxfdZil2pz/DLly+Osrze/rUZBOyR/DE0vbAUSDKZX oVbINja3ORX43eO0HIeCXQtnY2d8bZCH76UAS3r5i8Rn0L34tqbfVl9zK05pfl9s0aQcdPU7CnQZC ZeY/RmLgW4kUDjpSs7lA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oVxsC-007koU-Ib; Wed, 07 Sep 2022 16:25:56 +0000 Received: from ams.source.kernel.org ([145.40.68.75]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oVxs9-007kmO-DC for linux-arm-kernel@lists.infradead.org; Wed, 07 Sep 2022 16:25:55 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 1DBAAB81E12; Wed, 7 Sep 2022 16:25:50 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8FC28C433C1; Wed, 7 Sep 2022 16:25:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1662567948; bh=TpYqXYWixTxxZJq8I2kRXvIOEJn0xgzryZut9E80lDA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gCOXw/s25qKpLaIG5JdD6Vu+4KY+PcfDETXfAU7q/0EJURJxyJJ9hUUUmwnoMEibp CyQf1yQgWAN5Em46Armrr4RTorRJUutvJep8vxFsHTjzXE2exgF2TCQjVT2HaVe9HD baAwdsMzqHToaN1s0IIEsomps7we3ZDzE4jUUztoncoyRqycHqsRamhOXFEMQCtows N9MOZcYfb2CUmdBroCE676CtPWMBs+UP+FBN947/uVPjQBivzoBmnzixn3omA2buuU vIr8WiwWp7DSHczVxZzbXLa4YJTE2i2TthdQJFlfFmxQeble5JI7et7vH3U0W8g0l0 h1KYIQw1s+1Hw== Date: Wed, 7 Sep 2022 17:25:43 +0100 From: Will Deacon To: Robin Murphy Cc: Christoph Hellwig , linux-arm-kernel@lists.infradead.org, Catalin Marinas , Mark Rutland , Ard Biesheuvel Subject: Re: [PATCH] arm64: dma: Drop cache invalidation from arch_dma_prep_coherent() Message-ID: <20220907162543.GA30558@willie-the-truck> References: <20220823122111.17439-1-will@kernel.org> <20220907090305.GA30704@lst.de> <5d856574-4cd7-70d0-adcb-3e284fef315f@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <5d856574-4cd7-70d0-adcb-3e284fef315f@arm.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220907_092553_781304_9770C453 X-CRM114-Status: GOOD ( 38.32 ) 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 Wed, Sep 07, 2022 at 10:27:45AM +0100, Robin Murphy wrote: > On 2022-09-07 10:03, Christoph Hellwig wrote: > > On Tue, Aug 23, 2022 at 01:21:11PM +0100, Will Deacon wrote: > > > arch_dma_prep_coherent() is called when preparing a non-cacheable region > > > for a consistent DMA buffer allocation. Since the buffer pages may > > > previously have been written via a cacheable mapping and consequently > > > allocated as dirty cachelines, the purpose of this function is to remove > > > these dirty lines from the cache, writing them back so that the > > > non-coherent device is able to see them. > > > > Yes. > > > > > I'm slightly wary about this change as other architectures seem to do > > > clean+invalidate here, but I'd like to hear what others think in any > > > case. > > > > If arm64 is fine with having clean but present cachelines when creating > > an uncached mapping for a cache line, the invalidate is not required. > > > > But isn't it better for the cache if these by definition useless > > cachelines get evicted? > > > > My biggest concern here is that we're now moving from consolidating > > these semantics in all the different architectures to different ones, > > making a centralization of the policies even harder. > > FWIW I agree with Ard in not being entirely confident with this change. The > impression I had (which may be wrong) was that the architecture never > actually ruled out unexpected cache hits in the case of mismatched > attributes, it just quietly stopped mentioning it at all. And even if the > architecture did rule them out, how confident are we about errata that might > still allow them to happen? The architecture does rule this out as long as the cacheable alias is not dirty (i.e. has not been written to). If this was not the case, then our non-cacheable DMA buffers would be unreliable because speculative fills from the linear map could "mask" the actual DMA data. The presence of the linear map also means that the lines could immediately be refetched by speculation, rendering the invalidation pointless in the current code. Regardless of this patch, a CPU erratum violating the architectural guarantee would be broken. It is the presence of the linear alias that would need to be resolved. The architecture text is buried in the infamous "B2.8 Mismatched memory attributes" section of the Arm ARM, but it's also worth talking to Mark R, Catalin and the handful of folks in ATG who grok this to get a summary which is easier to digest. > It seems like we don't stand to gain much by removing the invalidation - > since the overhead will still be in the clean - other than the potential for > a slightly increased chance of rare and hard-to-debug memory corruption :/ I just find it odd that we rely on the CPU not hitting the cacheable alias in other places, yet we're using an invalidation for this path. It's inconsistent and difficult to explain to people. As I said, I'm happy to add a comment to the existing code instead of the change here, but I don't know what to say other than something like: /* * The architecture says we only need a clean here, but invalidate as * well just in case. */ Which feels deeply unsatisfactory. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel