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
