Re: [PATCH 4/7] swiotlb: Allow arch override of address_needs_mapping

[Jeremy Fitzhardinge <jeremy@goop.org>]

FUJITA Tomonori wrote:
> On Wed, 08 Apr 2009 16:16:17 -0700
> Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>
>   
>> FUJITA Tomonori wrote:
>>     
>>>> Becky's patches of last week also added __weak annotations to 
>>>> swiotlb_bus_to_virt, virt_to_bus and bus_to_phys; added the hwdev 
>>>> parameter to swiotlb_bus_to_phys; and added a weak 
>>>> swiotlb_arch_address_needs_mapping.  I assume that was needed because 
>>>> powerpc needs non-trivial implementations for those functions.
>>>>     
>>>>         
>>> Hmm, what she added are wrappers of virt_to_bus and bus_to_virt. We
>>> can remove these and directly use virt_to_bus and bus_to_virt.
>>>   
>>>       
>> In general those interfaces are deprecated.  Are we un-deprecating 
>> them?  Or do you mean adding virt<->bus to dma_ops?
>>     
>
> Hmm, these interfaces are wrong for drivers surely because they can't
> handle dma mapping properly. However, they are exactly what swiotlb
> needs (swiotlb doesn't need to care about dma mapping).

It needs to care about the mapping from phys to bus.  On x86 they're 
identical, but on powerpc there can be at least an offset between them.

>  Until 2.6.28,
> swiotlb has used them. They are with IA64, X86_64 and PPC_32, I think.
>   

Well, Becky's patches also added the hwdev argument to them, so 
presumably the powerpc implementation needs that (different 
devices/buses have differing views of physical memory, I guess).

> I'm not sure what you mean. And I don't think ppc wants swiotlb_alloc.
>   

No, its something we need for Xen.  I was thinking that swiotlb could 
allocate its memory with dma_alloc_coherent(NULL, size, ...).  That 
would allocate via x86_fallback_device, which would not have the right 
behaviour (it would set GFP_DMA, for a start), and would end up hitting 
the uninitialized dma_ops.  So the idea doesn't really work; it would 
need swiotlb to define another placeholder  device who's alloc_coherent 
operation could be overridden, and it all gets pretty ugly.

As an aside, I'm also wondering why there's a distinction between 
swiotlb_alloc() and swiotlb_alloc_boot().  The latter allocates from 
bootmem, but I don't see what's wrong with allocating from slab a little 
bit later, once it has been initialized.  The comment mentions something 
about allocating ISA DMA memory, but the code doesn't make any attempt 
to allocate the buffer below 16MB (its generally much larger than 16MB 
anyway).

    J