Where do we stand with the Xen patches?

Where do we stand with the Xen patches?

[Jeremy Fitzhardinge]

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

Where do we stand with the Xen patches?

[Jeremy Fitzhardinge]

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

Re: Where do we stand with the Xen patches?

[Ingo Molnar]

* 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

Re: Where do we stand with the Xen patches?

[Ingo Molnar]

* 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

Re: Where do we stand with the Xen patches?

[Jeremy Fitzhardinge]

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

Re: Where do we stand with the Xen patches?

[FUJITA Tomonori]

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.

Re: Where do we stand with the Xen patches?

[FUJITA Tomonori]

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.

Re: Where do we stand with the Xen patches?

[Jeremy Fitzhardinge]

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

Re: Where do we stand with the Xen patches?

[Jeremy Fitzhardinge]

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

Re: Where do we stand with the Xen patches?

[Ingo Molnar]

* 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

Re: Where do we stand with the Xen patches?

[Ingo Molnar]

* 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

Re: Where do we stand with the Xen patches?

[FUJITA Tomonori]

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.

Re: Where do we stand with the Xen patches?

[FUJITA Tomonori]

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.

Re: Where do we stand with the Xen patches?

[Ingo Molnar]

* 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

Re: Where do we stand with the Xen patches?

[Ingo Molnar]

* 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

Re: Where do we stand with the Xen patches?

[FUJITA Tomonori]

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.

Re: Where do we stand with the Xen patches?

[Ian Campbell]

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

Re: Where do we stand with the Xen patches?

[Ian Campbell]

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

Re: Where do we stand with the Xen patches?

[Jeremy Fitzhardinge]

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

Re: Where do we stand with the Xen patches?

[Jeremy Fitzhardinge]

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

Re: Where do we stand with the Xen patches?

[FUJITA Tomonori]

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

Re: Where do we stand with the Xen patches?

[FUJITA Tomonori]

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

Re: Where do we stand with the Xen patches?

[Ian Campbell]

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

Re: Where do we stand with the Xen patches?

[Ian Campbell]

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

Re: Where do we stand with the Xen patches?

[Ian Campbell]

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

Re: Where do we stand with the Xen patches?

[Ian Campbell]

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

Re: Where do we stand with the Xen patches?

[FUJITA Tomonori]

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; }
> +#endif

I 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.

Re: Where do we stand with the Xen patches?

[FUJITA Tomonori]

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; }
> +#endif

I 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.

Re: [Xen-devel] Re: Where do we stand with the Xen patches?

[Ian Campbell]

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

Re: Re: Where do we stand with the Xen patches?

[Ian Campbell]

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

Re: [Xen-devel] Re: Where do we stand with the Xen patches?

[Ian Campbell]

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.

Re: Re: Where do we stand with the Xen patches?

[Ian Campbell]

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.

Re: [Xen-devel] Re: Where do we stand with the Xen patches?

[FUJITA Tomonori]

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.

Re: Re: Where do we stand with the Xen patches?

[FUJITA Tomonori]

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.

Re: [Xen-devel] Re: Where do we stand with the Xen patches?

[Ian Campbell]

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.c

Again 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.

Re: Re: Where do we stand with the Xen patches?

[Ian Campbell]

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.c

Again 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.

swiotlb: remove __weak hooks in favour of architecture-specific functions

[Ian Campbell]

At the end of this series there are no more __weak functions in
lib/swiotlb.c

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(-)

Re: swiotlb: remove __weak hooks in favour of architecture-specific functions

[Ian Campbell]

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.c

Hmm, 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(-)

Re: swiotlb: remove __weak hooks in favour of architecture-specific functions

[Ian Campbell]

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.c

Hmm, 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(-)

Re: swiotlb: remove __weak hooks in favour of architecture-specific functions

[Randy Dunlap]

Ian Campbell wrote:
> At the end of this series there are no more __weak functions in
> lib/swiotlb.c
> 
> 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)

Try these.  They are fairly current.  I have used several of them.

http://bakeyournoodle.com/cross/

-- 
~Randy
LPC 2009, Sept. 23-25, Portland, Oregon
http://linuxplumbersconf.org/2009/

Re: swiotlb: remove __weak hooks in favour of architecture-specific functions

[Ian Campbell]

On Thu, 2009-05-21 at 12:47 -0400, Randy Dunlap wrote:
> Try these.  They are fairly current.  I have used several of them.
> 
> http://bakeyournoodle.com/cross/

Thanks for the pointer, these look ideal.

Ian.

Re: swiotlb: remove __weak hooks in favour of architecture-specific functions

[FUJITA Tomonori]

On Thu, 21 May 2009 17:15:21 +0100
Ian Campbell <ian.campbell@citrix.com> wrote:

> At the end of this series there are no more __weak functions in
> lib/swiotlb.c
> 
> 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>

Sorry, Nack.

As I wrote in another mail, this patch makes the code more difficult
to understand; it just moves the hacks in lib/swiotlb.c somewhere else
in a strange way instead of killing them.

Please go with the following way (that I posted yesterday):

http://marc.info/?l=xen-devel&m=124292666214380&w=2


Export the core feature of swiotlb, managing iotlb buffer and
implement the Xen mapping functions. With that approach, there is not
much code duplication and there is no need for ugly hooks for dom0;
the phys/bus address conversion and address checking.

Re: swiotlb: remove __weak hooks in favour of architecture-specific functions

[Ian Campbell]

On Fri, 2009-05-22 at 07:13 -0400, FUJITA Tomonori wrote:
> On Thu, 21 May 2009 17:15:21 +0100
> Ian Campbell <ian.campbell@citrix.com> wrote:
> Please go with the following way (that I posted yesterday):
> 
> http://marc.info/?l=xen-devel&m=124292666214380&w=2
> 
> 
> Export the core feature of swiotlb, managing iotlb buffer and
> implement the Xen mapping functions.

I feel that should be a last resort, before we go down that path we
should try and find a way for us to use the generic code in a clean way
which makes everyone happy.

We have had several attempts at this and admittedly have not yet come up
with something that satisfies everyone but I don't really think we have
gotten to the point of admitting defeat and just duplicating the code.

I think the proposal to use a dma_map_range-like function which I sent a
few minutes ago I think gets us closer to something which satisfies
everyone's requirements, including yours for a clean abstraction.

>  With that approach, there is not
> much code duplication and there is no need for ugly hooks for dom0;
> the phys/bus address conversion and address checking.

The phys/bus address conversion is also needed for powerpc.

I think the two address checking functions can be collapsed into a
single one which satisfies the needs of both Xen and powerpc.

What dom0 specific "ugly" hooks does that leave? The alloc one? I've
discussed that in another mail.

Ian.

Re: swiotlb: remove __weak hooks in favour of architecture-specific functions

[FUJITA Tomonori]

On Fri, 22 May 2009 12:43:16 +0100
Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:

> On Fri, 2009-05-22 at 07:13 -0400, FUJITA Tomonori wrote:
> > On Thu, 21 May 2009 17:15:21 +0100
> > Ian Campbell <ian.campbell@citrix.com> wrote:
> > Please go with the following way (that I posted yesterday):
> > 
> > http://marc.info/?l=xen-devel&m=124292666214380&w=2
> > 
> > 
> > Export the core feature of swiotlb, managing iotlb buffer and
> > implement the Xen mapping functions.
> 
> I feel that should be a last resort, before we go down that path we
> should try and find a way for us to use the generic code in a clean way
> which makes everyone happy.
>
> We have had several attempts at this and admittedly have not yet come up
> with something that satisfies everyone but I don't really think we have
> gotten to the point of admitting defeat and just duplicating the code.

There should not be much duplication.


> I think the proposal to use a dma_map_range-like function which I sent a
> few minutes ago I think gets us closer to something which satisfies
> everyone's requirements, including yours for a clean abstraction.

Seems you don't understand the point; with dom0, we can't cleanly use
arch/*/include/asm/.

You need to insert Xen's hook like this:


+++ b/arch/x86/include/asm/dma-mapping.h
@@ -309,4 +309,20 @@ static inline void dma_free_coherent(struct device *dev, size_t size,
 		ops->free_coherent(dev, size, vaddr, bus);
 }
 
+static inline dma_addr_t dma_map_range(struct device *dev, u64 mask,
+				       phys_addr_t addr, size_t size)
+{
+	dma_addr_t dma_addr;
+#ifdef CONFIG_XEN
+	extern int xen_range_needs_mapping(phys_addr_t paddr, size_t size);
+	if (xen_pv_domain() && xen_range_needs_mapping(addr, size))
+		return 0;
+#endif
+
+	dma_addr = swiotlb_phys_to_bus(dev, addr);
+	if (dma_addr + size <= mask)
+		return 0;
+	return dma_addr;
+}

Or you need to use a function pointer in a strange way.

--- a/arch/x86/include/asm/dma-mapping.h
+++ b/arch/x86/include/asm/dma-mapping.h
@@ -315,4 +315,13 @@ static inline int is_buffer_dma_capable(struct device *dev, u64 mask,
 	return addr + size <= mask;
 }
 
+extern int (*x86_swiotlb_force_mapping)(phys_addr_t paddr, size_t size);
+
+static inline int swiotlb_force_mapping(phys_addr_t paddr, size_t size)
+{
+	if (x86_swiotlb_force_mapping)
+		return x86_swiotlb_force_mapping(paddr, size);
+	return 0;
+}
+

Or you could invent something more.


As you said in another mail, it's up to the x86 maintainer :

> This case is internal to the x86 arch code and I'd really like to hear
> the x86 maintainer's opinion of the general approach.


But the above code looks really ugly to me.


> >  With that approach, there is not
> > much code duplication and there is no need for ugly hooks for dom0;
> > the phys/bus address conversion and address checking.
> 
> The phys/bus address conversion is also needed for powerpc.
> 
> I think the two address checking functions can be collapsed into a
> single one which satisfies the needs of both Xen and powerpc.
> 
> What dom0 specific "ugly" hooks does that leave? The alloc one? I've
> discussed that in another mail.

See above. POWERPC can use arch/*/include/asm/ cleanly for the
phys/bus address conversion while dom0 can't. That's what I said again
and again.

Re: swiotlb: remove __weak hooks in favour of architecture-specific functions

[Ian Campbell]

On Fri, 2009-05-22 at 07:55 -0400, FUJITA Tomonori wrote:
> On Fri, 22 May 2009 12:43:16 +0100
> Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
> 
> > On Fri, 2009-05-22 at 07:13 -0400, FUJITA Tomonori wrote:
> > > On Thu, 21 May 2009 17:15:21 +0100
> > > Ian Campbell <ian.campbell@citrix.com> wrote:
> > > Please go with the following way (that I posted yesterday):
> > > 
> > > http://marc.info/?l=xen-devel&m=124292666214380&w=2
> > > 
> > > 
> > > Export the core feature of swiotlb, managing iotlb buffer and
> > > implement the Xen mapping functions.
> > 
> > I feel that should be a last resort, before we go down that path we
> > should try and find a way for us to use the generic code in a clean way
> > which makes everyone happy.
> >
> > We have had several attempts at this and admittedly have not yet come up
> > with something that satisfies everyone but I don't really think we have
> > gotten to the point of admitting defeat and just duplicating the code.
> 
> There should not be much duplication.
> 
> 
> > I think the proposal to use a dma_map_range-like function which I sent a
> > few minutes ago I think gets us closer to something which satisfies
> > everyone's requirements, including yours for a clean abstraction.
> 
> Seems you don't understand the point; with dom0, we can't cleanly use
> arch/*/include/asm/.

I understand precisely what you are saying, I just fundamentally
disagree with you. It is perfectly possible for an arch/*/include/asm
interface for this stuff to be defined which is completely abstracted
from the POV of the swiotlb code (and any other arch-external code).

> You need to insert Xen's hook like this:

As I said in the email this snippet was contained in:
>       * The Xen specific stuff in arch/x86/include/asm/dma-mapping.h
>         clearly needs to be properly abstracted away (I just stuck it
>         there for testing).

So obviously I am well aware that it is unacceptable as it stands.

I think we can find a way to implement this functionality which is
contained entirely within the arch/x86 code and is also acceptable to
the x86 maintainers. There are plenty of cases where we define similar
interfaces where arch code implements an API with different backends for
different configurations, hardware configurations etc etc.

> See above. POWERPC can use arch/*/include/asm/ cleanly for the
> phys/bus address conversion while dom0 can't. That's what I said again
> and again.

And I dispute this claim, again and again.

Ian.

Re: swiotlb: remove __weak hooks in favour of architecture-specific functions

[FUJITA Tomonori]

On Fri, 22 May 2009 15:02:04 +0100
Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:

> On Fri, 2009-05-22 at 07:55 -0400, FUJITA Tomonori wrote:
> > On Fri, 22 May 2009 12:43:16 +0100
> > Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
> > 
> > > On Fri, 2009-05-22 at 07:13 -0400, FUJITA Tomonori wrote:
> > > > On Thu, 21 May 2009 17:15:21 +0100
> > > > Ian Campbell <ian.campbell@citrix.com> wrote:
> > > > Please go with the following way (that I posted yesterday):
> > > > 
> > > > http://marc.info/?l=xen-devel&m=124292666214380&w=2
> > > > 
> > > > 
> > > > Export the core feature of swiotlb, managing iotlb buffer and
> > > > implement the Xen mapping functions.
> > > 
> > > I feel that should be a last resort, before we go down that path we
> > > should try and find a way for us to use the generic code in a clean way
> > > which makes everyone happy.
> > >
> > > We have had several attempts at this and admittedly have not yet come up
> > > with something that satisfies everyone but I don't really think we have
> > > gotten to the point of admitting defeat and just duplicating the code.
> > 
> > There should not be much duplication.
> > 
> > 
> > > I think the proposal to use a dma_map_range-like function which I sent a
> > > few minutes ago I think gets us closer to something which satisfies
> > > everyone's requirements, including yours for a clean abstraction.
> > 
> > Seems you don't understand the point; with dom0, we can't cleanly use
> > arch/*/include/asm/.
> 
> I understand precisely what you are saying, I just fundamentally
> disagree with you. It is perfectly possible for an arch/*/include/asm
> interface for this stuff to be defined which is completely abstracted
> from the POV of the swiotlb code (and any other arch-external code).
> 
> > You need to insert Xen's hook like this:
> 
> As I said in the email this snippet was contained in:
> >       * The Xen specific stuff in arch/x86/include/asm/dma-mapping.h
> >         clearly needs to be properly abstracted away (I just stuck it
> >         there for testing).
> 
> So obviously I am well aware that it is unacceptable as it stands.

I don't think that you can't clean up the above much.

After all, you need a hook for Xen in
arch/x86/include/asm/dma-mapping.h and define it in Xen's land.

That will not be much different from:

http://marc.info/?l=linux-kernel&m=124299344606890&w=2


And adding 'if xen' hook in common header files in arch/x86/include/ is a
wrong design to me. Do you add a similar 'if xen' hook in other x86's
common headers files?


> I think we can find a way to implement this functionality which is
k> contained entirely within the arch/x86 code and is also acceptable to
> the x86 maintainers. There are plenty of cases where we define similar

I'm not sure about it (hopefully, not). But if the x86 maintainers ACK
the above code, I have no complaint.


> interfaces where arch code implements an API with different backends for
> different configurations, hardware configurations etc etc.

[PATCH] swiotlb: make is_buffer_dma_capable architecture-specific

[Ian Campbell]

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.

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>
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 |    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         |    3 ++-
 include/linux/dma-mapping.h         |    5 -----
 lib/swiotlb.c                       |   25 +++++++------------------
 7 files changed, 24 insertions(+), 27 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 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..23fa16e 100644
--- a/arch/x86/kernel/pci-nommu.c
+++ b/arch/x86/kernel/pci-nommu.c
@@ -14,7 +14,8 @@
 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 cec5f62..98726a2 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -147,12 +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;
@@ -318,12 +312,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);
@@ -565,8 +553,8 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 
 	ret = (void *)__get_free_pages(flags, order);
 	if (ret &&
-	    !is_buffer_dma_capable(dma_mask, swiotlb_virt_to_bus(hwdev, ret),
-				   size)) {
+	    !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 +576,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 +646,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 +664,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 +811,8 @@ 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) {
-- 
1.5.6.5

[PATCH] swiotlb: make range_needs_mapping architecture-specific

[Ian Campbell]

This moves range_needs_mapping() from lib/swiotlb.c to
arch/*/include/asm/dma-mapping.h and renames it
swiotlb_arch_force_mapping() to more acurately reflect its usage.

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    |    5 +++++
 arch/powerpc/include/asm/dma-mapping.h |    5 +++++
 arch/x86/include/asm/dma-mapping.h     |    9 +++++++++
 arch/x86/kernel/pci-swiotlb.c          |    2 ++
 include/linux/swiotlb.h                |    2 --
 lib/swiotlb.c                          |   13 ++++---------
 6 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/arch/ia64/include/asm/dma-mapping.h b/arch/ia64/include/asm/dma-mapping.h
index a091648..3e8fb31 100644
--- a/arch/ia64/include/asm/dma-mapping.h
+++ b/arch/ia64/include/asm/dma-mapping.h
@@ -180,4 +180,9 @@ static inline int is_buffer_dma_capable(struct device *dev, u64 mask,
 	return addr + size <= mask;
 }
 
+static inline int swiotlb_force_mapping(phys_addr_t paddr, size_t size)
+{
+       return 0;
+}
+
 #endif /* _ASM_IA64_DMA_MAPPING_H */
diff --git a/arch/powerpc/include/asm/dma-mapping.h b/arch/powerpc/include/asm/dma-mapping.h
index c69f2b5..7e3510d 100644
--- a/arch/powerpc/include/asm/dma-mapping.h
+++ b/arch/powerpc/include/asm/dma-mapping.h
@@ -429,5 +429,10 @@ static inline void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
 	__dma_sync(vaddr, size, (int)direction);
 }
 
+static inline int swiotlb_force_mapping(phys_addr_t paddr, size_t size)
+{
+	return 0;
+}
+
 #endif /* __KERNEL__ */
 #endif	/* _ASM_DMA_MAPPING_H */
diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h
index 52b8d36..aa9639f 100644
--- a/arch/x86/include/asm/dma-mapping.h
+++ b/arch/x86/include/asm/dma-mapping.h
@@ -315,4 +315,13 @@ static inline int is_buffer_dma_capable(struct device *dev, u64 mask,
 	return addr + size <= mask;
 }
 
+extern int (*x86_swiotlb_force_mapping)(phys_addr_t paddr, size_t size);
+
+static inline int swiotlb_force_mapping(phys_addr_t paddr, size_t size)
+{
+	if (x86_swiotlb_force_mapping)
+		return x86_swiotlb_force_mapping(paddr, size);
+	return 0;
+}
+
 #endif
diff --git a/arch/x86/kernel/pci-swiotlb.c b/arch/x86/kernel/pci-swiotlb.c
index 7267376..d48912c 100644
--- a/arch/x86/kernel/pci-swiotlb.c
+++ b/arch/x86/kernel/pci-swiotlb.c
@@ -15,6 +15,8 @@
 
 int swiotlb __read_mostly;
 
+int (*x86_swiotlb_force_mapping)(phys_addr_t paddr, size_t size);
+
 static void *x86_swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 					dma_addr_t *dma_handle, gfp_t flags)
 {
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 98726a2..d311654 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -147,11 +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_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;
@@ -312,9 +307,9 @@ cleanup1:
 	return -ENOMEM;
 }
 
-static inline int range_needs_mapping(phys_addr_t paddr, size_t size)
+static inline int force_mapping(phys_addr_t paddr, size_t size)
 {
-	return swiotlb_force || swiotlb_arch_range_needs_mapping(paddr, size);
+	return swiotlb_force || swiotlb_force_mapping(paddr, size);
 }
 
 static int is_swiotlb_buffer(char *addr)
@@ -647,7 +642,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 	 * buffering it.
 	 */
 	if (is_buffer_dma_capable(dev, dma_get_mask(dev), dev_addr, size) &&
-	    !range_needs_mapping(phys, size))
+	    !force_mapping(phys, size))
 		return dev_addr;
 
 	/*
@@ -810,7 +805,7 @@ 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) ||
+		if (force_mapping(paddr, sg->length) ||
 		    !is_buffer_dma_capable(hwdev, dma_get_mask(hwdev),
 					   dev_addr, sg->length)) {
 			void *map = map_single(hwdev, sg_phys(sg),
-- 
1.5.6.5

Re: [PATCH] swiotlb: make range_needs_mapping architecture-specific

[FUJITA Tomonori]

On Thu, 21 May 2009 17:15:23 +0100
Ian Campbell <ian.campbell@citrix.com> wrote:

> This moves range_needs_mapping() from lib/swiotlb.c to
> arch/*/include/asm/dma-mapping.h and renames it
> swiotlb_arch_force_mapping() to more acurately reflect its usage.
> 
> 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    |    5 +++++
>  arch/powerpc/include/asm/dma-mapping.h |    5 +++++
>  arch/x86/include/asm/dma-mapping.h     |    9 +++++++++
>  arch/x86/kernel/pci-swiotlb.c          |    2 ++
>  include/linux/swiotlb.h                |    2 --
>  lib/swiotlb.c                          |   13 ++++---------
>  6 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/ia64/include/asm/dma-mapping.h b/arch/ia64/include/asm/dma-mapping.h
> index a091648..3e8fb31 100644
> --- a/arch/ia64/include/asm/dma-mapping.h
> +++ b/arch/ia64/include/asm/dma-mapping.h
> @@ -180,4 +180,9 @@ static inline int is_buffer_dma_capable(struct device *dev, u64 mask,
>  	return addr + size <= mask;
>  }
>  
> +static inline int swiotlb_force_mapping(phys_addr_t paddr, size_t size)
> +{
> +       return 0;
> +}
> +
>  #endif /* _ASM_IA64_DMA_MAPPING_H */
> diff --git a/arch/powerpc/include/asm/dma-mapping.h b/arch/powerpc/include/asm/dma-mapping.h
> index c69f2b5..7e3510d 100644
> --- a/arch/powerpc/include/asm/dma-mapping.h
> +++ b/arch/powerpc/include/asm/dma-mapping.h
> @@ -429,5 +429,10 @@ static inline void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
>  	__dma_sync(vaddr, size, (int)direction);
>  }
>  
> +static inline int swiotlb_force_mapping(phys_addr_t paddr, size_t size)
> +{
> +	return 0;
> +}
> +

Adding a swiotlb specific function to asm/dma-mapping.h is wrong.


>  #endif /* __KERNEL__ */
>  #endif	/* _ASM_DMA_MAPPING_H */
> diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h
> index 52b8d36..aa9639f 100644
> --- a/arch/x86/include/asm/dma-mapping.h
> +++ b/arch/x86/include/asm/dma-mapping.h
> @@ -315,4 +315,13 @@ static inline int is_buffer_dma_capable(struct device *dev, u64 mask,
>  	return addr + size <= mask;
>  }
>  
> +extern int (*x86_swiotlb_force_mapping)(phys_addr_t paddr, size_t size);
> +
> +static inline int swiotlb_force_mapping(phys_addr_t paddr, size_t size)
> +{
> +	if (x86_swiotlb_force_mapping)
> +		return x86_swiotlb_force_mapping(paddr, size);
> +	return 0;
> +}
> +
>  #endif
> diff --git a/arch/x86/kernel/pci-swiotlb.c b/arch/x86/kernel/pci-swiotlb.c
> index 7267376..d48912c 100644
> --- a/arch/x86/kernel/pci-swiotlb.c
> +++ b/arch/x86/kernel/pci-swiotlb.c
> @@ -15,6 +15,8 @@
>  
>  int swiotlb __read_mostly;
>  
> +int (*x86_swiotlb_force_mapping)(phys_addr_t paddr, size_t size);
> +
>  static void *x86_swiotlb_alloc_coherent(struct device *hwdev, size_t size,
>  					dma_addr_t *dma_handle, gfp_t flags)
>  {
> 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);

Ditto.

And using a function pointer for the architecture abstraction is worse
than __weak.


> -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 98726a2..d311654 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -147,11 +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_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;
> @@ -312,9 +307,9 @@ cleanup1:
>  	return -ENOMEM;
>  }
>  
> -static inline int range_needs_mapping(phys_addr_t paddr, size_t size)
> +static inline int force_mapping(phys_addr_t paddr, size_t size)
>  {
> -	return swiotlb_force || swiotlb_arch_range_needs_mapping(paddr, size);
> +	return swiotlb_force || swiotlb_force_mapping(paddr, size);
>  }
>  
>  static int is_swiotlb_buffer(char *addr)
> @@ -647,7 +642,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
>  	 * buffering it.
>  	 */
>  	if (is_buffer_dma_capable(dev, dma_get_mask(dev), dev_addr, size) &&
> -	    !range_needs_mapping(phys, size))
> +	    !force_mapping(phys, size))
>  		return dev_addr;
>  
>  	/*
> @@ -810,7 +805,7 @@ 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) ||
> +		if (force_mapping(paddr, sg->length) ||
>  		    !is_buffer_dma_capable(hwdev, dma_get_mask(hwdev),
>  					   dev_addr, sg->length)) {
>  			void *map = map_single(hwdev, sg_phys(sg),
> -- 
> 1.5.6.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] swiotlb: make range_needs_mapping architecture-specific

[Ian Campbell]

On Fri, 2009-05-22 at 07:13 -0400, FUJITA Tomonori wrote:
> On Thu, 21 May 2009 17:15:23 +0100
> Ian Campbell <ian.campbell@citrix.com> wrote:
> > +static inline int swiotlb_force_mapping(phys_addr_t paddr, size_t size)
> > +{
> > +	return 0;
> > +}
> > +
> 
> Adding a swiotlb specific function to asm/dma-mapping.h is wrong.

This one is unnecessary with the dma_map_range proposal.

> > +int (*x86_swiotlb_force_mapping)(phys_addr_t paddr, size_t size);
> > +
[...]
> And using a function pointer for the architecture abstraction is worse
> than __weak.

This specific hook is unnecessary with the dma_map_range proposal but in
general we use function pointers quite extensively for abstraction in
the kernel.

This case is internal to the x86 arch code and I'd really like to hear
the x86 maintainer's opinion of the general approach.

Ian.

[PATCH] swiotlb/xen: update xen for swiotlb_arch_force_mapping changes

[Ian Campbell]

This function is now implemented via a hook in
arch/x86/include/asm/dma-mapping.h rather than as a weak hook in
swiotlb.c

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/x86/xen/pci-swiotlb.c |    8 --------
 drivers/pci/xen-iommu.c    |    4 +++-
 include/xen/swiotlb.h      |    1 -
 3 files changed, 3 insertions(+), 10 deletions(-)

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..f65660a 100644
--- a/drivers/pci/xen-iommu.c
+++ b/drivers/pci/xen-iommu.c
@@ -130,7 +130,7 @@ static int range_straddles_page_boundary(phys_addr_t p, size_t size)
 	return 1;
 }
 
-int xen_range_needs_mapping(phys_addr_t paddr, size_t size)
+static int xen_swiotlb_force_mapping(phys_addr_t paddr, size_t size)
 {
 	return range_straddles_page_boundary(paddr, size);
 }
@@ -324,6 +324,8 @@ void __init xen_iommu_init(void)
 	force_iommu = 0;
 	dma_ops = &xen_dma_ops;
 
+	x86_swiotlb_force_mapping = &xen_swiotlb_force_mapping;
+
 	if (swiotlb) {
 		printk(KERN_INFO "Xen: Enabling DMA fallback to swiotlb\n");
 		dma_ops = &xen_swiotlb_dma_ops;
diff --git a/include/xen/swiotlb.h b/include/xen/swiotlb.h
index 75d1da1..64137fa 100644
--- a/include/xen/swiotlb.h
+++ b/include/xen/swiotlb.h
@@ -4,7 +4,6 @@
 extern void xen_swiotlb_fixup(void *buf, size_t size, unsigned long nslabs);
 extern phys_addr_t xen_bus_to_phys(dma_addr_t daddr);
 extern dma_addr_t xen_phys_to_bus(phys_addr_t paddr);
-extern int xen_range_needs_mapping(phys_addr_t phys, size_t size);
 
 #ifdef CONFIG_PCI_XEN
 extern int xen_wants_swiotlb(void);
-- 
1.5.6.5

[PATCH] swiotlb: make swiotlb allocation functions architecture-specific

[Ian Campbell]

Some architectures need to allocate memory to be used as a swiotlb in
a special manner.

Also make the swiotlb_late_init_with_default_size() function only
available on architectures which require it.

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 |    5 +++++
 arch/ia64/kernel/pci-swiotlb.c      |   11 +++++++++++
 arch/x86/include/asm/dma-mapping.h  |    4 ++++
 arch/x86/kernel/pci-swiotlb.c       |   12 ++++++++++++
 arch/x86/xen/pci-swiotlb.c          |   17 -----------------
 include/linux/swiotlb.h             |    3 ---
 lib/swiotlb.c                       |   12 ++----------
 7 files changed, 34 insertions(+), 30 deletions(-)

diff --git a/arch/ia64/include/asm/dma-mapping.h b/arch/ia64/include/asm/dma-mapping.h
index 3e8fb31..9f9994d 100644
--- a/arch/ia64/include/asm/dma-mapping.h
+++ b/arch/ia64/include/asm/dma-mapping.h
@@ -185,4 +185,9 @@ static inline int swiotlb_force_mapping(phys_addr_t paddr, size_t size)
        return 0;
 }
 
+#define SWIOTLB_WANT_LATE_INIT 1
+
+extern void *swiotlb_alloc_boot(size_t size, unsigned long nslabs);
+extern void *swiotlb_alloc(unsigned order, unsigned long nslabs);
+
 #endif /* _ASM_IA64_DMA_MAPPING_H */
diff --git a/arch/ia64/kernel/pci-swiotlb.c b/arch/ia64/kernel/pci-swiotlb.c
index 285aae8..1b42f21 100644
--- a/arch/ia64/kernel/pci-swiotlb.c
+++ b/arch/ia64/kernel/pci-swiotlb.c
@@ -4,6 +4,7 @@
 #include <linux/cache.h>
 #include <linux/module.h>
 #include <linux/dma-mapping.h>
+#include <linux/bootmem.h>
 
 #include <asm/swiotlb.h>
 #include <asm/dma.h>
@@ -13,6 +14,16 @@
 int swiotlb __read_mostly;
 EXPORT_SYMBOL(swiotlb);
 
+void * __init swiotlb_alloc_boot(size_t size, unsigned long nslabs)
+{
+	return alloc_bootmem_low_pages(size);
+}
+
+void *swiotlb_alloc(unsigned order, unsigned long nslabs)
+{
+	return (void *)__get_free_pages(GFP_DMA | __GFP_NOWARN, order);
+}
+
 static void *ia64_swiotlb_alloc_coherent(struct device *dev, size_t size,
 					 dma_addr_t *dma_handle, gfp_t gfp)
 {
diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h
index aa9639f..5a3180d 100644
--- a/arch/x86/include/asm/dma-mapping.h
+++ b/arch/x86/include/asm/dma-mapping.h
@@ -324,4 +324,8 @@ static inline int swiotlb_force_mapping(phys_addr_t paddr, size_t size)
 	return 0;
 }
 
+extern void (*x86_swiotlb_alloc_fixup)(void *buf, size_t size,
+				       unsigned long nslabs);
+extern void *swiotlb_alloc_boot(size_t size, unsigned long nslabs);
+
 #endif
diff --git a/arch/x86/kernel/pci-swiotlb.c b/arch/x86/kernel/pci-swiotlb.c
index d48912c..7de9fdd 100644
--- a/arch/x86/kernel/pci-swiotlb.c
+++ b/arch/x86/kernel/pci-swiotlb.c
@@ -6,6 +6,7 @@
 #include <linux/swiotlb.h>
 #include <linux/bootmem.h>
 #include <linux/dma-mapping.h>
+#include <linux/bootmem.h>
 
 #include <asm/iommu.h>
 #include <asm/swiotlb.h>
@@ -16,6 +17,17 @@
 int swiotlb __read_mostly;
 
 int (*x86_swiotlb_force_mapping)(phys_addr_t paddr, size_t size);
+void __initdata (*x86_swiotlb_alloc_fixup)(void *buf, size_t size, unsigned long nslabs);
+
+void * __init swiotlb_alloc_boot(size_t size, unsigned long nslabs)
+{
+	void *ret = alloc_bootmem_low_pages(size);
+
+	if (ret && x86_swiotlb_alloc_fixup)
+		x86_swiotlb_alloc_fixup(ret, size, nslabs);
+
+	return ret;
+}
 
 static void *x86_swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 					dma_addr_t *dma_handle, gfp_t flags)
diff --git a/arch/x86/xen/pci-swiotlb.c b/arch/x86/xen/pci-swiotlb.c
index e23b00b..dcf2ef8 100644
--- a/arch/x86/xen/pci-swiotlb.c
+++ b/arch/x86/xen/pci-swiotlb.c
@@ -11,23 +11,6 @@
  * implementations in lib/swiotlb.c.
  */
 
