Message ID | 20221114014049.3959-5-baolu.lu@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp1913133wru; Sun, 13 Nov 2022 17:50:14 -0800 (PST) X-Google-Smtp-Source: AA0mqf7QzrY1STLIeMHo8g8THxsfuJ+QwAft2oSPoqdR6HChysuJ7xcFuwWgNEaKksgNomw+zI8y X-Received: by 2002:a05:6402:2070:b0:461:8692:2787 with SMTP id bd16-20020a056402207000b0046186922787mr9584357edb.199.1668390614741; Sun, 13 Nov 2022 17:50:14 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668390614; cv=none; d=google.com; s=arc-20160816; b=EDdIqJU4XRhkHdA6/XICsDE14jDtpyLWTaX6U+45CcWkwsKgYqK37D43ab44mNjJ2V ntPnuDMEClU9LXm88f5/iaT5vG2KcOaPO6z9zE3rf27CzyV50dJUH+e7U01huUO5sEc/ /OJgfnuXf4aNveEMtFkD1TzNpQh27eH2EMlRFtCwajRei8zcpHbz0ALFi+CDZS2FWLV4 3QCzdCcQ3x4kEVnCJQXFPMCe5IL50KWdIuPLVcrj5biXHQQWH5cTTCZPhjAMTvr7skjI avi+f9NKxfy+nZUzb/s2L3hxEmwt0MbVjaJRZtvYUI7b0EyJgXKkAd+bQLWmD9hg9sOq Lv8Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=+LQJgBsi2FNAsgMuUHEg0heW1WdgpMLtfw/b/XVzgho=; b=pvoXeCFUjYZtiOFpMLmfJl6SbwSfR5ZCXNdvCzEUwMg4mhfrR2crPEr/iEZiLoatnz ligLaZ493hc9zQmQUI2SNyAZJnOaie4kj9NdM+BRR+pm/l2rzWl+bxglY2SfzDkcaKIb lctRkDf6vN8SyFB+7pJ5k4+V0EQksgitie3ElCylkTfv8GjQFqvt0y/HF+Ao1lShfwhC PZLlE7WoXXYylK1ypT1vUi9bJdLaoy8g0nJg71KwZhjCWa6cY7T3PbshWgE9LcgT/Ud+ HJVHVOjm9SmgIP6oCL0NgVaygPEr+8IPsgpCtXCuYzD4cWUzM6GVg28xTjygLM1HPl9a +3jw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=hAOb7q5i; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id i37-20020a0564020f2500b004589ea2d983si7747100eda.287.2022.11.13.17.49.51; Sun, 13 Nov 2022 17:50:14 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=hAOb7q5i; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235600AbiKNBsQ (ORCPT <rfc822;winker.wchi@gmail.com> + 99 others); Sun, 13 Nov 2022 20:48:16 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53298 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235706AbiKNBsC (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sun, 13 Nov 2022 20:48:02 -0500 Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 26540DFC3 for <linux-kernel@vger.kernel.org>; Sun, 13 Nov 2022 17:47:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1668390477; x=1699926477; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=2i7NS4jJ8/ISC5Wd85I3fce+lIQh/XAYJASCa5qbM8k=; b=hAOb7q5i/q19ZW8Bw+QvnCjUHXbaDEY1kmdPySjhyJJTzZ14MEslADqM 0jUAU0KauyAmHABR/IiG6bza46v9nnrqkE1kMEQFBKwn/4R5MjKTukJV8 sPi4RF8ZFT00oUs9GDfviULJ8MXTZTGVIPr8p345MXhbR0xEIwW1F1zRE UIHb+34PjMkEOsTpXrokhgrIGYs8VgJsyQhAwaRkcaWqAG9BZcIkcHyla s7aR+okYyfuCtka7pcPS5GBj7FPD9cJwF1cN6TluGpKV+W+tSuqkMUv0J cmkUGAt9rJbIeD9T6bGd4aMF9IBJcjOpwRwI1FU5pWGa5+Iw46YY90/b3 Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10530"; a="313006690" X-IronPort-AV: E=Sophos;i="5.96,161,1665471600"; d="scan'208";a="313006690" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Nov 2022 17:47:56 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10530"; a="707124225" X-IronPort-AV: E=Sophos;i="5.96,161,1665471600"; d="scan'208";a="707124225" Received: from allen-box.sh.intel.com ([10.239.159.48]) by fmsmga004.fm.intel.com with ESMTP; 13 Nov 2022 17:47:54 -0800 From: Lu Baolu <baolu.lu@linux.intel.com> To: iommu@lists.linux.dev Cc: Joerg Roedel <joro@8bytes.org>, Kevin Tian <kevin.tian@intel.com>, Will Deacon <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>, Liu Yi L <yi.l.liu@intel.com>, Jacob jun Pan <jacob.jun.pan@intel.com>, linux-kernel@vger.kernel.org, Lu Baolu <baolu.lu@linux.intel.com> Subject: [PATCH v3 4/7] iommu/vt-d: Fold dmar_remove_one_dev_info() into its caller Date: Mon, 14 Nov 2022 09:40:46 +0800 Message-Id: <20221114014049.3959-5-baolu.lu@linux.intel.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20221114014049.3959-1-baolu.lu@linux.intel.com> References: <20221114014049.3959-1-baolu.lu@linux.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1749434357080938620?= X-GMAIL-MSGID: =?utf-8?q?1749434357080938620?= |
Series |
iommu/vt-d: Some cleanups
|
|
Commit Message
Baolu Lu
Nov. 14, 2022, 1:40 a.m. UTC
Fold dmar_remove_one_dev_info() into intel_iommu_release_device() which
is its only caller. Replace most of the code with
device_block_translation() to make the code neat and tidy.
Rename iommu_disable_dev_iotlb() to iommu_disable_pci_caps() to pair with
iommu_enable_pci_caps().
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/intel/iommu.c | 33 ++++++---------------------------
1 file changed, 6 insertions(+), 27 deletions(-)
Comments
> From: Lu Baolu <baolu.lu@linux.intel.com> > Sent: Monday, November 14, 2022 9:41 AM > @@ -4562,7 +4538,10 @@ static void intel_iommu_release_device(struct > device *dev) > { > struct device_domain_info *info = dev_iommu_priv_get(dev); > > - dmar_remove_one_dev_info(dev); > + iommu_disable_pci_caps(info); > + domain_context_clear(info); > + device_block_translation(dev); clear context after blocking translation. with that, Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On 11/16/22 11:53 AM, Tian, Kevin wrote: >> From: Lu Baolu<baolu.lu@linux.intel.com> >> Sent: Monday, November 14, 2022 9:41 AM >> @@ -4562,7 +4538,10 @@ static void intel_iommu_release_device(struct >> device *dev) >> { >> struct device_domain_info *info = dev_iommu_priv_get(dev); >> >> - dmar_remove_one_dev_info(dev); >> + iommu_disable_pci_caps(info); >> + domain_context_clear(info); >> + device_block_translation(dev); > clear context after blocking translation. Unfortunately domain_context_clear() needs reference to info->domain (for domain id when flushing cache), which is cleared in device_block_translation(). Best regards, baolu
> From: Baolu Lu <baolu.lu@linux.intel.com> > Sent: Wednesday, November 16, 2022 12:36 PM > > On 11/16/22 11:53 AM, Tian, Kevin wrote: > >> From: Lu Baolu<baolu.lu@linux.intel.com> > >> Sent: Monday, November 14, 2022 9:41 AM > >> @@ -4562,7 +4538,10 @@ static void intel_iommu_release_device(struct > >> device *dev) > >> { > >> struct device_domain_info *info = dev_iommu_priv_get(dev); > >> > >> - dmar_remove_one_dev_info(dev); > >> + iommu_disable_pci_caps(info); > >> + domain_context_clear(info); > >> + device_block_translation(dev); > > clear context after blocking translation. > > Unfortunately domain_context_clear() needs reference to info->domain > (for domain id when flushing cache), which is cleared in > device_block_translation(). > this sounds an ordering problem. clearing context should be after blocking translation in concept.
On 2022/11/16 13:35, Tian, Kevin wrote: >> From: Baolu Lu<baolu.lu@linux.intel.com> >> Sent: Wednesday, November 16, 2022 12:36 PM >> >> On 11/16/22 11:53 AM, Tian, Kevin wrote: >>>> From: Lu Baolu<baolu.lu@linux.intel.com> >>>> Sent: Monday, November 14, 2022 9:41 AM >>>> @@ -4562,7 +4538,10 @@ static void intel_iommu_release_device(struct >>>> device *dev) >>>> { >>>> struct device_domain_info *info = dev_iommu_priv_get(dev); >>>> >>>> - dmar_remove_one_dev_info(dev); >>>> + iommu_disable_pci_caps(info); >>>> + domain_context_clear(info); >>>> + device_block_translation(dev); >>> clear context after blocking translation. >> Unfortunately domain_context_clear() needs reference to info->domain >> (for domain id when flushing cache), which is cleared in >> device_block_translation(). >> > this sounds an ordering problem. clearing context should be after > blocking translation in concept. At present, when the default domain is attached to the device, we first populate the pasid table entry, and then populate the device context entry. Above code is just the reverse operation. Can you see any practical problems caused by this sequence? If so, it seems that we should carefully consider whether such problems already exist. Best regards, baolu
> From: Baolu Lu <baolu.lu@linux.intel.com> > Sent: Wednesday, November 16, 2022 4:03 PM > > On 2022/11/16 13:35, Tian, Kevin wrote: > >> From: Baolu Lu<baolu.lu@linux.intel.com> > >> Sent: Wednesday, November 16, 2022 12:36 PM > >> > >> On 11/16/22 11:53 AM, Tian, Kevin wrote: > >>>> From: Lu Baolu<baolu.lu@linux.intel.com> > >>>> Sent: Monday, November 14, 2022 9:41 AM > >>>> @@ -4562,7 +4538,10 @@ static void > intel_iommu_release_device(struct > >>>> device *dev) > >>>> { > >>>> struct device_domain_info *info = dev_iommu_priv_get(dev); > >>>> > >>>> - dmar_remove_one_dev_info(dev); > >>>> + iommu_disable_pci_caps(info); > >>>> + domain_context_clear(info); > >>>> + device_block_translation(dev); > >>> clear context after blocking translation. > >> Unfortunately domain_context_clear() needs reference to info->domain > >> (for domain id when flushing cache), which is cleared in > >> device_block_translation(). > >> > > this sounds an ordering problem. clearing context should be after > > blocking translation in concept. > > At present, when the default domain is attached to the device, we first > populate the pasid table entry, and then populate the device context > entry. Above code is just the reverse operation. > > Can you see any practical problems caused by this sequence? If so, it > seems that we should carefully consider whether such problems already > exist. > there is no problem with existing code. Just after this patch the order looks weird based on the literal name of those functions. domain_context_clear() is a big hammer to disable the context entry, implying translation must be blocked. Then calling another block translation afterwards becomes unnecessary. Probably it should be split into two functions with one requiring info->domain called before block translation and the rest which actually clears the context entry being the last step?
On 2022/11/16 17:15, Tian, Kevin wrote: >> From: Baolu Lu <baolu.lu@linux.intel.com> >> Sent: Wednesday, November 16, 2022 4:03 PM >> >> On 2022/11/16 13:35, Tian, Kevin wrote: >>>> From: Baolu Lu<baolu.lu@linux.intel.com> >>>> Sent: Wednesday, November 16, 2022 12:36 PM >>>> >>>> On 11/16/22 11:53 AM, Tian, Kevin wrote: >>>>>> From: Lu Baolu<baolu.lu@linux.intel.com> >>>>>> Sent: Monday, November 14, 2022 9:41 AM >>>>>> @@ -4562,7 +4538,10 @@ static void >> intel_iommu_release_device(struct >>>>>> device *dev) >>>>>> { >>>>>> struct device_domain_info *info = dev_iommu_priv_get(dev); >>>>>> >>>>>> - dmar_remove_one_dev_info(dev); >>>>>> + iommu_disable_pci_caps(info); >>>>>> + domain_context_clear(info); >>>>>> + device_block_translation(dev); >>>>> clear context after blocking translation. >>>> Unfortunately domain_context_clear() needs reference to info->domain >>>> (for domain id when flushing cache), which is cleared in >>>> device_block_translation(). >>>> >>> this sounds an ordering problem. clearing context should be after >>> blocking translation in concept. >> >> At present, when the default domain is attached to the device, we first >> populate the pasid table entry, and then populate the device context >> entry. Above code is just the reverse operation. >> >> Can you see any practical problems caused by this sequence? If so, it >> seems that we should carefully consider whether such problems already >> exist. >> > > there is no problem with existing code. Just after this patch the order > looks weird based on the literal name of those functions. > > domain_context_clear() is a big hammer to disable the context entry, > implying translation must be blocked. Then calling another block > translation afterwards becomes unnecessary. > > Probably it should be split into two functions with one requiring > info->domain called before block translation and the rest which > actually clears the context entry being the last step? This is what the existing code does. Perhaps I should drop this patch, or only rename iommu_disable_dev_iotlb() to iommu_disable_pci_caps(). Best regards, baolu
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 3d3fa182fcb1..f165e0d37bab 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -1443,7 +1443,7 @@ static void iommu_enable_pci_caps(struct device_domain_info *info) } } -static void iommu_disable_dev_iotlb(struct device_domain_info *info) +static void iommu_disable_pci_caps(struct device_domain_info *info) { struct pci_dev *pdev; @@ -4089,30 +4089,6 @@ static void domain_context_clear(struct device_domain_info *info) &domain_context_clear_one_cb, info); } -static void dmar_remove_one_dev_info(struct device *dev) -{ - struct device_domain_info *info = dev_iommu_priv_get(dev); - struct dmar_domain *domain = info->domain; - struct intel_iommu *iommu = info->iommu; - unsigned long flags; - - if (!dev_is_real_dma_subdevice(info->dev)) { - if (dev_is_pci(info->dev) && sm_supported(iommu)) - intel_pasid_tear_down_entry(iommu, info->dev, - PASID_RID2PASID, false); - - iommu_disable_dev_iotlb(info); - domain_context_clear(info); - } - - spin_lock_irqsave(&domain->lock, flags); - list_del(&info->link); - spin_unlock_irqrestore(&domain->lock, flags); - - domain_detach_iommu(domain, iommu); - info->domain = NULL; -} - /* * Clear the page table pointer in context or pasid table entries so that * all DMA requests without PASID from the device are blocked. If the page @@ -4124,7 +4100,7 @@ static void device_block_translation(struct device *dev) struct intel_iommu *iommu = info->iommu; unsigned long flags; - iommu_disable_dev_iotlb(info); + iommu_disable_pci_caps(info); if (!dev_is_real_dma_subdevice(dev)) { if (sm_supported(iommu)) intel_pasid_tear_down_entry(iommu, dev, @@ -4562,7 +4538,10 @@ static void intel_iommu_release_device(struct device *dev) { struct device_domain_info *info = dev_iommu_priv_get(dev); - dmar_remove_one_dev_info(dev); + iommu_disable_pci_caps(info); + domain_context_clear(info); + device_block_translation(dev); + intel_pasid_free_table(dev); dev_iommu_priv_set(dev, NULL); kfree(info);