Re: [PATCH 2/3] [media] venus: don't abuse dma_alloc for non-DMA allocations

[Arnd Bergmann]

On Tue, Jun 27, 2017 at 9:39 PM, Stanimir Varbanov
<stanimir.varbanov@xxxxxxxxxx> wrote:
> Hi Arnd,
>
> On 27.06.2017 18:02, Arnd Bergmann wrote:
>>
>> In venus_boot(), we pass a pointer to a phys_addr_t
>> into dmam_alloc_coherent, which the compiler warns about:
>>
>> platform/qcom/venus/firmware.c: In function 'venus_boot':
>> platform/qcom/venus/firmware.c:63:49: error: passing argument 3 of
>> 'dmam_alloc_coherent' from incompatible pointer type
>> [-Werror=incompatible-pointer-types]
>>
>> The returned DMA address is later passed on to a function that
>> takes a phys_addr_t, so it's clearly wrong to use the DMA
>> mapping interface here: the memory may be uncached, or the
>> address may be completely wrong if there is an IOMMU connected
>> to the device.
>>
>> My interpretation is that using dmam_alloc_coherent() had two
>> purposes:
>>
>>   a) get a chunk of consecutive memory that may be larger than
>>      the limit for kmalloc()
>>
>>   b) use the devres infrastructure to simplify the unwinding
>>      in the error case.
>
>
> The intension here is to use per-device memory which is removed from kernel
> allocator, that memory is used by remote processor (Venus) for its code
> section and system memory, the memory must not be mapped to kernel to avoid
> any cache issues.
>
> As the memory in subject is reserved per-device memory the only legal way to
> allocate it is by dmam_alloc_coherent() -> dma_alloc_from_coherent().
>
> For me the confusion comes from phys_addr_t which is passed to
> qcom_mdt_load() and then the address passed to qcom_scm_pas_mem_setup()
> which probably protects that physical memory. And the tz really expects
> physical address.
>
> The only solution I see is by casting dma_addr_t to phys_addr_t. Yes it is
> ugly but what is proper solution then?

If you actually have a separate remote processor that accesses this memory,
then qcom_mdt_load() is the wrong interface, as it takes a physical address,
and we need to introduce another interface that can take a DMA address
relative to a particular device.

You cannot cast between the two types because phys_addr_t is an address
as seen from the CPU, and dma_addr_t is seen by a particular device,
and can only be used together with that device pointer.

It looks like the pointer gets passed down to
qcom_scm_call(dev, QCOM_SCM_SVC_PIL,
QCOM_SCM_PAS_MEM_SETUP_CMD, ...), which in turn takes
a 32-bit address, suggesting that this is indeed a dma address for that
device (possibly going through an IOMMU), so maybe it just needs to
all be changed to dma_addr_t.

Is there any official documentation for qcom_scm_call() that clarifies
what address space the arguments are in?

        Arnd