Received-SPF: pass (google.com: domain of linux-kernel+bounces-76106-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <d6b9ccff-3aba-4508-b83e-4bc3bc859e96@intel.com>
Date: Thu, 22 Feb 2024 16:34:10 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH rc 3/8] iommu/vt-d: Add missing iotlb flush for parent
 domain
Content-Language: en-US
To: Jason Gunthorpe <jgg@nvidia.com>
CC: <joro@8bytes.org>, <kevin.tian@intel.com>, <baolu.lu@linux.intel.com>,
	<alex.williamson@redhat.com>, <robin.murphy@arm.com>,
	<eric.auger@redhat.com>, <nicolinc@nvidia.com>, <kvm@vger.kernel.org>,
	<chao.p.peng@linux.intel.com>, <yi.y.sun@linux.intel.com>,
	<iommu@lists.linux.dev>, <linux-kernel@vger.kernel.org>,
	<linux-kselftest@vger.kernel.org>, <zhenzhong.duan@intel.com>,
	<joao.m.martins@oracle.com>
References: <20240208082307.15759-1-yi.l.liu@intel.com>
 <20240208082307.15759-4-yi.l.liu@intel.com>
 <20240221151923.GU13330@nvidia.com>
From: Yi Liu <yi.l.liu@intel.com>
In-Reply-To: <20240221151923.GU13330@nvidia.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?QldNRFI0Z25WNVVuV1pseUpQVnVtNG5ra3o2eE9yV2FMcG9tTEg3MExjMHFS?=
 =?utf-8?B?T1d6ejlZOU9veTh5Z1hjdFJDd1NvOElxbVgxTWJCaTVydnBrOTR5UjdKTVlR?=
 =?utf-8?B?YkFtUkJiS205Z2xDZitJOUZVZXdyMDRESjJFY0s1d1VFMlU4QjF6WW9FOUp6?=
 =?utf-8?B?bjlpcG9NWitKSDU3cGdRZzNuTEFlTEd4djg3UG52blJZZVpxRTdhWkdNMjVZ?=
 =?utf-8?B?bWdJcFFIUWpqcmdYcVcrVk9KZXlQaXhYdU1wSXZaR2IzV2tmSGtsNXJWTnlz?=
 =?utf-8?B?aS9ab0UxR216Q3dtdmhVaHo5c3doUXgxeEU1ZnoxRTVtSmU4YzVDRG5WTmla?=
 =?utf-8?B?aUtKcUx0SHNtWWJ5Y1VBcEtDcVhld2RMZmJkZUZ2RktwRWZjTGJoV1l1dzNF?=
 =?utf-8?B?cVpZYTFBZWtFaE1sZU9uc2xMdHJMdExKYXpZSVY3TUdEdjNWOVk2R1BPSTBL?=
 =?utf-8?B?UUZaZWVVTW9RdTVSYmlEbDl1YndiTG04ZEFwZWEzRGliRTAreENmN2g3Z2cy?=
 =?utf-8?B?SWd0LzRqcjhJRE96YWp5a3o5VXkwR3pIUnZBSUpWR1pOMjFyZkFkdmZrNXlK?=
 =?utf-8?B?Vzh4QXBCemczcThwbE9zcjRlbHUzT3UrdkJyUXlPNWhuTE1DektzcFdlR1h6?=
 =?utf-8?B?SDVoUThoWFRkeFRUMFJ4dGZHMW5rYkt5eTdROGtUWWcxVTVITlhpbmFaY3ht?=
 =?utf-8?B?WFlBZCtIUit2WDBuRDlSRHRSeVZmczRaaDFXc2VlazRUNzViaUo4TkQwcmp5?=
 =?utf-8?B?ZnBwb08vNFdWOVpEYTRLM3pGdnIrU1BDNHBGSVN4MkgwRWVwUjNld0gyWjB1?=
 =?utf-8?B?RnQ0Mkt4QTZlS1QwR3F1MGJUMDdvVitWUlVxSUI0N2hpZkhZQ2V1M1lCbmU3?=
 =?utf-8?B?L0QyVElEUmdBM2FNNjBwditMZzBidml3VUZNOHdqYS9mZTROcUh3ZDMvNUlM?=
 =?utf-8?B?eE5uQ0NMNjNzRGhaeWduUi9oajdmRDM4bnZDNEp2N2c2cDlwa0VXRVpoNGJ6?=
 =?utf-8?B?THlsRmhsSVhRQzBuQngwYWgvaWg4czBiUno1WHA5STAyUXhrRmtVcW5zSmYw?=
 =?utf-8?B?Q3VDN2lGbVZtRlNsb0d2WFJuNmh6cG5Nd21jUVhDWktxTW1UeG1CK2Zha0Fy?=
 =?utf-8?B?b0VkSnQvQlZ4T0lCR0greGhCSEhWb0JvdkN6YTVRczY4SjNDTkp1QjcxSzE4?=
 =?utf-8?B?djBzOGI5NUo3K3BWbWIwSFdlcng5S0VRaDVWalJiZ0t2MDU4KzFUQUpMWEQ4?=
 =?utf-8?B?cUw2RU1BM0NFUEV0d3NoZTRyU3ZXemJZQ2ZDYURHRFA2N0QxQ1o2SFhBd2ov?=
 =?utf-8?B?OG5PbndUWGJ3SU40anZ6eFNkNmVIM2liVmtSOWl3UkppVHZNRTI1RGlwUHBI?=
 =?utf-8?B?dkJOTGtrZ0FTOUlqcTJXS2NTakltMU9IZUY5R1BSbFNWUUExZXFrdEgwbGdR?=
 =?utf-8?B?WURhZmtXamd1R3VoWTZkUGljbm83MlZydTYxeUFnNWNWeHpjV3hhczJyNmJV?=
 =?utf-8?B?enVWcFdZT2FVaGkwNFVOdzlHNC9iZWtIOXFRL1N1VGo4aXpUYW5FY2dPR296?=
 =?utf-8?B?cHpDZUV5STk5bTUvQWx6L2NHWEdEdmZPNjZ4UlRXYncwMTUyUjZVTVJINDdG?=
 =?utf-8?B?MmRScysrcG1XTXVoVzNDaHpVc2ZoWHU1UGl6WWl6ZkJYZnZ6N0V0QXVVSUtx?=
 =?utf-8?B?dGF5d2NXZWc3ejdJOEw4ZXUvZzcxK2lsRnpOZmhuMnRUMzM4dUZGdnR0RDJ0?=
 =?utf-8?B?NEFjMmVEVW5FOHZwUnJTOU1WOVo3TmpyVm5yZ1Q2ejhKb01wYlZzOVkzZDl4?=
 =?utf-8?B?VHVUUkJhRlZPUG9tZ21TQ1UzSUovZTFEd1RObmRjUWVBNjUrKzdRaHcyN0Iw?=
 =?utf-8?B?V1huTzQzNW9sTHZEdXQ0YjVDSVpLRU9zdTNXTlMybUFSMnUwNHp6b0g3R3J1?=
 =?utf-8?B?OTlHeHc2cGoxdUJIM09uVXVXYkJlYmRJU3ZFYXhKSkRDRi9oTXpSaTA1OVBT?=
 =?utf-8?B?V3RxVmxqQ3d0bWYrd0ZCV2szU3B4bHV5QzArKzJRNXhqOWJwaXUrdUlGWElx?=
 =?utf-8?B?ZllJY2VwSy94OGFGSzk3cENDNGNuUUFSZmF4Z2ZQRktGOGVWMjk2L2YwcUlV?=
 =?utf-8?Q?vOUya1dA4IxB7YT3KJpm5IKl5?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c350a0e5-cd95-415a-ea55-08dc33809acf
