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

Hi
commit to .24 tree:
http://git.kernel.org/?p=linux/kerne...62da84db06ded8

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
--
Tobias Powalowski
Archlinux Developer & Package Maintainer (tpowa)
http://www.archlinux.org
tpowa@archlinux.org

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)

iD8DBQBHOX7q81yYyRqLOvgRArHqAJ4w+pW0fJGEA3JTFrLnOC RsX/LdjQCeOYSH
L57t8Le0qFR4OJQ3XfP7Mz8=
=jbU0
-----END PGP SIGNATURE-----

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:
> commit to .24 tree:
> http://git.kernel.org/?p=linux/kerne...62da84db06ded8
>
> 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.

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

Signed-off-by: Jeremy Fitzhardinge
Cc: Tobias Powalowski
Cc: Zach Amsden

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

================================================== =================
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -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);

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Tue, Nov 13, 2007 at 12:21:16PM -0800, Jeremy Fitzhardinge wrote:
> 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.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Tue, 2007-11-13 at 22:22 +0000, Christoph Hellwig wrote:
> On Tue, Nov 13, 2007 at 12:21:16PM -0800, Jeremy Fitzhardinge wrote:
> > 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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Christoph Hellwig wrote:
> 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.

J

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

At Tue, 13 Nov 2007 16:51:04 -0800,
Zachary Amsden wrote:
>
> On Tue, 2007-11-13 at 22:22 +0000, Christoph Hellwig wrote:
> > On Tue, Nov 13, 2007 at 12:21:16PM -0800, Jeremy Fitzhardinge wrote:
> > > 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.

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Takashi Iwai wrote:
> 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.

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

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

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

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

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

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

J
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

At Mon, 19 Nov 2007 17:14:15 -0800,
Jeremy Fitzhardinge wrote:
>
> Takashi Iwai wrote:
> > 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

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

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

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

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

Agreed.


thanks,

Takashi
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/