[00/15] swiotlb cleanup

[Ingo Molnar]

* Ian Campbell <Ian.Campbell@...> wrote:

> 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. [...]

+400 lines of code to avoid much fewer lines of generic code impact
on the lib/swiotlb.c side sounds like a bad technical choice to me.

It makes the swiotlb code less useful and basically forks a random
implementation of it in drivers/pci/xen-iommu.c.

Fujita-san, can you think of a solution that avoids the whole-sale
copying of hundreds of lines of code?

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

[FUJITA Tomonori]

On Fri, 10 Jul 2009 16:12:48 +0200
Ingo Molnar <mingo@...> wrote:

> > 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. [...]
>
> +400 lines of code to avoid much fewer lines of generic code impact
> on the lib/swiotlb.c side sounds like a bad technical choice to me.

The amount of code is not the point. The way to impact on the
lib/swiotlb.c is totally wrong from the perspective of the kernel
design; it uses architecture code in the very original (xen) way.


> It makes the swiotlb code less useful and basically forks a random
> implementation of it in drivers/pci/xen-iommu.c.

I don't think so. We always have the reasonable amount of duplication
rather than integration in a dirty way (at least about IOMMU
code).


> Fujita-san, can you think of a solution that avoids the whole-sale
> copying of hundreds of lines of code?

Ian didn't give a pointer to his code in a public place so I'm not
sure his claim is valid. But if his code does the right thing and
the duplication is less than 400 lines, it doesn't sound that bad to
me.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@...
https://lists.ozlabs.org/listinfo/linuxppc-dev

[FUJITA Tomonori]

On Fri, 10 Jul 2009 15:02:00 +0100
Ian Campbell <Ian.Campbell@...> wrote:

