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

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

[Jeremy Fitzhardinge]

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 <t.powa@gmx.de> reports:
> 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.

Takashi Iwai <tiwai@suse.de> adds:
> 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.

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.

Signed-off-by: Jeremy Fitzhardinge <Jeremy.Fitzhardinge@citrix.com>
Cc: Tobias Powalowski <t.powa@gmx.de>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Zachary Amsden <zach@vmware.com>

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

diff -r 0328c71b880c arch/x86/kernel/paravirt_32.c
--- a/arch/x86/kernel/paravirt_32.c	Tue Nov 27 11:21:09 2007 -0800
+++ b/arch/x86/kernel/paravirt_32.c	Tue Nov 27 11:21:16 2007 -0800
@@ -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);

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

[Adrian Bunk]

On Tue, Nov 27, 2007 at 02:57:30PM -0800, Jeremy Fitzhardinge wrote:
>...
> 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.

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.

> 2. It's a regression from previous kernels, which would work these
>    modules even with CONFIG_PARAVIRT enabled.
>...

It cannot be a regression since the kernel does not have a stable API 
for modules.

> 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

-- 

       "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

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

[Jeremy Fitzhardinge]

Adrian Bunk wrote:
> On Tue, Nov 27, 2007 at 02:57:30PM -0800, Jeremy Fitzhardinge wrote:
>   
>> ...
>> 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.
>>     
>
> 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.

>> 2. It's a regression from previous kernels, which would work these
>>    modules even with CONFIG_PARAVIRT enabled.
>> ...
>>     
>
> 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.

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

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

[Adrian Bunk]

On Wed, Nov 28, 2007 at 01:15:39PM -0800, Jeremy Fitzhardinge wrote:
> Adrian Bunk wrote:
> > On Tue, Nov 27, 2007 at 02:57:30PM -0800, Jeremy Fitzhardinge wrote:
>...
> >> 2. It's a regression from previous kernels, which would work these
> >>    modules even with CONFIG_PARAVIRT enabled.
> >> ...
> >>     
> >
> > 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.

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

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.

>     J

cu
Adrian

[1] even additions would be problematic since buggy external modules 
    might have hardcoded the struct size somewhere...

-- 

       "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

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

[Jeremy Fitzhardinge]

Adrian Bunk wrote:
> 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

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

[Adrian Bunk]

On Wed, Nov 28, 2007 at 03:57:47PM -0800, Jeremy Fitzhardinge wrote:
> Adrian Bunk wrote:
> > 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.
>...

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.

>     J

cu
Adrian

[1] and that includes what is visible to modules

-- 

       "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

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

[Tobias Powalowski]

[-- 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 --]

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

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

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

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

[Christoph Hellwig]

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.

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

[Zachary Amsden]

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

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

[Takashi Iwai]

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

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

[Jeremy Fitzhardinge]

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

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

[Takashi Iwai]

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

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

[Jeremy Fitzhardinge]

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