Jeremy Fitzhardinge
2009-May-14 19:54 UTC
[Xen-devel] Where do we stand with the Xen patches?
Hi Ingo,
Over the last week or so, I''ve set out pull requests for the following
branches in git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git :
for-ingo/xen/dom0/core
You made two comments about the first post of this set:
1. The // comments in the mtrr code. Now fixed.
2. A query about when Xen can support PAT. In progress; when its
done, we can remove the unconditional PAT disable.
for-ingo/xen/dom0/pci
for-ingo/xen/dom0/swiotlb
Updated with Joerg Roedel, FUJITA Tomonori and Matthew Wilcox''s
comments, Acked-by and Reviewed-bys as appropriate.
for-ingo/xen/dom0/apic-ops
After discussion between yourself and HPA, we resolved that using
io_apic_ops was the right way to go forward with this. I replaced
for-ingo/xen/dom0/apic with the new branch
for-ingo/xen/dom0/apic-ops, which is identical aside from
implementing and using io_apic_ops.
for-ingo/xen/dom0/mtrr
You queried the value of "extending" this interface, given that
its
considered to be deprecated. These changes in no way extend the
interface, but just make the existing interface functional under
Xen. And while we don''t have PAT support, there''s no
other way of
setting cachability attributes on memory, so not supporting it has a
fairly severe performance impact on things like X.
Aside from some whitespace issues around some Impact: lines, I don''t
know of any outstanding problems. (I just pushed an updates to these
branches to fix those, and fold a change to address Jesse''s comment.)
Please tell me if you have any further issues which prevents you from
pulling these changes. Otherwise I''d appreciate it if you pulled them
soon, as we''re already on -rc5, and I have more changes I''d
like to prep
for the next merge window.
Thanks,
J
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
* Jeremy Fitzhardinge <jeremy@goop.org> wrote:> Hi Ingo, > > Over the last week or so, I''ve set out pull requests for the following > branches in git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git > : > > for-ingo/xen/dom0/core > > You made two comments about the first post of this set: > > 1. The // comments in the mtrr code. Now fixed. > 2. A query about when Xen can support PAT. In progress; when its > done, we can remove the unconditional PAT disable. > > for-ingo/xen/dom0/pci > for-ingo/xen/dom0/swiotlb > > Updated with Joerg Roedel, FUJITA Tomonori and Matthew Wilcox''s > comments, Acked-by and Reviewed-bys as appropriate.These look fine but i still need to go over them one last time before pulling them.> for-ingo/xen/dom0/apic-ops > > After discussion between yourself and HPA, we resolved that using > io_apic_ops was the right way to go forward with this. I replaced > for-ingo/xen/dom0/apic with the new branch > for-ingo/xen/dom0/apic-ops, which is identical aside from > implementing and using io_apic_ops.Yes. Here too i still need to go over them once more before pulling them.> for-ingo/xen/dom0/mtrr > > You queried the value of "extending" this interface, given that its > considered to be deprecated. These changes in no way extend the > interface, but just make the existing interface functional under > Xen. And while we don''t have PAT support, there''s no other way of > setting cachability attributes on memory, so not supporting it has a > fairly severe performance impact on things like X.Latest Xorg doesnt need /proc/mtrr. By the time this kernel (.31 or later) hits any distribution, /proc/mtrr using Xorg will be a distant memory. So i see no reason why to apply those bits at all, and i see a lot of reasons to not apply them.> Aside from some whitespace issues around some Impact: lines, I > don''t know of any outstanding problems. (I just pushed an updates > to these branches to fix those, and fold a change to address > Jesse''s comment.) > > Please tell me if you have any further issues which prevents you > from pulling these changes. Otherwise I''d appreciate it if you > pulled them soon, as we''re already on -rc5, and I have more > changes I''d like to prep for the next merge window.As in the past, my main worry is performance overhead of paravirt in general. The patches that dont affect any native kernel fast path are probably OK (but still pending final review). Regarding patches that do change the fastpath i''ll do a round of measurements of CONFIG_PARAVIRT against !CONFIG_PARAVIRT kernels, and make up my mind based on that. You could accelerate this by sending some "perf stat" hard numbers to give us an idea about where we stand today. Ingo _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2009-May-15 19:59 UTC
[Xen-devel] Re: Where do we stand with the Xen patches?
Ingo Molnar wrote:> These look fine but i still need to go over them one last time > before pulling them. > > > Yes. Here too i still need to go over them once more before pulling > them. >I''ve been posting these patches in fundamentally the same form for about 6 months now. I don''t think you''ll find anything surprising.>> for-ingo/xen/dom0/mtrr >> >> You queried the value of "extending" this interface, given that its >> considered to be deprecated. These changes in no way extend the >> interface, but just make the existing interface functional under >> Xen. And while we don''t have PAT support, there''s no other way of >> setting cachability attributes on memory, so not supporting it has a >> fairly severe performance impact on things like X. >> > > Latest Xorg doesnt need /proc/mtrr. By the time this kernel (.31 or > later) hits any distribution, /proc/mtrr using Xorg will be a > distant memory. So i see no reason why to apply those bits at all, > and i see a lot of reasons to not apply them. >In general we don''t break usermode ABIs, even when using new kernels with old distros, so I don''t see why this case is any different. Besides, these changes are not only for /proc/mtrr, but also to implement the internal mtrr_add() APIs that DRM uses to set the MTRR inside the kernel, so failing to implement them will cause performance regressions on new X servers. Given that we''re talking about 120 lines of code with no runtime impact and tiny kernel size impact (when configured), I''m at a loss for what your "lot of reasons" might be. Perhaps you could be more specific.> As in the past, my main worry is performance overhead of paravirt in > general. > > The patches that dont affect any native kernel fast path are > probably OK (but still pending final review). >These changes don''t have anything much to do with paravirt_ops, per se, and are all specifically about Xen dom0 support. Aside from that, none of the changes are on performance-critical paths.> Regarding patches that do change the fastpath i''ll do a round of > measurements of CONFIG_PARAVIRT against !CONFIG_PARAVIRT kernels, > and make up my mind based on that. > > You could accelerate this by sending some "perf stat" hard numbers > to give us an idea about where we stand today.I posted them the other day; those perf stat measurements pointing out the pv-spinlock problem also showed that paravirt_ops in general has a sub-1% performance hit (and possible performance benefit) when running mmap-perf. You added them into the commit log for the patch, so I presume you read it. J _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
FUJITA Tomonori
2009-May-18 01:36 UTC
[Xen-devel] Re: Where do we stand with the Xen patches?
On Thu, 14 May 2009 12:54:54 -0700 Jeremy Fitzhardinge <jeremy@goop.org> wrote:> Hi Ingo, > > Over the last week or so, I''ve set out pull requests for the following > branches in git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git : > > for-ingo/xen/dom0/core > > You made two comments about the first post of this set: > > 1. The // comments in the mtrr code. Now fixed. > 2. A query about when Xen can support PAT. In progress; when its > done, we can remove the unconditional PAT disable. > > for-ingo/xen/dom0/pci > for-ingo/xen/dom0/swiotlb > > Updated with Joerg Roedel, FUJITA Tomonori and Matthew Wilcox''s > comments, Acked-by and Reviewed-bys as appropriate.The original code added dom0-specific dma mapping stuff in the generic place, which is completely wrong. I asked you to move the hacky stuff to Xen-specific code and ack''ed the patchset. But as I said again and again, the dom0 changes to the generic dma mapipng code is really ugly and I don''t like them at all. I didn''t ack''ed such changes. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2009-May-18 01:42 UTC
[Xen-devel] Re: Where do we stand with the Xen patches?
FUJITA Tomonori wrote:>> for-ingo/xen/dom0/pci >> for-ingo/xen/dom0/swiotlb >> >> Updated with Joerg Roedel, FUJITA Tomonori and Matthew Wilcox''s >> comments, Acked-by and Reviewed-bys as appropriate. >> > > The original code added dom0-specific dma mapping stuff in the generic > place, which is completely wrong. I asked you to move the hacky stuff > to Xen-specific code and ack''ed the patchset. > > But as I said again and again, the dom0 changes to the generic dma > mapipng code is really ugly and I don''t like them at all. I didn''t > ack''ed such changes. >I didn''t mean to imply that you all acked all the patches. I tried to be careful to add Acked tags to the actual patches you each respectively acked. J _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
* FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:> On Thu, 14 May 2009 12:54:54 -0700 > Jeremy Fitzhardinge <jeremy@goop.org> wrote: > > > Hi Ingo, > > > > Over the last week or so, I''ve set out pull requests for the following > > branches in git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git : > > > > for-ingo/xen/dom0/core > > > > You made two comments about the first post of this set: > > > > 1. The // comments in the mtrr code. Now fixed. > > 2. A query about when Xen can support PAT. In progress; when its > > done, we can remove the unconditional PAT disable. > > > > for-ingo/xen/dom0/pci > > for-ingo/xen/dom0/swiotlb > > > > Updated with Joerg Roedel, FUJITA Tomonori and Matthew Wilcox''s > > comments, Acked-by and Reviewed-bys as appropriate. > > The original code added dom0-specific dma mapping stuff in the > generic place, which is completely wrong. I asked you to move the > hacky stuff to Xen-specific code and ack''ed the patchset. > > But as I said again and again, the dom0 changes to the generic dma > mapipng code is really ugly and I don''t like them at all. I didn''t > ack''ed such changes.How should it be solved instead? Can you see a clean way to achieve it? (maybe you already explained it in past threads - if yes then have you got subject lines or URIs to that?) Thanks, Ingo _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
FUJITA Tomonori
2009-May-19 05:27 UTC
[Xen-devel] Re: Where do we stand with the Xen patches?
On Mon, 18 May 2009 10:40:52 +0200 Ingo Molnar <mingo@elte.hu> wrote:> > * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote: > > > On Thu, 14 May 2009 12:54:54 -0700 > > Jeremy Fitzhardinge <jeremy@goop.org> wrote: > > > > > Hi Ingo, > > > > > > Over the last week or so, I''ve set out pull requests for the following > > > branches in git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git : > > > > > > for-ingo/xen/dom0/core > > > > > > You made two comments about the first post of this set: > > > > > > 1. The // comments in the mtrr code. Now fixed. > > > 2. A query about when Xen can support PAT. In progress; when its > > > done, we can remove the unconditional PAT disable. > > > > > > for-ingo/xen/dom0/pci > > > for-ingo/xen/dom0/swiotlb > > > > > > Updated with Joerg Roedel, FUJITA Tomonori and Matthew Wilcox''s > > > comments, Acked-by and Reviewed-bys as appropriate. > > > > The original code added dom0-specific dma mapping stuff in the > > generic place, which is completely wrong. I asked you to move the > > hacky stuff to Xen-specific code and ack''ed the patchset. > > > > But as I said again and again, the dom0 changes to the generic dma > > mapipng code is really ugly and I don''t like them at all. I didn''t > > ack''ed such changes. > > How should it be solved instead? Can you see a clean way to achieve > it? (maybe you already explained it in past threads - if yes then > have you got subject lines or URIs to that?)If we really need to merge dom0 support, then dom0 should have the own IOMMU implementation instead of adding any hacks to swiotlb (as I said long time ago). Yeah, there might be some duplication between swiotlb and the Xen IOMMU but IMO it''s better to have clean code with some duplication rather than ugly unified code; unification makes sense only if we do cleanly. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
* FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:> On Mon, 18 May 2009 10:40:52 +0200 > Ingo Molnar <mingo@elte.hu> wrote: > > > > > * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote: > > > > > On Thu, 14 May 2009 12:54:54 -0700 > > > Jeremy Fitzhardinge <jeremy@goop.org> wrote: > > > > > > > Hi Ingo, > > > > > > > > Over the last week or so, I''ve set out pull requests for the following > > > > branches in git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git : > > > > > > > > for-ingo/xen/dom0/core > > > > > > > > You made two comments about the first post of this set: > > > > > > > > 1. The // comments in the mtrr code. Now fixed. > > > > 2. A query about when Xen can support PAT. In progress; when its > > > > done, we can remove the unconditional PAT disable. > > > > > > > > for-ingo/xen/dom0/pci > > > > for-ingo/xen/dom0/swiotlb > > > > > > > > Updated with Joerg Roedel, FUJITA Tomonori and Matthew Wilcox''s > > > > comments, Acked-by and Reviewed-bys as appropriate. > > > > > > The original code added dom0-specific dma mapping stuff in the > > > generic place, which is completely wrong. I asked you to move the > > > hacky stuff to Xen-specific code and ack''ed the patchset. > > > > > > But as I said again and again, the dom0 changes to the generic dma > > > mapipng code is really ugly and I don''t like them at all. I didn''t > > > ack''ed such changes. > > > > How should it be solved instead? Can you see a clean way to achieve > > it? (maybe you already explained it in past threads - if yes then > > have you got subject lines or URIs to that?) > > If we really need to merge dom0 support, then dom0 should have the > own IOMMU implementation instead of adding any hacks to swiotlb > (as I said long time ago). Yeah, there might be some duplication > between swiotlb and the Xen IOMMU but IMO it''s better to have > clean code with some duplication rather than ugly unified code; > unification makes sense only if we do cleanly.Well, but since we merged PowerPC highmem support ... the precedent is there already that the swiotlb code can be librarized and generalized some more, right? What is the fundamental difference between a DMA32 device on native hardware using lib/swiotlb.c to bounce IO directed to/from a high address over buffers in a low, DMA-capable address, and between a Xen Dom0 instance using the same functionality to DMA buffers? The payback is significant: Linux drivers can be used basically as-is (as far as their DMA functionality goes) in a Xen dom0 guest. There''s really just two non-standard Xen features compared to the PowerPC-highmem use: - special allocation - non-standard virtual/physical/bus memory translations The latter is understandable - Xen cannot do the (mostly-) linear transformations that (most) platforms can do in their address transformation primitives. But other than the non-linearity, it''s a "mapping" just as much. The special allocator is arguably a bit of a hack - Xen should _probably_ recognize GFP_DMA32 allocations at the page allocator level and rearrange buffers there. So realizing that dom0 _has_ to have guest OS help here, the best possible design seems to be to minimalize the Xen cross section to Linux, while still keeping it fast enough. And that seems to be these two straightforward hooks: ''ready buffers for DMA'' and ''map addresses''. As long as we''ll want to have swiotlb code in the kernel, we''ll deal with such primitives anyway - so there doesnt seem to be a significant mainteinance drag here. Hm? Ingo _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
FUJITA Tomonori
2009-May-19 15:30 UTC
[Xen-devel] Re: Where do we stand with the Xen patches?
On Tue, 19 May 2009 15:03:00 +0200 Ingo Molnar <mingo@elte.hu> wrote:> > If we really need to merge dom0 support, then dom0 should have the > > own IOMMU implementation instead of adding any hacks to swiotlb > > (as I said long time ago). Yeah, there might be some duplication > > between swiotlb and the Xen IOMMU but IMO it''s better to have > > clean code with some duplication rather than ugly unified code; > > unification makes sense only if we do cleanly. > > Well, but since we merged PowerPC highmem support ... the precedent > is there already that the swiotlb code can be librarized and > generalized some more, right?No, The highmem support is ok for me (I was against the initial highmem patchset but Becky''s highmem patchset was much cleaner and we merged it). Due to dom0 support, we can''t use architecture abstraction (such as arch/include/asm/): http://marc.info/?l=linux-kernel&m=124271086910299&w=2 We also need hacks for memory allocation due to dom0 support.> What is the fundamental difference between a DMA32 device on native > hardware using lib/swiotlb.c to bounce IO directed to/from a high > address over buffers in a low, DMA-capable address, and between a > Xen Dom0 instance using the same functionality to DMA buffers? > > The payback is significant: Linux drivers can be used basically > as-is (as far as their DMA functionality goes) in a Xen dom0 guest. > > There''s really just two non-standard Xen features compared to the > PowerPC-highmem use: > > - special allocation > > - non-standard virtual/physical/bus memory translations > > The latter is understandable - Xen cannot do the (mostly-) linear > transformations that (most) platforms can do in their address > transformation primitives. But other than the non-linearity, it''s a > "mapping" just as much. > > The special allocator is arguably a bit of a hack - Xen should > _probably_ recognize GFP_DMA32 allocations at the page allocator > level and rearrange buffers there. > > So realizing that dom0 _has_ to have guest OS help here, the best > possible design seems to be to minimalize the Xen cross section to > Linux, while still keeping it fast enough. And that seems to be > these two straightforward hooks: ''ready buffers for DMA'' and ''map > addresses''. As long as we''ll want to have swiotlb code in the > kernel, we''ll deal with such primitives anyway - so there doesnt > seem to be a significant mainteinance drag here.We need these hooks but as I wrote above, they are architecture-specific and we should handle them with the architecture abstraction (as we handle similar problems) however we can''t due to dom0 support. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2009-May-19 15:56 UTC
[Xen-devel] Re: Where do we stand with the Xen patches?
On Wed, 2009-05-20 at 00:30 +0900, FUJITA Tomonori wrote:> > We need these hooks but as I wrote above, they are > architecture-specific and we should handle them with the architecture > abstraction (as we handle similar problems) however we can''t due to > dom0 support.I don''t understand this. What exactly about the dom0 support patch prevents future abstraction here? The dom0 hooks would simply move into the per-arch abstractions as appropriate, wouldn''t they? Ian. -- Ian Campbell Current Noise: Mondo Generator - Simple Exploding Man "We are on the verge: Today our program proved Fermat''s next-to-last theorem." -- Epigrams in Programming, ACM SIGPLAN Sept. 1982 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2009-May-20 17:06 UTC
[Xen-devel] Re: Where do we stand with the Xen patches?
Ian Campbell wrote:> On Wed, 2009-05-20 at 00:30 +0900, FUJITA Tomonori wrote: > >> We need these hooks but as I wrote above, they are >> architecture-specific and we should handle them with the architecture >> abstraction (as we handle similar problems) however we can''t due to >> dom0 support. >> > > I don''t understand this. What exactly about the dom0 support patch > prevents future abstraction here? > > The dom0 hooks would simply move into the per-arch abstractions as > appropriate, wouldn''t they?Fujita-san''s suggestion to me was that swiotlb could just use the normal (albeit deprecated) phys_to_bus()/bus_to_phys() interfaces rather than defining its own. That would be perfectly OK for Xen; we have a single global translation which is unaffected by the target device, etc. But I''m not sure it would work for powerpc; Becky''s patches which added swiotlb_bus_to_phys/phys_bus made them take a device argument, because (apparently) the bus/phys offset can differ on a per-device or per-bus basis. The powerpc side of swiotlb doesn''t seem to be in mainline yet, so I''m not sure what the details are here (maybe it can be handled with a single big runtime switch, if the offset is always constant on a given machine). (Hm, now that I look I see that they''re defined as virt_to_bus/bus_to_virt, which doesn''t work for highmem at all; it would need to be phys.) But I may have misinterpreted what he meant. J _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
FUJITA Tomonori
2009-May-21 08:54 UTC
[Xen-devel] Re: Where do we stand with the Xen patches?
On Wed, 20 May 2009 10:06:13 -0700 Jeremy Fitzhardinge <jeremy@goop.org> wrote:> Ian Campbell wrote: > > On Wed, 2009-05-20 at 00:30 +0900, FUJITA Tomonori wrote: > > > >> We need these hooks but as I wrote above, they are > >> architecture-specific and we should handle them with the architecture > >> abstraction (as we handle similar problems) however we can''t due to > >> dom0 support. > >> > > > > I don''t understand this. What exactly about the dom0 support patch > > prevents future abstraction here? > > > > The dom0 hooks would simply move into the per-arch abstractions as > > appropriate, wouldn''t they? > > Fujita-san''s suggestion to me was that swiotlb could just use the normal > (albeit deprecated) phys_to_bus()/bus_to_phys() interfaces rather than > defining its own. That would be perfectly OK for Xen; we have a single > global translation which is unaffected by the target device, etc. > > But I''m not sure it would work for powerpc; Becky''s patches which added > swiotlb_bus_to_phys/phys_bus made them take a device argument, because > (apparently) the bus/phys offset can differ on a per-device or per-bus > basis. The powerpc side of swiotlb doesn''t seem to be in mainline yet, > so I''m not sure what the details are here (maybe it can be handled with > a single big runtime switch, if the offset is always constant on a given > machine). > > (Hm, now that I look I see that they''re defined as > virt_to_bus/bus_to_virt, which doesn''t work for highmem at all; it would > need to be phys.) > > But I may have misinterpreted what he meant.What I want to remove all the __weak hacks and use the architecture abstraction. For example, the following patch is killing swiotlb_arch_address_needs_mapping() and swiotlb_arch_range_needs_mapping(). As you said, POWERPC needs a pair of conversion functions between phys and bus. I want to use arch/*/include/ for it in the same way. From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> Subject: [PATCH] make is_buffer_dma_capable architecture-specific This moves is_buffer_dma_capable() from include/linux/dma-mapping.h to arch/*/include/asm/dma-mapping.h because it''s architecture-specific; we shouldn''t have added it in the generic place. This function is used only in swiotlb (supported by x86 and IA64, and POWERPC shortly). POWERPC needs struct device to see if an address is DMA-capable or not. How POWERPC implements is_buffer_dma_capable() is still under discussion. So this patch doesn''t add POWERPC''s one. Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> --- arch/ia64/include/asm/dma-mapping.h | 6 +++++ arch/x86/include/asm/dma-mapping.h | 6 +++++ arch/x86/kernel/pci-dma.c | 2 +- arch/x86/kernel/pci-gart_64.c | 4 +- arch/x86/kernel/pci-nommu.c | 2 +- include/linux/dma-mapping.h | 5 ---- lib/swiotlb.c | 39 +++++++--------------------------- 7 files changed, 24 insertions(+), 40 deletions(-) diff --git a/arch/ia64/include/asm/dma-mapping.h b/arch/ia64/include/asm/dma-mapping.h index 36c0009..a091648 100644 --- a/arch/ia64/include/asm/dma-mapping.h +++ b/arch/ia64/include/asm/dma-mapping.h @@ -174,4 +174,10 @@ dma_cache_sync (struct device *dev, void *vaddr, size_t size, #define dma_is_consistent(d, h) (1) /* all we do is coherent memory... */ +static inline int is_buffer_dma_capable(struct device *dev, u64 mask, + dma_addr_t addr, size_t size) +{ + return addr + size <= mask; +} + #endif /* _ASM_IA64_DMA_MAPPING_H */ diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h index 916cbb6..52b8d36 100644 --- a/arch/x86/include/asm/dma-mapping.h +++ b/arch/x86/include/asm/dma-mapping.h @@ -309,4 +309,10 @@ static inline void dma_free_coherent(struct device *dev, size_t size, ops->free_coherent(dev, size, vaddr, bus); } +static inline int is_buffer_dma_capable(struct device *dev, u64 mask, + dma_addr_t addr, size_t size) +{ + return addr + size <= mask; +} + #endif diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c index 745579b..efb0bd6 100644 --- a/arch/x86/kernel/pci-dma.c +++ b/arch/x86/kernel/pci-dma.c @@ -145,7 +145,7 @@ again: return NULL; addr = page_to_phys(page); - if (!is_buffer_dma_capable(dma_mask, addr, size)) { + if (!is_buffer_dma_capable(dev, dma_mask, addr, size)) { __free_pages(page, get_order(size)); if (dma_mask < DMA_BIT_MASK(32) && !(flag & GFP_DMA)) { diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c index 1e8920d..13f5265 100644 --- a/arch/x86/kernel/pci-gart_64.c +++ b/arch/x86/kernel/pci-gart_64.c @@ -191,13 +191,13 @@ static inline int need_iommu(struct device *dev, unsigned long addr, size_t size) { return force_iommu || - !is_buffer_dma_capable(*dev->dma_mask, addr, size); + !is_buffer_dma_capable(dev, *dev->dma_mask, addr, size); } static inline int nonforced_iommu(struct device *dev, unsigned long addr, size_t size) { - return !is_buffer_dma_capable(*dev->dma_mask, addr, size); + return !is_buffer_dma_capable(dev, *dev->dma_mask, addr, size); } /* Map a single continuous physical area into the IOMMU. diff --git a/arch/x86/kernel/pci-nommu.c b/arch/x86/kernel/pci-nommu.c index 71d412a..df930f3 100644 --- a/arch/x86/kernel/pci-nommu.c +++ b/arch/x86/kernel/pci-nommu.c @@ -14,7 +14,7 @@ static int check_addr(char *name, struct device *hwdev, dma_addr_t bus, size_t size) { - if (hwdev && !is_buffer_dma_capable(*hwdev->dma_mask, bus, size)) { + if (hwdev && !is_buffer_dma_capable(hwdev, *hwdev->dma_mask, bus, size)) { if (*hwdev->dma_mask >= DMA_BIT_MASK(32)) printk(KERN_ERR "nommu_%s: overflow %Lx+%zu of device mask %Lx\n", diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index 8083b6a..85dafa1 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -96,11 +96,6 @@ static inline int is_device_dma_capable(struct device *dev) return dev->dma_mask != NULL && *dev->dma_mask != DMA_MASK_NONE; } -static inline int is_buffer_dma_capable(u64 mask, dma_addr_t addr, size_t size) -{ - return addr + size <= mask; -} - #ifdef CONFIG_HAS_DMA #include <asm/dma-mapping.h> #else diff --git a/lib/swiotlb.c b/lib/swiotlb.c index bffe6d7..01c5775 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -145,17 +145,6 @@ 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) -{ - return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size); -} - -int __weak swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size) -{ - return 0; -} - static void swiotlb_print_info(unsigned long bytes) { phys_addr_t pstart, pend; @@ -315,17 +304,6 @@ cleanup1: return -ENOMEM; } -static inline int -address_needs_mapping(struct device *hwdev, dma_addr_t addr, size_t size) -{ - return swiotlb_arch_address_needs_mapping(hwdev, addr, size); -} - -static inline int range_needs_mapping(phys_addr_t paddr, size_t size) -{ - return swiotlb_force || swiotlb_arch_range_needs_mapping(paddr, size); -} - static int is_swiotlb_buffer(char *addr) { return addr >= io_tlb_start && addr < io_tlb_end; @@ -561,9 +539,8 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size, dma_mask = hwdev->coherent_dma_mask; ret = (void *)__get_free_pages(flags, order); - if (ret && - !is_buffer_dma_capable(dma_mask, swiotlb_virt_to_bus(hwdev, ret), - size)) { + if (ret && !is_buffer_dma_capable(hwdev, dma_mask, + swiotlb_virt_to_bus(hwdev, ret), size)) { /* * The allocated memory isn''t reachable by the device. */ @@ -585,7 +562,7 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size, dev_addr = swiotlb_virt_to_bus(hwdev, ret); /* Confirm address can be DMA''d by device */ - if (!is_buffer_dma_capable(dma_mask, dev_addr, size)) { + if (!is_buffer_dma_capable(hwdev, dma_mask, dev_addr, size)) { printk("hwdev DMA mask = 0x%016Lx, dev_addr = 0x%016Lx\n", (unsigned long long)dma_mask, (unsigned long long)dev_addr); @@ -655,8 +632,8 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, * we can safely return the device addr and not worry about bounce * buffering it. */ - if (!address_needs_mapping(dev, dev_addr, size) && - !range_needs_mapping(phys, size)) + if (is_buffer_dma_capable(dev, dma_get_mask(dev), dev_addr, size) && + !swiotlb_force) return dev_addr; /* @@ -673,7 +650,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, /* * Ensure that the address returned is DMA''ble */ - if (address_needs_mapping(dev, dev_addr, size)) + if (!is_buffer_dma_capable(dev, dma_get_mask(dev), dev_addr, size)) panic("map_single: bounce buffer is not DMA''ble"); return dev_addr; @@ -819,8 +796,8 @@ swiotlb_map_sg_attrs(struct device *hwdev, struct scatterlist *sgl, int nelems, phys_addr_t paddr = sg_phys(sg); dma_addr_t dev_addr = swiotlb_phys_to_bus(hwdev, paddr); - if (range_needs_mapping(paddr, sg->length) || - address_needs_mapping(hwdev, dev_addr, sg->length)) { + if (!is_buffer_dma_capable(hwdev, dma_get_mask(hwdev), paddr, sg->length) || + swiotlb_force) { void *map = map_single(hwdev, sg_phys(sg), sg->length, dir); if (!map) { -- 1.6.0.6 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2009-May-21 10:27 UTC
[Xen-devel] Re: Where do we stand with the Xen patches?
On Thu, 2009-05-21 at 17:54 +0900, FUJITA Tomonori wrote:> > What I want to remove all the __weak hacks and use the architecture > abstraction. For example, the following patch is killing > swiotlb_arch_address_needs_mapping() and > swiotlb_arch_range_needs_mapping().I think the swiotlb_arch_address_needs_mapping()/is_buffer_dma_capable() aspects of this are fine from the Xen POV but a hook along the lines of swiotlb_arch_range_needs_mapping() is unfortunately still required to handle potential discontigousness in multipage buffers. There''s no reason this can''t be handled in a similar way though. e.g. the following updated patch is based on yours but also moves the swiotlb_range_needs mapping hook to dma-mapping.h. The corresponding the Xen updates will. Subject: swiotlb: is_buffer_dma_capable and swiotlb_range_needs_mapping are arch-specific This moves is_buffer_dma_capable() from include/linux/dma-mapping.h to arch/*/include/asm/dma-mapping.h because it''s architecture-specific; we shouldn''t have added it in the generic place. This function is used only in swiotlb (supported by x86 and IA64, and POWERPC shortly). POWERPC needs struct device to see if an address is DMA-capable or not. How POWERPC implements is_buffer_dma_capable() is still under discussion. So this patch doesn''t add POWERPC''s one. The range_needs_mapping hook is needed by Xen PCI to support multipage buffers which are potentially discontiguous in the DMA address space. Based on original patch by FUJITA Tomonori. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> diff --git a/arch/ia64/include/asm/dma-mapping.h b/arch/ia64/include/asm/dma-mapping.h index 36c0009..cc25a4a 100644 --- a/arch/ia64/include/asm/dma-mapping.h +++ b/arch/ia64/include/asm/dma-mapping.h @@ -174,4 +174,15 @@ dma_cache_sync (struct device *dev, void *vaddr, size_t size, #define dma_is_consistent(d, h) (1) /* all we do is coherent memory... */ +static inline int is_buffer_dma_capable(struct device *dev, u64 mask, + dma_addr_t addr, size_t size) +{ + return addr + size <= mask; +} + +static int inline swiotlb_range_needs_mapping(phys_addr_t paddr, size_t size) +{ + return 0; +} + #endif /* _ASM_IA64_DMA_MAPPING_H */ diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h index 916cbb6..a80139a 100644 --- a/arch/x86/include/asm/dma-mapping.h +++ b/arch/x86/include/asm/dma-mapping.h @@ -309,4 +309,15 @@ static inline void dma_free_coherent(struct device *dev, size_t size, ops->free_coherent(dev, size, vaddr, bus); } +static inline int is_buffer_dma_capable(struct device *dev, u64 mask, + dma_addr_t addr, size_t size) +{ + return addr + size <= mask; +} + +static int inline swiotlb_range_needs_mapping(phys_addr_t paddr, size_t size) +{ + return 0; +} + #endif diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c index 2fffc22..587cc6a 100644 --- a/arch/x86/kernel/pci-dma.c +++ b/arch/x86/kernel/pci-dma.c @@ -146,7 +146,7 @@ again: return NULL; addr = page_to_phys(page); - if (!is_buffer_dma_capable(dma_mask, addr, size)) { + if (!is_buffer_dma_capable(dev, dma_mask, addr, size)) { __free_pages(page, get_order(size)); if (dma_mask < DMA_BIT_MASK(32) && !(flag & GFP_DMA)) { diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c index 1e8920d..13f5265 100644 --- a/arch/x86/kernel/pci-gart_64.c +++ b/arch/x86/kernel/pci-gart_64.c @@ -191,13 +191,13 @@ static inline int need_iommu(struct device *dev, unsigned long addr, size_t size) { return force_iommu || - !is_buffer_dma_capable(*dev->dma_mask, addr, size); + !is_buffer_dma_capable(dev, *dev->dma_mask, addr, size); } static inline int nonforced_iommu(struct device *dev, unsigned long addr, size_t size) { - return !is_buffer_dma_capable(*dev->dma_mask, addr, size); + return !is_buffer_dma_capable(dev, *dev->dma_mask, addr, size); } /* Map a single continuous physical area into the IOMMU. diff --git a/arch/x86/kernel/pci-nommu.c b/arch/x86/kernel/pci-nommu.c index 71d412a..df930f3 100644 --- a/arch/x86/kernel/pci-nommu.c +++ b/arch/x86/kernel/pci-nommu.c @@ -14,7 +14,7 @@ static int check_addr(char *name, struct device *hwdev, dma_addr_t bus, size_t size) { - if (hwdev && !is_buffer_dma_capable(*hwdev->dma_mask, bus, size)) { + if (hwdev && !is_buffer_dma_capable(hwdev, *hwdev->dma_mask, bus, size)) { if (*hwdev->dma_mask >= DMA_BIT_MASK(32)) printk(KERN_ERR "nommu_%s: overflow %Lx+%zu of device mask %Lx\n", diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index 8083b6a..85dafa1 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -96,11 +96,6 @@ static inline int is_device_dma_capable(struct device *dev) return dev->dma_mask != NULL && *dev->dma_mask != DMA_MASK_NONE; } -static inline int is_buffer_dma_capable(u64 mask, dma_addr_t addr, size_t size) -{ - return addr + size <= mask; -} - #ifdef CONFIG_HAS_DMA #include <asm/dma-mapping.h> #else diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h index cb1a663..32f4fa4 100644 --- a/include/linux/swiotlb.h +++ b/include/linux/swiotlb.h @@ -32,8 +32,6 @@ extern dma_addr_t swiotlb_phys_to_bus(struct device *hwdev, extern phys_addr_t swiotlb_bus_to_phys(struct device *hwdev, dma_addr_t address); -extern int swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size); - extern void *swiotlb_alloc_coherent(struct device *hwdev, size_t size, dma_addr_t *dma_handle, gfp_t flags); diff --git a/lib/swiotlb.c b/lib/swiotlb.c index cec5f62..bde376c 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -147,17 +147,6 @@ 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) -{ - return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size); -} - -int __weak swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size) -{ - return 0; -} - static void swiotlb_print_info(unsigned long bytes) { phys_addr_t pstart, pend; @@ -318,15 +307,9 @@ cleanup1: return -ENOMEM; } -static inline int -address_needs_mapping(struct device *hwdev, dma_addr_t addr, size_t size) -{ - return swiotlb_arch_address_needs_mapping(hwdev, addr, size); -} - static inline int range_needs_mapping(phys_addr_t paddr, size_t size) { - return swiotlb_force || swiotlb_arch_range_needs_mapping(paddr, size); + return swiotlb_force || swiotlb_range_needs_mapping(paddr, size); } static int is_swiotlb_buffer(char *addr) @@ -564,9 +547,8 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size, dma_mask = hwdev->coherent_dma_mask; ret = (void *)__get_free_pages(flags, order); - if (ret && - !is_buffer_dma_capable(dma_mask, swiotlb_virt_to_bus(hwdev, ret), - size)) { + if (ret && !is_buffer_dma_capable(hwdev, dma_mask, + swiotlb_virt_to_bus(hwdev, ret), size)) { /* * The allocated memory isn''t reachable by the device. */ @@ -588,7 +570,7 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size, dev_addr = swiotlb_virt_to_bus(hwdev, ret); /* Confirm address can be DMA''d by device */ - if (!is_buffer_dma_capable(dma_mask, dev_addr, size)) { + if (!is_buffer_dma_capable(hwdev, dma_mask, dev_addr, size)) { printk("hwdev DMA mask = 0x%016Lx, dev_addr = 0x%016Lx\n", (unsigned long long)dma_mask, (unsigned long long)dev_addr); @@ -658,7 +640,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, * we can safely return the device addr and not worry about bounce * buffering it. */ - if (!address_needs_mapping(dev, dev_addr, size) && + if (is_buffer_dma_capable(dev, dma_get_mask(dev), dev_addr, size) && !range_needs_mapping(phys, size)) return dev_addr; @@ -676,7 +658,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page, /* * Ensure that the address returned is DMA''ble */ - if (address_needs_mapping(dev, dev_addr, size)) + if (!is_buffer_dma_capable(dev, dma_get_mask(dev), dev_addr, size)) panic("map_single: bounce buffer is not DMA''ble"); return dev_addr; @@ -823,7 +805,7 @@ swiotlb_map_sg_attrs(struct device *hwdev, struct scatterlist *sgl, int nelems, dma_addr_t dev_addr = swiotlb_phys_to_bus(hwdev, paddr); if (range_needs_mapping(paddr, sg->length) || - address_needs_mapping(hwdev, dev_addr, sg->length)) { + !is_buffer_dma_capable(hwdev, dma_get_mask(hwdev), dev_addr, sg->length)) { void *map = map_single(hwdev, sg_phys(sg), sg->length, dir); if (!map) { -- Ian Campbell Current Noise: Dark Fortress - Edge Of Night Yow! Is this sexual intercourse yet?? Is it, huh, is it?? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2009-May-21 10:28 UTC
[Xen-devel] Re: Where do we stand with the Xen patches?
On Thu, 2009-05-21 at 11:27 +0100, Ian Campbell wrote:> The corresponding the Xen updates will......follow. Subject:swiotlb/xen: add implementation of swiotlb_range_needs_mapping hook This function is now implemented via asm/dma-mapping.h rather than as a weak hook in swiotlb.c Signed-off-by: Ian Campbell <ian.campbell@citrix.com> diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h index a80139a..ed51bd1 100644 --- a/arch/x86/include/asm/dma-mapping.h +++ b/arch/x86/include/asm/dma-mapping.h @@ -315,9 +315,15 @@ static inline int is_buffer_dma_capable(struct device *dev, u64 mask, return addr + size <= mask; } +#ifdef CONFIG_PCI_XEN +extern int xen_range_needs_mapping(phys_addr_t paddr, size_t size); +#else +static inline int xen_range_needs_mapping(phys_addr_t paddr, size_t size) { return 0; } +#endif + static int inline swiotlb_range_needs_mapping(phys_addr_t paddr, size_t size) { - return 0; + return xen_range_needs_mapping(paddr, size); } #endif diff --git a/arch/x86/xen/pci-swiotlb.c b/arch/x86/xen/pci-swiotlb.c index 5a46418..e23b00b 100644 --- a/arch/x86/xen/pci-swiotlb.c +++ b/arch/x86/xen/pci-swiotlb.c @@ -43,11 +43,3 @@ phys_addr_t swiotlb_bus_to_phys(struct device *hwdev, dma_addr_t baddr) return baddr; } - -int swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size) -{ - if (xen_pv_domain()) - return xen_range_needs_mapping(paddr, size); - - return 0; -} diff --git a/drivers/pci/xen-iommu.c b/drivers/pci/xen-iommu.c index 41c276f..003ffe3 100644 --- a/drivers/pci/xen-iommu.c +++ b/drivers/pci/xen-iommu.c @@ -132,6 +132,8 @@ static int range_straddles_page_boundary(phys_addr_t p, size_t size) int xen_range_needs_mapping(phys_addr_t paddr, size_t size) { + if (!xen_pv_domain()) + return 0; return range_straddles_page_boundary(paddr, size); } -- Ian Campbell Current Noise: Isis - Wills Dissolve "Hey Ivan, check your six." -- Sidewinder missile jacket patch, showing a Sidewinder driving up the tail of a Russian Su-27 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
FUJITA Tomonori
2009-May-21 10:39 UTC
[Xen-devel] Re: Where do we stand with the Xen patches?
On Thu, 21 May 2009 11:28:53 +0100 Ian Campbell <ijc@hellion.org.uk> wrote:> On Thu, 2009-05-21 at 11:27 +0100, Ian Campbell wrote: > > The corresponding the Xen updates will... > ...follow. > > Subject:swiotlb/xen: add implementation of swiotlb_range_needs_mapping hook > > This function is now implemented via asm/dma-mapping.h rather than as > a weak hook in swiotlb.c > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h > index a80139a..ed51bd1 100644 > --- a/arch/x86/include/asm/dma-mapping.h > +++ b/arch/x86/include/asm/dma-mapping.h > @@ -315,9 +315,15 @@ static inline int is_buffer_dma_capable(struct device *dev, u64 mask, > return addr + size <= mask; > } > > +#ifdef CONFIG_PCI_XEN > +extern int xen_range_needs_mapping(phys_addr_t paddr, size_t size); > +#else > +static inline int xen_range_needs_mapping(phys_addr_t paddr, size_t size) { return 0; } > +#endifI know Xen can do something like this but you think that this is clean? In addition, you also the similar hack in arch/ia64/include/asm/dma-mapping.h for ia64''s dom0 support, I think. IMO, your patch just moves the ugly hacks from lib/swiotlb.c to arch/{x86|ia64}/include/asm/dma-mapping.h. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2009-May-21 10:48 UTC
[Xen-devel] Re: Where do we stand with the Xen patches?
On Thu, 2009-05-21 at 17:54 +0900, FUJITA Tomonori wrote:> @@ -819,8 +796,8 @@ swiotlb_map_sg_attrs(struct device *hwdev, struct scatterlist *sgl, int nelems, > phys_addr_t paddr = sg_phys(sg); > dma_addr_t dev_addr = swiotlb_phys_to_bus(hwdev, paddr); > > - if (range_needs_mapping(paddr, sg->length) || > - address_needs_mapping(hwdev, dev_addr, sg->length)) { > + if (!is_buffer_dma_capable(hwdev, dma_get_mask(hwdev), paddr, sg->length) || > + swiotlb_force) { > void *map = map_single(hwdev, sg_phys(sg), > sg->length, dir); > if (!map) {BTW I think there was a typo here, dev_addr becomes paddr. I fixed that in my updated version. Ian. -- Ian Campbell Current Noise: Isis - In Fiction Poorochrondria: Hypochrondria derived from not having medical insurance. -- Douglas Coupland, "Generation X: Tales for an Accelerated Culture" _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2009-May-21 11:03 UTC
Re: [Xen-devel] Re: Where do we stand with the Xen patches?
On Thu, 2009-05-21 at 06:39 -0400, FUJITA Tomonori wrote:> On Thu, 21 May 2009 11:28:53 +0100 > Ian Campbell <ijc@hellion.org.uk> wrote: > > > +#ifdef CONFIG_PCI_XEN > > +extern int xen_range_needs_mapping(phys_addr_t paddr, size_t size); > > +#else > > +static inline int xen_range_needs_mapping(phys_addr_t paddr, size_t size) { return 0; } > > +#endif > > I know Xen can do something like this but you think that this is > clean?Well, defining a static inline function when a CONFIG option is disabled is fairly idiomatic in the kernel and in general hiding these sorts of things in the headers in this way is preferred to having them in .c files. See e.g. the handling of CONFIG_PRINTK in include/linux/kernel.h or CONFIG_HIGHMEM in include/linux/highmem.h for just two examples out of many.> In addition, you also the similar hack in > arch/ia64/include/asm/dma-mapping.h for ia64''s dom0 support, I think. > > IMO, your patch just moves the ugly hacks from lib/swiotlb.c to > arch/{x86|ia64}/include/asm/dma-mapping.h.I nearly suggested that for this hook it might actually be preferable to put the one line Xen hook directly into swiotlb.c. I didn''t think this suggestion would go down very well though. In any case something along these lines needs to go somewhere. I think you are slightly mischaracterising this as an "ugly hack" -- it is a necessary interface to enable a particular use-case, and it actually has a very small cross section (it''s basically five or six lines of code). If there was a cleaner way to achieve the same result we would of course go with that. I don''t think duplicating swiotlb.c, as has been suggested as the alternative, just for that one hook point makes sense. Ian. -- Ian Campbell Current Noise: Isis - Altered Course "For a male and female to live continuously together is... biologically speaking, an extremely unnatural condition." -- Robert Briffault _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2009-May-21 11:08 UTC
Re: [Xen-devel] Re: Where do we stand with the Xen patches?
On Thu, 2009-05-21 at 07:03 -0400, Ian Campbell wrote:> On Thu, 2009-05-21 at 06:39 -0400, FUJITA Tomonori wrote: > > On Thu, 21 May 2009 11:28:53 +0100 > > Ian Campbell <ijc@hellion.org.uk> wrote: > > > > > +#ifdef CONFIG_PCI_XEN > > > +extern int xen_range_needs_mapping(phys_addr_t paddr, size_t > size); > > > +#else > > > +static inline int xen_range_needs_mapping(phys_addr_t paddr, > size_t size) { return 0; } > > > +#endif > > > > I know Xen can do something like this but you think that this is > > clean? > > Well, defining a static inline function when a CONFIG option is > disabled is fairly idiomatic in the kernel and in general hiding these > sorts of things in the headers in this way is preferred to having them > in .c files.Although I do concede that the function definition would probably be better placed in a xen specific header. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
FUJITA Tomonori
2009-May-21 11:19 UTC
Re: [Xen-devel] Re: Where do we stand with the Xen patches?
On Thu, 21 May 2009 12:03:05 +0100 Ian Campbell <ijc@hellion.org.uk> wrote:> On Thu, 2009-05-21 at 06:39 -0400, FUJITA Tomonori wrote: > > On Thu, 21 May 2009 11:28:53 +0100 > > Ian Campbell <ijc@hellion.org.uk> wrote: > > > > > +#ifdef CONFIG_PCI_XEN > > > +extern int xen_range_needs_mapping(phys_addr_t paddr, size_t size); > > > +#else > > > +static inline int xen_range_needs_mapping(phys_addr_t paddr, size_t size) { return 0; } > > > +#endif > > > > I know Xen can do something like this but you think that this is > > clean? > > Well, defining a static inline function when a CONFIG option is disabled > is fairly idiomatic in the kernel and in general hiding these sorts of > things in the headers in this way is preferred to having them in .c > files. See e.g. the handling of CONFIG_PRINTK in include/linux/kernel.h > or CONFIG_HIGHMEM in include/linux/highmem.h for just two examples out > of many.Well, I know that it''s idiomatic, but placing CONFIG_PCI_XEN in arch/{x86|ia64}/include/asm/ is a wrong abstraction to me.> > In addition, you also the similar hack in > > arch/ia64/include/asm/dma-mapping.h for ia64''s dom0 support, I think. > > > > IMO, your patch just moves the ugly hacks from lib/swiotlb.c to > > arch/{x86|ia64}/include/asm/dma-mapping.h. > > I nearly suggested that for this hook it might actually be preferable to > put the one line Xen hook directly into swiotlb.c. I didn''t think this > suggestion would go down very well though.Any arch or Xen specific code should not live in swiotlb.c> In any case something along these lines needs to go somewhere. I think > you are slightly mischaracterising this as an "ugly hack" -- it is a > necessary interface to enable a particular use-case, and it actually has > a very small cross section (it''s basically five or six lines of code).A necessary interface? Sorry, I don''t buy it. It''s necessary for only Xen. And it''s not fit well for swiotlb and the architecture abstraction.> If there was a cleaner way to achieve the same result we would of course > go with that. I don''t think duplicating swiotlb.c, as has been suggested > as the alternative, just for that one hook point makes sense.One hook? You need to reread swiotlb.c. There are more. All should go. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2009-May-21 11:45 UTC
Re: [Xen-devel] Re: Where do we stand with the Xen patches?
On Thu, 2009-05-21 at 20:19 +0900, FUJITA Tomonori wrote:> On Thu, 21 May 2009 12:03:05 +0100 > Ian Campbell <ijc@hellion.org.uk> wrote: > > > On Thu, 2009-05-21 at 06:39 -0400, FUJITA Tomonori wrote: > > > On Thu, 21 May 2009 11:28:53 +0100 > > > Ian Campbell <ijc@hellion.org.uk> wrote: > > > > > > > +#ifdef CONFIG_PCI_XEN > > > > +extern int xen_range_needs_mapping(phys_addr_t paddr, size_t size); > > > > +#else > > > > +static inline int xen_range_needs_mapping(phys_addr_t paddr, size_t size) { return 0; } > > > > +#endif > > > > > > I know Xen can do something like this but you think that this is > > > clean? > > > > Well, defining a static inline function when a CONFIG option is disabled > > is fairly idiomatic in the kernel and in general hiding these sorts of > > things in the headers in this way is preferred to having them in .c > > files. See e.g. the handling of CONFIG_PRINTK in include/linux/kernel.h > > or CONFIG_HIGHMEM in include/linux/highmem.h for just two examples out > > of many. > > Well, I know that it''s idiomatic, but placing CONFIG_PCI_XEN in > arch/{x86|ia64}/include/asm/ is a wrong abstraction to me.Agreed, include/xen/swiotlb.h is the right place for it.> > > In addition, you also the similar hack in > > > arch/ia64/include/asm/dma-mapping.h for ia64''s dom0 support, I think. > > > > > > IMO, your patch just moves the ugly hacks from lib/swiotlb.c to > > > arch/{x86|ia64}/include/asm/dma-mapping.h. > > > > I nearly suggested that for this hook it might actually be preferable to > > put the one line Xen hook directly into swiotlb.c. I didn''t think this > > suggestion would go down very well though. > > Any arch or Xen specific code should not live in swiotlb.cAgain agreed, which is why I didn''t suggest it ;-)> > In any case something along these lines needs to go somewhere. I think > > you are slightly mischaracterising this as an "ugly hack" -- it is a > > necessary interface to enable a particular use-case, and it actually has > > a very small cross section (it''s basically five or six lines of code). >> > A necessary interface? Sorry, I don''t buy it. It''s necessary for > only Xen. And it''s not fit well for swiotlb and the architecture > abstraction.I said "necessary interface to enable a particular use-case". Xen is a valid use case -- I appreciate that you personally may have no interest in it but plenty of people do and plenty of people would like to see it working in the mainline kernels. In terms of clean abstraction this is a architecture hook to allow mappings to be forced through the swiotlb. It is essentially a more flexible extension to the existing swiotlb_force flag, in fact swiotlb_force_mapping() might even be a better name for it. IMHO, the benefits here (working Xen domain 0 plus PCI passthrough to guest domains) are well worth the costs.> > If there was a cleaner way to achieve the same result we would of course > > go with that. I don''t think duplicating swiotlb.c, as has been suggested > > as the alternative, just for that one hook point makes sense. > > One hook? You need to reread swiotlb.c. There are more. All should go.I meant that swiotlb_range_needs_mapping is the single contentious hook as far as I can tell. It is the only one which you appear to be disputing the very existence of (irrespective of whether it uses __weak or is moved into asm/dma-mapping.h). The other existing __weak hooks are useful to other users too and all can, AFAICT, be moved into asm/dma-mapping.h. You have already dealt with moving swiotlb_address_needs_mapping and I am currently looking into the the phys_to_bus and bus_to_phys ones. That just leaves the alloc functions, which are next on my list after phys_to_bus and bus_to_phys. Will finishing up those patches resolve most of your objections are do you have others? Ian. -- Ian Campbell This supersedes all previous notices. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2009-May-21 16:19 UTC
[Xen-devel] Re: swiotlb: remove __weak hooks in favour of architecture-specific functions
On Thu, 2009-05-21 at 12:15 -0400, Ian Campbell wrote:> At the end of this series there are no more __weak functions in > lib/swiotlb.cHmm, I was expecting the patches to be numbered in the subject by default. The correct order is: swiotlb: make is_buffer_dma_capable architecture-specific swiotlb: make range_needs_mapping architecture-specific swiotlb/xen: update xen for swiotlb_arch_force_mapping changes swiotlb: make swiotlb allocation functions architecture-specific swiotlb/xen: update xen for changes to swiotlb allocation interface swiotlb: make swiotlb phys<->bus translations architecture-specific swiotlb/xen: update xen swiotlb for phys<->bus API changes xen: remove arch/x86/xen/pci-swiotlb.c Also xen-devel missed out due to a typo, sorry. See the LKML archives if you are interested. Ian.> > The series adds several hook functions to the x86 architecture. Would > they be preferred as a struct x86_swiotlb_ops or as individual hooks? > > I was unsure what to do about powerpc in most places since the > existing support seems to in-progress so it wasn''t always clear where > to put the implementation. If there is a tree somewhere with more > complete support I''ll be happy to provide additional patches. > > Boot tested on x86 under xen but not even compiled for ia64 or > powerpc. If someone can point me to a decent source of cross compilers > I can sort that out. (http://www.kernel.org/pub/tools/crosstool/ seems > to be out-of-date and only has ia64 in any case) > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> > Cc: Jeremy Fitzhardinge <jeremy@goop.org> > Cc: Becky Bruce <beckyb@kernel.crashing.org> > Cc: Olaf Kirch <okir@suse.de> > Cc: Ingo Molnar <mingo@elte.hu> > Cc: Greg KH <gregkh@suse.de> > Cc: xen-devel <xendevel@lists.xensource.com> > Cc: x86 maintainers <x86@kernel.org> > Cc: lkml <linux-kernel@vger.kernel.org> > > --- > arch/ia64/include/asm/dma-mapping.h | 29 ++++++++++++++ > arch/ia64/kernel/pci-swiotlb.c | 11 +++++ > arch/powerpc/include/asm/dma-mapping.h | 5 ++ > arch/x86/include/asm/agp.h | 4 +- > arch/x86/include/asm/dma-mapping.h | 34 +++++++++++++++++ > arch/x86/include/asm/xen/iommu.h | 4 ++ > arch/x86/kernel/pci-dma.c | 6 ++- > arch/x86/kernel/pci-gart_64.c | 4 +- > arch/x86/kernel/pci-nommu.c | 3 +- > arch/x86/kernel/pci-swiotlb.c | 32 ++++++++++++++++ > arch/x86/xen/Makefile | 3 +- > arch/x86/xen/pci-swiotlb.c | 53 -------------------------- > drivers/pci/xen-iommu.c | 20 ++++++++-- > include/linux/dma-mapping.h | 5 -- > include/linux/swiotlb.h | 10 ----- > include/xen/swiotlb.h | 5 -- > lib/swiotlb.c | 64 +++++++------------------------- > 17 files changed, 156 insertions(+), 136 deletions(-)_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
FUJITA Tomonori
2009-May-21 17:21 UTC
Re: [Xen-devel] Re: Where do we stand with the Xen patches?
On Thu, 21 May 2009 12:45:35 +0100 Ian Campbell <ijc@hellion.org.uk> wrote:> > A necessary interface? Sorry, I don''t buy it. It''s necessary for > > only Xen. And it''s not fit well for swiotlb and the architecture > > abstraction. > > I said "necessary interface to enable a particular use-case". Xen is a > valid use case -- I appreciate that you personally may have no interest > in it but plenty of people do and plenty of people would like to see it > working in the mainline kernels. > > In terms of clean abstraction this is a architecture hook to allow > mappings to be forced through the swiotlb. It is essentially a more > flexible extension to the existing swiotlb_force flag, in fact > swiotlb_force_mapping() might even be a better name for it. > > IMHO, the benefits here (working Xen domain 0 plus PCI passthrough to > guest domains) are well worth the costs.All I care about is a clean design; an wrong design will hurt us.> > > If there was a cleaner way to achieve the same result we would of course > > > go with that. I don''t think duplicating swiotlb.c, as has been suggested > > > as the alternative, just for that one hook point makes sense. > > > > One hook? You need to reread swiotlb.c. There are more. All should go. > > I meant that swiotlb_range_needs_mapping is the single contentious hook > as far as I can tell. It is the only one which you appear to be > disputing the very existence of (irrespective of whether it uses __weak > or is moved into asm/dma-mapping.h). The other existing __weak hooks are > useful to other users too and all can, AFAICT, be moved into > asm/dma-mapping.h. > > You have already dealt with moving swiotlb_address_needs_mapping and I > am currently looking into the the phys_to_bus and bus_to_phys ones. That > just leaves the alloc functions, which are next on my list after > phys_to_bus and bus_to_phys. Will finishing up those patches resolve > most of your objections are do you have others?The latest your patch is more hacky than the current code. You just move the ugly hacks for dom0 from lib/swiotlb.c to somewhere else. I guess that the only way to keep the Xen-specific stuff in Xen''s land is something like this. Then xen/pci-swiotlb.c implements its own map/unmap functions without messing up the generic code. I guess that do_map_single''s io_tlb_daddr argument is pointless for Xen since swiotlb doesn''t work if iotlb buffer is not physically continuous (you will see data corruption with some device drivers). diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h index cb1a663..e1c40ae 100644 --- a/include/linux/swiotlb.h +++ b/include/linux/swiotlb.h @@ -21,8 +21,17 @@ struct scatterlist; */ #define IO_TLB_SHIFT 11 -extern void -swiotlb_init(void); +extern void swiotlb_init(void); +extern void swiotlb_register_io_tlb(void *); + +extern void *do_map_single(struct device *hwdev, dma_addr_t io_tlb_daddr, + phys_addr_t phys, size_t size, + enum dma_data_direction dir); +extern void do_unmap_single(struct device *hwdev, char *dma_addr, size_t size, + enum dma_data_direction dir); + +extern void sync_single(struct device *hwdev, char *dma_addr, size_t size, + int dir, int target); extern void *swiotlb_alloc_boot(size_t bytes, unsigned long nslabs); extern void *swiotlb_alloc(unsigned order, unsigned long nslabs); diff --git a/lib/swiotlb.c b/lib/swiotlb.c index cec5f62..6efa8cc 100644 --- a/lib/swiotlb.c +++ b/lib/swiotlb.c @@ -176,24 +176,16 @@ static void swiotlb_print_info(unsigned long bytes) * Statically reserve bounce buffer space and initialize bounce buffer data * structures for the software IO TLB used to implement the DMA API. */ -void __init -swiotlb_init_with_default_size(size_t default_size) +void __init swiotlb_register_iotlb(void *iotlb) { unsigned long i, bytes; - if (!io_tlb_nslabs) { - io_tlb_nslabs = (default_size >> IO_TLB_SHIFT); - io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE); - } - bytes = io_tlb_nslabs << IO_TLB_SHIFT; /* * Get IO TLB memory from the low pages */ - io_tlb_start = swiotlb_alloc_boot(bytes, io_tlb_nslabs); - if (!io_tlb_start) - panic("Cannot allocate SWIOTLB buffer"); + io_tlb_start = iotlb; io_tlb_end = io_tlb_start + bytes; /* @@ -207,21 +199,30 @@ swiotlb_init_with_default_size(size_t default_size) io_tlb_index = 0; io_tlb_orig_addr = alloc_bootmem(io_tlb_nslabs * sizeof(phys_addr_t)); - /* - * Get the overflow emergency buffer - */ - io_tlb_overflow_buffer = swiotlb_alloc_boot(io_tlb_overflow, - io_tlb_overflow >> IO_TLB_SHIFT); - if (!io_tlb_overflow_buffer) - panic("Cannot allocate SWIOTLB overflow buffer!\n"); - swiotlb_print_info(bytes); } void __init swiotlb_init(void) { - swiotlb_init_with_default_size(64 * (1<<20)); /* default to 64MB */ + size_t default_size = 64 * (1 << 20); /* default to 64MB */ + void *iotlb; + + if (!io_tlb_nslabs) { + io_tlb_nslabs = (default_size >> IO_TLB_SHIFT); + io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE); + } + + iotlb = swiotlb_alloc_boot(io_tlb_nslabs << IO_TLB_SHIFT, + io_tlb_nslabs); + BUG_ON(!iotlb); + + io_tlb_overflow_buffer = swiotlb_alloc_boot(io_tlb_overflow, + io_tlb_overflow >> IO_TLB_SHIFT); + if (!io_tlb_overflow_buffer) + panic("Cannot allocate SWIOTLB overflow buffer!\n"); + + swiotlb_register_iotlb(iotlb); } /* @@ -378,8 +379,9 @@ static void swiotlb_bounce(phys_addr_t phys, char *dma_addr, size_t size, /* * Allocates bounce buffer and returns its kernel virtual address. */ -static void * -map_single(struct device *hwdev, phys_addr_t phys, size_t size, int dir) +void *do_map_single(struct device *hwdev, dma_addr_t io_tlb_daddr, + phys_addr_t phys, size_t size, + enum dma_data_direction dir) { unsigned long flags; char *dma_addr; @@ -391,7 +393,7 @@ map_single(struct device *hwdev, phys_addr_t phys, size_t size, int dir) unsigned long max_slots; mask = dma_get_seg_boundary(hwdev); - start_dma_addr = swiotlb_virt_to_bus(hwdev, io_tlb_start) & mask; + start_dma_addr = io_tlb_daddr & mask; offset_slots = ALIGN(start_dma_addr, 1 << IO_TLB_SHIFT) >> IO_TLB_SHIFT; @@ -481,11 +483,18 @@ found: return dma_addr; } +static void *map_single(struct device *dev, phys_addr_t phys, size_t size, + enum dma_data_direction dir) +{ + return do_map_single(dev, swiotlb_virt_to_bus(dev, io_tlb_start), + phys, size, dir); +} + /* * dma_addr is the kernel virtual address of the bounce buffer to unmap. */ -static void -do_unmap_single(struct device *hwdev, char *dma_addr, size_t size, int dir) +void do_unmap_single(struct device *hwdev, char *dma_addr, size_t size, + enum dma_data_direction dir) { unsigned long flags; int i, count, nslots = ALIGN(size, 1 << IO_TLB_SHIFT) >> IO_TLB_SHIFT; @@ -524,7 +533,7 @@ do_unmap_single(struct device *hwdev, char *dma_addr, size_t size, int dir) spin_unlock_irqrestore(&io_tlb_lock, flags); } -static void +void sync_single(struct device *hwdev, char *dma_addr, size_t size, int dir, int target) { _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel