Xen devel - Jul 2007 - mthca use of dma_sync_single is bogus

It seems the problems running mthca in a Xen domU have uncovered a bug
in mthca: mthca uses dma_sync_single in mthca_arbel_write_mtt_seg()
and mthca_arbel_map_phys_fmr() to sync the MTTs that get written.
However, Documentation/DMA-API.txt says:

    void
    dma_sync_single(struct device *dev, dma_addr_t dma_handle, size_t size,
    		enum dma_data_direction direction)

    synchronise a single contiguous or scatter/gather mapping.  All the
    parameters must be the same as those passed into the single mapping
    API.

and mthca is *not* following this clear rule: it is trying to sync
only a subrange of the mapping.  Later on in the document, there is:

    void
    dma_sync_single_range(struct device *dev, dma_addr_t dma_handle,
    		      unsigned long offset, size_t size,
    		      enum dma_data_direction direction)
    
    does a partial sync.  starting at offset and continuing for size.  You
    must be careful to observe the cache alignment and width when doing
    anything like this.  You must also be extra careful about accessing
    memory you intend to sync partially.

but that is in a section dealing with non-consistent memory so it''s
not entirely clear to me whether it''s kosher to use this as mthca
wants.

The other alternative is to put the MTT table in coherent memory just
like the MPT table.  That might be the best solution I suppose...

Michael, anyone else, thoughts on this?

 - R.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>     void
 >     dma_sync_single_range(struct device *dev, dma_addr_t dma_handle,
 >     		      unsigned long offset, size_t size,
 >     		      enum dma_data_direction direction)

It seems the document has bitrotted a little, since
dma_sync_single_range() doesn''t actually exist for most architectures;
what is really implemented is dma_sync_single_range_for_cpu() and
dma_sync_single_range_for_device().  But assuming those are usable in
our situation, they seem to be exactly what we want.  I''ll try to get
clarification from the DMA API experts (and also fix the documentation
in the kernel).

Unfortunately it seems like the kernel''s swiotlb does not implement
the full DMA API so this won''t actually fix Xen :(.

 - R.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> One thought is that if you *do* move to dma_sync_single_range() then
 > lib/swiotlb.c still needs fixing. It''s buggy in that
 > swiotlb_sync_single_range(dma_addr, offset) calls
 > swiotlb_sync_single(dma_addr+offset), and this will fail if the offset is
 > large enough that it ends up dereferencing a different slot index in
 > io_tlb_orig_addr.

Yes, I realized the same thing (our emails crossed).

 > So, I should be able to get my swiotlb workaround fixes accepted upstream
as
 > a genuine bug fix. :-)

Yeah, seems so.

 > dma_sync_single_range() looks to me to be the right thing for you to be
 > using. But I''m not a DMA-API expert.

yes, I''ll try to get confirmation from James Bottomley and/or Dave
Miller
that it is the right thing to do (and also fix the documentation to
match what the kernel actually implements).

 - R.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

One thought is that if you *do* move to dma_sync_single_range() then
lib/swiotlb.c still needs fixing. It''s buggy in that
swiotlb_sync_single_range(dma_addr, offset) calls
swiotlb_sync_single(dma_addr+offset), and this will fail if the offset is
large enough that it ends up dereferencing a different slot index in
io_tlb_orig_addr.

So, I should be able to get my swiotlb workaround fixes accepted upstream as
a genuine bug fix. :-)

dma_sync_single_range() looks to me to be the right thing for you to be
using. But I''m not a DMA-API expert.

 -- Keir

On 9/7/07 22:16, "Roland Dreier" <rdreier@cisco.com> wrote:
> It seems the problems running mthca in a Xen domU have uncovered a bug
> in mthca: mthca uses dma_sync_single in mthca_arbel_write_mtt_seg()
> and mthca_arbel_map_phys_fmr() to sync the MTTs that get written.
> However, Documentation/DMA-API.txt says:
> 
>     void
>     dma_sync_single(struct device *dev, dma_addr_t dma_handle, size_t size,
> enum dma_data_direction direction)
> 
>     synchronise a single contiguous or scatter/gather mapping.  All the
>     parameters must be the same as those passed into the single mapping
>     API.
> 
> and mthca is *not* following this clear rule: it is trying to sync
> only a subrange of the mapping.  Later on in the document, there is:
> 
>     void
>     dma_sync_single_range(struct device *dev, dma_addr_t dma_handle,
>      unsigned long offset, size_t size,
>      enum dma_data_direction direction)
>     
>     does a partial sync.  starting at offset and continuing for size.  You
>     must be careful to observe the cache alignment and width when doing
>     anything like this.  You must also be extra careful about accessing
>     memory you intend to sync partially.
> 
> but that is in a section dealing with non-consistent memory so
it''s
> not entirely clear to me whether it''s kosher to use this as mthca
> wants.
> 
> The other alternative is to put the MTT table in coherent memory just
> like the MPT table.  That might be the best solution I suppose...
> 
> Michael, anyone else, thoughts on this?
> 
>  - R.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 9/7/07 22:29, "Roland Dreier" <rdreier@cisco.com> wrote:
> Unfortunately it seems like the kernel''s swiotlb does not
implement
> the full DMA API so this won''t actually fix Xen :(.
It implements the sync_single_range_for_{cpu,device} functions.

But we use our own swiotlb implementation anyway. arch/i386/kernel/swiotlb.c
in a Xen-patched tree is used by both i386/xen and x64/xen. We haven''t
yet
merged with main lib/swiotlb.c.

 -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> Quoting Roland Dreier <rdreier@cisco.com>:
> Subject: mthca use of dma_sync_single is bogus
> 
> It seems the problems running mthca in a Xen domU have uncovered a bug
> in mthca: mthca uses dma_sync_single in mthca_arbel_write_mtt_seg()
> and mthca_arbel_map_phys_fmr() to sync the MTTs that get written.
> However, Documentation/DMA-API.txt says:
> 
>     void
>     dma_sync_single(struct device *dev, dma_addr_t dma_handle, size_t size,
>     		enum dma_data_direction direction)
> 
>     synchronise a single contiguous or scatter/gather mapping.  All the
>     parameters must be the same as those passed into the single mapping
>     API.
> 
> and mthca is *not* following this clear rule: it is trying to sync
> only a subrange of the mapping.
Yes, this looks like a bug.
> Later on in the document, there is:
> 
>     void
>     dma_sync_single_range(struct device *dev, dma_addr_t dma_handle,
>     		      unsigned long offset, size_t size,
>     		      enum dma_data_direction direction)
>     
>     does a partial sync.  starting at offset and continuing for size.  You
>     must be careful to observe the cache alignment and width when doing
>     anything like this.  You must also be extra careful about accessing
>     memory you intend to sync partially.
> 
> but that is in a section dealing with non-consistent memory so
it''s
> not entirely clear to me whether it''s kosher to use this as mthca
> wants.
This is under Part II - Advanced dma_ usage - I don''t think
it''s dealing with
non-consistent memory only (e.g. dma_declare_coherent_memory is there), and this
looks like a good fit.  Most functions here work for both consistent and
non-consistent memory...  What makes you suspicious?
> The other alternative is to put the MTT table in coherent memory just
> like the MPT table.  That might be the best solution I suppose...
> 
> Michael, anyone else, thoughts on this?
Certainly easy ...

I''m concerned that MTTs need a fair amount of memory,
while the amount of coherent memory might be limited.
Not that non-coherent memory systems are widespread ...

-- 
MST

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> >     void
 > >     dma_sync_single_range(struct device *dev, dma_addr_t dma_handle,
 > >     		      unsigned long offset, size_t size,
 > >     		      enum dma_data_direction direction)

 > This is under Part II - Advanced dma_ usage - I don''t think
it''s dealing with
 > non-consistent memory only (e.g. dma_declare_coherent_memory is there),
and this
 > looks like a good fit.  Most functions here work for both consistent and
 > non-consistent memory...  What makes you suspicious?

I was suspicious because it is described between the main noncoherent
API stuff and dma_cache_sync().  But I think it is probably OK.

Unfortunately it is not that good a fit for our current code, since we
use pci_map_sg() to do the DMA mapping on the MTT memory instead of
dma_map_single().

 > I''m concerned that MTTs need a fair amount of memory,
 > while the amount of coherent memory might be limited.
 > Not that non-coherent memory systems are widespread ...

Yes, for example on ppc 4xx the amount of coherent memory is quite
small by default (address space for non-cached mappings is actually
what is limited, but it amounts to the same thing).

Maybe the least bad solution is to change to using dma_map_single()
instead of pci_map_sg() in mthca_memfree.c.

 - R.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> Quoting Roland Dreier <rdreier@cisco.com>:
> Subject: Re: mthca use of dma_sync_single is bogus
> 
>  > >     void
>  > >     dma_sync_single_range(struct device *dev, dma_addr_t
dma_handle,
>  > >     		      unsigned long offset, size_t size,
>  > >     		      enum dma_data_direction direction)
> 
>  > This is under Part II - Advanced dma_ usage - I don''t think
it''s dealing with
>  > non-consistent memory only (e.g. dma_declare_coherent_memory is
there), and this
>  > looks like a good fit.  Most functions here work for both consistent
and
>  > non-consistent memory...  What makes you suspicious?
> 
> I was suspicious because it is described between the main noncoherent
> API stuff and dma_cache_sync().  But I think it is probably OK.
> 
> Unfortunately it is not that good a fit for our current code, since we
> use pci_map_sg() to do the DMA mapping on the MTT memory instead of
> dma_map_single().
> 
>  > I''m concerned that MTTs need a fair amount of memory,
>  > while the amount of coherent memory might be limited.
>  > Not that non-coherent memory systems are widespread ...
> 
> Yes, for example on ppc 4xx the amount of coherent memory is quite
> small by default (address space for non-cached mappings is actually
> what is limited, but it amounts to the same thing).
> 
> Maybe the least bad solution is to change to using dma_map_single()
> instead of pci_map_sg() in mthca_memfree.c.
Hmm.
What makes you think dma_sync_single_range can''t be used on memory
mapped
by pci_map_sg/dma_map_sg?

-- 
MST

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, Jul 09, 2007 at 11:48:06PM -0700, Roland Dreier
wrote:
> Yes, for example on ppc 4xx the amount of coherent memory is quite
> small by default (address space for non-cached mappings is actually
> what is limited, but it amounts to the same thing).
> 
> Maybe the least bad solution is to change to using dma_map_single()
> instead of pci_map_sg() in mthca_memfree.c.
And what about the attached patch to mthca_memfree? It changes alloc_pages for
pci_alloc_consistent. Using it, I can enable FMR and the driver runs fine.

Indeed, it does not solve problem with dma_sync_single() per se, on the other
hand, with pci_alloc_consistent() swiotlb is not needed thus dma_sync_single()
does nothing. But I agree it is not conceptual.

-- 
Lukáš Hejtmánek


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> What makes you think dma_sync_single_range can''t be used on memory
mapped
 > by pci_map_sg/dma_map_sg?

The fact that it''s dma_sync_*SINGLE*_range, and that there''s a
separate dma_sync_sg() function defined in DMA-API.txt.

 - R.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> Quoting Roland Dreier <rdreier@cisco.com>:
> Subject: Re: mthca use of dma_sync_single is bogus
> 
>  > What makes you think dma_sync_single_range can''t be used on
memory mapped
>  > by pci_map_sg/dma_map_sg?
> 
> The fact that it''s dma_sync_*SINGLE*_range, and that
there''s a
> separate dma_sync_sg() function defined in DMA-API.txt.
Aha. I looked at the code a bit.
Basically is seems that some architectures use the dma handle
and some the virtual address to flush the cache, that''s
where the requirement that same parameters are used for
sync single as for map single comes from.

So it seems that this requirement does not apply to s/g, and that we can just
build a scatterlist structure and do dma_sync_sg?

-- 
MST

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> And what about the attached patch to mthca_memfree? It changes alloc_pages
for
 > pci_alloc_consistent. Using it, I can enable FMR and the driver runs fine.

As Michael said, this uses a lot of consistent memory.  Probably too
much on some systems.

 - R.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> Aha. I looked at the code a bit.
 > Basically is seems that some architectures use the dma handle
 > and some the virtual address to flush the cache, that''s
 > where the requirement that same parameters are used for
 > sync single as for map single comes from.
 > 
 > So it seems that this requirement does not apply to s/g, and that we can
just
 > build a scatterlist structure and do dma_sync_sg?

The statement

    synchronise a single contiguous or scatter/gather mapping.  All the
    parameters must be the same as those passed into the single mapping
    API.

in DMA-API.txt also is clearly attached to dma_sync_sg().  So I don''t
think it''s a good idea to rely on being able to sync a different
scatterlist than the one that was originally mapped.

It actually doesn''t look too bad to replace our use of pci_map_sg()
with dma_map_single(), at least at first glance.  I''ll try to write a
patch later.

 - R.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> Quoting Roland Dreier <rdreier@cisco.com>:
> Subject: Re: [ofa-general] Re: mthca use of dma_sync_single is bogus
> 
>  > Aha. I looked at the code a bit.
>  > Basically is seems that some architectures use the dma handle
>  > and some the virtual address to flush the cache, that''s
>  > where the requirement that same parameters are used for
>  > sync single as for map single comes from.
>  > 
>  > So it seems that this requirement does not apply to s/g, and that we
can just
>  > build a scatterlist structure and do dma_sync_sg?
> 
> The statement
> 
>     synchronise a single contiguous or scatter/gather mapping.  All the
>     parameters must be the same as those passed into the single mapping
>     API.
> 
> in DMA-API.txt also is clearly attached to dma_sync_sg().  So I
don''t
> think it''s a good idea to rely on being able to sync a different
> scatterlist than the one that was originally mapped.
Hmm. This means there''s no way to sync a range within
mapping created with map_sg?
> It actually doesn''t look too bad to replace our use of
pci_map_sg()
> with dma_map_single(), at least at first glance.  I''ll try to
write a
> patch later.
Well, the reason map_sg is there is presumably because on some
architectures it''s worth it to try and make the region contigious in
DMA space.
But I agree this seems the lesser evil at this point ...

-- 
MST

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, Jul 10, 2007 at 11:06:29AM -0700, Roland Dreier
wrote:
> > And what about the attached patch to mthca_memfree? It changes
alloc_pages
> > for  pci_alloc_consistent. Using it, I can enable FMR and the driver 
> > runs fine.
> 
> As Michael said, this uses a lot of consistent memory.  Probably too
> much on some systems.
I think he spoke about coherent, didn''t he? On i386/x86_64, the
consistent and
coherent are the same but on some architectures they are not and I think that
using consistent (in particular pci_alloc_consistent) is exactly what should
be used. Keir also recommended to use this one.

And moreover, it avoids using swiotlb and bounce buffers, I think. Am I right,
Keir?

-- 
Lukáš Hejtmánek

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> I think he spoke about coherent, didn''t he? On i386/x86_64, the
consistent and
 > coherent are the same but on some architectures they are not and I think
that
 > using consistent (in particular pci_alloc_consistent) is exactly what
should
 > be used. Keir also recommended to use this one.

coherent and consistent are synonyms.  It''s confusing because there is
pci_alloc_consistent(), which is in general just a wrapper for
dma_alloc_coherent().

 > And moreover, it avoids using swiotlb and bounce buffers, I think. Am I
right,
 > Keir?

Yes, but I''m not really willing to make things worse for standard i386
just to make Xen work a little better.

 - R.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, Jul 10, 2007 at 12:08:43PM -0700, Roland Dreier
wrote:
>  > I think he spoke about coherent, didn''t he? On i386/x86_64,
the consistent and
>  > coherent are the same but on some architectures they are not and I
think that
>  > using consistent (in particular pci_alloc_consistent) is exactly what
should
>  > be used. Keir also recommended to use this one.
> 
> coherent and consistent are synonyms.  It''s confusing because
there is
> pci_alloc_consistent(), which is in general just a wrapper for
> dma_alloc_coherent().
According to DMA-mapping.txt they are not. Alpha, M68000 wihtout MMU, PPC,
Sparc, Sparc64, V850 have own implementation of pci_alloc_consistent().

Yes, on i386, the pci_alloc_consistent() is just wrapper for
dma_alloc_coherent().
>  > And moreover, it avoids using swiotlb and bounce buffers, I think. Am
I right,
>  > Keir?
> 
> Yes, but I''m not really willing to make things worse for standard
i386
> just to make Xen work a little better.
So, what about some #ifdefs ? E.g., allow config option - Xen optimizations?

-- 
Lukáš Hejtmánek

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> > coherent and consistent are synonyms.  It''s confusing because
there is
 > > pci_alloc_consistent(), which is in general just a wrapper for
 > > dma_alloc_coherent().
 > 
 > According to DMA-mapping.txt they are not. Alpha, M68000 wihtout MMU, PPC,
 > Sparc, Sparc64, V850 have own implementation of pci_alloc_consistent().
 > 
 > Yes, on i386, the pci_alloc_consistent() is just wrapper for
 > dma_alloc_coherent().

Sorry, I was a little confusing.  The implementations may be different
but in general there is no real difference between consistent and
coherent memory.  Using either pci_alloc_consistent() or
dma_alloc_coherent() will exhaust the same small pool of address space
on powerpc 4xx for example.

 > So, what about some #ifdefs ? E.g., allow config option - Xen
optimizations?

Seems pretty ugly, especially given that Xen is not upstream.  I think
the Xen tree should just carry such patches, at least until Xen is
merged.  Even then I''m quite dubious about having two code paths for
this.

 - R.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> Hmm. This means there''s no way to sync a range within
 > mapping created with map_sg?

It doesn''t seem that there is one right now at least.

 > > It actually doesn''t look too bad to replace our use of
pci_map_sg()
 > > with dma_map_single(), at least at first glance.  I''ll try
to write a
 > > patch later.
 > 
 > Well, the reason map_sg is there is presumably because on some
 > architectures it''s worth it to try and make the region contigious
in DMA space.
 > But I agree this seems the lesser evil at this point ...

Given that we''re already trying to allocate big chunks of physically
contiguous memory, I think that any virtual merging we get is likely
to be of very small benefit.

It is kind of a shame to give this up though.

 - R.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, Jul 10, 2007 at 12:24:02PM -0700, Roland Dreier
wrote:
> Sorry, I was a little confusing.  The implementations may be different
> but in general there is no real difference between consistent and
> coherent memory.  Using either pci_alloc_consistent() or
> dma_alloc_coherent() will exhaust the same small pool of address space
> on powerpc 4xx for example.
I thought that consistent only refers to physically contiguous area whereas
coherent refers to memory where no barrier need to be used. But I may be
wrong.

Anyway, with my patch, I can turn off swiotlb and I''m still able to
load
ib_mthca cleanly in DomU. On the other hand, Xen bug me about DMA bug in
ib_ipoib, there may be another problem with dma_sync_single().
> Seems pretty ugly, especially given that Xen is not upstream.  I think
> the Xen tree should just carry such patches, at least until Xen is
> merged.  Even then I''m quite dubious about having two code paths
for this.
OK, I will keep it for my own.

-- 
Lukáš Hejtmánek

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> Quoting Roland Dreier <rdreier@cisco.com>:
> Subject: Re: [ofa-general] Re: mthca use of dma_sync_single is bogus
> 
>  > Hmm. This means there''s no way to sync a range within
>  > mapping created with map_sg?
> 
> It doesn''t seem that there is one right now at least.
> 
>  > > It actually doesn''t look too bad to replace our use of
pci_map_sg()
>  > > with dma_map_single(), at least at first glance.  I''ll
try to write a
>  > > patch later.
>  > 
>  > Well, the reason map_sg is there is presumably because on some
>  > architectures it''s worth it to try and make the region
contigious in DMA space.
>  > But I agree this seems the lesser evil at this point ...
> 
> Given that we''re already trying to allocate big chunks of
physically
> contiguous memory, I think that any virtual merging we get is likely
> to be of very small benefit.
> 
> It is kind of a shame to give this up though.
Did we reach any conclusion? Are you switching to map_single?

-- 
MST

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> Did we reach any conclusion? Are you switching to map_single?
haven''t had a chance to work on it yet, but I don''t see a
better alternative.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel