[PATCH] remove fullflush and nofullflush in IOMMU generic option

[PATCH] remove fullflush and nofullflush in IOMMU generic option

[FUJITA Tomonori]

This patch against tip/x86/iommu virtually reverts
2842e5bf3115193f05dc9dac20f940e7abf44c1a. But just reverting the
commit breaks AMD IOMMU so this patch also includes some fixes.

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

http://lkml.org/lkml/2008/9/19/106

For me, adding these boot parameters doesn't make sense.

All the hardware IOMMUs could use 'fullflush' for lazy IOTLB flushing
but Calgary doesn't support it. Intel VT-d has the different option
for it (and we can't rename it). So it might be useful for only GART
and AMD IOMMU.

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

This patch removes the fullflush and nofullflush from the generic
kernel boot options and adds 'fullflush' support for AMD IOMMU.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 Documentation/kernel-parameters.txt       |    9 +++++----
 Documentation/x86/x86_64/boot-options.txt |    2 ++
 arch/x86/kernel/amd_iommu.c               |    4 ++--
 arch/x86/kernel/amd_iommu_init.c          |    5 ++++-
 arch/x86/kernel/pci-dma.c                 |   13 -------------
 arch/x86/kernel/pci-gart_64.c             |   13 +++++++++++++
 include/asm-x86/amd_iommu_types.h         |    6 ++++++
 include/asm-x86/iommu.h                   |    1 -
 8 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 569527e..4b9ee9b 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -284,6 +284,11 @@ and is between 256 and 4096 characters. It is defined in the file
 			isolate - enable device isolation (each device, as far
 			          as possible, will get its own protection
 			          domain)
+			fullflush - enable flushing of IO/TLB entries when
+				    they are unmapped. Otherwise they are
+				    flushed before they will be reused, which
+				    is a lot of faster
+
 	amd_iommu_size= [HW,X86-64]
 			Define the size of the aperture for the AMD IOMMU
 			driver. Possible values are:
@@ -888,10 +893,6 @@ 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)
 
 
 	intel_iommu=	[DMAR] Intel IOMMU driver (DMAR) option
diff --git a/Documentation/x86/x86_64/boot-options.txt b/Documentation/x86/x86_64/boot-options.txt
index 42c887b..72ffb53 100644
--- a/Documentation/x86/x86_64/boot-options.txt
+++ b/Documentation/x86/x86_64/boot-options.txt
@@ -229,6 +229,8 @@ IOMMU (input/output memory management unit)
   iommu options only relevant to the AMD GART hardware IOMMU:
     <size>             Set the size of the remapping area in bytes.
     allowed            Overwrite iommu off workarounds for specific chipsets.
+    fullflush          Flush IOMMU on each allocation (default).
+    nofullflush        Don't use IOMMU fullflush.
     leak               Turn on simple iommu leak tracing (only when
                        CONFIG_IOMMU_LEAK is on). Default number of leak pages
                        is 20.
diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index 853b614..523e113 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -958,7 +958,7 @@ static dma_addr_t __map_single(struct device *dev,
 	}
 	address += offset;
 
-	if (unlikely(dma_dom->need_flush && !iommu_fullflush)) {
+	if (unlikely(dma_dom->need_flush && !amd_iommu_unmap_flush)) {
 		iommu_flush_tlb(iommu, dma_dom->domain.id);
 		dma_dom->need_flush = false;
 	} else if (unlikely(iommu_has_npcache(iommu)))
@@ -995,7 +995,7 @@ static void __unmap_single(struct amd_iommu *iommu,
 
 	dma_ops_free_addresses(dma_dom, dma_addr, pages);
 
-	if (iommu_fullflush)
+	if (amd_iommu_unmap_flush)
 		iommu_flush_pages(iommu, dma_dom->domain.id, dma_addr, size);
 }
 
diff --git a/arch/x86/kernel/amd_iommu_init.c b/arch/x86/kernel/amd_iommu_init.c
index db0c83a..148fcfe 100644
--- a/arch/x86/kernel/amd_iommu_init.c
+++ b/arch/x86/kernel/amd_iommu_init.c
@@ -122,6 +122,7 @@ LIST_HEAD(amd_iommu_unity_map);		/* a list of required unity mappings
 					   we find in ACPI */
 unsigned amd_iommu_aperture_order = 26; /* size of aperture in power of 2 */
 int amd_iommu_isolate;			/* if 1, device isolation is enabled */
+bool amd_iommu_unmap_flush;		/* if true, flush on every unmap */
 
 LIST_HEAD(amd_iommu_list);		/* list of all AMD IOMMUs in the
 					   system */
@@ -1144,7 +1145,7 @@ int __init amd_iommu_init(void)
 	else
 		printk("disabled\n");
 
-	if (iommu_fullflush)
+	if (amd_iommu_unmap_flush)
 		printk(KERN_INFO "AMD IOMMU: IO/TLB flush on unmap enabled\n");
 	else
 		printk(KERN_INFO "AMD IOMMU: Lazy IO/TLB flushing enabled\n");
@@ -1214,6 +1215,8 @@ static int __init parse_amd_iommu_options(char *str)
 	for (; *str; ++str) {
 		if (strncmp(str, "isolate", 7) == 0)
 			amd_iommu_isolate = 1;
+		if (strncmp(str, "fullflush", 11) == 0)
+			amd_iommu_unmap_flush = true;
 	}
 
 	return 1;
diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index 6dae123..23882c4 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -16,15 +16,6 @@ EXPORT_SYMBOL(dma_ops);
 
 static int iommu_sac_force __read_mostly;
 
-/*
- * If this is disabled the IOMMU will use an optimized flushing strategy
- * of only flushing when an mapping is reused. With it true the GART is
- * flushed for every mapping. Problem is that doing the lazy flush seems
- * to trigger bugs with some popular PCI cards, in particular 3ware (but
- * has been also also seen with Qlogic at least).
- */
-int iommu_fullflush;
-
 #ifdef CONFIG_IOMMU_DEBUG
 int panic_on_overflow __read_mostly = 1;
 int force_iommu __read_mostly = 1;
@@ -180,10 +171,6 @@ static __init int iommu_setup(char *p)
 		}
 		if (!strncmp(p, "nomerge", 7))
 			iommu_merge = 0;
-		if (!strncmp(p, "fullflush", 8))
-			iommu_fullflush = 1;
-		if (!strncmp(p, "nofullflush", 11))
-			iommu_fullflush = 0;
 		if (!strncmp(p, "forcesac", 8))
 			iommu_sac_force = 1;
 		if (!strncmp(p, "allowdac", 8))
diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
index 508ef47..9739d56 100644
--- a/arch/x86/kernel/pci-gart_64.c
+++ b/arch/x86/kernel/pci-gart_64.c
@@ -45,6 +45,15 @@ static unsigned long iommu_pages;	/* .. and in pages */
 
 static u32 *iommu_gatt_base;		/* Remapping table */
 
+/*
+ * If this is disabled the IOMMU will use an optimized flushing strategy
+ * of only flushing when an mapping is reused. With it true the GART is
+ * flushed for every mapping. Problem is that doing the lazy flush seems
+ * to trigger bugs with some popular PCI cards, in particular 3ware (but
+ * has been also also seen with Qlogic at least).
+ */
+int iommu_fullflush = 1;
+
 /* Allocation bitmap for the remapping area: */
 static DEFINE_SPINLOCK(iommu_bitmap_lock);
 /* Guarded by iommu_bitmap_lock: */
@@ -892,6 +901,10 @@ void __init gart_parse_options(char *p)
 #endif
 	if (isdigit(*p) && get_option(&p, &arg))
 		iommu_size = arg;
+	if (!strncmp(p, "fullflush", 8))
+		iommu_fullflush = 1;
+	if (!strncmp(p, "nofullflush", 11))
+		iommu_fullflush = 0;
 	if (!strncmp(p, "noagp", 5))
 		no_agp = 1;
 	if (!strncmp(p, "noaperture", 10))
diff --git a/include/asm-x86/amd_iommu_types.h b/include/asm-x86/amd_iommu_types.h
index d90b66a..b308586 100644
--- a/include/asm-x86/amd_iommu_types.h
+++ b/include/asm-x86/amd_iommu_types.h
@@ -376,6 +376,12 @@ extern unsigned long *amd_iommu_pd_alloc_bitmap;
 /* will be 1 if device isolation is enabled */
 extern int amd_iommu_isolate;
 
+/*
+ * If true, the addresses will be flushed on unmap time, not when
+ * they are reused
+ */
+extern bool amd_iommu_unmap_flush;
+
 /* takes a PCI device id and prints it out in a readable form */
 static inline void print_devid(u16 devid, int nl)
 {
diff --git a/include/asm-x86/iommu.h b/include/asm-x86/iommu.h
index bcebc37..546ad31 100644
--- a/include/asm-x86/iommu.h
+++ b/include/asm-x86/iommu.h
@@ -7,7 +7,6 @@ extern struct dma_mapping_ops nommu_dma_ops;
 extern int force_iommu, no_iommu;
 extern int iommu_detected;
 extern int dmar_disabled;
-extern int iommu_fullflush;
 
 extern unsigned long iommu_num_pages(unsigned long addr, unsigned long len);
 
-- 
1.5.4.2

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

[Joerg Roedel]

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.

> http://lkml.org/lkml/2008/9/19/106
> 
> For me, adding these boot parameters doesn't make sense.

May be true for nofullflush. For fullflush it makes sense to have it in
the generic code since all x86 hardware IOMMUs except Calgary can make
use of it. 

> All the hardware IOMMUs could use 'fullflush' for lazy IOTLB flushing
> but Calgary doesn't support it. Intel VT-d has the different option
> for it (and we can't rename it). So it might be useful for only GART
> and AMD IOMMU.

We can keep the current Intel option and have iommu=fullflush in
parallel. It could also affect Intel IOMMU.

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

> This patch removes the fullflush and nofullflush from the generic
> kernel boot options and adds 'fullflush' support for AMD IOMMU.

I disagree. Keep it in the generic code.

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

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

[FUJITA Tomonori]

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?


> > http://lkml.org/lkml/2008/9/19/106
> > 
> > For me, adding these boot parameters doesn't make sense.
> 
> May be true for nofullflush. For fullflush it makes sense to have it in
> the generic code since all x86 hardware IOMMUs except Calgary can make
> use of it. 
> 
> > All the hardware IOMMUs could use 'fullflush' for lazy IOTLB flushing
> > but Calgary doesn't support it. Intel VT-d has the different option
> > for it (and we can't rename it). So it might be useful for only GART
> > and AMD IOMMU.
> 
> We can keep the current Intel option and have iommu=fullflush in
> parallel. It could also affect Intel IOMMU.

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.


> > '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 interface.


> > This patch removes the fullflush and nofullflush from the generic
> > kernel boot options and adds 'fullflush' support for AMD IOMMU.
> 
> I disagree. Keep it in the generic code.

We would have it in the generic code but the point is that you change
the generic code without any discussion with other IOMMU people,
Calgary and Intel developers.

After discussion, if we agree on having it in the generic code, it's
fine by me. But again, you changed the generic code without any
discussion with other IOMMU people. It's a wrong development process.

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.

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

[Joerg Roedel]

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

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

[Joerg Roedel]

On Sat, Sep 20, 2008 at 02:09:21AM +0900, FUJITA Tomonori wrote:
> 
> 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.
>

Btw, you already agreed with a generic iommu= parameter for lazy IO/TLB
flushing"


 > True. We should merge common parameters across IOMMUs into the
 > iommu= parameter some time in the future, I think. It would also be the
 > place for the IOMMU size parameter.

 Hmm, now is better than the future? I think that now you can add
 something like 'disable_batching_flush' as a common parameter and
 change AMD IOMMU to use it.

in http://lkml.org/lkml/2008/9/17/376

And since we already have a iommu=fullflush parameter it makes sense of
make it generic.

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

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

[FUJITA Tomonori]

On Fri, 19 Sep 2008 19:20:36 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

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

It's not a proper Linux development process for me.

A patch to make such change should be merged with Acked-by from other
developers. Making such change first then saying, "if you don't like
it, revert it" is not nice.


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

I'm not sure what you mean. You think that we can't change the
exported option, how can we remove nofullflush option?


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

Again, it's not a proper process for me.

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

[FUJITA Tomonori]

On Fri, 19 Sep 2008 19:30:04 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Sat, Sep 20, 2008 at 02:09:21AM +0900, FUJITA Tomonori wrote:
> > 
> > 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.
> >
> 
> Btw, you already agreed with a generic iommu= parameter for lazy IO/TLB
> flushing"
> 
> 
>  > True. We should merge common parameters across IOMMUs into the
>  > iommu= parameter some time in the future, I think. It would also be the
>  > place for the IOMMU size parameter.
> 
>  Hmm, now is better than the future? I think that now you can add
>  something like 'disable_batching_flush' as a common parameter and
>  change AMD IOMMU to use it.
> 
> in http://lkml.org/lkml/2008/9/17/376
> 
> And since we already have a iommu=fullflush parameter it makes sense of
> make it generic.

I'm not against fullflush but we need to discuss it with other people
before making the change.

Please,

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

[Joerg Roedel]

On Sat, Sep 20, 2008 at 02:34:15AM +0900, FUJITA Tomonori wrote:
> > You keep this option for AMD IOMMU too. If you move it to AMD IOMMU code
> > then you can remove nofullflush there.
> 
> I'm not sure what you mean. You think that we can't change the
> exported option, how can we remove nofullflush option?

You can keep it for GART. But as I already wrote its ok to remove it for
AMD IOMMU.

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

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

[Joerg Roedel]

On Sat, Sep 20, 2008 at 02:40:35AM +0900, FUJITA Tomonori wrote:
> On Fri, 19 Sep 2008 19:30:04 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > On Sat, Sep 20, 2008 at 02:09:21AM +0900, FUJITA Tomonori wrote:
> > > 
> > > 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.
> > >
> > 
> > Btw, you already agreed with a generic iommu= parameter for lazy IO/TLB
> > flushing"
> > 
> > 
> >  > True. We should merge common parameters across IOMMUs into the
> >  > iommu= parameter some time in the future, I think. It would also be the
> >  > place for the IOMMU size parameter.
> > 
> >  Hmm, now is better than the future? I think that now you can add
> >  something like 'disable_batching_flush' as a common parameter and
> >  change AMD IOMMU to use it.
> > 
> > in http://lkml.org/lkml/2008/9/17/376
> > 
> > And since we already have a iommu=fullflush parameter it makes sense of
> > make it generic.
> 
> I'm not against fullflush but we need to discuss it with other people
> before making the change.

Weird. Just 2 hours ago you wrote:

|http://lkml.org/lkml/2008/9/19/106
|
|For me, adding these boot parameters doesn't make sense.

Anyway, I wrote to the Intel and Calgary developers and asked them for
their opinion. If they have real objections I am the last person NACKing
your original patch in this thread again.
The reason why I queued this patch in AMD IOMMU updates was that I
didn't wanted to implement an option specificly for AMD IOMMU when there
will be a generic one soon. This is double work I prefered to do it
right in the first step. The change does not break anything on Intel and
Calgary.

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

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

[FUJITA Tomonori]

On Fri, 19 Sep 2008 19:46:05 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Sat, Sep 20, 2008 at 02:34:15AM +0900, FUJITA Tomonori wrote:
> > > You keep this option for AMD IOMMU too. If you move it to AMD IOMMU code
> > > then you can remove nofullflush there.
> > 
> > I'm not sure what you mean. You think that we can't change the
> > exported option, how can we remove nofullflush option?
> 
> You can keep it for GART. But as I already wrote its ok to remove it for
> AMD IOMMU.

My patch doesn't add nofullflush option to AMD IOMMU. So we don't
remove to it.

If my patch adds it to AMD IOMMU, it's a bug.

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

[FUJITA Tomonori]

On Fri, 19 Sep 2008 20:01:18 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Sat, Sep 20, 2008 at 02:40:35AM +0900, FUJITA Tomonori wrote:
> > On Fri, 19 Sep 2008 19:30:04 +0200
> > Joerg Roedel <joerg.roedel@amd.com> wrote:
> > 
> > > On Sat, Sep 20, 2008 at 02:09:21AM +0900, FUJITA Tomonori wrote:
> > > > 
> > > > 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.
> > > >
> > > 
> > > Btw, you already agreed with a generic iommu= parameter for lazy IO/TLB
> > > flushing"
> > > 
> > > 
> > >  > True. We should merge common parameters across IOMMUs into the
> > >  > iommu= parameter some time in the future, I think. It would also be the
> > >  > place for the IOMMU size parameter.
> > > 
> > >  Hmm, now is better than the future? I think that now you can add
> > >  something like 'disable_batching_flush' as a common parameter and
> > >  change AMD IOMMU to use it.
> > > 
> > > in http://lkml.org/lkml/2008/9/17/376
> > > 
> > > And since we already have a iommu=fullflush parameter it makes sense of
> > > make it generic.
> > 
> > I'm not against fullflush but we need to discuss it with other people
> > before making the change.
> 
> Weird. Just 2 hours ago you wrote:
> 
> |http://lkml.org/lkml/2008/9/19/106
> |
> |For me, adding these boot parameters doesn't make sense.

See:

http://lkml.org/lkml/2008/9/19/221


> Anyway, I wrote to the Intel and Calgary developers and asked them for
> their opinion. If they have real objections I am the last person NACKing
> your original patch in this thread again.

I think that I already expressed a real objection for nofullflush
twice though I'm not the maintainer of any IOMMUs.


> The reason why I queued this patch in AMD IOMMU updates was that I
> didn't wanted to implement an option specificly for AMD IOMMU when there
> will be a generic one soon. This is double work I prefered to do it

You were not sure that they will be generic before discussion.


> right in the first step. The change does not break anything on Intel and
> Calgary.

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

[Joerg Roedel]

On Sat, Sep 20, 2008 at 03:40:32AM +0900, FUJITA Tomonori wrote:
> On Fri, 19 Sep 2008 19:46:05 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > On Sat, Sep 20, 2008 at 02:34:15AM +0900, FUJITA Tomonori wrote:
> > > > You keep this option for AMD IOMMU too. If you move it to AMD IOMMU code
> > > > then you can remove nofullflush there.
> > > 
> > > I'm not sure what you mean. You think that we can't change the
> > > exported option, how can we remove nofullflush option?
> > 
> > You can keep it for GART. But as I already wrote its ok to remove it for
> > AMD IOMMU.
> 
> My patch doesn't add nofullflush option to AMD IOMMU. So we don't
> remove to it.
> 
> If my patch adds it to AMD IOMMU, it's a bug.

Ah ok, true. I looked at the wrong portion of the patch. Sorry.

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

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

[Joerg Roedel]

On Sat, Sep 20, 2008 at 03:48:11AM +0900, FUJITA Tomonori wrote:
> On Fri, 19 Sep 2008 20:01:18 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > On Sat, Sep 20, 2008 at 02:40:35AM +0900, FUJITA Tomonori wrote:
> > > On Fri, 19 Sep 2008 19:30:04 +0200
> > > Joerg Roedel <joerg.roedel@amd.com> wrote:
> > > 
> > > > On Sat, Sep 20, 2008 at 02:09:21AM +0900, FUJITA Tomonori wrote:
> > > > > 
> > > > > 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.
> > > > >
> > > > 
> > > > Btw, you already agreed with a generic iommu= parameter for lazy IO/TLB
> > > > flushing"
> > > > 
> > > > 
> > > >  > True. We should merge common parameters across IOMMUs into the
> > > >  > iommu= parameter some time in the future, I think. It would also be the
> > > >  > place for the IOMMU size parameter.
> > > > 
> > > >  Hmm, now is better than the future? I think that now you can add
> > > >  something like 'disable_batching_flush' as a common parameter and
> > > >  change AMD IOMMU to use it.
> > > > 
> > > > in http://lkml.org/lkml/2008/9/17/376
> > > > 
> > > > And since we already have a iommu=fullflush parameter it makes sense of
> > > > make it generic.
> > > 
> > > I'm not against fullflush but we need to discuss it with other people
> > > before making the change.
> > 
> > Weird. Just 2 hours ago you wrote:
> > 
> > |http://lkml.org/lkml/2008/9/19/106
> > |
> > |For me, adding these boot parameters doesn't make sense.
> 
> See:
> 
> http://lkml.org/lkml/2008/9/19/221

Removing nofullflush and moving fullflush to the generic code are two
different questions. You talk about the first and I talk about the
second here. We should make sure we talk about the same things
when we flame each other ;)


> 
> > Anyway, I wrote to the Intel and Calgary developers and asked them for
> > their opinion. If they have real objections I am the last person NACKing
> > your original patch in this thread again.
> 
> I think that I already expressed a real objection for nofullflush
> twice though I'm not the maintainer of any IOMMUs.

And I agree with that. But AMD IOMMU updates are not the right place to
remove it.
 
> > The reason why I queued this patch in AMD IOMMU updates was that I
> > didn't wanted to implement an option specificly for AMD IOMMU when there
> > will be a generic one soon. This is double work I prefered to do it
> 
> You were not sure that they will be generic before discussion.

Since Intel has lazy flushing too it is generic enough. Its only the
question if the Intel VT-d maintainer want to use it.

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

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

[FUJITA Tomonori]

On Fri, 19 Sep 2008 21:52:16 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Sat, Sep 20, 2008 at 03:48:11AM +0900, FUJITA Tomonori wrote:
> > On Fri, 19 Sep 2008 20:01:18 +0200
> > Joerg Roedel <joerg.roedel@amd.com> wrote:
> > 
> > > On Sat, Sep 20, 2008 at 02:40:35AM +0900, FUJITA Tomonori wrote:
> > > > On Fri, 19 Sep 2008 19:30:04 +0200
> > > > Joerg Roedel <joerg.roedel@amd.com> wrote:
> > > > 
> > > > > On Sat, Sep 20, 2008 at 02:09:21AM +0900, FUJITA Tomonori wrote:
> > > > > > 
> > > > > > 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.
> > > > > >
> > > > > 
> > > > > Btw, you already agreed with a generic iommu= parameter for lazy IO/TLB
> > > > > flushing"
> > > > > 
> > > > > 
> > > > >  > True. We should merge common parameters across IOMMUs into the
> > > > >  > iommu= parameter some time in the future, I think. It would also be the
> > > > >  > place for the IOMMU size parameter.
> > > > > 
> > > > >  Hmm, now is better than the future? I think that now you can add
> > > > >  something like 'disable_batching_flush' as a common parameter and
> > > > >  change AMD IOMMU to use it.
> > > > > 
> > > > > in http://lkml.org/lkml/2008/9/17/376
> > > > > 
> > > > > And since we already have a iommu=fullflush parameter it makes sense of
> > > > > make it generic.
> > > > 
> > > > I'm not against fullflush but we need to discuss it with other people
> > > > before making the change.
> > > 
> > > Weird. Just 2 hours ago you wrote:
> > > 
> > > |http://lkml.org/lkml/2008/9/19/106
> > > |
> > > |For me, adding these boot parameters doesn't make sense.
> > 
> > See:
> > 
> > http://lkml.org/lkml/2008/9/19/221
> 
> Removing nofullflush and moving fullflush to the generic code are two
> different questions. You talk about the first and I talk about the
> second here. We should make sure we talk about the same things
> when we flame each other ;)

And the root cause is that your patch does the two different things.

So please revert the changes. I've already send a patch to do
that. Please do these things in the proper way. If you think that you
are free to remove nofullflush from GART, send a patch to do that. If
you think that fullflush should be generic, send a patch to do it with
CC'ed to IOMMU people.

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

[Joerg Roedel]

On Sat, Sep 20, 2008 at 05:02:16AM +0900, FUJITA Tomonori wrote:
> On Fri, 19 Sep 2008 21:52:16 +0200
> Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > On Sat, Sep 20, 2008 at 03:48:11AM +0900, FUJITA Tomonori wrote:
> > > On Fri, 19 Sep 2008 20:01:18 +0200
> > > Joerg Roedel <joerg.roedel@amd.com> wrote:
> > > 
> > > > On Sat, Sep 20, 2008 at 02:40:35AM +0900, FUJITA Tomonori wrote:
> > > > > On Fri, 19 Sep 2008 19:30:04 +0200
> > > > > Joerg Roedel <joerg.roedel@amd.com> wrote:
> > > > > 
> > > > > > On Sat, Sep 20, 2008 at 02:09:21AM +0900, FUJITA Tomonori wrote:
> > > > > > > 
> > > > > > > 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.
> > > > > > >
> > > > > > 
> > > > > > Btw, you already agreed with a generic iommu= parameter for lazy IO/TLB
> > > > > > flushing"
> > > > > > 
> > > > > > 
> > > > > >  > True. We should merge common parameters across IOMMUs into the
> > > > > >  > iommu= parameter some time in the future, I think. It would also be the
> > > > > >  > place for the IOMMU size parameter.
> > > > > > 
> > > > > >  Hmm, now is better than the future? I think that now you can add
> > > > > >  something like 'disable_batching_flush' as a common parameter and
> > > > > >  change AMD IOMMU to use it.
> > > > > > 
> > > > > > in http://lkml.org/lkml/2008/9/17/376
> > > > > > 
> > > > > > And since we already have a iommu=fullflush parameter it makes sense of
> > > > > > make it generic.
> > > > > 
> > > > > I'm not against fullflush but we need to discuss it with other people
> > > > > before making the change.
> > > > 
> > > > Weird. Just 2 hours ago you wrote:
> > > > 
> > > > |http://lkml.org/lkml/2008/9/19/106
> > > > |
> > > > |For me, adding these boot parameters doesn't make sense.
> > > 
> > > See:
> > > 
> > > http://lkml.org/lkml/2008/9/19/221
> > 
> > Removing nofullflush and moving fullflush to the generic code are two
> > different questions. You talk about the first and I talk about the
> > second here. We should make sure we talk about the same things
> > when we flame each other ;)
> 
> And the root cause is that your patch does the two different things.
> 
> So please revert the changes. I've already send a patch to do
> that. Please do these things in the proper way. If you think that you
> are free to remove nofullflush from GART, send a patch to do that. If
> you think that fullflush should be generic, send a patch to do it with
> CC'ed to IOMMU people.

I don't think that there is a reason to revert the patch until
objections agains the generic iommu=fullflush come up. The patch does
not break anything and just moves the iommu=flush parameter (which is
already available) to pci-dma.c to make it useable by AMD IOMMU too.

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

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

[FUJITA Tomonori]

On Fri, 19 Sep 2008 22:19:09 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Sat, Sep 20, 2008 at 05:02:16AM +0900, FUJITA Tomonori wrote:
> > On Fri, 19 Sep 2008 21:52:16 +0200
> > Joerg Roedel <joerg.roedel@amd.com> wrote:
> > 
> > > On Sat, Sep 20, 2008 at 03:48:11AM +0900, FUJITA Tomonori wrote:
> > > > On Fri, 19 Sep 2008 20:01:18 +0200
> > > > Joerg Roedel <joerg.roedel@amd.com> wrote:
> > > > 
> > > > > On Sat, Sep 20, 2008 at 02:40:35AM +0900, FUJITA Tomonori wrote:
> > > > > > On Fri, 19 Sep 2008 19:30:04 +0200
> > > > > > Joerg Roedel <joerg.roedel@amd.com> wrote:
> > > > > > 
> > > > > > > On Sat, Sep 20, 2008 at 02:09:21AM +0900, FUJITA Tomonori wrote:
> > > > > > > > 
> > > > > > > > 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.
> > > > > > > >
> > > > > > > 
> > > > > > > Btw, you already agreed with a generic iommu= parameter for lazy IO/TLB
> > > > > > > flushing"
> > > > > > > 
> > > > > > > 
> > > > > > >  > True. We should merge common parameters across IOMMUs into the
> > > > > > >  > iommu= parameter some time in the future, I think. It would also be the
> > > > > > >  > place for the IOMMU size parameter.
> > > > > > > 
> > > > > > >  Hmm, now is better than the future? I think that now you can add
> > > > > > >  something like 'disable_batching_flush' as a common parameter and
> > > > > > >  change AMD IOMMU to use it.
> > > > > > > 
> > > > > > > in http://lkml.org/lkml/2008/9/17/376
> > > > > > > 
> > > > > > > And since we already have a iommu=fullflush parameter it makes sense of
> > > > > > > make it generic.
> > > > > > 
> > > > > > I'm not against fullflush but we need to discuss it with other people
> > > > > > before making the change.
> > > > > 
> > > > > Weird. Just 2 hours ago you wrote:
> > > > > 
> > > > > |http://lkml.org/lkml/2008/9/19/106
> > > > > |
> > > > > |For me, adding these boot parameters doesn't make sense.
> > > > 
> > > > See:
> > > > 
> > > > http://lkml.org/lkml/2008/9/19/221
> > > 
> > > Removing nofullflush and moving fullflush to the generic code are two
> > > different questions. You talk about the first and I talk about the
> > > second here. We should make sure we talk about the same things
> > > when we flame each other ;)
> > 
> > And the root cause is that your patch does the two different things.
> > 
> > So please revert the changes. I've already send a patch to do
> > that. Please do these things in the proper way. If you think that you
> > are free to remove nofullflush from GART, send a patch to do that. If
> > you think that fullflush should be generic, send a patch to do it with
> > CC'ed to IOMMU people.
> 
> I don't think that there is a reason to revert the patch until

Because you did things int the wrong way, I said again and again.

I can't see why you refuse to do the things in the proper way.


> objections agains the generic iommu=fullflush come up. The patch does
> not break anything and just moves the iommu=flush parameter (which is
> already available) to pci-dma.c to make it useable by AMD IOMMU too.

Breaking anything does mean that it's fine. My patch doesn't break
anything too.

I'm not against fullflush (as I said again and again). I guess that
it's the right move though it might be not so useful if VT-d doesn't
support it.

I'm against totally pointless nofullflush and the way you changed the
generic IOMMU code.

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

[Joerg Roedel]

On Sat, Sep 20, 2008 at 06:56:21AM +0900, FUJITA Tomonori wrote:
> 
> Because you did things int the wrong way, I said again and again.
> 
> I can't see why you refuse to do the things in the proper way.
> 
> 
> > objections agains the generic iommu=fullflush come up. The patch does
> > not break anything and just moves the iommu=flush parameter (which is
> > already available) to pci-dma.c to make it useable by AMD IOMMU too.
> 
> Breaking anything does mean that it's fine. My patch doesn't break
> anything too.
> 
> I'm not against fullflush (as I said again and again). I guess that
> it's the right move though it might be not so useful if VT-d doesn't
> support it.
> 
> I'm against totally pointless nofullflush and the way you changed the
> generic IOMMU code.

