Re: [Xen-devel] arm64: dma_to_phys/phys_to_dma need to be properly implemented

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

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.

Thank you for helping,
Oleksandr

[1] 
https://01.org/linuxgraphics/gfx-docs/drm/drm-memory-management.html#drm-prime-support
[2] http://lxr.free-electrons.com/source/Documentation/dma-buf-sharing.txt
[3] 
http://lxr.free-electrons.com/source/drivers/gpu/drm/drm_prime.c?v=4.9#L766
[4] 
http://lxr.free-electrons.com/source/drivers/gpu/drm/drm_prime.c?v=4.9#L242
[5] https://patchwork.kernel.org/patch/9468093/
[6] https://patchwork.kernel.org/patch/9289859/


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