Re: [PATCH 00 of 14] swiotlb/x86: lay groundwork for xen dom0 use of swiotlb

[Becky Bruce <beckyb@kernel.crashing.org>]

On Dec 17, 2008, at 10:56 AM, FUJITA Tomonori wrote:

> On Wed, 17 Dec 2008 08:31:43 -0800
> Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>
>>> I think that the whole patchset is against the swiotlb design.  
>>> swiotlb
>>> is designed to be used as a library. Each architecture implements  
>>> the
>>> own swiotlb by using swiotlb library
>>> (e.g. arch/x86/kernel/pci-swiotlb_64.c).
>>>
>>
>> The whole patchset?  The bulk of the changes to lib/swiotlb.c are
>> relatively minor to remove the unwarranted assumptions it is making  
>> in
>> the face of a new user.  They will have no effect on other existing
>> users, including non-Xen x86 builds.
>>
>> If you have specific objections we can discuss those, but I don't  
>> think
>> there's anything fundamentally wrong with making lib/swiotlb.c a bit
>> more generically useful.
>
> Sorry, but the highmem support is not generically useful.
>
> I'm especially against the highmem support. As you said, the rest
> looks fine but if you go with pci-swiotlb_32.c, I think that you don't
> need the most of them.

>
>
>
>>> For example, adding the following code (9/14) for just Xen that the
>>> majority of swiotbl users (x86_64 and IA64) don't need to the  
>>> library
>>> is against the design.
>>>
>>
>> If the architecture doesn't support highmem then this code will  
>> compile
>> to nothing - PageHighMem() will always evaluate to 0.  It will  
>> therefore
>
> I'm not talking about it will be complied or not. As I wrote in
> another mail, I'm talking about the maintainability and readability of
> the common library.
>
>
>> have zero effect on the code generated for IA64 or x86-64.  This is  
>> not
>> really a Xen-specific change, but a result of adding swiotlb  
>> support for
>> i386.  Other architectures which support a notion of highmem would  
>> also
>> need this code if they wanted to use swiotlb.
>
> Can you be more specific? What architecture is plan to use highmem
> support in swiotlb?

32-bit powerpc needs this support - I was actually about to push a  
similar set of patches.  We have several processors that support 36  
bits of physical address space and do not have any iommu capability.   
The rest of the kernel support for those processors is now in place,  
so swiotlb is the last piece of the puzzle for that to be fully  
functional.  I need to take a closer look at this series to see  
exactly what it's doing and how it differs from what I've been testing.

So there is another immediate use case, and I'd really hate to see  
this code duplicated.  It should be entirely possible to remove the  
assumption that we can save off the VA of the original buffer, which  
is the thing that precludes HIGHMEM support, and still have nice  
readable, maintainable code.

Cheers,
-Becky