>>> Keir Fraser <Keir.Fraser@xxxxxxxxxxxx> 12/25/06 11:20 AM >>>
>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.
Because we allow highmem pages in the I/O path, hence page_address() cannot
be used. As you may have concluded from my sending of a second rev of the
patches, I had a bug in exactly that path, so I know it is being exercised.
Of course, all this exists for x86-32/PAE *only*, so it may be valid to
raise the question if it's worth it. But otoh with supporting (only) 32-bit
PAE PV guests on x86-64 we are in the process of widening the use case here.
> * 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.
Likewise.
> * 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?
Fixing the block drivers would certainly be nice (I'm not exactly clear where
this dependency lives on their side), but due to the above we'll be unable to
completely switch over to memcpy() only.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home