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 smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (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 CDF87C433F5 for ; Sun, 29 May 2022 05:14:57 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 4AFCD4264D; Sun, 29 May 2022 05:14:57 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 52rRgfQ8Si5b; Sun, 29 May 2022 05:14:56 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp4.osuosl.org (Postfix) with ESMTPS id A5FD941901; Sun, 29 May 2022 05:14:55 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 42485C0039; Sun, 29 May 2022 05:14:55 +0000 (UTC) Received: from smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 8EDF3C0032 for ; Sun, 29 May 2022 05:14:53 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 8009E6136F for ; Sun, 29 May 2022 05:14:53 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Authentication-Results: smtp3.osuosl.org (amavisd-new); dkim=pass (2048-bit key) header.d=intel.com Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 5uDajgdvY21G for ; Sun, 29 May 2022 05:14:52 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by smtp3.osuosl.org (Postfix) with ESMTPS id 8576561153 for ; Sun, 29 May 2022 05:14:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1653801292; x=1685337292; h=message-id:date:mime-version:cc:subject:to:references: from:in-reply-to:content-transfer-encoding; bh=enAxaDvI5KYRp7pqOsswil/w+G42o1j4MPV69EUb6uk=; b=YjfYnItTT3MpZi+Yuz26WhcMkUm16LeinjMIud8qHU4UsW4JZKQs6Cx+ Ad7hGv3TY1n4AQZq8f8Fv17jAR0lL96owy5L+/kNT/K7cS1bOB+5IfQEc ll/MScvmtZsdle/9E6BMUXNA4EARGPwi8f4k9Nw+Dji+mrEKunCa8y4qB 4Rpwv/ibMp8kA1yNlAUBqLZ1BNfjUToKapvpK20JxKbL/UUgtslYsCcV5 SCxHfWigPlbgDPCbvRZEiLE6b/UxayawSIpBnIP6iJ0+lEBgIOroVvKXx 4jTIlwuGSbSukR4MWrVR5FES8rMXWkb+7K7n2L6wUXSp/vulcA9MfcUO5 g==; X-IronPort-AV: E=McAfee;i="6400,9594,10361"; a="254626690" X-IronPort-AV: E=Sophos;i="5.91,260,1647327600"; d="scan'208";a="254626690" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 May 2022 22:14:51 -0700 X-IronPort-AV: E=Sophos;i="5.91,260,1647327600"; d="scan'208";a="575463620" Received: from unknown (HELO [10.255.28.211]) ([10.255.28.211]) by orsmga007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 May 2022 22:14:48 -0700 Message-ID: Date: Sun, 29 May 2022 13:14:46 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [PATCH 01/12] iommu/vt-d: Use iommu_get_domain_for_dev() in debugfs Content-Language: en-US To: Jason Gunthorpe References: <20220527063019.3112905-1-baolu.lu@linux.intel.com> <20220527063019.3112905-2-baolu.lu@linux.intel.com> <20220527145910.GQ1343366@nvidia.com> From: Baolu Lu In-Reply-To: <20220527145910.GQ1343366@nvidia.com> Cc: Kevin Tian , Ashok Raj , Robin Murphy , linux-kernel@vger.kernel.org, Christoph Hellwig , iommu@lists.linux-foundation.org, Jacob jun Pan , Will Deacon X-BeenThere: iommu@lists.linux-foundation.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Development issues for Linux IOMMU support List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: iommu-bounces@lists.linux-foundation.org Sender: "iommu" On 2022/5/27 22:59, Jason Gunthorpe wrote: > On Fri, May 27, 2022 at 02:30:08PM +0800, Lu Baolu wrote: >> Retrieve the attached domain for a device through the generic interface >> exposed by the iommu core. This also makes device_domain_lock static. >> >> Signed-off-by: Lu Baolu >> drivers/iommu/intel/iommu.h | 1 - >> drivers/iommu/intel/debugfs.c | 20 ++++++++------------ >> drivers/iommu/intel/iommu.c | 2 +- >> 3 files changed, 9 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h >> index a22adfbdf870..8a6d64d726c0 100644 >> +++ b/drivers/iommu/intel/iommu.h >> @@ -480,7 +480,6 @@ enum { >> #define VTD_FLAG_SVM_CAPABLE (1 << 2) >> >> extern int intel_iommu_sm; >> -extern spinlock_t device_domain_lock; >> >> #define sm_supported(iommu) (intel_iommu_sm && ecap_smts((iommu)->ecap)) >> #define pasid_supported(iommu) (sm_supported(iommu) && \ >> diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/debugfs.c >> index d927ef10641b..eea8727aa7bc 100644 >> +++ b/drivers/iommu/intel/debugfs.c >> @@ -344,19 +344,21 @@ static void pgtable_walk_level(struct seq_file *m, struct dma_pte *pde, >> >> static int show_device_domain_translation(struct device *dev, void *data) >> { >> - struct device_domain_info *info = dev_iommu_priv_get(dev); >> - struct dmar_domain *domain = info->domain; >> + struct dmar_domain *dmar_domain; >> + struct iommu_domain *domain; >> struct seq_file *m = data; >> u64 path[6] = { 0 }; >> >> + domain = iommu_get_domain_for_dev(dev); >> if (!domain) >> return 0; > > The iommu_get_domain_for_dev() API should be called something like > 'iommu_get_dma_api_domain()' and clearly documented that it is safe to > call only so long as a DMA API using driver is attached to the device, > which is most of the current callers. Yes, agreed. > This use in random sysfs inside the iommu driver is not OK because it > doesn't have any locking protecting domain from concurrent free. This is not sysfs, but debugfs. The description of this patch is confusing. I should make it specific and straight-forward. How about below one? From 1e87b5df40c6ce9414cdd03988c3b52bfb17af5f Mon Sep 17 00:00:00 2001 From: Lu Baolu Date: Sun, 29 May 2022 10:18:56 +0800 Subject: [PATCH 1/1] iommu/vt-d: debugfs: Remove device_domain_lock usage The domain_translation_struct debugfs node is used to dump static mappings of PCI devices. It potentially races with setting new domains to devices and the iommu_map/unmap() interfaces. The existing code tries to use the global spinlock device_domain_lock to avoid the races, but this is problematical as this lock is only used to protect the device tracking lists of the domains. Instead of using an immature lock to cover up the problem, it's better to explicitly restrict the use of this debugfs node. This also makes device_domain_lock static. Signed-off-by: Lu Baolu --- drivers/iommu/intel/debugfs.c | 17 ++++++++--------- drivers/iommu/intel/iommu.c | 2 +- drivers/iommu/intel/iommu.h | 1 - 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/debugfs.c index d927ef10641b..9642e3e9d6b0 100644 --- a/drivers/iommu/intel/debugfs.c +++ b/drivers/iommu/intel/debugfs.c @@ -362,17 +362,16 @@ static int show_device_domain_translation(struct device *dev, void *data) return 0; } +/* + * Dump the static mappings of PCI devices. This is only for DEBUGFS code, + * don't use it for other purposes. It potentially races with setting new + * domains to devices and iommu_map/unmap(). Use the trace events under + * /sys/kernel/debug/tracing/events/iommu/ for dynamic debugging. + */ static int domain_translation_struct_show(struct seq_file *m, void *unused) { - unsigned long flags; - int ret; - - spin_lock_irqsave(&device_domain_lock, flags); - ret = bus_for_each_dev(&pci_bus_type, NULL, m, - show_device_domain_translation); - spin_unlock_irqrestore(&device_domain_lock, flags); - - return ret; + return bus_for_each_dev(&pci_bus_type, NULL, m, + show_device_domain_translation); } DEFINE_SHOW_ATTRIBUTE(domain_translation_struct); diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 1af4b6562266..cacae8bdaa65 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -314,7 +314,7 @@ static int iommu_skip_te_disable; #define IDENTMAP_GFX 2 #define IDENTMAP_AZALIA 4 -DEFINE_SPINLOCK(device_domain_lock); +static DEFINE_SPINLOCK(device_domain_lock); static LIST_HEAD(device_domain_list); /* diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h index a22adfbdf870..8a6d64d726c0 100644 --- a/drivers/iommu/intel/iommu.h +++ b/drivers/iommu/intel/iommu.h @@ -480,7 +480,6 @@ enum { #define VTD_FLAG_SVM_CAPABLE (1 << 2) extern int intel_iommu_sm; -extern spinlock_t device_domain_lock; #define sm_supported(iommu) (intel_iommu_sm && ecap_smts((iommu)->ecap)) #define pasid_supported(iommu) (sm_supported(iommu) && \ -- 2.25.1 Best regards, baolu _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu