Re: [RFC PATCH v3 4/5] dma-mapping: Encapsulate memory state during allocation

[Mostafa Saleh <smostafa@google.com>]

On Fri, Apr 10, 2026 at 03:05:04PM -0300, Jason Gunthorpe wrote:
> On Wed, Apr 08, 2026 at 07:47:41PM +0000, Mostafa Saleh wrote:
> > Introduce a new dma-direct internal type dma_page which is
> > "struct page" and a bit indicate whether the memory has been decrypted
> > or not.
> > This is useful to pass such information encapsulated through
> > allocation functions, which is currently set from swiotlb_alloc().
> > 
> > No functional changes.
> > 
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> >  kernel/dma/direct.c | 58 +++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 46 insertions(+), 12 deletions(-)
> > 
> > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> > index de63e0449700..204bc566480c 100644
> > --- a/kernel/dma/direct.c
> > +++ b/kernel/dma/direct.c
> > @@ -16,6 +16,33 @@
> >  #include <linux/pci-p2pdma.h>
> >  #include "direct.h"
> >  
> > +/*
> > + * Represent DMA allocation and 1 bit flag for it's state
> > + */
> 
> I'd explain this wrappers a pointer and uses the low PAGE_SHIFT bits
> for flags..
> 
> > +struct dma_page {
> > +	unsigned long val;
> 
> unintptr_t ?

I thought about that, but I don’t see unintptr_t anywhere in the
kernel, it seems similar cases use “unsigned long” as in xarray.h

> 
> > @@ -103,20 +130,21 @@ static void __dma_direct_free_pages(struct device *dev, struct page *page,
> >  	dma_free_contiguous(dev, page, size);
> >  }
> >  
> > -static struct page *dma_direct_alloc_swiotlb(struct device *dev, size_t size)
> > +static struct dma_page dma_direct_alloc_swiotlb(struct device *dev, size_t size)
> >  {
> > -	struct page *page = swiotlb_alloc(dev, size, NULL);
> > +	enum swiotlb_page_state state;
> > +	struct page *page = swiotlb_alloc(dev, size, &state);
> >  
> >  	if (page && !dma_coherent_ok(dev, page_to_phys(page), size)) {
> >  		swiotlb_free(dev, page, size);
> > -		return NULL;
> > +		return DMA_PAGE_NULL;
> >  	}
> >  
> > -	return page;
> > +	return page_to_dma_page(page, state == SWIOTLB_PAGE_DECRYPTED);
> 
> Should the struct dma_page have been introduced earlier instead of the
> swiotlb_page_state ? Seems a bit odd to have both

It can be introduced earlier, but It looked cleaner to decouple swiotlb
from direct-dma and keep the dma_page type completely internal.

> 
> If these are actually internally allocated struct pages, could you use
> the struct page memory itself to record the decrypted state? That
> would require more significant changes to the allocator calls.
> 
> > @@ -184,9 +212,11 @@ static void *dma_direct_alloc_from_pool(struct device *dev, size_t size,
> >  static void *dma_direct_alloc_no_mapping(struct device *dev, size_t size,
> >  		dma_addr_t *dma_handle, gfp_t gfp)
> >  {
> > +	struct dma_page dma_page;
> >  	struct page *page;
> >  
> > -	page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO, true);
> > +	dma_page = __dma_direct_alloc_pages(dev, size, gfp & ~__GFP_ZERO, true);
> > +	page = dma_page_to_page(dma_page);
> >  	if (!page)
> >  		return NULL;
> 
> I would expect to see more usage of the dma_page here..
> 
> Like I don't think this is really right:
> 
>         *dma_handle = phys_to_dma_direct(dev, page_to_phys(page));
> 
> Does page_to_phys(page) really work on decrypted memory? On CCA it
> will return the protected alias which doesn't seem like something
> useful?
>

Not sure, but that’s not related to this patch, that’s already the
status quo, I can look more into it.

> static inline dma_addr_t phys_to_dma_direct(struct device *dev,
>                 phys_addr_t phys)
> {
>         if (force_dma_unencrypted(dev))
>                 return phys_to_dma_unencrypted(dev, phys);
>         return phys_to_dma(dev, phys);
> 
> Above is all nonsense now that you have a direct indication of the
> address is decrypted memory or not, it should also be used right here
> directly.
> 
> if (is_dma_page_decrypted(dma_page))
>    *dma_handle = phys_to_dma_unencrypted(..)
> else
>    *dma_handle = phys_to_dma(..);
> 
> The later patch just makes it worse by adding even more confusing
> flags to phys_to_dma_direct().
> 
> I think it should work out that everyone already knows what memory
> type they are working with before they call down to
> phys_to_dma_direct() - the calls to force_dma_unecrypted() here are
> just hacks because it previously did not.
> 
> Anyhow, I think this series is alot better than the previous one. If
> you work a little harder to make it so there is only one
> force_dma_unecrypted() per high level DMA API call that would be
> perfect.

I see, that makes a lot of sense.

Thanks,
Mostafa

> 
> Jason