[PATCH v3 0/2] XEN SWIOTLB dma operations extension

[PATCH v3 0/2] XEN SWIOTLB dma operations extension

[Andrii Anisov]

From: Andrii Anisov <andrii_anisov@epam.com>

Some drivers need additional dma ops to be provided by xen swiotlb. Because
common operations implementation came from x86 world and does not suite ARM
needs we have to provide needed XEN SWIOTLB for ARM callbacks.

dma_mmap patch is a port of an antique and lost patch discussed here:
http://marc.info/?l=xen-devel&m=139695901430667&w=4

Changes in V3:
 - dma ops are moved from ARM specific to generic code
 - runtime operation verified for arm64 with LK 4.9
 - compilation verified for arm, arm64, x86 with the current LK master HEAD
   566cf877a1fcb6d6dc0126b076aad062054c2637

Changes in V2:
 - patches are rebased and checked for compilation for x86, arm, arm64 with
   the current LK master HEAD 83346fbc07d267de777e2597552f785174ad0373

Andrii Anisov (1):
  swiotlb-xen: implement xen_swiotlb_get_sgtable callback

Stefano Stabellini (1):
  swiotlb-xen: implement xen_swiotlb_dma_mmap callback

 arch/arm/xen/mm.c         |  2 ++
 drivers/xen/swiotlb-xen.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/xen/swiotlb-xen.h | 11 +++++++++++
 3 files changed, 53 insertions(+)

-- 
2.7.4


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

[PATCH v3 1/2] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Andrii Anisov]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

This function creates userspace mapping for the DMA-coherent memory.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
---
 arch/arm/xen/mm.c         |  1 +
 drivers/xen/swiotlb-xen.c | 18 ++++++++++++++++++
 include/xen/swiotlb-xen.h |  5 +++++
 3 files changed, 24 insertions(+)

diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index bd62d94..cd1684e 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -198,6 +198,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = {
 	.unmap_page = xen_swiotlb_unmap_page,
 	.dma_supported = xen_swiotlb_dma_supported,
 	.set_dma_mask = xen_swiotlb_set_dma_mask,
+	.mmap = xen_swiotlb_dma_mmap,
 };
 
 int __init xen_mm_init(void)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index f8afc6d..8ac36b4 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -681,3 +681,21 @@ xen_swiotlb_set_dma_mask(struct device *dev, u64 dma_mask)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(xen_swiotlb_set_dma_mask);
+
+/*
+ * Create userspace mapping for the DMA-coherent memory.
+ * Following function should be called with the local pages only.
+ */
+int
+xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
+			void *cpu_addr, dma_addr_t dma_addr, size_t size,
+			unsigned long attrs)
+{
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
+	if (__generic_dma_ops(dev)->mmap)
+		return __generic_dma_ops(dev)->mmap(dev, vma, cpu_addr,
+						    dma_addr, size, attrs);
+#endif
+	return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
+}
+EXPORT_SYMBOL_GPL(xen_swiotlb_dma_mmap);
diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
index a0083be..5c8f4c8 100644
--- a/include/xen/swiotlb-xen.h
+++ b/include/xen/swiotlb-xen.h
@@ -55,4 +55,9 @@ xen_swiotlb_dma_supported(struct device *hwdev, u64 mask);
 
 extern int
 xen_swiotlb_set_dma_mask(struct device *dev, u64 dma_mask);
+
+extern int
+xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
+			void *cpu_addr, dma_addr_t dma_addr, size_t size,
+			unsigned long attrs);
 #endif /* __LINUX_SWIOTLB_XEN_H */
-- 
2.7.4


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

Re: [PATCH v3 1/2] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Konrad Rzeszutek Wilk]

