[PATCH v4 3/7] [RFC] arm/arm64: introduce is_dma_coherent

[Stefano Stabellini]

Introduce a function to check whether a device is dma coherent.
---
arch/arm/include/asm/dma-mapping.h | 6 ++++++
arch/arm64/include/asm/dma-mapping.h | 5 +++++
2 files changed, 11 insertions(+)
diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index c45b61a..bededbb 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -126,6 +126,12 @@ static inline int set_arch_dma_coherent_ops(struct device *dev)
set_dma_ops(dev, &arm_coherent_dma_ops);
return 0;
}
+
+static inline bool is_dma_coherent(struct device *dev)
+{
+ return (__generic_dma_ops(dev) == &arm_coherent_dma_ops);
+}

Hmm, what about the IOMMU ops?

Maybe I should check __generic_dma_ops(dev) != &arm_dma_ops?
Do you have any better suggestions?

[Stefano Stabellini]

Post by Stefano Stabellini

Introduce a function to check whether a device is dma coherent.
---
arch/arm/include/asm/dma-mapping.h | 6 ++++++
arch/arm64/include/asm/dma-mapping.h | 5 +++++
2 files changed, 11 insertions(+)
diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index c45b61a..bededbb 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -126,6 +126,12 @@ static inline int set_arch_dma_coherent_ops(struct device *dev)
set_dma_ops(dev, &arm_coherent_dma_ops);
return 0;
}
+
+static inline bool is_dma_coherent(struct device *dev)
+{
+ return (__generic_dma_ops(dev) == &arm_coherent_dma_ops);
+}

Hmm, what about the IOMMU ops?

Maybe I should check __generic_dma_ops(dev) != &arm_dma_ops?
Do you have any better suggestions?

ping?

[Will Deacon]

Post by Stefano Stabellini

Introduce a function to check whether a device is dma coherent.
---
arch/arm/include/asm/dma-mapping.h | 6 ++++++
arch/arm64/include/asm/dma-mapping.h | 5 +++++
2 files changed, 11 insertions(+)
diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index c45b61a..bededbb 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -126,6 +126,12 @@ static inline int set_arch_dma_coherent_ops(struct device *dev)
set_dma_ops(dev, &arm_coherent_dma_ops);
return 0;
}
+
+static inline bool is_dma_coherent(struct device *dev)
+{
+ return (__generic_dma_ops(dev) == &arm_coherent_dma_ops);
+}

Hmm, what about the IOMMU ops?

Maybe I should check __generic_dma_ops(dev) != &arm_dma_ops?
Do you have any better suggestions?

ping?

Without any clear idea about why this is needed or how it's used, I don't
have any better ideas.

Will

[Stefano Stabellini]

Post by Will Deacon

Post by Stefano Stabellini

Introduce a function to check whether a device is dma coherent.
---
arch/arm/include/asm/dma-mapping.h | 6 ++++++
arch/arm64/include/asm/dma-mapping.h | 5 +++++
2 files changed, 11 insertions(+)
diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index c45b61a..bededbb 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -126,6 +126,12 @@ static inline int set_arch_dma_coherent_ops(struct device *dev)
set_dma_ops(dev, &arm_coherent_dma_ops);
return 0;
}
+
+static inline bool is_dma_coherent(struct device *dev)
+{
+ return (__generic_dma_ops(dev) == &arm_coherent_dma_ops);
+}

Hmm, what about the IOMMU ops?

Maybe I should check __generic_dma_ops(dev) != &arm_dma_ops?
Do you have any better suggestions?

ping?

Without any clear idea about why this is needed or how it's used, I don't
have any better ideas.

Thanks for the quick reply.

It is used in dom0 to figure out whether the device is not coherent: in
that case Dom0 is going to issue an hypercall and Xen is going to
execute the required cache maintenance operations on Dom0's behalf.

In practice for this use case iommu_dma_ops is not a problem, as the
device is never going to appear as being behind an SMMU in dom0.

[Konrad Rzeszutek Wilk]

Introduce an arch specific function to find out whether a particular dma
mapping operation needs to bounce on the swiotlb buffer.
On ARM and ARM64, if the page involved is a foreign page and the device
is not coherent, we need to bounce because at unmap time we cannot
execute any required cache maintenance operations (we don't know how to
find the pfn from the mfn).
No change of behaviour for x86.
---
arch/arm/include/asm/xen/page.h | 4 ++++
arch/arm/xen/mm.c | 7 +++++++
arch/x86/include/asm/xen/page.h | 7 +++++++
drivers/xen/swiotlb-xen.c | 5 ++++-
4 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/arch/arm/include/asm/xen/page.h b/arch/arm/include/asm/xen/page.h
index 135c24a..fb7839d 100644
--- a/arch/arm/include/asm/xen/page.h
+++ b/arch/arm/include/asm/xen/page.h
@@ -107,4 +107,8 @@ static inline bool set_phys_to_machine(unsigned long pfn, unsigned long mfn)
#define xen_remap(cookie, size) ioremap_cache((cookie), (size))
#define xen_unmap(cookie) iounmap((cookie))
+bool xen_arch_need_swiotlb(struct device *dev,
+ unsigned long pfn,
+ unsigned long mfn);
+
#endif /* _ASM_ARM_XEN_PAGE_H */
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index 9d460e0..0c2a75a 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -139,6 +139,13 @@ void xen_dma_sync_single_for_device(struct device *hwdev,
__xen_dma_page_cpu_to_dev(hwdev, handle, size, dir);
}
+bool xen_arch_need_swiotlb(struct device *dev,
+ unsigned long pfn,
+ unsigned long mfn)
+{
+ return ((pfn != mfn) && !is_dma_coherent(dev));
+}
+
int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
unsigned int address_bits,
dma_addr_t *dma_handle)
diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
index c949923..0a45242 100644
--- a/arch/x86/include/asm/xen/page.h
+++ b/arch/x86/include/asm/xen/page.h
@@ -236,4 +236,11 @@ void make_lowmem_page_readwrite(void *vaddr);
#define xen_remap(cookie, size) ioremap((cookie), (size));
#define xen_unmap(cookie) iounmap((cookie))
+static inline bool xen_arch_need_swiotlb(struct device *dev,
+ unsigned long pfn,
+ unsigned long mfn)

Something funny happend here.

+{
+ return false;
+}

Why not make this an macro?

The xen_unmap and xen_remap are that.

+
#endif /* _ASM_X86_XEN_PAGE_H */
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index ebd8f21..ac0bd60 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -399,7 +399,9 @@ dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page,
* buffering it.
*/
if (dma_capable(dev, dev_addr, size) &&
- !range_straddles_page_boundary(phys, size) && !swiotlb_force) {
+ !range_straddles_page_boundary(phys, size) &&
+ !xen_arch_need_swiotlb(dev, PFN_DOWN(phys), PFN_DOWN(dev_addr)) &&
+ !swiotlb_force) {
/* we are not interested in the dma_addr returned by
* xen_dma_map_page, only in the potential cache flushes executed
* by the function. */
@@ -557,6 +559,7 @@ xen_swiotlb_map_sg_attrs(struct device *hwdev, struct scatterlist *sgl,
dma_addr_t dev_addr = xen_phys_to_bus(paddr);
if (swiotlb_force ||
+ xen_arch_need_swiotlb(hwdev, PFN_DOWN(paddr), PFN_DOWN(dev_addr)) ||
!dma_capable(hwdev, dev_addr, sg->length) ||
range_straddles_page_boundary(paddr, sg->length)) {
phys_addr_t map = swiotlb_tbl_map_single(hwdev,
--
1.7.10.4

[David Vrabel]

Post by Konrad Rzeszutek Wilk

Introduce an arch specific function to find out whether a particular dma
mapping operation needs to bounce on the swiotlb buffer.
On ARM and ARM64, if the page involved is a foreign page and the device
is not coherent, we need to bounce because at unmap time we cannot
execute any required cache maintenance operations (we don't know how to
find the pfn from the mfn).

...

Post by Konrad Rzeszutek Wilk

--- a/arch/x86/include/asm/xen/page.h
+++ b/arch/x86/include/asm/xen/page.h
@@ -236,4 +236,11 @@ void make_lowmem_page_readwrite(void *vaddr);
#define xen_remap(cookie, size) ioremap((cookie), (size));
#define xen_unmap(cookie) iounmap((cookie))
+static inline bool xen_arch_need_swiotlb(struct device *dev,
+ unsigned long pfn,
+ unsigned long mfn)
+{
+ return false;
+}

Why not make this an macro?

Because macros are evil and inline functions are preferred.

David

[Konrad Rzeszutek Wilk]

Post by David Vrabel

Post by Konrad Rzeszutek Wilk

Introduce an arch specific function to find out whether a particular dma
mapping operation needs to bounce on the swiotlb buffer.
On ARM and ARM64, if the page involved is a foreign page and the device
is not coherent, we need to bounce because at unmap time we cannot
execute any required cache maintenance operations (we don't know how to
find the pfn from the mfn).

...

Post by Konrad Rzeszutek Wilk

--- a/arch/x86/include/asm/xen/page.h
+++ b/arch/x86/include/asm/xen/page.h
@@ -236,4 +236,11 @@ void make_lowmem_page_readwrite(void *vaddr);
#define xen_remap(cookie, size) ioremap((cookie), (size));
#define xen_unmap(cookie) iounmap((cookie))
+static inline bool xen_arch_need_swiotlb(struct device *dev,
+ unsigned long pfn,
+ unsigned long mfn)
+{
+ return false;
+}

Why not make this an macro?

Because macros are evil and inline functions are preferred.

<laughs>

Fair enough.

Post by David Vrabel
David

[Ian Campbell]

The feature has been removed from Xen. Also Linux cannot use it on ARM32
without CONFIG_ARM_LPAE.

Acked-by: Ian Campbell <***@citrix.com>

[Ian Campbell]

Use is_dma_coherent to check whether we need to issue cache maintenance
operations rather than checking on the existence of a particular
dma_ops function for the device.

Acked-by: Ian Campbell <***@citrix.com>

[Ian Campbell]

Merge xen/mm32.c into xen/mm.c.
As a consequence the code gets compiled on arm64 too: introduce a few
compat functions to actually be able to compile it.

Acked-by: Ian Campbell <***@citrix.com>

[Ian Campbell]

Introduce support for new hypercall GNTTABOP_cache_flush.
Use it to perform cache flashing on pages used for dma when necessary.
If GNTTABOP_cache_flush is supported by the hypervisor, we don't need to
bounce dma map operations that involve foreign grants and non-coherent
devices.
---
- add comment;
- avoid bouncing dma map operations that involve foreign grants and
non-coherent devices if GNTTABOP_cache_flush is provided by Xen.
- fix the cache maintenance op call to match what Linux does natively;
- update the hypercall interface to match Xen.
- update the hypercall interface to match Xen;
- call the interface on a single page at a time.
---
arch/arm/xen/mm.c | 41 ++++++++++++++++++++++++++++++-----
include/xen/interface/grant_table.h | 19 ++++++++++++++++
2 files changed, 54 insertions(+), 6 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index 0c2a75a..21db123 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -11,6 +11,7 @@
#include <linux/swiotlb.h>
#include <xen/xen.h>
+#include <xen/interface/grant_table.h>
#include <xen/interface/memory.h>
#include <xen/swiotlb-xen.h>
@@ -44,6 +45,8 @@ static inline void *kmap_high_get(struct page *page)
static inline void kunmap_high(struct page *page) {}
#endif
+static bool hypercall_flush = false;

Would be nice to include the word "cache" (or at least cflush) in this.

-arch_initcall(xen_mm_init);
+arch_initcall(xen_mm_init)

I think this is stray?

Acked-by: Ian Campbell <***@citrix.com>