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 DB16BC04A6A for ; Wed, 9 Aug 2023 13:50:59 +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=300tH5B2VeSg3f3EOYFpPOoNtZ31QnbQvL9Gr40g3ZE=; b=qmKLQLUbkYpNbq a7uGWNRsh10bpgissQ57vYRs3ljODuyAoGZajicL2XzGo8KolL1kqh8FEV+KdM1NIuC5aFk+9yzVE vGQdQaG5gpEjybZpZnd3KTpJbJTO7CCBdUPn4AwEEXfOfDEzuTmyECql0PLAeDhJM4C9uS/gt/N8Y 0OH1UIkR4ti2xKpALy/sRNgLDcb55AbwbOR8h1NnVUIE1T2UT1tfbKjdommEWjqoG//yDbtRSetOH eKtE0hp+XY0xPDLeMv3OMkjjNDz+TBYMHWKAKK1WvjtPBR5H5r/jXcJFq65LAGS41ppmfECJTQHO3 0Lk2i3QoBG/4BGXV49sw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qTja6-0054wq-0v; Wed, 09 Aug 2023 13:50:34 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qTja3-0054vE-0m for linux-arm-kernel@lists.infradead.org; Wed, 09 Aug 2023 13:50:32 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id B832D613C5; Wed, 9 Aug 2023 13:50:30 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 93292C433CA; Wed, 9 Aug 2023 13:50:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1691589030; bh=Ttd/GCX5wRwkEEojWI8vGi9iFAE5AgRVud/qLF/aTj0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=cQYhiZ4zfQt35rRYlT6nQITMz8qubxI0lUV2X7d2J8IqZIK2wyTROCAzjEhUStMI/ pVHNF8XaepzYEmib0IzCOq+uFKVNdeJcJeMdp8v4+cIyPkbRifzRLyT3hZjOidGWwu uLyStbavPg+g5T7c64Ht6gvqjOE6sWW6MHnSy9IdtAYAKI3mYchHKsbKzeZfd8NdbB Q62Xm02yu/2o1eWw6AL3ZEd5R2SWTyyk7AHZkXvmNXC7rkgyvukE4xwh00FBwsxKrX iS2Xt1Osorq8s5Dccxiw32V/kIZy44eh0i1hOPIzQ9KfCfuEb0og7qrnI0dAHW/pfj R2pyL9F/CDduA== Date: Wed, 9 Aug 2023 14:50:25 +0100 From: Will Deacon To: Michael Shavit Cc: iommu@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, robin.murphy@arm.com, nicolinc@nvidia.com, jgg@nvidia.com, jean-philippe@linaro.org Subject: Re: [PATCH v5 6/9] iommu/arm-smmu-v3: Move CD table to arm_smmu_master Message-ID: <20230809135024.GD4226@willie-the-truck> References: <20230808171446.2187795-1-mshavit@google.com> <20230809011204.v5.6.Ice063dcf87d1b777a72e008d9e3406d2bcf6d876@changeid> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230809011204.v5.6.Ice063dcf87d1b777a72e008d9e3406d2bcf6d876@changeid> 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-20230809_065031_375059_0A60AA0E X-CRM114-Status: GOOD ( 21.73 ) 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, Aug 09, 2023 at 01:12:02AM +0800, Michael Shavit wrote: > @@ -2203,7 +2186,7 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain, > ias = min_t(unsigned long, ias, VA_BITS); > oas = smmu->ias; > fmt = ARM_64_LPAE_S1; > - finalise_stage_fn = arm_smmu_domain_finalise_s1; > + finalise_stage_fn = arm_smmu_domain_finalise_cd; Why is this a better name? Now we have inconsistency with arm_smmu_domain_finalise_s2(). > break; > case ARM_SMMU_DOMAIN_NESTED: > case ARM_SMMU_DOMAIN_S2: > @@ -2402,6 +2385,16 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master) > master->domain = NULL; > master->ats_enabled = false; > arm_smmu_install_ste_for_dev(master); > + /* > + * The table is uninstalled before clearing the CD to prevent an > + * unnecessary sync in arm_smmu_write_ctx_desc. Although clearing the > + * CD entry isn't strictly required to detach the domain since the > + * table is uninstalled anyway, it's more proper and helps avoid > + * confusion in the call to arm_smmu_write_ctx_desc on the next attach You can remove the "it's more proper" part. > + * (which expects the entry to be empty). > + */ > + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && master->cd_table.cdtab) > + arm_smmu_write_ctx_desc(master, 0, NULL); > } > > static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > @@ -2436,22 +2429,14 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > if (!smmu_domain->smmu) { > smmu_domain->smmu = smmu; > ret = arm_smmu_domain_finalise(domain, master); > - if (ret) { > + if (ret) > smmu_domain->smmu = NULL; > - goto out_unlock; > - } > - } else if (smmu_domain->smmu != smmu) { > - ret = -EINVAL; > - goto out_unlock; > - } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && > - master->ssid_bits != smmu_domain->cd_table.max_cds_bits) { > + } else if (smmu_domain->smmu != smmu) > ret = -EINVAL; > - goto out_unlock; > - } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && > - smmu_domain->cd_table.stall_enabled != master->stall_enabled) { > - ret = -EINVAL; > - goto out_unlock; > - } Removing these checks on the domain is pretty nice. > @@ -2465,6 +2450,22 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS) > master->ats_enabled = arm_smmu_ats_supported(master); > > + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { > + if (!master->cd_table.cdtab) { > + ret = arm_smmu_alloc_cd_tables(master); > + if (ret) { > + master->domain = NULL; > + return ret; > + } > + } > + > + ret = arm_smmu_write_ctx_desc(master, 0, &smmu_domain->cd); > + if (ret) { > + master->domain = NULL; > + return ret; Can you leak the cd tables here if you just allocated them? > @@ -2472,10 +2473,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > > arm_smmu_enable_ats(master); > - > -out_unlock: > - mutex_unlock(&smmu_domain->init_mutex); > - return ret; > + return 0; > } > > static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned long iova, > @@ -2719,6 +2717,8 @@ static void arm_smmu_release_device(struct device *dev) > arm_smmu_detach_dev(master); > arm_smmu_disable_pasid(master); > arm_smmu_remove_master(master); > + if (master->cd_table.cdtab_dma) Why are you checking 'cdtab_dma' here instead of just 'cdtab'? Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel