>>> On 12.12.12 at 02:03, "Xu, Dongxiao" <dongxiao...@intel.com> wrote: >> From: Konrad Rzeszutek Wilk [mailto:konrad.w...@oracle.com] >> On Tue, Dec 11, 2012 at 06:39:35AM +0000, Xu, Dongxiao wrote: >> > > From: Konrad Rzeszutek Wilk [mailto:konrad.w...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
