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 3B3B8C38145 for ; Thu, 8 Sep 2022 10:33:40 +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=exVHMIhhC4rZATf7hp+UP21PTMJ4WZ/kXml82sgVy4w=; b=NtdnSMY1JU4u83 AFUQTsMu30NneNKGjFMwgH5SJ//1CzhcZ1UGfX1SDbwfGl31E90cN8ZM94f8wGrHmXQEQMvMWYpfH Q7wWDcEqhpyrJbC2ho1bTFz6C+oke9Oa6jDzWWOLzuRlxl8dR75dV1yk5J1zhb1ImYq4Qqso+6uC1 sYpv8sfxxFZU97V1i3DKOsaeawoZMvE73fByjyWjS0smWufhJMt1lrBVRt/Mr+Rux2GMdrr9XnZXu fEhiiuBV/Jdv8ETUYRE5kxmPUgZ2kU4Ey26dxYbdeGrlDt8GKO9gQdfeV1yBT0X4qYikXnYvNrSHI 3ukNgeAvQ3W1skHYcTyw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oWEpi-002LUL-G4; Thu, 08 Sep 2022 10:32:30 +0000 Received: from sin.source.kernel.org ([2604:1380:40e1:4800::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oWEpf-002LT1-0j for linux-arm-kernel@lists.infradead.org; Thu, 08 Sep 2022 10:32:29 +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 sin.source.kernel.org (Postfix) with ESMTPS id 36D77CE1EEA; Thu, 8 Sep 2022 10:32:22 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 067ABC433D6; Thu, 8 Sep 2022 10:32:18 +0000 (UTC) Date: Thu, 8 Sep 2022 11:32:15 +0100 From: Catalin Marinas To: Will Deacon Cc: Robin Murphy , Christoph Hellwig , linux-arm-kernel@lists.infradead.org, Mark Rutland , Ard Biesheuvel Subject: Re: [PATCH] arm64: dma: Drop cache invalidation from arch_dma_prep_coherent() Message-ID: References: <20220823122111.17439-1-will@kernel.org> <20220907090305.GA30704@lst.de> <5d856574-4cd7-70d0-adcb-3e284fef315f@arm.com> <20220907162543.GA30558@willie-the-truck> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220907162543.GA30558@willie-the-truck> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220908_033227_871374_771F8A9D X-CRM114-Status: GOOD ( 45.28 ) 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 05:25:43PM +0100, Will Deacon wrote: > 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? For the erratum you have in mind, invalidation doesn't help either (as you know already). The architecture does rule out unexpected cache hits, though not by stating this explicitly but rather only listing the cases where one needs cache maintenance in the presence of mismatched aliases. Such unexpected cache hits may happen micro-architecturally but are not visible to software (in theory and ignoring timing side-channels). > > 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. > */ The architecture requires invalidate or clean while also stating that clean+invalidate can be used instead, so I don't think there's much to justify. From the mismatched attributes section: 1. If the mismatched attributes for a memory location all assign the same shareability attribute to a Location that has a cacheable attribute, any loss of uniprocessor semantics, ordering, or coherency within a shareability domain can be avoided by use of software cache management. To do so, software must use the techniques that are required for the software management of the ordering or coherency of cacheable Locations between agents in different shareability domains. This means: - Before writing to a cacheable Location not using the Write-Back attribute, software must invalidate, or clean, a Location from the caches if any agent might have written to the Location with the Write-Back attribute. This avoids the possibility of overwriting the Location with stale data. - After writing to a cacheable Location with the Write-Back attribute, software must clean the Location from the caches, to make the write visible to external memory. ... When creating the non-cacheable alias for DMA, what we need is the first bullet point above. A clean would be required if we wrote something via the write-back mapping and we want that data to be visible (not needed if the zeroing is done via the non-cacheable alias). However, it just crossed my mind that we can't always drop the CPU-written (meta)data with an invalidate even if the zeroing is done on the non-cacheable alias. With MTE+kasan, the page allocator may colour the memory (via the cacheable alias). Even if non-cacheable accesses are not checked by MTE, when freeing it back it could potentially confuse the allocator if the tags were lost. Similarly for the streaming DMA and this could have been worse but luckily we had c50f11c6196f ("arm64: mm: Don't invalidate FROM_DEVICE buffers at start of DMA transfer") for other reasons. So yeah, I think a clean is needed here or clean+invalidate but not invalidate only due to the addition of MTE. -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel