Received-SPF: pass (google.com: domain of linux-kernel+bounces-142415-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <5e17af59-e544-4c5e-a718-2cd77b8a07a4@intel.com>
Date: Fri, 12 Apr 2024 17:34:42 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/1] iommu/vt-d: Remove caching mode check before
 device TLB flush
Content-Language: en-US
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>
References: <20240410055823.264501-1-baolu.lu@linux.intel.com>
 <7e78917f-f84c-4e98-a612-73b8013ae367@intel.com>
 <0231631b-44ca-45ee-adf9-0a5c8852cc27@linux.intel.com>
 <be0b254d-d6c0-4a94-8234-936f40538bbc@intel.com>
 <776ef2f1-747d-46f0-94be-747c6fca8ce0@linux.intel.com>
From: Yi Liu <yi.l.liu@intel.com>
In-Reply-To: <776ef2f1-747d-46f0-94be-747c6fca8ce0@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MytVVnZWKzZGaDVPQ0NDS2l5ak95U1prUDRMVzdzU0ZOTXJkb3lwTDROU2M0?=
 =?utf-8?B?N1hybm9SZHlSajdrdU5OemdhR3pFYXRySHdLbGxTUU9RTmZycWwyV2RMZG5H?=
 =?utf-8?B?UEt0T0xEaDQ3OE1RU1dRU3owenROMERKSFBzTTVLeHFUdVlKY3lWaWUvQk9K?=
 =?utf-8?B?T1NmZG0zYmQ2KzVIMWJtWFRTS0NldUkrRmtza0xUZktBeThZbGE1Z1BlQUt1?=
 =?utf-8?B?TzhVZkhGbHlURFJPYUZOUnRScTBHNWd3ZG52S3F1YWVXeVNsbDJzY0w1cjlN?=
 =?utf-8?B?NzU3blAwMmdsQlBKVllrUjN0ZlYySHcyTUhoWXM4WFFpcllPN1NnREd2eW5i?=
 =?utf-8?B?d29pQ3V5L0pYbmV6OW5RWVloTG05VVRjMkJjeGpscW11d3Z1VjlCZUpqZDBo?=
 =?utf-8?B?QnkxMXNqNmpGZVRENGxiYVZ3MVlMMVIwaG43dW9sNnpmL0h2eGkrTHJDYWo2?=
 =?utf-8?B?UmtXcFR0QzZZczhFUm1jVGZJM2RRM1llS3JkZEhtRGVmQUNRNXlRVVVLT2Ny?=
 =?utf-8?B?OExvWEtpakFWUjZDa1dHV0pveXEwMDBXV2lybDhncmwrbXNKWEo0NnB2bGZU?=
 =?utf-8?B?ZW1DMHA3MVM1Wk4rZkpLVVdPVlFTQ3FsVzFXbVpDQnU2MU44WlNWRHVhaW9Y?=
 =?utf-8?B?dDBSTnlvMFhXUUFXQ1FEaUdiNUhWbVIxWkw2dzhmK1pGdlJKQmRzVlQrUmhK?=
 =?utf-8?B?cEw0eFVuWFVhTXdKVFdOa1Y1QmYzMDBMZGs1UlQ5U2RoTDhRVDhtVHVFOWxt?=
 =?utf-8?B?MDUxRk1vZVlRdTZsbzZyeFliYVFBN0FUZUVCRk5zTHQ0T0JPVjEyejNqVUdQ?=
 =?utf-8?B?OXNxQlRHZU5sb3NhV0M3WUo0T09KejlxaUpOdUxCd2RwUmVvdmE5U3dHcGNu?=
 =?utf-8?B?VURLQkJDbTJxNHN4VGdqQTk0aWJZQXp1UVF3alYwVmo0SDY3SmpISTJSVWFr?=
 =?utf-8?B?WXNpaDBtMmcvZkkvVExGLzIrS0U4YTloT0ZxZ3Z0dnJLcWVlbTQveVJiMXlP?=
 =?utf-8?B?dVljUGY5RTVINUorSmRKOVVjM1NtYUJ0WnVYdXdrME5ZcXhZR0VCUUdydldF?=
 =?utf-8?B?aVJzc0QvV1E1cmNWV0JKY0VTanEzdzlyZWVFeGVYcWdkM05UczZXcTZPWFJh?=
 =?utf-8?B?ZTFYdmhTaVdwbHFORFlYaSs3aWtqM2R0c3RBcEJnZENHYTNuTHY2UGNlSlNY?=
 =?utf-8?B?ZUdsU0UvYjkxNFovVTUyamRObVg5MmNmV0ZreW1IQXU3Q2V2WUhOTGhJT0ZM?=
 =?utf-8?B?eENsYWRVaUtBTUx4MjNpT1F1U01kM0JPTDJJc2dXUmY4aHFxbzllKzd3SVZB?=
 =?utf-8?B?NVNrRXowVUdmZ2dkTlJma3ZLNTdvcWllc0FwRUY2b2JySEptYmo5MkJ4akg0?=
 =?utf-8?B?TkdyOUpkd1E2V3FoMFlWZStXUktyQmdOekNsZ0ExYVRRN0wyRHZNSkFDS08w?=
 =?utf-8?B?b2M3bXVJUFN1aVEzWnpBNnV3YThhWlgxY2JZYklnbFByelA1QitBczVXMnAz?=
 =?utf-8?B?a1ZIRU8zNjhtWTZnUHEzWXhqaS9aRW95N1R1ejhYNjhDRmRxSDBrTUV4bldt?=
 =?utf-8?B?b2REb2RUUEJDMGNoM0l5N0FMYW5KV1YzN1lrL09KbUowRUpsU2VRQ0RyU0RK?=
 =?utf-8?B?TkRvQ2tPSzhwZXVEQ3MyNFlsc1VGUjA1WkZWWmpCMHloQXFwYkFmNHFQb0JW?=
 =?utf-8?B?c2tMMXBoa0JPMkJFVmpnVEdrbU1ueFN5TUJFWC9YamMvNS8zc1Jra2hJVndt?=
 =?utf-8?B?b2duUHNwbjBQUDd6aUxHL05wYVJzQUcyUmNZVVozeVpGMlpHb1FVUit3T3NR?=
 =?utf-8?B?UUJWUGZpSzhyUXpiUzZIMzJ6WnF1THNWakYrMnR1WlYwaDV4QndVZXFrcW83?=
 =?utf-8?B?d0tKRlZFemlUekJoTFVPbXhrQWdoN3pWQ2tBWDhzektTeHpUQitCZkZ4UHRH?=
 =?utf-8?B?L2JMRHlGWUNQRHpoYkVjdS96UVBvY3JRbXpXeDB1UFRNK1RsS1Y0bjJhRVEr?=
 =?utf-8?B?NHpscFlPdWhpN1l5dXBZekZYaEdrUzBlcTlraFdMdEJNUkRQTHY3QkVLbzNk?=
 =?utf-8?B?NkUveVJmZ3BaWEZCRWhMMEFBQitGeFhNbzFEMGpTQTVXaThxajRUanNNWnJn?=
 =?utf-8?Q?prB2cgKgPrnaCGUzMbr2Gxxyg?=
X-MS-Exchange-CrossTenant-Network-Message-Id: dc71aa0e-765f-45e0-3205-08dc5ad3492d
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB7529.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Apr 2024 09:31:09.3153
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: BEYtSuA6qqxZfEWSHiro+d+79nX+fQzJvSCfhfs3aZWZ2hQhWA/TAXRaSCOd/ice8okkY4c8dj2wVFGmtPZUYw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB6398
X-OriginatorOrg: 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