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

["Xu, Dongxiao" <dongxiao.xu@xxxxxxxxx>]

> -----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.

> > 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