From: Ethan Zhao <haifeng.zhao@linux.intel.com>
To: Lu Baolu <baolu.lu@linux.intel.com>, iommu@lists.linux.dev
Cc: Kevin Tian <kevin.tian@intel.com>, Yi Liu <yi.l.liu@intel.com>,
Jacob Pan <jacob.jun.pan@linux.intel.com>,
Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
Robin Murphy <robin.murphy@arm.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush
Date: Tue, 16 Apr 2024 08:53:34 +0800 [thread overview]
Message-ID: <9ba0b6a2-e6c1-46de-8d30-929567952c54@linux.intel.com> (raw)
In-Reply-To: <20240415013835.9527-1-baolu.lu@linux.intel.com>
On 4/15/2024 9:38 AM, Lu Baolu wrote:
> The Caching Mode (CM) of the Intel IOMMU indicates if the hardware
> implementation caches not-present or erroneous translation-structure
> entries except for the first-stage translation. The caching mode is
> irrelevant to the device TLB, therefore there is no need to check it
> before a device TLB invalidation operation.
>
> Remove two caching mode checks before device TLB invalidation in the
> driver. The removal of these checks doesn't change the driver's behavior
> in critical map/unmap paths. Hence, there is no functionality or
> performance impact, especially since commit <29b32839725f> ("iommu/vt-d:
> Do not use flush-queue when caching-mode is on") has already disabled
> flush-queue for caching mode. Therefore, caching mode will never call
> intel_flush_iotlb_all().
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
> drivers/iommu/intel/iommu.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> Change log:
> v3:
> - It turned out that the removals don't change the driver's behavior,
> hence change it from a fix patch to a cleanup one.
> - No functionality changes.
> v2: https://lore.kernel.org/lkml/20240410055823.264501-1-baolu.lu@linux.intel.com/
> - Squash two patches into a single one.
> - No functionality changes.
> v1: https://lore.kernel.org/linux-iommu/20240407144232.190355-1-baolu.lu@linux.intel.com/
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index a7ecd90303dc..f0a67e9d9faf 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1501,11 +1501,7 @@ static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
> else
> __iommu_flush_iotlb_psi(iommu, did, pfn, pages, ih);
>
> - /*
> - * In caching mode, changes of pages from non-present to present require
> - * flush. However, device IOTLB doesn't need to be flushed in this case.
> - */
> - if (!cap_caching_mode(iommu->cap) || !map)
> + if (!map)
> iommu_flush_dev_iotlb(domain, addr, mask);
> }
Given devTLB flushing is irrelavent to CM, put iommu_flush_dev_iotlb()
in iommu_flush_iotlb_psi() and called with CM checking context is not
reasonable. the logic is buggy.
static void __mapping_notify_one(struct intel_iommu *iommu, struct dmar_domain *domain,
unsigned long pfn, unsigned int pages)
{
/*
* It's a non-present to present mapping. Only flush if caching mode
* and second level.
*/
if (cap_caching_mode(iommu->cap) && !domain->use_first_level)
iommu_flush_iotlb_psi(iommu, domain, pfn, pages, 0, 1);
else
iommu_flush_write_buffer(iommu);
then how about fold all CM checking logic in iommu_flush_iotlb_psi()
or speperate iommu_flush_dev_iotlb() from iommu_flush_iotlb_psi() ?
Thanks,
Ethan
>
> @@ -1579,8 +1575,7 @@ static void intel_flush_iotlb_all(struct iommu_domain *domain)
> iommu->flush.flush_iotlb(iommu, did, 0, 0,
> DMA_TLB_DSI_FLUSH);
>
> - if (!cap_caching_mode(iommu->cap))
> - iommu_flush_dev_iotlb(dmar_domain, 0, MAX_AGAW_PFN_WIDTH);
> + iommu_flush_dev_iotlb(dmar_domain, 0, MAX_AGAW_PFN_WIDTH);
> }
>
> if (dmar_domain->nested_parent)
next prev parent reply other threads:[~2024-04-16 0:53 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-15 1:38 [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush Lu Baolu
2024-04-15 2:22 ` Zhang, Tina
2024-04-15 2:43 ` Yi Liu
2024-04-15 5:00 ` Baolu Lu
2024-04-15 6:56 ` Yi Liu
2024-04-16 3:00 ` Baolu Lu
2024-04-17 3:40 ` Yi Liu
2024-04-16 0:53 ` Ethan Zhao [this message]
2024-04-16 2:57 ` Baolu Lu
2024-04-16 6:30 ` Ethan Zhao
2024-04-24 3:41 ` Baolu Lu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9ba0b6a2-e6c1-46de-8d30-929567952c54@linux.intel.com \
--to=haifeng.zhao@linux.intel.com \
--cc=baolu.lu@linux.intel.com \
--cc=iommu@lists.linux.dev \
--cc=jacob.jun.pan@linux.intel.com \
--cc=joro@8bytes.org \
--cc=kevin.tian@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=robin.murphy@arm.com \
--cc=will@kernel.org \
--cc=yi.l.liu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.