dma-mapping: Track shared DMA state through direct, pool and swiotlb paths

Commit 5b138c534fda ("dma-direct: factor out a dma_direct_alloc_from_pool helper") changed dma_direct_alloc_from_pool() to return the CPU address from dma_alloc_from_pool(). That fits dma_direct_alloc(), but dma_direct_alloc_pages() also uses the helper and expects a struct page *.

Fix this by making dma_direct_alloc_from_pool() return the struct page * again, and pass the CPU address back through an out-parameter for the dma_direct_alloc() caller.

Fixes 5b138c534fda ("dma-direct: factor out a dma_direct_alloc_from_pool helper")

Cc [email protected]

Tested-by Michael Kelley <[email protected]>

Tested-by Mostafa Saleh <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>

---
 kernel/dma/direct.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

kernel/dma/direct.c +4 -5

index d8219efe3273..363d984d90e7 100644
@@ -164,22 +164,21 @@ static bool dma_direct_use_pool(struct device *dev, gfp_t gfp)
 	return !gfpflags_allow_blocking(gfp) && !is_swiotlb_for_alloc(dev);
 }
 
-static void *dma_direct_alloc_from_pool(struct device *dev, size_t size,
-		dma_addr_t *dma_handle, gfp_t gfp)
+static struct page *dma_direct_alloc_from_pool(struct device *dev, size_t size,
+		dma_addr_t *dma_handle, void **cpu_addr, gfp_t gfp)
 {
 	struct page *page;
 	u64 phys_limit;
-	void *ret;
 
 	if (WARN_ON_ONCE(!IS_ENABLED(CONFIG_DMA_COHERENT_POOL)))
 		return NULL;
 
 	gfp |= dma_direct_optimal_gfp_mask(dev, &phys_limit);
-	page = dma_alloc_from_pool(dev, size, &ret, gfp, dma_coherent_ok);
+	page = dma_alloc_from_pool(dev, size, cpu_addr, gfp, dma_coherent_ok);
 	if (!page)
 		return NULL;
 	*dma_handle = phys_to_dma_direct(dev, page_to_phys(page));
-	return ret;
+	return page;
 }
 
 static void *dma_direct_alloc_no_mapping(struct device *dev, size_t size,

kernel/dma/direct.c +2 -1

+	}
 
 	/* we always manually zero the memory once we are done */
 	page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO, true);

@@ -357,7 +359,7 @@ struct page *dma_direct_alloc_pages(struct device *dev, size_t size,
 	void *ret;
 
 	if (force_dma_unencrypted(dev) && dma_direct_use_pool(dev, gfp))
-		return dma_direct_alloc_from_pool(dev, size, dma_handle, gfp);
+		return dma_direct_alloc_from_pool(dev, size, dma_handle, &ret, gfp);
 
 	page = __dma_direct_alloc_pages(dev, size, gfp, false);
 	if (!page)

Signature · 1 line

2.43.0

Quoted 51 lines · Aneesh Kumar K.V (Arm)

Commit 5b138c534fda ("dma-direct: factor out a dma_direct_alloc_from_pool helper") changed dma_direct_alloc_from_pool() to return the CPU address from dma_alloc_from_pool(). That fits dma_direct_alloc(), but dma_direct_alloc_pages() also uses the helper and expects a struct page *.

Fix this by making dma_direct_alloc_from_pool() return the struct page * again, and pass the CPU address back through an out-parameter for the dma_direct_alloc() caller.

Fixes 5b138c534fda ("dma-direct: factor out a dma_direct_alloc_from_pool helper")

Cc [email protected]

Tested-by Michael Kelley <[email protected]>

Tested-by Mostafa Saleh <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>

---
 kernel/dma/direct.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

kernel/dma/direct.c +4 -5

index d8219efe3273..363d984d90e7 100644
@@ -164,22 +164,21 @@ static bool dma_direct_use_pool(struct device *dev, gfp_t gfp)
 	return !gfpflags_allow_blocking(gfp) && !is_swiotlb_for_alloc(dev);
 }
 
