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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 12.12.12 at 02:03, "Xu, Dongxiao" <dongxiao.xu@intel.com> wrote:
>> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>> On Tue, Dec 11, 2012 at 06:39:35AM +0000, Xu, Dongxiao wrote:
>> > > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>> > > 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.

I wonder whether such use of sg_init_one()/sg_set_buf() is correct
in the first place. While there aren't any restrictions documented for
its use, one clearly can't pass in whatever one wants (a pointer into
vmalloc()-ed memory, for instance, won't work afaict).

I didn't go through all other users of it, but quite a few of the uses
elsewhere look similarly questionable.

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

That's why, after mapping a buffer (or SG list) one has to call the
sync functions before looking at data. Any race as described by
you is therefore a programming error.

Jan