Re: [PATCH v4 03/13] dma-pool: track decrypted atomic pools and select them via attrs

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Alexey Kardashevskiy <aik@amd.com> 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