[PATCH 0/2] Preparation to .map_page and .unmap_page removal

[PATCH 0/2] Preparation to .map_page and .unmap_page removal

[Leon Romanovsky]

This is followup to "dma-mapping: migrate to physical address-based API"
series https://lore.kernel.org/all/cover.1750854543.git.leon@kernel.org/

These preparation changes are limited to DMA subsystem to avoid possible
complications with submission to multiple trees.

Thanks

Leon Romanovsky (2):
  dma-mapping: prepare dma_map_ops to conversion to physical address
  dma-mapping: convert dummy ops to physical address mapping

 include/linux/dma-map-ops.h |  6 ++++++
 kernel/dma/dummy.c          | 13 ++++++-------
 kernel/dma/mapping.c        | 24 ++++++++++++++++++++----
 kernel/dma/ops_helpers.c    | 14 +++++++++++---
 4 files changed, 43 insertions(+), 14 deletions(-)

-- 
2.50.1

[PATCH 1/2] dma-mapping: prepare dma_map_ops to conversion to physical address

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Add new .map_phys() and .unmap_phys() callbacks to dma_map_ops as a
preparation to replace .map_page() and .unmap_page() respectively.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 include/linux/dma-map-ops.h |  6 ++++++
 kernel/dma/mapping.c        | 24 ++++++++++++++++++++----
 kernel/dma/ops_helpers.c    | 14 +++++++++++---
 3 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h
index 71f5b30254159..0048ec28ed43b 100644
--- a/include/linux/dma-map-ops.h
+++ b/include/linux/dma-map-ops.h
@@ -37,6 +37,12 @@ struct dma_map_ops {
 	void (*unmap_page)(struct device *dev, dma_addr_t dma_handle,
 			size_t size, enum dma_data_direction dir,
 			unsigned long attrs);
+	dma_addr_t (*map_phys)(struct device *dev, phys_addr_t phys,
+			size_t size, enum dma_data_direction dir,
+			unsigned long attrs);
+	void (*unmap_phys)(struct device *dev, dma_addr_t dma_handle,
+			size_t size, enum dma_data_direction dir,
+			unsigned long attrs);
 	/*
 	 * map_sg should return a negative error code on error. See
 	 * dma_map_sgtable() for a list of appropriate error codes
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index 8d29ff76aa4db..e1586eb52ab34 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -173,9 +173,13 @@ dma_addr_t dma_map_phys(struct device *dev, phys_addr_t phys, size_t size,
 		size_t offset = offset_in_page(phys);
 		bool is_pfn_valid = true;
 
-		if (IS_ENABLED(CONFIG_DMA_API_DEBUG))
+		if (IS_ENABLED(CONFIG_DMA_API_DEBUG)) {
 			is_pfn_valid = pfn_valid(PHYS_PFN(phys));
 
+			/* We shouldn't have both functions */
+			WARN_ON_ONCE(ops->map_page && ops->map_phys);
+		}
+
 		if (unlikely(!is_pfn_valid))
 			return DMA_MAPPING_ERROR;
 
@@ -183,7 +187,11 @@ dma_addr_t dma_map_phys(struct device *dev, phys_addr_t phys, size_t size,
 		 * All platforms which implement .map_page() don't support
 		 * non-struct page backed addresses.
 		 */
-		addr = ops->map_page(dev, page, offset, size, dir, attrs);
+		if (ops->map_phys)
+			addr = ops->map_phys(dev, phys, size, dir, attrs);
+		if (ops->map_page)
+			addr = ops->map_page(dev, page, offset, size, dir,
+					     attrs);
 	}
 
 	kmsan_handle_dma(phys, size, dir);
@@ -213,8 +221,16 @@ void dma_unmap_phys(struct device *dev, dma_addr_t addr, size_t size,
 		dma_direct_unmap_phys(dev, addr, size, dir, attrs);
 	else if (use_dma_iommu(dev))
 		iommu_dma_unmap_phys(dev, addr, size, dir, attrs);
-	else
-		ops->unmap_page(dev, addr, size, dir, attrs);
+	else {
+		if (IS_ENABLED(CONFIG_DMA_API_DEBUG))
+			/* We shouldn't have both functions */
+			WARN_ON_ONCE(ops->unmap_page && ops->unmap_phys);
+
+		if (ops->unmap_phys)
+			ops->unmap_phys(dev, addr, size, dir, attrs);
+		if (ops->unmap_page)
+			ops->unmap_page(dev, addr, size, dir, attrs);
+	}
 	trace_dma_unmap_phys(dev, addr, size, dir, attrs);
 	debug_dma_unmap_phys(dev, addr, size, dir);
 }
diff --git a/kernel/dma/ops_helpers.c b/kernel/dma/ops_helpers.c
index 6f9d604d9d406..64daa1428c022 100644
--- a/kernel/dma/ops_helpers.c
+++ b/kernel/dma/ops_helpers.c
@@ -64,6 +64,7 @@ struct page *dma_common_alloc_pages(struct device *dev, size_t size,
 {
 	const struct dma_map_ops *ops = get_dma_ops(dev);
 	struct page *page;
+	phys_addr_t phys;
 
 	page = dma_alloc_contiguous(dev, size, gfp);
 	if (!page)
@@ -71,10 +72,14 @@ struct page *dma_common_alloc_pages(struct device *dev, size_t size,
 	if (!page)
 		return NULL;
 
+	phys = page_to_phys(page);
 	if (use_dma_iommu(dev))
-		*dma_handle = iommu_dma_map_phys(dev, page_to_phys(page), size,
-						 dir, DMA_ATTR_SKIP_CPU_SYNC);
-	else
+		*dma_handle = iommu_dma_map_phys(dev, phys, size, dir,
+						 DMA_ATTR_SKIP_CPU_SYNC);
+	else if (ops->map_phys)
+		*dma_handle = ops->map_phys(dev, phys, size, dir,
+					    DMA_ATTR_SKIP_CPU_SYNC);
+	else if (ops->map_page)
 		*dma_handle = ops->map_page(dev, page, 0, size, dir,
 					    DMA_ATTR_SKIP_CPU_SYNC);
 	if (*dma_handle == DMA_MAPPING_ERROR) {
@@ -94,6 +99,9 @@ void dma_common_free_pages(struct device *dev, size_t size, struct page *page,
 	if (use_dma_iommu(dev))
 		iommu_dma_unmap_phys(dev, dma_handle, size, dir,
 				     DMA_ATTR_SKIP_CPU_SYNC);
+	else if (ops->unmap_phys)
+		ops->unmap_phys(dev, dma_handle, size, dir,
+				DMA_ATTR_SKIP_CPU_SYNC);
 	else if (ops->unmap_page)
 		ops->unmap_page(dev, dma_handle, size, dir,
 				DMA_ATTR_SKIP_CPU_SYNC);
-- 
2.50.1

[PATCH 2/2] dma-mapping: convert dummy ops to physical address mapping

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Change dma_dummy_map_page and dma_dummy_unmap_page routines
to accept physical address and rename them.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 kernel/dma/dummy.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/dma/dummy.c b/kernel/dma/dummy.c
index 92de80e5b057e..dcc4c619b1389 100644
--- a/kernel/dma/dummy.c
+++ b/kernel/dma/dummy.c
@@ -11,17 +11,16 @@ static int dma_dummy_mmap(struct device *dev, struct vm_area_struct *vma,
 	return -ENXIO;
 }
 
-static dma_addr_t dma_dummy_map_page(struct device *dev, struct page *page,
-		unsigned long offset, size_t size, enum dma_data_direction dir,
-		unsigned long attrs)
+static dma_addr_t dma_dummy_map_phys(struct device *dev, phys_addr_t phys,
+		size_t size, enum dma_data_direction dir, unsigned long attrs)
 {
 	return DMA_MAPPING_ERROR;
 }
-static void dma_dummy_unmap_page(struct device *dev, dma_addr_t dma_handle,
+static void dma_dummy_unmap_phys(struct device *dev, dma_addr_t dma_handle,
 		size_t size, enum dma_data_direction dir, unsigned long attrs)
 {
 	/*
-	 * Dummy ops doesn't support map_page, so unmap_page should never be
+	 * Dummy ops doesn't support map_phys, so unmap_phys should never be
 	 * called.
 	 */
 	WARN_ON_ONCE(true);
@@ -51,8 +50,8 @@ static int dma_dummy_supported(struct device *hwdev, u64 mask)
 
 const struct dma_map_ops dma_dummy_ops = {
 	.mmap                   = dma_dummy_mmap,
-	.map_page               = dma_dummy_map_page,
-	.unmap_page             = dma_dummy_unmap_page,
+	.map_phys               = dma_dummy_map_phys,
+	.unmap_phys             = dma_dummy_unmap_phys,
 	.map_sg                 = dma_dummy_map_sg,
 	.unmap_sg               = dma_dummy_unmap_sg,
 	.dma_supported          = dma_dummy_supported,
-- 
2.50.1

Re: [PATCH 1/2] dma-mapping: prepare dma_map_ops to conversion to physical address

[Jason Gunthorpe]

On Thu, Jul 17, 2025 at 09:47:09AM +0300, Leon Romanovsky wrote:

> @@ -183,7 +187,11 @@ dma_addr_t dma_map_phys(struct device *dev, phys_addr_t phys, size_t size,
>  		 * All platforms which implement .map_page() don't support
>  		 * non-struct page backed addresses.
>  		 */
> -		addr = ops->map_page(dev, page, offset, size, dir, attrs);
> +		if (ops->map_phys)
> +			addr = ops->map_phys(dev, phys, size, dir, attrs);
> +		if (ops->map_page)
> +			addr = ops->map_page(dev, page, offset, size, dir,
> +					     attrs);

I would expect 'else if'

> @@ -213,8 +221,16 @@ void dma_unmap_phys(struct device *dev, dma_addr_t addr, size_t size,
>  		dma_direct_unmap_phys(dev, addr, size, dir, attrs);
>  	else if (use_dma_iommu(dev))
>  		iommu_dma_unmap_phys(dev, addr, size, dir, attrs);
> -	else
> -		ops->unmap_page(dev, addr, size, dir, attrs);
> +	else {
> +		if (IS_ENABLED(CONFIG_DMA_API_DEBUG))
> +			/* We shouldn't have both functions */
> +			WARN_ON_ONCE(ops->unmap_page && ops->unmap_phys);
> +
> +		if (ops->unmap_phys)
> +			ops->unmap_phys(dev, addr, size, dir, attrs);
> +		if (ops->unmap_page)
> +			ops->unmap_page(dev, addr, size, dir, attrs);

Here too

What is the practical difference between unmap_phys and unmap_page?

Do we expect implementations of map/unmap phys to never touch the
struct page? I see a few cases of dma_ops that are doing this, eg for
cache flushing so they won't be converted?

If not, how are the ops supposed to know if a KVA is available or not
for the phys argument?

Jason

Re: [PATCH 1/2] dma-mapping: prepare dma_map_ops to conversion to physical address

[Leon Romanovsky]

On Thu, Jul 17, 2025 at 10:44:35AM -0300, Jason Gunthorpe wrote:
> On Thu, Jul 17, 2025 at 09:47:09AM +0300, Leon Romanovsky wrote:
> 
> > @@ -183,7 +187,11 @@ dma_addr_t dma_map_phys(struct device *dev, phys_addr_t phys, size_t size,
> >  		 * All platforms which implement .map_page() don't support
> >  		 * non-struct page backed addresses.
> >  		 */
> > -		addr = ops->map_page(dev, page, offset, size, dir, attrs);
> > +		if (ops->map_phys)
> > +			addr = ops->map_phys(dev, phys, size, dir, attrs);
> > +		if (ops->map_page)
> > +			addr = ops->map_page(dev, page, offset, size, dir,
> > +					     attrs);
> 
> I would expect 'else if'

I don't expect both of them to exist. There is WARN_ON_ONCE() to catch it.
If it is important, I can add else.

> 
> > @@ -213,8 +221,16 @@ void dma_unmap_phys(struct device *dev, dma_addr_t addr, size_t size,
> >  		dma_direct_unmap_phys(dev, addr, size, dir, attrs);
> >  	else if (use_dma_iommu(dev))
> >  		iommu_dma_unmap_phys(dev, addr, size, dir, attrs);
> > -	else
> > -		ops->unmap_page(dev, addr, size, dir, attrs);
> > +	else {
> > +		if (IS_ENABLED(CONFIG_DMA_API_DEBUG))
> > +			/* We shouldn't have both functions */
> > +			WARN_ON_ONCE(ops->unmap_page && ops->unmap_phys);
> > +
> > +		if (ops->unmap_phys)
> > +			ops->unmap_phys(dev, addr, size, dir, attrs);
> > +		if (ops->unmap_page)
> > +			ops->unmap_page(dev, addr, size, dir, attrs);
> 
> Here too
> 
> What is the practical difference between unmap_phys and unmap_page?

There is no difference. It is provided to provide symmetrical interface
to .map_phys, simple rename.

Thanks

Re: [PATCH 1/2] dma-mapping: prepare dma_map_ops to conversion to physical address

[Jason Gunthorpe]

On Thu, Jul 17, 2025 at 05:52:13PM +0300, Leon Romanovsky wrote:
> > I would expect 'else if'
> 
> I don't expect both of them to exist. There is WARN_ON_ONCE() to catch it.
> If it is important, I can add else.

Compiler generates better code with the else.

> > > @@ -213,8 +221,16 @@ void dma_unmap_phys(struct device *dev, dma_addr_t addr, size_t size,
> > >  		dma_direct_unmap_phys(dev, addr, size, dir, attrs);
> > >  	else if (use_dma_iommu(dev))
> > >  		iommu_dma_unmap_phys(dev, addr, size, dir, attrs);
> > > -	else
> > > -		ops->unmap_page(dev, addr, size, dir, attrs);
> > > +	else {
> > > +		if (IS_ENABLED(CONFIG_DMA_API_DEBUG))
> > > +			/* We shouldn't have both functions */
> > > +			WARN_ON_ONCE(ops->unmap_page && ops->unmap_phys);
> > > +
> > > +		if (ops->unmap_phys)
> > > +			ops->unmap_phys(dev, addr, size, dir, attrs);
> > > +		if (ops->unmap_page)
> > > +			ops->unmap_page(dev, addr, size, dir, attrs);
> > 
> > Here too
> > 
> > What is the practical difference between unmap_phys and unmap_page?
> 
> There is no difference. It is provided to provide symmetrical interface
> to .map_phys, simple rename.

We really should not be converting phys to page without knowing there
is a page available..

IMHO if the underlying dma_ops are expecting a struct page it is
better to leave the ops with the struct page versions and have the
core code do the conversion to struct page. At least that way there is
one place we can prevent things that are not allowed.

The phys versions of the ops should never touch struct page or KVA. It
looked like there were a bunch of drivers like that - cache flushing
was the main issue requiring a struct page and KVA.

Jason

Re: [PATCH 1/2] dma-mapping: prepare dma_map_ops to conversion to physical address

[Leon Romanovsky]

On Thu, Jul 17, 2025 at 02:07:58PM -0300, Jason Gunthorpe wrote:
> On Thu, Jul 17, 2025 at 05:52:13PM +0300, Leon Romanovsky wrote:
> > > I would expect 'else if'
> > 
> > I don't expect both of them to exist. There is WARN_ON_ONCE() to catch it.
> > If it is important, I can add else.
> 
> Compiler generates better code with the else.
> 
> > > > @@ -213,8 +221,16 @@ void dma_unmap_phys(struct device *dev, dma_addr_t addr, size_t size,
> > > >  		dma_direct_unmap_phys(dev, addr, size, dir, attrs);
> > > >  	else if (use_dma_iommu(dev))
> > > >  		iommu_dma_unmap_phys(dev, addr, size, dir, attrs);
> > > > -	else
> > > > -		ops->unmap_page(dev, addr, size, dir, attrs);
> > > > +	else {
> > > > +		if (IS_ENABLED(CONFIG_DMA_API_DEBUG))
> > > > +			/* We shouldn't have both functions */
> > > > +			WARN_ON_ONCE(ops->unmap_page && ops->unmap_phys);
> > > > +
> > > > +		if (ops->unmap_phys)
> > > > +			ops->unmap_phys(dev, addr, size, dir, attrs);
> > > > +		if (ops->unmap_page)
> > > > +			ops->unmap_page(dev, addr, size, dir, attrs);
> > > 
> > > Here too
> > > 
> > > What is the practical difference between unmap_phys and unmap_page?
> > 
> > There is no difference. It is provided to provide symmetrical interface
> > to .map_phys, simple rename.
> 
> We really should not be converting phys to page without knowing there
> is a page available..

I didn't want. See this email and followup discussion.
https://lore.kernel.org/all/20250627170213.GL17401@unreal/

Thanks