-static void *dma_direct_alloc_from_pool(struct device *dev, size_t size,
-		dma_addr_t *dma_handle, gfp_t gfp)
+static struct page *dma_direct_alloc_from_pool(struct device *dev, size_t size,
+		dma_addr_t *dma_handle, void **cpu_addr, gfp_t gfp)
 {
 	struct page *page;
 	u64 phys_limit;
-	void *ret;
 
 	if (WARN_ON_ONCE(!IS_ENABLED(CONFIG_DMA_COHERENT_POOL)))
 		return NULL;
 
 	gfp |= dma_direct_optimal_gfp_mask(dev, &phys_limit);
-	page = dma_alloc_from_pool(dev, size, &ret, gfp, dma_coherent_ok);
+	page = dma_alloc_from_pool(dev, size, cpu_addr, gfp, dma_coherent_ok);
 	if (!page)
 		return NULL;
 	*dma_handle = phys_to_dma_direct(dev, page_to_phys(page));
-	return ret;
+	return page;
 }
 
 static void *dma_direct_alloc_no_mapping(struct device *dev, size_t size,

kernel/dma/direct.c void *dma_direct_alloc(struct device *dev, size_t size, · line 252

Quoted 9 lines

kernel/dma/direct.c +4 -2

@@ -247,8 +246,11 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 	 * the atomic pools instead if we aren't allowed block.
 	 */
 	if ((remap || force_dma_unencrypted(dev)) &&
-	    dma_direct_use_pool(dev, gfp))
-		return dma_direct_alloc_from_pool(dev, size, dma_handle, gfp);
+	    dma_direct_use_pool(dev, gfp)) {
+		page = dma_direct_alloc_from_pool(dev, size, dma_handle,
+						  &ret, gfp);
+		return page ? ret : NULL;

Sorry for joining the discussion late, but the line above caught my attention.

Why do we need both ret and page? We can derive cpu_addr from page and vice versa. Do we really need the &ret parameter? Or, more generally, do we really need "struct page *"?

static struct page *__dma_alloc_from_pool(struct device *dev, size_t size,
struct gen_pool *pool, void **cpu_addr,
bool (*phys_addr_ok)(struct device *, phys_addr_t, size_t))
{
...

	*cpu_addr = (void *)addr;
	memset(*cpu_addr, 0, size);
	return pfn_to_page(__phys_to_pfn(phys));
}

Why

Quoted 79 lines · Leon Romanovsky

Quoted 51 lines · Aneesh Kumar K.V (Arm)

Commit 5b138c534fda ("dma-direct: factor out a dma_direct_alloc_from_pool helper") changed dma_direct_alloc_from_pool() to return the CPU address from dma_alloc_from_pool(). That fits dma_direct_alloc(), but dma_direct_alloc_pages() also uses the helper and expects a struct page *.

Fix this by making dma_direct_alloc_from_pool() return the struct page * again, and pass the CPU address back through an out-parameter for the dma_direct_alloc() caller.

Fixes 5b138c534fda ("dma-direct: factor out a dma_direct_alloc_from_pool helper")

Cc [email protected]

Tested-by Michael Kelley <[email protected]>

Tested-by Mostafa Saleh <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>

---
 kernel/dma/direct.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

kernel/dma/direct.c +4 -5

index d8219efe3273..363d984d90e7 100644
@@ -164,22 +164,21 @@ static bool dma_direct_use_pool(struct device *dev, gfp_t gfp)
 	return !gfpflags_allow_blocking(gfp) && !is_swiotlb_for_alloc(dev);
 }
 
-static void *dma_direct_alloc_from_pool(struct device *dev, size_t size,
-		dma_addr_t *dma_handle, gfp_t gfp)
+static struct page *dma_direct_alloc_from_pool(struct device *dev, size_t size,
+		dma_addr_t *dma_handle, void **cpu_addr, gfp_t gfp)
 {
 	struct page *page;
 	u64 phys_limit;
-	void *ret;
 
 	if (WARN_ON_ONCE(!IS_ENABLED(CONFIG_DMA_COHERENT_POOL)))
 		return NULL;
 
 	gfp |= dma_direct_optimal_gfp_mask(dev, &phys_limit);
-	page = dma_alloc_from_pool(dev, size, &ret, gfp, dma_coherent_ok);
+	page = dma_alloc_from_pool(dev, size, cpu_addr, gfp, dma_coherent_ok);
 	if (!page)
 		return NULL;
 	*dma_handle = phys_to_dma_direct(dev, page_to_phys(page));
-	return ret;
+	return page;
 }
 
 static void *dma_direct_alloc_no_mapping(struct device *dev, size_t size,

kernel/dma/direct.c void *dma_direct_alloc(struct device *dev, size_t size, · line 252

Quoted 9 lines

kernel/dma/direct.c +4 -2

@@ -247,8 +246,11 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 	 * the atomic pools instead if we aren't allowed block.
 	 */
 	if ((remap || force_dma_unencrypted(dev)) &&
-	    dma_direct_use_pool(dev, gfp))
-		return dma_direct_alloc_from_pool(dev, size, dma_handle, gfp);
+	    dma_direct_use_pool(dev, gfp)) {
+		page = dma_direct_alloc_from_pool(dev, size, dma_handle,
+						  &ret, gfp);
+		return page ? ret : NULL;

Sorry for joining the discussion late, but the line above caught my attention.

Why do we need both ret and page? We can derive cpu_addr from page and vice versa. Do we really need the &ret parameter? Or, more generally, do we really need "struct page *"?

static struct page *__dma_alloc_from_pool(struct device *dev, size_t size,
struct gen_pool *pool, void **cpu_addr,
bool (*phys_addr_ok)(struct device *, phys_addr_t, size_t))
{
...

	*cpu_addr = (void *)addr;
	memset(*cpu_addr, 0, size);
	return pfn_to_page(__phys_to_pfn(phys));
}

Quoted 2 lines

Why

With CONFIG_DMA_DIRECT_REMAP the cpu_addr can be different from page_address.

-aneesh

Quoted 3 lines · Aneesh Kumar K.V

Quoted 1 line · Leon Romanovsky

On Fri, Jul 17, 2026 at 11:34:19PM +0530, Aneesh Kumar K.V (Arm) wrote:

Quoted 32 lines

Quoted 31 lines

kernel/dma/direct.c void *dma_direct_alloc(struct device *dev, size_t size, · line 252

Quoted 11 lines

static void *dma_direct_alloc_no_mapping(struct device *dev, size_t size,

kernel/dma/direct.c +4 -2

@@ -247,8 +246,11 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 	 * the atomic pools instead if we aren't allowed block.
 	 */
 	if ((remap || force_dma_unencrypted(dev)) &&
-	    dma_direct_use_pool(dev, gfp))
-		return dma_direct_alloc_from_pool(dev, size, dma_handle, gfp);
+	    dma_direct_use_pool(dev, gfp)) {
+		page = dma_direct_alloc_from_pool(dev, size, dma_handle,
+						  &ret, gfp);
+		return page ? ret : NULL;

Sorry for joining the discussion late, but the line above caught my attention.

Why do we need both ret and page? We can derive cpu_addr from page and vice versa. Do we really need the &ret parameter? Or, more generally, do we really need "struct page *"?

static struct page *__dma_alloc_from_pool(struct device *dev, size_t size,
struct gen_pool *pool, void **cpu_addr,
bool (*phys_addr_ok)(struct device *, phys_addr_t, size_t))
{
...

	*cpu_addr = (void *)addr;
	memset(*cpu_addr, 0, size);
	return pfn_to_page(__phys_to_pfn(phys));
}

Why

Quoted 2 lines

With CONFIG_DMA_DIRECT_REMAP the cpu_addr can be different from page_address.

Can you please point to the code there it can happen? __dma_alloc_from_pool() has direct connection between physical address and struct page.

Quoted 4 lines · Leon Romanovsky

Quoted 3 lines · Aneesh Kumar K.V

Quoted 1 line · Leon Romanovsky

On Fri, Jul 17, 2026 at 11:34:19PM +0530, Aneesh Kumar K.V (Arm) wrote:

....

Quoted 32 lines

Quoted 32 lines

Quoted 31 lines

kernel/dma/direct.c void *dma_direct_alloc(struct device *dev, size_t size, · line 252

Quoted 11 lines

static void *dma_direct_alloc_no_mapping(struct device *dev, size_t size,

kernel/dma/direct.c +4 -2

@@ -247,8 +246,11 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 	 * the atomic pools instead if we aren't allowed block.
 	 */
 	if ((remap || force_dma_unencrypted(dev)) &&
-	    dma_direct_use_pool(dev, gfp))
-		return dma_direct_alloc_from_pool(dev, size, dma_handle, gfp);
+	    dma_direct_use_pool(dev, gfp)) {
+		page = dma_direct_alloc_from_pool(dev, size, dma_handle,
+						  &ret, gfp);
+		return page ? ret : NULL;

Sorry for joining the discussion late, but the line above caught my attention.

Why do we need both ret and page? We can derive cpu_addr from page and vice versa. Do we really need the &ret parameter? Or, more generally, do we really need "struct page *"?

static struct page *__dma_alloc_from_pool(struct device *dev, size_t size,
struct gen_pool *pool, void **cpu_addr,
bool (*phys_addr_ok)(struct device *, phys_addr_t, size_t))
{
...

	*cpu_addr = (void *)addr;
	memset(*cpu_addr, 0, size);
	return pfn_to_page(__phys_to_pfn(phys));
}

Why

Quoted 7 lines

Quoted 2 lines

With CONFIG_DMA_DIRECT_REMAP the cpu_addr can be different from page_address.

Can you please point to the code there it can happen? __dma_alloc_from_pool() has direct connection between physical address and struct page.

dma_direct_alloc -> 	remap = IS_ENABLED(CONFIG_DMA_DIRECT_REMAP);
	if ((remap && dma_direct_use_pool(dev, gfp)) {
		page = dma_direct_alloc_from_pool(dev, size,

..
__dma_alloc_from_pool ->

	addr = gen_pool_alloc(pool, size);
	if (!addr)

We expand the pool as below..

atomic_pool_expand ->

#ifdef CONFIG_DMA_DIRECT_REMAP
	addr = dma_common_contiguous_remap(page, pool_size,
			pgprot_decrypted(pgprot_dmacoherent(PAGE_KERNEL)),
			__builtin_return_address(0));
	if (!addr)
		goto free_page;
#else
	addr = page_to_virt(page);

#endif

-aneesh

Something like, but probably void* needs to be phys_addr_t:

static inline struct page *dma_phys_to_page(void *addr) {

#ifdef CONFIG_DMA_DIRECT_REMAP
	return vmalloc_to_page(addr);
#else
	return virt_to_page(addr);

#endif
}

Quoted 10 lines · Leon Romanovsky

Something like, but probably void* needs to be phys_addr_t:

static inline struct page *dma_phys_to_page(void *addr) {

#ifdef CONFIG_DMA_DIRECT_REMAP
	return vmalloc_to_page(addr);
#else
	return virt_to_page(addr);

#endif
}

I don't like this, we don't know for sure the addr will be in a vmap and this will fail if it isn't.

Jason

Quoted 13 lines · Jason Gunthorpe

Quoted 10 lines · Leon Romanovsky

Something like, but probably void* needs to be phys_addr_t:

static inline struct page *dma_phys_to_page(void *addr) {

#ifdef CONFIG_DMA_DIRECT_REMAP
	return vmalloc_to_page(addr);
#else
	return virt_to_page(addr);

#endif
}

Quoted 2 lines

I don't like this, we don't know for sure the addr will be in a vmap and this will fail if it isn't.

Of course we know. The existing "#ifdef CONFIG_DMA_DIRECT_REMAP" is relevant for addresses acquired from pool.

dma_phys_to_page() is a bad name for some low-level conversion function. It needs to be internal to DMA logic, in the level when we convert from phys to page.

Quoted 17 lines · Leon Romanovsky

Quoted 15 lines · Jason Gunthorpe

Quoted 10 lines · Leon Romanovsky

Something like, but probably void* needs to be phys_addr_t:

static inline struct page *dma_phys_to_page(void *addr) {

#ifdef CONFIG_DMA_DIRECT_REMAP
	return vmalloc_to_page(addr);
#else
	return virt_to_page(addr);

#endif
}

I don't like this, we don't know for sure the addr will be in a vmap and this will fail if it isn't.

Quoted 2 lines

Of course we know. The existing "#ifdef CONFIG_DMA_DIRECT_REMAP" is relevant for addresses acquired from pool.

Yeah, but I still don't like it :) It is hard to follow if you make those kinds of leaps, someone will call this new helper on something they shouldn't

Quoted 4 lines

dma_phys_to_page() is a bad name for some low-level conversion function. It needs to be internal to DMA logic, in the level when we convert from phys to page.

I think we should not convert from phys to page, that's also easy to do wrong

Jason

Currently, the phys type universally describes memory and can be reliably translated into any required representation. It is the most fundamental type we have.

Quoted 3 lines · Leon Romanovsky

Currently, the phys type universally describes memory and can be reliably translated into any required representation. It is the most fundamental type we have.

You cannot go from phys to page reliably unless you make assumptions about the phys, that's the whole issue here. If the callchain expects and intends to use a page it should really stay in a page and not take a side trip to phys.

I agree it would help to have some helpers that were more phys based for the callchains that want that since we are mixing two fairly different activities in these functions..

But making phys the only output out of the pool isn't going to be an improvement.

BTW how does a struct page even work for decrypted memory? You can't actually use it that way right? If you try to access it then you'll get the wrong IPA and it should explode?

Maybe we should be blocking the struct page path entirely for CC shared?

Jason

Quoted 18 lines · Jason Gunthorpe

Quoted 17 lines · Leon Romanovsky

Quoted 15 lines · Jason Gunthorpe

Quoted 10 lines · Leon Romanovsky

Something like, but probably void* needs to be phys_addr_t:

static inline struct page *dma_phys_to_page(void *addr) {

#ifdef CONFIG_DMA_DIRECT_REMAP
	return vmalloc_to_page(addr);
#else
	return virt_to_page(addr);

#endif
}

I don't like this, we don't know for sure the addr will be in a vmap and this will fail if it isn't.

Quoted 6 lines

Quoted 2 lines

Of course we know. The existing "#ifdef CONFIG_DMA_DIRECT_REMAP" is relevant for addresses acquired from pool.

Yeah, but I still don't like it :) It is hard to follow if you make those kinds of leaps, someone will call this new helper on something they shouldn't

This issue is straightforward to address today. Limit the scope, use an appropriate function name, and add a comment indicating that the function is local to and specific to the pool. The latter helps AI-based review tools flag incorrect usage outside the intended scope.

Quoted 7 lines

Quoted 4 lines

dma_phys_to_page() is a bad name for some low-level conversion function. It needs to be internal to DMA logic, in the level when we convert from phys to page.

I think we should not convert from phys to page, that's also easy to do wrong

Perhaps I'll reiterate my complaint. DMA internals are already hard to understand. Part of that complexity is necessary to support every possible use case, but another part comes from a maze of types that is entirely self-inflicted. I think this code only makes the latter worse.

Currently, the phys type universally describes memory and can be reliably translated into any required representation. It is the most fundamental type we have.

Commit 5b138c534fda ("dma-direct: factor out a dma_direct_alloc_from_pool helper") changed dma_direct_alloc_from_pool() to return the CPU address from dma_alloc_from_pool(). That fits dma_direct_alloc(), but dma_direct_alloc_pages() also uses the helper and expects a struct page *.

Fix this by making dma_direct_alloc_from_pool() return the struct page * again, and pass the CPU address back through an out-parameter for the dma_direct_alloc() caller.

Fixes 5b138c534fda ("dma-direct: factor out a dma_direct_alloc_from_pool helper")

Cc [email protected]

Tested-by Michael Kelley <[email protected]>

Tested-by Mostafa Saleh <[email protected]>

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>

---
 kernel/dma/direct.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

kernel/dma/direct.c +10 -8

index d8219efe3273..363d984d90e7 100644
@@ -164,22 +164,21 @@ static bool dma_direct_use_pool(struct device *dev, gfp_t gfp)
 	return !gfpflags_allow_blocking(gfp) && !is_swiotlb_for_alloc(dev);
 }
 
-static void *dma_direct_alloc_from_pool(struct device *dev, size_t size,
-		dma_addr_t *dma_handle, gfp_t gfp)
+static struct page *dma_direct_alloc_from_pool(struct device *dev, size_t size,
+		dma_addr_t *dma_handle, void **cpu_addr, gfp_t gfp)
 {
 	struct page *page;
 	u64 phys_limit;
-	void *ret;
 
 	if (WARN_ON_ONCE(!IS_ENABLED(CONFIG_DMA_COHERENT_POOL)))
 		return NULL;
 
 	gfp |= dma_direct_optimal_gfp_mask(dev, &phys_limit);
-	page = dma_alloc_from_pool(dev, size, &ret, gfp, dma_coherent_ok);
+	page = dma_alloc_from_pool(dev, size, cpu_addr, gfp, dma_coherent_ok);
 	if (!page)
 		return NULL;
 	*dma_handle = phys_to_dma_direct(dev, page_to_phys(page));
-	return ret;
+	return page;
 }
 
 static void *dma_direct_alloc_no_mapping(struct device *dev, size_t size,

@@ -247,8 +246,11 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 	 * the atomic pools instead if we aren't allowed block.
 	 */
 	if ((remap || force_dma_unencrypted(dev)) &&
-	    dma_direct_use_pool(dev, gfp))
-		return dma_direct_alloc_from_pool(dev, size, dma_handle, gfp);
+	    dma_direct_use_pool(dev, gfp)) {
+		page = dma_direct_alloc_from_pool(dev, size, dma_handle,
+						  &ret, gfp);
+		return page ? ret : NULL;
+	}
 
 	/* we always manually zero the memory once we are done */
 	page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO, true);

@@ -357,7 +359,7 @@ struct page *dma_direct_alloc_pages(struct device *dev, size_t size,
 	void *ret;
 
 	if (force_dma_unencrypted(dev) && dma_direct_use_pool(dev, gfp))
-		return dma_direct_alloc_from_pool(dev, size, dma_handle, gfp);
+		return dma_direct_alloc_from_pool(dev, size, dma_handle, &ret, gfp);
 
 	page = __dma_direct_alloc_pages(dev, size, gfp, false);
 	if (!page)

Signature · 1 line

2.43.0

atomic_pool_expand() frees the allocated pages from the remove_mapping error path only when CONFIG_DMA_DIRECT_REMAP is enabled.

When CONFIG_DMA_DIRECT_REMAP is disabled, failures after page allocation, such as gen_pool_add_virt(), jump to remove_mapping and return without freeing the pages.

Move __free_pages(page, order) out of the CONFIG_DMA_DIRECT_REMAP block so that cleanup paths always release the allocation.

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Michael Kelley <[email protected]>

Tested-by Mostafa Saleh <[email protected]>

Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>

---
 kernel/dma/pool.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

kernel/dma/pool.c +4 -1

index 2b2fbb709242..b0303efbc153 100644
@@ -81,6 +81,7 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
 {
 	unsigned int order;
 	struct page *page = NULL;
+	bool leak_pages = false;
 	void *addr;
 	int ret = -ENOMEM;
 

@@ -115,8 +116,10 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
 	 */
 	ret = set_memory_decrypted((unsigned long)page_to_virt(page),
 				   1 << order);
-	if (ret)
+	if (ret) {
+		leak_pages = true;
 		goto remove_mapping;
+	}
 	ret = gen_pool_add_virt(pool, (unsigned long)addr, page_to_phys(page),
 				pool_size, NUMA_NO_NODE);
 	if (ret)

kernel/dma/pool.c +0 -0

 out:
 	return ret;
 }

Signature · 1 line

2.43.0

Quoted 43 lines · Aneesh Kumar K.V (Arm)

atomic_pool_expand() frees the allocated pages from the remove_mapping error path only when CONFIG_DMA_DIRECT_REMAP is enabled.

When CONFIG_DMA_DIRECT_REMAP is disabled, failures after page allocation, such as gen_pool_add_virt(), jump to remove_mapping and return without freeing the pages.

Move __free_pages(page, order) out of the CONFIG_DMA_DIRECT_REMAP block so that cleanup paths always release the allocation.

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Michael Kelley <[email protected]>

Tested-by Mostafa Saleh <[email protected]>

Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>

---
 kernel/dma/pool.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

kernel/dma/pool.c +4 -1

index 2b2fbb709242..b0303efbc153 100644
@@ -81,6 +81,7 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
 {
 	unsigned int order;
 	struct page *page = NULL;
+	bool leak_pages = false;
 	void *addr;
 	int ret = -ENOMEM;
 

@@ -115,8 +116,10 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
 	 */
 	ret = set_memory_decrypted((unsigned long)page_to_virt(page),
 				   1 << order);
-	if (ret)
+	if (ret) {
+		leak_pages = true;
 		goto remove_mapping;
+	}
 	ret = gen_pool_add_virt(pool, (unsigned long)addr, page_to_phys(page),
 				pool_size, NUMA_NO_NODE);
 	if (ret)

@@ -130,14 +133,15 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,

kernel/dma/pool.c static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size, · line 136

Quoted 5 lines

kernel/dma/pool.c +1 -1

 				   1 << order);
 	if (WARN_ON_ONCE(ret)) {
 		/* Decrypt succeeded but encrypt failed, purposely leak */
-		goto out;
+		leak_pages = true;

Instead of doing this dance with temporal variable, change "goto out" to be "return true".

kernel/dma/pool.c static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size, · line 141

Quoted 6 lines

kernel/dma/pool.c +0 -0

 	}
 remove_mapping:
 #ifdef CONFIG_DMA_DIRECT_REMAP
 	dma_common_free_remap(addr, pool_size);
 free_page:

Remove free_page label, and change leftover of "goto free_page" to be "goto out"

kernel/dma/pool.c static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size, · line 144

Quoted 4 lines

kernel/dma/pool.c +2 -1

-	__free_pages(page, order);
 #endif
+	if (!leak_pages)
+		__free_pages(page, order);

Put these checks under out label and rely on page != NULL as a marker. if (page)
__free_pages(page, order);

Quoted 7 lines

kernel/dma/pool.c +0 -0

 out:
 	return ret;
 }

Signature · 1 line

2.43.0

Quoted 44 lines · Leon Romanovsky

Quoted 43 lines · Aneesh Kumar K.V (Arm)

atomic_pool_expand() frees the allocated pages from the remove_mapping error path only when CONFIG_DMA_DIRECT_REMAP is enabled.

When CONFIG_DMA_DIRECT_REMAP is disabled, failures after page allocation, such as gen_pool_add_virt(), jump to remove_mapping and return without freeing the pages.

Move __free_pages(page, order) out of the CONFIG_DMA_DIRECT_REMAP block so that cleanup paths always release the allocation.

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Michael Kelley <[email protected]>

Tested-by Mostafa Saleh <[email protected]>

Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>

---
 kernel/dma/pool.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

kernel/dma/pool.c +4 -1

index 2b2fbb709242..b0303efbc153 100644
@@ -81,6 +81,7 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
 {
 	unsigned int order;
 	struct page *page = NULL;
+	bool leak_pages = false;
 	void *addr;
 	int ret = -ENOMEM;
 

@@ -115,8 +116,10 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
 	 */
 	ret = set_memory_decrypted((unsigned long)page_to_virt(page),
 				   1 << order);
-	if (ret)
+	if (ret) {
+		leak_pages = true;
 		goto remove_mapping;
+	}
 	ret = gen_pool_add_virt(pool, (unsigned long)addr, page_to_phys(page),
 				pool_size, NUMA_NO_NODE);
 	if (ret)

@@ -130,14 +133,15 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,

Quoted 9 lines

kernel/dma/pool.c static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size, · line 136

Quoted 5 lines

kernel/dma/pool.c +1 -1

 				   1 << order);
 	if (WARN_ON_ONCE(ret)) {
 		/* Decrypt succeeded but encrypt failed, purposely leak */
-		goto out;
+		leak_pages = true;

Instead of doing this dance with temporal variable, change "goto out" to be "return true".

I didn't follow the return true part. A failure in set_memory_encrypted() or set_memory_decrypted() requires the pages to be leaked, so this is not the only call site that sets leak_pages = true. There is a similar case a few lines above.

Quoted 12 lines

kernel/dma/pool.c static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size, · line 141

Quoted 8 lines

kernel/dma/pool.c +0 -0

 	}
 remove_mapping:
 #ifdef CONFIG_DMA_DIRECT_REMAP
 	dma_common_free_remap(addr, pool_size);
 free_page:

Remove free_page label, and change leftover of "goto free_page" to be "goto out"

Quoted 16 lines

kernel/dma/pool.c static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size, · line 144

Quoted 4 lines

kernel/dma/pool.c +2 -1

-	__free_pages(page, order);
 #endif
+	if (!leak_pages)
+		__free_pages(page, order);

Put these checks under out label and rely on page != NULL as a marker. if (page)
__free_pages(page, order);

Quoted 7 lines

kernel/dma/pool.c +0 -0

 out:
 	return ret;
 }

Signature · 1 line

2.43.0

-aneesh

swiotlb_alloc_tlb() can allocate from the DMA atomic pool when a decrypted pool is needed from atomic context. With CONFIG_DMA_DIRECT_REMAP, the atomic pool is backed by remapped virtual addresses, which are not the same as the direct-map addresses returned by phys_to_virt().

swiotlb_init_io_tlb_pool() currently reconstructs the pool virtual address from the physical start address. For atomic-pool backed allocations this stores the wrong address in pool->vaddr. Later, swiotlb_free_tlb() passes that address to dma_free_from_pool(), which will fail to recognize the chunk

Pass the virtual address returned by the allocation path into swiotlb_init_io_tlb_pool(), and store that address in pool->vaddr. This keeps the pool free path using the same virtual address as the allocator.

Fixes 79636caad361 ("swiotlb: if swiotlb is full, fall back to a transient memory pool")

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Michael Kelley <[email protected]>

Tested-by Mostafa Saleh <[email protected]>

Reviewed-by Petr Tesarik <[email protected]>

Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>

---
 kernel/dma/swiotlb.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

kernel/dma/swiotlb.c +19 -12

index 1abd3e6146f4..6e8db52866bf 100644
@@ -266,9 +266,9 @@ void __init swiotlb_update_mem_attributes(void)
 }
 
 static void swiotlb_init_io_tlb_pool(struct io_tlb_pool *mem, phys_addr_t start,
-		unsigned long nslabs, bool late_alloc, unsigned int nareas)
+		void *vaddr, unsigned long nslabs, bool late_alloc,
+		unsigned int nareas)
 {
-	void *vaddr = phys_to_virt(start);
 	unsigned long bytes = nslabs << IO_TLB_SHIFT, i;
 
 	mem->nslabs = nslabs;

@@ -409,7 +409,7 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
 		return;
 	}
 
-	swiotlb_init_io_tlb_pool(mem, __pa(tlb), nslabs, false, nareas);
+	swiotlb_init_io_tlb_pool(mem, __pa(tlb), tlb, nslabs, false, nareas);
 	add_mem_pool(&io_tlb_default_mem, mem);
 
 	if (flags & SWIOTLB_VERBOSE)

@@ -507,7 +507,7 @@ int swiotlb_init_late(size_t size, gfp_t gfp_mask,
 
 	set_memory_decrypted((unsigned long)vstart,
 			     (nslabs << IO_TLB_SHIFT) >> PAGE_SHIFT);
-	swiotlb_init_io_tlb_pool(mem, virt_to_phys(vstart), nslabs, true,
+	swiotlb_init_io_tlb_pool(mem, virt_to_phys(vstart), vstart, nslabs, true,
 				 nareas);
 	add_mem_pool(&io_tlb_default_mem, mem);
 

@@ -605,25 +605,26 @@ static struct page *alloc_dma_pages(gfp_t gfp, size_t bytes, u64 phys_limit)
  * @bytes:	Size of the buffer.
  * @phys_limit:	Maximum allowed physical address of the buffer.
  * @gfp:	GFP flags for the allocation.
+ * @vaddr:	Receives the virtual address for the allocated buffer.
  *
  * Return: Allocated pages, or %NULL on allocation failure.
  */
 static struct page *swiotlb_alloc_tlb(struct device *dev, size_t bytes,
-		u64 phys_limit, gfp_t gfp)
+		u64 phys_limit, gfp_t gfp, void **vaddr)
 {
 	struct page *page;
 
+	*vaddr = NULL;
+
 	/*
 	 * Allocate from the atomic pools if memory is encrypted and
 	 * the allocation is atomic, because decrypting may block.
 	 */
 	if (!gfpflags_allow_blocking(gfp) && dev && force_dma_unencrypted(dev)) {
-		void *vaddr;
-
 		if (!IS_ENABLED(CONFIG_DMA_COHERENT_POOL))
 			return NULL;
 
-		return dma_alloc_from_pool(dev, bytes, &vaddr, gfp,
+		return dma_alloc_from_pool(dev, bytes, vaddr, gfp,
 					   dma_coherent_ok);
 	}
 

@@ -645,6 +646,8 @@ static struct page *swiotlb_alloc_tlb(struct device *dev, size_t bytes,
 			return NULL;
 	}
 
+	if (page)
+		*vaddr = phys_to_virt(page_to_phys(page));
 	return page;
 }
 

@@ -685,6 +688,7 @@ static struct io_tlb_pool *swiotlb_alloc_pool(struct device *dev,
 {
 	struct io_tlb_pool *pool;
 	unsigned int slot_order;
+	void *tlb_vaddr;
 	struct page *tlb;
 	size_t pool_size;
 	size_t tlb_size;

@@ -701,7 +705,8 @@ static struct io_tlb_pool *swiotlb_alloc_pool(struct device *dev,
 	pool->areas = (void *)pool + sizeof(*pool);
 
 	tlb_size = nslabs << IO_TLB_SHIFT;
-	while (!(tlb = swiotlb_alloc_tlb(dev, tlb_size, phys_limit, gfp))) {
+	while (!(tlb = swiotlb_alloc_tlb(dev, tlb_size, phys_limit, gfp,
+					 &tlb_vaddr))) {
 		if (nslabs <= minslabs)
 			goto error_tlb;
 		nslabs = ALIGN(nslabs >> 1, IO_TLB_SEGSIZE);

@@ -715,11 +720,12 @@ static struct io_tlb_pool *swiotlb_alloc_pool(struct device *dev,
 	if (!pool->slots)
 		goto error_slots;
 
-	swiotlb_init_io_tlb_pool(pool, page_to_phys(tlb), nslabs, true, nareas);
+	swiotlb_init_io_tlb_pool(pool, page_to_phys(tlb), tlb_vaddr, nslabs,
+				 true, nareas);
 	return pool;
 
 error_slots:
-	swiotlb_free_tlb(page_address(tlb), tlb_size);
+	swiotlb_free_tlb(tlb_vaddr, tlb_size);
 error_tlb:
 	kfree(pool);
 error:

@@ -1851,7 +1857,8 @@ static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
 
 		set_memory_decrypted((unsigned long)phys_to_virt(rmem->base),
 				     rmem->size >> PAGE_SHIFT);
-		swiotlb_init_io_tlb_pool(pool, rmem->base, nslabs,
+		swiotlb_init_io_tlb_pool(pool, rmem->base, phys_to_virt(rmem->base),
+					 nslabs,
 					 false, nareas);
 		mem->force_bounce = true;
 		mem->for_alloc = true;

Signature · 1 line

2.43.0

Move swiotlb allocation out of __dma_direct_alloc_pages() and handle it in dma_direct_alloc() / dma_direct_alloc_pages().

This is needed for follow-up changes that simplify the handling of memory encryption/decryption based on the DMA attribute flags.

swiotlb backing pages are already mapped decrypted by swiotlb_update_mem_attributes() and rmem_swiotlb_device_init(), so dma-direct should not call dma_set_decrypted() on allocation nor dma_set_encrypted() on free for swiotlb-backed memory.

Update alloc/free paths to detect swiotlb-backed pages and skip encrypt/decrypt transitions for those paths. Keep the existing highmem rejection in dma_direct_alloc_pages() for swiotlb allocations.

Only for "restricted-dma-pool", we currently set for_alloc = true, while rmem_swiotlb_device_init() decrypts the whole pool up front. This pool is typically used together with "shared-dma-pool", where the shared region is accessed after remap/ioremap and the returned address is suitable for decrypted memory access. So existing code paths remain valid.

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Jiri Pirko <[email protected]>

Tested-by Michael Kelley <[email protected]>

Tested-by Mostafa Saleh <[email protected]>

Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>

---
 include/linux/swiotlb.h |  6 ++++
 kernel/dma/direct.c     | 71 ++++++++++++++++++++++++++++++-----------
 kernel/dma/swiotlb.c    |  6 ++++
 3 files changed, 65 insertions(+), 18 deletions(-)

include/linux/swiotlb.h +6 -0

index 3dae0f592063..c92ff6791595 100644
@@ -284,6 +284,8 @@ extern void swiotlb_print_info(void);
 #ifdef CONFIG_DMA_RESTRICTED_POOL
 struct page *swiotlb_alloc(struct device *dev, size_t size);
 bool swiotlb_free(struct device *dev, struct page *page, size_t size);
+void swiotlb_free_from_pool(struct device *dev,
+		phys_addr_t tlb_addr, struct io_tlb_pool *pool);
 
 static inline bool is_swiotlb_for_alloc(struct device *dev)
 {

@@ -299,6 +301,10 @@ static inline bool swiotlb_free(struct device *dev, struct page *page,
 {
 	return false;
 }
+static inline void swiotlb_free_from_pool(struct device *dev,
+		phys_addr_t tlb_addr, struct io_tlb_pool *pool)
+{
+}
 static inline bool is_swiotlb_for_alloc(struct device *dev)
 {
 	return false;

kernel/dma/direct.c +53 -18

index a81597f8c3ed..171c89fe99c9 100644
@@ -96,14 +96,6 @@ static int dma_set_encrypted(struct device *dev, void *vaddr, size_t size)
 	return ret;
 }
 
-static void __dma_direct_free_pages(struct device *dev, struct page *page,
-				    size_t size)
-{
-	if (swiotlb_free(dev, page, size))
-		return;
-	dma_free_contiguous(dev, page, size);
-}
-
 static struct page *dma_direct_alloc_swiotlb(struct device *dev, size_t size)
 {
 	struct page *page = swiotlb_alloc(dev, size);

@@ -125,9 +117,6 @@ static struct page *__dma_direct_alloc_pages(struct device *dev, size_t size,
 
 	WARN_ON_ONCE(!PAGE_ALIGNED(size));
 
-	if (is_swiotlb_for_alloc(dev))
-		return dma_direct_alloc_swiotlb(dev, size);
-
 	gfp |= dma_direct_optimal_gfp_mask(dev, &phys_limit);
 	page = dma_alloc_contiguous(dev, size, gfp);
 	if (page) {

@@ -203,6 +192,7 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 		dma_addr_t *dma_handle, gfp_t gfp, unsigned long attrs)
 {
 	bool remap = false, set_uncached = false;
+	bool mark_mem_decrypt = true;
 	struct page *page;
 	void *ret;
 

@@ -252,11 +242,21 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 		return page ? ret : NULL;
 	}
 
+	if (is_swiotlb_for_alloc(dev)) {
+		page = dma_direct_alloc_swiotlb(dev, size);
+		if (page) {
+			mark_mem_decrypt = false;
+			goto setup_page;
+		}
+		return NULL;
+	}
+
 	/* we always manually zero the memory once we are done */
 	page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO, true);
 	if (!page)
 		return NULL;
 
+setup_page:
 	/*
 	 * dma_alloc_contiguous can return highmem pages depending on a
 	 * combination the cma= arguments and per-arch setup.  These need to be

@@ -283,7 +283,7 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 			goto out_free_pages;
 	} else {
 		ret = page_address(page);
-		if (dma_set_decrypted(dev, ret, size))
+		if (mark_mem_decrypt && dma_set_decrypted(dev, ret, size))
 			goto out_leak_pages;
 	}
 

@@ -300,10 +300,11 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 	return ret;
 
 out_encrypt_pages:
-	if (dma_set_encrypted(dev, page_address(page), size))
+	if (mark_mem_decrypt && dma_set_encrypted(dev, page_address(page), size))
 		return NULL;
 out_free_pages:
-	__dma_direct_free_pages(dev, page, size);
+	if (!swiotlb_free(dev, page, size))
+		dma_free_contiguous(dev, page, size);
 	return NULL;
 out_leak_pages:
 	return NULL;

@@ -312,6 +313,9 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 void dma_direct_free(struct device *dev, size_t size,
 		void *cpu_addr, dma_addr_t dma_addr, unsigned long attrs)
 {
+	phys_addr_t phys;
+	bool mark_mem_encrypted = true;
+	struct io_tlb_pool *swiotlb_pool;
 	unsigned int page_order = get_order(size);
 
 	if ((attrs & DMA_ATTR_NO_KERNEL_MAPPING) &&

@@ -340,16 +344,25 @@ void dma_direct_free(struct device *dev, size_t size,
 	    dma_free_from_pool(dev, cpu_addr, PAGE_ALIGN(size)))
 		return;
 
+	phys = dma_to_phys(dev, dma_addr);
+	swiotlb_pool = swiotlb_find_pool(dev, phys);
+	if (swiotlb_pool)
+		/* Swiotlb doesn't need a page attribute update on free */
+		mark_mem_encrypted = false;
+
 	if (is_vmalloc_addr(cpu_addr)) {
 		vunmap(cpu_addr);
 	} else {
 		if (IS_ENABLED(CONFIG_ARCH_HAS_DMA_CLEAR_UNCACHED))
 			arch_dma_clear_uncached(cpu_addr, size);
-		if (dma_set_encrypted(dev, cpu_addr, size))
+		if (mark_mem_encrypted && dma_set_encrypted(dev, cpu_addr, size))
 			return;
 	}
 
-	__dma_direct_free_pages(dev, dma_direct_to_page(dev, dma_addr), size);
+	if (swiotlb_pool)
+		swiotlb_free_from_pool(dev, phys, swiotlb_pool);
+	else
+		dma_free_contiguous(dev, dma_direct_to_page(dev, dma_addr), size);
 }
 
 struct page *dma_direct_alloc_pages(struct device *dev, size_t size,

@@ -361,6 +374,15 @@ struct page *dma_direct_alloc_pages(struct device *dev, size_t size,
 	if (force_dma_unencrypted(dev) && dma_direct_use_pool(dev, gfp))
 		return dma_direct_alloc_from_pool(dev, size, dma_handle, &ret, gfp);
 
+	if (is_swiotlb_for_alloc(dev)) {
+		page = dma_direct_alloc_swiotlb(dev, size);
+		if (!page)
+			return NULL;
+
+		ret = page_address(page);
+		goto setup_page;
+	}
+
 	page = __dma_direct_alloc_pages(dev, size, gfp, false);
 	if (!page)
 		return NULL;

@@ -368,6 +390,7 @@ struct page *dma_direct_alloc_pages(struct device *dev, size_t size,
 	ret = page_address(page);
 	if (dma_set_decrypted(dev, ret, size))
 		goto out_leak_pages;
+setup_page:
 	memset(ret, 0, size);
 	*dma_handle = phys_to_dma_direct(dev, page_to_phys(page));
 	return page;

@@ -379,16 +402,28 @@ void dma_direct_free_pages(struct device *dev, size_t size,
 		struct page *page, dma_addr_t dma_addr,
 		enum dma_data_direction dir)
 {
+	phys_addr_t phys;
 	void *vaddr = page_address(page);
+	struct io_tlb_pool *swiotlb_pool;
+	bool mark_mem_encrypted = true;
 
 	/* If page is not from an atomic pool, dma_free_from_pool_page() fails */
 	if (IS_ENABLED(CONFIG_DMA_COHERENT_POOL) &&
 	    dma_free_from_pool_page(dev, page, size))
 		return;
 
-	if (dma_set_encrypted(dev, vaddr, size))
+	phys = page_to_phys(page);
+	swiotlb_pool = swiotlb_find_pool(dev, phys);
+	if (swiotlb_pool)
+		mark_mem_encrypted = false;
+
+	if (mark_mem_encrypted && dma_set_encrypted(dev, vaddr, size))
 		return;
-	__dma_direct_free_pages(dev, page, size);
+
+	if (swiotlb_pool)
+		swiotlb_free_from_pool(dev, phys, swiotlb_pool);
+	else
+		dma_free_contiguous(dev, page, size);
 }
 
 #if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \

kernel/dma/swiotlb.c +6 -0

index 6e8db52866bf..d54154c165e5 100644
@@ -1815,6 +1815,12 @@ bool swiotlb_free(struct device *dev, struct page *page, size_t size)
 	return true;
 }
 
+void swiotlb_free_from_pool(struct device *dev,
+		phys_addr_t tlb_addr, struct io_tlb_pool *pool)
+{
+	swiotlb_release_slots(dev, tlb_addr, pool);
+}
+
 static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
 				    struct device *dev)
 {

Signature · 1 line

2.43.0

DMA_ATTR_CC_SHARED describes an existing DMA mapping whose backing memory is already shared, or decrypted, for confidential computing. It is a mapping attribute: callers use it to request a shared DMA address encoding for memory that has already been prepared for shared DMA.

Allocation paths need a related but different state. Once the DMA core decides that an allocation must use shared backing pages, the lower-level allocation helpers need to select shared pools, decrypt newly allocated pages, derive the DMA address with the shared-memory translation and restore encryption on free. That state is internal to the DMA-mapping implementation and should not be passed by drivers to dma_alloc_attrs().

Reviewed-by Jason Gunthorpe <[email protected]>

Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>

---
 Documentation/core-api/dma-attributes.rst | 29 +++++++++++++++++++++++
 include/linux/dma-mapping.h               |  8 +++++++
 include/trace/events/dma.h                |  3 ++-
 3 files changed, 39 insertions(+), 1 deletion(-)

Documentation/core-api/dma-attributes.rst +29 -0

index 123c8468d58f..eee743184acd 100644
@@ -179,3 +179,32 @@ interface when building their uAPIs, when possible.
 
 It must never be used in an in-kernel driver that only works with
 kernel memory.
+
+DMA_ATTR_CC_SHARED
+------------------
+
+This attribute indicates that a DMA mapping is shared, or decrypted, for
+confidential computing guests. For normal system memory, the caller must
+already have marked the memory decrypted with set_memory_decrypted(). CPU
+PTEs for the mapping must use pgprot_decrypted(), and the same shared
+semantic may be passed to a vIOMMU when it sets up the IOPTE.
+
+This attribute describes an existing mapping. It does not allocate shared
+backing pages and must not be passed to dma_alloc_attrs(). For MMIO, use
+this together with DMA_ATTR_MMIO to indicate shared MMIO. Unless
+DMA_ATTR_MMIO is provided, the mapping requires a struct page.
+
+__DMA_ATTR_ALLOC_CC_SHARED
+--------------------------
+
+This is an internal DMA-mapping attribute for confidential computing guests.
+It is used by allocation paths after the DMA core has determined that the
+backing pages must be shared, or decrypted. For example, the direct DMA and
+SWIOTLB allocation paths use it to select shared DMA pools, decrypt newly
+allocated pages, derive DMA addresses using the shared-memory translation, and
+restore encryption on free.
+
+__DMA_ATTR_ALLOC_CC_SHARED differs from DMA_ATTR_CC_SHARED in that it is not
+a caller-visible DMA API attribute. DMA_ATTR_CC_SHARED describes an
+already-shared mapping and requires the caller to have prepared normal
+system memory before mapping it.

include/linux/dma-mapping.h +8 -0

index cc0823a99cfd..a3e880649fa4 100644
@@ -103,6 +103,14 @@
  */
 #define DMA_ATTR_CC_SHARED	(1UL << 13)
 
+/*
+ * __DMA_ATTR_ALLOC_CC_SHARED: Internal DMA-mapping attribute used by
+ * allocation paths that create shared (decrypted) backing pages for
+ * confidential computing guests. Drivers must not pass this attribute to
+ * dma_alloc_attrs().
+ */
+#define __DMA_ATTR_ALLOC_CC_SHARED	(1UL << 14)
+
 /*
  * A dma_addr_t can hold any valid DMA or bus address for the platform.  It can
  * be given to a device to use as a DMA source or target.  It is specific to a

include/trace/events/dma.h +2 -1

index 31c9ddf72c9d..9df02c1511de 100644
@@ -35,7 +35,8 @@ TRACE_DEFINE_ENUM(DMA_NONE);
 		{ DMA_ATTR_MMIO, "MMIO" }, \
 		{ DMA_ATTR_DEBUGGING_IGNORE_CACHELINES, "CACHELINES_OVERLAP" }, \
 		{ DMA_ATTR_REQUIRE_COHERENT, "REQUIRE_COHERENT" }, \
-		{ DMA_ATTR_CC_SHARED, "CC_SHARED" })
+		{ DMA_ATTR_CC_SHARED, "CC_SHARED" }, \
+		{ __DMA_ATTR_ALLOC_CC_SHARED, "ALLOC_CC_SHARED" })
 
 DECLARE_EVENT_CLASS(dma_map,
 	TP_PROTO(struct device *dev, phys_addr_t phys_addr, dma_addr_t dma_addr,

Signature · 1 line

2.43.0

Change swiotlb_tbl_map_single() to take the DMA mapping attributes by reference and update the direct callers accordingly.

This is a preparatory change for a follow-up patch which updates the attributes based on the selected swiotlb pool. Keeping the signature change separate makes the follow-up patch easier to review.

No functional change in this patch.

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Michael Kelley <[email protected]>

Tested-by Mostafa Saleh <[email protected]>

Reviewed-by Petr Tesarik <[email protected]>

Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>

---
 drivers/iommu/dma-iommu.c | 2 +-
 drivers/xen/swiotlb-xen.c | 2 +-
 include/linux/swiotlb.h   | 2 +-
 kernel/dma/swiotlb.c      | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

drivers/iommu/dma-iommu.c +1 -1

index 83bf15ae9777..c42e40e60575 100644
@@ -1180,7 +1180,7 @@ static phys_addr_t iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
 	trace_swiotlb_bounced(dev, phys, size);
 
 	phys = swiotlb_tbl_map_single(dev, phys, size, iova_mask(iovad), dir,
-			attrs);
+				      &attrs);
 
 	/*
 	 * Untrusted devices should not see padding areas with random leftover

drivers/xen/swiotlb-xen.c +1 -1

index 2cbf2b588f5b..8c4abe65cd49 100644
@@ -243,7 +243,7 @@ static dma_addr_t xen_swiotlb_map_phys(struct device *dev, phys_addr_t phys,
 	 */
 	trace_swiotlb_bounced(dev, dev_addr, size);
 
-	map = swiotlb_tbl_map_single(dev, phys, size, 0, dir, attrs);
+	map = swiotlb_tbl_map_single(dev, phys, size, 0, dir, &attrs);
 	if (map == (phys_addr_t)DMA_MAPPING_ERROR)
 		return DMA_MAPPING_ERROR;
 

include/linux/swiotlb.h +1 -1

index c92ff6791595..ea4c0a292dea 100644
@@ -238,7 +238,7 @@ static inline phys_addr_t default_swiotlb_limit(void)
 
 phys_addr_t swiotlb_tbl_map_single(struct device *hwdev, phys_addr_t phys,
 		size_t mapping_size, unsigned int alloc_aligned_mask,
-		enum dma_data_direction dir, unsigned long attrs);
+		enum dma_data_direction dir, unsigned long *attrs);
 dma_addr_t swiotlb_map(struct device *dev, phys_addr_t phys,
 		size_t size, enum dma_data_direction dir, unsigned long attrs);
 

kernel/dma/swiotlb.c +3 -3

index 908de28aceb2..046ae92c4832 100644
@@ -1396,7 +1396,7 @@ static unsigned long mem_used(struct io_tlb_mem *mem)
  */
 phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
 		size_t mapping_size, unsigned int alloc_align_mask,
-		enum dma_data_direction dir, unsigned long attrs)
+		enum dma_data_direction dir, unsigned long *attrs)
 {
 	struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
 	unsigned int offset;

@@ -1430,7 +1430,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
 	size = ALIGN(mapping_size + offset, alloc_align_mask + 1);
 	index = swiotlb_find_slots(dev, orig_addr, size, alloc_align_mask, &pool);
 	if (index == -1) {
-		if (!(attrs & DMA_ATTR_NO_WARN))
+		if (!(*attrs & DMA_ATTR_NO_WARN))
 			dev_warn_ratelimited(dev,
 	"swiotlb buffer is full (sz: %zd bytes), total %lu (slots), used %lu (slots)\n",
 				 size, mem->nslabs, mem_used(mem));

@@ -1609,7 +1609,7 @@ dma_addr_t swiotlb_map(struct device *dev, phys_addr_t paddr, size_t size,
 
 	trace_swiotlb_bounced(dev, phys_to_dma(dev, paddr), size);
 
-	swiotlb_addr = swiotlb_tbl_map_single(dev, paddr, size, 0, dir, attrs);
+	swiotlb_addr = swiotlb_tbl_map_single(dev, paddr, size, 0, dir, &attrs);
 	if (swiotlb_addr == (phys_addr_t)DMA_MAPPING_ERROR)
 		return DMA_MAPPING_ERROR;
 

Signature · 1 line

2.43.0

Quoted 7 lines · Aneesh Kumar K.V (Arm)

Change swiotlb_tbl_map_single() to take the DMA mapping attributes by reference and update the direct callers accordingly.

This is a preparatory change for a follow-up patch which updates the attributes based on the selected swiotlb pool. Keeping the signature change separate makes the follow-up patch easier to review.

I do not understand the point of this, why would swiotlb_tbl_map_single() force attrs over the caller which should already know the context (whether DMA is shared or not) which case would this be useful to force from the SWIOTLB code?

Quoted 9 lines · Mostafa Saleh

Quoted 7 lines · Aneesh Kumar K.V (Arm)

Change swiotlb_tbl_map_single() to take the DMA mapping attributes by reference and update the direct callers accordingly.

This is a preparatory change for a follow-up patch which updates the attributes based on the selected swiotlb pool. Keeping the signature change separate makes the follow-up patch easier to review.

Quoted 5 lines

I do not understand the point of this, why would swiotlb_tbl_map_single() force attrs over the caller which should already know the context (whether DMA is shared or not) which case would this be useful to force from the SWIOTLB code?

The mapping request was for private memory, but the device could not use it because the dma_capable() (force_dma_unencrypted() == true) check failed. As a result, dma_direct_map_phys() allocated a swiotlb bounce buffer, which is shared. Since the DMA now targets the shared bounce buffer rather than the original private memory, the attributes must be updated to reflect the new shared mapping.

dma_addr = phys_to_dma(dev, phys);
if (unlikely(!dma_capable(dev, dma_addr, size, true)))

	if (is_swiotlb_active(dev))
		return swiotlb_map(dev, phys, size, dir, attrs);

-aneesh

Quoted 24 lines · Aneesh Kumar K.V

Quoted 9 lines · Mostafa Saleh

Quoted 7 lines · Aneesh Kumar K.V (Arm)

Change swiotlb_tbl_map_single() to take the DMA mapping attributes by reference and update the direct callers accordingly.

This is a preparatory change for a follow-up patch which updates the attributes based on the selected swiotlb pool. Keeping the signature change separate makes the follow-up patch easier to review.

Quoted 5 lines

I do not understand the point of this, why would swiotlb_tbl_map_single() force attrs over the caller which should already know the context (whether DMA is shared or not) which case would this be useful to force from the SWIOTLB code?

The mapping request was for private memory, but the device could not use it because the dma_capable() (force_dma_unencrypted() == true) check failed. As a result, dma_direct_map_phys() allocated a swiotlb bounce buffer, which is shared. Since the DMA now targets the shared bounce buffer rather than the original private memory, the attributes must be updated to reflect the new shared mapping.

Quoted 6 lines

dma_addr = phys_to_dma(dev, phys);
if (unlikely(!dma_capable(dev, dma_addr, size, true)))

	if (is_swiotlb_active(dev))
		return swiotlb_map(dev, phys, size, dir, attrs);

But it does not make sense to me to have it in such a low level function, and it shouldn't change the attrs that was passed.

dma_direct_map_phys() is the right place to do this it should set the correct attrs before calling swiotlb_map().

There is a possibility that we may support io_tlb_mem with cc_shared = false in the future. As a result, only swiotlb_map() knows which type of bounce buffer was used, making it the only place where the attributes can be updated correctly.

Quoted 4 lines · Aneesh Kumar K.V

There is a possibility that we may support io_tlb_mem with cc_shared = false in the future. As a result, only swiotlb_map() knows which type of bounce buffer was used, making it the only place where the attributes can be updated correctly.

Yeah, +1, the attribute should be changed at the same effective place the source memory is changed away from what the DMA API user provided. Only the thing providing the new memory (eg swiotlb) should know its properties.

Jason

Quoted 10 lines · Jason Gunthorpe

Quoted 4 lines · Aneesh Kumar K.V

There is a possibility that we may support io_tlb_mem with cc_shared = false in the future. As a result, only swiotlb_map() knows which type of bounce buffer was used, making it the only place where the attributes can be updated correctly.

Yeah, +1, the attribute should be changed at the same effective place the source memory is changed away from what the DMA API user provided. Only the thing providing the new memory (eg swiotlb) should know its properties.

I will keep the conversation here instead of 2 threads.

That seems like a big leap, I'd be worried about devices that operate on confidential data that should not be shared/decrypted.

One example for this which exists in pKVM (this part is not upstream yet) is non coherent devices that require bouncing but they still want to keep the data private. In that case ideally they get an encrypted SWIOTLB pool, but it's always better to fail than to use a decrypted pool behind it's back.

I have not been following the work on T=1/T=0 devices, but IIRC, they required some complexity to handle their stage-2 as these modes will be emulated differently (for CCA, RMM vs untrusted host). I was thinking that it might be easier to represent those to the guest kernel as 2 separate devices (bounded to different groups...) where one is trusted and the other is not, and that way the DMA-API can have strict rules about memory sharing.

Otherwise, SWIOTLB does not seem like the right place to me, as it does not understand the context the device is operating in, and the DMA-API should deduce that from the flags passed.

Quoted 12 lines · Mostafa Saleh

Quoted 10 lines · Jason Gunthorpe

Quoted 4 lines · Aneesh Kumar K.V

There is a possibility that we may support io_tlb_mem with cc_shared = false in the future. As a result, only swiotlb_map() knows which type of bounce buffer was used, making it the only place where the attributes can be updated correctly.

Yeah, +1, the attribute should be changed at the same effective place the source memory is changed away from what the DMA API user provided. Only the thing providing the new memory (eg swiotlb) should know its properties.

Quoted 4 lines

I will keep the conversation here instead of 2 threads.

That seems like a big leap, I'd be worried about devices that operate on confidential data that should not be shared/decrypted.

That seems like something very differnt.

Quoted 5 lines

One example for this which exists in pKVM (this part is not upstream yet) is non coherent devices that require bouncing but they still want to keep the data private. In that case ideally they get an encrypted SWIOTLB pool, but it's always better to fail than to use a decrypted pool behind it's back.

We don't have any API for a DMA API caller to signal 'must be confidential'. If we want to add one it would be a flag to check before changing the physical address in swiotlb.

As defined today the DMA API expressly copies from private to shared memory, that is baked into it's design and not a bug.

My expectation is all these cases have to be solved by setting up the swiotlb properly so it provides suitable bounce buffers. Yes, this means we will eventually need both private and shared swiotlb pools.

Quoted 7 lines

I have not been following the work on T=1/T=0 devices, but IIRC, they required some complexity to handle their stage-2 as these modes will be emulated differently (for CCA, RMM vs untrusted host). I was thinking that it might be easier to represent those to the guest kernel as 2 separate devices (bounded to different groups...) where one is trusted and the other is not, and that way the DMA-API can have strict rules about memory sharing.

No way! That's would be a giant disaster for the driver model.

Each struct device will have a flag that shows if it is in T=1 or T=0 state. The flag can only change while a driver is not bound, and changing the flag will update the DMA API configuration.

A T=1 device should have the same issue as you point out for pkvm, it really needs to have a private SWIOTLB pool.

Quoted 3 lines

Otherwise, SWIOTLB does not seem like the right place to me, as it does not understand the context the device is operating in, and the DMA-API should deduce that from the flags passed.

It is exactly the right place because it is the one supplying the new memory. Context is irrelevant, the flag only describes what the new memory actually *IS*, and swiotlb knows 100% if the new memory is shared or private.

For your issue it would be appropriate to add a debugging check that the new memory is not incompatible with the expected policy. eg a T=1 device getting decrypted swiotlb memory is a bug in swiotlb setup that should be caught. But that's a debugging check, we expect the swiotlb to have selected the right kind of memory by construction. This isn't done yet since this series doesn't even yet support a T=1 device..

Jason

Quoted 3 lines

Each struct device will have a flag that shows if it is in T=1 or T=0 state. The flag can only change while a driver is not bound, and changing the flag will update the DMA API configuration.

I see we no longer seem to have the concern of devices in TDISP RUN state being able to send T=0 transactions other than MSI writes,

where GICv3 means the host is already going to have to be in on the game to some degree there anyway.

support in firmware bindings nor the IOMMU API itself to be able to describe a single device being associated with both the RMM's vSMMU for the T=1 (Realm) VA space, and the NS VMM's vSMMU for the T=0 VA space, even if we

did only have to care about using strictly one or the other at any given time based on device state. Having distinct struct devices is indeed clunky, but does at least reduce the problem to a currently-manageable one of the client driver having to wrangle them as a pair.

(And FWIW when we do get there, I'd be inclined to make the default SWIOTLB the "private" one, so all the sharing/decryption stuff can be neatly partitioned off into the CoCo etc. special cases...)

Quoted 6 lines · Robin Murphy

Quoted 3 lines

Each struct device will have a flag that shows if it is in T=1 or T=0 state. The flag can only change while a driver is not bound, and changing the flag will update the DMA API configuration.

I see we no longer seem to have the concern of devices in TDISP RUN state being able to send T=0 transactions other than MSI writes,

Right it was too complex and nobody needed it, so Linux will not support T=0 through the DMA API on an T=1 device, and devices should be designed to only send T=1 to the greatest degree possible.

If someone foolishly makes a device that works otherwise they will have to take the difficult SW tasks of making it work.

Quoted 2 lines

where GICv3 means the host is already going to have to be in on the game to some degree there anyway.

As I understand it:

We will need to teach GIC to understand if/when the device uses a T=0 translation for MSI and then use a shared physical address for the ITS IOVA instead of getting an IOVA translation from the iommu. The hypervisor will setup the S2 for the T=0 SMMU translation to be identity with all shared memory and the ITS page will be shared memory.

Presumably in future we will have HW to handle a T=1 ITS page access and some way to negotiate with devices if they should use a T=1 path for MSI or not.

There won't be a vSMMU attached to the T=0 instance at all, it is just wired to be bypass.

Quoted 4 lines

support in firmware bindings nor the IOMMU API itself to be able to describe a single device being associated with both the RMM's vSMMU for the T=1 (Realm) VA space, and the NS VMM's vSMMU for the T=0 VA space, even if we

Right!

Quoted 4 lines

did only have to care about using strictly one or the other at any given time based on device state. Having distinct struct devices is indeed clunky, but does at least reduce the problem to a currently-manageable one of the client driver having to wrangle them as a pair.

So hard! A meaningful goal here is to have existing drivers largely work unchanged in a CC world, so the device starts doing T=1 for almost everything and the existing driver just works.

Quoted 4 lines

(And FWIW when we do get there, I'd be inclined to make the default SWIOTLB the "private" one, so all the sharing/decryption stuff can be neatly partitioned off into the CoCo etc. special cases...)

Yeah

Jason

Quoted 6 lines

We will need to teach GIC to understand if/when the device uses a T=0 translation for MSI and then use a shared physical address for the ITS IOVA instead of getting an IOVA translation from the iommu. The hypervisor will setup the S2 for the T=0 SMMU translation to be identity with all shared memory and the ITS page will be shared memory.

IIRC, for MSI or unlocked MSI-X it should look and work pretty much exactly like regular VFIO, as everything can be mediated by the VMM and host kernel.

Quoted 3 lines

Presumably in future we will have HW to handle a T=1 ITS page access and some way to negotiate with devices if they should use a T=1 path for MSI or not.

Locked MSI-X would be a pain right now as we cannot intercept the Realm programming the MSI-X cap with a doorbell address and EventID value decided by the ITS driver in the Realm guest, so we'd somehow have to sniff those values out of the VMM's vITS emulation then try to configure an equivalent NS LPI to match, or have a hook in the ITS driver that knows when it's in a Realm and do some RSI handshake to proxy-allocate NS MSI vectors on the host and pass the real values back into the Realm.

I'm still hoping we (both CCA and possibly Linux in general) can get away with just refusing to support Locked MSI-X without GICv5, as once we have proper Realm MSIs with direct injection then all the problems go away (or at least become much smaller RMM problems that remain invisible to the host - the one "big" problem being that the RMM has to begrudgingly implement an entire GIC driver since it now has its own whole GIC to look after).

Quoted 2 lines

There won't be a vSMMU attached to the T=0 instance at all, it is just wired to be bypass.

If that's all anyone will ever want then it does allow some degree of hackery like mirroring the whole of Realm S2 in an equivalent NS IOMMUFD domain, then forcing IOMMU_DOMAIN_IDENTITY for unaccepted devices within the guest. However I was under the impression that folks want to make meaningful use of devices while still in their untrusted pre-acceptance state (but maybe still switch them later), so it seems almost inevitable that eventually someone says "actually, we would like S1 vSMMU for untrusted scatter-gather as well..."

Quoted 9 lines · Robin Murphy

Quoted 6 lines

We will need to teach GIC to understand if/when the device uses a T=0 translation for MSI and then use a shared physical address for the ITS IOVA instead of getting an IOVA translation from the iommu. The hypervisor will setup the S2 for the T=0 SMMU translation to be identity with all shared memory and the ITS page will be shared memory.

IIRC, for MSI or unlocked MSI-X it should look and work pretty much exactly like regular VFIO, as everything can be mediated by the VMM and host kernel.

Right

Quoted 11 lines

Quoted 3 lines

Presumably in future we will have HW to handle a T=1 ITS page access and some way to negotiate with devices if they should use a T=1 path for MSI or not.

Locked MSI-X would be a pain right now as we cannot intercept the Realm programming the MSI-X cap with a doorbell address and EventID value decided by the ITS driver in the Realm guest, so we'd somehow have to sniff those values out of the VMM's vITS emulation then try to configure an equivalent NS LPI to match, or have a hook in the ITS driver that knows when it's in a Realm and do some RSI handshake to proxy-allocate NS MSI vectors on the host and pass the real values back into the Realm.

Yes, we'd need to move to a model where the guest programs MSI directly and we learn the configuration required to emulate through the vGIC, not through MSI trapping. This broadly is the "righter" way to do interrupt routing but it will be hard to get there, if ever.

Quoted 6 lines

I'm still hoping we (both CCA and possibly Linux in general) can get away with just refusing to support Locked MSI-X without GICv5, as once we have proper Realm MSIs with direct injection then all the problems go away (or at least become much smaller RMM problems that remain invisible to the host - the one "big" problem being that the RMM has to begrudgingly implement an entire GIC driver since it now has its own whole GIC to look after).

Yeah, I'm fine with this, if GICv5 can allow direct MSI-X programming then great.

Devices are going to need some way to negotiate if MSI-X is locked, and if MSI is T=1 or not, I don't know if PCI has something for that already..

Quoted 11 lines

Quoted 2 lines

There won't be a vSMMU attached to the T=0 instance at all, it is just wired to be bypass.

If that's all anyone will ever want then it does allow some degree of hackery like mirroring the whole of Realm S2 in an equivalent NS IOMMUFD domain, then forcing IOMMU_DOMAIN_IDENTITY for unaccepted devices within the guest. However I was under the impression that folks want to make meaningful use of devices while still in their untrusted pre-acceptance state (but maybe still switch them later), so it seems almost inevitable that eventually someone says "actually, we would like S1 vSMMU for untrusted scatter-gather as well..."

So far almost all VMs today don't use vIOMMU at all, the ones that do are using it for things like PASID (or interrupt remapping on x86), not for translation. The cases where a vIOMMU is deliberately needed for translation seem to be mostly around SVA and PASID which isn't going to meaningfully work out of the box on a T=0 device.

My prediction is this is fine.

At least it is sufficiently hard to make two parallel vSMMU's controlling the same PCI device, and to make the ITS routing also somehow work right, that it isn't worth doing at this point when there is so much other more basic stuff to get done.

I imagine the ACPI might someday gain a description of the T=0 vSMMU in a way that is invisible to todays Linux and a future Linux could understand how to juggle the two iommu drivers for the same struct device, somehow.

Regards,
Jason

Cool. So in fact that puts us in an interesting position for now where non-CoCo "untrusted" (i.e. external) devices should have IOMMU translation forced on by default, while CoCo "unaccepted" devices (i.e. those which do have a mechanism to transition into a T=1 or equivalent state) should *not* try to use an associated IOMMU, on the assumption that it may only work for T=1 traffic. All the more reason to sort these abstractions out so we can make the right distinctions clearly :)

(And while untrusted vIOMMUs for purely-untrusted devices in CoCo environments would be pretty straightforward as well, I guess we might need some sort of acceptance status for trusted vIOMMU devices themselves? Hmm...)

Teach dma_capable() about DMA_ATTR_CC_SHARED so the capability check can reject encrypted DMA addresses for devices that require unencrypted/shared DMA.

Also propagate DMA_ATTR_CC_SHARED in swiotlb_map() when the selected SWIOTLB pool is decrypted so the capability check sees the correct DMA address attribute.

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Jiri Pirko <[email protected]>

Tested-by Michael Kelley <[email protected]>

Tested-by Mostafa Saleh <[email protected]>

Reviewed-by Petr Tesarik <[email protected]>

Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>

---
 arch/x86/kernel/amd_gart_64.c | 30 ++++++++++++++++--------------
 drivers/xen/swiotlb-xen.c     |  6 +++---
 include/linux/dma-direct.h    | 10 +++++++++-
 kernel/dma/direct.h           |  6 +++---
 kernel/dma/swiotlb.c          |  2 +-
 5 files changed, 32 insertions(+), 22 deletions(-)

arch/x86/kernel/amd_gart_64.c +16 -14

index e8000a56732e..b5f1f031d45b 100644
@@ -180,22 +180,23 @@ static void iommu_full(struct device *dev, size_t size, int dir)
 }
 
 static inline int
-need_iommu(struct device *dev, unsigned long addr, size_t size)
+need_iommu(struct device *dev, unsigned long addr, size_t size, unsigned long attrs)
 {
-	return force_iommu || !dma_capable(dev, addr, size, true);
+	return force_iommu || !dma_capable(dev, addr, size, true, attrs);
 }
 
 static inline int
-nonforced_iommu(struct device *dev, unsigned long addr, size_t size)
+nonforced_iommu(struct device *dev, unsigned long addr, size_t size,
+		unsigned long attrs)
 {
-	return !dma_capable(dev, addr, size, true);
+	return !dma_capable(dev, addr, size, true, attrs);
 }
 
 /* Map a single continuous physical area into the IOMMU.
  * Caller needs to check if the iommu is needed and flush.
  */
 static dma_addr_t dma_map_area(struct device *dev, dma_addr_t phys_mem,
-				size_t size, int dir, unsigned long align_mask)
+		size_t size, int dir, unsigned long align_mask, unsigned long attrs)
 {
 	unsigned long npages = iommu_num_pages(phys_mem, size, PAGE_SIZE);
 	unsigned long iommu_page;

@@ -206,7 +207,7 @@ static dma_addr_t dma_map_area(struct device *dev, dma_addr_t phys_mem,
 
 	iommu_page = alloc_iommu(dev, npages, align_mask);
 	if (iommu_page == -1) {
-		if (!nonforced_iommu(dev, phys_mem, size))
+		if (!nonforced_iommu(dev, phys_mem, size, attrs))
 			return phys_mem;
 		if (panic_on_overflow)
 			panic("dma_map_area overflow %lu bytes\n", size);

@@ -231,10 +232,10 @@ static dma_addr_t gart_map_phys(struct device *dev, phys_addr_t paddr,
 	if (unlikely(attrs & DMA_ATTR_MMIO))
 		return DMA_MAPPING_ERROR;
 
-	if (!need_iommu(dev, paddr, size))
+	if (!need_iommu(dev, paddr, size, attrs))
 		return paddr;
 
-	bus = dma_map_area(dev, paddr, size, dir, 0);
+	bus = dma_map_area(dev, paddr, size, dir, 0, attrs);
 	flush_gart();
 
 	return bus;

@@ -289,7 +290,7 @@ static void gart_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
 
 /* Fallback for dma_map_sg in case of overflow */
 static int dma_map_sg_nonforce(struct device *dev, struct scatterlist *sg,
-			       int nents, int dir)
+		int nents, int dir, unsigned long attrs)
 {
 	struct scatterlist *s;
 	int i;

@@ -301,8 +302,8 @@ static int dma_map_sg_nonforce(struct device *dev, struct scatterlist *sg,
 	for_each_sg(sg, s, nents, i) {
 		unsigned long addr = sg_phys(s);
 
-		if (nonforced_iommu(dev, addr, s->length)) {
-			addr = dma_map_area(dev, addr, s->length, dir, 0);
+		if (nonforced_iommu(dev, addr, s->length, attrs)) {
+			addr = dma_map_area(dev, addr, s->length, dir, 0, attrs);
 			if (addr == DMA_MAPPING_ERROR) {
 				if (i > 0)
 					gart_unmap_sg(dev, sg, i, dir, 0);

@@ -401,7 +402,7 @@ static int gart_map_sg(struct device *dev, struct scatterlist *sg, int nents,
 		s->dma_address = addr;
 		BUG_ON(s->length == 0);
 
-		nextneed = need_iommu(dev, addr, s->length);
+		nextneed = need_iommu(dev, addr, s->length, attrs);
 
 		/* Handle the previous not yet processed entries */
 		if (i > start) {

@@ -449,7 +450,7 @@ static int gart_map_sg(struct device *dev, struct scatterlist *sg, int nents,
 
 	/* When it was forced or merged try again in a dumb way */
 	if (force_iommu || iommu_merge) {
-		out = dma_map_sg_nonforce(dev, sg, nents, dir);
+		out = dma_map_sg_nonforce(dev, sg, nents, dir, attrs);
 		if (out > 0)
 			return out;
 	}

@@ -473,7 +474,8 @@ gart_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_addr,
 		return vaddr;
 
 	*dma_addr = dma_map_area(dev, virt_to_phys(vaddr), size,
-			DMA_BIDIRECTIONAL, (1UL << get_order(size)) - 1);
+				 DMA_BIDIRECTIONAL,
+				 (1UL << get_order(size)) - 1, attrs);
 	flush_gart();
 	if (unlikely(*dma_addr == DMA_MAPPING_ERROR))
 		goto out_free;

drivers/xen/swiotlb-xen.c +3 -3

index 8c4abe65cd49..e2538824ef52 100644
@@ -212,7 +212,7 @@ static dma_addr_t xen_swiotlb_map_phys(struct device *dev, phys_addr_t phys,
 	BUG_ON(dir == DMA_NONE);
 
 	if (attrs & DMA_ATTR_MMIO) {
-		if (unlikely(!dma_capable(dev, phys, size, false))) {
+		if (unlikely(!dma_capable(dev, phys, size, false, attrs))) {
 			dev_err_once(
 				dev,
 				"DMA addr %pa+%zu overflow (mask %llx, bus limit %llx).\n",

@@ -231,7 +231,7 @@ static dma_addr_t xen_swiotlb_map_phys(struct device *dev, phys_addr_t phys,
 	 * we can safely return the device addr and not worry about bounce
 	 * buffering it.
 	 */
-	if (dma_capable(dev, dev_addr, size, true) &&
+	if (dma_capable(dev, dev_addr, size, true, attrs) &&
 	    !dma_kmalloc_needs_bounce(dev, size, dir) &&
 	    !range_straddles_page_boundary(phys, size) &&
 		!xen_arch_need_swiotlb(dev, phys, dev_addr) &&

@@ -253,7 +253,7 @@ static dma_addr_t xen_swiotlb_map_phys(struct device *dev, phys_addr_t phys,
 	/*
 	 * Ensure that the address returned is DMA'ble
 	 */
-	if (unlikely(!dma_capable(dev, dev_addr, size, true))) {
+	if (unlikely(!dma_capable(dev, dev_addr, size, true, attrs))) {
 		__swiotlb_tbl_unmap_single(dev, map, size, dir,
 				attrs | DMA_ATTR_SKIP_CPU_SYNC,
 				swiotlb_find_pool(dev, map));

include/linux/dma-direct.h +1 -1

index 94fad4e7c11e..daa31a1adf7b 100644
@@ -135,12 +135,20 @@ static inline bool force_dma_unencrypted(struct device *dev)
 #endif /* CONFIG_ARCH_HAS_FORCE_DMA_UNENCRYPTED */
 
 static inline bool dma_capable(struct device *dev, dma_addr_t addr, size_t size,
-		bool is_ram)
+		bool is_ram, unsigned long attrs)
 {
 	dma_addr_t end = addr + size - 1;