Re: [Xen-devel] [PATCH] xen/swiotlb: Exchange to contiguous memory for map_sg hook

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Wed, Dec 12, 2012 at 01:03:55AM +0000, Xu, Dongxiao wrote:
> > -----Original Message-----
> > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx]
> > Sent: Wednesday, December 12, 2012 1:07 AM
> > To: Xu, Dongxiao
> > Cc: xen-devel@xxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH] xen/swiotlb: Exchange to contiguous memory for map_sg
> > hook
> > 
> > On Tue, Dec 11, 2012 at 06:39:35AM +0000, Xu, Dongxiao wrote:
> > > > -----Original Message-----
> > > > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx]
> > > > Sent: Friday, December 07, 2012 10:09 PM
> > > > To: Xu, Dongxiao
> > > > Cc: xen-devel@xxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > > > Subject: Re: [PATCH] xen/swiotlb: Exchange to contiguous memory for
> > > > map_sg hook
> > > >
> > > > On Thu, Dec 06, 2012 at 09:08:42PM +0800, Dongxiao Xu wrote:
> > > > > While mapping sg buffers, checking to cross page DMA buffer is
> > > > > also needed. If the guest DMA buffer crosses page boundary, Xen
> > > > > should exchange contiguous memory for it.
> > > >
> > > > So this is when we cross those 2MB contingous swatch of buffers.
> > > > Wouldn't we get the same problem with the 'map_page' call? If the
> > > > driver tried to map say a 4MB DMA region?
> > >
> > > Yes, it also needs such check, as I just replied to Jan's mail.
> > >
> > > >
> > > > What if this check was done in the routines that provide the
> > > > software static buffers and there try to provide a nice DMA contingous
> > swatch of pages?
> > >
> > > Yes, this approach also came to our mind, which needs to modify the driver
> > itself.
> > > If so, it requires driver not using such static buffers (e.g., from 
> > > kmalloc) to do
> > DMA even if the buffer is continuous in native.
> > 
> > I am bit loss here.
> > 
> > Is the issue you found only with drivers that do not use DMA API?
> > Can you perhaps point me to the code that triggered this fix in the first 
> > place?
> 
> Yes, we met this issue on a specific SAS device/driver, and it calls into 
> libata-core code, you can refer to function ata_dev_read_id() called from 
> ata_dev_reread_id() in drivers/ata/libata-core.c.
> 
> In the above function, the target buffer is (void 
> *)dev->link->ap->sector_buf, which is 512 bytes static buffer and 
> unfortunately it across the page boundary.

Hm, that looks like a DMA API violation. If you ran the code with
CONFIG_DEBUG_DMA_API did it complain about this? I recall the floppy
driver doing something similar and the Debug DMA API was quite verbose
in pointing this out.

> 
> > > Is this acceptable by kernel/driver upstream?
> > 
> > I am still not completely clear on what you had in mind. The one method I
> > thought about that might help in this is to have Xen-SWIOTLB track which
> > memory ranges were exchanged (so xen_swiotlb_fixup would save the *buf
> > and the size for each call to xen_create_contiguous_region in a list or 
> > array).
> > 
> > When xen_swiotlb_map/xen_swiotlb_map_sg_attrs care called they would
> > consult said array/list to see if the region they retrieved crosses said 2MB
> > chunks. If so.. and here I am unsure of what would be the best way to 
> > proceed.
> 
> We thought we can solve the issue in several ways:
> 
> 1) Like the previous patch I sent out, we check the DMA region in 
> xen_swiotlb_map_page() and xen_swiotlb_map_sg_attr(), and if DMA region 
> crosses page boundary, we exchange the memory and copy the content. However 
> it has race condition that when copying the memory content (we introduced two 
> memory copies in the patch), some other code may also visit the page, which 
> may encounter incorrect values.
> 2) Mostly the same as item 1), the only difference is that we put the memory 
> content copy inside Xen hypervisor but not in Dom0. This requires we add 
> certain flags to indicating memory moving in the XENMEM_exchange hypercall.
> 3) As you also mentioned, this is not a common case, it is only triggered by 
> some specific devices/drivers. we can fix it in certain driver to avoid DMA 
> into static buffers. Like (void *)dev->link->ap->sector_buf in the above 
> case. But I am not sure whether it is acceptable by kernel/driver upstream.
> 
> Thanks,
> Dongxiao
> 
> 

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