Then submit a patch that changes it to the version you prefer. I already
said that I am fine with the removal of nofullflush. But completly
reverting is the wrong way. For AMD IOMMU I want to use the
iommu=fullflush way because I want to reuse a parameter thats already
there. Thats why I am against your reverting patch.
So now I stop repeating my points again and again. EOD.

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

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

[FUJITA Tomonori]

On Sat, 20 Sep 2008 00:09:46 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Sat, Sep 20, 2008 at 06:56:21AM +0900, FUJITA Tomonori wrote:
> > 
> > Because you did things int the wrong way, I said again and again.
> > 
> > I can't see why you refuse to do the things in the proper way.
> > 
> > 
> > > objections agains the generic iommu=fullflush come up. The patch does
> > > not break anything and just moves the iommu=flush parameter (which is
> > > already available) to pci-dma.c to make it useable by AMD IOMMU too.
> > 
> > Breaking anything does mean that it's fine. My patch doesn't break
> > anything too.
> > 
> > I'm not against fullflush (as I said again and again). I guess that
> > it's the right move though it might be not so useful if VT-d doesn't
> > support it.
> > 
> > I'm against totally pointless nofullflush and the way you changed the
> > generic IOMMU code.
> 
> Then submit a patch that changes it to the version you prefer. I already
> said that I am fine with the removal of nofullflush. But completly

I don't know it's acceptable if removing the exported interface even
if the maintainer of it wants to remove it and it's totally
meaningless.


> reverting is the wrong way. For AMD IOMMU I want to use the
> iommu=fullflush way because I want to reuse a parameter thats already
> there. Thats why I am against your reverting patch.
> So now I stop repeating my points again and again. EOD.

I understood you want to use iommu=fullflush but you can't touch the
generic code without any discussion.

And even if everyone is happy about the change, it's much better to
start from scratch rather than try to fix the things done in the wrong
way.

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

[Joerg Roedel]

On Sat, Sep 20, 2008 at 07:18:11AM +0900, FUJITA Tomonori wrote:
> I don't know it's acceptable if removing the exported interface even
> if the maintainer of it wants to remove it and it's totally
> meaningless.
> 
> 
> > reverting is the wrong way. For AMD IOMMU I want to use the
> > iommu=fullflush way because I want to reuse a parameter thats already
> > there. Thats why I am against your reverting patch.
> > So now I stop repeating my points again and again. EOD.
> 
> I understood you want to use iommu=fullflush but you can't touch the
> generic code without any discussion.
> 
> And even if everyone is happy about the change, it's much better to
> start from scratch rather than try to fix the things done in the wrong
> way.

Just sleep a night about the discussions for this issue and count then
how often you changed your opinions and points. It all started with a
objection from you against 'nofullflush' and ended in that you want to
explain me the develpment process. And then think again if this
unimportant minor change was worth all that ridiculous flaming.

Good night,

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

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

[FUJITA Tomonori]

On Sat, 20 Sep 2008 00:39:51 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Sat, Sep 20, 2008 at 07:18:11AM +0900, FUJITA Tomonori wrote:
> > I don't know it's acceptable if removing the exported interface even
> > if the maintainer of it wants to remove it and it's totally
> > meaningless.
> > 
> > 
> > > reverting is the wrong way. For AMD IOMMU I want to use the
> > > iommu=fullflush way because I want to reuse a parameter thats already
> > > there. Thats why I am against your reverting patch.
> > > So now I stop repeating my points again and again. EOD.
> > 
> > I understood you want to use iommu=fullflush but you can't touch the
> > generic code without any discussion.
> > 
> > And even if everyone is happy about the change, it's much better to
> > start from scratch rather than try to fix the things done in the wrong
> > way.
> 
> Just sleep a night about the discussions for this issue and count then
> how often you changed your opinions and points. It all started with a
> objection from you against 'nofullflush' and ended in that you want to
> explain me the develpment process. And then think again if this
> unimportant minor change was worth all that ridiculous flaming.

Your patch made two changes to the interfaces exported to users, which
we can't change in the future. And they are the changes to the
interface that all IOMMUs can use. Any changes to the interfaces
exported to users are always very important for me because we can't
change or remove them later. I think that we should make such changes
after other developers agree that making the changes is a good idea.

But seems that you have a different opinion about changes to the
interfaces exported to users. Then I can see why we can't agree about
these changes.

If Ingo doesn't marge my patch to revert your changes to the
interfaces exported for users, then I'll fix only 'nofullflush'
option, which is clearly a bad change, as you admitted. I'll move
'nofullflush' back to GART as before. If you think that you can remove
the option, please send your patch to remove it because I don't think
that any changes to the existing exported interface is a good idea and
I just try to fix the problem that your patch introduced. Removing
'nofullflush' is a different topic.

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

[Ingo Molnar]

* FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> 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.
> 
> 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):
> 
> http://lkml.org/lkml/2008/9/19/106
> 
> For me, adding these boot parameters doesn't make sense.
> 
> All the hardware IOMMUs could use 'fullflush' for lazy IOTLB flushing
> but Calgary doesn't support it. Intel VT-d has the different option
> for it (and we can't rename it). [...]

Well if the new option is desired, you dont have to rename the old 
option - just alias it to the new too and deprecate the old one 
eventually. Boot options are not really ABIs in the traditional sense 
anyway.

But, in general, it's pretty pointless to add boot options for anything 
but debugging - nobody will use them unless there's a _problem_ with the 
default. So the right approach is to not add boot options and to make 
sure that the defaults are sane and intelligent.

So could we work towards removing unnecessary boot options please? _If_ 
we want any strategy switch then that should be runtime anyway - nobody 
sane will reboot a server just to tune it slightly, and no distro will 
litter the boot commandline with hardware dependent tunings either. So 
it's only the default that matters, plus boot parameters for debugging.

	Ingo

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

[FUJITA Tomonori]

On Sat, 20 Sep 2008 08:00:59 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> 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.
> > 
> > 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):
> > 
> > http://lkml.org/lkml/2008/9/19/106
> > 
> > For me, adding these boot parameters doesn't make sense.
> > 
> > All the hardware IOMMUs could use 'fullflush' for lazy IOTLB flushing
> > but Calgary doesn't support it. Intel VT-d has the different option
> > for it (and we can't rename it). [...]
> 
> Well if the new option is desired, you dont have to rename the old 
> option - just alias it to the new too and deprecate the old one
> eventually. Boot options are not really ABIs in the traditional sense 
> anyway.

I see. I thought that the similar rule is applied to boot options and
ABIs.


> But, in general, it's pretty pointless to add boot options for anything 
> but debugging - nobody will use them unless there's a _problem_ with the 
> default. So the right approach is to not add boot options and to make 
> sure that the defaults are sane and intelligent.

In general (or ideally), I agreed. But we can't do that in some cases.

And IOMMUs has some options for non general cases. For example, the
option, we are talking about, 'fullflush' for disabling lazy IOTLB
flushing, has no correct setting for everyone. It's kinda policy. The
boot option changes the policy that the IOMMU maintainer set by
default.


> So could we work towards removing unnecessary boot options please? _If_ 
> we want any strategy switch then that should be runtime anyway - nobody 
> sane will reboot a server just to tune it slightly, and no distro will 
> litter the boot commandline with hardware dependent tunings either. So 
> it's only the default that matters, plus boot parameters for debugging.

Ok, though I still think that deprecating and removing the existing
kernel parameter is bad for users.


Joerg's patch does two things, adding fullflush and nofullflush to the
generic option. The former needs more discussion though it might be a
correct idea. The latter is clearly wrong (as Joerg agreed).

I sent you the patch to revert his patch. But If you like to fix
things in a different way, then it's fine.

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

[Ingo Molnar]

* FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> > But, in general, it's pretty pointless to add boot options for 
> > anything but debugging - nobody will use them unless there's a 
> > _problem_ with the default. So the right approach is to not add boot 
> > options and to make sure that the defaults are sane and intelligent.
> 
> In general (or ideally), I agreed. But we can't do that in some cases.
> 
> And IOMMUs has some options for non general cases. For example, the 
> option, we are talking about, 'fullflush' for disabling lazy IOTLB 
> flushing, has no correct setting for everyone. It's kinda policy. The 
> boot option changes the policy that the IOMMU maintainer set by 
> default.

yes. But boot options really have a lot less relevance in practice. In 
99.99% of cases they are only used if things go wrong. They are almost 
never used to tune production systems. (yes, there are exceptions - in 
0.01% of the cases)

so ... i'd really suggest for you to get the bootup defaults right, and 
then just to give enough boot option flexibility to turn something off 
that might break in practice.

Otherwise, dont bother - if you want to offer advanced performance 
tuning then at _minimum_ it should be debugfs or sysctl exposed. It's 
totally PITA to performance-tune a system via a boot option.

> > So could we work towards removing unnecessary boot options please? _If_ 
> > we want any strategy switch then that should be runtime anyway - nobody 
> > sane will reboot a server just to tune it slightly, and no distro will 
> > litter the boot commandline with hardware dependent tunings either. So 
> > it's only the default that matters, plus boot parameters for debugging.
> 
> Ok, though I still think that deprecating and removing the existing 
> kernel parameter is bad for users.

yes - the solution could be as easy as to add back that single option to 
the affected iommu alone. (i.e. create a compatibility alias in essence)

> Joerg's patch does two things, adding fullflush and nofullflush to the 
> generic option. The former needs more discussion though it might be a 
> correct idea. The latter is clearly wrong (as Joerg agreed).
> 
> I sent you the patch to revert his patch. But If you like to fix 
> things in a different way, then it's fine.

well, i'd like you two to agree on how to proceed. Could you send a 
patch please that adds back the option that Joerg's patch removed? I 
certainly dont argue with the point that we should preserve existing 
options.

	Ingo

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

[Joerg Roedel]

On Mon, Sep 22, 2008 at 01:17:30PM +0200, Ingo Molnar wrote:
> well, i'd like you two to agree on how to proceed. Could you send a 
> patch please that adds back the option that Joerg's patch removed? I 
> certainly dont argue with the point that we should preserve existing 
> options.

I didn't removed any option. I just moved them from GART to the generic
code. I am fine with every solution that keeps iommu=fullflush usable
for AMD IOMMU (except maybe an amd_iommu_parse_options hook in the
generic code like its there for GART).

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

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

[FUJITA Tomonori]

On Mon, 22 Sep 2008 13:17:30 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> 
> > > But, in general, it's pretty pointless to add boot options for 
> > > anything but debugging - nobody will use them unless there's a 
> > > _problem_ with the default. So the right approach is to not add boot 
> > > options and to make sure that the defaults are sane and intelligent.
> > 
> > In general (or ideally), I agreed. But we can't do that in some cases.
> > 
> > And IOMMUs has some options for non general cases. For example, the 
> > option, we are talking about, 'fullflush' for disabling lazy IOTLB 
> > flushing, has no correct setting for everyone. It's kinda policy. The 
> > boot option changes the policy that the IOMMU maintainer set by 
> > default.
> 
> yes. But boot options really have a lot less relevance in practice. In 
> 99.99% of cases they are only used if things go wrong. They are almost 
> never used to tune production systems. (yes, there are exceptions - in 
> 0.01% of the cases)
> 
> so ... i'd really suggest for you to get the bootup defaults right, and 
> then just to give enough boot option flexibility to turn something off 
> that might break in practice.

Well, in general we agreed, I think. We should get the bootup defaults
right but we can't do that in some cases.

Another example about x86 IOMMU is a kernel couldn't even know using
an IOMMU is good for users or not.

In the case of max_pfn < MAX_DMA32_PFN, probably, the majority of
users doesn't want IOMMU since IOMMU leads performance drop. But if an
user plans to use something like kvm, he might want to enable VT-d (or
AMD IOMMU) for something like a passthrough feature.


Well, anyway, we don't need to discuss this issue further.

 
> Otherwise, dont bother - if you want to offer advanced performance 
> tuning then at _minimum_ it should be debugfs or sysctl exposed. It's 
> totally PITA to performance-tune a system via a boot option.

I guess that there are some things that a kernel can set up only
at startup.


> > > So could we work towards removing unnecessary boot options please? _If_ 
> > > we want any strategy switch then that should be runtime anyway - nobody 
> > > sane will reboot a server just to tune it slightly, and no distro will 
> > > litter the boot commandline with hardware dependent tunings either. So 
> > > it's only the default that matters, plus boot parameters for debugging.
> > 
> > Ok, though I still think that deprecating and removing the existing 
> > kernel parameter is bad for users.
> 
> yes - the solution could be as easy as to add back that single option to 
> the affected iommu alone. (i.e. create a compatibility alias in essence)
> 
> > Joerg's patch does two things, adding fullflush and nofullflush to the 
> > generic option. The former needs more discussion though it might be a 
> > correct idea. The latter is clearly wrong (as Joerg agreed).
> > 
> > I sent you the patch to revert his patch. But If you like to fix 
> > things in a different way, then it's fine.
> 
> well, i'd like you two to agree on how to proceed.

I'm not sure we can because he has a quite different opinion on this,
he thinks that the IOMMU parameters are unimportant and minor issue.


> Could you send a 
> patch please that adds back the option that Joerg's patch removed? I 
> certainly dont argue with the point that we should preserve existing 
> options.

Joerg's patch adds two generic IOMMU parameters (he said move two GART
parameters to generic but the result is same).

I'm against adding new generic IOMMU parameters without serious
consideration. Joerg thinks that we can clean up the parameters like
adding some for 2.6.28, then add some for 2.6.X, then rename some, in
an ad-hoc way.

That's exactly we have done in the past. As we saw in another thread
[1], now the IOMMUs parameters are too complicated so that even IOMMU
maintainers can't understand them properly. A simple task like
disabling IOMMUs is too complicated. Especially, adding a generic
IOMMU parameter needs serious consideration since it leads lots of new
combinations with IOMMUs specific parameters.

Unlike the past, we support many IOMMUs now. We should have a better
idea about what we need. I think that we need to discuss and serious
consideration of the last picture of all the IOMMUs parameters we need
BEFORE starting to modify (add, rename, delete) the current IOMMU
parameters. Otherwise, we would have the similar situation as we are
in now.

That's why I asked you to the patch to revert the generic IOMMU
parameters he added.


[1] http://lkml.org/lkml/2008/9/22/154

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

[Joerg Roedel]

On Tue, Sep 23, 2008 at 12:25:24AM +0900, FUJITA Tomonori wrote:
> 
> I'm not sure we can because he has a quite different opinion on this,
> he thinks that the IOMMU parameters are unimportant and minor issue.
> 

Stop your personal attacks and come down. I've never said that iommu
parameters are unimportant. The _move_ of the iommu=[no]fullflush to
generic code is a minor issue in my opinion. For the user it changes
nothing. There was an iommu=fullflush parameter before and now there is
also an iommu=fullflush parameter. The only difference is that it now
affects AMD IOMMU too.

> I'm against adding new generic IOMMU parameters without serious
> consideration. Joerg thinks that we can clean up the parameters like
> adding some for 2.6.28, then add some for 2.6.X, then rename some, in
> an ad-hoc way.

Again. Stop your personal attacks.

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

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

[FUJITA Tomonori]

On Mon, 22 Sep 2008 18:23:14 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Tue, Sep 23, 2008 at 12:25:24AM +0900, FUJITA Tomonori wrote:
> > 
> > I'm not sure we can because he has a quite different opinion on this,
> > he thinks that the IOMMU parameters are unimportant and minor issue.
> > 
> 
> Stop your personal attacks and come down. I've never said that iommu
> parameters are unimportant. The _move_ of the iommu=[no]fullflush to
> generic code is a minor issue in my opinion. For the user it changes
> nothing. There was an iommu=fullflush parameter before and now there is
> also an iommu=fullflush parameter. The only difference is that it now
> affects AMD IOMMU too.

I have no intention to attack you. I just described what you did.

You moved two IOMMU parameters to generic code, they interact with all
other IOMMUs specific parameters.

If you add parameters to AMD IOMMU, they interact with only AMD IOMMU.

That's a huge difference. You might think that the two parameters are
minor but it would turn out that they are not. What we know now is
that the changes you make affect all the IOMMUs. It's not a minor
issue.


> > I'm against adding new generic IOMMU parameters without serious
> > consideration. Joerg thinks that we can clean up the parameters like
> > adding some for 2.6.28, then add some for 2.6.X, then rename some, in
> > an ad-hoc way.
> 
> Again. Stop your personal attacks.

You added new generic fullflush and nofullflush parameters and
suggested to remove nofullflush parameter without discussing any plan
about further IOMMU parameter changes. We don't even start to discuss
a plan to how to clean up the IOMMU parameters.

I just described what you did.


The patch that I sent doesn't make any changes for you. GART works as
before and AMD IOMMU takes a new parameter, fullflush, as you want. It
does the exact same thing that the first version of your patchset for
2.6.28.

So please let me apply it then we can stop this discussion.

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

[Joerg Roedel]

On Tue, Sep 23, 2008 at 01:51:05AM +0900, FUJITA Tomonori wrote:
> The patch that I sent doesn't make any changes for you. GART works as
> before and AMD IOMMU takes a new parameter, fullflush, as you want. It
> does the exact same thing that the first version of your patchset for
> 2.6.28.
> 
> So please let me apply it then we can stop this discussion.

Ok, so it was not nice from me to let Ingo pull this patch without your
ack, I already admitted. To end this discussion and get to productive
discussions, Ingo, please apply this patch. I hope we will work together
on a final solution for the problem.

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

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

[Ingo Molnar]

* Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Tue, Sep 23, 2008 at 01:51:05AM +0900, FUJITA Tomonori wrote:
> > The patch that I sent doesn't make any changes for you. GART works as
> > before and AMD IOMMU takes a new parameter, fullflush, as you want. It
> > does the exact same thing that the first version of your patchset for
> > 2.6.28.
> > 
> > So please let me apply it then we can stop this discussion.
> 
> Ok, so it was not nice from me to let Ingo pull this patch without 
> your ack, I already admitted. To end this discussion and get to 
> productive discussions, Ingo, please apply this patch. I hope we will 
> work together on a final solution for the problem.

applied Fujita's patch below to tip/x86/iommu that reverts those 
options, thanks guys!

i'm wondering how we should proceed. For debug, iommu=off is certainly 
good enough - all kernels are supposed to work out of box on all hw, and 
every other result is a regression that must be fixed.

For performance tuning, it probably makes sense for developers to tune 
IOMMU details (so that the bootup defaults can be improved - this is not 
really something that should be done on a workload basis), but for that 
IMO a /debug interface is a lot more useful than rather intrusive (and 
inflexible) boot options.

What do you think about a debugfs based tuning of various IOMMU details? 
Maybe even the active driver could be changed - say from AMD-IOMMU to 
GART, or to off.

Flipping IOMMU state on a running system means a whole lot of new 
complexities, but it might be rather useful. It's useful for testing as 
well, obviously.

	Ingo

------------------->
>From afa9fdc2f5f8e4d98f3e77bfa204412cbc181346 Mon Sep 17 00:00:00 2001
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Date: Sat, 20 Sep 2008 01:23:30 +0900
Subject: [PATCH] iommu: remove fullflush and nofullflush in IOMMU generic option

This patch against tip/x86/iommu virtually reverts
2842e5bf3115193f05dc9dac20f940e7abf44c1a. But just reverting the
commit breaks AMD IOMMU so this patch also includes some fixes.

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

http://lkml.org/lkml/2008/9/19/106

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Acked-by: Joerg Roedel <joerg.roedel@amd.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 Documentation/kernel-parameters.txt       |    9 +++++----
 Documentation/x86/x86_64/boot-options.txt |    2 ++
 arch/x86/kernel/amd_iommu.c               |    4 ++--
 arch/x86/kernel/amd_iommu_init.c          |    5 ++++-
 arch/x86/kernel/pci-dma.c                 |   13 -------------
 arch/x86/kernel/pci-gart_64.c             |   13 +++++++++++++
 include/asm-x86/amd_iommu_types.h         |    6 ++++++
 include/asm-x86/iommu.h                   |    1 -
 8 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 40066ce..040ce30 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -284,6 +284,11 @@ and is between 256 and 4096 characters. It is defined in the file
 			isolate - enable device isolation (each device, as far
 			          as possible, will get its own protection
 			          domain)
+			fullflush - enable flushing of IO/TLB entries when
+				    they are unmapped. Otherwise they are
+				    flushed before they will be reused, which
+				    is a lot of faster
+
 	amd_iommu_size= [HW,X86-64]
 			Define the size of the aperture for the AMD IOMMU
 			driver. Possible values are:
@@ -893,10 +898,6 @@ 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)
 
 
 	intel_iommu=	[DMAR] Intel IOMMU driver (DMAR) option
diff --git a/Documentation/x86/x86_64/boot-options.txt b/Documentation/x86/x86_64/boot-options.txt
index c83c8e4..b0c7b6c 100644
--- a/Documentation/x86/x86_64/boot-options.txt
+++ b/Documentation/x86/x86_64/boot-options.txt
@@ -233,6 +233,8 @@ IOMMU (input/output memory management unit)
   iommu options only relevant to the AMD GART hardware IOMMU:
     <size>             Set the size of the remapping area in bytes.
     allowed            Overwrite iommu off workarounds for specific chipsets.
+    fullflush          Flush IOMMU on each allocation (default).
+    nofullflush        Don't use IOMMU fullflush.
     leak               Turn on simple iommu leak tracing (only when
                        CONFIG_IOMMU_LEAK is on). Default number of leak pages
                        is 20.
diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index 70537d1..c192121 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -948,7 +948,7 @@ static dma_addr_t __map_single(struct device *dev,
 	}
 	address += offset;
 
-	if (unlikely(dma_dom->need_flush && !iommu_fullflush)) {
+	if (unlikely(dma_dom->need_flush && !amd_iommu_unmap_flush)) {
 		iommu_flush_tlb(iommu, dma_dom->domain.id);
 		dma_dom->need_flush = false;
 	} else if (unlikely(iommu_has_npcache(iommu)))
@@ -985,7 +985,7 @@ static void __unmap_single(struct amd_iommu *iommu,
 
 	dma_ops_free_addresses(dma_dom, dma_addr, pages);
 
-	if (iommu_fullflush)
+	if (amd_iommu_unmap_flush)
 		iommu_flush_pages(iommu, dma_dom->domain.id, dma_addr, size);
 }
 
diff --git a/arch/x86/kernel/amd_iommu_init.c b/arch/x86/kernel/amd_iommu_init.c
index db0c83a..148fcfe 100644
--- a/arch/x86/kernel/amd_iommu_init.c
+++ b/arch/x86/kernel/amd_iommu_init.c
@@ -122,6 +122,7 @@ LIST_HEAD(amd_iommu_unity_map);		/* a list of required unity mappings
 					   we find in ACPI */
 unsigned amd_iommu_aperture_order = 26; /* size of aperture in power of 2 */
 int amd_iommu_isolate;			/* if 1, device isolation is enabled */
+bool amd_iommu_unmap_flush;		/* if true, flush on every unmap */
 
 LIST_HEAD(amd_iommu_list);		/* list of all AMD IOMMUs in the
 					   system */
@@ -1144,7 +1145,7 @@ int __init amd_iommu_init(void)
 	else
 		printk("disabled\n");
 
-	if (iommu_fullflush)
+	if (amd_iommu_unmap_flush)
 		printk(KERN_INFO "AMD IOMMU: IO/TLB flush on unmap enabled\n");
 	else
 		printk(KERN_INFO "AMD IOMMU: Lazy IO/TLB flushing enabled\n");
@@ -1214,6 +1215,8 @@ static int __init parse_amd_iommu_options(char *str)
 	for (; *str; ++str) {
 		if (strncmp(str, "isolate", 7) == 0)
 			amd_iommu_isolate = 1;
+		if (strncmp(str, "fullflush", 11) == 0)
+			amd_iommu_unmap_flush = true;
 	}
 
 	return 1;
diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index d2f2c01..0a1408a 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -16,15 +16,6 @@ EXPORT_SYMBOL(dma_ops);
 
 static int iommu_sac_force __read_mostly;
 
-/*
- * If this is disabled the IOMMU will use an optimized flushing strategy
- * of only flushing when an mapping is reused. With it true the GART is
- * flushed for every mapping. Problem is that doing the lazy flush seems
- * to trigger bugs with some popular PCI cards, in particular 3ware (but
- * has been also also seen with Qlogic at least).
- */
-int iommu_fullflush;
-
 #ifdef CONFIG_IOMMU_DEBUG
 int panic_on_overflow __read_mostly = 1;
 int force_iommu __read_mostly = 1;
@@ -180,10 +171,6 @@ static __init int iommu_setup(char *p)
 		}
 		if (!strncmp(p, "nomerge", 7))
 			iommu_merge = 0;
-		if (!strncmp(p, "fullflush", 8))
-			iommu_fullflush = 1;
-		if (!strncmp(p, "nofullflush", 11))
-			iommu_fullflush = 0;
 		if (!strncmp(p, "forcesac", 8))
 			iommu_sac_force = 1;
 		if (!strncmp(p, "allowdac", 8))
diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
index 3dcb1ad..9e390f1 100644
--- a/arch/x86/kernel/pci-gart_64.c
+++ b/arch/x86/kernel/pci-gart_64.c
@@ -45,6 +45,15 @@ static unsigned long iommu_pages;	/* .. and in pages */
 
 static u32 *iommu_gatt_base;		/* Remapping table */
 
+/*
+ * If this is disabled the IOMMU will use an optimized flushing strategy
+ * of only flushing when an mapping is reused. With it true the GART is
+ * flushed for every mapping. Problem is that doing the lazy flush seems
+ * to trigger bugs with some popular PCI cards, in particular 3ware (but
+ * has been also also seen with Qlogic at least).
+ */
+int iommu_fullflush = 1;
+
 /* Allocation bitmap for the remapping area: */
 static DEFINE_SPINLOCK(iommu_bitmap_lock);
 /* Guarded by iommu_bitmap_lock: */
@@ -892,6 +901,10 @@ void __init gart_parse_options(char *p)
 #endif
 	if (isdigit(*p) && get_option(&p, &arg))
 		iommu_size = arg;
+	if (!strncmp(p, "fullflush", 8))
+		iommu_fullflush = 1;
+	if (!strncmp(p, "nofullflush", 11))
+		iommu_fullflush = 0;
 	if (!strncmp(p, "noagp", 5))
 		no_agp = 1;
 	if (!strncmp(p, "noaperture", 10))
diff --git a/include/asm-x86/amd_iommu_types.h b/include/asm-x86/amd_iommu_types.h
index f953309..4ff892f 100644
--- a/include/asm-x86/amd_iommu_types.h
+++ b/include/asm-x86/amd_iommu_types.h
@@ -376,6 +376,12 @@ extern unsigned long *amd_iommu_pd_alloc_bitmap;
 /* will be 1 if device isolation is enabled */
 extern int amd_iommu_isolate;
 
+/*
+ * If true, the addresses will be flushed on unmap time, not when
+ * they are reused
+ */
+extern bool amd_iommu_unmap_flush;
+
 /* takes a PCI device id and prints it out in a readable form */
 static inline void print_devid(u16 devid, int nl)
 {
diff --git a/include/asm-x86/iommu.h b/include/asm-x86/iommu.h
index 67b2fd5..621a1af 100644
--- a/include/asm-x86/iommu.h
+++ b/include/asm-x86/iommu.h
@@ -7,7 +7,6 @@ extern struct dma_mapping_ops nommu_dma_ops;
 extern int force_iommu, no_iommu;
 extern int iommu_detected;
 extern int dmar_disabled;
-extern int iommu_fullflush;
 
 extern unsigned long iommu_num_pages(unsigned long addr, unsigned long len);
 

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

[Joerg Roedel]

On Mon, Sep 22, 2008 at 08:48:22PM +0200, Ingo Molnar wrote:
> applied Fujita's patch below to tip/x86/iommu that reverts those 
> options, thanks guys!
> 
> i'm wondering how we should proceed. For debug, iommu=off is certainly 
> good enough - all kernels are supposed to work out of box on all hw, and 
> every other result is a regression that must be fixed.
> 
> For performance tuning, it probably makes sense for developers to tune 
> IOMMU details (so that the bootup defaults can be improved - this is not 
> really something that should be done on a workload basis), but for that 
> IMO a /debug interface is a lot more useful than rather intrusive (and 
> inflexible) boot options.
>
> What do you think about a debugfs based tuning of various IOMMU details? 
> Maybe even the active driver could be changed - say from AMD-IOMMU to 
> GART, or to off.

Yes, for the IO/TLB flushing policy a /debug interface is usefull and
certainly for a number of other parameters we have today at the
commandline.
But I am not sure if we can change the IOMMU implementation on-the-fly
at runtime. We have to wait until all drivers have released their dma
memory before we can switch it off. For coherent allocations this may
take a long time.
So for selecting the type of IOMMU active in the system the command line
parameters are needed, I think. I prefer the iommu=$type parameter for
selecting the default (better: to select a default different from what
the kernel had chosen itself). We can have the IOMMU specific options to
switch it off in addition if we like to.

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

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

[FUJITA Tomonori]

On Mon, 22 Sep 2008 20:34:14 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Tue, Sep 23, 2008 at 01:51:05AM +0900, FUJITA Tomonori wrote:
> > The patch that I sent doesn't make any changes for you. GART works as
> > before and AMD IOMMU takes a new parameter, fullflush, as you want. It
> > does the exact same thing that the first version of your patchset for
> > 2.6.28.
> > 
> > So please let me apply it then we can stop this discussion.
> 
> Ok, so it was not nice from me to let Ingo pull this patch without your
> ack, I already admitted.

I didn't ask you to try to get my ACK before pushing the patches that
affect all the IOMMUs. I'll never ask for such.

I asked you to get the ACKs from the IOMMUs maintainers that your
patches affect.

If the IOMMUs maintainers who your patch affects agree on your patch,
I will not be against it even if I don't like it.

This time, you tried to add a generic parameter that would be useful
for VT-d, but seems that the VT-d maintainer is not ready for such
decision. Thus, I think that postponing adding the parameter was a
reasonable decision for everyone.

I also insisted that we need serious consideration about how all the
generic parameters interact with each other before changing them, but
again, if all the IOMMUs maintainers agree on a different plan, it's
fine by me.


> To end this discussion and get to productive
> discussions, Ingo, please apply this patch. I hope we will work together
> on a final solution for the problem.

Thanks very much.