Re: [PATCH v5 05/17] dma-mapping: Provide an interface to allow allocate IOVA

[Christoph Hellwig <hch@lst.de>]

On Tue, Jan 14, 2025 at 08:50:28PM +0000, Robin Murphy wrote:
>> +bool dma_iova_try_alloc(struct device *dev, struct dma_iova_state *state,
>> +		phys_addr_t phys, size_t size)
>> +{
>> +	struct iommu_domain *domain = iommu_get_dma_domain(dev);
>> +	struct iommu_dma_cookie *cookie = domain->iova_cookie;
>> +	struct iova_domain *iovad = &cookie->iovad;
>> +	size_t iova_off = iova_offset(iovad, phys);
>> +	dma_addr_t addr;
>> +
>> +	memset(state, 0, sizeof(*state));
>> +	if (!use_dma_iommu(dev))
>> +		return false;
>
> Can you guess why that return won't ever be taken?

It is regularly taken.  Now that it's quoted this way it would probably
good to split the thing up to not do the deferferences above, as they
might cause problems if the compiler wasn't smart enough to only
perform them after the check..

>> +	if (static_branch_unlikely(&iommu_deferred_attach_enabled) &&
>> +	    iommu_deferred_attach(dev, iommu_get_domain_for_dev(dev)))
>> +		return false;
>> +
>> +	if (WARN_ON_ONCE(!size))
>> +		return false;
>> +	if (WARN_ON_ONCE(size & DMA_IOVA_USE_SWIOTLB))
>
> This looks weird. Why would a caller ever set an effectively-private flag 
> in the first place? If it's actually supposed to be a maximum size check, 
> please make it look like a maximum size check.

As the person who added it - this is to catch a user passing in a value
that would set it.  To me this looks obvious, but should we add a
comment?

> (Which also makes me consider iommu_dma_max_mapping_size() returning 
> SIZE_MAX isn't strictly accurate, ho hum...)

You can still map SIZE_MAX, just not using this interface.  Assuming
no other real life limitations get in way, which I bet they will.

>> @@ -72,6 +74,21 @@
>>     #define DMA_BIT_MASK(n)	(((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
>>   +struct dma_iova_state {
>> +	dma_addr_t addr;
>> +	size_t __size;
>> +};
>> +
>> +/*
>> + * Use the high bit to mark if we used swiotlb for one or more ranges.
>> + */
>> +#define DMA_IOVA_USE_SWIOTLB		(1ULL << 63)
>
> This will give surprising results for 32-bit size_t (in fact I guess it 
> might fire some build warnings already).

Good point.  I guess __size should simply become a u64.