X-MS-Exchange-CrossTenant-AuthSource: SN7PR11MB7540.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Feb 2024 08:31:02.8941
 (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: quXv/iDF7xWnTiqsvTCgHbUG3MWWc041nrubvajIF/MxMC0IbcchfUt3r0BbcRHgfrhWOWOZTIc0khObx2aeqA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW6PR11MB8391
X-OriginatorOrg: intel.com

On 2024/2/21 23:19, Jason Gunthorpe wrote:
> On Thu, Feb 08, 2024 at 12:23:02AM -0800, Yi Liu wrote:
>> If a domain is used as the parent in nested translation its mappings might
>> be cached using DID of the nested domain. But the existing code ignores
>> this fact to only invalidate the iotlb entries tagged by the domain's own
>> DID.
> 
>> Loop the s1_domains list, if any, to invalidate all iotlb entries related
>> to the target s2 address range. According to VT-d spec there is no need for
>> software to explicitly flush the affected s1 cache. It's implicitly done by
>> HW when s2 cache is invalidated.
> 
> I had to look this up to understand what it means.. The HW caches
> per-DID and if you invalidate the DID's S2 then the HW flushes the S1
> as well within that DID only.

yes.

> 
> It doesn't mean that the S2 is globally shared across all the nesting
> translations (like ARM does), and you still have to iterate over every
> nesting DID.
> 
> In light of that this design seems to have gone a bit off..
> 
> A domain should have a list of places that need invalidation,
> specifically a list of DIDs and ATCs that need an invalidation to be
> issued.
> 
> Instead we now somehow have 4 different lists in the domain the
> invalidation code iterates over?
> 
> So I would think this:
> 
> struct dmar_domain {
> 	struct xarray iommu_array;	/* Attached IOMMU array */
> 	struct list_head devices;	/* all devices' list */
> 	struct list_head dev_pasids;	/* all attached pasids */
> 	struct list_head s1_domains;
> 
> Would make sense to be collapsed into one logical list of attached
> devices:
> 
> struct intel_iommu_domain_attachment {
>     unsigned int did;
>     ioasid_t pasid;
>     struct device_domain_info *info;
>     list_head item;
> };
> 
> When you attach a S1/S2 nest you allocate two of the above structs and
> one is linked on the S1 and one is linked on the S2..

yes, this shall work. ATC flushing should be fine. But there can be a
drawback when flushing DIDs. VT-d side, if there are multiple devices
behind the same iommu unit, just need one DID flushing is enough. But
after the above change, the number of DID flushing would increase per
the number of devices. Although no functional issue, but it submits
duplicated invalidation.

-- 
Regards,
Yi Liu