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

Mostafa Saleh
2026-04-17T15:45:17Z

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