RPmsg, DMA and ARM64

RPmsg, DMA and ARM64

[Edgar E. Iglesias]

Hi,

I'm trying to run rpmsg and remoteproc on the ZynqMP but hitting an mm error.
I'm not sure who is breaking the rules, rpmsg or the dma allocators?

When rpmsg sets up the virtqueues, it allocates memory with
dma_alloc_coherent() and initializes a scatterlist with sg_init_one().
drivers/rpmsg/virtio_rpmsg_bus.c:rpmsg_probe().
sg_init_one() requires that the memory it gets is virt_addr_valid().

The problem I'm seeing is that on arm64, the dma alloc functions can
return vmalloced (via dma_common_contiguous_remap) memory. This
then causes havoc when the scatterlist code tries to go virt_to_page
and back to get hold of a physical adress (sg_phys()).

I couldn't find a definitive answer in the docs, is the arm64 DMA code
doing the right here when returning vmalloced memory?

Cheers,
Edgar

RPmsg, DMA and ARM64

[Catalin Marinas]

On Tue, Mar 24, 2015 at 02:37:49PM +1000, Edgar E. Iglesias wrote:
> I'm trying to run rpmsg and remoteproc on the ZynqMP but hitting an mm error.
> I'm not sure who is breaking the rules, rpmsg or the dma allocators?
> 
> When rpmsg sets up the virtqueues, it allocates memory with
> dma_alloc_coherent() and initializes a scatterlist with sg_init_one().
> drivers/rpmsg/virtio_rpmsg_bus.c:rpmsg_probe().
> sg_init_one() requires that the memory it gets is virt_addr_valid().
> 
> The problem I'm seeing is that on arm64, the dma alloc functions can
> return vmalloced (via dma_common_contiguous_remap) memory. This
> then causes havoc when the scatterlist code tries to go virt_to_page
> and back to get hold of a physical adress (sg_phys()).

dma_alloc_coherent may return vmap'ed memory when it needs to create a
non-cacheable alias.

Is the sg code supposed to be used with coherent DMA allocations? I
thought it's normally used with the streaming DMA, i.e. standard page
allocation rather than dma_alloc_coherent().

I'm also not sure why virtio_rpmsg_bus.c needs non-cacheable memory, I
thought normal cacheable memory would be enough for virtio.

-- 
Catalin

RPmsg, DMA and ARM64

[Edgar E. Iglesias]

On Wed, Mar 25, 2015 at 03:36:34PM +0000, Catalin Marinas wrote:
> On Tue, Mar 24, 2015 at 02:37:49PM +1000, Edgar E. Iglesias wrote:
> > I'm trying to run rpmsg and remoteproc on the ZynqMP but hitting an mm error.
> > I'm not sure who is breaking the rules, rpmsg or the dma allocators?
> > 
> > When rpmsg sets up the virtqueues, it allocates memory with
> > dma_alloc_coherent() and initializes a scatterlist with sg_init_one().
> > drivers/rpmsg/virtio_rpmsg_bus.c:rpmsg_probe().
> > sg_init_one() requires that the memory it gets is virt_addr_valid().
> > 
> > The problem I'm seeing is that on arm64, the dma alloc functions can
> > return vmalloced (via dma_common_contiguous_remap) memory. This
> > then causes havoc when the scatterlist code tries to go virt_to_page
> > and back to get hold of a physical adress (sg_phys()).
> 
> dma_alloc_coherent may return vmap'ed memory when it needs to create a
> non-cacheable alias.

Right, if returning vmapped memory is OK for dma allocs, then I can assume
that the rpmsg code is doing something bad.

> Is the sg code supposed to be used with coherent DMA allocations? I
> thought it's normally used with the streaming DMA, i.e. standard page
> allocation rather than dma_alloc_coherent().

Yes, I think that is the normal use-case but for virtio, the scatterlist
describes a ring of buffers that are not temporary/streaming. I can see
why rpmsg wants to use dma_alloc_coherent()...

My impression is though, that it may be wrong to pass the result of
dma_alloc_coherent directly to sg_init_one. Maybe we need another mechanism
to create an sg and virtio rings from a virtual address and a dma_addr_t,
avoiding the sg_phys page-based address translation.

Does this make any sense?

> I'm also not sure why virtio_rpmsg_bus.c needs non-cacheable memory, I
> thought normal cacheable memory would be enough for virtio.

Virtio is normally used within a coherent system for communication between
hypervisor and guests. But it can also, in the remoteproc use-case be
used between a CPU and a remote CPU/device. In the latter case, my
understanding is that coherent memory mappings across the local domain
are important (either via HW coherency or by using slow non-cached mappings).

Cheers,
Edgar

RPmsg, DMA and ARM64

[Russell King - ARM Linux]