On Tue, Jan 31, 2017 at 08:30:25PM +0200, Andrii Anisov wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> This function creates userspace mapping for the DMA-coherent memory.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> ---
>  arch/arm/xen/mm.c         |  1 +
>  drivers/xen/swiotlb-xen.c | 18 ++++++++++++++++++
>  include/xen/swiotlb-xen.h |  5 +++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index bd62d94..cd1684e 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -198,6 +198,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = {
>  	.unmap_page = xen_swiotlb_unmap_page,
>  	.dma_supported = xen_swiotlb_dma_supported,
>  	.set_dma_mask = xen_swiotlb_set_dma_mask,
> +	.mmap = xen_swiotlb_dma_mmap,
>  };
>  
>  int __init xen_mm_init(void)
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index f8afc6d..8ac36b4 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -681,3 +681,21 @@ xen_swiotlb_set_dma_mask(struct device *dev, u64 dma_mask)
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(xen_swiotlb_set_dma_mask);
> +
> +/*
> + * Create userspace mapping for the DMA-coherent memory.
> + * Following function should be called with the local pages only.

What does 'local pages' mean?

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

Re: [PATCH v3 1/2] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Stefano Stabellini]

On Tue, 31 Jan 2017, Konrad Rzeszutek Wilk wrote:
> On Tue, Jan 31, 2017 at 08:30:25PM +0200, Andrii Anisov wrote:
> > From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > This function creates userspace mapping for the DMA-coherent memory.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
> > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> > ---
> >  arch/arm/xen/mm.c         |  1 +
> >  drivers/xen/swiotlb-xen.c | 18 ++++++++++++++++++
> >  include/xen/swiotlb-xen.h |  5 +++++
> >  3 files changed, 24 insertions(+)
> > 
> > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> > index bd62d94..cd1684e 100644
> > --- a/arch/arm/xen/mm.c
> > +++ b/arch/arm/xen/mm.c
> > @@ -198,6 +198,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = {
> >  	.unmap_page = xen_swiotlb_unmap_page,
> >  	.dma_supported = xen_swiotlb_dma_supported,
> >  	.set_dma_mask = xen_swiotlb_set_dma_mask,
> > +	.mmap = xen_swiotlb_dma_mmap,
> >  };
> >  
> >  int __init xen_mm_init(void)
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index f8afc6d..8ac36b4 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -681,3 +681,21 @@ xen_swiotlb_set_dma_mask(struct device *dev, u64 dma_mask)
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(xen_swiotlb_set_dma_mask);
> > +
> > +/*
> > + * Create userspace mapping for the DMA-coherent memory.
> > + * Following function should be called with the local pages only.
> 
> What does 'local pages' mean?

A page that belongs to this domain, rather than a foreign page that has
been mapped.

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

Re: [PATCH v3 1/2] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Andrii Anisov]

Konrad, Stefano,

>> What does 'local pages' mean?
>
> A page that belongs to this domain, rather than a foreign page that has
> been mapped.
I guess it should be clarified, but not sure in the commit message or
comment to the code?

Sincerely,
Andrii Anisov.

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

Re: [PATCH v3 1/2] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Konrad Rzeszutek Wilk]

On Wed, Feb 01, 2017 at 01:46:48PM +0200, Andrii Anisov wrote:
> Konrad, Stefano,
> 
> >> What does 'local pages' mean?
> >
> > A page that belongs to this domain, rather than a foreign page that has
> > been mapped.
> I guess it should be clarified, but not sure in the commit message or
> comment to the code?

in the code pls.
> 
> Sincerely,
> Andrii Anisov.

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

[PATCH v3 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback

[Andrii Anisov]

From: Andrii Anisov <andrii_anisov@epam.com>

Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
---
 arch/arm/xen/mm.c         |  1 +
 drivers/xen/swiotlb-xen.c | 22 ++++++++++++++++++++++
 include/xen/swiotlb-xen.h |  6 ++++++
 3 files changed, 29 insertions(+)

diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index cd1684e..76ea48a 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -199,6 +199,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = {
 	.dma_supported = xen_swiotlb_dma_supported,
 	.set_dma_mask = xen_swiotlb_set_dma_mask,
 	.mmap = xen_swiotlb_dma_mmap,
+	.get_sgtable = xen_swiotlb_get_sgtable,
 };
 
 int __init xen_mm_init(void)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 8ac36b4..a809d43 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -699,3 +699,25 @@ xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
 	return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
 }
 EXPORT_SYMBOL_GPL(xen_swiotlb_dma_mmap);
+
+/*
+ * Following function should be called with the local pages only.
+ */
+int
+xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
+			void *cpu_addr, dma_addr_t handle, size_t size,
+			unsigned long attrs)
+{
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
+	if (__generic_dma_ops(dev)->get_sgtable) {
+#ifdef DEBUG
+		unsigned long bfn = PHYS_PFN(dma_to_phys(dev, handle));
+		BUG_ON (!page_is_ram(bfn));
+#endif
+		return __generic_dma_ops(dev)->get_sgtable(dev, sgt, cpu_addr,
+							   handle, size, attrs);
+	}
+#endif
+	return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size);
+}
+EXPORT_SYMBOL_GPL(xen_swiotlb_get_sgtable);
diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
index 5c8f4c8..c554c23 100644
--- a/include/xen/swiotlb-xen.h
+++ b/include/xen/swiotlb-xen.h
@@ -60,4 +60,10 @@ extern int
 xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
 			void *cpu_addr, dma_addr_t dma_addr, size_t size,
 			unsigned long attrs);
+
+extern int
+xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
+			void *cpu_addr, dma_addr_t handle, size_t size,
+			unsigned long attrs);
+
 #endif /* __LINUX_SWIOTLB_XEN_H */
-- 
2.7.4


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

Re: [PATCH v3 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback

[Konrad Rzeszutek Wilk]

On Tue, Jan 31, 2017 at 08:30:26PM +0200, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@epam.com>
> 
> Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>  arch/arm/xen/mm.c         |  1 +
>  drivers/xen/swiotlb-xen.c | 22 ++++++++++++++++++++++
>  include/xen/swiotlb-xen.h |  6 ++++++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> index cd1684e..76ea48a 100644
> --- a/arch/arm/xen/mm.c
> +++ b/arch/arm/xen/mm.c
> @@ -199,6 +199,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = {
>  	.dma_supported = xen_swiotlb_dma_supported,
>  	.set_dma_mask = xen_swiotlb_set_dma_mask,
>  	.mmap = xen_swiotlb_dma_mmap,
> +	.get_sgtable = xen_swiotlb_get_sgtable,
>  };
>  
>  int __init xen_mm_init(void)
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 8ac36b4..a809d43 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -699,3 +699,25 @@ xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
>  	return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
>  }
>  EXPORT_SYMBOL_GPL(xen_swiotlb_dma_mmap);
> +
> +/*
> + * Following function should be called with the local pages only.

What does 'local pages' mean?

> + */
> +int
> +xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
> +			void *cpu_addr, dma_addr_t handle, size_t size,
> +			unsigned long attrs)
> +{
> +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> +	if (__generic_dma_ops(dev)->get_sgtable) {
> +#ifdef DEBUG
> +		unsigned long bfn = PHYS_PFN(dma_to_phys(dev, handle));
> +		BUG_ON (!page_is_ram(bfn));
> +#endif

Could you remove the above please?

> +		return __generic_dma_ops(dev)->get_sgtable(dev, sgt, cpu_addr,
> +							   handle, size, attrs);
> +	}
> +#endif
> +	return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size);
> +}
> +EXPORT_SYMBOL_GPL(xen_swiotlb_get_sgtable);
> diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
> index 5c8f4c8..c554c23 100644
> --- a/include/xen/swiotlb-xen.h
> +++ b/include/xen/swiotlb-xen.h
> @@ -60,4 +60,10 @@ extern int
>  xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
>  			void *cpu_addr, dma_addr_t dma_addr, size_t size,
>  			unsigned long attrs);
> +
> +extern int
> +xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
> +			void *cpu_addr, dma_addr_t handle, size_t size,
> +			unsigned long attrs);

And perhaps fix this to be aligned properly?
> +
>  #endif /* __LINUX_SWIOTLB_XEN_H */
> -- 
> 2.7.4
> 

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

Re: [PATCH v3 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback

[Stefano Stabellini]

On Tue, 31 Jan 2017, Konrad Rzeszutek Wilk wrote:
> On Tue, Jan 31, 2017 at 08:30:26PM +0200, Andrii Anisov wrote:
> > From: Andrii Anisov <andrii_anisov@epam.com>
> > 
> > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> >  arch/arm/xen/mm.c         |  1 +
> >  drivers/xen/swiotlb-xen.c | 22 ++++++++++++++++++++++
> >  include/xen/swiotlb-xen.h |  6 ++++++
> >  3 files changed, 29 insertions(+)
> > 
> > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> > index cd1684e..76ea48a 100644
> > --- a/arch/arm/xen/mm.c
> > +++ b/arch/arm/xen/mm.c
> > @@ -199,6 +199,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = {
> >  	.dma_supported = xen_swiotlb_dma_supported,
> >  	.set_dma_mask = xen_swiotlb_set_dma_mask,
> >  	.mmap = xen_swiotlb_dma_mmap,
> > +	.get_sgtable = xen_swiotlb_get_sgtable,
> >  };
> >  
> >  int __init xen_mm_init(void)
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index 8ac36b4..a809d43 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -699,3 +699,25 @@ xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
> >  	return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
> >  }
> >  EXPORT_SYMBOL_GPL(xen_swiotlb_dma_mmap);
> > +
> > +/*
> > + * Following function should be called with the local pages only.
> 
> What does 'local pages' mean?
> 
> > + */
> > +int
> > +xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
> > +			void *cpu_addr, dma_addr_t handle, size_t size,
> > +			unsigned long attrs)
> > +{
> > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> > +	if (__generic_dma_ops(dev)->get_sgtable) {
> > +#ifdef DEBUG
> > +		unsigned long bfn = PHYS_PFN(dma_to_phys(dev, handle));
> > +		BUG_ON (!page_is_ram(bfn));
> > +#endif
> 
> Could you remove the above please?

I asked him to add it (see
http://marc.info/?l=xen-devel&m=148461541618751): the reason is that
this function cannot work on foreign pages. From the commit message
(d2b7428eb0caa7c66e34b6ac869a43915b294123) it looks like it is supposed
to be called on buffers returned by dma_alloc_coherent, thus it should
be safe.  However, if that's not the case in any of the drivers, it
would lead to memory corruptions. The two lines under ifdef DEBUG are an
nice way to catch this kind of errors. However, given that they cost a
few cpu cycles more than necessary, I wouldn't enable them in
production, hence the ifdef DEBUG.


> > +		return __generic_dma_ops(dev)->get_sgtable(dev, sgt, cpu_addr,
> > +							   handle, size, attrs);
> > +	}
> > +#endif
> > +	return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size);
> > +}
> > +EXPORT_SYMBOL_GPL(xen_swiotlb_get_sgtable);
> > diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
> > index 5c8f4c8..c554c23 100644
> > --- a/include/xen/swiotlb-xen.h
> > +++ b/include/xen/swiotlb-xen.h
> > @@ -60,4 +60,10 @@ extern int
> >  xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
> >  			void *cpu_addr, dma_addr_t dma_addr, size_t size,
> >  			unsigned long attrs);
> > +
> > +extern int
> > +xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
> > +			void *cpu_addr, dma_addr_t handle, size_t size,
> > +			unsigned long attrs);
> 
> And perhaps fix this to be aligned properly?
> > +
> >  #endif /* __LINUX_SWIOTLB_XEN_H */
> > -- 
> > 2.7.4
> > 
> 

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

Re: [PATCH v3 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback

[Konrad Rzeszutek Wilk]

On Tue, Jan 31, 2017 at 11:15:50AM -0800, Stefano Stabellini wrote:
> On Tue, 31 Jan 2017, Konrad Rzeszutek Wilk wrote:
> > On Tue, Jan 31, 2017 at 08:30:26PM +0200, Andrii Anisov wrote:
> > > From: Andrii Anisov <andrii_anisov@epam.com>
> > > 
> > > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > > ---
> > >  arch/arm/xen/mm.c         |  1 +
> > >  drivers/xen/swiotlb-xen.c | 22 ++++++++++++++++++++++
> > >  include/xen/swiotlb-xen.h |  6 ++++++
> > >  3 files changed, 29 insertions(+)
> > > 
> > > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> > > index cd1684e..76ea48a 100644
> > > --- a/arch/arm/xen/mm.c
> > > +++ b/arch/arm/xen/mm.c
> > > @@ -199,6 +199,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = {
> > >  	.dma_supported = xen_swiotlb_dma_supported,
> > >  	.set_dma_mask = xen_swiotlb_set_dma_mask,
> > >  	.mmap = xen_swiotlb_dma_mmap,
> > > +	.get_sgtable = xen_swiotlb_get_sgtable,
> > >  };
> > >  
> > >  int __init xen_mm_init(void)
> > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > > index 8ac36b4..a809d43 100644
> > > --- a/drivers/xen/swiotlb-xen.c
> > > +++ b/drivers/xen/swiotlb-xen.c
> > > @@ -699,3 +699,25 @@ xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
> > >  	return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
> > >  }
> > >  EXPORT_SYMBOL_GPL(xen_swiotlb_dma_mmap);
> > > +
> > > +/*
> > > + * Following function should be called with the local pages only.
> > 
> > What does 'local pages' mean?
> > 
> > > + */
> > > +int
> > > +xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
> > > +			void *cpu_addr, dma_addr_t handle, size_t size,
> > > +			unsigned long attrs)
> > > +{
> > > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> > > +	if (__generic_dma_ops(dev)->get_sgtable) {
> > > +#ifdef DEBUG
> > > +		unsigned long bfn = PHYS_PFN(dma_to_phys(dev, handle));
> > > +		BUG_ON (!page_is_ram(bfn));
> > > +#endif
> > 
> > Could you remove the above please?
> 
> I asked him to add it (see
> http://marc.info/?l=xen-devel&m=148461541618751): the reason is that
> this function cannot work on foreign pages. From the commit message
> (d2b7428eb0caa7c66e34b6ac869a43915b294123) it looks like it is supposed
> to be called on buffers returned by dma_alloc_coherent, thus it should
> be safe.  However, if that's not the case in any of the drivers, it
> would lead to memory corruptions. The two lines under ifdef DEBUG are an
> nice way to catch this kind of errors. However, given that they cost a
> few cpu cycles more than necessary, I wouldn't enable them in
> production, hence the ifdef DEBUG.

But there are no Kconfig DEBUG - so you may as well just do
#if 0

around the code.

> 
> 
> > > +		return __generic_dma_ops(dev)->get_sgtable(dev, sgt, cpu_addr,
> > > +							   handle, size, attrs);
> > > +	}
> > > +#endif
> > > +	return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size);
> > > +}
> > > +EXPORT_SYMBOL_GPL(xen_swiotlb_get_sgtable);
> > > diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
> > > index 5c8f4c8..c554c23 100644
> > > --- a/include/xen/swiotlb-xen.h
> > > +++ b/include/xen/swiotlb-xen.h
> > > @@ -60,4 +60,10 @@ extern int
> > >  xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
> > >  			void *cpu_addr, dma_addr_t dma_addr, size_t size,
> > >  			unsigned long attrs);
> > > +
> > > +extern int
> > > +xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
> > > +			void *cpu_addr, dma_addr_t handle, size_t size,
> > > +			unsigned long attrs);
> > 
> > And perhaps fix this to be aligned properly?
> > > +
> > >  #endif /* __LINUX_SWIOTLB_XEN_H */
> > > -- 
> > > 2.7.4
> > > 
> > 

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

Re: [PATCH v3 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback

[Stefano Stabellini]

On Tue, 31 Jan 2017, Konrad Rzeszutek Wilk wrote:
> On Tue, Jan 31, 2017 at 11:15:50AM -0800, Stefano Stabellini wrote:
> > On Tue, 31 Jan 2017, Konrad Rzeszutek Wilk wrote:
> > > On Tue, Jan 31, 2017 at 08:30:26PM +0200, Andrii Anisov wrote:
> > > > From: Andrii Anisov <andrii_anisov@epam.com>
> > > > 
> > > > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
> > > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
> > > > ---
> > > >  arch/arm/xen/mm.c         |  1 +
> > > >  drivers/xen/swiotlb-xen.c | 22 ++++++++++++++++++++++
> > > >  include/xen/swiotlb-xen.h |  6 ++++++
> > > >  3 files changed, 29 insertions(+)
> > > > 
> > > > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
> > > > index cd1684e..76ea48a 100644
> > > > --- a/arch/arm/xen/mm.c
> > > > +++ b/arch/arm/xen/mm.c
> > > > @@ -199,6 +199,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = {
> > > >  	.dma_supported = xen_swiotlb_dma_supported,
> > > >  	.set_dma_mask = xen_swiotlb_set_dma_mask,
> > > >  	.mmap = xen_swiotlb_dma_mmap,
> > > > +	.get_sgtable = xen_swiotlb_get_sgtable,
> > > >  };
> > > >  
> > > >  int __init xen_mm_init(void)
> > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > > > index 8ac36b4..a809d43 100644
> > > > --- a/drivers/xen/swiotlb-xen.c
> > > > +++ b/drivers/xen/swiotlb-xen.c
> > > > @@ -699,3 +699,25 @@ xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
> > > >  	return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(xen_swiotlb_dma_mmap);
> > > > +
> > > > +/*
> > > > + * Following function should be called with the local pages only.
> > > 
> > > What does 'local pages' mean?
> > > 
> > > > + */
> > > > +int
> > > > +xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
> > > > +			void *cpu_addr, dma_addr_t handle, size_t size,
> > > > +			unsigned long attrs)
> > > > +{
> > > > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> > > > +	if (__generic_dma_ops(dev)->get_sgtable) {
> > > > +#ifdef DEBUG
> > > > +		unsigned long bfn = PHYS_PFN(dma_to_phys(dev, handle));
> > > > +		BUG_ON (!page_is_ram(bfn));
> > > > +#endif
> > > 
> > > Could you remove the above please?
> > 
> > I asked him to add it (see
> > http://marc.info/?l=xen-devel&m=148461541618751): the reason is that
> > this function cannot work on foreign pages. From the commit message
> > (d2b7428eb0caa7c66e34b6ac869a43915b294123) it looks like it is supposed
> > to be called on buffers returned by dma_alloc_coherent, thus it should
> > be safe.  However, if that's not the case in any of the drivers, it
> > would lead to memory corruptions. The two lines under ifdef DEBUG are an
> > nice way to catch this kind of errors. However, given that they cost a
> > few cpu cycles more than necessary, I wouldn't enable them in
> > production, hence the ifdef DEBUG.
> 
> But there are no Kconfig DEBUG - so you may as well just do
> #if 0
> 
> around the code.

Ah! Ops :-)

DEBUG_KERNEL?

> > > > +		return __generic_dma_ops(dev)->get_sgtable(dev, sgt, cpu_addr,
> > > > +							   handle, size, attrs);
> > > > +	}
> > > > +#endif
> > > > +	return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(xen_swiotlb_get_sgtable);
> > > > diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
> > > > index 5c8f4c8..c554c23 100644
> > > > --- a/include/xen/swiotlb-xen.h
> > > > +++ b/include/xen/swiotlb-xen.h
> > > > @@ -60,4 +60,10 @@ extern int
> > > >  xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
> > > >  			void *cpu_addr, dma_addr_t dma_addr, size_t size,
> > > >  			unsigned long attrs);
> > > > +
> > > > +extern int
> > > > +xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
> > > > +			void *cpu_addr, dma_addr_t handle, size_t size,
> > > > +			unsigned long attrs);
> > > 
> > > And perhaps fix this to be aligned properly?
> > > > +
> > > >  #endif /* __LINUX_SWIOTLB_XEN_H */
> > > > -- 
> > > > 2.7.4
> > > > 
> > > 
> 

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

