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

[Stefano Stabellini <sstabellini@kernel.org>]

Ok, no worries

On Fri, 31 Mar 2017, Oleksandr Andrushchenko wrote:
> 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