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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel