Received-SPF: pass (google.com: domain of linux-kernel+bounces-138129-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <0231631b-44ca-45ee-adf9-0a5c8852cc27@linux.intel.com>
Date: Wed, 10 Apr 2024 16:02:24 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: baolu.lu@linux.intel.com, 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
To: Yi Liu <yi.l.liu@intel.com>, iommu@lists.linux.dev
References: <20240410055823.264501-1-baolu.lu@linux.intel.com>
 <7e78917f-f84c-4e98-a612-73b8013ae367@intel.com>
Content-Language: en-US
From: Baolu Lu <baolu.lu@linux.intel.com>
In-Reply-To: <7e78917f-f84c-4e98-a612-73b8013ae367@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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.

Best regards,
baolu