-void * __init swiotlb_alloc_boot(size_t size, unsigned long nslabs)
-{
-	void *ret = alloc_bootmem_low_pages(size);
-
-	if (ret && xen_pv_domain())
-		xen_swiotlb_fixup(ret, size, nslabs);
-
-	return ret;
-}
-
-void *swiotlb_alloc(unsigned order, unsigned long nslabs)
-{
-	/* Never called on x86.  Warn, just in case. */
-	WARN_ON(1);
-	return NULL;
-}
-
 dma_addr_t swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr)
 {
 	if (xen_pv_domain())
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index 32f4fa4..be8b77d 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -24,9 +24,6 @@ struct scatterlist;
 extern void
 swiotlb_init(void);
 
-extern void *swiotlb_alloc_boot(size_t bytes, unsigned long nslabs);
-extern void *swiotlb_alloc(unsigned order, unsigned long nslabs);
-
 extern dma_addr_t swiotlb_phys_to_bus(struct device *hwdev,
 				      phys_addr_t address);
 extern phys_addr_t swiotlb_bus_to_phys(struct device *hwdev,
diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index d311654..640c2f1 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -114,16 +114,6 @@ setup_io_tlb_npages(char *str)
 __setup("swiotlb=", setup_io_tlb_npages);
 /* make io_tlb_overflow tunable too? */
 
-void * __weak __init swiotlb_alloc_boot(size_t size, unsigned long nslabs)
-{
-	return alloc_bootmem_low_pages(size);
-}
-
-void * __weak swiotlb_alloc(unsigned order, unsigned long nslabs)
-{
-	return (void *)__get_free_pages(GFP_DMA | __GFP_NOWARN, order);
-}
-
 dma_addr_t __weak swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr)
 {
 	return paddr;
@@ -213,6 +203,7 @@ swiotlb_init(void)
 	swiotlb_init_with_default_size(64 * (1<<20));	/* default to 64MB */
 }
 
+#ifdef SWIOTLB_WANT_LATE_INIT
 /*
  * Systems with larger DMA zones (those that don't support ISA) can
  * initialize the swiotlb later using the slab allocator if needed.
@@ -306,6 +297,7 @@ cleanup1:
 	io_tlb_nslabs = req_nslabs;
 	return -ENOMEM;
 }
+#endif
 
 static inline int force_mapping(phys_addr_t paddr, size_t size)
 {
-- 
1.5.6.5

Re: [PATCH] swiotlb: make swiotlb allocation functions architecture-specific

[FUJITA Tomonori]

On Thu, 21 May 2009 17:15:25 +0100
Ian Campbell <ian.campbell@citrix.com> wrote:

> Some architectures need to allocate memory to be used as a swiotlb in
> a special manner.

Hmm, what architectures need a special manner? I guess only Xen.


> Also make the swiotlb_late_init_with_default_size() function only
> available on architectures which require it.
> 
> 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 |    5 +++++
>  arch/ia64/kernel/pci-swiotlb.c      |   11 +++++++++++
>  arch/x86/include/asm/dma-mapping.h  |    4 ++++
>  arch/x86/kernel/pci-swiotlb.c       |   12 ++++++++++++
>  arch/x86/xen/pci-swiotlb.c          |   17 -----------------
>  include/linux/swiotlb.h             |    3 ---
>  lib/swiotlb.c                       |   12 ++----------
>  7 files changed, 34 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/ia64/include/asm/dma-mapping.h b/arch/ia64/include/asm/dma-mapping.h
> index 3e8fb31..9f9994d 100644
> --- a/arch/ia64/include/asm/dma-mapping.h
> +++ b/arch/ia64/include/asm/dma-mapping.h
> @@ -185,4 +185,9 @@ static inline int swiotlb_force_mapping(phys_addr_t paddr, size_t size)
>         return 0;
>  }
>  
> +#define SWIOTLB_WANT_LATE_INIT 1
> +
> +extern void *swiotlb_alloc_boot(size_t size, unsigned long nslabs);
> +extern void *swiotlb_alloc(unsigned order, unsigned long nslabs);
> +
>  #endif /* _ASM_IA64_DMA_MAPPING_H */
> diff --git a/arch/ia64/kernel/pci-swiotlb.c b/arch/ia64/kernel/pci-swiotlb.c
> index 285aae8..1b42f21 100644
> --- a/arch/ia64/kernel/pci-swiotlb.c
> +++ b/arch/ia64/kernel/pci-swiotlb.c
> @@ -4,6 +4,7 @@
>  #include <linux/cache.h>
>  #include <linux/module.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/bootmem.h>
>  
>  #include <asm/swiotlb.h>
>  #include <asm/dma.h>
> @@ -13,6 +14,16 @@
>  int swiotlb __read_mostly;
>  EXPORT_SYMBOL(swiotlb);
>  
> +void * __init swiotlb_alloc_boot(size_t size, unsigned long nslabs)
> +{
> +	return alloc_bootmem_low_pages(size);
> +}
> +
> +void *swiotlb_alloc(unsigned order, unsigned long nslabs)
> +{
> +	return (void *)__get_free_pages(GFP_DMA | __GFP_NOWARN, order);
> +}
> +
>  static void *ia64_swiotlb_alloc_coherent(struct device *dev, size_t size,
>  					 dma_addr_t *dma_handle, gfp_t gfp)
>  {
> diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h
> index aa9639f..5a3180d 100644
> --- a/arch/x86/include/asm/dma-mapping.h
> +++ b/arch/x86/include/asm/dma-mapping.h
> @@ -324,4 +324,8 @@ static inline int swiotlb_force_mapping(phys_addr_t paddr, size_t size)
>  	return 0;
>  }
>  
> +extern void (*x86_swiotlb_alloc_fixup)(void *buf, size_t size,
> +				       unsigned long nslabs);
> +extern void *swiotlb_alloc_boot(size_t size, unsigned long nslabs);
> +
>  #endif
> diff --git a/arch/x86/kernel/pci-swiotlb.c b/arch/x86/kernel/pci-swiotlb.c
> index d48912c..7de9fdd 100644
> --- a/arch/x86/kernel/pci-swiotlb.c
> +++ b/arch/x86/kernel/pci-swiotlb.c
> @@ -6,6 +6,7 @@
>  #include <linux/swiotlb.h>
>  #include <linux/bootmem.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/bootmem.h>
>  
>  #include <asm/iommu.h>
>  #include <asm/swiotlb.h>
> @@ -16,6 +17,17 @@
>  int swiotlb __read_mostly;
>  
>  int (*x86_swiotlb_force_mapping)(phys_addr_t paddr, size_t size);
> +void __initdata (*x86_swiotlb_alloc_fixup)(void *buf, size_t size, unsigned long nslabs);
> +
> +void * __init swiotlb_alloc_boot(size_t size, unsigned long nslabs)
> +{
> +	void *ret = alloc_bootmem_low_pages(size);
> +
> +	if (ret && x86_swiotlb_alloc_fixup)
> +		x86_swiotlb_alloc_fixup(ret, size, nslabs);
> +
> +	return ret;
> +}
>  
>  static void *x86_swiotlb_alloc_coherent(struct device *hwdev, size_t size,
>  					dma_addr_t *dma_handle, gfp_t flags)
> diff --git a/arch/x86/xen/pci-swiotlb.c b/arch/x86/xen/pci-swiotlb.c
> index e23b00b..dcf2ef8 100644
> --- a/arch/x86/xen/pci-swiotlb.c
> +++ b/arch/x86/xen/pci-swiotlb.c
> @@ -11,23 +11,6 @@
>   * implementations in lib/swiotlb.c.
>   */
>  
> -void * __init swiotlb_alloc_boot(size_t size, unsigned long nslabs)
> -{
> -	void *ret = alloc_bootmem_low_pages(size);
> -
> -	if (ret && xen_pv_domain())
> -		xen_swiotlb_fixup(ret, size, nslabs);
> -
> -	return ret;
> -}
> -
> -void *swiotlb_alloc(unsigned order, unsigned long nslabs)
> -{
> -	/* Never called on x86.  Warn, just in case. */
> -	WARN_ON(1);
> -	return NULL;
> -}
> -
>  dma_addr_t swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr)
>  {
>  	if (xen_pv_domain())
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index 32f4fa4..be8b77d 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -24,9 +24,6 @@ struct scatterlist;
>  extern void
>  swiotlb_init(void);
>  
> -extern void *swiotlb_alloc_boot(size_t bytes, unsigned long nslabs);
> -extern void *swiotlb_alloc(unsigned order, unsigned long nslabs);
> -
>  extern dma_addr_t swiotlb_phys_to_bus(struct device *hwdev,
>  				      phys_addr_t address);
>  extern phys_addr_t swiotlb_bus_to_phys(struct device *hwdev,
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index d311654..640c2f1 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -114,16 +114,6 @@ setup_io_tlb_npages(char *str)
>  __setup("swiotlb=", setup_io_tlb_npages);
>  /* make io_tlb_overflow tunable too? */
>  
> -void * __weak __init swiotlb_alloc_boot(size_t size, unsigned long nslabs)
> -{
> -	return alloc_bootmem_low_pages(size);
> -}
> -
> -void * __weak swiotlb_alloc(unsigned order, unsigned long nslabs)
> -{
> -	return (void *)__get_free_pages(GFP_DMA | __GFP_NOWARN, order);
> -}
> -
>  dma_addr_t __weak swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr)
>  {
>  	return paddr;
> @@ -213,6 +203,7 @@ swiotlb_init(void)
>  	swiotlb_init_with_default_size(64 * (1<<20));	/* default to 64MB */
>  }
>  
> +#ifdef SWIOTLB_WANT_LATE_INIT
>  /*
>   * Systems with larger DMA zones (those that don't support ISA) can
>   * initialize the swiotlb later using the slab allocator if needed.
> @@ -306,6 +297,7 @@ cleanup1:
>  	io_tlb_nslabs = req_nslabs;
>  	return -ENOMEM;
>  }
> +#endif
>  
>  static inline int force_mapping(phys_addr_t paddr, size_t size)
>  {
> -- 
> 1.5.6.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] swiotlb: make swiotlb allocation functions architecture-specific

[Ian Campbell]

On Fri, 2009-05-22 at 07:13 -0400, FUJITA Tomonori wrote:
> On Thu, 21 May 2009 17:15:25 +0100
> Ian Campbell <ian.campbell@citrix.com> wrote:
> 
> > Some architectures need to allocate memory to be used as a swiotlb in
> > a special manner.
> 
> Hmm, what architectures need a special manner? I guess only Xen.

The x86 architecture, when running as a paravirtualised guest under Xen,
needs it. I guess ia64 might need it for similar reasons, I'm not sure.

How does the fact that x86-Xen is (currently) the only user invalidate
the requirement? You seem to have a knee-jerk reaction to anything which
might be useful to Xen and I really don't understand why that should be
the case.

We are talking about an initialisation path and we are not talking about
changes which completely obscure all meaning or anything. The semantics
of the API are pretty clear, I think.

Ian.

[PATCH] swiotlb/xen: update xen for changes to swiotlb allocation interface

[Ian Campbell]

This function is now implemented via an explicit hook in
arch/x86/kernel/pci-swiotlb.c rather than as a weak hook in
swiotlb.c

Initialsation of the hook needs to happen before xen_iommu_init() is
called so add detect_xen_iommu() (mirroring other IOMMU
implementations) which is called early enough.

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/x86/include/asm/xen/iommu.h |    4 ++++
 arch/x86/kernel/pci-dma.c        |    4 +++-
 drivers/pci/xen-iommu.c          |   14 +++++++++++---
 include/xen/swiotlb.h            |    1 -
 4 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/xen/iommu.h b/arch/x86/include/asm/xen/iommu.h
index 75df312..22b6091 100644
--- a/arch/x86/include/asm/xen/iommu.h
+++ b/arch/x86/include/asm/xen/iommu.h
@@ -1,8 +1,12 @@
 #ifndef ASM_X86__XEN_IOMMU_H
 
 #ifdef CONFIG_PCI_XEN
+extern void detect_xen_iommu(void);
 extern void xen_iommu_init(void);
 #else
+static inline void detect_xen_iommu(void)
+{
+}
 static inline void xen_iommu_init(void)
 {
 }
diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index 587cc6a..695f8a6 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -121,6 +121,8 @@ void __init pci_iommu_alloc(void)
 	 */
 	gart_iommu_hole_init();
 
+	detect_xen_iommu();
+
 	detect_calgary();
 
 	detect_intel_iommu();
@@ -277,7 +279,7 @@ static int __init pci_iommu_init(void)
 #endif
 
 	xen_iommu_init();
-	
+
 	calgary_iommu_init();
 
 	intel_iommu_init();
diff --git a/drivers/pci/xen-iommu.c b/drivers/pci/xen-iommu.c
index f65660a..2d727d1 100644
--- a/drivers/pci/xen-iommu.c
+++ b/drivers/pci/xen-iommu.c
@@ -39,7 +39,8 @@ do {							\
 
 static int max_dma_bits = 32;
 
-void xen_swiotlb_fixup(void *buf, size_t size, unsigned long nslabs)
+static void __init xen_swiotlb_alloc_fixup(void *buf, size_t size,
+					   unsigned long nslabs)
 {
 	int i, rc;
 	int dma_bits;
@@ -314,6 +315,15 @@ static struct dma_map_ops xen_swiotlb_dma_ops = {
 	.is_phys = 0,
 };
 
+void __init detect_xen_iommu(void)
+{
+	if (!xen_pv_domain())
+		return;
+
+	x86_swiotlb_force_mapping = &xen_swiotlb_force_mapping;
+	x86_swiotlb_alloc_fixup = &xen_swiotlb_alloc_fixup;
+}
+
 void __init xen_iommu_init(void)
 {
 	if (!xen_pv_domain())
@@ -324,8 +334,6 @@ void __init xen_iommu_init(void)
 	force_iommu = 0;
 	dma_ops = &xen_dma_ops;
 
-	x86_swiotlb_force_mapping = &xen_swiotlb_force_mapping;
-
 	if (swiotlb) {
 		printk(KERN_INFO "Xen: Enabling DMA fallback to swiotlb\n");
 		dma_ops = &xen_swiotlb_dma_ops;
diff --git a/include/xen/swiotlb.h b/include/xen/swiotlb.h
index 64137fa..83ec002 100644
--- a/include/xen/swiotlb.h
+++ b/include/xen/swiotlb.h
@@ -1,7 +1,6 @@
 #ifndef _XEN_SWIOTLB_H
 #define _XEN_SWIOTLB_H
 
-extern void xen_swiotlb_fixup(void *buf, size_t size, unsigned long nslabs);
 extern phys_addr_t xen_bus_to_phys(dma_addr_t daddr);
 extern dma_addr_t xen_phys_to_bus(phys_addr_t paddr);
 
-- 
1.5.6.5

[PATCH] swiotlb: make swiotlb phys<->bus translations architecture-specific

[Ian Campbell]

This moves swiotlb_bus_to_phys() and swiotlb_phys_to_bus() from
lib/swiotlb.c to arch/*/include/asm/dma-mapping.h. On x86 gart_to_phys
and phys_to_gart translations need an exported function for use from
drivers so introduce common x86_ variants to implement the interface
required by both.

The __weak swiotlb_bus_to_virt is not currently overriden by any
architecture therefore it can be made static again. I imagine the
ability to override phys<->bus translations will be sufficient for any
potential users anyhow, otherwise we can revisit this aspect.

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 |   13 +++++++++++++
 arch/x86/include/asm/agp.h          |    4 ++--
 arch/x86/include/asm/dma-mapping.h  |   15 +++++++++++++++
 arch/x86/kernel/pci-swiotlb.c       |   18 ++++++++++++++++++
 arch/x86/xen/pci-swiotlb.c          |   15 ---------------
 include/linux/swiotlb.h             |    5 -----
 lib/swiotlb.c                       |   14 +-------------
 7 files changed, 49 insertions(+), 35 deletions(-)

diff --git a/arch/ia64/include/asm/dma-mapping.h b/arch/ia64/include/asm/dma-mapping.h
index 9f9994d..667d04e 100644
--- a/arch/ia64/include/asm/dma-mapping.h
+++ b/arch/ia64/include/asm/dma-mapping.h
@@ -190,4 +190,17 @@ static inline int swiotlb_force_mapping(phys_addr_t paddr, size_t size)
 extern void *swiotlb_alloc_boot(size_t size, unsigned long nslabs);
 extern void *swiotlb_alloc(unsigned order, unsigned long nslabs);
 
+static inline dma_addr_t swiotlb_phys_to_bus(struct device *hwdev,
+					     phys_addr_t paddr)
+{
+	return paddr;
+}
+
+static inline phys_addr_t swiotlb_bus_to_phys(struct device *hwdev,
+					      dma_addr_t baddr)
+{
+	return baddr;
+}
+
+
 #endif /* _ASM_IA64_DMA_MAPPING_H */
diff --git a/arch/x86/include/asm/agp.h b/arch/x86/include/asm/agp.h
index d972b14..085de80 100644
--- a/arch/x86/include/asm/agp.h
+++ b/arch/x86/include/asm/agp.h
@@ -26,8 +26,8 @@
 #define flush_agp_cache() wbinvd()
 
 /* Convert a physical address to an address suitable for the GART. */
-#define phys_to_gart(x) swiotlb_phys_to_bus(NULL, (x))
-#define gart_to_phys(x) swiotlb_bus_to_phys(NULL, (x))
+#define phys_to_gart(x) x86_phys_to_bus(NULL, (x))
+#define gart_to_phys(x) x86_bus_to_phys(NULL, (x))
 
 /* GATT allocation. Returns/accepts GATT kernel virtual address. */
 #define alloc_gatt_pages(order)	({                                          \
diff --git a/arch/x86/include/asm/dma-mapping.h b/arch/x86/include/asm/dma-mapping.h
index 5a3180d..bccf196 100644
--- a/arch/x86/include/asm/dma-mapping.h
+++ b/arch/x86/include/asm/dma-mapping.h
@@ -328,4 +328,19 @@ extern void (*x86_swiotlb_alloc_fixup)(void *buf, size_t size,
 				       unsigned long nslabs);
 extern void *swiotlb_alloc_boot(size_t size, unsigned long nslabs);
 
+extern dma_addr_t (*x86_phys_to_bus)(struct device *hwdev, phys_addr_t paddr);
+extern phys_addr_t (*x86_bus_to_phys)(struct device *hwdev, dma_addr_t baddr);
+
+static inline dma_addr_t swiotlb_phys_to_bus(struct device *hwdev,
+					     phys_addr_t paddr)
+{
+	return x86_phys_to_bus(hwdev, paddr);
+}
+
+static inline phys_addr_t swiotlb_bus_to_phys(struct device *hwdev,
+					      dma_addr_t baddr)
+{
+	return x86_bus_to_phys(hwdev, baddr);
+}
+
 #endif
diff --git a/arch/x86/kernel/pci-swiotlb.c b/arch/x86/kernel/pci-swiotlb.c
index 7de9fdd..6cc1020 100644
--- a/arch/x86/kernel/pci-swiotlb.c
+++ b/arch/x86/kernel/pci-swiotlb.c
@@ -29,6 +29,24 @@ void * __init swiotlb_alloc_boot(size_t size, unsigned long nslabs)
 	return ret;
 }
 
+static dma_addr_t __x86_phys_to_bus(struct device *hwdev, phys_addr_t paddr)
+{
+	return paddr;
+}
+
+static phys_addr_t __x86_bus_to_phys(struct device *hwdev, dma_addr_t baddr)
+{
+	return baddr;
+}
+
+dma_addr_t (*x86_phys_to_bus)(struct device *hwdev,
+			      phys_addr_t paddr) = __x86_phys_to_bus;
+EXPORT_SYMBOL_GPL(x86_phys_to_bus);
+
+phys_addr_t (*x86_bus_to_phys)(struct device *hwdev,
+			       dma_addr_t baddr) = __x86_bus_to_phys;
+EXPORT_SYMBOL_GPL(x86_bus_to_phys);
+
 static void *x86_swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 					dma_addr_t *dma_handle, gfp_t flags)
 {
diff --git a/arch/x86/xen/pci-swiotlb.c b/arch/x86/xen/pci-swiotlb.c
index dcf2ef8..92f04a3 100644
--- a/arch/x86/xen/pci-swiotlb.c
+++ b/arch/x86/xen/pci-swiotlb.c
@@ -11,18 +11,3 @@
  * implementations in lib/swiotlb.c.
  */
 
-dma_addr_t swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr)
-{
-	if (xen_pv_domain())
-		return xen_phys_to_bus(paddr);
-
-	return paddr;
-}
-
-phys_addr_t swiotlb_bus_to_phys(struct device *hwdev, dma_addr_t baddr)
-{
-	if (xen_pv_domain())
-		return xen_bus_to_phys(baddr);
-
-	return baddr;
-}
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index be8b77d..b5b2245 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -24,11 +24,6 @@ struct scatterlist;
 extern void
 swiotlb_init(void);
 
-extern dma_addr_t swiotlb_phys_to_bus(struct device *hwdev,
-				      phys_addr_t address);
-extern phys_addr_t swiotlb_bus_to_phys(struct device *hwdev,
-				       dma_addr_t address);
-
 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 640c2f1..b5dcb68 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -114,25 +114,13 @@ setup_io_tlb_npages(char *str)
 __setup("swiotlb=", setup_io_tlb_npages);
 /* make io_tlb_overflow tunable too? */
 
-dma_addr_t __weak swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr)
-{
-	return paddr;
-}
-EXPORT_SYMBOL_GPL(swiotlb_phys_to_bus);
-
-phys_addr_t __weak swiotlb_bus_to_phys(struct device *hwdev, dma_addr_t baddr)
-{
-	return baddr;
-}
-EXPORT_SYMBOL_GPL(swiotlb_bus_to_phys);
-
 static dma_addr_t swiotlb_virt_to_bus(struct device *hwdev,
 				      volatile void *address)
 {
 	return swiotlb_phys_to_bus(hwdev, virt_to_phys(address));
 }
 
-void * __weak swiotlb_bus_to_virt(struct device *hwdev, dma_addr_t address)
+static void *swiotlb_bus_to_virt(struct device *hwdev, dma_addr_t address)
 {
 	return phys_to_virt(swiotlb_bus_to_phys(hwdev, address));
 }
-- 
1.5.6.5

Re: [PATCH] swiotlb: make swiotlb phys<->bus translations architecture-specific

[FUJITA Tomonori]

On Thu, 21 May 2009 17:15:27 +0100
Ian Campbell <ian.campbell@citrix.com> wrote:

> This moves swiotlb_bus_to_phys() and swiotlb_phys_to_bus() from
> lib/swiotlb.c to arch/*/include/asm/dma-mapping.h. On x86 gart_to_phys
> and phys_to_gart translations need an exported function for use from
> drivers so introduce common x86_ variants to implement the interface
> required by both.
> 
> The __weak swiotlb_bus_to_virt is not currently overriden by any
> architecture therefore it can be made static again. I imagine the
> ability to override phys<->bus translations will be sufficient for any
> potential users anyhow, otherwise we can revisit this aspect.
> 
> 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 |   13 +++++++++++++
>  arch/x86/include/asm/agp.h          |    4 ++--
>  arch/x86/include/asm/dma-mapping.h  |   15 +++++++++++++++
>  arch/x86/kernel/pci-swiotlb.c       |   18 ++++++++++++++++++
>  arch/x86/xen/pci-swiotlb.c          |   15 ---------------
>  include/linux/swiotlb.h             |    5 -----
>  lib/swiotlb.c                       |   14 +-------------
>  7 files changed, 49 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/ia64/include/asm/dma-mapping.h b/arch/ia64/include/asm/dma-mapping.h
> index 9f9994d..667d04e 100644
> --- a/arch/ia64/include/asm/dma-mapping.h
> +++ b/arch/ia64/include/asm/dma-mapping.h
> @@ -190,4 +190,17 @@ static inline int swiotlb_force_mapping(phys_addr_t paddr, size_t size)
>  extern void *swiotlb_alloc_boot(size_t size, unsigned long nslabs);
>  extern void *swiotlb_alloc(unsigned order, unsigned long nslabs);
>  
> +static inline dma_addr_t swiotlb_phys_to_bus(struct device *hwdev,
> +					     phys_addr_t paddr)
> +{
> +	return paddr;
> +}
> +
> +static inline phys_addr_t swiotlb_bus_to_phys(struct device *hwdev,
> +					      dma_addr_t baddr)
> +{
> +	return baddr;
> +}
> +
> +

As I wrote in another mail, adding swiotlb specific code in
dma-mapping.h is wrong.

Re: [PATCH] swiotlb: make swiotlb phys<->bus translations architecture-specific

[Ian Campbell]

On Fri, 2009-05-22 at 07:13 -0400, FUJITA Tomonori wrote:
> > +static inline phys_addr_t swiotlb_bus_to_phys(struct device *hwdev,
> > +                                           dma_addr_t baddr)
> > +{
> > +     return baddr;
> > +}
> > +
> > +
> 
> As I wrote in another mail, adding swiotlb specific code in
> dma-mapping.h is wrong.

They aren't really swiotlb specific, I just foolishly carried over the
old name, really I think they should be called just bus_to_phys and
phys_to_bus or something.

Ian.

[PATCH] swiotlb/xen: update xen swiotlb for phys<->bus API changes

[Ian Campbell]

These functions are now implemented via explicit hooks in
arch/x86/kernel/pci-swiotlb.c rather than as weak hooks in swiotlb.c

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>
---
 drivers/pci/xen-iommu.c |    6 ++++--
 include/xen/swiotlb.h   |    3 ---
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/xen-iommu.c b/drivers/pci/xen-iommu.c
index 2d727d1..02de48a 100644
--- a/drivers/pci/xen-iommu.c
+++ b/drivers/pci/xen-iommu.c
@@ -72,12 +72,12 @@ int xen_wants_swiotlb(void)
 	return xen_initial_domain();
 }
 
-dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
+static dma_addr_t xen_phys_to_bus(struct device *hwdev, phys_addr_t paddr)
 {
 	return phys_to_machine(XPADDR(paddr)).maddr;
 }
 
-phys_addr_t xen_bus_to_phys(dma_addr_t daddr)
+static phys_addr_t xen_bus_to_phys(struct device *hwdev, dma_addr_t daddr)
 {
 	return machine_to_phys(XMADDR(daddr)).paddr;
 }
@@ -322,6 +322,8 @@ void __init detect_xen_iommu(void)
 
 	x86_swiotlb_force_mapping = &xen_swiotlb_force_mapping;
 	x86_swiotlb_alloc_fixup = &xen_swiotlb_alloc_fixup;
+	x86_bus_to_phys = &xen_bus_to_phys;
+	x86_phys_to_bus = &xen_phys_to_bus;
 }
 
 void __init xen_iommu_init(void)
diff --git a/include/xen/swiotlb.h b/include/xen/swiotlb.h
index 83ec002..e6c7707 100644
--- a/include/xen/swiotlb.h
+++ b/include/xen/swiotlb.h
@@ -1,9 +1,6 @@
 #ifndef _XEN_SWIOTLB_H
 #define _XEN_SWIOTLB_H
 
-extern phys_addr_t xen_bus_to_phys(dma_addr_t daddr);
-extern dma_addr_t xen_phys_to_bus(phys_addr_t paddr);
-
 #ifdef CONFIG_PCI_XEN
 extern int xen_wants_swiotlb(void);
 #else
-- 
1.5.6.5

Re: [Xen-devel] Re: Where do we stand with the Xen patches?

[FUJITA Tomonori]

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)
 {

Re: Re: Where do we stand with the Xen patches?

[FUJITA Tomonori]

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)
 {

Re: Where do we stand with the Xen patches?

[Ian Campbell]

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"

Re: Where do we stand with the Xen patches?

[Ian Campbell]

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"

Re: Where do we stand with the Xen patches?

[devzero]

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

maybe this is iust a stupid comment (please forgive, iŽm no advanced kernel
hacker), but canŽt the code inserted by the patches and which changes the 
fastpath just #IFDEF`ed at the critical offsets ?  (as building a dom0 kernel is 
just another build target, isn`t it ?)

regards
roland
______________________________________________________
GRATIS für alle WEB.DE-Nutzer: Die maxdome Movie-FLAT!
Jetzt freischalten unter http://movieflat.web.de

Re: Where do we stand with the Xen patches?

[david]

On Sun, 17 May 2009, devzero@web.de wrote:

>>> 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
>
> maybe this is iust a stupid comment (please forgive, i?m no advanced kernel
> hacker), but can?t the code inserted by the patches and which changes the
> fastpath just #IFDEF`ed at the critical offsets ?  (as building a dom0 kernel is
> just another build target, isn`t it ?)

no, if dom0 is going to be widely deployed, it will be because the distros 
turn on dom0 support by default. as a result any penalties due to xen 
support will be felt by all users of those distros (even if they don't use 
xen)

David Lang

Re: Where do we stand with the Xen patches?

[Arjan van de Ven]

On Sun, 17 May 2009 12:25:38 -0700 (PDT)
david@lang.hm wrote:

> On Sun, 17 May 2009, devzero@web.de wrote:
> > maybe this is iust a stupid comment (please forgive, i?m no
> > advanced kernel hacker), but can?t the code inserted by the patches
> > and which changes the fastpath just #IFDEF`ed at the critical
> > offsets ?  (as building a dom0 kernel is just another build target,
> > isn`t it ?)
> 
> no, if dom0 is going to be widely deployed, it will be because the
> distros turn on dom0 support by default. as a result any penalties
> due to xen support will be felt by all users of those distros (even
> if they don't use xen)
> 

at minimum we need to split CONFIG_PARAVIRT up into 
"want to be nice to hypervisors" and "I want to be Xen Dom0";
they look to largely not overlap.... so lets not make the costs overlap
either.


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: Where do we stand with the Xen patches?

[devzero]

> >> 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
> >
> > maybe this is iust a stupid comment (please forgive, i?m no advanced kernel
> > hacker), but can?t the code inserted by the patches and which changes the
> > fastpath just #IFDEF`ed at the critical offsets ?  (as building a dom0 kernel is
> > just another build target, isn`t it ?)
> 
> no, if dom0 is going to be widely deployed, it will be because the distros 
> turn on dom0 support by default. as a result any penalties due to xen 
> support will be felt by all users of those distros (even if they don't use 
> xen)
> 
> David Lang

so what?

print a huge warning on boot that running dom0 for xen may affect performance and that 
you should better run a normal kernel instead if you don`t use xen , and you`re done.

or is maintaining two different kernel packages a problem?

if so, instead of using IFDEF`s, can`t the critical path`s being generously circumvented 
by default, (if, else...), needing some dom0 kernel bootparam to be activated (i.e. use
the kernel as dom0 kernel) ?

or is this too short-sighted view of the things ?

regards
roland




______________________________________________________
GRATIS für alle WEB.DE-Nutzer: Die maxdome Movie-FLAT!
Jetzt freischalten unter http://movieflat.web.de

Re: Where do we stand with the Xen patches?

[Jeremy Fitzhardinge]

devzero@web.de wrote:
> or is maintaining two different kernel packages a problem?
>   

Yes, distros hate the proliferation of kernel packages with different 
config options, partly because of the combinatorial explosion (32 vs 64, 
UP vs SMP, PAE vs non-PAE...).  An explicit design intent of all the Xen 
work is that it can be compile-time enabled without any (significant) 
effect on native performance, so that the decision to enable Xen doesn't 
have any downsides (either in terms of raw performance or maintenance of 
the kernel package).

> if so, instead of using IFDEF`s, can`t the critical path`s being generously circumvented 
> by default, (if, else...), needing some dom0 kernel bootparam to be activated (i.e. use
> the kernel as dom0 kernel) ?
>   
Well, broadly speaking, yes.  We try to avoid putting if/thens in 
critical paths, and where there are changes to hot patches, we use 
dynamic code patching to make it as efficient as possible.

    J

Re: Where do we stand with the Xen patches?

[Chris Mason]

On Sun, May 17, 2009 at 06:54:48PM -0700, Jeremy Fitzhardinge wrote:
> devzero@web.de wrote:
>> or is maintaining two different kernel packages a problem?
>>   
>
> Yes, distros hate the proliferation of kernel packages with different  
> config options, partly because of the combinatorial explosion (32 vs 64,  
> UP vs SMP, PAE vs non-PAE...).  An explicit design intent of all the Xen  
> work is that it can be compile-time enabled without any (significant)  
> effect on native performance, so that the decision to enable Xen doesn't  
> have any downsides (either in terms of raw performance or maintenance of  
> the kernel package).

There is a long list of CONFIG_* that had performance impacts when
enabled later had that impact tuned away.  Especially now that
the source of the performance problem is understood, it makes sense to
me to merge and then focus energy on fixing it in tree instead of
spending time maintaining the out of tree patches.

-chris