x86/paravirt: revert exports to restore old behaviour
Subsystems (2)
PATCH 1/1 x86/paravirt: revert exports to restore old behaviour
Subdividing the paravirt_ops structure caused a regression in certain non-GPL modules which try to use mmu_ops and cpu_ops. This restores the old behaviour, and makes it consistent with the non-CONFIG_PARAVIRT case.
Tobias Powalowski <[email protected]> reports:
Quoted 7 lines
commit to .24 tree:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=93b1eab3d29e7ea32ee583de3362da84db06ded8introduces:
(quoted diff) +2
-0+EXPORT_SYMBOL_GPL(pv_mmu_ops); +EXPORT_SYMBOL_GPL(pv_cpu_ops);
Quoted 5 lines
pv_cpu_opsis for nvidiapv_mmu_ops' is for amd(ati)which will break 32bit systems with paravirt enabled and trying to compile the binary graphic drivers from amd(ati) and nvidia.
Takashi Iwai <[email protected]> adds:
Quoted 10 lines
I took at this problem (as I have an nvidia card on one of my workstations), and found out that the following suffer from
EXPORT_SYMBOL_GPLchanges:*
local_disable_irq(), local_irq_save*(), etc.
* MSR-related macros likerdmsr(),wrmsr(),read_cr0(), etc.wbinvd(), too.
*pmd_val(),pgd_val(), etc are all involved withpv_mm_ops.pmd_large()andpmd_bad()is also indirectly involved.__flush_tlb()and friends suffer, too.
Christoph Hellwig objects to this patch on the grounds that modules shouldn't be using these operations anyway. I don't think this is a particularly good reason to reject the patch, for several reasons:
1. These operations are still available to modules when not using CONFIG_PARAVIRT, since they are implicitly exported as inline functions via the kernel headers. Exporting the same functionality as GPL-only symbols just adds a gratuitious difference between CONFIG_PARAVIRT and non-CONFIG_PARAVIRT configurations. If we really think these operations are not for module use (or non-GPL module use), then we should solve the problem in a general way.
2. It's a regression from previous kernels, which would work these modules even with CONFIG_PARAVIRT enabled.
3. The operations in question seem pretty reasonable for modules to use. The control registers/MSRs can be accessed directly anyway, so there's no benefit in preventing modules from using standard interfaces. And it seems reasonable to allow a graphics driver to create its own mappings if it wants.
Therefore, I think this patch should go in for 2.6.24. If people really think that these operations should not be available to modules, then we can address that separately.
--- arch/x86/kernel/paravirt_32.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/kernel/paravirt_32.c +2 -2
@@ -465,8 +465,8 @@ struct pv_mmu_ops pv_mmu_ops = { };EXPORT_SYMBOL_GPL (pv_time_ops); -EXPORT_SYMBOL_GPL (pv_cpu_ops); -EXPORT_SYMBOL_GPL (pv_mmu_ops); +EXPORT_SYMBOL (pv_cpu_ops); +EXPORT_SYMBOL (pv_mmu_ops);EXPORT_SYMBOL_GPL (pv_apic_ops);EXPORT_SYMBOL_GPL (pv_info);EXPORT_SYMBOL (pv_irq_ops);
Quoted 5 lines · Jeremy Fitzhardinge
...
Christoph Hellwig objects to this patch on the grounds that modules shouldn't be using these operations anyway. I don't think this is a particularly good reason to reject the patch, for several reasons:
Quoted 7 lines
1. These operations are still available to modules when not using
CONFIG_PARAVIRT, since they are implicitly exported as inline functions via the kernel headers. Exporting the same functionality as GPL-only symbols just adds a gratuitious difference betweenCONFIG_PARAVIRTand non-CONFIG_PARAVIRT configurations. If we really think these operations are not for module use (or non-GPL module use), then we should solve the problem in a general way.
Current practice in the kernel is that when something that should not be done works by chance with some kernel versions and/or configurations that's simply an unfortunate fact that should be fixed but doesn't result in any guarantee that it works with all kernel versions and/or configurations.
Quoted 3 lines
2. It's a regression from previous kernels, which would work these modules even with
CONFIG_PARAVIRTenabled.
...
It cannot be a regression since the kernel does not have a stable API for modules.
Quoted 4 lines
Therefore, I think this patch should go in for 2.6.24. If people really think that these operations should not be available to modules, then we can address that separately.
...
Why should we start with one step back for getting two steps ahead?
cu
Adrian
Signature · 4 lines
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon SeedQuoted 16 lines · Adrian Bunk
Quoted 5 lines · Jeremy Fitzhardinge
...
Christoph Hellwig objects to this patch on the grounds that modules shouldn't be using these operations anyway. I don't think this is a particularly good reason to reject the patch, for several reasons:Quoted 8 lines
1. These operations are still available to modules when not using
CONFIG_PARAVIRT, since they are implicitly exported as inline functions via the kernel headers. Exporting the same functionality as GPL-only symbols just adds a gratuitious difference betweenCONFIG_PARAVIRTand non-CONFIG_PARAVIRT configurations. If we really think these operations are not for module use (or non-GPL module use), then we should solve the problem in a general way.
Quoted 6 lines
Current practice in the kernel is that when something that should not be done works by chance with some kernel versions and/or configurations that's simply an unfortunate fact that should be fixed but doesn't result in any guarantee that it works with all kernel versions and/or configurations.
Well, I suppose, but there's nothing particularly "internal" about the functions that these modules want to get access to. They are the proper API functions for doing what the modules want to do.
And we have enough weird little config-dependent corners of the kernel API that complicate testing; I don't think we need to knowingly introduce more.
Quoted 8 lines
Quoted 4 lines
2. It's a regression from previous kernels, which would work these modules even with
CONFIG_PARAVIRTenabled.
...It cannot be a regression since the kernel does not have a stable API for modules.
Anything that once worked but now does not is a regression. Generally when we intend a regression like this, we call it a deprecation of an interface and have a procedure for that. It was not my intention to cause a regression with this change, and an unintended regression is a bug.
Quoted 8 lines
Quoted 5 lines
Therefore, I think this patch should go in for 2.6.24. If people really think that these operations should not be available to modules, then we can address that separately.
...Why should we start with one step back for getting two steps ahead?
Why is it a step back? What are the steps ahead? There's been no discussion on this point, so I don't think you can assume there's a clear way forward.
I think Linus and Andrew have been pretty clear on the policy for these kinds of things: regressions are always bad, even if fixing the regression means some other bugfix needs to be put off.
J
Quoted 8 lines · Jeremy Fitzhardinge
Quoted 1 line · Adrian Bunk
On Tue, Nov 27, 2007 at 02:57:30PM -0800, Jeremy Fitzhardinge wrote:
...
Quoted 5 lines
Quoted 4 lines
2. It's a regression from previous kernels, which would work these modules even with
CONFIG_PARAVIRTenabled.
...
Quoted 8 lines
Quoted 3 lines
It cannot be a regression since the kernel does not have a stable API for modules.
Anything that once worked but now does not is a regression. Generally when we intend a regression like this, we call it a deprecation of an interface and have a procedure for that. It was not my intention to cause a regression with this change, and an unintended regression is a bug.
The standard procdure for changing the in-kernel API is to simply break the API without any deprecation or advance notice.
Andrew has some strange views that big changes can be done immediately while smaller changes should require deprecation periods, but everyone else seems to have less strange views.
Quoted 8 lines
Quoted 7 lines
Quoted 5 lines
Therefore, I think this patch should go in for 2.6.24. If people really think that these operations should not be available to modules, then we can address that separately.
...Why should we start with one step back for getting two steps ahead?
Quoted 7 lines
Why is it a step back? What are the steps ahead? There's been no discussion on this point, so I don't think you can assume there's a clear way forward.
I think Linus and Andrew have been pretty clear on the policy for these kinds of things: regressions are always bad, even if fixing the regression means some other bugfix needs to be put off.
This does not apply since we do not have a stable in-kernel API, and therefore changes to the in-kernel API can by definition not be regressions.
2.6.24 most likely contains hundreds of changes and removals of in-kernel APIs that existed in 2.6.23.
Are you seriously suggesting that e.g. every single change to any struct under include/ [1] would require an announcement x kernel releases before it can be implemented?
And yes, things like that would be required if we offered any kind of API stability.
Quoted 1 line
J
cu
Adrian
[1] even additions would be problematic since buggy external modules might have hardcoded the struct size somewhere...
Signature · 4 lines
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon SeedQuoted 10 lines · Adrian Bunk
This does not apply since we do not have a stable in-kernel API, and therefore changes to the in-kernel API can by definition not be regressions.
2.6.24 most likely contains hundreds of changes and removals of in-kernel APIs that existed in 2.6.23.
Are you seriously suggesting that e.g. every single change to any struct under
include/[1] would require an announcement x kernel releases before it can be implemented?
Well, no, but that's not the point.
You're not addressing the substance of this specific issue, which is simply whether rdmsr/wrmsr and pmd_val/make_pmd are actually internal interfaces that drivers have no business in using, or are they legitimate interfaces? If not, then what interface *should* drivers be using to do these things? They seem like perfectly reasonable things for a driver to want to do, and I don't see any inherent problem with these interfaces.
It's not like we need to curtail these interfaces for any technical reason. It's pretty much the arbitrary result of me choosing to type "_GPL" rather than leaving it off. Refusing to correct what amounts to a typo seems petty.
J
Quoted 8 lines · Jeremy Fitzhardinge
Quoted 7 lines · Adrian Bunk
This does not apply since we do not have a stable in-kernel API, and therefore changes to the in-kernel API can by definition not be regressions.
2.6.24 most likely contains hundreds of changes and removals of in-kernel APIs that existed in 2.6.23.
Quoted 6 lines
Quoted 3 lines
Are you seriously suggesting that e.g. every single change to any struct under
include/[1] would require an announcement x kernel releases before it can be implemented?Well, no, but that's not the point.
...
Sorry if I was a bit harsh, but no change to the in-kernel API [1] could ever be called a regression since we do not have a stable in-kernel API.
And what annoyed was that this was one of at least 3 ongoing linux-kernel threads where people tried to bring the notion that any part of the in-kernel API had any kind of stability.
Quoted 1 line
J
cu
Adrian
[1] and that includes what is visible to modules
Signature · 4 lines
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed