Re: [GIT PULL] AMD IOMMU updates for 2.6.28

[FUJITA Tomonori]

On Fri, 19 Sep 2008 13:52:59 +0200
Joerg Roedel <joerg.roedel@xxxxxxx> wrote:

> On Fri, Sep 19, 2008 at 08:47:54PM +0900, FUJITA Tomonori wrote:
> > On Fri, 19 Sep 2008 20:23:50 +0900
> > FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> wrote:
> > 
> > > On Fri, 19 Sep 2008 13:02:40 +0200
> > > Joerg Roedel <joerg.roedel@xxxxxxx> wrote:
> > > 
> > > > On Fri, Sep 19, 2008 at 07:51:11PM +0900, FUJITA Tomonori wrote:
> > > > > On Fri, 19 Sep 2008 12:39:23 +0200
> > > > > Joerg Roedel <joerg.roedel@xxxxxxx> wrote:
> > > > > 
> > > > > > On Fri, Sep 19, 2008 at 07:28:13PM +0900, FUJITA Tomonori wrote:
> > > > > > > On Fri, 19 Sep 2008 12:07:11 +0200
> > > > > > > Joerg Roedel <joerg.roedel@xxxxxxx> wrote:
> > > > > > > 
> > > > > > > > Hi Ingo,
> > > > > > > > 
> > > > > > > > please pull the updates for the AMD IOMMU driver for the 2.6.28 merge
> > > > > > > > window. Most of the patches had been sent to LKML for review and got
> > > > > > > > comments. One patch touches the GART driver and was added after an
> > > > > > > > objection with a patch previously in this series. Please pull.
> > > > > > > > 
> > > > > > > > The following changes since commit 74546a8cd9a4e2c26a15a534f0be2f9cc08ae675:
> > > > > > > >   Ingo Molnar (1):
> > > > > > > >         Revert "fix warning in: "x86: HPET_MSI Initialise per-cpu HPET timers""
> > > > > > > > 
> > > > > > > > are available in the git repository at:
> > > > > > > > 
> > > > > > > >   git://git.kernel.org/pub/scm/linux/kernel/git/joro/linux-2.6-iommu.git iommu-updates-2.6.28
> > > > > > > > 
> > > > > > > > FUJITA Tomonori (1):
> > > > > > > >       AMD IOMMU: avoid unnecessary low zone allocation in alloc_coherent
> > > > > > > > 
> > > > > > > > Joerg Roedel (24):
> > > > > > > >       AMD IOMMU: check for invalid device pointers
> > > > > > > >       AMD IOMMU: move TLB flushing to the map/unmap helper functions
> > > > > > > >       x86: move GART TLB flushing options to generic code
> > > > > > > 
> > > > > > > As I wrote, I don't think that this GART patch is the right approach.
> > > > > > 
> > > > > > I don't thinks its clean to leave nofullflush in GART code and move
> > > > > > fullflush to generic code only.
> > > > > 
> > > > > It's not clean for GART but clean for other IOMMUs.
> > > > 
> > > > So its unclean anyway. It doesn't matter if for GART or for AMD IOMMU.
> > > > 
> > > > > > If we move something then both. But feel
> > > > > > free to submit a patch that removes the nofullflush option. I would not
> > > > > > disagree with that.
> > > > > 
> > > > > I'm not sure we can remove an option that we added. If we can, please
> > > > > remove it now. There is no point to make the meaningless option
> > > > > generic.
> > > > 
> > > > I don't added this option. I just moved it to generic code from the
> > > 
> > > You added it to the generic place. Moving it to the generic place
> > > means adding it to the generic place.
> > > 
> > > There is no difference in the end. Now 'nofullflush' is listed as the
> > > generic place.
> > > 
> > > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > > index c2e00ee..569527e 100644
> > > --- a/Documentation/kernel-parameters.txt
> > > +++ b/Documentation/kernel-parameters.txt
> > > @@ -888,6 +888,10 @@ and is between 256 and 4096 characters. It is defined in the file
> > >  		nomerge
> > >  		forcesac
> > >  		soft
> > > +		fullflush
> > > +			Flush IO/TLB at every deallocation
> > > +		nofullflush
> > > +			Flush IO/TLB only when addresses are reused (default)
> > 
> > And you don't need to add 'fullflush' to the generic place too.
> > 
> > 'fullflush' will be supported with only GART and AMD IOMMU. So adding
> > the description of it to both GART and AMD IOMMU should be fine.
> > 
> > 'fullflush' has the same meaning for both IOMMUs. That's nice
> > consistency, I think.
> 
> Huh? The whole point of this patch was to have a common option between
> IOMMUs to disable lazy IOTLB flushing. This was suggested by _you_ and
> the only reason I wrote this patch.

You misunderstand what I meant. I'm sorry if my explanation is not
clear.


> After this patch we can change other IOMMU implementations with lazy
> flushing to use that parameter too.

I'm not sure that Calgary wants to support such option. It always uses
lazy flushing.


What I don't like is that there is no consistency about the option
name for lazy flushing. It doesn't mean that we move the option to the
generic place.

Here's my first reply:

=
Would it be nice to have consistency of IOMMU parameters?

VT-d also has the kernel-boot option for this lazy flushing trick
though VT-d 'strict' option is more vague than 'unmap_flush'
=

What I meant that using the option name 'strict' that VT-d uses for
lazy flushing for AMD IOMMU would be better than introducing a new
option name, "unmap_flush" for AMD IOMMU though I don't think that
'strict' is the good name.


Seems 'fullflush' is better than 'strict'. So I think that it's better
to use 'fullflush' for AMD IMMU rather introducing a new name,
'unmap_flush'. But again, it doesn't mean that 'fullflush' moves to
the generic place.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/