From: Yi Liu <yi.l.liu@intel.com>
To: Baolu Lu <baolu.lu@linux.intel.com>, <iommu@lists.linux.dev>
Cc: Kevin Tian <kevin.tian@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 v2 1/1] iommu/vt-d: Remove caching mode check before device TLB flush
Date: Fri, 12 Apr 2024 17:34:42 +0800 [thread overview]
Message-ID: <5e17af59-e544-4c5e-a718-2cd77b8a07a4@intel.com> (raw)
In-Reply-To: <776ef2f1-747d-46f0-94be-747c6fca8ce0@linux.intel.com>
On 2024/4/10 18:38, Baolu Lu wrote:
> On 2024/4/10 17:14, Yi Liu wrote:
>>
>>
>> On 2024/4/10 16:02, Baolu Lu wrote:
>>> On 2024/4/10 14:30, Yi Liu wrote:
>>>> On 2024/4/10 13:58, 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 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.
>>>>>
>>>>> iommu_flush_iotlb_psi() is called in map and unmap paths. The caching
>>>>> mode check before device TLB invalidation will cause device TLB
>>>>> invalidation always issued if IOMMU is not running in caching mode.
>>>>> This is wrong and causes unnecessary performance overhead.
>>>>
>>>> I don't think the original code is wrong. As I replied before, if CM==0,
>>>> the iommu_flush_iotlb_psi() is only called in unmap path, in which the
>>>> @map is false. [1] The reason to make the change is to make the logic
>>>> simpler. 🙂
>>>
>>> Oh, I see. There is a magic
>>>
>>> if (cap_caching_mode(iommu->cap) && !domain->use_first_level)
>>> iommu_flush_iotlb_psi(iommu, domain, pfn, pages, 0, 1);
>>>
>>> in __mapping_notify_one().
>>>
>>> So if it's caching mode, then
>>>
>>> - iommu_flush_iotlb_psi() will be called with @map=1 from
>>> __mapping_notify_one(), "!cap_caching_mode(iommu->cap) || !map" is
>>> not true, and device TLB is not invalidated.
>>> - iommu_flush_iotlb_psi() will also be called with @map=0 from
>>> intel_iommu_tlb_sync(), device TLB is issued there.
>>>
>>> That's the expected behavior for caching mode.
>>>
>>> If it's not the caching mode, then
>>>
>>> - iommu_flush_iotlb_psi() will be called with @map=0 from
>>> intel_iommu_tlb_sync(), device TLB is issued there.
>>>
>>> That's also the expected behavior.
>>>
>>> So the existing code is correct but obscure and difficult to understand,
>>> right? If so, we should make this patch as a cleanup rather than a fix.
>>
>> aha, yes. As the below table, iommu_flush_iotlb_psi() does flush device TLB
>> as expected. But there is a NA case. When CM==0, it should not be possible
>> to call iommu_flush_iotlb_psi() with @map==1 as cache invalidation is not
>> required when CM==0. So the existing code logic is really confusing,
>> checking @map is enough and clearer. Since the old code works, so perhaps
>> no fix tag is needed. :)
>>
>> +----+------+-----------+------------+
>> | \ | | |
>> | \ @map | | |
>> | CM \ | 0 | 1 |
>> | \ | | |
>> +------+---+------------+------------+
>> | | | |
>> | 0 | Y | NA |
>> +----------+------------+------------+
>> | | | |
>> | 1 | Y | N |
>> +----------+------------+------------+
>>
>> Y means flush dev-TLB please
>> N means no need to flush dev-TLB
>> NA means not applied
>
> Yes. We have the same understanding now. :-)
yeah, would be helpful to refine the commit message w.r.t. this change.
>>
>> BTW. I think it is better to have the below change in a separate patch.
>> The below change does fix a improper dev-TLB flushing behavior. Also
>> how about Kevin's concern in the end of [1]. I didn't see your respond
>> about it.
>
> I had an offline discussion with him and I included the conclusion in
> the commit message of this patch.
ok. So the key reason is that intel_flush_iotlb_all() is not called at
all if CM==1 after that commit. So there is nothing fixed by this change.
right? This just makes the logic cleaner. Perhaps no need for a fix tag.
Reviewed-by: Yi Liu <yi.l.liu@intel.com>
--
Regards,
Yi Liu
next prev parent reply other threads:[~2024-04-12 9:31 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-10 5:58 [PATCH v2 1/1] iommu/vt-d: Remove caching mode check before device TLB flush Lu Baolu
2024-04-10 6:14 ` Tian, Kevin
2024-04-10 6:30 ` Yi Liu
2024-04-10 8:02 ` Baolu Lu
2024-04-10 9:14 ` Yi Liu
2024-04-10 10:38 ` Baolu Lu
2024-04-12 9:34 ` Yi Liu [this message]
2024-04-11 13:13 ` Robin Murphy
2024-04-11 13:48 ` 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=5e17af59-e544-4c5e-a718-2cd77b8a07a4@intel.com \
--to=yi.l.liu@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 \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).