lkml.org 
[lkml]   [2012]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [Xen-devel] [PATCH] xen/swiotlb: Exchange to contiguous memory for map_sg hook
On Wed, Dec 12, 2012 at 09:38:23AM +0000, Jan Beulich wrote:
> >>> 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.

And from finally looking at the code I misunderstood your initial description.

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

I am with Jan here. It looks like the libata is depending on the dma_unmap
to do this. But it is unclear to me when the ata_qc_complete is actually
called to unmap the buffer (and hence sync it).

>
> Jan
>
>


\
 
 \ /
  Last update: 2012-12-19 21:21    [W:0.110 / U:0.428 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site