Message-ID: <1247234512.4002.417.camel@zakaz.uk.xensource.com>
Date:	Fri, 10 Jul 2009 15:01:52 +0100
From:	Ian Campbell <Ian.Campbell@...citrix.com>
To:	Ingo Molnar <mingo@...e.hu>
CC:	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>,
	Jeremy Fitzhardinge <jeremy@...p.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-ia64@...r.kernel.org" <linux-ia64@...r.kernel.org>,
	"linuxppc-dev@...abs.org" <linuxppc-dev@...abs.org>,
	"benh@...nel.crashing.org" <benh@...nel.crashing.org>,
	"tony.luck@...el.com" <tony.luck@...el.com>,
	"x86@...nel.org" <x86@...nel.org>,
	"beckyb@...nel.crashing.org" <beckyb@...nel.crashing.org>,
	Joerg Roedel <joerg.roedel@....com>
Subject: Re: [00/15] swiotlb cleanup

On Fri, 2009-07-10 at 06:12 +0100, Ingo Molnar wrote:
> Hm, the functions and facilities you remove here were added as part 
> of preparatory patches for Xen guest support. You were aware of 
> them, you were involved in discussions about those aspects with Ian 
> and Jeremy but still you chose not to Cc: either of them and you 
> failed to address that aspect in the changelogs.

Thanks for adding me Ingo.

> Alas, on the technical level the cleanups themselves look mostly 
> fine to me. Ian, Jeremy, the changes will alter Xen's use of 
> swiotlb, but can the Xen side still live with these new methods - in 
> particular is dma_capable() sufficient as a mechanism and can the 
> Xen side filter out DMA allocations to make them physically 
> continuous?

I've not examined the series in detail it looks OK but I don't think it
is quite sufficient. The Xen determination of whether a buffer is
dma_capable or not is based on the physical address while dma_capable
takes only the dma address.

I'm not sure we can "invert" our conditions to work back from dma
address to physical since given a start dma address and a length we
would need to check that dma_to_phys(dma+PAGE_SIZE) ==
dma_to_phys(dma)+PAGE_SIZE etc. However dma+PAGE_SIZE might belong to a
different domain so translating it to a physical address in isolation
tells us nothing especially useful since it would give us the physical
address in that other guest which is useless to us. If we could pass
both physical and dma address to dma_capable I think that would probably
be sufficient for our purposes.

As well as that Xen needs some way to influence the allocation of the
actual bounce buffer itself since we need to arrange for it to be
machine address contiguous as well as physical address contiguous. This
series explicitly removes those hooks without replacement. My most
recent proposal was to have a new swiotlb_init variant which was given a
preallocated buffer which this series doesn't necessarily preclude.

The phys_to_dma and dma_to_phys translation points are the last piece
Xen needs and seem to be preserved in this series.

However Fujita's objection to all of the previous swiotlb-for-xen
proposals was around the addition of the Xen hooks in whichever
location. Originally these hooks were via __weak functions and later
proposals implemented them via function pointer hooks in the x86
implementations of the arch-abstract interfaces (phys<->dma and
dma_capable etc). I don't think this series addresses those objections
(fair enough -- it wasn't intended to) or leads to any new approach to
solving the issue, although I also don't think it makes the issue any
harder to address. I don't think it will be possible to make progress on
Xen usage of swiotlb until a solution can be found to this conflict of
opinion.

Fujita suggested that we export the core sync_single() functionality and
reimplemented the surrounding infrastructure in terms of that (and
incorporating our additional requirements). I prototyped this (it is
currently unworking, in fact it seems to have developed rather a taste
for filesystems :-() but the diffstat of my WIP patch is:
         arch/x86/kernel/pci-swiotlb.c |    6 
         arch/x86/xen/pci-swiotlb.c    |    2 
         drivers/pci/xen-iommu.c       |  385 ++++++++++++++++++++++++++++++++++++++++--
         include/linux/swiotlb.h       |   12 +
         lib/swiotlb.c                 |   10 -
         5 files changed, 385 insertions(+), 30 deletions(-)
where a fair number of the lines in xen-iommu.c are copies of functions
from swiotlb.c with minor modifications. As I say it doesn't work yet
but I think it's roughly indicative of what such an approach would look
like. I don't like it much but am happy to run with it if it looks to be
the most acceptable approach. To be honest at the moment I've
deliberately been taking some time away from this stuff to try and gain
a bit of perspective so I haven't looked at it in a while.

Ian.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists