Re: [RFC PATCH 1/4] dma-buf: heaps: restricted_heap: add no_map attribute

["Christian König" <christian.koenig@amd.com>]

Am 05.09.24 um 08:56 schrieb Jens Wiklander:
> On Fri, Aug 30, 2024 at 10:47 AM Christian König
> <christian.koenig@amd.com> wrote:
>> Am 30.08.24 um 09:03 schrieb Jens Wiklander:
>>> Add a no_map attribute to struct restricted_heap_attachment and struct
>>> restricted_heap to skip the call to dma_map_sgtable() if set. This
>>> avoids trying to map a dma-buf that doens't refer to memory accessible
>>> by the kernel.
>> You seem to have a misunderstanding here dma_map_sgtable() is called to
>> map a table into IOMMU and not any kernel address space.
>>
>> So please explain why you need that?
> You're right, I had misunderstood dma_map_sgtable(). There's no need
> for the no_map attribute, so I'll remove it.
>
> Perhaps you have a suggestion on how to fix a problem when using
> dma_map_sgtable()?
>
> Without it, I'll have to assign a pointer to teedev->dev.dma_mask when
> using the restricted heap with the OP-TEE driver or there will be a
> warning in __dma_map_sg_attrs() ending with a failure when trying to
> register the dma-buf fd. OP-TEE is probed with a platform device, and
> taking the dma_mask pointer from that device works. Is that a good
> approach or is there a better way of assigning dma_mask?

Mhm, I don't know the full picture so I have to make some assumptions.

The teedev is just a virtual device which represents the restricted 
memory access paths of a real device?

If that is true then taking the dma-mask from the real device is most 
likely the right thing to do.

Regards,
Christian.

>
> Thanks,
> Jens
>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
>>> ---
>>>    drivers/dma-buf/heaps/restricted_heap.c | 17 +++++++++++++----
>>>    drivers/dma-buf/heaps/restricted_heap.h |  2 ++
>>>    2 files changed, 15 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/heaps/restricted_heap.c b/drivers/dma-buf/heaps/restricted_heap.c
>>> index 8bc8a5e3f969..4bf28e3727ca 100644
>>> --- a/drivers/dma-buf/heaps/restricted_heap.c
>>> +++ b/drivers/dma-buf/heaps/restricted_heap.c
>>> @@ -16,6 +16,7 @@
>>>    struct restricted_heap_attachment {
>>>        struct sg_table                 *table;
>>>        struct device                   *dev;
>>> +     bool no_map;
>>>    };
>>>
>>>    static int
>>> @@ -54,6 +55,8 @@ restricted_heap_memory_free(struct restricted_heap *rheap, struct restricted_buf
>>>    static int restricted_heap_attach(struct dma_buf *dmabuf, struct dma_buf_attachment *attachment)
>>>    {
>>>        struct restricted_buffer *restricted_buf = dmabuf->priv;
>>> +     struct dma_heap *heap = restricted_buf->heap;
>>> +     struct restricted_heap *rheap = dma_heap_get_drvdata(heap);
>>>        struct restricted_heap_attachment *a;
>>>        struct sg_table *table;
>>>
>>> @@ -70,6 +73,7 @@ static int restricted_heap_attach(struct dma_buf *dmabuf, struct dma_buf_attachm
>>>        sg_dma_mark_restricted(table->sgl);
>>>        a->table = table;
>>>        a->dev = attachment->dev;
>>> +     a->no_map = rheap->no_map;
>>>        attachment->priv = a;
>>>
>>>        return 0;
>>> @@ -92,9 +96,12 @@ restricted_heap_map_dma_buf(struct dma_buf_attachment *attachment,
>>>        struct sg_table *table = a->table;
>>>        int ret;
>>>
>>> -     ret = dma_map_sgtable(attachment->dev, table, direction, DMA_ATTR_SKIP_CPU_SYNC);
>>> -     if (ret)
>>> -             return ERR_PTR(ret);
>>> +     if (!a->no_map) {
>>> +             ret = dma_map_sgtable(attachment->dev, table, direction,
>>> +                                   DMA_ATTR_SKIP_CPU_SYNC);
>>> +             if (ret)
>>> +                     return ERR_PTR(ret);
>>> +     }
>>>        return table;
>>>    }
>>>
>>> @@ -106,7 +113,9 @@ restricted_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_t
>>>
>>>        WARN_ON(a->table != table);
>>>
>>> -     dma_unmap_sgtable(attachment->dev, table, direction, DMA_ATTR_SKIP_CPU_SYNC);
>>> +     if (!a->no_map)
>>> +             dma_unmap_sgtable(attachment->dev, table, direction,
>>> +                               DMA_ATTR_SKIP_CPU_SYNC);
>>>    }
>>>
>>>    static int
>>> diff --git a/drivers/dma-buf/heaps/restricted_heap.h b/drivers/dma-buf/heaps/restricted_heap.h
>>> index 7dec4b8a471b..94cc0842f70d 100644
>>> --- a/drivers/dma-buf/heaps/restricted_heap.h
>>> +++ b/drivers/dma-buf/heaps/restricted_heap.h
>>> @@ -27,6 +27,8 @@ struct restricted_heap {
>>>        unsigned long           cma_paddr;
>>>        unsigned long           cma_size;
>>>
>>> +     bool                    no_map;
>>> +
>>>        void                    *priv_data;
>>>    };
>>>