On Tue, Mar 24, 2015 at 02:37:49PM +1000, Edgar E. Iglesias wrote:
> I'm trying to run rpmsg and remoteproc on the ZynqMP but hitting an mm error.
> I'm not sure who is breaking the rules, rpmsg or the dma allocators?
> 
> When rpmsg sets up the virtqueues, it allocates memory with
> dma_alloc_coherent() and initializes a scatterlist with sg_init_one().
> drivers/rpmsg/virtio_rpmsg_bus.c:rpmsg_probe().
> sg_init_one() requires that the memory it gets is virt_addr_valid().
> 
> The problem I'm seeing is that on arm64, the dma alloc functions can
> return vmalloced (via dma_common_contiguous_remap) memory. This
> then causes havoc when the scatterlist code tries to go virt_to_page
> and back to get hold of a physical adress (sg_phys()).

dma_alloc_coherent() is permitted to remap the memory which it returns;
it's allowed not to be part of the linear mapping.  The underlying
memory could even be sourced from highmem and mapped in on demand.

This means that using virt_to_page() et.al. on the return value from
dma_alloc_coherent() is not permitted.

The only way to pass such memory using scatterlists is by doing:

	sg_init_table(sg, 1);
	sg_dma_address(sg) = addr;
	sg_dma_length(sg) = length;

Such a scatterlist must _never_ have the dma_(map|unmap|sync)_sg*()
functions called on it - the only operations which would be permissible
is to walk the scatterlist, and access it using the standard DMA
accessors sg_dma_address() and sg_dma_length().

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

RPmsg, DMA and ARM64

[Edgar E. Iglesias]

On Thu, Mar 26, 2015 at 03:26:07PM +0000, Russell King - ARM Linux wrote:
> On Tue, Mar 24, 2015 at 02:37:49PM +1000, Edgar E. Iglesias wrote:
> > I'm trying to run rpmsg and remoteproc on the ZynqMP but hitting an mm error.
> > I'm not sure who is breaking the rules, rpmsg or the dma allocators?
> > 
> > When rpmsg sets up the virtqueues, it allocates memory with
> > dma_alloc_coherent() and initializes a scatterlist with sg_init_one().
> > drivers/rpmsg/virtio_rpmsg_bus.c:rpmsg_probe().
> > sg_init_one() requires that the memory it gets is virt_addr_valid().
> > 
> > The problem I'm seeing is that on arm64, the dma alloc functions can
> > return vmalloced (via dma_common_contiguous_remap) memory. This
> > then causes havoc when the scatterlist code tries to go virt_to_page
> > and back to get hold of a physical adress (sg_phys()).
> 
> dma_alloc_coherent() is permitted to remap the memory which it returns;
> it's allowed not to be part of the linear mapping.  The underlying
> memory could even be sourced from highmem and mapped in on demand.
> 
> This means that using virt_to_page() et.al. on the return value from
> dma_alloc_coherent() is not permitted.

Thanks Russel, that's info is of great help.

> 
> The only way to pass such memory using scatterlists is by doing:
> 
> 	sg_init_table(sg, 1);
> 	sg_dma_address(sg) = addr;
> 	sg_dma_length(sg) = length;
> 
> Such a scatterlist must _never_ have the dma_(map|unmap|sync)_sg*()
> functions called on it - the only operations which would be permissible
> is to walk the scatterlist, and access it using the standard DMA
> accessors sg_dma_address() and sg_dma_length().

I'll have a closer look and see if I can change rpmsg to deal with
this.

Cheers,
Edgar

RPmsg, DMA and ARM64

[Russell King - ARM Linux]

On Fri, Mar 27, 2015 at 02:01:36AM +1000, Edgar E. Iglesias wrote:
> On Thu, Mar 26, 2015 at 03:26:07PM +0000, Russell King - ARM Linux wrote:
> > The only way to pass such memory using scatterlists is by doing:
> > 
> > 	sg_init_table(sg, 1);
> > 	sg_dma_address(sg) = addr;
> > 	sg_dma_length(sg) = length;
> > 
> > Such a scatterlist must _never_ have the dma_(map|unmap|sync)_sg*()
> > functions called on it - the only operations which would be permissible
> > is to walk the scatterlist, and access it using the standard DMA
> > accessors sg_dma_address() and sg_dma_length().
> 
> I'll have a closer look and see if I can change rpmsg to deal with
> this.

Thanks - I should point out too that when accessing the scatterlist for
DMA purposes, using the accessors I mentioned above is a must - especially
as we're now getting IOMMUs on ARM.

This is because of a subtlety of the DMA streaming API.

The reason is that sg_dma_length(sg) may not be equal to sg->length -
IOMMUs (as part of dma_map_sg()) are permitted to coalesce entries
together by using their remapping abilities, which can result in fewer
scatterlist entries being needed.

It's also important to realise that dma_map_sg() returns the number of
scatterlist entries to be walked for DMA purposes, rather than the nents
passed into dma_map_sg() should _not_ be used.  It's the same reason -
if coalescing has occurred, the return value will be smaller, telling
you how many scatterlist entries are to be walked.

Finally, it's important to realise that when such coalescing occurs,
the values you have in sg_dma_address() and sg_dma_length() may _not_
correspond with the struct page in the entry.  In other words, you
can't expect to be able to switch between DMA and PIO and use the
same scatterlist entry, and expect to hit the same memory.

I suspect a lot of ARM driver authors are not aware of these finer
points.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.