Re: [PATCH 4/9] soc: apple: Add SART driver

["Sven Peter" <sven@svenpeter.dev>]

On Mon, Mar 21, 2022, at 18:17, Alyssa Rosenzweig wrote:
>> +/*
>> + * Adds the region [paddr, paddr+size] to the DMA allow list.
>> + *
>> + * @sart: SART reference
>> + * @paddr: Start address of the region to be used for DMA
>> + * @size: Size of the region to be used for DMA.
>> + */
>> +int apple_sart_add_allowed_region(struct apple_sart *sart, phys_addr_t paddr,
>> +				  size_t size);
>> +
>> +/*
>> + * Removes the region [paddr, paddr+size] from the DMA allow list.
>> + *
>> + * Note that exact same paddr and size used for apple_sart_add_allowed_region
>> + * have to be passed.
>> + *
>> + * @sart: SART reference
>> + * @paddr: Start address of the region no longer used for DMA
>> + * @size: Size of the region no longer used for DMA.
>> + */
>> +int apple_sart_remove_allowed_region(struct apple_sart *sart, phys_addr_t paddr,
>> +				     size_t size);
>
> Might it be simpler for add_allowed_region to return a handle that
> remove_allowed_region takes instead of paddr+size? Then
> remove_allowed_region doesn't have to walk anything, and the requirement
> to use the same paddr/size is implicit.

I liked that idea and just prototyped it and them remembered why I have
the API like it is:

In a perfect world this would be an IOMMU and just implement the IOMMU device ops.
In a slightly less perfect world this would implement dma_map_ops and transparently
allow using the normal DMA API inside the NVMe driver.
In Apple's world the NVMe driver needs two separate IOMMU-like filters and you can't
have two separate dma_map_ops without ugly hacks.

This will usually be used just next to dma_map_coherent and dma_free_coherent and
for those you need to keep track of phys_addr, dma_addr and size anyway,
e.g. (with the error handling removed here)

	bfr->buffer = dma_alloc_coherent(anv->dev, size, &iova, GFP_KERNEL);
	ret = apple_sart_add_allowed_region(anv->sart, iova, size);

and later then

	apple_sart_remove_allowed_region(anv->sart, bfr->iova, bfr->size);
	dma_free_coherent(anv->dev, bfr->size, bfr->buffer, bfr->iova);



Best,

Sven