Re: [Linux] [ARM] Granting memory obtained from the DMA API

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 20 Aug 2020, Julien Grall wrote:
> > Part of virtio is having shared memory. So naturally, I'm using Xen's
> > grant system for that. Part of the Xenbus client API is the function
> > xenbus_grant_ring which, by its documentation grants access to a block
> > of memory starting at vaddr to another domain. I tried using this in my
> > driver which created the grants and returned without any error, but
> > after mounting the grants on another domain, it turns out that some
> > other location in memory was actually granted instead of the one behind
> > the original vaddr.
> > 
> > So I found the problem: The vaddr that I was using xenbus_grant_ring
> > with was obtained by dma_alloc_coherent (whereas the other split
> > drivers included in the mainline kernel use Xen IO rings allocated by
> > the "regular" mechanisms such as __get_free_page, alloc_page etc.).
> > But xenbus_grant_ring uses virt_to_gfn to get the GFN for the vaddr
> > which on ARM(64) must not be used for DMA addresses. So I could fix the
> > problem by providing a modified version of xenbus_grant_ring as part of
> > my driver which takes a dma_addr_t instead of a void* for the start
> > address, gets the PFN via dma_to_phys, converts it to a GFN and then
> > delegates to gnttab_grant_foreign_access, just like xenbus_grant_ring.
> > I can confirm that this works on Linux 5.4.0.
>
> > My question to you is: How can this be fixed "the right way"?
> > Is there anything that can be done to prevent others from debugging
> > the same problem (which for me, took some hours...)?
> > 
> > I can see multiple approaches:
> > 1. Have xenbus_grant_ring "just work" even with DMA addresses on ARM
> >     This would certainly be the nicest solution, but I don't see how
> >     it could be implemented. I don't know how to check whether some
> >     address actually is a DMA address and even if there was a way to
> >     know, dma_to_phys still requires a pointer to the device struct
> >     which was used for allocation.
> > 2. Provide another version which takes a dma_addr_t instead of void*
> >     This can be easily done, but things get complicated when the device
> >     for which the DMA memory was allocated is not the xenbus_device
> >     which is passed anyway. So, it would be necessary to include an
> >     additional argument pointing the actual device struct which was used
> >     for allocation.
> > 3. Just use gnttab_grant_foreign_access which works with GFNs anyway
> >     Which is essentially what I'm doing currently, as in my driver I
> >     know from which the device the DMA addresses were allocated.
> >     If this is the preferred solution to this problem, I propose adding
> >     a warning to the documentation of xenbus_grant_ring that forbids
> >     using this for vaddrs obtained from the DMA API as it will not work
> >     (at least on ARM).
> > 
> > What do you think?

Thank for the well-written analysis of the problem. The following should
work to translate the virtual address properly in xenbus_grant_ring:

        if (is_vmalloc_addr(vaddr))
                page = vmalloc_to_page(vaddr);
        else
                page = virt_to_page(vaddr);

Please give it a try and let me know. Otherwise, if it cannot be made to
work, option 3 with a proper warning is also fine.