Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
User-Agent: Cyrus-JMAP/3.7.0-alpha0-382-g88b93171a9-fm-20220330.001-g88b93171
Mime-Version: 1.0
Message-Id: <28ed16fd-84db-427c-bba0-7f2669f50517@www.fastmail.com>
In-Reply-To: <YjizGmjlAhyU6bC6@rosenzweig.io>
References: <20220321165049.35985-1-sven@svenpeter.dev>
 <20220321165049.35985-5-sven@svenpeter.dev> <YjizGmjlAhyU6bC6@rosenzweig.io>
Date:   Sat, 02 Apr 2022 14:40:06 +0200
From:   "Sven Peter" <sven@svenpeter.dev>
To:     "Alyssa Rosenzweig" <alyssa@rosenzweig.io>
Cc:     "Hector Martin" <marcan@marcan.st>,
        "Rob Herring" <robh+dt@kernel.org>,
        "Arnd Bergmann" <arnd@arndb.de>, "Keith Busch" <kbusch@kernel.org>,
        "axboe@fb.com" <axboe@fb.com>, "hch@lst.de" <hch@lst.de>,
        "sagi@grimberg.me" <sagi@grimberg.me>,
        "Marc Zyngier" <maz@kernel.org>, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-nvme@lists.infradead.org
Subject: Re: [PATCH 4/9] soc: apple: Add SART driver
Content-Type: text/plain
Precedence: bulk

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