LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* REGRESSION: 2.6.24 breaks nvidia and amd/ati binary drivers, by exporting paravirt symbols as GPL
@ 2007-11-13 10:39 Tobias Powalowski
  2007-11-13 20:21 ` [PATCH] x86/paravirt: revert exports to restore old behaviour Jeremy Fitzhardinge
  0 siblings, 1 reply; 8+ messages in thread
From: Tobias Powalowski @ 2007-11-13 10:39 UTC (permalink / raw)
  To: linux-kernel

[-- Attachment #1: Type: text/plain, Size: 687 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] x86/paravirt: revert exports to restore old behaviour
  2007-11-13 10:39 REGRESSION: 2.6.24 breaks nvidia and amd/ati binary drivers, by exporting paravirt symbols as GPL Tobias Powalowski
@ 2007-11-13 20:21 ` Jeremy Fitzhardinge
  2007-11-13 22:22   ` Christoph Hellwig
  0 siblings, 1 reply; 8+ messages in thread
From: Jeremy Fitzhardinge @ 2007-11-13 20:21 UTC (permalink / raw)
  To: Tobias Powalowski
  Cc: linux-kernel, Zachary Amsden, Ingo Molnar, Thomas Gleixner

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

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

Signed-off-by: Jeremy Fitzhardinge <Jeremy.Fitzhardinge@citrix.com>
Cc: Tobias Powalowski <t.powa@gmx.de>
Cc: Zach Amsden <zach@vmware.com>

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/paravirt: revert exports to restore old behaviour
  2007-11-13 20:21 ` [PATCH] x86/paravirt: revert exports to restore old behaviour Jeremy Fitzhardinge
@ 2007-11-13 22:22   ` Christoph Hellwig
  2007-11-14  0:51     ` Zachary Amsden
  2007-11-14  1:22     ` Jeremy Fitzhardinge
  0 siblings, 2 replies; 8+ messages in thread
From: Christoph Hellwig @ 2007-11-13 22:22 UTC (permalink / raw)
  To: Jeremy Fitzhardinge
  Cc: Tobias Powalowski, linux-kernel, Zachary Amsden, Ingo Molnar,
	Thomas Gleixner

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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/paravirt: revert exports to restore old behaviour
  2007-11-13 22:22   ` Christoph Hellwig
@ 2007-11-14  0:51     ` Zachary Amsden
  2007-11-19 17:05       ` Takashi Iwai
  2007-11-14  1:22     ` Jeremy Fitzhardinge
  1 sibling, 1 reply; 8+ messages in thread
From: Zachary Amsden @ 2007-11-14  0:51 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jeremy Fitzhardinge, Tobias Powalowski, linux-kernel,
	Ingo Molnar, Thomas Gleixner

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/paravirt: revert exports to restore old behaviour
  2007-11-13 22:22   ` Christoph Hellwig
  2007-11-14  0:51     ` Zachary Amsden
@ 2007-11-14  1:22     ` Jeremy Fitzhardinge
  1 sibling, 0 replies; 8+ messages in thread
From: Jeremy Fitzhardinge @ 2007-11-14  1:22 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Tobias Powalowski, linux-kernel, Zachary Amsden, Ingo Molnar,
	Thomas Gleixner

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/paravirt: revert exports to restore old behaviour
  2007-11-14  0:51     ` Zachary Amsden
@ 2007-11-19 17:05       ` Takashi Iwai
  2007-11-20  1:14         ` Jeremy Fitzhardinge
  0 siblings, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2007-11-19 17:05 UTC (permalink / raw)
  To: Zachary Amsden
  Cc: Christoph Hellwig, Jeremy Fitzhardinge, Tobias Powalowski,
	linux-kernel, Ingo Molnar, Thomas Gleixner

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/paravirt: revert exports to restore old behaviour
  2007-11-19 17:05       ` Takashi Iwai
@ 2007-11-20  1:14         ` Jeremy Fitzhardinge
  2007-11-20  6:25           ` Takashi Iwai
  0 siblings, 1 reply; 8+ messages in thread
From: Jeremy Fitzhardinge @ 2007-11-20  1:14 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Zachary Amsden, Christoph Hellwig, Tobias Powalowski,
	linux-kernel, Ingo Molnar, Thomas Gleixner

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/paravirt: revert exports to restore old behaviour
  2007-11-20  1:14         ` Jeremy Fitzhardinge
@ 2007-11-20  6:25           ` Takashi Iwai
  0 siblings, 0 replies; 8+ messages in thread
From: Takashi Iwai @ 2007-11-20  6:25 UTC (permalink / raw)
  To: Jeremy Fitzhardinge
  Cc: Zachary Amsden, Christoph Hellwig, Tobias Powalowski,
	linux-kernel, Ingo Molnar, Thomas Gleixner

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2007-11-20  6:52 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-11-13 10:39 REGRESSION: 2.6.24 breaks nvidia and amd/ati binary drivers, by exporting paravirt symbols as GPL Tobias Powalowski
2007-11-13 20:21 ` [PATCH] x86/paravirt: revert exports to restore old behaviour Jeremy Fitzhardinge
2007-11-13 22:22   ` Christoph Hellwig
2007-11-14  0:51     ` Zachary Amsden
2007-11-19 17:05       ` Takashi Iwai
2007-11-20  1:14         ` Jeremy Fitzhardinge
2007-11-20  6:25           ` Takashi Iwai
2007-11-14  1:22     ` Jeremy Fitzhardinge

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).