Re: [PATCH v3 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback

[Andrii Anisov]

Stefano,

> Ah! Ops :-)
>
> DEBUG_KERNEL?

You said DEBUG_DRIVER last time.
Both DEBUG_KERNEL and DEBUG_DRIVER are not used in code. DEBUG_KERNEL
is used through Kconfigs to make available debug options to other
different stuff, which consequently define DEBUG through appropriate
makefiles.
DEBUG_DRIVER enables DEBUG macro in base, opp and power makefiles.

Sincerely,
Andrii Anisov.


On Tue, Jan 31, 2017 at 9:21 PM, Stefano Stabellini
<sstabellini@kernel.org> wrote:
> On Tue, 31 Jan 2017, Konrad Rzeszutek Wilk wrote:
>> On Tue, Jan 31, 2017 at 11:15:50AM -0800, Stefano Stabellini wrote:
>> > On Tue, 31 Jan 2017, Konrad Rzeszutek Wilk wrote:
>> > > On Tue, Jan 31, 2017 at 08:30:26PM +0200, Andrii Anisov wrote:
>> > > > From: Andrii Anisov <andrii_anisov@epam.com>
>> > > >
>> > > > Signed-off-by: Andrii Anisov <andrii_anisov@epam.com>
>> > > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>> > > > ---
>> > > >  arch/arm/xen/mm.c         |  1 +
>> > > >  drivers/xen/swiotlb-xen.c | 22 ++++++++++++++++++++++
>> > > >  include/xen/swiotlb-xen.h |  6 ++++++
>> > > >  3 files changed, 29 insertions(+)
>> > > >
>> > > > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
>> > > > index cd1684e..76ea48a 100644
>> > > > --- a/arch/arm/xen/mm.c
>> > > > +++ b/arch/arm/xen/mm.c
>> > > > @@ -199,6 +199,7 @@ static struct dma_map_ops xen_swiotlb_dma_ops = {
>> > > >         .dma_supported = xen_swiotlb_dma_supported,
>> > > >         .set_dma_mask = xen_swiotlb_set_dma_mask,
>> > > >         .mmap = xen_swiotlb_dma_mmap,
>> > > > +       .get_sgtable = xen_swiotlb_get_sgtable,
>> > > >  };
>> > > >
>> > > >  int __init xen_mm_init(void)
>> > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
>> > > > index 8ac36b4..a809d43 100644
>> > > > --- a/drivers/xen/swiotlb-xen.c
>> > > > +++ b/drivers/xen/swiotlb-xen.c
>> > > > @@ -699,3 +699,25 @@ xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
>> > > >         return dma_common_mmap(dev, vma, cpu_addr, dma_addr, size);
>> > > >  }
>> > > >  EXPORT_SYMBOL_GPL(xen_swiotlb_dma_mmap);
>> > > > +
>> > > > +/*
>> > > > + * Following function should be called with the local pages only.
>> > >
>> > > What does 'local pages' mean?
>> > >
>> > > > + */
>> > > > +int
>> > > > +xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
>> > > > +                       void *cpu_addr, dma_addr_t handle, size_t size,
>> > > > +                       unsigned long attrs)
>> > > > +{
>> > > > +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
>> > > > +       if (__generic_dma_ops(dev)->get_sgtable) {
>> > > > +#ifdef DEBUG
>> > > > +               unsigned long bfn = PHYS_PFN(dma_to_phys(dev, handle));
>> > > > +               BUG_ON (!page_is_ram(bfn));
>> > > > +#endif
>> > >
>> > > Could you remove the above please?
>> >
>> > I asked him to add it (see
>> > http://marc.info/?l=xen-devel&m=148461541618751): the reason is that
>> > this function cannot work on foreign pages. From the commit message
>> > (d2b7428eb0caa7c66e34b6ac869a43915b294123) it looks like it is supposed
>> > to be called on buffers returned by dma_alloc_coherent, thus it should
>> > be safe.  However, if that's not the case in any of the drivers, it
>> > would lead to memory corruptions. The two lines under ifdef DEBUG are an
>> > nice way to catch this kind of errors. However, given that they cost a
>> > few cpu cycles more than necessary, I wouldn't enable them in
>> > production, hence the ifdef DEBUG.
>>
>> But there are no Kconfig DEBUG - so you may as well just do
>> #if 0
>>
>> around the code.
>

>
>> > > > +               return __generic_dma_ops(dev)->get_sgtable(dev, sgt, cpu_addr,
>> > > > +                                                          handle, size, attrs);
>> > > > +       }
>> > > > +#endif
>> > > > +       return dma_common_get_sgtable(dev, sgt, cpu_addr, handle, size);
>> > > > +}
>> > > > +EXPORT_SYMBOL_GPL(xen_swiotlb_get_sgtable);
>> > > > diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
>> > > > index 5c8f4c8..c554c23 100644
>> > > > --- a/include/xen/swiotlb-xen.h
>> > > > +++ b/include/xen/swiotlb-xen.h
>> > > > @@ -60,4 +60,10 @@ extern int
>> > > >  xen_swiotlb_dma_mmap(struct device *dev, struct vm_area_struct *vma,
>> > > >                         void *cpu_addr, dma_addr_t dma_addr, size_t size,
>> > > >                         unsigned long attrs);
>> > > > +
>> > > > +extern int
>> > > > +xen_swiotlb_get_sgtable(struct device *dev, struct sg_table *sgt,
>> > > > +                       void *cpu_addr, dma_addr_t handle, size_t size,
>> > > > +                       unsigned long attrs);
>> > >
>> > > And perhaps fix this to be aligned properly?
>> > > > +
>> > > >  #endif /* __LINUX_SWIOTLB_XEN_H */
>> > > > --
>> > > > 2.7.4
>> > > >
>> > >
>>

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

Re: [PATCH v3 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback

[Julien Grall]

Hi,

On 01/02/2017 10:42, Andrii Anisov wrote:
>> Ah! Ops :-)
>>
>> DEBUG_KERNEL?
>
> You said DEBUG_DRIVER last time.
> Both DEBUG_KERNEL and DEBUG_DRIVER are not used in code. DEBUG_KERNEL
> is used through Kconfigs to make available debug options to other
> different stuff, which consequently define DEBUG through appropriate
> makefiles.
> DEBUG_DRIVER enables DEBUG macro in base, opp and power makefiles.

Technically this check should be done on any debug build for safety. So 
I would add new Kconfig DEBUG_KENREL that is selected by DEBUG_KERNEL.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v3 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback

[Julien Grall]

On 01/02/2017 10:57, Julien Grall wrote:
> Hi,
>
> On 01/02/2017 10:42, Andrii Anisov wrote:
>>> Ah! Ops :-)
>>>
>>> DEBUG_KERNEL?
>>
>> You said DEBUG_DRIVER last time.
>> Both DEBUG_KERNEL and DEBUG_DRIVER are not used in code. DEBUG_KERNEL
>> is used through Kconfigs to make available debug options to other
>> different stuff, which consequently define DEBUG through appropriate
>> makefiles.
>> DEBUG_DRIVER enables DEBUG macro in base, opp and power makefiles.
>
> Technically this check should be done on any debug build for safety. So
> I would add new Kconfig DEBUG_KENREL that is selected by DEBUG_KERNEL.

