RE: [PATCH v5 05/20] dma-pool: track decrypted atomic pools and select them via attrs

[Michael Kelley]

From: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxx> Sent: Monday, June 1, 2026 11:05 PM
> 
> Michael Kelley <mhklinux@xxxxxxxxxxx> writes:
> 
> > From: Aneesh Kumar K.V (Arm) <aneesh.kumar@xxxxxxxxxx>Sent: Thursday, May 21, 2026 9:28 PM
> >>
> >> Teach the atomic DMA pool code to distinguish between encrypted and
> >> unencrypted pools, and make pool allocation select the matching pool based
> >> on DMA attributes.
> >>
> >> Introduce a dma_gen_pool wrapper that records whether a pool is
> >> unencrypted, initialize that state when the atomic pools are created, and
> >> use it when expanding and resizing the pools. Update dma_alloc_from_pool()
> >> to take attrs and skip pools whose encrypted state does not match
> >> DMA_ATTR_CC_SHARED. Update dma_free_from_pool() accordingly.
> >>
> >> Also pass DMA_ATTR_CC_SHARED from the swiotlb atomic allocation path so
> >> decrypted swiotlb allocations are taken from the correct atomic pool.
> >>
> >> Tested-by: Jiri Pirko <jiri@xxxxxxxxxx>
> >> Reviewed-by: Mostafa Saleh <smostafa@xxxxxxxxxx>
> >> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@xxxxxxxxxx>
> >> ---
> >>  drivers/iommu/dma-iommu.c   |   2 +-
> >>  include/linux/dma-map-ops.h |   2 +-
> >>  kernel/dma/direct.c         |  11 ++-
> >>  kernel/dma/pool.c           | 167 +++++++++++++++++++++++-------------
> >>  kernel/dma/swiotlb.c        |   7 +-
> >>  5 files changed, 123 insertions(+), 66 deletions(-)
> >>
> >
> > [snip]
> >
> >> +static __init struct dma_gen_pool *__dma_atomic_pool_init(struct dma_gen_pool *dma_pool,
> >> +		size_t pool_size, gfp_t gfp)
> >>  {
> >> -	struct gen_pool *pool;
> >>  	int ret;
> >>
> >> -	pool = gen_pool_create(PAGE_SHIFT, NUMA_NO_NODE);
> >> -	if (!pool)
> >> +	dma_pool->pool = gen_pool_create(PAGE_SHIFT, NUMA_NO_NODE);
> >> +	if (!dma_pool->pool)
> >>  		return NULL;
> >>
> >> -	gen_pool_set_algo(pool, gen_pool_first_fit_order_align, NULL);
> >> +	gen_pool_set_algo(dma_pool->pool, gen_pool_first_fit_order_align, NULL);
> >> +
> >> +	/* if platform is using memory encryption atomic pools are by default decrypted. */
> >> +	if (cc_platform_has(CC_ATTR_MEM_ENCRYPT))
> >> +		dma_pool->unencrypted = true;
> >> +	else
> >> +		dma_pool->unencrypted = false;
> >
> > I'm curious about the name of the "unencrypted" field in struct dma_gen_pool,
> > and similarly in Patch 7 of the series for the swiotlb struct io_tlb_pool and
> > struct io_tlb_mem. Up through v3 of this series, you used "decrypted", but
> > starting in v4 switched to "unencrypted".
> >
> > To me, the above "if" statement has some cognitive dissonance in that if
> > CC_ATTR_MEM_ENCRYPT is false (i.e., a normal VM), "unencrypted" is set
> > to false. But I think of memory in a normal VM as "unencrypted" since it
> > was never encrypted. A similar "if" statement occurs in your swiotlb changes.
> >
> > Two related concepts are captured by the field:
> > 1) Is some action needed to put the memory into the unencrypted state,
> > and to remove it from that state? This applies when assigning memory to the
> > pool, or freeing the memory in the pool.
> > 2) Is the memory currently in the unencrypted state? This applies when
> > allocating memory from the pool to a caller.
> >
> > It's hard to capture all that in a short field name. But I think I prefer "decrypted"
> > over "unencrypted".  The former implies that some action was taken. It's a
> > little easier to think of a normal VM as *not* having decrypted memory. The
> > memory was never encrypted in the first place, so no decryption action was taken.
> >
> > Throughout the kernel, "decrypted" occurs much more frequently than
> > "unencrypted".  We have set_memory_encrypted() and set_memory_decrypted()
> > that are "take action" names.  But we also have force_dma_unencrypted(),
> > phys_to_dma_unencrypted(), and dma_addr_unencrypted(). So it's a bit
> > of a mess.
> >
> >
> > But maybe there's more background here that led to the change
> > between your v3 and v4.
> >
> > Michael
> 
> The current APIs, phys_to_dma_unencrypted() and dma_addr_unencrypted(),
> are the reason I changed the pool attribute name from decrypted to
> unencrypted. The rationale was that nobody actually decrypted the
> memory; the memory was already in an unencrypted state.
> 
> In other words, the DMA pool did not contain encrypted content that was
> later decrypted. Rather, the DMA pool itself was in an unencrypted
> state.
> 
> IMHO, set_memory_decrypted()/set_memory_encrypted() is the right naming
> because those APIs describe an operation that transitions memory between
> states. In contrast, the pool attribute describes the state of the
> memory itself, which is why I used unencrypted rather than decrypted.
>

Except that in a normal VM, the "unencrypted" pool attribute does *not*
describe the state of the memory itself.  In a normal VM, the memory is
unencrypted, but the "unencrypted" pool attribute is false. That
contradiction is the essence of my concern.

Michael