login
Search
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 ...
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 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 We can keep the current Intel option and have iommu=fullflush in 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 --
On Fri, 19 Sep 2008 18:45:41 +0200 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 Yeah, Intel IOMMU need to keep the current option for IOTLB batching but it also support fullflush. But we don't discuss anything with 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. --
No. But we can have the generic option in parallel. There is no reason
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
You keep this option for AMD IOMMU too. If you move it to AMD IOMMU code
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
--
On Fri, 19 Sep 2008 19:20:36 +0200 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 I'm not sure what you mean. You think that we can't change the Again, it's not a proper process for me. --
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
--
On Fri, 19 Sep 2008 19:46:05 +0200 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
--
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 --
On Fri, 19 Sep 2008 19:30:04 +0200 I'm not against fullflush but we need to discuss it with other people before making the change. Please, --
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
--
On Fri, 19 Sep 2008 20:01:18 +0200 See: I think that I already expressed a real objection for nofullflush --
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
And I agree with that. But AMD IOMMU updates are not the right place to
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
--
On Fri, 19 Sep 2008 21:52:16 +0200 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
--
On Fri, 19 Sep 2008 22:19:09 +0200 Because you did things int the wrong way, I said again and again. 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
--
On Sat, 20 Sep 2008 00:09:46 +0200 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 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
--
On Sat, 20 Sep 2008 00:39:51 +0200 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. --
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 --
On Sat, 20 Sep 2008 08:00:59 +0200 I see. I thought that the similar rule is applied to boot options and 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 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. --
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 yes - the solution could be as easy as to add back that single option to 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 --
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
--
On Mon, 22 Sep 2008 13:17:30 +0200 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. I guess that there are some things that a kernel can set up only I'm not sure we can because he has a quite different opinion on this, 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 --
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
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
--
On Mon, 22 Sep 2008 18:23:14 +0200 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 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. --
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
--
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 ++-- ...
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
--
On Mon, 22 Sep 2008 20:34:14 +0200 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 Thanks very much. --
