Received-SPF: pass (google.com: domain of linux-kernel+bounces-137984-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <7e78917f-f84c-4e98-a612-73b8013ae367@intel.com>
Date: Wed, 10 Apr 2024 14:30:20 +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: Lu Baolu <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>
From: Yi Liu <yi.l.liu@intel.com>
In-Reply-To: <20240410055823.264501-1-baolu.lu@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bStCb0RDUjdWSTNEQ1UyKzZJR3RSR2Z2Q1gvbVBTZDhGS2dmRmE5Znh2S05u?=
 =?utf-8?B?eHlEUFl3VGxsc2JFV1hsaWE1ZnlzOTFYM29kMUdwR0QwdUh1RW5TSFV3d2ZN?=
 =?utf-8?B?c3VvbzhXZ2FDd29XcHNSRk5TRlFMOWl5YVlyMmlKWGdkYnJoNzRWUXRwYVd4?=
 =?utf-8?B?UFpMUkVRL1ljMnowYWdaUnVVSFNabGlQT3ZiYkoxc0xOcjdxRDF6NDdpOXFC?=
 =?utf-8?B?WnYvU09MaFV0aEZWcjU5eUZ4eGtIQkpaTG5pMnpBTzhXdGZxWlQ2eXZxNFNF?=
 =?utf-8?B?REdWRk9PbTdPMkw2TGFsd0VHM25yWFdMR1NuZk42VHowNjRMT1VhbW94ZSt5?=
 =?utf-8?B?QURxSC9NM3lJR3pUSTlhV3dvWk5XSFRkWGZHb0p1dm5iV3lMOXR3d1BMb3F4?=
 =?utf-8?B?ZGdCM2hXdlhWWndQSTJtNGliYUNXUmpwY0hlZ1ppakZPbXowNVQxb015M0Zm?=
 =?utf-8?B?eTViTnNaYUJyYzhDTjcxTEg2elh3L21QekxBTWdjUVZQeG5oK09Bd255Tlhh?=
 =?utf-8?B?UTVVZFFFSUlSNTFWa2I5V2VwVGJxbi9DZmFXMmlQbTBqNGw3a0p0TXFEZVov?=
 =?utf-8?B?ZGZyZC9Rdm1QR05mbVYzbmdLNmxhOStNNGUzdTVVTmtMSnhWT05jM1pTODdD?=
 =?utf-8?B?eDhmZFNrUmR1MGdHQXlhaGIxeDRPNHNpTFcvYnA2MVRjTFE0RndVTEQ3ZDAw?=
 =?utf-8?B?bFJ1dHk5eXFyVjhMYTRrMTF6azRzNld2WXVlUkM2SjdIakRiYXpValF5bFF4?=
 =?utf-8?B?dkd4eU1xRG9WSE0zRGpnVWJHaUZXR3FXWTFxcmxJemZFbk9iUzMycU9FNjdz?=
 =?utf-8?B?ZjlwL0JaSzZ4NlFrMklNSHJQTmZBRXFrQU1ISS9qaUgzSTNVbzRPOVByTDJi?=
 =?utf-8?B?Tno2by8vY2crSUVVbFJVTEpTNkt5c0pxQlEyMCt2Uk5MTFdodERKaTc1OVZQ?=
 =?utf-8?B?b1lqR1JRdndueThBbkY0QzVhaU9pWHZHQTQ3VDZTN1hIamZOOU1rVFo0RmlG?=
 =?utf-8?B?eUFTTktJSURiV0ZKMDNmditYTDNDRXNwMHJOOXRKb2tjL2JDWkZGZU1CQ1RO?=
 =?utf-8?B?REo2OURPVmZSUU5oNEZBeVZmNTZTU0xieDJuWTlZRVFCdGJNUkJyMzd2WTc4?=
 =?utf-8?B?a0FXUXJ2U2JEazAyTnNLK3VBYlRNZy9UcnB1ajg4VGVUY3NLcUtaL0M2VTA0?=
 =?utf-8?B?SmlVaHZvREdzR1R0M2U4dWhUdzZKbHRieWp0SWJFcGZCbWNaQW1naDRVRm53?=
 =?utf-8?B?aldQU1pYdXRqTVFxVVFxU2NxTHhCeWQ2OCtrVnFkM3l4U3ZwR3hrL1V5RWJ6?=
 =?utf-8?B?K0xvRnE5WEQ1MEhrb2w2Q0ZhU1VsaUNQWXlNQXZabStWdzdWNjg1QXdRRWRY?=
 =?utf-8?B?a3o3RHdtWVFiSGdLNndmZjVxcENmSUxaSW1paDR0QU9tVVdVM0g0clEwUEdy?=
 =?utf-8?B?R1g3NmloS09FSmpXRXcxSmtSeFhDRWhjalE3YUlNSG82b1lVRElhcHczY2FO?=
 =?utf-8?B?dk5QNGlQaG1XWW1XUDd0Q3VMWHU4a0VCYy96ZFBMQjIrS21NY3lOdzVZdHZE?=
 =?utf-8?B?U0dEaXpIbnl5U2Rmd3NNS29mMkkrN2FuUDh2ZzdpUmlXMk9CcnNlekVVWkg2?=
 =?utf-8?B?aFNVa2t2SmNPd3lmRDBjeFhvYWJpYk45azVLTm44RDZCRVJ0YVYzWjNQZEFF?=
 =?utf-8?B?cW9SajFEWWRtRDgrTENjdy9aRTgxNVVla1lQbUlCampkQ2dyT3NUdWVzaHVZ?=
 =?utf-8?B?Qnp5UHNyTXliTE5VWVhkL3V4ZDBOc3hVSFpET2R0c3VtOUVBaVg3cXJYK0o1?=
 =?utf-8?B?T05Lbm80VkVDUzB6S01kNWJ6YU5QMmliaWFyaWtNM1ZoUUlsdzdUZEtCMG9Z?=
 =?utf-8?B?SHdyR0FGY2ZmNzZ3Y1YxSWw2bGJEMGhkTHZISW5oVWhiZERGczNUOUpyNWpM?=
 =?utf-8?B?aEZsTTlPMGtvZlo0aGR2TU5aSVI2WHN0UzZHbHluQnJ5Z3hSUTM1WkNrTFY0?=
 =?utf-8?B?bEFLVytGRkRxV2lUWFlFd1FIL2dJVWFiMUdybTZ6TlpTVjk3WFBJSUEvYVVm?=
 =?utf-8?B?bGN4em5GSEpDRE9WK05ibkhwNlhJZ2t0aE4zOFhNZTZhd21JcDJxdXlQUDB3?=
 =?utf-8?Q?iVDqyu3hhMTOEF0xHnaq8npmK?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1975f6e8-59f3-4cc3-098f-08dc59273382
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB7529.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Apr 2024 06:26:48.3980
 (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: ZWQ2d3xfvt/qM6HGH5xHOym+0FkxFolkEMA/mpdjN13YACuZSFL/IqVRjxfHL5y2c4ZhRoOJGntqWIFnnXDWvw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6545
X-OriginatorOrg: intel.com

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. :)

[1] 
https://lore.kernel.org/linux-iommu/ac7fdf0f-ecb9-415f-9c1b-600536b92ca5@intel.com/

> The removal of caching mode check in intel_flush_iotlb_all() doesn't
> impact anything no matter the IOMMU is working in caching mode or not.
> Commit <29b32839725f> ("iommu/vt-d: Do not use flush-queue when
> caching-mode is on") has already disabled flush-queue for caching mode,
> hence caching mode will never call intel_flush_iotlb_all().
> 
> Fixes: bf92df30df90 ("intel-iommu: Only avoid flushing device IOTLB for domain ID 0 in caching mode")
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)
> 
> Change log:
> 
> v2:
>   - 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 50eb9aed47cc..681789b1258d 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);
>   }
>   
> @@ -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);

why not one more step to move it out of the iommu_array loop? There is
no more reason to keep it in the loop.

>   	}
>   
>   	if (dmar_domain->nested_parent)

-- 
Regards,
Yi Liu