Re: arm64: dma_to_phys/phys_to_dma need to be properly implemented

[Oleksandr Andrushchenko <andr2000@gmail.com>]

Hi, Stefano

Unfortunately I had to switch to other tasks,

but I'll get back to this issue asap

Thank you


On 03/30/2017 01:36 AM, Stefano Stabellini wrote:
> On Wed, 29 Mar 2017, Oleksandr Andrushchenko wrote:
>> Hi, Stefano!
>>
>> Ok, probably I need to put more details into the use-case
>> so it is clear. What I am doing is a DRM driver which
>> utilizes PRIME buffer sharing [1] to implement zero-copy
>> of display buffers between DomU and Dom0. PRIME is based on
>> DMA Buffer Sharing API [2], so this is the reason I am
>> dealing with sg_table here.
>>
>> On 03/28/2017 10:20 PM, Stefano Stabellini wrote:
>>> On Tue, 28 Mar 2017, Oleksandr Andrushchenko wrote:
>>>> Hi, Stefano!
>>>>
>>>> On 03/27/2017 11:23 PM, Stefano Stabellini wrote:
>>>>> Hello Oleksandr,
>>>>>
>>>>> Just to clarify, you are talking about dma_to_phys/phys_to_dma in Linux
>>>>> (not in Xen), right?
>>>> I am talking about Linux, sorry I was not clear
>>>>> Drivers shouldn't use those functions directly (see the comment in
>>>>> arch/arm64/include/asm/dma-mapping.h), they should call the appropriate
>>>>> dma_map_ops functions instead.
>>>> Yes, you are correct and I know about this and do not call
>>>> dma_to_phys/phys_to_dma directly
>>>>>     The dma_map_ops functions should do the
>>>>> right thing on Xen, even for pages where pfn != mfn, thanks to the
>>>>> swiotlb-xen (drivers/xen/swiotlb-xen.c). Specifically, you can see the
>>>>> special case for pfn != mfn here (see the "local" variable):
>>>>>
>>>>> arch/arm/include/asm/xen/page-coherent.h:xen_dma_map_page
>>>> Yes, the scenarios with pfn != mfn we had so
>>>> far are all working correct
>>>>> So, why are you calling dma_to_phys and phys_to_dma instead of the
>>>>> dma_map_ops functions?
>>>> Ok, let me give you an example of failing scenario which
>>>> was not used before this by any backend (this is from
>>>> my work on implementing zero copy for DRM drivers):
>>>>
>>>> 1. Create sg table from pages:
>>>> sg_alloc_table_from_pages(sgt, PFN_0, ...);
>>>> map it and get dev_bus_addr - at this stage sg table is
>>>> perfectly correct and properly mapped,
>>>> dev_bus_addr == (MFN_u << PAGE_SHIFT)
>>> Let me get this straight: one of the pages passed to
>>> sg_alloc_table_from_pages is actually a foreign page (pfn != mfn), is
>>> that right?
>>>
>>> And by "map", you mean dma_get_sgtable_attrs is called on it, right?
>> What happening here is:
>> ------------- my driver ------------
>> 1. I create an sg table from pages with pfn != mfn (PFN_0/MFN_u)
>> using drm_prime_pages_to_sg [3] which effectively is
>> sg_alloc_table_from_pages
>> ------------- DRM framework ------------
>> 2. I pass the sgt via PRIME to the real display driver
>> and it does drm_gem_map_dma_buf [4]
>> 3. So, at this point everyting is just fine, because sgt is
>> correct (sgl->page_link points to my PFN_0 and p2m translation
>> succeeds)
>> ------------- real HW DRM driver ------------
>> 4. When real HW display driver accesses sgt it calls dma_get_sgtable
>> [5] and then dma_map_sg [6]. And all this is happening on the sgt
>> which my driver has provided, but PFN_0 is not honored anymore
>> because dma_get_sgtable is expected to be able to figure out
>> pfn from the corresponding DMA address.
>>
>> So, strictly speaking real HW DRM driver has no issues,
>> the API it uses is perfectly valid.
>>>> 2. Duplicate it:
>>>> dma_get_sgtable(..., sgt, ... dev_bus_addr,...);
>>> Yeah, if one of the pages passed to sg_alloc_table_from_pages is
>>> foreign, as Andrii pointed out, dma_get_sgtable
>>> (xen_swiotlb_get_sgtable) doesn't actually work.
>> This is the case
>>> Is it legitimate that one of those pages is foreign or is it a mistake?
>> This is the goal - I want pages from DomU to be directly
>> accessed by the HW in Dom0 (I have DomU 1:1 mapped,
>> so even contiguous buffers can come from DomU, if not
>> 1:1 then IPMMU will be in place)
>>> If it is a mistake, you could fix it.
>>  From the above - this is the intention
>>>    Otherwise, if the use of
>>> sg_alloc_table_from_pages or the following call to dma_get_sgtable are
>>> part of your code, I suggest you work-around the problem by avoiding
>>> the dma_get_sgtable call altogether.
>> As seen from the above the problematic part is not in my
>> driver, it is either DRM framework or HW display driver
>>>    Don't use the sg_ dma api, use the
>>> regular dma api instead.
>> I use what DRM provides and dma_xxx if something is missed
>>>
>>> Unfortunately, if the dma_get_sgtable is part of existing code, then we
>>> have a problem. In that case, could you point me to the code that call
>>> dma_get_sgtable?
>> This is the case, see [5]
>>> There is no easy way to make it work on Xen: virt_to_phys doesn't work
>>> on ARM and dma_to_phys doesn't work on Xen. We could implement
>>> xen_swiotlb_get_sgtable correctly if we had the physical address of the
>>> page, because we could easily find out if the page is local or foreign
>>> with a pfn != mfn check (similar to the one in
>>> include/xen/arm/page-coherent.h:xen_dma_map_page).
>> Yes, I saw this code and it helped me to figure out
>> where the use-case fails
>>> If the page is local, then we would do what we do today. If the page is
>>> foreign, then we would need to do something like:
>>>
>>>       int ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
>>>
>>> 	if (!ret) {
>>> 		sg_set_page(sgt->sgl, phys_to_page(phys), PAGE_ALIGN(size),
>>> 0);
>>> 		sgt->sgl->dma_address = dev_addr;
>>> 	}
>> Agree, the crucial part here phys_to_page(phys): we need mfn->pfn
>>> Now the question is, how do we get the pseudo-physical address of the
>>> page?
>> yes, this is the root of the problem
>>> We could parse the stage1 page table entry of the kernel, or we
>>> could use the "at" instruction (see for example gva_to_ipa_par in xen).
>>> It is a bit rough, but I cannot think of anything else.
>> Me neither, this is why I hope community will help here...
>> Otherwise I'll need to patch kernel drivers if it's still possible.
> If you can come up with a patch that only affects
> xen_swiotlb_get_sgtable, and translates successfully void *cpu_addr into
> a physical address using "at", I think I would take that patch. I would
> recommend to test the patch on ARM32 too, where virt_to_phys reliably
> fails.
>
> I don't think we can ask the community to drop dma_get_sgtable on the
> basis that the DMA api is broken.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel