[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

FUJITA Tomonori wrote:
On Wed, 17 Dec 2008 08:31:43 -0800
Jeremy Fitzhardinge <jeremy@xxxxxxxx> 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.

That's a circular argument. lib/swiotlb currently used by 1 1/2 of the 23 architectures, neither of which happens to use highmem. If you consider swiotlb to be a general purpose mechanism, then presumably the other 21 1/2 architectures are at least potential users (and 6 1/2 of those have highmem configurations). If you base your judgement of what's a "generically useful" change based on what the current users need, then you'll naturally exclude the requirements of all the other (potential) users.

And the matter arises now because we're trying to unify the use of swiotlb in x86, bringing the number of users up to 2.

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.

I really don't want to have to duplicate a lot of code just to incorporate a few small changes. In fact the original Xen patch set included its own swiotlb implementation, and that was rejected on the grounds that we should use the common swiotlb.c.

This patch set abstracts out a few more architecture-specific details, to allow the common swiotlb logic to be in one place. By not putting highmem support into lib/swiotlb.c, you're effectively saying that we should have separate lib/swiotlb-nohighmem.c and lib/swiotlb-highmem.c files, with almost identical contents aside from the few places where we need to use page+offset rather than a kernel vaddr.

Yes, highmem is ugly, and there'll be cheering in the streets when we can finally kill the last highmem user. But for now we need to deal with it, and unfortunately swiotlb is now one of the places that it affects. Its true that adding highmem support to swiotlb.c increases the maintenance burden to some extent. But I can't see an alternative that doesn't increase the burden a lot more.

We can look at abstracting things a bit more so that non-highmem architectures can keep using simple addresses rather than page+offset, but that highmem code has got to be *somewhere*.

> 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?

Well, concretely, x86-32. I don't know about the others, but I don't see why there's an inherent reason that they won't have a problem that swiotlb solves.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.