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

[Oleksandr Andrushchenko <andr2000@gmail.com>]

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)

2. Duplicate it:
dma_get_sgtable(..., sgt, ... dev_bus_addr,...);
                               ^^^^^^^^^^^^
which finally lands at
__swiotlb_get_sgtable(..., dma_addr_t handle,...)
{
     ...
     sg_set_page(..., phys_to_page(dma_to_phys(dev, handle)),...);
                                   ^^^^^^^^^^^      ^^^^^^
     ...
}
At this stage sg table's page_link points to bad page, because
of linear translation of dma_to_phys, e.g. paddr == dev_bus_addr

3. Map the duplicated sg table:
dma_map_sg(..., sgt->sgl,...);
     xen_swiotlb_map_sg_attrs(..., struct scatterlist *sgl,...)
     {
         phys_addr_t paddr = sg_phys(sg);
                             ^^^^^^^
         dma_addr_t dev_addr = xen_phys_to_bus(paddr);
/*
the translation (sg_phys) above does:
         page_to_phys(...sg->page_link & ~0x3...) + ...;
so after this step we have WRONG paddr and correct dev_addr (because
wrong PFN is not found in P2M table, so it is used as is
*/
4. Now map segments of the table:
__swiotlb_map_page(..., struct page *page,...)
{
     ...
     dev_addr = swiotlb_map_page(dev, page, offset, size, dir, attrs);
     if (!is_device_dma_coherent(dev))
         __dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)),...);
                                     ^^^^^^^^^^^      ^^^^^^^^
     ...
}

5. Crash happens at
__dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)),...);
with
Unable to handle kernel paging request at virtual address
because dev_addr was NOT translated to correct paddr by dma_to_phys

Hope, this better explains why I am talking about dma_to_phys

So, I'm curious if we need to change dma_to_phys/phys_to_dma
or hide this translation in Xen specific code.
Another question is that for phys_to_dma we have __pfn_to_mfn,
but there is no reverse translation , e.g. __mfn_to_pfn
> Cheers,
>
> Stefano
Thank you,
Oleksandr
> On Mon, 27 Mar 2017, Oleksandr Andrushchenko wrote:
>> Hi, all!
>>
>> First of all this could be a generic ARM64 question, but
>> only(?) Xen users will suffer.
>>
>> At the moment for ARM64 dma_to_phys/phys_to_dma both assume that
>> MFN == PFN and though those do not do MFN <-> PFN conversion.
>> In case if MFN is mapped from other domain, then
>> MFN != PFN and the above assumption is not true any more and
>> proper MFN to PFN and vice versa must be implemented.
>>
>> Use-case:
>> 1. Get gref_u in DomU (PFN_u/MFN_u) and pass to Dom0
>> 2. Balloon out a page PFN_0
>> 3. Map gref_u onto PFN_0
>> 4. Now PFN_0 is mapped to MFN_u and PFN_0 != MFN_u
>>
>> If pair PFN_0/MFN_u is about to be used for example for
>> DMA sg table (which is my case), then conversion
>> dma_to_phys(MFN_u << PAGE_SHIFT) results in wrong PFN_bad,
>> not usable by CPU anymore because PFN_0 is expected instead
>>
>> Is there any reason for ARM64's dma_to_phys/phys_to_dma to be
>> not implemented?
>>
>> Thank you,
>>
>> Oleksandr
>>
>>
>> On 03/23/2017 05:40 PM, Oleksandr Andrushchenko wrote:
>>> Hi, all!
>>>
>>> I am trying to implement a zero-copy scenario for DRM front/back,
>>> e.g. buffers allocated by DomU in the DRM frontend used as is
>>> by Dom0. The requirement I have is that the buffer is contiguous.
>>>
>>> So, what I currently have is:
>>> 1. DomU is 1:1 mapped and is able to allocate physically contiguous
>>> buffer, e.g. PFNs and MFNs are sequential.
>>> 2. On Dom0 side I allocate ballooned pages (because I need reservation)
>>> and am able to map those (gnttab_map_refs with grefs from DomU):
>>> pfn 52a35 dev_bus_addr 6c100000
>>> pfn 52a34 dev_bus_addr 6c101000
>>> ...
>>> pfn 52a2c dev_bus_addr 6c109000
>>>
>>> 3. Then I have to create an SG table from these, so I can pass the buffer
>>> to real HW DRM driver: as you can see from the above PFNs they are not
>>> sequential as those were allocated from the balloon. Thus, sg_table
>>> is also has number of segments != 1 which is not ok for my use-case.
>>>
>>> Still, if I do __pfn_to_mfn(page_to_pfn(balloon_page[i])) I get the
>>>
>>> correct MFN, which is expected because p2m translation happens.
>>>
>>> 4. If I try to do a hack:
>>> dev_bus_pages[i] = pfn_to_page(MFN[i]) and then create an SG table from
>>> these pages then of course the sg table is configuous and
>>> I can share the SG with HW DRM driver.
>>> What is naturally happens next is:
>>> "Unable to handle kernel paging request at virtual address ffff80002c100000"
>>> [   86.263197] PC is at __clean_dcache_area_poc+0x20/0x40
>>> [   86.268377] LR is at __swiotlb_map_page+0x80/0x98
>>> for the very first maddr == 6c100000, because my dev_bus_addr[i] and
>>> dev_bus_pages[i] have no translation set up.
>>>
>>> So, the question: is there any way I can make those ballooned pages
>>> to convert into contiguous scatter-gather? So, sg table consists
>>> of a single entry?
>>>
>>> I was thinking of:
>>> 1. Is it possible to update translation manually so dev_bus_addr[i]
>>> has corresponding continuous pages, e.g. so I can do
>>> dev_bus_pages[i] = pfn_to_page(MFN[i]) and CPU can access those?
>>> 2. I can of course allocate contiguous buffer in Dom0 and
>>> manually (unfortunately there is no API to do that) increase/decrease
>>> reservation for the pages as balloon driver does and use those for mapping.
>>> This way MFN will follow PFN (we are in Dom0 which is 1:1).
>>> But this is going to be clumsy, as I'll have to copy-paste part
>>> of the balloon driver into mine (decrease_reservation/increase_reservation)
>>> 3. Any other neat solution?
>>>
>>> Thank you,
>>> Oleksandr


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