[BACKPORT] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[BACKPORT] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Stefano Stabellini]

Hi all,

We are getting reports from Xen on ARM users about DMA issues. The
problem is that the commit below
(7e91c7df29b5e196de3dc6f086c8937973bd0b88) is necessary to support mmap
on Xen on ARM. It is self-contained and doesn't affect anything outside
of Xen on ARM, so I think is a good candidate for backporting. It went
upstream in 4.11.


Could you please backport the following commit:

  commit 7e91c7df29b5e196de3dc6f086c8937973bd0b88
  Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
  Date:   Tue Feb 7 19:58:02 2017 +0200
  
      swiotlb-xen: implement xen_swiotlb_dma_mmap callback
      
      This function creates userspace mapping for the DMA-coherent memory.
    
to the stable trees up until 3.14?


Because of 00085f1efa387a8ce100e3734920f7639c80caa3 "dma-mapping: use
unsigned long for dma_attrs", the appended patch (to be applied on top)
is required for trees older than 4.8. 


Thank you!

- Stefano


diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index a0a819c..c6d47e5 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -693,7 +693,7 @@ EXPORT_SYMBOL_GPL(xen_swiotlb_set_dma_mask);
 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)
+		     struct dma_attrs *attrs)
 {
 #if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
 	if (__generic_dma_ops(dev)->mmap)
diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
index fab4fb9..4d7fdbf 100644
--- a/include/xen/swiotlb-xen.h
+++ b/include/xen/swiotlb-xen.h
@@ -62,5 +62,5 @@ 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);
+		     struct dma_attrs *attrs);
 #endif /* __LINUX_SWIOTLB_XEN_H */

Re: [BACKPORT] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Greg KH]

On Thu, Sep 14, 2017 at 04:23:05PM -0700, Stefano Stabellini wrote:
> Hi all,
> 
> We are getting reports from Xen on ARM users about DMA issues. The
> problem is that the commit below
> (7e91c7df29b5e196de3dc6f086c8937973bd0b88) is necessary to support mmap
> on Xen on ARM. It is self-contained and doesn't affect anything outside
> of Xen on ARM, so I think is a good candidate for backporting. It went
> upstream in 4.11.

But it's a new feature, right?  How does that fit the stable kernel
rules?

> 
> 
> Could you please backport the following commit:
> 
>   commit 7e91c7df29b5e196de3dc6f086c8937973bd0b88
>   Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>   Date:   Tue Feb 7 19:58:02 2017 +0200
>   
>       swiotlb-xen: implement xen_swiotlb_dma_mmap callback
>       
>       This function creates userspace mapping for the DMA-coherent memory.
>     
> to the stable trees up until 3.14?
> 
> 
> Because of 00085f1efa387a8ce100e3734920f7639c80caa3 "dma-mapping: use
> unsigned long for dma_attrs", the appended patch (to be applied on top)
> is required for trees older than 4.8. 

What does the kvm maintainers think about this?

thanks,

greg k-h

Re: [BACKPORT] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Stefano Stabellini]

On Fri, 15 Sep 2017, Greg KH wrote:
> On Thu, Sep 14, 2017 at 04:23:05PM -0700, Stefano Stabellini wrote:
> > Hi all,
> > 
> > We are getting reports from Xen on ARM users about DMA issues. The
> > problem is that the commit below
> > (7e91c7df29b5e196de3dc6f086c8937973bd0b88) is necessary to support mmap
> > on Xen on ARM. It is self-contained and doesn't affect anything outside
> > of Xen on ARM, so I think is a good candidate for backporting. It went
> > upstream in 4.11.
> 
> But it's a new feature, right?  How does that fit the stable kernel
> rules?

It implements a previously unimplemented function (mmap), although it
calls the generic functions to do it. Yes, I agree with you that it
can be classified as a new feature. If that is against the stable kernel
rules, then please discard this request.

FYI the reason why it didn't raise a flag in my mind is that users
reported something like "unhandled alignment fault (11) at
0xffffa6048080, esr 0x92000061", which really looks more like a bug.


> > Could you please backport the following commit:
> > 
> >   commit 7e91c7df29b5e196de3dc6f086c8937973bd0b88
> >   Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >   Date:   Tue Feb 7 19:58:02 2017 +0200
> >   
> >       swiotlb-xen: implement xen_swiotlb_dma_mmap callback
> >       
> >       This function creates userspace mapping for the DMA-coherent memory.
> >     
> > to the stable trees up until 3.14?
> > 
> > 
> > Because of 00085f1efa387a8ce100e3734920f7639c80caa3 "dma-mapping: use
> > unsigned long for dma_attrs", the appended patch (to be applied on top)
> > is required for trees older than 4.8. 
> 
> What does the kvm maintainers think about this?

That would be the Xen maintainers right? In that case, Boris, Juergen,
please let us know what you think.

Re: [BACKPORT] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Boris Ostrovsky]

On 09/18/2017 02:08 PM, Stefano Stabellini wrote:
> On Fri, 15 Sep 2017, Greg KH wrote:
>> On Thu, Sep 14, 2017 at 04:23:05PM -0700, Stefano Stabellini wrote:
>>> Hi all,
>>>
>>> We are getting reports from Xen on ARM users about DMA issues. The
>>> problem is that the commit below
>>> (7e91c7df29b5e196de3dc6f086c8937973bd0b88) is necessary to support mmap
>>> on Xen on ARM. It is self-contained and doesn't affect anything outside
>>> of Xen on ARM, so I think is a good candidate for backporting. It went
>>> upstream in 4.11.
>> But it's a new feature, right?  How does that fit the stable kernel
>> rules?
> It implements a previously unimplemented function (mmap), although it
> calls the generic functions to do it. Yes, I agree with you that it
> can be classified as a new feature. If that is against the stable kernel
> rules, then please discard this request.
>
> FYI the reason why it didn't raise a flag in my mind is that users
> reported something like "unhandled alignment fault (11) at
> 0xffffa6048080, esr 0x92000061", which really looks more like a bug.
>
>
>>> Could you please backport the following commit:
>>>
>>>   commit 7e91c7df29b5e196de3dc6f086c8937973bd0b88
>>>   Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>   Date:   Tue Feb 7 19:58:02 2017 +0200
>>>   
>>>       swiotlb-xen: implement xen_swiotlb_dma_mmap callback
>>>       
>>>       This function creates userspace mapping for the DMA-coherent memory.
>>>     
>>> to the stable trees up until 3.14?
>>>
>>>
>>> Because of 00085f1efa387a8ce100e3734920f7639c80caa3 "dma-mapping: use
>>> unsigned long for dma_attrs", the appended patch (to be applied on top)
>>> is required for trees older than 4.8. 
>> What does the kvm maintainers think about this?
> That would be the Xen maintainers right? In that case, Boris, Juergen,
> please let us know what you think.


This is a nop for x86 so it's safe from that perspective. I can't find
mmap op for ARM though (xen_get_dma_ops(dev)->mmap).

-boris

Re: [BACKPORT] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Stefano Stabellini]

On Mon, 18 Sep 2017, Boris Ostrovsky wrote:
> On 09/18/2017 02:08 PM, Stefano Stabellini wrote:
> > On Fri, 15 Sep 2017, Greg KH wrote:
> >> On Thu, Sep 14, 2017 at 04:23:05PM -0700, Stefano Stabellini wrote:
> >>> Hi all,
> >>>
> >>> We are getting reports from Xen on ARM users about DMA issues. The
> >>> problem is that the commit below
> >>> (7e91c7df29b5e196de3dc6f086c8937973bd0b88) is necessary to support mmap
> >>> on Xen on ARM. It is self-contained and doesn't affect anything outside
> >>> of Xen on ARM, so I think is a good candidate for backporting. It went
> >>> upstream in 4.11.
> >> But it's a new feature, right?  How does that fit the stable kernel
> >> rules?
> > It implements a previously unimplemented function (mmap), although it
> > calls the generic functions to do it. Yes, I agree with you that it
> > can be classified as a new feature. If that is against the stable kernel
> > rules, then please discard this request.
> >
> > FYI the reason why it didn't raise a flag in my mind is that users
> > reported something like "unhandled alignment fault (11) at
> > 0xffffa6048080, esr 0x92000061", which really looks more like a bug.
> >
> >
> >>> Could you please backport the following commit:
> >>>
> >>>   commit 7e91c7df29b5e196de3dc6f086c8937973bd0b88
> >>>   Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>>   Date:   Tue Feb 7 19:58:02 2017 +0200
> >>>   
> >>>       swiotlb-xen: implement xen_swiotlb_dma_mmap callback
> >>>       
> >>>       This function creates userspace mapping for the DMA-coherent memory.
> >>>     
> >>> to the stable trees up until 3.14?
> >>>
> >>>
> >>> Because of 00085f1efa387a8ce100e3734920f7639c80caa3 "dma-mapping: use
> >>> unsigned long for dma_attrs", the appended patch (to be applied on top)
> >>> is required for trees older than 4.8. 
> >> What does the kvm maintainers think about this?
> > That would be the Xen maintainers right? In that case, Boris, Juergen,
> > please let us know what you think.
> 
> 
> This is a nop for x86 so it's safe from that perspective. I can't find
> mmap op for ARM though (xen_get_dma_ops(dev)->mmap).

arch/arm/mm/dma-mapping.c:arm_dma_mmap
arch/arm/mm/dma-mapping.c:arm_coherent_dma_mmap
arch/arm64/mm/dma-mapping.c:__swiotlb_mmap

Re: [BACKPORT] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Leonard Crestez]

On Mon, 2017-09-18 at 11:08 -0700, Stefano Stabellini wrote:
On Fri, 15 Sep 2017, Greg KH wrote:
> On Thu, Sep 14, 2017 at 04:23:05PM -0700, Stefano Stabellini wrote:
> > Hi all,
> > 
> > We are getting reports from Xen on ARM users about DMA issues. The
> > problem is that the commit below
> > (7e91c7df29b5e196de3dc6f086c8937973bd0b88) is necessary to support mmap
> > on Xen on ARM. It is self-contained and doesn't affect anything outside
> > of Xen on ARM, so I think is a good candidate for backporting. It went
> > upstream in 4.11.
> 
> But it's a new feature, right?  How does that fit the stable kernel
> rules?


It implements a previously unimplemented function (mmap), although it
calls the generic functions to do it. Yes, I agree with you that it
can be classified as a new feature. If that is against the stable kernel
rules, then please discard this request.


FYI the reason why it didn't raise a flag in my mind is that users
reported something like "unhandled alignment fault (11) at
0xffffa6048080, esr 0x92000061", which really looks more like a bug.

I am the one who reported this, on the #xenarm IRC channel.

Not implementing mmap in dma_map_ops means that dma_common_mmap is
called by dma_map_attrs as a fallback. The end result is not something
like -ENOSYS but what seem to be corrupt mappings.

However I agree that backporting might be excessive. I ran into this by
experimenting with using a GPU from dom0. It seems reasonable to get
kernel crashes if you try this kind of stuff.

This patch results in calling __swiotlb_mmap instead of
dma_common_mmap. I don't know the implementation details of the DMA api
but the interesting difference between these paths seems to be the way
pfn is fetched (from dma_addr instead of the kernel virt addr).

--
Regards,
Leonard

Re: [BACKPORT] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Stefano Stabellini]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2308 bytes --]

On Wed, 20 Sep 2017, Leonard Crestez wrote:
> On Mon, 2017-09-18 at 11:08 -0700, Stefano Stabellini wrote:
> On Fri, 15 Sep 2017, Greg KH wrote:
> > On Thu, Sep 14, 2017 at 04:23:05PM -0700, Stefano Stabellini wrote:
> > > Hi all,
> > > 
> > > We are getting reports from Xen on ARM users about DMA issues. The
> > > problem is that the commit below
> > > (7e91c7df29b5e196de3dc6f086c8937973bd0b88) is necessary to support mmap
> > > on Xen on ARM. It is self-contained and doesn't affect anything outside
> > > of Xen on ARM, so I think is a good candidate for backporting. It went
> > > upstream in 4.11.
> > 
> > But it's a new feature, right?  How does that fit the stable kernel
> > rules?
> 
> 
> It implements a previously unimplemented function (mmap), although it
> calls the generic functions to do it. Yes, I agree with you that it
> can be classified as a new feature. If that is against the stable kernel
> rules, then please discard this request.
> 
> 
> FYI the reason why it didn't raise a flag in my mind is that users
> reported something like "unhandled alignment fault (11) at
> 0xffffa6048080, esr 0x92000061", which really looks more like a bug.
> 
> I am the one who reported this, on the #xenarm IRC channel.

Thank you for jumping into this thread.


> Not implementing mmap in dma_map_ops means that dma_common_mmap is
> called by dma_map_attrs as a fallback. The end result is not something
> like -ENOSYS but what seem to be corrupt mappings.
> 
> However I agree that backporting might be excessive. I ran into this by
> experimenting with using a GPU from dom0. It seems reasonable to get
> kernel crashes if you try this kind of stuff.
> 
> This patch results in calling __swiotlb_mmap instead of
> dma_common_mmap. I don't know the implementation details of the DMA api
> but the interesting difference between these paths seems to be the way
> pfn is fetched (from dma_addr instead of the kernel virt addr).

Yes, on ARM and ARM64 dma_map_ops functions can return pages for which
virt_to_page doesn't work as expected (for example on ARM alloc_coherent
returns an ioremap'ped virtual address, I don't remember the details of
the ARM64 implementation right now). This is why the dma_map_ops
functions are implemented by looking up the physical address from the
dma address.

Re: [BACKPORT] swiotlb-xen: implement xen_swiotlb_dma_mmap callback

[Juergen Gross]

On 18/09/17 20:08, Stefano Stabellini wrote:
> On Fri, 15 Sep 2017, Greg KH wrote:
>> On Thu, Sep 14, 2017 at 04:23:05PM -0700, Stefano Stabellini wrote:
>>> Hi all,
>>>
>>> We are getting reports from Xen on ARM users about DMA issues. The
>>> problem is that the commit below
>>> (7e91c7df29b5e196de3dc6f086c8937973bd0b88) is necessary to support mmap
>>> on Xen on ARM. It is self-contained and doesn't affect anything outside
>>> of Xen on ARM, so I think is a good candidate for backporting. It went
>>> upstream in 4.11.
>>
>> But it's a new feature, right?  How does that fit the stable kernel
>> rules?
> 
> It implements a previously unimplemented function (mmap), although it
> calls the generic functions to do it. Yes, I agree with you that it
> can be classified as a new feature. If that is against the stable kernel
> rules, then please discard this request.
> 
> FYI the reason why it didn't raise a flag in my mind is that users
> reported something like "unhandled alignment fault (11) at
> 0xffffa6048080, esr 0x92000061", which really looks more like a bug.
> 
> 
>>> Could you please backport the following commit:
>>>
>>>   commit 7e91c7df29b5e196de3dc6f086c8937973bd0b88
>>>   Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>   Date:   Tue Feb 7 19:58:02 2017 +0200
>>>   
>>>       swiotlb-xen: implement xen_swiotlb_dma_mmap callback
>>>       
>>>       This function creates userspace mapping for the DMA-coherent memory.
>>>     
>>> to the stable trees up until 3.14?
>>>
>>>
>>> Because of 00085f1efa387a8ce100e3734920f7639c80caa3 "dma-mapping: use
>>> unsigned long for dma_attrs", the appended patch (to be applied on top)
>>> is required for trees older than 4.8. 
>>
>> What does the kvm maintainers think about this?
> 
> That would be the Xen maintainers right? In that case, Boris, Juergen,
> please let us know what you think.
> 

I have no specific preference.


Juergen