REGRESSION: 2.6.24 breaks nvidia and amd/ati binary drivers, by exporting paravirt symbols as GPL

PATCH NAK Tobias Powalowski 5 participants 8 messages Nov 13, 2007 – Nov 20, 2007

Subsystems (2)

x86 PARAVIRT_OPS interface

lore

Share & tools

lore

Tobias Powalowski

author

2007-11-13 10:39 UTC #0 lore raw

Hi
commit to .24 tree:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=93b1eab3d29e7ea32ee583de3362da84db06ded8

introduces:
+EXPORT_SYMBOL_GPL(pv_mmu_ops);
+EXPORT_SYMBOL_GPL(pv_cpu_ops);

pv_cpu_ops is for nvidia
pv_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.

is there a chance to see these symbols not exported as GPL? Or do they have to change their binary drivers?

thanks in advance
greetings
tpowa

Signature · 4 lines

Tobias Powalowski
Archlinux Developer & Package Maintainer (tpowa)
http://www.archlinux.org
[email protected]

Jeremy Fitzhardinge

2007-11-13 20:23 UTC #1 lore raw

PATCH 1/1 x86/paravirt: revert exports to restore old behaviour

Subject: 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's mail:

Quoted 7 lines

commit to .24 tree:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=93b1eab3d29e7ea32ee583de3362da84db06ded8
introduces:
(quoted diff) +2 -0
+EXPORT_SYMBOL_GPL(pv_mmu_ops);
+EXPORT_SYMBOL_GPL(pv_cpu_ops);

Quoted 5 lines

pv_cpu_ops is for nvidia
pv_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.

[ Tobias: It would be nice to know exactly which operations the
modules you're trying to compile are using. ]

Signed-off-by Jeremy Fitzhardinge <[email protected]>

Cc Tobias Powalowski <[email protected]>

Cc Zach Amsden <[email protected]>

---
 arch/x86/kernel/paravirt.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

===================================================================

arch/x86/kernel/paravirt.c +2 -2

@@ -440,8 +440,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);

Christoph Hellwig

NAK

2007-11-13 22:22 UTC #2 lore raw

Re: [PATCH] x86/paravirt: revert exports to restore old behaviour

Quoted 6 lines · Jeremy Fitzhardinge

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

NACK, both of these are internal and graphics drivers should not be using them.

Zachary Amsden

2007-11-14 00:48 UTC #3 lore raw

Re: [PATCH] x86/paravirt: revert exports to restore old behaviour

Quoted 10 lines · Christoph Hellwig

Quoted 6 lines · Jeremy Fitzhardinge
Subject: 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.
NACK, both of these are internal and graphics drivers should not be using them.

Some of them are internal, but some are not, they just happened to be privileged CPU operations available to anyone.

Does anyone know what hooks they are actually using? Something like reading MSRs is not internal at all, it is CPU specific, and the graphics drivers might have very good reasons to read them to figure out how AGP is configured for example.

The graphics drivers most certainly don't need to be paravirtualized however, so they could always work around this with raw asm constructs.

The mmu_ops hook is more debatable. But until someone figures out what hooks they want, we can't know whether they are internal or not. In any case, this is a regression.

Zach

Takashi Iwai

2007-11-19 17:28 UTC #4 lore raw

Re: [PATCH] x86/paravirt: revert exports to restore old behaviour

At Tue, 13 Nov 2007 16:51:04 -0800,

Quoted 21 lines · Zachary Amsden

Quoted 10 lines · Christoph Hellwig
Quoted 6 lines · Jeremy Fitzhardinge
Subject: 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.
NACK, both of these are internal and graphics drivers should not be using them.
Some of them are internal, but some are not, they just happened to be privileged CPU operations available to anyone.
Does anyone know what hooks they are actually using? Something like reading MSRs is not internal at all, it is CPU specific, and the graphics drivers might have very good reasons to read them to figure out how AGP is configured for example.

Quoted 6 lines

The graphics drivers most certainly don't need to be paravirtualized however, so they could always work around this with raw asm constructs.
The mmu_ops hook is more debatable. But until someone figures out what hooks they want, we can't know whether they are internal or not. In any case, this is a regression.

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_GPL changes:

* local_disable_irq(), local_irq_save*(), etc.
* MSR-related macros like rdmsr(), wrmsr(), read_cr0(), etc. wbinvd(), too.
* pmd_val(), pgd_val(), etc are all involved with pv_mm_ops. pmd_large() and pmd_bad() is also indirectly involved. __flush_tlb() and friends suffer, too.

The easiest workaround I found was to undefine CONFIG_PARAVIRT before inclusion of linux kernel headers, but it is really ugly and hacky. Redefinig with raw_*() and native_*() is another way, but it takes much more work than defining these primitive functions in assembly.

So, in short, with EXPORT_SYMBOL_GPL change, it's pretty hard to write a non-GPL driver in a same manner...

Takashi

Jeremy Fitzhardinge

2007-11-20 01:28 UTC #5 lore raw

Re: [PATCH] x86/paravirt: revert exports to restore old behaviour

Quoted 4 lines · Takashi Iwai

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_GPL changes:

Which kernel version are you using? This is different in .24-rc compared to .23.

Quoted 2 lines

* local_disable_irq(), local_irq_save*(), etc.

These should be OK either way. pv_irq_ops is not _GPL.

Quoted 3 lines

* MSR-related macros like rdmsr(), wrmsr(), read_cr0(), etc. wbinvd(), too.

These could reasonably use the the native_* versions anyway, since the driver won't be being used in an environment where these won't work. Perhaps they should be split out separate from the gdt/ldt operations, which they should have no business touching.

Quoted 4 lines

* pmd_val(), pgd_val(), etc are all involved with pv_mm_ops. pmd_large() and pmd_bad() is also indirectly involved. __flush_tlb() and friends suffer, too.

Yeah, I guess they can be expected to play with pagetables.

Quoted 3 lines

The easiest workaround I found was to undefine CONFIG_PARAVIRT before inclusion of linux kernel headers, but it is really ugly and hacky.

Yeah. It will explode if you are running in a virtual environment which still gives the virtual machine graphics hardware access.

Quoted 6 lines

Redefinig with raw_*() and native_*() is another way, but it takes much more work than defining these primitive functions in assembly.
So, in short, with EXPORT_SYMBOL_GPL change, it's pretty hard to write a non-GPL driver in a same manner...

Yeah. I think removing the difference between PARAVIRT and non-PARAVIRT is enough to justify the exports. If we want to make the policy decision that modules can't use pagetable or msr operations at all, then that's a separate decision which can be applied uniformly to PARAVIRT and non-PARAVIRT.

Takashi Iwai

2007-11-20 06:52 UTC #6 lore raw

Re: [PATCH] x86/paravirt: revert exports to restore old behaviour

At Mon, 19 Nov 2007 17:14:15 -0800,

Quoted 9 lines · Jeremy Fitzhardinge

Quoted 4 lines · Takashi Iwai
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_GPL changes:
Which kernel version are you using? This is different in .24-rc compared to .23.

24-rc2. 23 has no problem, as you know :)

Quoted 4 lines

Quoted 2 lines
* local_disable_irq(), local_irq_save*(), etc.
These should be OK either way. pv_irq_ops is not _GPL.

Right. I thought it somehow involved with other pv ops indirectly, but it seems not.

Quoted 8 lines

Quoted 3 lines
* MSR-related macros like rdmsr(), wrmsr(), read_cr0(), etc. wbinvd(), too.
These could reasonably use the the native_* versions anyway, since the driver won't be being used in an environment where these won't work. Perhaps they should be split out separate from the gdt/ldt operations, which they should have no business touching.

Yes, that's possible.

Quoted 5 lines

Quoted 4 lines
* pmd_val(), pgd_val(), etc are all involved with pv_mm_ops. pmd_large() and pmd_bad() is also indirectly involved. __flush_tlb() and friends suffer, too.

Quoted 8 lines

Yeah, I guess they can be expected to play with pagetables.
Quoted 3 lines
The easiest workaround I found was to undefine CONFIG_PARAVIRT before inclusion of linux kernel headers, but it is really ugly and hacky.
Yeah. It will explode if you are running in a virtual environment which still gives the virtual machine graphics hardware access.

Yes. More over, there is no guarantee that this will be built properly in the future. It's a kind of coincident that the driver is built. If any non-paravirt implementation accesses an exported symbol instead of inlining, then this won't work, too.

Quoted 7 lines

Quoted 6 lines
Redefinig with raw_*() and native_*() is another way, but it takes much more work than defining these primitive functions in assembly.
So, in short, with EXPORT_SYMBOL_GPL change, it's pretty hard to write a non-GPL driver in a same manner...

Quoted 5 lines

Yeah. I think removing the difference between PARAVIRT and non-PARAVIRT is enough to justify the exports. If we want to make the policy decision that modules can't use pagetable or msr operations at all, then that's a separate decision which can be applied uniformly to PARAVIRT and non-PARAVIRT.

Agreed.

thanks,

Takashi

Jeremy Fitzhardinge

2007-11-14 01:24 UTC #7 lore raw

Re: [PATCH] x86/paravirt: revert exports to restore old behaviour

In reply to Christoph Hellwig

Quoted 3 lines · Christoph Hellwig

NACK, both of these are internal and graphics drivers should not be using them.

I don't feel very strongly about it either way. I think the two arguments for it are:

1. it's a regression compared to previous kernels
2. it's a visible difference between CONFIG_PARAVIRT and non-CONFIG_PARAVIRT

Arguments against are all the usual ones.