Re: [PATCH v6 17/17] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Jan 25, 2024 at 12:11:35PM +0000, Vasant Hegde wrote:

> Hence, avoid the TLB aliasing issue with v2 page table by allocating unique
> domain ID for each device even when multiple devices are sharing the same v1
> page table. Please note that this workaround would result in multiple
> INVALIDATE_IOMMU_PAGES commands (one per domain id) when unmapping a
> translation.

I think the bigger downside is that it causes V2 domains to become
IOTLB unsharable even in simple cases with no possible PASID.

BTW "workaround" is not quite right, this is "fix", and it is a
security problem to get this wrong.

Anyhow:

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index 79a07a2e4bec..d5786c33aee6 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -536,6 +536,7 @@ struct gcr3_tbl_info {
>  	u64	*gcr3_tbl;	/* Guest CR3 table */
>  	int	glx;		/* Number of levels for GCR3 table */
>  	u32	pasid_cnt;	/* Track attached PASIDs */
> +	u16	domid;		/* Per device domain ID */

It is minor, but per-table domain ID, not per device.

Same-gcr3 tables can always safely share the domain id.

> -static void __domain_flush_pages(struct protection_domain *domain,
> +static int domain_flush_pages_v2(struct protection_domain *pdom,
>  				 u64 address, size_t size)
>  {
>  	struct iommu_dev_data *dev_data;
>  	struct iommu_cmd cmd;
> -	int ret = 0, i;
> -	ioasid_t pasid = IOMMU_NO_PASID;
> -	bool gn = false;
> +	int ret = 0;
>  
> -	if (pdom_is_v2_pgtbl_mode(domain))
> -		gn = true;
> +	list_for_each_entry(dev_data, &pdom->dev_list, list) {

Suggest a future patch to add

lockdep_assert_held(&pdom->lock);

To all functions touching dev_list.. That locking is a bit obscure.
Also, I'm not sure using a spinlock with such a wide scope makes
sense. :|

Jason