Re: [PATCH] remove fullflush and nofullflush in IOMMU generic option

[Joerg Roedel <joerg.roedel@amd.com>]

On Sat, Sep 20, 2008 at 02:09:21AM +0900, FUJITA Tomonori wrote:
> On Fri, 19 Sep 2008 18:45:41 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > On Sat, Sep 20, 2008 at 01:23:30AM +0900, FUJITA Tomonori wrote:
> > > This patch against tip/x86/iommu virtually reverts
> > > 2842e5bf3115193f05dc9dac20f940e7abf44c1a. But just reverting the
> > > commit breaks AMD IOMMU so this patch also includes some fixes.
> >  
> > NACK.
> > 
> > > The above commit adds new two options to x86 IOMMU generic kernel boot
> > > options, fullflush and nofullflush. But such change that affects all
> > > the IOMMUs needs more discussion (all IOMMU parties need the chance to
> > > discuss it):
> > 
> > It affects only IOMMUs which use the iommu_fullflush variable. This are
> > GART, which used it since ages, and AMD IOMMU which was introduced by
> > the above commit. It absolutly makes sense to have command line parameters
> > which make sense for more than one or most of the IOMMUs in x86 into
> > 'iommu='. Ingo agreed with this, see http://lkml.org/lkml/2008/6/30/155
> > I agree with that too. The commit you are trying to revert here was a
> > step into this direction.
> 
> The point is we can't remove or rename VT-d option for IOTLB batching
> because we already exported it for users.
> 
> I think that once we export a boot option to users, we can't remove or
> rename it. It's the user interface.
> 
> You have a different policy for the kernel boot option? You think we
> can change or rename it after exporting it?

No. But we can have the generic option in parallel. There is no reason
to remove the VT-d specific option.

> 
> Yeah, Intel IOMMU need to keep the current option for IOTLB batching
> but it also support fullflush. But we don't discuss anything with
> Intel developers. That's what I'm against.

Thats why my patch does not change VT-d code. So why do you send revert
patches and not starting this discussion? If Muli and the Intel people
disagree we can still revert it.

> > > 'nofullflush' definitely is pointless. 'nofullflush' option doesn't
> > > change any kernel behavior and it was added just for GART
> > > compatibility. We should not have such generic meaningless option just
> > > for GART.
> > 
> > So why do you keep it in this patch then?
> 
> As I wrote, I think that we can't remove the exported user interfacea

You keep this option for AMD IOMMU too. If you move it to AMD IOMMU code
then you can remove nofullflush there.

> 
> Please keep it for AMD option for now. Please send a patch to make it
> generic to other IOMMU people and give them a chance to discuss on
> it.
>

Ok, I will forward the patch to Intel VT-d maintainers and Muli to
discuss it. If they disagree we can revert the patch.

Joerg

-- 
           |           AMD Saxony Limited Liability Company & Co. KG
 Operating |         Wilschdorfer Landstr. 101, 01109 Dresden, Germany
 System    |                  Register Court Dresden: HRA 4896
 Research  |              General Partner authorized to represent:
 Center    |             AMD Saxony LLC (Wilmington, Delaware, US)
           | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy