On Wed, Jun 10, 2026 at 01:37:26PM +0530, Aneesh Kumar K.V wrote: > Jason Gunthorpe <[email protected]> writes: > > > On Thu, Jun 04, 2026 at 02:09:43PM +0530, Aneesh Kumar K.V (Arm) wrote: > >> struct page *dma_alloc_from_pool(struct device *dev, size_t size, > >> - void **cpu_addr, gfp_t gfp, > >> + void **cpu_addr, gfp_t gfp, unsigned long attrs, > >> bool (*phys_addr_ok)(struct device *, phys_addr_t, size_t)) > >> { > >> - struct gen_pool *pool = NULL; > >> + struct dma_gen_pool *dma_pool = NULL; > >> struct page *page; > >> bool pool_found = false; > >> > >> - while ((pool = dma_guess_pool(pool, gfp))) { > >> + while ((dma_pool = dma_guess_pool(dma_pool, gfp))) { > >> + > >> + if (dma_pool->unencrypted != !!(attrs & DMA_ATTR_CC_SHARED)) > >> + continue; > > > > I don't think you should be overloading DMA_ATTR_CC_SHARED like this. > > > > /* > > * DMA_ATTR_CC_SHARED is not a caller-visible dma_alloc_*() > > * attribute. The direct allocator uses it internally after it has > > * decided that the backing pages must be shared/decrypted, so the > > * rest of the allocation path can consistently select DMA addresses, > > * choose compatible pools and restore encryption on free. > > */ > > if (attrs & DMA_ATTR_CC_SHARED) > > return NULL; > > > > if (force_dma_unencrypted(dev)) { > > attrs |= DMA_ATTR_CC_SHARED; > > mark_mem_decrypt = true; > > } > > > > It is fine to have a bit inside the attrs that is only used by the > > internal logic, but it needs to have a clearer name > > __DMA_ATTR_REQUIRE_CC_SHARED perhaps. > > > > Are you suggesting adding another attribute in addition to > DMA_ATTR_CC_SHARED? > > Is the idea that __DMA_ATTR_REQUIRE_CC_SHARED would be used in the > allocation path to request a CC_SHARED allocation, while > DMA_ATTR_CC_SHARED would be used in the mapping path to describe the > attribute of the address?
Yeah, it is a thought at least Maybe a comment is good enough. I just find it hard to follow when we have this dual usage. Like the code above for dma_pool->unencrypted is completely wrong if it is an "attribute of an address". Easy to cut & paste that into the wrong context. Especially if you move things up higher.. having the alloc set both CC_SHARED and REQUIRE_CC_SHARED or maybe ALLOC_CC_SHARED would make it clearer that the alloc code lives under that callchain Jason
