Alexey Kardashevskiy <[email protected]> writes:
> On 16/5/26 22:53, Alexey Kardashevskiy wrote:
>> On 12/5/26 19:03, Aneesh Kumar K.V (Arm) wrote:
...
>>> -static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
>>> +static int atomic_pool_expand(struct dma_gen_pool *dma_pool, size_t
>>> pool_size,
>>> gfp_t gfp)
>>> {
>>> unsigned int order;
>>> @@ -113,12 +119,15 @@ static int atomic_pool_expand(struct gen_pool *pool,
>>> size_t pool_size,
>>> * Memory in the atomic DMA pools must be unencrypted, the pools do
>>> not
>>> * shrink so no re-encryption occurs in dma_direct_free().
>>> */
>>> - ret = set_memory_decrypted((unsigned long)page_to_virt(page),
>>> + if (dma_pool->unencrypted) {
>>> + ret = set_memory_decrypted((unsigned long)page_to_virt(page),
>>> 1 << order);
>>> - if (ret)
>>> - goto remove_mapping;
>>> - ret = gen_pool_add_virt(pool, (unsigned long)addr, page_to_phys(page),
>>> - pool_size, NUMA_NO_NODE);
>>> + if (ret)
>>> + goto remove_mapping;
>>> + }
>>> +
>>> + ret = gen_pool_add_virt(dma_pool->pool, (unsigned long)addr,
>>> + page_to_phys(page), pool_size, NUMA_NO_NODE);
>
>
> This clause could go to the else branch.
>
>
Can you clarify this better?
>>> if (ret)
>>> goto encrypt_mapping;
>>> @@ -126,11 +135,15 @@ static int atomic_pool_expand(struct gen_pool *pool,
>>> size_t pool_size,
>>> return 0;
>>> encrypt_mapping:
>>> - ret = set_memory_encrypted((unsigned long)page_to_virt(page),
>>> - 1 << order);
>>> - if (WARN_ON_ONCE(ret)) {
>>> - /* Decrypt succeeded but encrypt failed, purposely leak */
>>> - goto out;
>>> + if (dma_pool->unencrypted) {
>>> + int rc;
>>> +
>>> + rc = set_memory_encrypted((unsigned long)page_to_virt(page),
>>> + 1 << order);
>>> + if (WARN_ON_ONCE(rc)) {
>>> + /* Decrypt succeeded but encrypt failed, purposely leak */
>>> + goto out;
>>> + }
>>> }
>>> remove_mapping:
>>> #ifdef CONFIG_DMA_DIRECT_REMAP
>>> @@ -142,46 +155,52 @@ static int atomic_pool_expand(struct gen_pool *pool,
>>> size_t pool_size,
>>> return ret;
>>> }
...
>>> bool dma_free_from_pool(struct device *dev, void *start, size_t size)
>>> {
>>> - struct gen_pool *pool = NULL;
>>> + struct dma_gen_pool *dma_pool = NULL;
>>> +
>>> + while ((dma_pool = dma_guess_pool(dma_pool, 0))) {
>>> - while ((pool = dma_guess_pool(pool, 0))) {
>>> - if (!gen_pool_has_addr(pool, (unsigned long)start, size))
>>> + if (!gen_pool_has_addr(dma_pool->pool, (unsigned long)start, size))
>>
>>
>> v3 of this just crashed here with dma_pool!=NULL but dma_pool->pool==NULL.
>> continuing debugging... Thanks,
>
>
> dma_direct_free:
> dma_free_from_pool (loop over pools) -> false
> [here was a crash which I fixed by "if (!dma_pool->pool) continue"]
> swiotlb_find_pool (loop again) -> false
> __dma_direct_free_pages
> swiotlb_free
> swiotlb_find_pool (loop again)
> dma_free_contiguous => done.
>
> so that works but kinda hard to follow and there is some room for
> optimization. I do not normally have swiottlb when I test this and
> there is too many of this swiotlb stuff on the real direct dma mapping
> path imho. Thanks,
>
I will work on this in the next update. I can possibly drop the
swiotlb_find_pool from the swiotlb_free() path.
>>
>>
>>> continue;
>>> - gen_pool_free(pool, (unsigned long)start, size);
>>> +
>>> + gen_pool_free(dma_pool->pool, (unsigned long)start, size);
>>> return true;
>>> }
>>> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
>>> index 1abd3e6146f4..ab4eccbaa076 100644
>>> --- a/kernel/dma/swiotlb.c
>>> +++ b/kernel/dma/swiotlb.c
>>> @@ -612,6 +612,7 @@ static struct page *swiotlb_alloc_tlb(struct device
>>> *dev, size_t bytes,
>>> u64 phys_limit, gfp_t gfp)
>>> {
>>> struct page *page;
>>> + unsigned long attrs = 0;
>>> /*
>>> * Allocate from the atomic pools if memory is encrypted and
>>> @@ -623,8 +624,12 @@ static struct page *swiotlb_alloc_tlb(struct device
>>> *dev, size_t bytes,
>>> if (!IS_ENABLED(CONFIG_DMA_COHERENT_POOL))
>>> return NULL;
>>> + /* swiotlb considered decrypted by default */
>>> + if (cc_platform_has(CC_ATTR_MEM_ENCRYPT))
>>> + attrs = DMA_ATTR_CC_SHARED;
>>> +
>>> return dma_alloc_from_pool(dev, bytes, &vaddr, gfp,
>>> - dma_coherent_ok);
>>> + attrs, dma_coherent_ok);
>>> }
>>> gfp &= ~GFP_ZONEMASK;
>>
>
> --
> Alexey
-aneesh
