Re: [PATCH v1 07/13] iommu/amd: Consolidate device IOTLB flush code

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Oct 06, 2023 at 10:16:18AM +0000, Vasant Hegde wrote:

> @@ -1380,12 +1396,13 @@ void amd_iommu_flush_all_caches(struct amd_iommu *iommu)
>  /*
>   * Command send function for flushing on-device TLB
>   */
> -static int device_flush_iotlb(struct iommu_dev_data *dev_data,
> -			      u64 address, size_t size)
> +static int device_flush_iotlb_range(struct iommu_dev_data *dev_data,
> +				    ioasid_t pasid, u64 address, size_t size)
>  {
>  	struct amd_iommu *iommu;
>  	struct iommu_cmd cmd;
>  	int qdep;
> +	bool gn = is_pasid_valid(pasid);

I've read this a few times, and this just looks really ugly..

Something has gone off if you have to encode the type of invalidation
to do inside the PASID.

I don't have a good advice for you, but I suspect this is a
consequence of not having robust datastructures. There is not a single
handle you can use here to refer to the thing that needs to be
invalidated.

I'm not sure unifying the v1/v2 flows is actually an improvement in
clarity. In all cases the caller should already know if it is a v1/v2
object, calling a specific function is not such a burden.

Regardless, don't let this remark discrouage Joerg from merging it,
just something to think about.

Jason