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 X-Spam-Level: X-Spam-Status: No, score=-11.5 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, NICE_REPLY_A,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7B1D6C433E0 for ; Wed, 22 Jul 2020 01:28:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3A188206F5 for ; Wed, 22 Jul 2020 01:28:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=ozlabs-ru.20150623.gappssmtp.com header.i=@ozlabs-ru.20150623.gappssmtp.com header.b="lkR0/0gQ" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731704AbgGVB2o (ORCPT ); Tue, 21 Jul 2020 21:28:44 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44012 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731590AbgGVB2n (ORCPT ); Tue, 21 Jul 2020 21:28:43 -0400 Received: from mail-pf1-x444.google.com (mail-pf1-x444.google.com [IPv6:2607:f8b0:4864:20::444]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A57F1C061794 for ; Tue, 21 Jul 2020 18:28:43 -0700 (PDT) Received: by mail-pf1-x444.google.com with SMTP id x72so328457pfc.6 for ; Tue, 21 Jul 2020 18:28:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ozlabs-ru.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:autocrypt:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=QNDO0W6m2omey41jkHkiXEG6oxvER9ORKwM6l7l7E0w=; b=lkR0/0gQXJLB8iawCVkENifJPZc/ouAno8gGkoNca2gWN+6fNiGRwMtmKzjDo2I0QN kgMViyVB+wO1crRwN2rhvtu/w8Wnq07wPeGaGUSf4cHL3+1eWbiEcg3hYU4010mTpGTK zwJEAX66HHCNpGCMbAC7eAAGPv3xX0AoezGlAZFZ4GNBSLlKsqtfRFqjRI12i6Z2AsqH /UA4LqeV+hOMo6LdVG2wAtb+l2eGn0yPkf+fVwliGr/I/xBJmM966uwDuvyytldQabG7 z26d7IE4KCS21j+60hIeitG+cdZPi5rmMjlmrgrPbbLW86LpbiX3oGGE0pK3AkvwvFeX iXZQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:autocrypt :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=QNDO0W6m2omey41jkHkiXEG6oxvER9ORKwM6l7l7E0w=; b=PXfu0yuoPa4/wRNNFlsvIBQJElgSKH/dd7nTAat4kIhrw2PtD6dhrwb5+QAr/qnynU ivFO9a7H2WWuTrSHHiet1HhsNE+ZNyPP0C/qEq2CZTh7ytjxxyZd54CEMFKhILxpBo3K DQI2VkJduivVoAqM/oovy1gpn8SyBo52HxJHKz41d2PqctYX/a05Pclw6ph+xAG2p16f BiW5aQWxhBxz98e2dENjfv0drZG0BLHxTD9+ADIaD6yhvEAuB4BBXbkDVdsKhiaGsRiy MX6KQqbwj6u5wsngDfgwPl7tQrMyL5K/uDgmaVgS7F8jRaDwyz2TlRgDhXKXLQqyAvnR +hUA== X-Gm-Message-State: AOAM532BSF+NV+4p7hmMkDJNHg+pmvNM9JoZNSkKLns5W7IkEFLX/ATE sVg7oGwE6hovgRI9OGEU+1hJFCDJc6kCMg== X-Google-Smtp-Source: ABdhPJwDtPV9CLwv4uZ3R1dtVwMVmH85j8I9XYRrbTreHyZMQgDkx8JXrxsypsiRQvYl1SOnAcsm/Q== X-Received: by 2002:a63:6fc7:: with SMTP id k190mr24744133pgc.54.1595381322637; Tue, 21 Jul 2020 18:28:42 -0700 (PDT) Received: from [192.168.10.94] (124-171-83-152.dyn.iinet.net.au. [124.171.83.152]) by smtp.gmail.com with ESMTPSA id r16sm21521880pfh.64.2020.07.21.18.28.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 21 Jul 2020 18:28:41 -0700 (PDT) Subject: Re: [PATCH v4 5/7] powerpc/iommu: Move iommu_table cleaning routine to iommu_table_clean To: Leonardo Bras , Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Joel Stanley , Christophe Leroy , Thiago Jung Bauermann , Ram Pai , Brian King Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org References: <20200716071658.467820-1-leobras.c@gmail.com> <20200716071658.467820-6-leobras.c@gmail.com> <51235292-a571-8792-c693-d0dc6faeb21c@ozlabs.ru> <0f4c2d84d0958e98e7ada53c25750fe548cadf0b.camel@gmail.com> From: Alexey Kardashevskiy Autocrypt: addr=aik@ozlabs.ru; keydata= mQINBE+rT0sBEADFEI2UtPRsLLvnRf+tI9nA8T91+jDK3NLkqV+2DKHkTGPP5qzDZpRSH6mD EePO1JqpVuIow/wGud9xaPA5uvuVgRS1q7RU8otD+7VLDFzPRiRE4Jfr2CW89Ox6BF+q5ZPV /pS4v4G9eOrw1v09lEKHB9WtiBVhhxKK1LnUjPEH3ifkOkgW7jFfoYgTdtB3XaXVgYnNPDFo PTBYsJy+wr89XfyHr2Ev7BB3Xaf7qICXdBF8MEVY8t/UFsesg4wFWOuzCfqxFmKEaPDZlTuR tfLAeVpslNfWCi5ybPlowLx6KJqOsI9R2a9o4qRXWGP7IwiMRAC3iiPyk9cknt8ee6EUIxI6 t847eFaVKI/6WcxhszI0R6Cj+N4y+1rHfkGWYWupCiHwj9DjILW9iEAncVgQmkNPpUsZECLT WQzMuVSxjuXW4nJ6f4OFHqL2dU//qR+BM/eJ0TT3OnfLcPqfucGxubhT7n/CXUxEy+mvWwnm s9p4uqVpTfEuzQ0/bE6t7dZdPBua7eYox1AQnk8JQDwC3Rn9kZq2O7u5KuJP5MfludMmQevm pHYEMF4vZuIpWcOrrSctJfIIEyhDoDmR34bCXAZfNJ4p4H6TPqPh671uMQV82CfTxTrMhGFq 8WYU2AH86FrVQfWoH09z1WqhlOm/KZhAV5FndwVjQJs1MRXD8QARAQABtCRBbGV4ZXkgS2Fy ZGFzaGV2c2tpeSA8YWlrQG96bGFicy5ydT6JAjgEEwECACIFAk+rT0sCGwMGCwkIBwMCBhUI AgkKCwQWAgMBAh4BAheAAAoJEIYTPdgrwSC5fAIP/0wf/oSYaCq9PhO0UP9zLSEz66SSZUf7 AM9O1rau1lJpT8RoNa0hXFXIVbqPPKPZgorQV8SVmYRLr0oSmPnTiZC82x2dJGOR8x4E01gK TanY53J/Z6+CpYykqcIpOlGsytUTBA+AFOpdaFxnJ9a8p2wA586fhCZHVpV7W6EtUPH1SFTQ q5xvBmr3KkWGjz1FSLH4FeB70zP6uyuf/B2KPmdlPkyuoafl2UrU8LBADi/efc53PZUAREih sm3ch4AxaL4QIWOmlE93S+9nHZSRo9jgGXB1LzAiMRII3/2Leg7O4hBHZ9Nki8/fbDo5///+ kD4L7UNbSUM/ACWHhd4m1zkzTbyRzvL8NAVQ3rckLOmju7Eu9whiPueGMi5sihy9VQKHmEOx OMEhxLRQbzj4ypRLS9a+oxk1BMMu9cd/TccNy0uwx2UUjDQw/cXw2rRWTRCxoKmUsQ+eNWEd iYLW6TCfl9CfHlT6A7Zmeqx2DCeFafqEd69DqR9A8W5rx6LQcl0iOlkNqJxxbbW3ddDsLU/Y r4cY20++WwOhSNghhtrroP+gouTOIrNE/tvG16jHs8nrYBZuc02nfX1/gd8eguNfVX/ZTHiR gHBWe40xBKwBEK2UeqSpeVTohYWGBkcd64naGtK9qHdo1zY1P55lHEc5Uhlk743PgAnOi27Q ns5zuQINBE+rT0sBEACnV6GBSm+25ACT+XAE0t6HHAwDy+UKfPNaQBNTTt31GIk5aXb2Kl/p AgwZhQFEjZwDbl9D/f2GtmUHWKcCmWsYd5M/6Ljnbp0Ti5/xi6FyfqnO+G/wD2VhGcKBId1X Em/B5y1kZVbzcGVjgD3HiRTqE63UPld45bgK2XVbi2+x8lFvzuFq56E3ZsJZ+WrXpArQXib2 hzNFwQleq/KLBDOqTT7H+NpjPFR09Qzfa7wIU6pMNF2uFg5ihb+KatxgRDHg70+BzQfa6PPA o1xioKXW1eHeRGMmULM0Eweuvpc7/STD3K7EJ5bBq8svoXKuRxoWRkAp9Ll65KTUXgfS+c0x gkzJAn8aTG0z/oEJCKPJ08CtYQ5j7AgWJBIqG+PpYrEkhjzSn+DZ5Yl8r+JnZ2cJlYsUHAB9 jwBnWmLCR3gfop65q84zLXRQKWkASRhBp4JK3IS2Zz7Nd/Sqsowwh8x+3/IUxVEIMaVoUaxk Wt8kx40h3VrnLTFRQwQChm/TBtXqVFIuv7/Mhvvcq11xnzKjm2FCnTvCh6T2wJw3de6kYjCO 7wsaQ2y3i1Gkad45S0hzag/AuhQJbieowKecuI7WSeV8AOFVHmgfhKti8t4Ff758Z0tw5Fpc BFDngh6Lty9yR/fKrbkkp6ux1gJ2QncwK1v5kFks82Cgj+DSXK6GUQARAQABiQIfBBgBAgAJ BQJPq09LAhsMAAoJEIYTPdgrwSC5NYEP/2DmcEa7K9A+BT2+G5GXaaiFa098DeDrnjmRvumJ BhA1UdZRdfqICBADmKHlJjj2xYo387sZpS6ABbhrFxM6s37g/pGPvFUFn49C47SqkoGcbeDz Ha7JHyYUC+Tz1dpB8EQDh5xHMXj7t59mRDgsZ2uVBKtXj2ZkbizSHlyoeCfs1gZKQgQE8Ffc F8eWKoqAQtn3j4nE3RXbxzTJJfExjFB53vy2wV48fUBdyoXKwE85fiPglQ8bU++0XdOr9oyy j1llZlB9t3tKVv401JAdX8EN0++ETiOovQdzE1m+6ioDCtKEx84ObZJM0yGSEGEanrWjiwsa nzeK0pJQM9EwoEYi8TBGhHC9ksaAAQipSH7F2OHSYIlYtd91QoiemgclZcSgrxKSJhyFhmLr QEiEILTKn/pqJfhHU/7R7UtlDAmFMUp7ByywB4JLcyD10lTmrEJ0iyRRTVfDrfVP82aMBXgF tKQaCxcmLCaEtrSrYGzd1sSPwJne9ssfq0SE/LM1J7VdCjm6OWV33SwKrfd6rOtvOzgadrG6 3bgUVBw+bsXhWDd8tvuCXmdY4bnUblxF2B6GOwSY43v6suugBttIyW5Bl2tXSTwP+zQisOJo +dpVG2pRr39h+buHB3NY83NEPXm1kUOhduJUA17XUY6QQCAaN4sdwPqHq938S3EmtVhsuQIN BFq54uIBEACtPWrRdrvqfwQF+KMieDAMGdWKGSYSfoEGGJ+iNR8v255IyCMkty+yaHafvzpl PFtBQ/D7Fjv+PoHdFq1BnNTk8u2ngfbre9wd9MvTDsyP/TmpF0wyyTXhhtYvE267Av4X/BQT lT9IXKyAf1fP4BGYdTNgQZmAjrRsVUW0j6gFDrN0rq2J9emkGIPvt9rQt6xGzrd6aXonbg5V j6Uac1F42ESOZkIh5cN6cgnGdqAQb8CgLK92Yc8eiCVCH3cGowtzQ2m6U32qf30cBWmzfSH0 HeYmTP9+5L8qSTA9s3z0228vlaY0cFGcXjdodBeVbhqQYseMF9FXiEyRs28uHAJEyvVZwI49 CnAgVV/n1eZa5qOBpBL+ZSURm8Ii0vgfvGSijPGbvc32UAeAmBWISm7QOmc6sWa1tobCiVmY SNzj5MCNk8z4cddoKIc7Wt197+X/X5JPUF5nQRvg3SEHvfjkS4uEst9GwQBpsbQYH9MYWq2P PdxZ+xQE6v7cNB/pGGyXqKjYCm6v70JOzJFmheuUq0Ljnfhfs15DmZaLCGSMC0Amr+rtefpA y9FO5KaARgdhVjP2svc1F9KmTUGinSfuFm3quadGcQbJw+lJNYIfM7PMS9fftq6vCUBoGu3L j4xlgA/uQl/LPneu9mcvit8JqcWGS3fO+YeagUOon1TRqQARAQABiQRsBBgBCAAgFiEEZSrP ibrORRTHQ99dhhM92CvBILkFAlq54uICGwICQAkQhhM92CvBILnBdCAEGQEIAB0WIQQIhvWx rCU+BGX+nH3N7sq0YorTbQUCWrni4gAKCRDN7sq0YorTbVVSD/9V1xkVFyUCZfWlRuryBRZm S4GVaNtiV2nfUfcThQBfF0sSW/aFkLP6y+35wlOGJE65Riw1C2Ca9WQYk0xKvcZrmuYkK3DZ 0M9/Ikkj5/2v0vxz5Z5w/9+IaCrnk7pTnHZuZqOh23NeVZGBls/IDIvvLEjpD5UYicH0wxv+ X6cl1RoP2Kiyvenf0cS73O22qSEw0Qb9SId8wh0+ClWet2E7hkjWFkQfgJ3hujR/JtwDT/8h 3oCZFR0KuMPHRDsCepaqb/k7VSGTLBjVDOmr6/C9FHSjq0WrVB9LGOkdnr/xcISDZcMIpbRm EkIQ91LkT/HYIImL33ynPB0SmA+1TyMgOMZ4bakFCEn1vxB8Ir8qx5O0lHMOiWMJAp/PAZB2 r4XSSHNlXUaWUg1w3SG2CQKMFX7vzA31ZeEiWO8tj/c2ZjQmYjTLlfDK04WpOy1vTeP45LG2 wwtMA1pKvQ9UdbYbovz92oyZXHq81+k5Fj/YA1y2PI4MdHO4QobzgREoPGDkn6QlbJUBf4To pEbIGgW5LRPLuFlOPWHmIS/sdXDrllPc29aX2P7zdD/ivHABslHmt7vN3QY+hG0xgsCO1JG5 pLORF2N5XpM95zxkZqvYfC5tS/qhKyMcn1kC0fcRySVVeR3tUkU8/caCqxOqeMe2B6yTiU1P aNDq25qYFLeYxg67D/4w/P6BvNxNxk8hx6oQ10TOlnmeWp1q0cuutccblU3ryRFLDJSngTEu ZgnOt5dUFuOZxmMkqXGPHP1iOb+YDznHmC0FYZFG2KAc9pO0WuO7uT70lL6larTQrEneTDxQ CMQLP3qAJ/2aBH6SzHIQ7sfbsxy/63jAiHiT3cOaxAKsWkoV2HQpnmPOJ9u02TPjYmdpeIfa X2tXyeBixa3i/6dWJ4nIp3vGQicQkut1YBwR7dJq67/FCV3Mlj94jI0myHT5PIrCS2S8LtWX ikTJSxWUKmh7OP5mrqhwNe0ezgGiWxxvyNwThOHc5JvpzJLd32VDFilbxgu4Hhnf6LcgZJ2c Zd44XWqUu7FzVOYaSgIvTP0hNrBYm/E6M7yrLbs3JY74fGzPWGRbBUHTZXQEqQnZglXaVB5V ZhSFtHopZnBSCUSNDbB+QGy4B/E++Bb02IBTGl/JxmOwG+kZUnymsPvTtnNIeTLHxN/H/ae0 c7E5M+/NpslPCmYnDjs5qg0/3ihh6XuOGggZQOqrYPC3PnsNs3NxirwOkVPQgO6mXxpuifvJ DG9EMkK8IBXnLulqVk54kf7fE0jT/d8RTtJIA92GzsgdK2rpT1MBKKVffjRFGwN7nQVOzi4T XrB5p+6ML7Bd84xOEGsj/vdaXmz1esuH7BOZAGEZfLRCHJ0GVCSssg== Message-ID: Date: Wed, 22 Jul 2020 11:28:36 +1000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <0f4c2d84d0958e98e7ada53c25750fe548cadf0b.camel@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 22/07/2020 08:13, Leonardo Bras wrote: > On Tue, 2020-07-21 at 14:59 +1000, Alexey Kardashevskiy wrote: >> >> On 16/07/2020 17:16, Leonardo Bras wrote: >>> Move the part of iommu_table_free() that does struct iommu_table cleaning >>> into iommu_table_clean, so we can invoke it separately. >>> >>> This new function is useful for cleaning struct iommu_table before >>> initializing it again with a new DMA window, without having it freed and >>> allocated again. >>> >>> Signed-off-by: Leonardo Bras >>> --- >>> arch/powerpc/kernel/iommu.c | 30 ++++++++++++++++++------------ >>> 1 file changed, 18 insertions(+), 12 deletions(-) >>> >>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c >>> index 9704f3f76e63..c3242253a4e7 100644 >>> --- a/arch/powerpc/kernel/iommu.c >>> +++ b/arch/powerpc/kernel/iommu.c >>> @@ -735,21 +735,10 @@ struct iommu_table *iommu_init_table(struct iommu_table *tbl, int nid, >>> return tbl; >>> } >>> >>> -static void iommu_table_free(struct kref *kref) >>> +static void iommu_table_clean(struct iommu_table *tbl) >> >> iommu_table_free() + iommu_init_table() + set_iommu_table_base() should >> work too, why new helper? > > iommu_table_free() also frees the tbl, which would need allocate it > again (new address) and to fill it up again, unnecessarily. It is a new table in fact, everything is new there. You are only saving kfree+kzalloc which does not seem a huge win. Also, iommu_table_update() simply assumes 64bit window by passing res_start=res_end=0 to iommu_init_table() which is not horribly robust either. Yeah, I know, iommu_init_table() is always called with zeroes in pseries but this is somewhat ok as those tables are from the device tree and those windows don't overlap with 32bit MMIO but under KVM they will (well, if we hack QEMU to advertise a single window). I suggest removing iommu_pseries_table_update() from 6/7 and do iommu_table_free() + iommu_init_table() + set_iommu_table_base() with a WARN_ON(pdev->dev.archdata.dma_offset>=SZ_4G), may be even do this all in enable_ddw() where we know for sure if it is 1:1 mapping or just a big window. Out of curiosity - what page sizes does pHyp advertise in "query"? > I think it's a better approach to only change what is needed. > >> There is also iommu_table_clear() which does a different thing so you >> need a better name. > > I agree. > I had not noticed this other function before sending the patchset. What > would be a better name though? __iommu_table_free()? > >> Second, iommu_table_free >> use and it would be ok as we would only see this when hot-unplugging a >> PE because we always kept the default window. >> Btw you must be seeing these warnings now every time you create DDW with >> these patches as at least the first page is reserved, do not you? > > It does not print a warning. > I noticed other warnings, And what are these? > but not this one from iommu_table_free(): > /* verify that table contains no entries */ > if (!bitmap_empty(tbl->it_ma > p, tbl->it_size)) > pr_warn("%s: Unexpected TCEs\n", __func__); > > Before that, iommu_table_release_pages(tbl) is supposed to clear the > bitmap, so this only tests for a tce that is created in this short period. iommu_table_release_pages() only clears reserved pages - page 0 (just a protection against NULL DMA pointers) and 32bit MMIO (these should not be set for 64bit window). The "%s: Unexpected TCEs\n" is what checks for actual mapped TCEs. >> Since we are replacing a table for a device which is still in the >> system, we should not try messing with its DMA if it already has >> mappings so the warning should become an error preventing DDW. It is >> rather hard to trigger in practice but I could hack a driver to ask for >> 32bit DMA mask first, map few pages and then ask for 64bit DMA mask, it >> is not illegal, I think. So this needs a new helper - "bool >> iommu_table_in_use(tbl)" - to use in enable_ddw(). Or I am overthinking >> this?... Thanks, > > As of today, there seems to be nothing like that happening in the > driver I am testing. > I spoke to Brian King on slack, and he mentioned that at the point DDW > is created there should be no allocations in place. Correct, there should not be. But it is also not a huge effort to fall back if there are. > But I suppose some driver could try to do this. > > Maybe a better approach would be removing the mapping only if the > default window is removed (at the end of enable_ddw, as an else to > resetting the default DMA window), and having a way to add more > mappings to those pools. But this last part doesn't look so simple, and > it would be better to understand if it's necessary investing work in > this. > > What do you think? Add iommu_table_in_use(tbl) and fail DDW if that says "yes". -- Alexey