> On Fri, 2009-07-10 at 14:35 +0900, FUJITA Tomonori wrote:
> > I don't think that we need to take account of dom0 support; we don't
> > have a clear idea about an acceptable dom0 design (it needs to use
> > swiotlb code? I don't know yet), we don't even know we will have dom0
> > support in mainline. That's why I didn't CC this patchset to Xen
> > camp.
>
> The core domain 0 patches which were the subject of the discussions a
> few week back are completely orthogonal to the swiotlb side of things

? If we don't merge dom0 patch, we don't need dom0 changes to
swiotlb. We don't know we would have dom0 support in mainline. Or I
overlooked something?

> and whatever form they eventually take I do not think it will have any
> impact on the shape of the solution which we arrive at for swiotlb. I
> don't think that assuming that domain 0 can never be done in a way which
> everyone finds acceptable and therefore discounting all consideration of
> it is a useful way to make progress with these issues.
>
> The DMA use case is much more tightly tied to the paravirtualized MMU
> (which is already in the kernel for domU purposes) than it is to "the
> domain 0" patches anyway. Although domain 0 is probably the main use
> case, at least today, swiotlb support is also used in a Xen domU as part
> of the support for direct assignment of PCI devices to paravirtualised
> guests (pci passthrough).
>
> The pci frontend driver depends on some bits of the domain 0 physical
> interrupt patches as well as swiotlb which is why I/we haven't tried to
> upstream that particular series yet.

As far as I know, you have not posted anything about changes to
swiotlb for domU. I can't discuss it. If you want, please send
patches.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[FUJITA Tomonori]

On Mon, 13 Jul 2009 13:20:22 +0900
FUJITA Tomonori <fujita.tomonori@...> wrote:

> On Fri, 10 Jul 2009 16:12:48 +0200
> Ingo Molnar <mingo@...> wrote:
>
> > > 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. [...]
> >
> > +400 lines of code to avoid much fewer lines of generic code impact
> > on the lib/swiotlb.c side sounds like a bad technical choice to me.
>
> The amount of code is not the point. The way to impact on the
> lib/swiotlb.c is totally wrong from the perspective of the kernel
> design; it uses architecture code in the very original (xen) way.

btw, '+400 lines of code to avoid much fewer lines of generic code
impact on the lib/swiotlb.c' doesn't sound true to me.

Here is a patch in the way that Xen people want to do:

http://patchwork.kernel.org/patch/26343/

---
 arch/x86/Kconfig                 |    4 +
 arch/x86/include/asm/io.h        |    2 +
 arch/x86/include/asm/pci_x86.h   |    1 +
 arch/x86/include/asm/xen/iommu.h |   12 ++
 arch/x86/kernel/pci-dma.c        |    3 +
 arch/x86/pci/Makefile            |    1 +
 arch/x86/pci/init.c              |    6 +
 arch/x86/pci/xen.c               |   51 +++++++
 drivers/pci/Makefile             |    2 +
 drivers/pci/xen-iommu.c          |  271 ++++++++++++++++++++++++++++++++++++++


Even with the way that Xen people want to do, drivers/pci/xen-iommu.c
is about 300 lines. And my patchset removes the nice amount of lines
for dom0 support. I don't see much difference wrt lines.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@...
https://lists.ozlabs.org/listinfo/linuxppc-dev

[Ian Campbell-12]

On Mon, 2009-07-13 at 05:20 +0100, FUJITA Tomonori wrote:

> On Fri, 10 Jul 2009 15:02:00 +0100
> Ian Campbell <Ian.Campbell@...> wrote:
>
> > On Fri, 2009-07-10 at 14:35 +0900, FUJITA Tomonori wrote:
> > > I don't think that we need to take account of dom0 support; we don't
> > > have a clear idea about an acceptable dom0 design (it needs to use
> > > swiotlb code? I don't know yet), we don't even know we will have dom0
> > > support in mainline. That's why I didn't CC this patchset to Xen
> > > camp.
> >
> > The core domain 0 patches which were the subject of the discussions a
> > few week back are completely orthogonal to the swiotlb side of things
>
> ? If we don't merge dom0 patch, we don't need dom0 changes to
> swiotlb. We don't know we would have dom0 support in mainline. Or I
> overlooked something?

[...]
> As far as I know, you have not posted anything about changes to
> swiotlb for domU. I can't discuss it. If you want, please send
> patches.

There are no separate domU swiotlb patches -- the exact the same patches
as we have already been discussing are useful and necessary for both
domU and dom0.

Ian.


_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@...
https://lists.ozlabs.org/listinfo/linuxppc-dev

[FUJITA Tomonori]

On Mon, 13 Jul 2009 10:40:29 +0100
Ian Campbell <Ian.Campbell@...> wrote:

> On Mon, 2009-07-13 at 05:20 +0100, FUJITA Tomonori wrote:
> > On Fri, 10 Jul 2009 15:02:00 +0100
> > Ian Campbell <Ian.Campbell@...> wrote:
> >
> > > On Fri, 2009-07-10 at 14:35 +0900, FUJITA Tomonori wrote:
> > > > I don't think that we need to take account of dom0 support; we don't
> > > > have a clear idea about an acceptable dom0 design (it needs to use
> > > > swiotlb code? I don't know yet), we don't even know we will have dom0
> > > > support in mainline. That's why I didn't CC this patchset to Xen
> > > > camp.
> > >
> > > The core domain 0 patches which were the subject of the discussions a
> > > few week back are completely orthogonal to the swiotlb side of things
> >
> > ? If we don't merge dom0 patch, we don't need dom0 changes to
> > swiotlb. We don't know we would have dom0 support in mainline. Or I
> > overlooked something?
> [...]
> > As far as I know, you have not posted anything about changes to
> > swiotlb for domU. I can't discuss it. If you want, please send
> > patches.
>
> There are no separate domU swiotlb patches -- the exact the same patches
> as we have already been discussing are useful and necessary for both
> domU and dom0.

Hmm, you guys introduced the swiotlb hooks by saying that it's for
only dom0.

=
http://lkml.org/lkml/2008/12/16/351

Hi Ingo,

Here's some patches to clean up swiotlb to prepare for some Xen dom0
patches.  These have been posted before, but undergone a round of
cleanups to address various comments.

=

I don't see any comments like 'this is useful to dom0 too'. I'm still
not sure what exactly part is useful to domU.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@...
https://lists.ozlabs.org/listinfo/linuxppc-dev

[Ian Campbell-12]

On Mon, 2009-07-13 at 18:53 +0900, FUJITA Tomonori wrote:

> On Mon, 13 Jul 2009 10:40:29 +0100
> Ian Campbell <Ian.Campbell@...> wrote:
>
> > On Mon, 2009-07-13 at 05:20 +0100, FUJITA Tomonori wrote:
> > > On Fri, 10 Jul 2009 15:02:00 +0100
> > > Ian Campbell <Ian.Campbell@...> wrote:
> > >
> > > > On Fri, 2009-07-10 at 14:35 +0900, FUJITA Tomonori wrote:
> > > > > I don't think that we need to take account of dom0 support; we don't
> > > > > have a clear idea about an acceptable dom0 design (it needs to use
> > > > > swiotlb code? I don't know yet), we don't even know we will have dom0
> > > > > support in mainline. That's why I didn't CC this patchset to Xen
> > > > > camp.
> > > >
> > > > The core domain 0 patches which were the subject of the discussions a
> > > > few week back are completely orthogonal to the swiotlb side of things
> > >
> > > ? If we don't merge dom0 patch, we don't need dom0 changes to
> > > swiotlb. We don't know we would have dom0 support in mainline. Or I
> > > overlooked something?
> > [...]
> > > As far as I know, you have not posted anything about changes to
> > > swiotlb for domU. I can't discuss it. If you want, please send
> > > patches.
> >
> > There are no separate domU swiotlb patches -- the exact the same patches
> > as we have already been discussing are useful and necessary for both
> > domU and dom0.
>
> Hmm, you guys introduced the swiotlb hooks by saying that it's for
> only dom0.

That was just sloppy wording on our part. domain 0 is the major usecase
today so there is a tendency to think in those terms but the changes are
actually relevant to any domain with access to a physical device that
can do DMA, this includes domU via PCI passthrough.

> I don't see any comments like 'this is useful to dom0 too'. I'm still
                                                      ^U?
> not sure what exactly part is useful to domU.

All of it...

Ian.

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@...
https://lists.ozlabs.org/listinfo/linuxppc-dev

[Becky Bruce-3]

On Jul 9, 2009, at 8:04 PM, FUJITA Tomonori wrote:

> swiotlb_bus_to_virt is unncessary; we can use swiotlb_bus_to_phys and
> phys_to_virt instead.

phys_to_virt (also, virt_to_phys) is invalid for highmem addresses on  
ppc.  In most of the uses in this file, it doesn't matter, as the  
iotlb buffers themselves are alloc'd out of lowmem, but the  
dma_mark_clean() calls could happen on a highmem addr.  Currently, on  
powerpc, dma_mark_clean() doesn't do anything, so it isn't a  
functional problem.  I'm fine with the bulk of this patch, because in  
reality, if I need to do something with a virtual address of a highmem  
page, I have to get a kmap for the page.  So the existing  
swiotlb_bus_to_virt isn't really helping.

What is dma_mark_clean used for?  Could it be made to take a paddr,  
and let the implementation deal with making sure there's a valid vaddr  
for the actual "clean" operation?

I'd also like to see a comment with swiotlb_virt_to_bus() that notes  
that it should only be called on addresses in lowmem.

Cheers,
Becky

>
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...>
> ---
> arch/powerpc/kernel/dma-swiotlb.c |   10 ----------
> lib/swiotlb.c                     |   34 +++++++++++++++
> +------------------
> 2 files changed, 16 insertions(+), 28 deletions(-)
>
> diff --git a/arch/powerpc/kernel/dma-swiotlb.c b/arch/powerpc/kernel/
> dma-swiotlb.c
> index 68ccf11..41534ae 100644
> --- a/arch/powerpc/kernel/dma-swiotlb.c
> +++ b/arch/powerpc/kernel/dma-swiotlb.c
> @@ -24,16 +24,6 @@
> int swiotlb __read_mostly;
> unsigned int ppc_swiotlb_enable;
>
> -void *swiotlb_bus_to_virt(struct device *hwdev, dma_addr_t addr)
> -{
> - unsigned long pfn = PFN_DOWN(swiotlb_bus_to_phys(hwdev, addr));
> - void *pageaddr = page_address(pfn_to_page(pfn));
> -
> - if (pageaddr != NULL)
> - return pageaddr + (addr % PAGE_SIZE);
> - return NULL;
> -}
> -
> dma_addr_t swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t  
> paddr)
> {
> return paddr + get_dma_direct_offset(hwdev);
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index dc1cd1f..1a89c84 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -130,11 +130,6 @@ static dma_addr_t swiotlb_virt_to_bus(struct  
> device *hwdev,
> return swiotlb_phys_to_bus(hwdev, virt_to_phys(address));
> }
>
> -void * __weak swiotlb_bus_to_virt(struct device *hwdev, dma_addr_t  
> address)
> -{
> - return phys_to_virt(swiotlb_bus_to_phys(hwdev, address));
> -}
> -
> int __weak swiotlb_arch_address_needs_mapping(struct device *hwdev,
>       dma_addr_t addr, size_t size)
> {
> @@ -307,9 +302,10 @@ address_needs_mapping(struct device *hwdev,  
> dma_addr_t addr, size_t size)
> return swiotlb_arch_address_needs_mapping(hwdev, addr, size);
> }
>
> -static int is_swiotlb_buffer(char *addr)
> +static int is_swiotlb_buffer(phys_addr_t paddr)
> {
> - return addr >= io_tlb_start && addr < io_tlb_end;
> + return paddr >= virt_to_phys(io_tlb_start) &&
> + paddr < virt_to_phys(io_tlb_end);
> }
>
> /*
> @@ -582,11 +578,13 @@ EXPORT_SYMBOL(swiotlb_alloc_coherent);
>
> void
> swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
> -      dma_addr_t dma_handle)
> +      dma_addr_t dev_addr)
> {
> + phys_addr_t paddr = swiotlb_bus_to_phys(hwdev, dev_addr);
> +
> WARN_ON(irqs_disabled());
> - if (!is_swiotlb_buffer(vaddr))
> - free_pages((unsigned long) vaddr, get_order(size));
> + if (!is_swiotlb_buffer(paddr))
> + free_pages((unsigned long)vaddr, get_order(size));
> else
> /* DMA_TO_DEVICE to avoid memcpy in unmap_single */
> do_unmap_single(hwdev, vaddr, size, DMA_TO_DEVICE);
> @@ -671,19 +669,19 @@ EXPORT_SYMBOL_GPL(swiotlb_map_page);
> static void unmap_single(struct device *hwdev, dma_addr_t dev_addr,
> size_t size, int dir)
> {
> - char *dma_addr = swiotlb_bus_to_virt(hwdev, dev_addr);
> + phys_addr_t paddr = swiotlb_bus_to_phys(hwdev, dev_addr);
>
> BUG_ON(dir == DMA_NONE);
>
> - if (is_swiotlb_buffer(dma_addr)) {
> - do_unmap_single(hwdev, dma_addr, size, dir);
> + if (is_swiotlb_buffer(paddr)) {
> + do_unmap_single(hwdev, phys_to_virt(paddr), size, dir);
> return;
> }
>
> if (dir != DMA_FROM_DEVICE)
> return;
>
> - dma_mark_clean(dma_addr, size);
> + dma_mark_clean(phys_to_virt(paddr), size);
> }
>
> void swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
> @@ -708,19 +706,19 @@ static void
> swiotlb_sync_single(struct device *hwdev, dma_addr_t dev_addr,
>    size_t size, int dir, int target)
> {
> - char *dma_addr = swiotlb_bus_to_virt(hwdev, dev_addr);
> + phys_addr_t paddr = swiotlb_bus_to_phys(hwdev, dev_addr);
>
> BUG_ON(dir == DMA_NONE);
>
> - if (is_swiotlb_buffer(dma_addr)) {
> - sync_single(hwdev, dma_addr, size, dir, target);
> + if (is_swiotlb_buffer(paddr)) {
> + sync_single(hwdev, phys_to_virt(paddr), size, dir, target);
> return;
> }
>
> if (dir != DMA_FROM_DEVICE)
> return;
>
> - dma_mark_clean(dma_addr, size);
> + dma_mark_clean(phys_to_virt(paddr), size);

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

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@...
https://lists.ozlabs.org/listinfo/linuxppc-dev

[Becky Bruce-3]

On Jul 10, 2009, at 12:12 AM, Ingo Molnar wrote:

>
> * FUJITA Tomonori <fujita.tomonori@...> wrote:
>
>> - removes unused (and unnecessary) hooks in swiotlb.
>>
>> - adds dma_capable() and converts swiotlb to use it. It can be used  
>> to
>> know if a memory area is dma capable or not. I added
>> is_buffer_dma_capable() for the same purpose long ago but it turned
>> out that the function doesn't work on POWERPC.
>>
>> This can be applied cleanly to linux-next, -mm, and mainline. This
>> patchset touches multiple architectures (ia64, powerpc, x86) so I
>> guess that -mm is appropriate for this patchset (I don't care much
>> what tree would merge this though).
>>
>> This is tested on x86 but only compile tested on POWERPC and IA64.
>>
>> Thanks,
>>
>> =
>> arch/ia64/include/asm/dma-mapping.h    |   18 ++++++
>> arch/powerpc/include/asm/dma-mapping.h |   23 +++++++
>> arch/powerpc/kernel/dma-swiotlb.c      |   48 +---------------
>> arch/x86/include/asm/dma-mapping.h     |   18 ++++++
>> arch/x86/kernel/pci-dma.c              |    2 +-
>> arch/x86/kernel/pci-gart_64.c          |    5 +-
>> arch/x86/kernel/pci-nommu.c            |    2 +-
>> arch/x86/kernel/pci-swiotlb.c          |   25 --------
>> include/linux/dma-mapping.h            |    5 --
>> include/linux/swiotlb.h                |   11 ----
>> lib/swiotlb.c                          |  102 ++++++++
>> +-----------------------
>> 11 files changed, 92 insertions(+), 167 deletions(-)
>
> 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.
>
> I'd like the Xen code to become cleaner more than anyone else here i
> guess, but patch submission methods like this are not really
> helpful. A far better method is to be open about such disagreements,
> to declare them, to Cc: everyone who disagrees, and to line out the
> arguments in the changelogs as well - instead of just curtly
> declaring those APIs 'unused' and failing to Cc: involved parties.
>
> 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?
>
> Ben, Tony, Becky, any objections wrt. the PowerPC / IA64 impact? If
> everyone agrees i can apply them to the IOMMU tree, test it and push
> it out to -next, etc.
>

Ingo,

With the exception of the patch I commented on, I think these look OK  
from the powerpc point of view.  I've successfully booted one of my  
test platforms with the entire series applied and will run some more  
extensive (i.e. not "Whee!  A prompt!") tests tomorrow.

-Becky

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@...
https://lists.ozlabs.org/listinfo/linuxppc-dev

[FUJITA Tomonori]

On Mon, 13 Jul 2009 21:17:21 -0500
Becky Bruce <beckyb@...> wrote:

>
> On Jul 9, 2009, at 8:04 PM, FUJITA Tomonori wrote:
>
> > swiotlb_bus_to_virt is unncessary; we can use swiotlb_bus_to_phys and
> > phys_to_virt instead.
>
> phys_to_virt (also, virt_to_phys) is invalid for highmem addresses on  
> ppc.  In most of the uses in this file, it doesn't matter, as the  
> iotlb buffers themselves are alloc'd out of lowmem,

Right,

> but the  
> dma_mark_clean() calls could happen on a highmem addr.  Currently, on  
> powerpc, dma_mark_clean() doesn't do anything, so it isn't a  
> functional problem.

Oops, I overlooked this. However, as you said, this is not a problem
with the current code.


>  I'm fine with the bulk of this patch, because in  
> reality, if I need to do something with a virtual address of a highmem  
> page, I have to get a kmap for the page.  So the existing  
> swiotlb_bus_to_virt isn't really helping.
>
> What is dma_mark_clean used for?  Could it be made to take a paddr,  
> and let the implementation deal with making sure there's a valid vaddr  
> for the actual "clean" operation?

I think that it's IA64's optimization (it's a NULL function on X86
like POWERPC). It's defined in arch/ia64/mm/init.c. Looks like POWERPC
could use this optimization, I guess.

dma_mark_clean() just modifies the page flag (what it needs is struct
page) so I think that we can make it take a paddr.


> I'd also like to see a comment with swiotlb_virt_to_bus() that notes  
> that it should only be called on addresses in lowmem.

Ok, I'll do.


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

[Becky Bruce-3]

On Jul 13, 2009, at 10:13 PM, Becky Bruce wrote:

>
> On Jul 10, 2009, at 12:12 AM, Ingo Molnar wrote:
>
>>
>> * FUJITA Tomonori <fujita.tomonori@...> wrote:
>>
>>> - removes unused (and unnecessary) hooks in swiotlb.
>>>
>>> - adds dma_capable() and converts swiotlb to use it. It can be  
>>> used to
>>> know if a memory area is dma capable or not. I added
>>> is_buffer_dma_capable() for the same purpose long ago but it turned
>>> out that the function doesn't work on POWERPC.
>>>
>>> This can be applied cleanly to linux-next, -mm, and mainline. This
>>> patchset touches multiple architectures (ia64, powerpc, x86) so I
>>> guess that -mm is appropriate for this patchset (I don't care much
>>> what tree would merge this though).
>>>
>>> This is tested on x86 but only compile tested on POWERPC and IA64.
>>>
>>> Thanks,
>>>
>>> =
>>> arch/ia64/include/asm/dma-mapping.h    |   18 ++++++
>>> arch/powerpc/include/asm/dma-mapping.h |   23 +++++++
>>> arch/powerpc/kernel/dma-swiotlb.c      |   48 +---------------
>>> arch/x86/include/asm/dma-mapping.h     |   18 ++++++
>>> arch/x86/kernel/pci-dma.c              |    2 +-
>>> arch/x86/kernel/pci-gart_64.c          |    5 +-
>>> arch/x86/kernel/pci-nommu.c            |    2 +-
>>> arch/x86/kernel/pci-swiotlb.c          |   25 --------
>>> include/linux/dma-mapping.h            |    5 --
>>> include/linux/swiotlb.h                |   11 ----
>>> lib/swiotlb.c                          |  102 ++++++++
>>> +-----------------------
>>> 11 files changed, 92 insertions(+), 167 deletions(-)
>>
>> 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.
>>
>> I'd like the Xen code to become cleaner more than anyone else here i
>> guess, but patch submission methods like this are not really
>> helpful. A far better method is to be open about such disagreements,
>> to declare them, to Cc: everyone who disagrees, and to line out the
>> arguments in the changelogs as well - instead of just curtly
>> declaring those APIs 'unused' and failing to Cc: involved parties.
>>
>> 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?
>>
>> Ben, Tony, Becky, any objections wrt. the PowerPC / IA64 impact? If
>> everyone agrees i can apply them to the IOMMU tree, test it and push
>> it out to -next, etc.
>>
>
> Ingo,
>
> With the exception of the patch I commented on, I think these look  
> OK from the powerpc point of view.  I've successfully booted one of  
> my test platforms with the entire series applied and will run some  
> more extensive (i.e. not "Whee!  A prompt!") tests tomorrow.

Well, I am still testing.  I've observed one unexpected LTP testcase  
failure with these patches applied, but so far have been unable to  
reproduce it.  So these patches are probably OK, but I will look into  
this some more next week.

-Becky

>
>
> -Becky
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@...
> https://lists.ozlabs.org/listinfo/linuxppc-dev

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@...
https://lists.ozlabs.org/listinfo/linuxppc-dev

[Benjamin Herrenschmidt]

On Mon, 2009-07-13 at 21:17 -0500, Becky Bruce wrote:
>
> phys_to_virt (also, virt_to_phys) is invalid for highmem addresses on  
> ppc.

I would be surprised if x86 was any different here. Most of our highmem
implementation comes straight from x86 :-)

Cheers,
Ben.


_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@...
https://lists.ozlabs.org/listinfo/linuxppc-dev

[Ingo Molnar]

* FUJITA Tomonori <fujita.tomonori@...> wrote:

> On Mon, 13 Jul 2009 13:20:22 +0900
> FUJITA Tomonori <fujita.tomonori@...> wrote:
>
> > On Fri, 10 Jul 2009 16:12:48 +0200
> > Ingo Molnar <mingo@...> wrote:
> >
> > > > 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. [...]
> > >
> > > +400 lines of code to avoid much fewer lines of generic code impact
> > > on the lib/swiotlb.c side sounds like a bad technical choice to me.
> >
> > The amount of code is not the point. The way to impact on the
> > lib/swiotlb.c is totally wrong from the perspective of the kernel
> > design; it uses architecture code in the very original (xen) way.
>
> btw, '+400 lines of code to avoid much fewer lines of generic code
> impact on the lib/swiotlb.c' doesn't sound true to me.
>
> Here is a patch in the way that Xen people want to do:
>
> http://patchwork.kernel.org/patch/26343/
>
> ---
>  arch/x86/Kconfig                 |    4 +
>  arch/x86/include/asm/io.h        |    2 +
>  arch/x86/include/asm/pci_x86.h   |    1 +
>  arch/x86/include/asm/xen/iommu.h |   12 ++
>  arch/x86/kernel/pci-dma.c        |    3 +
>  arch/x86/pci/Makefile            |    1 +
>  arch/x86/pci/init.c              |    6 +
>  arch/x86/pci/xen.c               |   51 +++++++
>  drivers/pci/Makefile             |    2 +
>  drivers/pci/xen-iommu.c          |  271 ++++++++++++++++++++++++++++++++++++++
>
> Even with the way that Xen people want to do,
> drivers/pci/xen-iommu.c is about 300 lines. And my patchset
> removes the nice amount of lines for dom0 support. I don't see
> much difference wrt lines.

ok, that kind of impact looks reasonable. If we are wrong and the
Xen model becomes duplicated anywhere else it can still be
generalized into core swiotlb code.

        Ingo
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@...
https://lists.ozlabs.org/listinfo/linuxppc-dev