Re: [Xen-devel] [PATCH 07/11] swiotlb-xen: provide a single page-coherent.h header

2019-08-17 Thread Julien Grall 

Hi Christoph,

On 8/17/19 7:50 AM, Christoph Hellwig wrote:

On Fri, Aug 16, 2019 at 11:40:43PM +0100, Julien Grall wrote:

I am not sure I agree with this rename. The implementation of the helpers
are very Arm specific as this is assuming Dom0 is 1:1 mapped.

This was necessary due to the lack of IOMMU on Arm platforms back then.
But this is now a pain to get rid of it on newer platform...


So if you look at the final version of the header after the whole
series, what assumes a 1:1 mapping?  It all just is

if (pfn_valid())
local cache sync;
else
call into the arch code;


In the context of Xen Arm, the dev_addr is a host physical address. From 
my understanding pfn_valid() is dealing with a guest physical frame.


Therefore by passing PFN_DOWN(dev_addr) in argument you assume that the 
host and guest address spaces are the same.




are you concerned that the local cache sync might have to be split
up more for a non-1:1 map in that case?  We could just movea
the xen_dma_* routines into the arch instead of __xen_dma, but it
really helps to have a common interface header.
Moving xen_dma_* routines into the arch would be a good option. 
Although, I would still consider a stub version for arch not requiring 
specific DMA.


Cheers,

--
Julien Grall


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 07/11] swiotlb-xen: provide a single page-coherent.h header

2019-08-17 Thread Christoph Hellwig 
On Fri, Aug 16, 2019 at 11:40:43PM +0100, Julien Grall wrote:
> I am not sure I agree with this rename. The implementation of the helpers 
> are very Arm specific as this is assuming Dom0 is 1:1 mapped.
>
> This was necessary due to the lack of IOMMU on Arm platforms back then.
> But this is now a pain to get rid of it on newer platform...

So if you look at the final version of the header after the whole
series, what assumes a 1:1 mapping?  It all just is

if (pfn_valid())
local cache sync;
else
call into the arch code;

are you concerned that the local cache sync might have to be split
up more for a non-1:1 map in that case?  We could just move
the xen_dma_* routines into the arch instead of __xen_dma, but it
really helps to have a common interface header.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 07/11] swiotlb-xen: provide a single page-coherent.h header

2019-08-16 Thread Julien Grall 

Hi,

On 8/16/19 2:00 PM, Christoph Hellwig wrote:

Merge the various page-coherent.h files into a single one that either
provides prototypes or stubs depending on the need for cache
maintainance.

For extra benefits alo include  in the file
actually implementing the interfaces provided.

Signed-off-by: Christoph Hellwig 
---
  arch/arm/include/asm/xen/page-coherent.h   |  2 --
  arch/arm/xen/mm.c  |  1 +
  arch/arm64/include/asm/xen/page-coherent.h |  2 --
  arch/x86/include/asm/xen/page-coherent.h   | 22 --
  drivers/xen/swiotlb-xen.c  |  4 +---
  include/Kbuild |  2 +-
  include/xen/{arm => }/page-coherent.h  | 27 +++---


I am not sure I agree with this rename. The implementation of the 
helpers are very Arm specific as this is assuming Dom0 is 1:1 mapped.


This was necessary due to the lack of IOMMU on Arm platforms back then.
But this is now a pain to get rid of it on newer platform...

Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 07/11] swiotlb-xen: provide a single page-coherent.h header

2019-08-16 Thread Christoph Hellwig 
Merge the various page-coherent.h files into a single one that either
provides prototypes or stubs depending on the need for cache
maintainance.

For extra benefits alo include  in the file
actually implementing the interfaces provided.

Signed-off-by: Christoph Hellwig 
---
 arch/arm/include/asm/xen/page-coherent.h   |  2 --
 arch/arm/xen/mm.c  |  1 +
 arch/arm64/include/asm/xen/page-coherent.h |  2 --
 arch/x86/include/asm/xen/page-coherent.h   | 22 --
 drivers/xen/swiotlb-xen.c  |  4 +---
 include/Kbuild |  2 +-
 include/xen/{arm => }/page-coherent.h  | 27 +++---
 7 files changed, 27 insertions(+), 33 deletions(-)
 delete mode 100644 arch/arm/include/asm/xen/page-coherent.h
 delete mode 100644 arch/arm64/include/asm/xen/page-coherent.h
 delete mode 100644 arch/x86/include/asm/xen/page-coherent.h
 rename include/xen/{arm => }/page-coherent.h (76%)

diff --git a/arch/arm/include/asm/xen/page-coherent.h 
b/arch/arm/include/asm/xen/page-coherent.h
deleted file mode 100644
index 27e984977402..
--- a/arch/arm/include/asm/xen/page-coherent.h
+++ /dev/null
@@ -1,2 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#include 
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index a59980f1aa54..85482cdda1e5 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
diff --git a/arch/arm64/include/asm/xen/page-coherent.h 
b/arch/arm64/include/asm/xen/page-coherent.h
deleted file mode 100644
index 27e984977402..
--- a/arch/arm64/include/asm/xen/page-coherent.h
+++ /dev/null
@@ -1,2 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#include 
diff --git a/arch/x86/include/asm/xen/page-coherent.h 
b/arch/x86/include/asm/xen/page-coherent.h
deleted file mode 100644
index 8ee33c5edded..
--- a/arch/x86/include/asm/xen/page-coherent.h
+++ /dev/null
@@ -1,22 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _ASM_X86_XEN_PAGE_COHERENT_H
-#define _ASM_X86_XEN_PAGE_COHERENT_H
-
-#include 
-#include 
-
-static inline void xen_dma_map_page(struct device *hwdev, struct page *page,
-dma_addr_t dev_addr, unsigned long offset, size_t size,
-enum dma_data_direction dir, unsigned long attrs) { }
-
-static inline void xen_dma_unmap_page(struct device *hwdev, dma_addr_t handle,
-   size_t size, enum dma_data_direction dir,
-   unsigned long attrs) { }
-
-static inline void xen_dma_sync_single_for_cpu(struct device *hwdev,
-   dma_addr_t handle, size_t size, enum dma_data_direction dir) { }
-
-static inline void xen_dma_sync_single_for_device(struct device *hwdev,
-   dma_addr_t handle, size_t size, enum dma_data_direction dir) { }
-
-#endif /* _ASM_X86_XEN_PAGE_COHERENT_H */
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index f9dd4cb6e4b3..7b23929854e7 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -31,12 +31,10 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
-#include 
-#include 
-
 #include 
 /*
  * Used to do a quick range check in swiotlb_tbl_unmap_single and
diff --git a/include/Kbuild b/include/Kbuild
index c38f0d46b267..e2ae52ef9e1e 100644
--- a/include/Kbuild
+++ b/include/Kbuild
@@ -1189,7 +1189,6 @@ header-test-  += video/vga.h
 header-test-   += video/w100fb.h
 header-test-   += xen/acpi.h
 header-test-   += xen/arm/hypercall.h
-header-test-   += xen/arm/page-coherent.h
 header-test-   += xen/arm/page.h
 header-test-   += xen/balloon.h
 header-test-   += xen/events.h
@@ -1231,6 +1230,7 @@ header-test-  += xen/interface/xen.h
 header-test-   += xen/interface/xenpmu.h
 header-test-   += xen/mem-reservation.h
 header-test-   += xen/page.h
+header-test-   += xen/page-coherent.h
 header-test-   += xen/platform_pci.h
 header-test-   += xen/swiotlb-xen.h
 header-test-   += xen/xen-front-pgdir-shbuf.h
diff --git a/include/xen/arm/page-coherent.h b/include/xen/page-coherent.h
similarity index 76%
rename from include/xen/arm/page-coherent.h
rename to include/xen/page-coherent.h
index 4294a31305ca..7c32944de051 100644
--- a/include/xen/arm/page-coherent.h
+++ b/include/xen/page-coherent.h
@@ -1,10 +1,12 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-#ifndef _XEN_ARM_PAGE_COHERENT_H
-#define _XEN_ARM_PAGE_COHERENT_H
+#ifndef _XEN_PAGE_COHERENT_H
+#define _XEN_PAGE_COHERENT_H
 
 #include 
 #include 
 
+#if defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE) || \
+defined(CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU)
 void __xen_dma_map_page(struct device *hwdev, struct page *page,
 dma_addr_t dev_addr, unsigned long