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

[Vasant Hegde <vasant.hegde@amd.com>]

On 11/4/2023 12:14 AM, Jason Gunthorpe wrote:
> 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 wanted to push all these checks to low level functions rather than caller side
adding checks. Also I wanted to avoid passing protection domain IOTLB flush
functions.

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

Yeah. I can introduce few more functions and few extra checks in caller side if
that makes it easy.

-Vasant