on i.MX8QM, M4_1 is communicating with DomU using rpmsg with a fixed
address as the dma mem buffer which is predefined.
Without this patch, the flow is:
vring_map_one_sg -> vring_use_dma_api
-> dma_map_page
-> __swiotlb_map_page
->swiotlb_map_page
->__dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
However we are using per device dma area for rpmsg, phys_to_virt
could not return a correct virtual address for virtual address in
vmalloc area. Then kernel panic.
With this patch, vring_use_dma_api will return false, and
vring_map_one_sg will return sg_phys(sg) which is the correct phys
address in the predefined memory region.
vring_map_one_sg -> vring_use_dma_api
-> sg_phys(sg)
Signed-off-by: Peng Fan <[email protected]>
---
drivers/virtio/virtio_ring.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index cd7e755484e3..8993d7cb3592 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -248,6 +248,8 @@ static inline bool virtqueue_use_indirect(struct virtqueue *_vq,
static bool vring_use_dma_api(struct virtio_device *vdev)
{
+ struct device *dma_dev = vdev->dev.parent;
+
if (!virtio_has_iommu_quirk(vdev))
return true;
@@ -260,7 +262,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
* the DMA API if we're a Xen guest, which at least allows
* all of the sensible Xen configurations to work correctly.
*/
- if (xen_domain())
+ if (xen_domain() && !dma_dev->dma_mem)
return true;
return false;
--
2.14.1
On Mon, Jan 21, 2019 at 04:51:57AM +0000, Peng Fan wrote:
> on i.MX8QM, M4_1 is communicating with DomU using rpmsg with a fixed
> address as the dma mem buffer which is predefined.
>
> Without this patch, the flow is:
> vring_map_one_sg -> vring_use_dma_api
> -> dma_map_page
> -> __swiotlb_map_page
> ->swiotlb_map_page
> ->__dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
> However we are using per device dma area for rpmsg, phys_to_virt
> could not return a correct virtual address for virtual address in
> vmalloc area. Then kernel panic.
And that is the right thing to do. You must not call dma_map_* on
memory that was allocated from dma_alloc_*.
We actually have another thread which appears to be for this same issue.
Hi
> -----Original Message-----
> From: [email protected] [mailto:[email protected]]
> Sent: 2019??1??21?? 16:29
> To: Peng Fan <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [RFC] virtio_ring: check dma_mem for xen_domain
>
> On Mon, Jan 21, 2019 at 04:51:57AM +0000, Peng Fan wrote:
> > on i.MX8QM, M4_1 is communicating with DomU using rpmsg with a fixed
> > address as the dma mem buffer which is predefined.
> >
> > Without this patch, the flow is:
> > vring_map_one_sg -> vring_use_dma_api
> > -> dma_map_page
> > -> __swiotlb_map_page
> > ->swiotlb_map_page
> > ->__dma_map_area(phys_to_virt(dma_to_phys(dev,
> dev_addr)), size,
> > dir); However we are using per device dma area for rpmsg, phys_to_virt
> > could not return a correct virtual address for virtual address in
> > vmalloc area. Then kernel panic.
>
> And that is the right thing to do. You must not call dma_map_* on memory
> that was allocated from dma_alloc_*.
Understand. But the current code is that vring_use_dma_api will always return
true, if the current OS is a xen VM.
Actually it needs to return false for my case, then we could use sg_phys(sg)
to get the correct physical address.
>
> We actually have another thread which appears to be for this same issue.
You mean https://patchwork.kernel.org/patch/10742923/ ?
You suggest use cma there, but vring_use_dma_api will still return true if the OS
is running on xen. Then vring_map_one_sg will still runs into __dma_map_area.
In my case, just need vring_use_dma_api to return false and use sg_phys(sg) to
get the correct physical address, whether per dma reserved area or per device cma.
Thanks,
Peng.
On Mon, Jan 21, 2019 at 12:28:30AM -0800, [email protected] wrote:
> On Mon, Jan 21, 2019 at 04:51:57AM +0000, Peng Fan wrote:
> > on i.MX8QM, M4_1 is communicating with DomU using rpmsg with a fixed
> > address as the dma mem buffer which is predefined.
> >
> > Without this patch, the flow is:
> > vring_map_one_sg -> vring_use_dma_api
> > -> dma_map_page
> > -> __swiotlb_map_page
> > ->swiotlb_map_page
> > ->__dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
> > However we are using per device dma area for rpmsg, phys_to_virt
> > could not return a correct virtual address for virtual address in
> > vmalloc area. Then kernel panic.
>
> And that is the right thing to do. You must not call dma_map_* on
> memory that was allocated from dma_alloc_*.
>
> We actually have another thread which appears to be for this same issue.
Sorry, which thread do you refer to?
--
MST
On Mon, 21 Jan 2019, Peng Fan wrote:
> on i.MX8QM, M4_1 is communicating with DomU using rpmsg with a fixed
> address as the dma mem buffer which is predefined.
>
> Without this patch, the flow is:
> vring_map_one_sg -> vring_use_dma_api
> -> dma_map_page
> -> __swiotlb_map_page
> ->swiotlb_map_page
> ->__dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
> However we are using per device dma area for rpmsg, phys_to_virt
> could not return a correct virtual address for virtual address in
> vmalloc area. Then kernel panic.
>
> With this patch, vring_use_dma_api will return false, and
> vring_map_one_sg will return sg_phys(sg) which is the correct phys
> address in the predefined memory region.
> vring_map_one_sg -> vring_use_dma_api
> -> sg_phys(sg)
>
> Signed-off-by: Peng Fan <[email protected]>
> ---
> drivers/virtio/virtio_ring.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index cd7e755484e3..8993d7cb3592 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -248,6 +248,8 @@ static inline bool virtqueue_use_indirect(struct virtqueue *_vq,
>
> static bool vring_use_dma_api(struct virtio_device *vdev)
> {
> + struct device *dma_dev = vdev->dev.parent;
> +
> if (!virtio_has_iommu_quirk(vdev))
> return true;
>
> @@ -260,7 +262,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> * the DMA API if we're a Xen guest, which at least allows
> * all of the sensible Xen configurations to work correctly.
> */
> - if (xen_domain())
> + if (xen_domain() && !dma_dev->dma_mem)
> return true;
>
> return false;
I can see you spotted a real issue, but this is not the right fix. We
just need something a bit more flexible than xen_domain(): there are
many kinds of Xen domains on different architectures, we basically want
to enable this (return true from vring_use_dma_api) only when the xen
swiotlb is meant to be used. Does the appended patch fix the issue you
have?
---
xen: introduce xen_vring_use_dma
From: Stefano Stabellini <[email protected]>
Export xen_swiotlb on arm and arm64.
Use xen_swiotlb to determine when vring should use dma APIs to map the
ring: when xen_swiotlb is enabled the dma API is required. When it is
disabled, it is not required.
Reported-by: Peng Fan <[email protected]>
Signed-off-by: Stefano Stabellini <[email protected]>
diff --git a/arch/arm/include/asm/xen/swiotlb-xen.h b/arch/arm/include/asm/xen/swiotlb-xen.h
new file mode 100644
index 0000000..455ade5
--- /dev/null
+++ b/arch/arm/include/asm/xen/swiotlb-xen.h
@@ -0,0 +1 @@
+#include <xen/arm/swiotlb-xen.h>
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index cb44aa2..8592863 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -21,6 +21,8 @@
#include <asm/xen/hypercall.h>
#include <asm/xen/interface.h>
+int xen_swiotlb __read_mostly;
+
unsigned long xen_get_swiotlb_free_pages(unsigned int order)
{
struct memblock_region *reg;
@@ -189,6 +191,7 @@ int __init xen_mm_init(void)
struct gnttab_cache_flush cflush;
if (!xen_initial_domain())
return 0;
+ xen_swiotlb = 1;
xen_swiotlb_init(1, false);
xen_dma_ops = &xen_swiotlb_dma_ops;
diff --git a/arch/arm64/include/asm/xen/swiotlb-xen.h b/arch/arm64/include/asm/xen/swiotlb-xen.h
new file mode 100644
index 0000000..455ade5
--- /dev/null
+++ b/arch/arm64/include/asm/xen/swiotlb-xen.h
@@ -0,0 +1 @@
+#include <xen/arm/swiotlb-xen.h>
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index cd7e755..bf8badc 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -260,7 +260,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
* the DMA API if we're a Xen guest, which at least allows
* all of the sensible Xen configurations to work correctly.
*/
- if (xen_domain())
+ if (xen_vring_use_dma())
return true;
return false;
diff --git a/include/xen/arm/swiotlb-xen.h b/include/xen/arm/swiotlb-xen.h
new file mode 100644
index 0000000..2aac7c4
--- /dev/null
+++ b/include/xen/arm/swiotlb-xen.h
@@ -0,0 +1,10 @@
+#ifndef _ASM_ARM_XEN_SWIOTLB_XEN_H
+#define _ASM_ARM_XEN_SWIOTLB_XEN_H
+
+#ifdef CONFIG_SWIOTLB_XEN
+extern int xen_swiotlb;
+#else
+#define xen_swiotlb (0)
+#endif
+
+#endif
diff --git a/include/xen/xen.h b/include/xen/xen.h
index 0e21567..74a536d 100644
--- a/include/xen/xen.h
+++ b/include/xen/xen.h
@@ -46,4 +46,10 @@ enum xen_domain_type {
bool xen_biovec_phys_mergeable(const struct bio_vec *vec1,
const struct bio_vec *vec2);
+#include <asm/xen/swiotlb-xen.h>
+static inline int xen_vring_use_dma(void)
+{
+ return !!xen_swiotlb;
+}
+
#endif /* _XEN_XEN_H */
On Tue, Jan 22, 2019 at 11:59:31AM -0800, Stefano Stabellini wrote:
> On Mon, 21 Jan 2019, Peng Fan wrote:
> > on i.MX8QM, M4_1 is communicating with DomU using rpmsg with a fixed
> > address as the dma mem buffer which is predefined.
> >
> > Without this patch, the flow is:
> > vring_map_one_sg -> vring_use_dma_api
> > -> dma_map_page
> > -> __swiotlb_map_page
> > ->swiotlb_map_page
> > ->__dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir);
> > However we are using per device dma area for rpmsg, phys_to_virt
> > could not return a correct virtual address for virtual address in
> > vmalloc area. Then kernel panic.
> >
> > With this patch, vring_use_dma_api will return false, and
> > vring_map_one_sg will return sg_phys(sg) which is the correct phys
> > address in the predefined memory region.
> > vring_map_one_sg -> vring_use_dma_api
> > -> sg_phys(sg)
> >
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> > drivers/virtio/virtio_ring.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index cd7e755484e3..8993d7cb3592 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -248,6 +248,8 @@ static inline bool virtqueue_use_indirect(struct virtqueue *_vq,
> >
> > static bool vring_use_dma_api(struct virtio_device *vdev)
> > {
> > + struct device *dma_dev = vdev->dev.parent;
> > +
> > if (!virtio_has_iommu_quirk(vdev))
> > return true;
> >
> > @@ -260,7 +262,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> > * the DMA API if we're a Xen guest, which at least allows
> > * all of the sensible Xen configurations to work correctly.
> > */
> > - if (xen_domain())
> > + if (xen_domain() && !dma_dev->dma_mem)
> > return true;
> >
> > return false;
>
> I can see you spotted a real issue, but this is not the right fix. We
> just need something a bit more flexible than xen_domain(): there are
> many kinds of Xen domains on different architectures, we basically want
> to enable this (return true from vring_use_dma_api) only when the xen
> swiotlb is meant to be used. Does the appended patch fix the issue you
> have?
>
> ---
>
> xen: introduce xen_vring_use_dma
>
> From: Stefano Stabellini <[email protected]>
>
> Export xen_swiotlb on arm and arm64.
>
> Use xen_swiotlb to determine when vring should use dma APIs to map the
> ring: when xen_swiotlb is enabled the dma API is required. When it is
> disabled, it is not required.
>
> Reported-by: Peng Fan <[email protected]>
> Signed-off-by: Stefano Stabellini <[email protected]>
>
> diff --git a/arch/arm/include/asm/xen/swiotlb-xen.h b/arch/arm/include/asm/xen/swiotlb-xen.h
> new file mode 100644
> index 0000000..455ade5
> --- /dev/null
> +++ b/arch/arm/include/asm/xen/swiotlb-xen.h
> @@ -0,0 +1 @@
> +#include <xen/arm/swiotlb-xen.h>
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index cb44aa2..8592863 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -21,6 +21,8 @@
> #include <asm/xen/hypercall.h>
> #include <asm/xen/interface.h>
>
> +int xen_swiotlb __read_mostly;
> +
> unsigned long xen_get_swiotlb_free_pages(unsigned int order)
> {
> struct memblock_region *reg;
> @@ -189,6 +191,7 @@ int __init xen_mm_init(void)
> struct gnttab_cache_flush cflush;
> if (!xen_initial_domain())
> return 0;
> + xen_swiotlb = 1;
> xen_swiotlb_init(1, false);
> xen_dma_ops = &xen_swiotlb_dma_ops;
>
> diff --git a/arch/arm64/include/asm/xen/swiotlb-xen.h b/arch/arm64/include/asm/xen/swiotlb-xen.h
> new file mode 100644
> index 0000000..455ade5
> --- /dev/null
> +++ b/arch/arm64/include/asm/xen/swiotlb-xen.h
> @@ -0,0 +1 @@
> +#include <xen/arm/swiotlb-xen.h>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index cd7e755..bf8badc 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -260,7 +260,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> * the DMA API if we're a Xen guest, which at least allows
> * all of the sensible Xen configurations to work correctly.
> */
> - if (xen_domain())
> + if (xen_vring_use_dma())
> return true;
>
> return false;
> diff --git a/include/xen/arm/swiotlb-xen.h b/include/xen/arm/swiotlb-xen.h
> new file mode 100644
> index 0000000..2aac7c4
> --- /dev/null
> +++ b/include/xen/arm/swiotlb-xen.h
> @@ -0,0 +1,10 @@
> +#ifndef _ASM_ARM_XEN_SWIOTLB_XEN_H
> +#define _ASM_ARM_XEN_SWIOTLB_XEN_H
> +
> +#ifdef CONFIG_SWIOTLB_XEN
> +extern int xen_swiotlb;
> +#else
> +#define xen_swiotlb (0)
> +#endif
> +
> +#endif
> diff --git a/include/xen/xen.h b/include/xen/xen.h
> index 0e21567..74a536d 100644
> --- a/include/xen/xen.h
> +++ b/include/xen/xen.h
> @@ -46,4 +46,10 @@ enum xen_domain_type {
> bool xen_biovec_phys_mergeable(const struct bio_vec *vec1,
> const struct bio_vec *vec2);
>
> +#include <asm/xen/swiotlb-xen.h>
> +static inline int xen_vring_use_dma(void)
> +{
> + return !!xen_swiotlb;
Given xen_swiotlb is only defined on arm, how will
this build on other architectures?
> +}
> +
> #endif /* _XEN_XEN_H */
I'd say at this point, I'm sorry we didn't come up with PLATFORM_ACCESS
when we added the xen hack.
I'm not objecting to this patch but I would also like to bypass the xen
hack for VIRTIO 1 devices. In particular I know rpmsg is still virtio 0
so it's not an issue for it. Is Xen already using VIRTIO 1, and without
setting PLATFORM_ACCESS? If not I would like to teach
vring_use_dma_api to return false on VIRTIO_1 && !PLATFORM_ACCESS.
Would that be acceptable?
--
MST
On Tue, Jan 22, 2019 at 11:59:31AM -0800, Stefano Stabellini wrote:
> > if (!virtio_has_iommu_quirk(vdev))
> > return true;
> >
> > @@ -260,7 +262,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> > * the DMA API if we're a Xen guest, which at least allows
> > * all of the sensible Xen configurations to work correctly.
> > */
> > - if (xen_domain())
> > + if (xen_domain() && !dma_dev->dma_mem)
> > return true;
> >
> > return false;
>
> I can see you spotted a real issue, but this is not the right fix. We
> just need something a bit more flexible than xen_domain(): there are
> many kinds of Xen domains on different architectures, we basically want
> to enable this (return true from vring_use_dma_api) only when the xen
> swiotlb is meant to be used. Does the appended patch fix the issue you
> have?
The problem generally is the other way around - if dma_dev->dma_mem
is set the device decription in the device tree explicitly requires
using this memory, so we must _always_ use the DMA API.
The problem is just that that rproc driver absuses the DMA API
in horrible ways.
On Tue, 22 Jan 2019, [email protected] wrote:
> On Tue, Jan 22, 2019 at 11:59:31AM -0800, Stefano Stabellini wrote:
> > > if (!virtio_has_iommu_quirk(vdev))
> > > return true;
> > >
> > > @@ -260,7 +262,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> > > * the DMA API if we're a Xen guest, which at least allows
> > > * all of the sensible Xen configurations to work correctly.
> > > */
> > > - if (xen_domain())
> > > + if (xen_domain() && !dma_dev->dma_mem)
> > > return true;
> > >
> > > return false;
> >
> > I can see you spotted a real issue, but this is not the right fix. We
> > just need something a bit more flexible than xen_domain(): there are
> > many kinds of Xen domains on different architectures, we basically want
> > to enable this (return true from vring_use_dma_api) only when the xen
> > swiotlb is meant to be used. Does the appended patch fix the issue you
> > have?
>
> The problem generally is the other way around - if dma_dev->dma_mem
> is set the device decription in the device tree explicitly requires
> using this memory, so we must _always_ use the DMA API.
>
> The problem is just that that rproc driver absuses the DMA API
> in horrible ways.
If vring_use_dma_api is actually supposed to return true when
dma_dev->dma_mem is set, then both Peng's patch and the patch I wrote
are not fixing the real issue here.
I don't know enough about remoteproc to know where the problem actually
lies though.
On Wed, Jan 23, 2019 at 01:04:33PM -0800, Stefano Stabellini wrote:
> If vring_use_dma_api is actually supposed to return true when
> dma_dev->dma_mem is set, then both Peng's patch and the patch I wrote
> are not fixing the real issue here.
>
> I don't know enough about remoteproc to know where the problem actually
> lies though.
The problem is the following:
Devices can declare a specific memory region that they want to use when
the driver calls dma_alloc_coherent for the device, this is done using
the shared-dma-pool DT attribute, which comes in two variants that
would be a little to much to explain here.
remoteproc makes use of that because apparently the device can
only communicate using that region. But it then feeds back memory
obtained with dma_alloc_coherent into the virtio code. For that
it calls vmalloc_to_page on the dma_alloc_coherent, which is a huge
no-go for the ĐMA API and only worked accidentally on a few platform,
and apparently arm64 just changed a few internals that made it stop
working for remoteproc.
The right answer is to not use the DMA API to allocate memory from
a device-speficic region, but to tie the driver directly into the
DT reserved memory API in a way that allows it to easilt obtain
a struct device for it.
This is orthogonal to another issue, and that is that hardware
virtio devices really always need to use the DMA API, otherwise
we'll bypass such features as the device specific DMA pools,
DMA offsets, cache flushing, etc, etc.
On Wed, 23 Jan 2019, [email protected] wrote:
> On Wed, Jan 23, 2019 at 01:04:33PM -0800, Stefano Stabellini wrote:
> > If vring_use_dma_api is actually supposed to return true when
> > dma_dev->dma_mem is set, then both Peng's patch and the patch I wrote
> > are not fixing the real issue here.
> >
> > I don't know enough about remoteproc to know where the problem actually
> > lies though.
>
> The problem is the following:
>
> Devices can declare a specific memory region that they want to use when
> the driver calls dma_alloc_coherent for the device, this is done using
> the shared-dma-pool DT attribute, which comes in two variants that
> would be a little to much to explain here.
>
> remoteproc makes use of that because apparently the device can
> only communicate using that region. But it then feeds back memory
> obtained with dma_alloc_coherent into the virtio code. For that
> it calls vmalloc_to_page on the dma_alloc_coherent, which is a huge
> no-go for the ĐMA API and only worked accidentally on a few platform,
> and apparently arm64 just changed a few internals that made it stop
> working for remoteproc.
>
> The right answer is to not use the DMA API to allocate memory from
> a device-speficic region, but to tie the driver directly into the
> DT reserved memory API in a way that allows it to easilt obtain
> a struct device for it.
If I understand correctly, Peng should be able to reproduce the problem
on native Linux without any Xen involvement simply by forcing
vring_use_dma_api to return true. Peng, can you confirm?
And the right fix is not to call vmalloc_to_page on a dma_alloc_coherent
buffer -- I don't know about the recent changes on arm64, but that's not
going to work with arm32 either AFAIK. Given that I don't have a repro,
I'll leave it to Peng and/or others to send the appropriate patch for
remoteproc.
> This is orthogonal to another issue, and that is that hardware
> virtio devices really always need to use the DMA API, otherwise
> we'll bypass such features as the device specific DMA pools,
> DMA offsets, cache flushing, etc, etc.
I understand, I'll drop my patch.
> -----Original Message-----
> From: Stefano Stabellini [mailto:[email protected]]
> Sent: 2019??1??23?? 4:00
> To: Peng Fan <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [Xen-devel] [RFC] virtio_ring: check dma_mem for xen_domain
>
> On Mon, 21 Jan 2019, Peng Fan wrote:
> > on i.MX8QM, M4_1 is communicating with DomU using rpmsg with a fixed
> > address as the dma mem buffer which is predefined.
> >
> > Without this patch, the flow is:
> > vring_map_one_sg -> vring_use_dma_api
> > -> dma_map_page
> > -> __swiotlb_map_page
> > ->swiotlb_map_page
> > ->__dma_map_area(phys_to_virt(dma_to_phys(dev,
> dev_addr)), size,
> > dir); However we are using per device dma area for rpmsg, phys_to_virt
> > could not return a correct virtual address for virtual address in
> > vmalloc area. Then kernel panic.
> >
> > With this patch, vring_use_dma_api will return false, and
> > vring_map_one_sg will return sg_phys(sg) which is the correct phys
> > address in the predefined memory region.
> > vring_map_one_sg -> vring_use_dma_api
> > -> sg_phys(sg)
> >
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> > drivers/virtio/virtio_ring.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c
> > b/drivers/virtio/virtio_ring.c index cd7e755484e3..8993d7cb3592 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -248,6 +248,8 @@ static inline bool virtqueue_use_indirect(struct
> > virtqueue *_vq,
> >
> > static bool vring_use_dma_api(struct virtio_device *vdev) {
> > + struct device *dma_dev = vdev->dev.parent;
> > +
> > if (!virtio_has_iommu_quirk(vdev))
> > return true;
> >
> > @@ -260,7 +262,7 @@ static bool vring_use_dma_api(struct virtio_device
> *vdev)
> > * the DMA API if we're a Xen guest, which at least allows
> > * all of the sensible Xen configurations to work correctly.
> > */
> > - if (xen_domain())
> > + if (xen_domain() && !dma_dev->dma_mem)
> > return true;
> >
> > return false;
>
> I can see you spotted a real issue, but this is not the right fix. We just need
> something a bit more flexible than xen_domain(): there are many kinds of Xen
> domains on different architectures, we basically want to enable this (return
> true from vring_use_dma_api) only when the xen swiotlb is meant to be used.
> Does the appended patch fix the issue you have?
>
> ---
>
> xen: introduce xen_vring_use_dma
>
> From: Stefano Stabellini <[email protected]>
>
> Export xen_swiotlb on arm and arm64.
>
> Use xen_swiotlb to determine when vring should use dma APIs to map the
> ring: when xen_swiotlb is enabled the dma API is required. When it is disabled,
> it is not required.
>
> Reported-by: Peng Fan <[email protected]>
> Signed-off-by: Stefano Stabellini <[email protected]>
>
> diff --git a/arch/arm/include/asm/xen/swiotlb-xen.h
> b/arch/arm/include/asm/xen/swiotlb-xen.h
> new file mode 100644
> index 0000000..455ade5
> --- /dev/null
> +++ b/arch/arm/include/asm/xen/swiotlb-xen.h
> @@ -0,0 +1 @@
> +#include <xen/arm/swiotlb-xen.h>
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c index
> cb44aa2..8592863 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -21,6 +21,8 @@
> #include <asm/xen/hypercall.h>
> #include <asm/xen/interface.h>
>
> +int xen_swiotlb __read_mostly;
> +
> unsigned long xen_get_swiotlb_free_pages(unsigned int order) {
> struct memblock_region *reg;
> @@ -189,6 +191,7 @@ int __init xen_mm_init(void)
> struct gnttab_cache_flush cflush;
> if (!xen_initial_domain())
> return 0;
> + xen_swiotlb = 1;
> xen_swiotlb_init(1, false);
> xen_dma_ops = &xen_swiotlb_dma_ops;
>
> diff --git a/arch/arm64/include/asm/xen/swiotlb-xen.h
> b/arch/arm64/include/asm/xen/swiotlb-xen.h
> new file mode 100644
> index 0000000..455ade5
> --- /dev/null
> +++ b/arch/arm64/include/asm/xen/swiotlb-xen.h
> @@ -0,0 +1 @@
> +#include <xen/arm/swiotlb-xen.h>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index
> cd7e755..bf8badc 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -260,7 +260,7 @@ static bool vring_use_dma_api(struct virtio_device
> *vdev)
> * the DMA API if we're a Xen guest, which at least allows
> * all of the sensible Xen configurations to work correctly.
> */
> - if (xen_domain())
> + if (xen_vring_use_dma())
> return true;
>
> return false;
> diff --git a/include/xen/arm/swiotlb-xen.h b/include/xen/arm/swiotlb-xen.h
> new file mode 100644 index 0000000..2aac7c4
> --- /dev/null
> +++ b/include/xen/arm/swiotlb-xen.h
> @@ -0,0 +1,10 @@
> +#ifndef _ASM_ARM_XEN_SWIOTLB_XEN_H
> +#define _ASM_ARM_XEN_SWIOTLB_XEN_H
> +
> +#ifdef CONFIG_SWIOTLB_XEN
> +extern int xen_swiotlb;
> +#else
> +#define xen_swiotlb (0)
> +#endif
> +
> +#endif
> diff --git a/include/xen/xen.h b/include/xen/xen.h index 0e21567..74a536d
> 100644
> --- a/include/xen/xen.h
> +++ b/include/xen/xen.h
> @@ -46,4 +46,10 @@ enum xen_domain_type { bool
> xen_biovec_phys_mergeable(const struct bio_vec *vec1,
> const struct bio_vec *vec2);
>
> +#include <asm/xen/swiotlb-xen.h>
> +static inline int xen_vring_use_dma(void) {
> + return !!xen_swiotlb;
> +}
> +
> #endif /* _XEN_XEN_H */
Tested-by: Peng Fan <[email protected]>
Thanks,
Peng
Hi stefano,
> -----Original Message-----
> From: Stefano Stabellini [mailto:[email protected]]
> Sent: 2019年1月24日 7:44
> To: [email protected]
> Cc: Stefano Stabellini <[email protected]>; Peng Fan
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [Xen-devel] [RFC] virtio_ring: check dma_mem for xen_domain
>
> On Wed, 23 Jan 2019, [email protected] wrote:
> > On Wed, Jan 23, 2019 at 01:04:33PM -0800, Stefano Stabellini wrote:
> > > If vring_use_dma_api is actually supposed to return true when
> > > dma_dev->dma_mem is set, then both Peng's patch and the patch I
> > > wrote are not fixing the real issue here.
> > >
> > > I don't know enough about remoteproc to know where the problem
> > > actually lies though.
> >
> > The problem is the following:
> >
> > Devices can declare a specific memory region that they want to use
> > when the driver calls dma_alloc_coherent for the device, this is done
> > using the shared-dma-pool DT attribute, which comes in two variants
> > that would be a little to much to explain here.
> >
> > remoteproc makes use of that because apparently the device can only
> > communicate using that region. But it then feeds back memory obtained
> > with dma_alloc_coherent into the virtio code. For that it calls
> > vmalloc_to_page on the dma_alloc_coherent, which is a huge no-go for
> > the ĐMA API and only worked accidentally on a few platform, and
> > apparently arm64 just changed a few internals that made it stop
> > working for remoteproc.
> >
> > The right answer is to not use the DMA API to allocate memory from a
> > device-speficic region, but to tie the driver directly into the DT
> > reserved memory API in a way that allows it to easilt obtain a struct
> > device for it.
>
> If I understand correctly, Peng should be able to reproduce the problem on
> native Linux without any Xen involvement simply by forcing
> vring_use_dma_api to return true. Peng, can you confirm?
It is another issue without xen involvement,
There is an thread talking this: https://patchwork.kernel.org/patch/10742923/
Without xen, vring_use_dma_api will return false.
With xen, if vring_use_dma_api returns true, it will dma_map_xx and trigger dump.
Thanks,
Peng.
>
> And the right fix is not to call vmalloc_to_page on a dma_alloc_coherent
> buffer -- I don't know about the recent changes on arm64, but that's not going
> to work with arm32 either AFAIK. Given that I don't have a repro, I'll leave it to
> Peng and/or others to send the appropriate patch for remoteproc.
>
>
> > This is orthogonal to another issue, and that is that hardware virtio
> > devices really always need to use the DMA API, otherwise we'll bypass
> > such features as the device specific DMA pools, DMA offsets, cache
> > flushing, etc, etc.
>
> I understand, I'll drop my patch.
On Thu, 24 Jan 2019, Peng Fan wrote:
> Hi stefano,
>
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:[email protected]]
> > Sent: 2019年1月24日 7:44
> > To: [email protected]
> > Cc: Stefano Stabellini <[email protected]>; Peng Fan
> > <[email protected]>; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: Re: [Xen-devel] [RFC] virtio_ring: check dma_mem for xen_domain
> >
> > On Wed, 23 Jan 2019, [email protected] wrote:
> > > On Wed, Jan 23, 2019 at 01:04:33PM -0800, Stefano Stabellini wrote:
> > > > If vring_use_dma_api is actually supposed to return true when
> > > > dma_dev->dma_mem is set, then both Peng's patch and the patch I
> > > > wrote are not fixing the real issue here.
> > > >
> > > > I don't know enough about remoteproc to know where the problem
> > > > actually lies though.
> > >
> > > The problem is the following:
> > >
> > > Devices can declare a specific memory region that they want to use
> > > when the driver calls dma_alloc_coherent for the device, this is done
> > > using the shared-dma-pool DT attribute, which comes in two variants
> > > that would be a little to much to explain here.
> > >
> > > remoteproc makes use of that because apparently the device can only
> > > communicate using that region. But it then feeds back memory obtained
> > > with dma_alloc_coherent into the virtio code. For that it calls
> > > vmalloc_to_page on the dma_alloc_coherent, which is a huge no-go for
> > > the ĐMA API and only worked accidentally on a few platform, and
> > > apparently arm64 just changed a few internals that made it stop
> > > working for remoteproc.
> > >
> > > The right answer is to not use the DMA API to allocate memory from a
> > > device-speficic region, but to tie the driver directly into the DT
> > > reserved memory API in a way that allows it to easilt obtain a struct
> > > device for it.
> >
> > If I understand correctly, Peng should be able to reproduce the problem on
> > native Linux without any Xen involvement simply by forcing
> > vring_use_dma_api to return true. Peng, can you confirm?
>
> It is another issue without xen involvement,
> There is an thread talking this: https://patchwork.kernel.org/patch/10742923/
>
> Without xen, vring_use_dma_api will return false.
> With xen, if vring_use_dma_api returns true, it will dma_map_xx and trigger dump.
It is true that for Xen on ARM DomUs it is not necessary today to return
true from vring_use_dma_api. However, returning true from
vring_use_dma_api should not break Linux. When the rpmesg issue is
fixed, this problem should also go away without any need for additional
changes on the xen side I think.
On Thu, Jan 24, 2019 at 11:14:53AM -0800, Stefano Stabellini wrote:
> On Thu, 24 Jan 2019, Peng Fan wrote:
> > Hi stefano,
> >
> > > -----Original Message-----
> > > From: Stefano Stabellini [mailto:[email protected]]
> > > Sent: 2019年1月24日 7:44
> > > To: [email protected]
> > > Cc: Stefano Stabellini <[email protected]>; Peng Fan
> > > <[email protected]>; [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Subject: Re: [Xen-devel] [RFC] virtio_ring: check dma_mem for xen_domain
> > >
> > > On Wed, 23 Jan 2019, [email protected] wrote:
> > > > On Wed, Jan 23, 2019 at 01:04:33PM -0800, Stefano Stabellini wrote:
> > > > > If vring_use_dma_api is actually supposed to return true when
> > > > > dma_dev->dma_mem is set, then both Peng's patch and the patch I
> > > > > wrote are not fixing the real issue here.
> > > > >
> > > > > I don't know enough about remoteproc to know where the problem
> > > > > actually lies though.
> > > >
> > > > The problem is the following:
> > > >
> > > > Devices can declare a specific memory region that they want to use
> > > > when the driver calls dma_alloc_coherent for the device, this is done
> > > > using the shared-dma-pool DT attribute, which comes in two variants
> > > > that would be a little to much to explain here.
> > > >
> > > > remoteproc makes use of that because apparently the device can only
> > > > communicate using that region. But it then feeds back memory obtained
> > > > with dma_alloc_coherent into the virtio code. For that it calls
> > > > vmalloc_to_page on the dma_alloc_coherent, which is a huge no-go for
> > > > the ĐMA API and only worked accidentally on a few platform, and
> > > > apparently arm64 just changed a few internals that made it stop
> > > > working for remoteproc.
> > > >
> > > > The right answer is to not use the DMA API to allocate memory from a
> > > > device-speficic region, but to tie the driver directly into the DT
> > > > reserved memory API in a way that allows it to easilt obtain a struct
> > > > device for it.
> > >
> > > If I understand correctly, Peng should be able to reproduce the problem on
> > > native Linux without any Xen involvement simply by forcing
> > > vring_use_dma_api to return true. Peng, can you confirm?
> >
> > It is another issue without xen involvement,
> > There is an thread talking this: https://patchwork.kernel.org/patch/10742923/
> >
> > Without xen, vring_use_dma_api will return false.
> > With xen, if vring_use_dma_api returns true, it will dma_map_xx and trigger dump.
>
> It is true that for Xen on ARM DomUs it is not necessary today to return
> true from vring_use_dma_api. However, returning true from
> vring_use_dma_api should not break Linux. When the rpmesg issue is
> fixed, this problem should also go away without any need for additional
> changes on the xen side I think.
Let less systems bypass the standard virtio logic (using feature bit
to figure out bypassing DMA API), the better.
Hi,
> -----Original Message-----
> From: [email protected] [mailto:[email protected]]
> Sent: 2019年1月24日 5:14
> To: Stefano Stabellini <[email protected]>
> Cc: [email protected]; Peng Fan <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [Xen-devel] [RFC] virtio_ring: check dma_mem for xen_domain
>
> On Wed, Jan 23, 2019 at 01:04:33PM -0800, Stefano Stabellini wrote:
> > If vring_use_dma_api is actually supposed to return true when
> > dma_dev->dma_mem is set, then both Peng's patch and the patch I wrote
> > are not fixing the real issue here.
> >
> > I don't know enough about remoteproc to know where the problem
> > actually lies though.
>
> The problem is the following:
>
> Devices can declare a specific memory region that they want to use when the
> driver calls dma_alloc_coherent for the device, this is done using the
> shared-dma-pool DT attribute, which comes in two variants that would be a
> little to much to explain here.
>
> remoteproc makes use of that because apparently the device can only
> communicate using that region. But it then feeds back memory obtained
> with dma_alloc_coherent into the virtio code. For that it calls
> vmalloc_to_page on the dma_alloc_coherent, which is a huge no-go for the
> ĐMA API and only worked accidentally on a few platform, and apparently
> arm64 just changed a few internals that made it stop working for remoteproc.
>
> The right answer is to not use the DMA API to allocate memory from a
> device-speficic region, but to tie the driver directly into the DT reserved
> memory API in a way that allows it to easilt obtain a struct device for it.
Just have a question,
Since vmalloc_to_page is ok for cma area, no need to take cma and per device
cma into consideration right?
we only need to implement a piece code to handle per device specific region
using RESERVEDMEM_OF_DECLARE, just like:
RESERVEDMEM_OF_DECLARE(rpmsg-dma, "rpmsg-dma-pool",
rmem_rpmsg_dma_setup);
And implement the device_init call back and build a map between page and phys.
Then in rpmsg driver, scatter list could use page structure, no need vmalloc_to_page
for per device dma.
Is this the right way?
Thanks
Peng.
>
> This is orthogonal to another issue, and that is that hardware virtio devices
> really always need to use the DMA API, otherwise we'll bypass such features
> as the device specific DMA pools, DMA offsets, cache flushing, etc, etc.
On Fri, 25 Jan 2019, Peng Fan wrote:
> > On Wed, Jan 23, 2019 at 01:04:33PM -0800, Stefano Stabellini wrote:
> > > If vring_use_dma_api is actually supposed to return true when
> > > dma_dev->dma_mem is set, then both Peng's patch and the patch I wrote
> > > are not fixing the real issue here.
> > >
> > > I don't know enough about remoteproc to know where the problem
> > > actually lies though.
> >
> > The problem is the following:
> >
> > Devices can declare a specific memory region that they want to use when the
> > driver calls dma_alloc_coherent for the device, this is done using the
> > shared-dma-pool DT attribute, which comes in two variants that would be a
> > little to much to explain here.
> >
> > remoteproc makes use of that because apparently the device can only
> > communicate using that region. But it then feeds back memory obtained
> > with dma_alloc_coherent into the virtio code. For that it calls
> > vmalloc_to_page on the dma_alloc_coherent, which is a huge no-go for the
> > ĐMA API and only worked accidentally on a few platform, and apparently
> > arm64 just changed a few internals that made it stop working for remoteproc.
> >
> > The right answer is to not use the DMA API to allocate memory from a
> > device-speficic region, but to tie the driver directly into the DT reserved
> > memory API in a way that allows it to easilt obtain a struct device for it.
>
> Just have a question,
>
> Since vmalloc_to_page is ok for cma area, no need to take cma and per device
> cma into consideration right?
>
> we only need to implement a piece code to handle per device specific region
> using RESERVEDMEM_OF_DECLARE, just like:
> RESERVEDMEM_OF_DECLARE(rpmsg-dma, "rpmsg-dma-pool",
> rmem_rpmsg_dma_setup);
> And implement the device_init call back and build a map between page and phys.
> Then in rpmsg driver, scatter list could use page structure, no need vmalloc_to_page
> for per device dma.
>
> Is this the right way?
I CC'ed the rpmsg maintainers, you want to keep them in the loop on
this.
On Fri, Jan 25, 2019 at 09:45:26AM +0000, Peng Fan wrote:
> Just have a question,
>
> Since vmalloc_to_page is ok for cma area, no need to take cma and per device
> cma into consideration right?
The CMA area itself it a physical memory region. If it is a non-highmem
region you can call virt_to_page on the virtual addresses for it. If
it is in highmem it doesn't even have a kernel virtual address by
default.
> we only need to implement a piece code to handle per device specific region
> using RESERVEDMEM_OF_DECLARE, just like:
> RESERVEDMEM_OF_DECLARE(rpmsg-dma, "rpmsg-dma-pool",
> rmem_rpmsg_dma_setup);
> And implement the device_init call back and build a map between page and phys.
> Then in rpmsg driver, scatter list could use page structure, no need vmalloc_to_page
> for per device dma.
>
> Is this the right way?
I think this should work fine. If you have the cycles for it I'd
actually love to be able to have generic CMA DT glue for non DMA API
driver allocations, as there obviously is a need for it. So basically
the same as above, just added to kernel/cma.c as a generic API.
> -----Original Message-----
> From: [email protected] [mailto:[email protected]]
> Sent: 2019??1??28?? 16:00
> To: Peng Fan <[email protected]>
> Cc: [email protected]; Stefano Stabellini <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Andy Duan <[email protected]>
> Subject: Re: [Xen-devel] [RFC] virtio_ring: check dma_mem for xen_domain
>
> On Fri, Jan 25, 2019 at 09:45:26AM +0000, Peng Fan wrote:
> > Just have a question,
> >
> > Since vmalloc_to_page is ok for cma area, no need to take cma and per
> > device cma into consideration right?
>
> The CMA area itself it a physical memory region. If it is a non-highmem
> region you can call virt_to_page on the virtual addresses for it. If it is in
> highmem it doesn't even have a kernel virtual address by default.
>
> > we only need to implement a piece code to handle per device specific
> > region using RESERVEDMEM_OF_DECLARE, just like:
> > RESERVEDMEM_OF_DECLARE(rpmsg-dma, "rpmsg-dma-pool",
> > rmem_rpmsg_dma_setup); And implement the device_init call back and
> > build a map between page and phys.
> > Then in rpmsg driver, scatter list could use page structure, no need
> > vmalloc_to_page for per device dma.
> >
> > Is this the right way?
>
> I think this should work fine. If you have the cycles for it I'd actually love to
> be able to have generic CMA DT glue for non DMA API driver allocations, as
> there obviously is a need for it. So basically the same as above, just added
> to kernel/cma.c as a generic API.
Thanks for the hints. I'll try to add that.
Thanks,
Peng.