"Consolidate get_dma_ops" breaks Xen on ARM

"Consolidate get_dma_ops" breaks Xen on ARM

[Stefano Stabellini]

Hi all,

Julien Grall (CC'ed) discovered that the following commit breaks Xen on ARM:


  commit 815dd18788fe0d41899f51b91d0560279cf16b0d
  Author: Bart Van Assche <bart.vanassche@sandisk.com>
  Date:   Fri Jan 20 13:04:04 2017 -0800
  
      treewide: Consolidate get_dma_ops() implementations
   

The relevant changes are:

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index c7432d6..7166569 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -23,12 +23,12 @@ static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
 	return &arm_dma_ops;
 }
 
-static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
+static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus)
 {
 	if (xen_initial_domain())
 		return xen_dma_ops;
 	else
-		return __generic_dma_ops(dev);
+		return __generic_dma_ops(NULL);
 }
 
 #define HAVE_ARCH_DMA_SUPPORTED 1
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index e97f23e..ab87108 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -164,6 +164,13 @@ int dma_mmap_from_coherent(struct device *dev, struct vm_area_struct *vma,
 
 #ifdef CONFIG_HAS_DMA
 #include <asm/dma-mapping.h>
+static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
+{
+	if (dev && dev->dma_ops)
+		return dev->dma_ops;
+	return get_arch_dma_ops(dev ? dev->bus : NULL);
+}
+
 static inline void set_dma_ops(struct device *dev,
 			       const struct dma_map_ops *dma_ops)
 {


I think the reason is that, as you can see, if (dev && dev->dma_ops),
dev->dma_ops is returned, while before this changes, xen_dma_ops was
returned on Xen on ARM.

Unfortunately DMA cannot work properly without using the appropriate
xen_dma_ops. See drivers/xen/swiotlb-xen.c and arch/arm/xen/mm.c for
more details. (The problem is easy to spot, but I wasn't CC'ed on the
patch.)

I don't know how to solve this problem without introducing some sort of
if (xen()) in include/linux/dma-mapping.h.

Re: "Consolidate get_dma_ops" breaks Xen on ARM

[Bart Van Assche]

On 04/10/17 17:31, Stefano Stabellini wrote:
> I think the reason is that, as you can see, if (dev && dev->dma_ops),
> dev->dma_ops is returned, while before this changes, xen_dma_ops was
> returned on Xen on ARM.
>
> Unfortunately DMA cannot work properly without using the appropriate
> xen_dma_ops. See drivers/xen/swiotlb-xen.c and arch/arm/xen/mm.c for
> more details. (The problem is easy to spot, but I wasn't CC'ed on the
> patch.)
>
> I don't know how to solve this problem without introducing some sort of
> if (xen()) in include/linux/dma-mapping.h.

Sorry but I don't have access to an ARM development system. Does your 
comment apply to dev == NULL only, dev != NULL only or perhaps to both? 
If your comment applies to dev != NULL only, can you check whether 
adding something like set_dma_ops(dev, get_arch_dma_ops(NULL)) to the 
appropriate ARM arch_setup_dma_ops() function is sufficient?

Thanks,

Bart.

Re: "Consolidate get_dma_ops" breaks Xen on ARM

[Julien Grall]

Hi Bart,

On 11/04/17 02:14, Bart Van Assche wrote:
> On 04/10/17 17:31, Stefano Stabellini wrote:
>> I think the reason is that, as you can see, if (dev && dev->dma_ops),
>> dev->dma_ops is returned, while before this changes, xen_dma_ops was
>> returned on Xen on ARM.
>>
>> Unfortunately DMA cannot work properly without using the appropriate
>> xen_dma_ops. See drivers/xen/swiotlb-xen.c and arch/arm/xen/mm.c for
>> more details. (The problem is easy to spot, but I wasn't CC'ed on the
>> patch.)
>>
>> I don't know how to solve this problem without introducing some sort of
>> if (xen()) in include/linux/dma-mapping.h.
> 
> Sorry but I don't have access to an ARM development system. Does your 
> comment apply to dev == NULL only, dev != NULL only or perhaps to both? 
> If your comment applies to dev != NULL only, can you check whether 
> adding something like set_dma_ops(dev, get_arch_dma_ops(NULL)) to the 
> appropriate ARM arch_setup_dma_ops() function is sufficient?

If I understand correctly, set_dma_ops will replace dev->dma_ops with
Xen DMA ops.

However, Xen DMA ops will need in some places to call the device 
specific DMA ops (see __generic_dma_ops(...)). So I think replacing
dev->dma_ops is not a solution here.

The hackish patch below is fixing the problem for both ARM64 and ARM32.

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 0977317c6835..43a73ddeec7a 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -174,6 +174,8 @@ int dma_mmap_from_coherent(struct device *dev, struct vm_area_struct *vma,
 #include <asm/dma-mapping.h>
 static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
 {
+       if (xen_initial_domain())
+              return xen_dma_ops;
        if (dev && dev->dma_ops)
                return dev->dma_ops;
        return get_arch_dma_ops(dev ? dev->bus : NULL);

It is not nice as this is common code, but I can't find a better solution
so far. Any opinions?

Cheers,

-- 
Julien Grall

"Consolidate get_dma_ops" breaks Xen on ARM

[Julien Grall]

Hi Bart,

On 11/04/17 02:14, Bart Van Assche wrote:
> On 04/10/17 17:31, Stefano Stabellini wrote:
>> I think the reason is that, as you can see, if (dev && dev->dma_ops),
>> dev->dma_ops is returned, while before this changes, xen_dma_ops was
>> returned on Xen on ARM.
>>
>> Unfortunately DMA cannot work properly without using the appropriate
>> xen_dma_ops. See drivers/xen/swiotlb-xen.c and arch/arm/xen/mm.c for
>> more details. (The problem is easy to spot, but I wasn't CC'ed on the
>> patch.)
>>
>> I don't know how to solve this problem without introducing some sort of
>> if (xen()) in include/linux/dma-mapping.h.
> 
> Sorry but I don't have access to an ARM development system. Does your 
> comment apply to dev == NULL only, dev != NULL only or perhaps to both? 
> If your comment applies to dev != NULL only, can you check whether 
> adding something like set_dma_ops(dev, get_arch_dma_ops(NULL)) to the 
> appropriate ARM arch_setup_dma_ops() function is sufficient?

If I understand correctly, set_dma_ops will replace dev->dma_ops with
Xen DMA ops.

However, Xen DMA ops will need in some places to call the device 
specific DMA ops (see __generic_dma_ops(...)). So I think replacing
dev->dma_ops is not a solution here.

The hackish patch below is fixing the problem for both ARM64 and ARM32.

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 0977317c6835..43a73ddeec7a 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -174,6 +174,8 @@ int dma_mmap_from_coherent(struct device *dev, struct vm_area_struct *vma,
 #include <asm/dma-mapping.h>
 static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
 {
+       if (xen_initial_domain())
+              return xen_dma_ops;
        if (dev && dev->dma_ops)
                return dev->dma_ops;
        return get_arch_dma_ops(dev ? dev->bus : NULL);

It is not nice as this is common code, but I can't find a better solution
so far. Any opinions?

Cheers,

-- 
Julien Grall

Re: "Consolidate get_dma_ops" breaks Xen on ARM

[Catalin Marinas]

On Tue, Apr 11, 2017 at 01:43:28PM +0100, Julien Grall wrote:
> On 11/04/17 02:14, Bart Van Assche wrote:
> > On 04/10/17 17:31, Stefano Stabellini wrote:
> >> I think the reason is that, as you can see, if (dev && dev->dma_ops),
> >> dev->dma_ops is returned, while before this changes, xen_dma_ops was
> >> returned on Xen on ARM.
> >>
> >> Unfortunately DMA cannot work properly without using the appropriate
> >> xen_dma_ops. See drivers/xen/swiotlb-xen.c and arch/arm/xen/mm.c for
> >> more details. (The problem is easy to spot, but I wasn't CC'ed on the
> >> patch.)
> >>
> >> I don't know how to solve this problem without introducing some sort of
> >> if (xen()) in include/linux/dma-mapping.h.
> > 
> > Sorry but I don't have access to an ARM development system. Does your 
> > comment apply to dev == NULL only, dev != NULL only or perhaps to both? 
> > If your comment applies to dev != NULL only, can you check whether 
> > adding something like set_dma_ops(dev, get_arch_dma_ops(NULL)) to the 
> > appropriate ARM arch_setup_dma_ops() function is sufficient?
> 
> If I understand correctly, set_dma_ops will replace dev->dma_ops with
> Xen DMA ops.
> 
> However, Xen DMA ops will need in some places to call the device 
> specific DMA ops (see __generic_dma_ops(...)). So I think replacing
> dev->dma_ops is not a solution here.
> 
> The hackish patch below is fixing the problem for both ARM64 and ARM32.
> 
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 0977317c6835..43a73ddeec7a 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -174,6 +174,8 @@ int dma_mmap_from_coherent(struct device *dev, struct vm_area_struct *vma,
>  #include <asm/dma-mapping.h>
>  static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
>  {
> +       if (xen_initial_domain())
> +              return xen_dma_ops;
>         if (dev && dev->dma_ops)
>                 return dev->dma_ops;
>         return get_arch_dma_ops(dev ? dev->bus : NULL);

If we do this, I guess there is no need to check for
xen_initial_domain() in the get_arch_dma_ops() function. Anyway, this
hunk would break the other architectures since xen_dma_ops is only
defined for arm and arm64.

> It is not nice as this is common code, but I can't find a better solution
> so far. Any opinions?

A different hack would be to avoid the generic get_dma_ops
implementation on arm with some #ifdef hacks above.

Yet another way would be for dom0 to always set dev->dma_ops to
xen_dma_ops and preserve the real dma_ops somewhere under dev->archdata.
You could intercept the arch_setup_dma_ops() function for this or use
bus_register_notifier() (though I think the former is easier). The Xen
code making use of the real dma_ops would have to dig them out from
dev->archdata.

-- 
Catalin

"Consolidate get_dma_ops" breaks Xen on ARM

[Catalin Marinas]

On Tue, Apr 11, 2017 at 01:43:28PM +0100, Julien Grall wrote:
> On 11/04/17 02:14, Bart Van Assche wrote:
> > On 04/10/17 17:31, Stefano Stabellini wrote:
> >> I think the reason is that, as you can see, if (dev && dev->dma_ops),
> >> dev->dma_ops is returned, while before this changes, xen_dma_ops was
> >> returned on Xen on ARM.
> >>
> >> Unfortunately DMA cannot work properly without using the appropriate
> >> xen_dma_ops. See drivers/xen/swiotlb-xen.c and arch/arm/xen/mm.c for
> >> more details. (The problem is easy to spot, but I wasn't CC'ed on the
> >> patch.)
> >>
> >> I don't know how to solve this problem without introducing some sort of
> >> if (xen()) in include/linux/dma-mapping.h.
> > 
> > Sorry but I don't have access to an ARM development system. Does your 
> > comment apply to dev == NULL only, dev != NULL only or perhaps to both? 
> > If your comment applies to dev != NULL only, can you check whether 
> > adding something like set_dma_ops(dev, get_arch_dma_ops(NULL)) to the 
> > appropriate ARM arch_setup_dma_ops() function is sufficient?
> 
> If I understand correctly, set_dma_ops will replace dev->dma_ops with
> Xen DMA ops.
> 
> However, Xen DMA ops will need in some places to call the device 
> specific DMA ops (see __generic_dma_ops(...)). So I think replacing
> dev->dma_ops is not a solution here.
> 
> The hackish patch below is fixing the problem for both ARM64 and ARM32.
> 
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 0977317c6835..43a73ddeec7a 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -174,6 +174,8 @@ int dma_mmap_from_coherent(struct device *dev, struct vm_area_struct *vma,
>  #include <asm/dma-mapping.h>
>  static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
>  {
> +       if (xen_initial_domain())
> +              return xen_dma_ops;
>         if (dev && dev->dma_ops)
>                 return dev->dma_ops;
>         return get_arch_dma_ops(dev ? dev->bus : NULL);

If we do this, I guess there is no need to check for
xen_initial_domain() in the get_arch_dma_ops() function. Anyway, this
hunk would break the other architectures since xen_dma_ops is only
defined for arm and arm64.

> It is not nice as this is common code, but I can't find a better solution
> so far. Any opinions?

A different hack would be to avoid the generic get_dma_ops
implementation on arm with some #ifdef hacks above.

Yet another way would be for dom0 to always set dev->dma_ops to
xen_dma_ops and preserve the real dma_ops somewhere under dev->archdata.
You could intercept the arch_setup_dma_ops() function for this or use
bus_register_notifier() (though I think the former is easier). The Xen
code making use of the real dma_ops would have to dig them out from
dev->archdata.

-- 
Catalin

Re: "Consolidate get_dma_ops" breaks Xen on ARM

[Stefano Stabellini]

On Tue, 11 Apr 2017, Catalin Marinas wrote:
> On Tue, Apr 11, 2017 at 01:43:28PM +0100, Julien Grall wrote:
> > On 11/04/17 02:14, Bart Van Assche wrote:
> > > On 04/10/17 17:31, Stefano Stabellini wrote:
> > >> I think the reason is that, as you can see, if (dev && dev->dma_ops),
> > >> dev->dma_ops is returned, while before this changes, xen_dma_ops was
> > >> returned on Xen on ARM.
> > >>
> > >> Unfortunately DMA cannot work properly without using the appropriate
> > >> xen_dma_ops. See drivers/xen/swiotlb-xen.c and arch/arm/xen/mm.c for
> > >> more details. (The problem is easy to spot, but I wasn't CC'ed on the
> > >> patch.)
> > >>
> > >> I don't know how to solve this problem without introducing some sort of
> > >> if (xen()) in include/linux/dma-mapping.h.
> > > 
> > > Sorry but I don't have access to an ARM development system. Does your 
> > > comment apply to dev == NULL only, dev != NULL only or perhaps to both? 
> > > If your comment applies to dev != NULL only, can you check whether 
> > > adding something like set_dma_ops(dev, get_arch_dma_ops(NULL)) to the 
> > > appropriate ARM arch_setup_dma_ops() function is sufficient?
> > 
> > If I understand correctly, set_dma_ops will replace dev->dma_ops with
> > Xen DMA ops.
> > 
> > However, Xen DMA ops will need in some places to call the device 
> > specific DMA ops (see __generic_dma_ops(...)). So I think replacing
> > dev->dma_ops is not a solution here.
> > 
> > The hackish patch below is fixing the problem for both ARM64 and ARM32.
> > 
> > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > index 0977317c6835..43a73ddeec7a 100644
> > --- a/include/linux/dma-mapping.h
> > +++ b/include/linux/dma-mapping.h
> > @@ -174,6 +174,8 @@ int dma_mmap_from_coherent(struct device *dev, struct vm_area_struct *vma,
> >  #include <asm/dma-mapping.h>
> >  static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
> >  {
> > +       if (xen_initial_domain())
> > +              return xen_dma_ops;
> >         if (dev && dev->dma_ops)
> >                 return dev->dma_ops;
> >         return get_arch_dma_ops(dev ? dev->bus : NULL);
> 
> If we do this, I guess there is no need to check for
> xen_initial_domain() in the get_arch_dma_ops() function. Anyway, this
> hunk would break the other architectures since xen_dma_ops is only
> defined for arm and arm64.
> 
> > It is not nice as this is common code, but I can't find a better solution
> > so far. Any opinions?
> 
> A different hack would be to avoid the generic get_dma_ops
> implementation on arm with some #ifdef hacks above.
> 
> Yet another way would be for dom0 to always set dev->dma_ops to
> xen_dma_ops and preserve the real dma_ops somewhere under dev->archdata.
> You could intercept the arch_setup_dma_ops() function for this or use
> bus_register_notifier() (though I think the former is easier). The Xen
> code making use of the real dma_ops would have to dig them out from
> dev->archdata.

This is a good suggestion, Catalin. Thank you. See below. Is that what
you have in mind? Julien could you test it, please? If it is the right
approach, I'll submit the patch properly and rename __generic_dma_ops to
xen_generic_dma_ops or something.

diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
index 220ba20..36ec9c8 100644
--- a/arch/arm/include/asm/device.h
+++ b/arch/arm/include/asm/device.h
@@ -16,6 +16,9 @@ struct dev_archdata {
 #ifdef CONFIG_ARM_DMA_USE_IOMMU
 	struct dma_iommu_mapping	*mapping;
 #endif
+#ifdef CONFIG_XEN
+	const struct dma_map_ops *dev_dma_ops;
+#endif
 	bool dma_coherent;
 };
 
diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index 7166569..680d3f3 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -16,19 +16,9 @@
 extern const struct dma_map_ops arm_dma_ops;
 extern const struct dma_map_ops arm_coherent_dma_ops;
 
-static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
-{
-	if (dev && dev->dma_ops)
-		return dev->dma_ops;
-	return &arm_dma_ops;
-}
-
 static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus)
 {
-	if (xen_initial_domain())
-		return xen_dma_ops;
-	else
-		return __generic_dma_ops(NULL);
+	return &arm_dma_ops;
 }
 
 #define HAVE_ARCH_DMA_SUPPORTED 1
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 63eabb0..82d61eb 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2396,6 +2396,11 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 		dma_ops = arm_get_dma_map_ops(coherent);
 
 	set_dma_ops(dev, dma_ops);
+
+	if (xen_initial_domain()) {
+		dev->archdata.dev_dma_ops = dev->dma_ops;
+		dev->dma_ops = xen_dma_ops;
+	}
 }
 
 void arch_teardown_dma_ops(struct device *dev)
diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index 73d5bab..5a5fa47 100644
--- a/arch/arm64/include/asm/device.h
+++ b/arch/arm64/include/asm/device.h
@@ -20,6 +20,9 @@ struct dev_archdata {
 #ifdef CONFIG_IOMMU_API
 	void *iommu;			/* private IOMMU data */
 #endif
+#ifdef CONFIG_XEN
+	const struct dma_map_ops *dev_dma_ops;
+#endif
 	bool dma_coherent;
 };
 
diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index 505756c..5392dbe 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -27,11 +27,8 @@
 #define DMA_ERROR_CODE	(~(dma_addr_t)0)
 extern const struct dma_map_ops dummy_dma_ops;
 
-static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
+static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus)
 {
-	if (dev && dev->dma_ops)
-		return dev->dma_ops;
-
 	/*
 	 * We expect no ISA devices, and all other DMA masters are expected to
 	 * have someone call arch_setup_dma_ops at device creation time.
@@ -39,14 +36,6 @@ static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
 	return &dummy_dma_ops;
 }
 
-static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus)
-{
-	if (xen_initial_domain())
-		return xen_dma_ops;
-	else
-		return __generic_dma_ops(NULL);
-}
-
 void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 			const struct iommu_ops *iommu, bool coherent);
 #define arch_setup_dma_ops	arch_setup_dma_ops
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 81cdb2e..e574c39 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -977,4 +977,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 
 	dev->archdata.dma_coherent = coherent;
 	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
+
+	if (xen_initial_domain()) {
+		dev->archdata.dev_dma_ops = dev->dma_ops;
+		dev->dma_ops = xen_dma_ops;
+	}
 }
diff --git a/include/xen/arm/page-coherent.h b/include/xen/arm/page-coherent.h
index 95ce6ac..b0a2bfc 100644
--- a/include/xen/arm/page-coherent.h
+++ b/include/xen/arm/page-coherent.h
@@ -2,8 +2,16 @@
 #define _ASM_ARM_XEN_PAGE_COHERENT_H
 
 #include <asm/page.h>
+#include <asm/dma-mapping.h>
 #include <linux/dma-mapping.h>
 
+static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
+{
+	if (dev && dev->archdata.dev_dma_ops)
+		return dev->archdata.dev_dma_ops;
+	return get_arch_dma_ops(NULL);
+}
+
 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);

"Consolidate get_dma_ops" breaks Xen on ARM

[Stefano Stabellini]

On Tue, 11 Apr 2017, Catalin Marinas wrote:
> On Tue, Apr 11, 2017 at 01:43:28PM +0100, Julien Grall wrote:
> > On 11/04/17 02:14, Bart Van Assche wrote:
> > > On 04/10/17 17:31, Stefano Stabellini wrote:
> > >> I think the reason is that, as you can see, if (dev && dev->dma_ops),
> > >> dev->dma_ops is returned, while before this changes, xen_dma_ops was
> > >> returned on Xen on ARM.
> > >>
> > >> Unfortunately DMA cannot work properly without using the appropriate
> > >> xen_dma_ops. See drivers/xen/swiotlb-xen.c and arch/arm/xen/mm.c for
> > >> more details. (The problem is easy to spot, but I wasn't CC'ed on the
> > >> patch.)
> > >>
> > >> I don't know how to solve this problem without introducing some sort of
> > >> if (xen()) in include/linux/dma-mapping.h.
> > > 
> > > Sorry but I don't have access to an ARM development system. Does your 
> > > comment apply to dev == NULL only, dev != NULL only or perhaps to both? 
> > > If your comment applies to dev != NULL only, can you check whether 
> > > adding something like set_dma_ops(dev, get_arch_dma_ops(NULL)) to the 
> > > appropriate ARM arch_setup_dma_ops() function is sufficient?
> > 
> > If I understand correctly, set_dma_ops will replace dev->dma_ops with
> > Xen DMA ops.
> > 
> > However, Xen DMA ops will need in some places to call the device 
> > specific DMA ops (see __generic_dma_ops(...)). So I think replacing
> > dev->dma_ops is not a solution here.
> > 
> > The hackish patch below is fixing the problem for both ARM64 and ARM32.
> > 
> > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > index 0977317c6835..43a73ddeec7a 100644
> > --- a/include/linux/dma-mapping.h
> > +++ b/include/linux/dma-mapping.h
> > @@ -174,6 +174,8 @@ int dma_mmap_from_coherent(struct device *dev, struct vm_area_struct *vma,
> >  #include <asm/dma-mapping.h>
> >  static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
> >  {
> > +       if (xen_initial_domain())
> > +              return xen_dma_ops;
> >         if (dev && dev->dma_ops)
> >                 return dev->dma_ops;
> >         return get_arch_dma_ops(dev ? dev->bus : NULL);
> 
> If we do this, I guess there is no need to check for
> xen_initial_domain() in the get_arch_dma_ops() function. Anyway, this
> hunk would break the other architectures since xen_dma_ops is only
> defined for arm and arm64.
> 
> > It is not nice as this is common code, but I can't find a better solution
> > so far. Any opinions?
> 
> A different hack would be to avoid the generic get_dma_ops
> implementation on arm with some #ifdef hacks above.
> 
> Yet another way would be for dom0 to always set dev->dma_ops to
> xen_dma_ops and preserve the real dma_ops somewhere under dev->archdata.
> You could intercept the arch_setup_dma_ops() function for this or use
> bus_register_notifier() (though I think the former is easier). The Xen
> code making use of the real dma_ops would have to dig them out from
> dev->archdata.

This is a good suggestion, Catalin. Thank you. See below. Is that what
you have in mind? Julien could you test it, please? If it is the right
approach, I'll submit the patch properly and rename __generic_dma_ops to
xen_generic_dma_ops or something.

diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
index 220ba20..36ec9c8 100644
--- a/arch/arm/include/asm/device.h
+++ b/arch/arm/include/asm/device.h
@@ -16,6 +16,9 @@ struct dev_archdata {
 #ifdef CONFIG_ARM_DMA_USE_IOMMU
 	struct dma_iommu_mapping	*mapping;
 #endif
+#ifdef CONFIG_XEN
+	const struct dma_map_ops *dev_dma_ops;
+#endif
 	bool dma_coherent;
 };
 
diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index 7166569..680d3f3 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -16,19 +16,9 @@
 extern const struct dma_map_ops arm_dma_ops;
 extern const struct dma_map_ops arm_coherent_dma_ops;
 
-static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
-{
-	if (dev && dev->dma_ops)
-		return dev->dma_ops;
-	return &arm_dma_ops;
-}
-
 static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus)
 {
-	if (xen_initial_domain())
-		return xen_dma_ops;
-	else
-		return __generic_dma_ops(NULL);
+	return &arm_dma_ops;
 }
 
 #define HAVE_ARCH_DMA_SUPPORTED 1
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 63eabb0..82d61eb 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2396,6 +2396,11 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 		dma_ops = arm_get_dma_map_ops(coherent);
 
 	set_dma_ops(dev, dma_ops);
+
+	if (xen_initial_domain()) {
+		dev->archdata.dev_dma_ops = dev->dma_ops;
+		dev->dma_ops = xen_dma_ops;
+	}
 }
 
 void arch_teardown_dma_ops(struct device *dev)
diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index 73d5bab..5a5fa47 100644
--- a/arch/arm64/include/asm/device.h
+++ b/arch/arm64/include/asm/device.h
@@ -20,6 +20,9 @@ struct dev_archdata {
 #ifdef CONFIG_IOMMU_API
 	void *iommu;			/* private IOMMU data */
 #endif
+#ifdef CONFIG_XEN
+	const struct dma_map_ops *dev_dma_ops;
+#endif
 	bool dma_coherent;
 };
 
diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index 505756c..5392dbe 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -27,11 +27,8 @@
 #define DMA_ERROR_CODE	(~(dma_addr_t)0)
 extern const struct dma_map_ops dummy_dma_ops;
 
-static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
+static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus)
 {
-	if (dev && dev->dma_ops)
-		return dev->dma_ops;
-
 	/*
 	 * We expect no ISA devices, and all other DMA masters are expected to
 	 * have someone call arch_setup_dma_ops at device creation time.
@@ -39,14 +36,6 @@ static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
 	return &dummy_dma_ops;
 }
 
-static inline const struct dma_map_ops *get_arch_dma_ops(struct bus_type *bus)
-{
-	if (xen_initial_domain())
-		return xen_dma_ops;
-	else
-		return __generic_dma_ops(NULL);
-}
-
 void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 			const struct iommu_ops *iommu, bool coherent);
 #define arch_setup_dma_ops	arch_setup_dma_ops
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 81cdb2e..e574c39 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -977,4 +977,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 
 	dev->archdata.dma_coherent = coherent;
 	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
+
+	if (xen_initial_domain()) {
+		dev->archdata.dev_dma_ops = dev->dma_ops;
+		dev->dma_ops = xen_dma_ops;
+	}
 }
diff --git a/include/xen/arm/page-coherent.h b/include/xen/arm/page-coherent.h
index 95ce6ac..b0a2bfc 100644
--- a/include/xen/arm/page-coherent.h
+++ b/include/xen/arm/page-coherent.h
@@ -2,8 +2,16 @@
 #define _ASM_ARM_XEN_PAGE_COHERENT_H
 
 #include <asm/page.h>
+#include <asm/dma-mapping.h>
 #include <linux/dma-mapping.h>
 
+static inline const struct dma_map_ops *__generic_dma_ops(struct device *dev)
+{
+	if (dev && dev->archdata.dev_dma_ops)
+		return dev->archdata.dev_dma_ops;
+	return get_arch_dma_ops(NULL);
+}
+
 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);

Re: "Consolidate get_dma_ops" breaks Xen on ARM

[Catalin Marinas]

On Tue, Apr 11, 2017 at 04:39:09PM -0700, Stefano Stabellini wrote:
> On Tue, 11 Apr 2017, Catalin Marinas wrote:
> > On Tue, Apr 11, 2017 at 01:43:28PM +0100, Julien Grall wrote:
> > > On 11/04/17 02:14, Bart Van Assche wrote:
> > > > On 04/10/17 17:31, Stefano Stabellini wrote:
> > > >> I think the reason is that, as you can see, if (dev && dev->dma_ops),
> > > >> dev->dma_ops is returned, while before this changes, xen_dma_ops was
> > > >> returned on Xen on ARM.
> > > >>
> > > >> Unfortunately DMA cannot work properly without using the appropriate
> > > >> xen_dma_ops. See drivers/xen/swiotlb-xen.c and arch/arm/xen/mm.c for
> > > >> more details. (The problem is easy to spot, but I wasn't CC'ed on the
> > > >> patch.)
> > > >>
> > > >> I don't know how to solve this problem without introducing some sort of
> > > >> if (xen()) in include/linux/dma-mapping.h.
> > > > 
> > > > Sorry but I don't have access to an ARM development system. Does your 
> > > > comment apply to dev == NULL only, dev != NULL only or perhaps to both? 
> > > > If your comment applies to dev != NULL only, can you check whether 
> > > > adding something like set_dma_ops(dev, get_arch_dma_ops(NULL)) to the 
> > > > appropriate ARM arch_setup_dma_ops() function is sufficient?
> > > 
> > > If I understand correctly, set_dma_ops will replace dev->dma_ops with
> > > Xen DMA ops.
[...]
> > Yet another way would be for dom0 to always set dev->dma_ops to
> > xen_dma_ops and preserve the real dma_ops somewhere under dev->archdata.
> > You could intercept the arch_setup_dma_ops() function for this or use
> > bus_register_notifier() (though I think the former is easier). The Xen
> > code making use of the real dma_ops would have to dig them out from
> > dev->archdata.
> 
> This is a good suggestion, Catalin. Thank you. See below. Is that what
> you have in mind? Julien could you test it, please? If it is the right
> approach, I'll submit the patch properly and rename __generic_dma_ops to
> xen_generic_dma_ops or something.

It looks fine to me (subject to testing successfully).

-- 
Catalin

"Consolidate get_dma_ops" breaks Xen on ARM

[Catalin Marinas]

On Tue, Apr 11, 2017 at 04:39:09PM -0700, Stefano Stabellini wrote:
> On Tue, 11 Apr 2017, Catalin Marinas wrote:
> > On Tue, Apr 11, 2017 at 01:43:28PM +0100, Julien Grall wrote:
> > > On 11/04/17 02:14, Bart Van Assche wrote:
> > > > On 04/10/17 17:31, Stefano Stabellini wrote:
> > > >> I think the reason is that, as you can see, if (dev && dev->dma_ops),
> > > >> dev->dma_ops is returned, while before this changes, xen_dma_ops was
> > > >> returned on Xen on ARM.
> > > >>
> > > >> Unfortunately DMA cannot work properly without using the appropriate
> > > >> xen_dma_ops. See drivers/xen/swiotlb-xen.c and arch/arm/xen/mm.c for
> > > >> more details. (The problem is easy to spot, but I wasn't CC'ed on the
> > > >> patch.)
> > > >>
> > > >> I don't know how to solve this problem without introducing some sort of
> > > >> if (xen()) in include/linux/dma-mapping.h.
> > > > 
> > > > Sorry but I don't have access to an ARM development system. Does your 
> > > > comment apply to dev == NULL only, dev != NULL only or perhaps to both? 
> > > > If your comment applies to dev != NULL only, can you check whether 
> > > > adding something like set_dma_ops(dev, get_arch_dma_ops(NULL)) to the 
> > > > appropriate ARM arch_setup_dma_ops() function is sufficient?
> > > 
> > > If I understand correctly, set_dma_ops will replace dev->dma_ops with
> > > Xen DMA ops.
[...]
> > Yet another way would be for dom0 to always set dev->dma_ops to
> > xen_dma_ops and preserve the real dma_ops somewhere under dev->archdata.
> > You could intercept the arch_setup_dma_ops() function for this or use
> > bus_register_notifier() (though I think the former is easier). The Xen
> > code making use of the real dma_ops would have to dig them out from
> > dev->archdata.
> 
> This is a good suggestion, Catalin. Thank you. See below. Is that what
> you have in mind? Julien could you test it, please? If it is the right
> approach, I'll submit the patch properly and rename __generic_dma_ops to
> xen_generic_dma_ops or something.

It looks fine to me (subject to testing successfully).

-- 
Catalin

Re: "Consolidate get_dma_ops" breaks Xen on ARM

[Julien Grall]

Hi Stefano,

Sorry for the late answer.

On 12/04/17 00:39, Stefano Stabellini wrote:
> On Tue, 11 Apr 2017, Catalin Marinas wrote:
>> On Tue, Apr 11, 2017 at 01:43:28PM +0100, Julien Grall wrote:
>>> On 11/04/17 02:14, Bart Van Assche wrote:
>>>> On 04/10/17 17:31, Stefano Stabellini wrote:
>>>>> I think the reason is that, as you can see, if (dev && dev->dma_ops),
>>>>> dev->dma_ops is returned, while before this changes, xen_dma_ops was
>>>>> returned on Xen on ARM.
>>>>>
>>>>> Unfortunately DMA cannot work properly without using the appropriate
>>>>> xen_dma_ops. See drivers/xen/swiotlb-xen.c and arch/arm/xen/mm.c for
>>>>> more details. (The problem is easy to spot, but I wasn't CC'ed on the
>>>>> patch.)
>>>>>
>>>>> I don't know how to solve this problem without introducing some sort of
>>>>> if (xen()) in include/linux/dma-mapping.h.
>>>>
>>>> Sorry but I don't have access to an ARM development system. Does your
>>>> comment apply to dev == NULL only, dev != NULL only or perhaps to both?
>>>> If your comment applies to dev != NULL only, can you check whether
>>>> adding something like set_dma_ops(dev, get_arch_dma_ops(NULL)) to the
>>>> appropriate ARM arch_setup_dma_ops() function is sufficient?
>>>
>>> If I understand correctly, set_dma_ops will replace dev->dma_ops with
>>> Xen DMA ops.
>>>
>>> However, Xen DMA ops will need in some places to call the device
>>> specific DMA ops (see __generic_dma_ops(...)). So I think replacing
>>> dev->dma_ops is not a solution here.
>>>
>>> The hackish patch below is fixing the problem for both ARM64 and ARM32.
>>>
>>> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
>>> index 0977317c6835..43a73ddeec7a 100644
>>> --- a/include/linux/dma-mapping.h
>>> +++ b/include/linux/dma-mapping.h
>>> @@ -174,6 +174,8 @@ int dma_mmap_from_coherent(struct device *dev, struct vm_area_struct *vma,
>>>  #include <asm/dma-mapping.h>
>>>  static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
>>>  {
>>> +       if (xen_initial_domain())
>>> +              return xen_dma_ops;
>>>         if (dev && dev->dma_ops)
>>>                 return dev->dma_ops;
>>>         return get_arch_dma_ops(dev ? dev->bus : NULL);
>>
>> If we do this, I guess there is no need to check for
>> xen_initial_domain() in the get_arch_dma_ops() function. Anyway, this
>> hunk would break the other architectures since xen_dma_ops is only
>> defined for arm and arm64.
>>
>>> It is not nice as this is common code, but I can't find a better solution
>>> so far. Any opinions?
>>
>> A different hack would be to avoid the generic get_dma_ops
>> implementation on arm with some #ifdef hacks above.
>>
>> Yet another way would be for dom0 to always set dev->dma_ops to
>> xen_dma_ops and preserve the real dma_ops somewhere under dev->archdata.
>> You could intercept the arch_setup_dma_ops() function for this or use
>> bus_register_notifier() (though I think the former is easier). The Xen
>> code making use of the real dma_ops would have to dig them out from
>> dev->archdata.
>
> This is a good suggestion, Catalin. Thank you. See below. Is that what
> you have in mind? Julien could you test it, please? If it is the right
> approach, I'll submit the patch properly and rename __generic_dma_ops to
> xen_generic_dma_ops or something.

This patch is fixing the bug I encountered.

Cheers,

-- 
Julien Grall

Re: "Consolidate get_dma_ops" breaks Xen on ARM

[Julien Grall]

Hi Stefano,

Sorry for the late answer.

On 12/04/17 00:39, Stefano Stabellini wrote:
> On Tue, 11 Apr 2017, Catalin Marinas wrote:
>> On Tue, Apr 11, 2017 at 01:43:28PM +0100, Julien Grall wrote:
>>> On 11/04/17 02:14, Bart Van Assche wrote:
>>>> On 04/10/17 17:31, Stefano Stabellini wrote:
>>>>> I think the reason is that, as you can see, if (dev && dev->dma_ops),
>>>>> dev->dma_ops is returned, while before this changes, xen_dma_ops was
>>>>> returned on Xen on ARM.
>>>>>
>>>>> Unfortunately DMA cannot work properly without using the appropriate
>>>>> xen_dma_ops. See drivers/xen/swiotlb-xen.c and arch/arm/xen/mm.c for
>>>>> more details. (The problem is easy to spot, but I wasn't CC'ed on the
>>>>> patch.)
>>>>>
>>>>> I don't know how to solve this problem without introducing some sort of
>>>>> if (xen()) in include/linux/dma-mapping.h.
>>>>
>>>> Sorry but I don't have access to an ARM development system. Does your
>>>> comment apply to dev == NULL only, dev != NULL only or perhaps to both?
>>>> If your comment applies to dev != NULL only, can you check whether
>>>> adding something like set_dma_ops(dev, get_arch_dma_ops(NULL)) to the
>>>> appropriate ARM arch_setup_dma_ops() function is sufficient?
>>>
>>> If I understand correctly, set_dma_ops will replace dev->dma_ops with
>>> Xen DMA ops.
>>>
>>> However, Xen DMA ops will need in some places to call the device
>>> specific DMA ops (see __generic_dma_ops(...)). So I think replacing
>>> dev->dma_ops is not a solution here.
>>>
>>> The hackish patch below is fixing the problem for both ARM64 and ARM32.
>>>
>>> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
>>> index 0977317c6835..43a73ddeec7a 100644
>>> --- a/include/linux/dma-mapping.h
>>> +++ b/include/linux/dma-mapping.h
>>> @@ -174,6 +174,8 @@ int dma_mmap_from_coherent(struct device *dev, struct vm_area_struct *vma,
>>>  #include <asm/dma-mapping.h>
>>>  static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
>>>  {
>>> +       if (xen_initial_domain())
>>> +              return xen_dma_ops;
>>>         if (dev && dev->dma_ops)
>>>                 return dev->dma_ops;
>>>         return get_arch_dma_ops(dev ? dev->bus : NULL);
>>
>> If we do this, I guess there is no need to check for
>> xen_initial_domain() in the get_arch_dma_ops() function. Anyway, this
>> hunk would break the other architectures since xen_dma_ops is only
>> defined for arm and arm64.
>>
>>> It is not nice as this is common code, but I can't find a better solution
>>> so far. Any opinions?
>>
>> A different hack would be to avoid the generic get_dma_ops
>> implementation on arm with some #ifdef hacks above.
>>
>> Yet another way would be for dom0 to always set dev->dma_ops to
>> xen_dma_ops and preserve the real dma_ops somewhere under dev->archdata.
>> You could intercept the arch_setup_dma_ops() function for this or use
>> bus_register_notifier() (though I think the former is easier). The Xen
>> code making use of the real dma_ops would have to dig them out from
>> dev->archdata.
>
> This is a good suggestion, Catalin. Thank you. See below. Is that what
> you have in mind? Julien could you test it, please? If it is the right
> approach, I'll submit the patch properly and rename __generic_dma_ops to
> xen_generic_dma_ops or something.

This patch is fixing the bug I encountered.

Cheers,

-- 
Julien Grall

"Consolidate get_dma_ops" breaks Xen on ARM

[Julien Grall]

Hi Stefano,

Sorry for the late answer.

On 12/04/17 00:39, Stefano Stabellini wrote:
> On Tue, 11 Apr 2017, Catalin Marinas wrote:
>> On Tue, Apr 11, 2017 at 01:43:28PM +0100, Julien Grall wrote:
>>> On 11/04/17 02:14, Bart Van Assche wrote:
>>>> On 04/10/17 17:31, Stefano Stabellini wrote:
>>>>> I think the reason is that, as you can see, if (dev && dev->dma_ops),
>>>>> dev->dma_ops is returned, while before this changes, xen_dma_ops was
>>>>> returned on Xen on ARM.
>>>>>
>>>>> Unfortunately DMA cannot work properly without using the appropriate
>>>>> xen_dma_ops. See drivers/xen/swiotlb-xen.c and arch/arm/xen/mm.c for
>>>>> more details. (The problem is easy to spot, but I wasn't CC'ed on the
>>>>> patch.)
>>>>>
>>>>> I don't know how to solve this problem without introducing some sort of
>>>>> if (xen()) in include/linux/dma-mapping.h.
>>>>
>>>> Sorry but I don't have access to an ARM development system. Does your
>>>> comment apply to dev == NULL only, dev != NULL only or perhaps to both?
>>>> If your comment applies to dev != NULL only, can you check whether
>>>> adding something like set_dma_ops(dev, get_arch_dma_ops(NULL)) to the
>>>> appropriate ARM arch_setup_dma_ops() function is sufficient?
>>>
>>> If I understand correctly, set_dma_ops will replace dev->dma_ops with
>>> Xen DMA ops.
>>>
>>> However, Xen DMA ops will need in some places to call the device
>>> specific DMA ops (see __generic_dma_ops(...)). So I think replacing
>>> dev->dma_ops is not a solution here.
>>>
>>> The hackish patch below is fixing the problem for both ARM64 and ARM32.
>>>
>>> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
>>> index 0977317c6835..43a73ddeec7a 100644
>>> --- a/include/linux/dma-mapping.h
>>> +++ b/include/linux/dma-mapping.h
>>> @@ -174,6 +174,8 @@ int dma_mmap_from_coherent(struct device *dev, struct vm_area_struct *vma,
>>>  #include <asm/dma-mapping.h>
>>>  static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
>>>  {
>>> +       if (xen_initial_domain())
>>> +              return xen_dma_ops;
>>>         if (dev && dev->dma_ops)
>>>                 return dev->dma_ops;
>>>         return get_arch_dma_ops(dev ? dev->bus : NULL);
>>
>> If we do this, I guess there is no need to check for
>> xen_initial_domain() in the get_arch_dma_ops() function. Anyway, this
>> hunk would break the other architectures since xen_dma_ops is only
>> defined for arm and arm64.
>>
>>> It is not nice as this is common code, but I can't find a better solution
>>> so far. Any opinions?
>>
>> A different hack would be to avoid the generic get_dma_ops
>> implementation on arm with some #ifdef hacks above.
>>
>> Yet another way would be for dom0 to always set dev->dma_ops to
>> xen_dma_ops and preserve the real dma_ops somewhere under dev->archdata.
>> You could intercept the arch_setup_dma_ops() function for this or use
>> bus_register_notifier() (though I think the former is easier). The Xen
>> code making use of the real dma_ops would have to dig them out from
>> dev->archdata.
>
> This is a good suggestion, Catalin. Thank you. See below. Is that what
> you have in mind? Julien could you test it, please? If it is the right
> approach, I'll submit the patch properly and rename __generic_dma_ops to
> xen_generic_dma_ops or something.

This patch is fixing the bug I encountered.

Cheers,

-- 
Julien Grall

Re: "Consolidate get_dma_ops" breaks Xen on ARM

[Stefano Stabellini]

On Thu, 13 Apr 2017, Julien Grall wrote:
> Hi Stefano,
> 
> Sorry for the late answer.
> 
> On 12/04/17 00:39, Stefano Stabellini wrote:
> > On Tue, 11 Apr 2017, Catalin Marinas wrote:
> > > On Tue, Apr 11, 2017 at 01:43:28PM +0100, Julien Grall wrote:
> > > > On 11/04/17 02:14, Bart Van Assche wrote:
> > > > > On 04/10/17 17:31, Stefano Stabellini wrote:
> > > > > > I think the reason is that, as you can see, if (dev &&
> > > > > > dev->dma_ops),
> > > > > > dev->dma_ops is returned, while before this changes, xen_dma_ops was
> > > > > > returned on Xen on ARM.
> > > > > > 
> > > > > > Unfortunately DMA cannot work properly without using the appropriate
> > > > > > xen_dma_ops. See drivers/xen/swiotlb-xen.c and arch/arm/xen/mm.c for
> > > > > > more details. (The problem is easy to spot, but I wasn't CC'ed on
> > > > > > the
> > > > > > patch.)
> > > > > > 
> > > > > > I don't know how to solve this problem without introducing some sort
> > > > > > of
> > > > > > if (xen()) in include/linux/dma-mapping.h.
> > > > > 
> > > > > Sorry but I don't have access to an ARM development system. Does your
> > > > > comment apply to dev == NULL only, dev != NULL only or perhaps to
> > > > > both?
> > > > > If your comment applies to dev != NULL only, can you check whether
> > > > > adding something like set_dma_ops(dev, get_arch_dma_ops(NULL)) to the
> > > > > appropriate ARM arch_setup_dma_ops() function is sufficient?
> > > > 
> > > > If I understand correctly, set_dma_ops will replace dev->dma_ops with
> > > > Xen DMA ops.
> > > > 
> > > > However, Xen DMA ops will need in some places to call the device
> > > > specific DMA ops (see __generic_dma_ops(...)). So I think replacing
> > > > dev->dma_ops is not a solution here.
> > > > 
> > > > The hackish patch below is fixing the problem for both ARM64 and ARM32.
> > > > 
> > > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > > > index 0977317c6835..43a73ddeec7a 100644
> > > > --- a/include/linux/dma-mapping.h
> > > > +++ b/include/linux/dma-mapping.h
> > > > @@ -174,6 +174,8 @@ int dma_mmap_from_coherent(struct device *dev,
> > > > struct vm_area_struct *vma,
> > > >  #include <asm/dma-mapping.h>
> > > >  static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
> > > >  {
> > > > +       if (xen_initial_domain())
> > > > +              return xen_dma_ops;
> > > >         if (dev && dev->dma_ops)
> > > >                 return dev->dma_ops;
> > > >         return get_arch_dma_ops(dev ? dev->bus : NULL);
> > > 
> > > If we do this, I guess there is no need to check for
> > > xen_initial_domain() in the get_arch_dma_ops() function. Anyway, this
> > > hunk would break the other architectures since xen_dma_ops is only
> > > defined for arm and arm64.
> > > 
> > > > It is not nice as this is common code, but I can't find a better
> > > > solution
> > > > so far. Any opinions?
> > > 
> > > A different hack would be to avoid the generic get_dma_ops
> > > implementation on arm with some #ifdef hacks above.
> > > 
> > > Yet another way would be for dom0 to always set dev->dma_ops to
> > > xen_dma_ops and preserve the real dma_ops somewhere under dev->archdata.
> > > You could intercept the arch_setup_dma_ops() function for this or use
> > > bus_register_notifier() (though I think the former is easier). The Xen
> > > code making use of the real dma_ops would have to dig them out from
> > > dev->archdata.
> > 
> > This is a good suggestion, Catalin. Thank you. See below. Is that what
> > you have in mind? Julien could you test it, please? If it is the right
> > approach, I'll submit the patch properly and rename __generic_dma_ops to
> > xen_generic_dma_ops or something.
> 
> This patch is fixing the bug I encountered.

I'll add your tested-by.

"Consolidate get_dma_ops" breaks Xen on ARM

[Stefano Stabellini]

On Thu, 13 Apr 2017, Julien Grall wrote:
> Hi Stefano,
> 
> Sorry for the late answer.
> 
> On 12/04/17 00:39, Stefano Stabellini wrote:
> > On Tue, 11 Apr 2017, Catalin Marinas wrote:
> > > On Tue, Apr 11, 2017 at 01:43:28PM +0100, Julien Grall wrote:
> > > > On 11/04/17 02:14, Bart Van Assche wrote:
> > > > > On 04/10/17 17:31, Stefano Stabellini wrote:
> > > > > > I think the reason is that, as you can see, if (dev &&
> > > > > > dev->dma_ops),
> > > > > > dev->dma_ops is returned, while before this changes, xen_dma_ops was
> > > > > > returned on Xen on ARM.
> > > > > > 
> > > > > > Unfortunately DMA cannot work properly without using the appropriate
> > > > > > xen_dma_ops. See drivers/xen/swiotlb-xen.c and arch/arm/xen/mm.c for
> > > > > > more details. (The problem is easy to spot, but I wasn't CC'ed on
> > > > > > the
> > > > > > patch.)
> > > > > > 
> > > > > > I don't know how to solve this problem without introducing some sort
> > > > > > of
> > > > > > if (xen()) in include/linux/dma-mapping.h.
> > > > > 
> > > > > Sorry but I don't have access to an ARM development system. Does your
> > > > > comment apply to dev == NULL only, dev != NULL only or perhaps to
> > > > > both?
> > > > > If your comment applies to dev != NULL only, can you check whether
> > > > > adding something like set_dma_ops(dev, get_arch_dma_ops(NULL)) to the
> > > > > appropriate ARM arch_setup_dma_ops() function is sufficient?
> > > > 
> > > > If I understand correctly, set_dma_ops will replace dev->dma_ops with
> > > > Xen DMA ops.
> > > > 
> > > > However, Xen DMA ops will need in some places to call the device
> > > > specific DMA ops (see __generic_dma_ops(...)). So I think replacing
> > > > dev->dma_ops is not a solution here.
> > > > 
> > > > The hackish patch below is fixing the problem for both ARM64 and ARM32.
> > > > 
> > > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > > > index 0977317c6835..43a73ddeec7a 100644
> > > > --- a/include/linux/dma-mapping.h
> > > > +++ b/include/linux/dma-mapping.h
> > > > @@ -174,6 +174,8 @@ int dma_mmap_from_coherent(struct device *dev,
> > > > struct vm_area_struct *vma,
> > > >  #include <asm/dma-mapping.h>
> > > >  static inline const struct dma_map_ops *get_dma_ops(struct device *dev)
> > > >  {
> > > > +       if (xen_initial_domain())
> > > > +              return xen_dma_ops;
> > > >         if (dev && dev->dma_ops)
> > > >                 return dev->dma_ops;
> > > >         return get_arch_dma_ops(dev ? dev->bus : NULL);
> > > 
> > > If we do this, I guess there is no need to check for
> > > xen_initial_domain() in the get_arch_dma_ops() function. Anyway, this
> > > hunk would break the other architectures since xen_dma_ops is only
> > > defined for arm and arm64.
> > > 
> > > > It is not nice as this is common code, but I can't find a better
> > > > solution
> > > > so far. Any opinions?
> > > 
> > > A different hack would be to avoid the generic get_dma_ops
> > > implementation on arm with some #ifdef hacks above.
> > > 
> > > Yet another way would be for dom0 to always set dev->dma_ops to
> > > xen_dma_ops and preserve the real dma_ops somewhere under dev->archdata.
> > > You could intercept the arch_setup_dma_ops() function for this or use
> > > bus_register_notifier() (though I think the former is easier). The Xen
> > > code making use of the real dma_ops would have to dig them out from
> > > dev->archdata.
> > 
> > This is a good suggestion, Catalin. Thank you. See below. Is that what
> > you have in mind? Julien could you test it, please? If it is the right
> > approach, I'll submit the patch properly and rename __generic_dma_ops to
> > xen_generic_dma_ops or something.
> 
> This patch is fixing the bug I encountered.

I'll add your tested-by.