Sent the e-mail too quickly, sorry. I meant adding DEBUG_XEN selected by 
DEBUG_KERNEL.

Cheers

-- 
Julien Grall

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

Re: [PATCH v3 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback

[Andrii Anisov]

Dear Konrad,

You are not correct here:
> But there are no Kconfig DEBUG - so you may as well just do
> #if 0
>
> around the code.

DEBUG macro is widely used in the kernel, just try grepping it through
the code. Elementary example is pr_debug definition which is resolved
through DEBUG macro, also reasonable pieces of debug code are widely
guarded by DEBUG macro.
DEBUG macro could be globally across drivers defined by
CONFIG_DEBUG_DRIVERS. See drivers/base/Makefile.

Sincerely,
Andrii Anisov.

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

Re: [PATCH v3 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback

[Andrii Anisov]

Yup,

Following is wrong:
> DEBUG macro could be globally across drivers defined by
> CONFIG_DEBUG_DRIVERS. See drivers/base/Makefile.
DEBUG is not defined globally. And there is no debug option to enable
DEBUG in drivers/xen/Makefile.
Should it be added?

Sincerely,
Andrii Anisov.

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

Re: [PATCH v3 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback

[Konrad Rzeszutek Wilk]

On Wed, Feb 01, 2017 at 12:46:32PM +0200, Andrii Anisov wrote:
> Yup,
> 
> Following is wrong:
> > DEBUG macro could be globally across drivers defined by
> > CONFIG_DEBUG_DRIVERS. See drivers/base/Makefile.
> DEBUG is not defined globally. And there is no debug option to enable
> DEBUG in drivers/xen/Makefile.
> Should it be added?

I am not exactly sure why you guys insist on having this, but
the lets leave it as #if 0 along with a comment saying why and what
the purpose of this is.

> 
> Sincerely,
> Andrii Anisov.

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

Re: [PATCH v3 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback

[Julien Grall]

Hi Konrad,

On 01/02/2017 14:25, Konrad Rzeszutek Wilk wrote:
> On Wed, Feb 01, 2017 at 12:46:32PM +0200, Andrii Anisov wrote:
>> Yup,
>>
>> Following is wrong:
>>> DEBUG macro could be globally across drivers defined by
>>> CONFIG_DEBUG_DRIVERS. See drivers/base/Makefile.
>> DEBUG is not defined globally. And there is no debug option to enable
>> DEBUG in drivers/xen/Makefile.
>> Should it be added?
>
> I am not exactly sure why you guys insist on having this, but
> the lets leave it as #if 0 along with a comment saying why and what
> the purpose of this is.

 From my understanding, the function has been implemented with the 
assumption of the page will always belongs to the domain and not foreign.

A user will unlikely know that it needs to remove #if 0 in order to 
check the driver is doing the right thing. So I think we should have an 
easy way to enable this code in kernel build.

If you are not happy with the Kconfig, what would be the other way to 
have this enabled without requiring the user to modify the code?

Cheers,

-- 
Julien Grall

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

Re: [PATCH v3 2/2] swiotlb-xen: implement xen_swiotlb_get_sgtable callback

[Konrad Rzeszutek Wilk]

On Wed, Feb 01, 2017 at 02:30:53PM +0000, Julien Grall wrote:
> Hi Konrad,
> 
> On 01/02/2017 14:25, Konrad Rzeszutek Wilk wrote:
> > On Wed, Feb 01, 2017 at 12:46:32PM +0200, Andrii Anisov wrote:
> > > Yup,
> > > 
> > > Following is wrong:
> > > > DEBUG macro could be globally across drivers defined by
> > > > CONFIG_DEBUG_DRIVERS. See drivers/base/Makefile.
> > > DEBUG is not defined globally. And there is no debug option to enable
> > > DEBUG in drivers/xen/Makefile.
> > > Should it be added?
> > 
> > I am not exactly sure why you guys insist on having this, but
> > the lets leave it as #if 0 along with a comment saying why and what
> > the purpose of this is.
> 
> From my understanding, the function has been implemented with the assumption
> of the page will always belongs to the domain and not foreign.
> 
> A user will unlikely know that it needs to remove #if 0 in order to check
> the driver is doing the right thing. So I think we should have an easy way
> to enable this code in kernel build.
> 
> If you are not happy with the Kconfig, what would be the other way to have
> this enabled without requiring the user to modify the code?

Just leave the #if 0 in the code and add a comment explaining why it is
there.
> 
> Cheers,
> 
> -- 
> Julien Grall

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