From: Ethan Zhao <haifeng.zhao@linux.intel.com>
To: Baolu Lu <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 14:30:38 +0800 [thread overview]
Message-ID: <916dbee7-5ab1-4def-91fe-546e6ab974de@linux.intel.com> (raw)
In-Reply-To: <da521760-c2d7-432e-8ace-8793d520ab09@linux.intel.com>
On 4/16/2024 10:57 AM, Baolu Lu wrote:
> On 4/16/24 8:53 AM, Ethan Zhao wrote:
>> 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() ?
>
> I am refactoring the code with a new series.
>
> https://lore.kernel.org/linux-iommu/20240410020844.253535-1-baolu.lu@linux.intel.com/
>
Great, thx.
>
> Best regards,
> baolu
next prev parent reply other threads:[~2024-04-16 6:30 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
2024-04-16 2:57 ` Baolu Lu
2024-04-16 6:30 ` Ethan Zhao [this message]
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=916dbee7-5ab1-4def-91fe-546e6ab974de@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.