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 53F2FCD5BC7 for ; Thu, 5 Sep 2024 13:26:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=XF3hawPkE3d0WFjT+nJtsD5Z8GKcz67e1Z7Y6a1uvfo=; b=CIJN7GafL/AVCAIRB+S+9aAt50 GaQla4fgqHOsus6NgCjmVUbKbU3wG6r2V+v4ucIxs+78JGsuA+pNKU9ohWsfQDuaUCHru7DfUA4EY CkSzo/EoPWWYx45WGah9Knn8Jx85bR5FPyNdmCWwSMBJWlHtiscFKA9UZQu7r+HlHJwEmNqV3FKeR C1Lq6AxPyAsRZelJTyAzVHEONGcYEBGUy8lgxgHoIrwAoe6rjrdy4VW+BENcq7POuapfKzSUzPrTo /UTpbaRuw7FQgiNg8m2pazUhb2U94f/SfGHkD5fu8ux/r4PzI6p5IV833ApUItPG861ZLertzE5to 20rqy+4g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1smCUu-00000008Wot-2GtF; Thu, 05 Sep 2024 13:26:04 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1smCTC-00000008WOC-430h for linux-arm-kernel@lists.infradead.org; Thu, 05 Sep 2024 13:24:20 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 7C1A5FEC; Thu, 5 Sep 2024 06:24:44 -0700 (PDT) Received: from [10.1.196.40] (e121345-lin.cambridge.arm.com [10.1.196.40]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6ED823F73F; Thu, 5 Sep 2024 06:24:16 -0700 (PDT) Message-ID: Date: Thu, 5 Sep 2024 14:24:14 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] Revert "iommu/io-pgtable-arm: Optimise non-coherent unmap" To: Rob Clark , iommu@lists.linux.dev Cc: linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org, Ashish Mhetre , Rob Clark , Will Deacon , Joerg Roedel , "moderated list:ARM SMMU DRIVERS" , open list References: <20240905124956.84932-1-robdclark@gmail.com> From: Robin Murphy Content-Language: en-GB In-Reply-To: <20240905124956.84932-1-robdclark@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240905_062419_249574_10433D19 X-CRM114-Status: GOOD ( 26.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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 05/09/2024 1:49 pm, Rob Clark wrote: > From: Rob Clark > > This reverts commit 85b715a334583488ad7fbd3001fe6fd617b7d4c0. > > It was causing gpu smmu faults on x1e80100. > > I _think_ what is causing this is the change in ordering of > __arm_lpae_clear_pte() (dma_sync_single_for_device() on the pgtable > memory) and io_pgtable_tlb_flush_walk(). As I just commented, how do you believe the order of operations between: __arm_lpae_clear_pte(); if (!iopte_leaf()) { io_pgtable_tlb_flush_walk(); and: if (!iopte_leaf()) { __arm_lpae_clear_pte(); io_pgtable_tlb_flush_walk(); fundamentally differs? I'm not saying there couldn't be some subtle bug in the implementation which we've all missed, but I still can't see an issue with the intended logic. > I'm not entirely sure how > this patch is supposed to work correctly in the face of other > concurrent translations (to buffers unrelated to the one being > unmapped(), because after the io_pgtable_tlb_flush_walk() we can have > stale data read back into the tlb. Read back from where? The ex-table PTE which was already set to zero before tlb_flush_walk was called? And isn't the hilariously overcomplicated TBU driver supposed to be telling you exactly what happened here? Otherwise I'm going to continue to seriously question the purpose of shoehorning that upstream at all... Thanks, Robin. > Signed-off-by: Rob Clark > --- > drivers/iommu/io-pgtable-arm.c | 31 ++++++++++++++----------------- > 1 file changed, 14 insertions(+), 17 deletions(-) > > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c > index 16e51528772d..85261baa3a04 100644 > --- a/drivers/iommu/io-pgtable-arm.c > +++ b/drivers/iommu/io-pgtable-arm.c > @@ -274,13 +274,13 @@ static void __arm_lpae_sync_pte(arm_lpae_iopte *ptep, int num_entries, > sizeof(*ptep) * num_entries, DMA_TO_DEVICE); > } > > -static void __arm_lpae_clear_pte(arm_lpae_iopte *ptep, struct io_pgtable_cfg *cfg, int num_entries) > +static void __arm_lpae_clear_pte(arm_lpae_iopte *ptep, struct io_pgtable_cfg *cfg) > { > - for (int i = 0; i < num_entries; i++) > - ptep[i] = 0; > > - if (!cfg->coherent_walk && num_entries) > - __arm_lpae_sync_pte(ptep, num_entries, cfg); > + *ptep = 0; > + > + if (!cfg->coherent_walk) > + __arm_lpae_sync_pte(ptep, 1, cfg); > } > > static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data, > @@ -653,28 +653,25 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data, > max_entries = ARM_LPAE_PTES_PER_TABLE(data) - unmap_idx_start; > num_entries = min_t(int, pgcount, max_entries); > > - /* Find and handle non-leaf entries */ > - for (i = 0; i < num_entries; i++) { > - pte = READ_ONCE(ptep[i]); > + while (i < num_entries) { > + pte = READ_ONCE(*ptep); > if (WARN_ON(!pte)) > break; > > - if (!iopte_leaf(pte, lvl, iop->fmt)) { > - __arm_lpae_clear_pte(&ptep[i], &iop->cfg, 1); > + __arm_lpae_clear_pte(ptep, &iop->cfg); > > + if (!iopte_leaf(pte, lvl, iop->fmt)) { > /* Also flush any partial walks */ > io_pgtable_tlb_flush_walk(iop, iova + i * size, size, > ARM_LPAE_GRANULE(data)); > __arm_lpae_free_pgtable(data, lvl + 1, iopte_deref(pte, data)); > + } else if (!iommu_iotlb_gather_queued(gather)) { > + io_pgtable_tlb_add_page(iop, gather, iova + i * size, size); > } > - } > > - /* Clear the remaining entries */ > - __arm_lpae_clear_pte(ptep, &iop->cfg, i); > - > - if (gather && !iommu_iotlb_gather_queued(gather)) > - for (int j = 0; j < i; j++) > - io_pgtable_tlb_add_page(iop, gather, iova + j * size, size); > + ptep++; > + i++; > + } > > return i * size; > } else if (iopte_leaf(pte, lvl, iop->fmt)) {