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 06673CD5BC7 for ; Thu, 5 Sep 2024 13:34:06 +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:References:Cc:To:From: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=jCsebiDvO20BfjKikyud1rwbC063yDMaIfpPi7RmXXM=; b=Gx5TeftX3zFaosH8CVYqDItd/k e6EKxbc1cNxmeA4BvTpRmYBGNcZMUnC8b9E3xLh+GoosoSqXsUaWX65gOmN8I1jnwQNJKcPtJEuc6 ReGsY5FT5P5CUc9Ke06w6ToHA6TLWhrzAA/wSpmkcwqfSiu4DpXO7aSZSI9rXrE2z0rSgYI81TEhv ChNZhU2BGBN0+ml6TLh/QsU9w4qazy7CQZKJzlg+5KjHHDXcxqtJW/j4fuDAOXqaSgoLebsB8s1LK 4euWQ03aXklJgraZarKIu/ak+wGd3YkO5nmhDnnADPI8T4+b8d3IDphrlyFNmOZlaONlCedGNl5tH a2BgDtWA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1smCcV-00000008YAZ-36XC; Thu, 05 Sep 2024 13:33:55 +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 1smCbV-00000008Xw1-24fx for linux-arm-kernel@lists.infradead.org; Thu, 05 Sep 2024 13:32:55 +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 D8953FEC; Thu, 5 Sep 2024 06:33:18 -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 48C453F73F; Thu, 5 Sep 2024 06:32:50 -0700 (PDT) Message-ID: <4dbdd1ee-fedd-4b97-86ba-ffe6af8d0473@arm.com> Date: Thu, 5 Sep 2024 14:32:46 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] Revert "iommu/io-pgtable-arm: Optimise non-coherent unmap" From: Robin Murphy 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> Content-Language: en-GB In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240905_063253_649987_B428B417 X-CRM114-Status: GOOD ( 27.27 ) 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 2:24 pm, Robin Murphy wrote: > 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: ...or at least I *thought* I'd just commented, but Thunderbird is now denying any knowledge of the reply to your other mail I swore I sent a couple of hours ago :/ Apologies for any confusion, Robin. > >     __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)) { >