Re: [Xen-devel] x86 swiotlb questions

[Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>]

On 25/12/06 4:50 am, "Muli Ben-Yehuda" <muli@xxxxxxxxxx> wrote:

> I'm sorry, but this patch is horrible. swiotlb.c is now pretty much
> unreadable. I'd be surprised if mainline accepted it - I would
> certainly NAK it with my mainline hat on, especially for an unmerged
> architecture.
> 
> If Xen needs so many "abstractions", I have to ask whether it isn't
> better off just using its own swiotlb.c as we are now.
> 
> I'll take another look at this later and try to come up with a
> different way of merging them that isn't quite this horrible. Maybe
> using function pointers for the "low level" operations?

A lot of this ugliness comes from swiotlb_[un]map_page, the introduction of
a structural 'io_tlb_addr_t', and a more complicated synchronisation
function than memcpy (which uses copy_to_user and kmap).

However, I don't remember *why* we need these Xen-specific customisations
(even though I was the one who originally made them, way back). That given,
it would probably be sensible to make a more brutal merge that discards Xen
differences which we cannot explain. For example:

 * Why can't we turn dma_[un]map_page into dma_[un]map_single, as x86_64
does? This would avoid needing to expand the swiotlb api.

 * Why can't we store virtual addresses in the io_tlb_orig_addr[] array just
like lib/swiotlb.c, given that the native swiotlb is happy to use
page_address() on any 'struct page' that is passed to it? I can't see why we
actually need KM_SWIOTLB and to use kmap_atomic.

 * If we make the above changes, do we need special sync_single() any more?
We'll no longer need kmap_atomic, but we'll still have
copy_to_user_inatomic, due to abuses of the DMA API by the block-device
subsystem. Maybe that has been fixed already? Or maybe, in merging this
upstream, we should aim to fix the block-device drivers rather than work
around?

Even if we wait for some of the patches to be merged upstream before
applying the swiotlb changes to our own Linux tree, I'd consider patches
just to remove the above differences relative to lib/swiotlb.c. This might
reduce the diff but would also let us get some testing of these swiotlb
simplifications.

The differences I was expecting to need to make were just the following:
 * Initialisation time -- we want to be able to automatically conditionally
initialise the swiotlb, and allocate the aperture contents in a special way.
 * Usage -- 'force' has difference semantics for Xen (means forcibly
allocate a swiotlb, but not use it on every device access whether it is
needed or not). We want to be able to use the swiotlb only when it is needed
for a particular device access; this may in particular require special
treatment of the unmap api calls to decide whether or not the given
dma_address resides in the swiotlb aperture.

 -- Keir



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