patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug

patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug

[Jan Beulich]

Converting __cpuinit functions called out of init_amd() (and similar others)
to __init (and making them subject of xxx_initcall() handling isn't valid, as
they would no longer be called for hot plugged CPUs.

Further, since it's likely that in virtualized environments the MSR write
would at best be ignored, I'd also recommend using the fault-safe
accessors here *and* check that the bit actually got set before setting
PCI_HAS_IO_ECS (one would obviously have to BUG() when hot-plugged
CPUs fail to set the bit when those available at boot successfully did so).

Jan

patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug

[Jan Beulich]

>Converting __cpuinit functions called out of init_amd() (and similar others)
>to __init (and making them subject of xxx_initcall() handling isn't valid, as
>they would no longer be called for hot plugged CPUs.
>
>Further, since it's likely that in virtualized environments the MSR write
>would at best be ignored, I'd also recommend using the fault-safe
>accessors here *and* check that the bit actually got set before setting
>PCI_HAS_IO_ECS (one would obviously have to BUG() when hot-plugged
>CPUs fail to set the bit when those available at boot successfully did so).

Even worse - this would even try to access the MSR on non-AMD CPUs
(currently probably prevented just by the fact that only AMD ones use
family values of 0x10 or higher).

Jan

Re: patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug

[Ingo Molnar]

* Jan Beulich <jbeulich@novell.com> wrote:

> Converting __cpuinit functions called out of init_amd() (and similar 
> others) to __init (and making them subject of xxx_initcall() handling 
> isn't valid, as they would no longer be called for hot plugged CPUs.
> 
> Further, since it's likely that in virtualized environments the MSR 
> write would at best be ignored, I'd also recommend using the 
> fault-safe accessors here *and* check that the bit actually got set 
> before setting PCI_HAS_IO_ECS (one would obviously have to BUG() when 
> hot-plugged CPUs fail to set the bit when those available at boot 
> successfully did so).

hm, which patch is this exactly, and in what tree? It's not upstream nor 
in -tip.

	Ingo

Re: patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug

[Robert Richter]

On 21.08.08 15:02:59, Ingo Molnar wrote:
> 
> * Jan Beulich <jbeulich@novell.com> wrote:
> 
> > Converting __cpuinit functions called out of init_amd() (and similar 
> > others) to __init (and making them subject of xxx_initcall() handling 
> > isn't valid, as they would no longer be called for hot plugged CPUs.
> > 
> > Further, since it's likely that in virtualized environments the MSR 
> > write would at best be ignored, I'd also recommend using the 
> > fault-safe accessors here *and* check that the bit actually got set 
> > before setting PCI_HAS_IO_ECS (one would obviously have to BUG() when 
> > hot-plugged CPUs fail to set the bit when those available at boot 
> > successfully did so).
> 
> hm, which patch is this exactly, and in what tree? It's not upstream nor 
> in -tip.

Probably this in the mainline kernel is meant:

commit 3a27dd1ce5de08e21e0266ddf00e6f1f843bfe8b
Author: Robert Richter <robert.richter@amd.com>
Date:   Thu Jun 12 20:19:23 2008 +0200

    x86: Move PCI IO ECS code to x86/pci
    
    "Form follows function". Code is now where it belongs to.
    
    Signed-off-by: Robert Richter <robert.richter@amd.com>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

The problem is code like this:

+static int __init enable_pci_io_ecs(void)
+{
+       /* assume all cpus from fam10h have IO ECS */
+        if (boot_cpu_data.x86 < 0x10)
+               return 0;
+       on_each_cpu(enable_pci_io_ecs_per_cpu, NULL, 1, 1);
+       pci_probe |= PCI_HAS_IO_ECS;
+       return 0;
+}

It is only called during the setup and if the cpu is enabled. I intend
to rewrite the cpu setup code using cpu notifiers.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center
email: robert.richter@amd.com

Re: patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug

[Jan Beulich]

>>> Ingo Molnar <mingo@elte.hu> 21.08.08 15:02 >>>
>
>* Jan Beulich <jbeulich@novell.com> wrote:
>
>> Converting __cpuinit functions called out of init_amd() (and similar 
>> others) to __init (and making them subject of xxx_initcall() handling 
>> isn't valid, as they would no longer be called for hot plugged CPUs.
>> 
>> Further, since it's likely that in virtualized environments the MSR 
>> write would at best be ignored, I'd also recommend using the 
>> fault-safe accessors here *and* check that the bit actually got set 
>> before setting PCI_HAS_IO_ECS (one would obviously have to BUG() when 
>> hot-plugged CPUs fail to set the bit when those available at boot 
>> successfully did so).
>
>hm, which patch is this exactly, and in what tree? It's not upstream nor 
>in -tip.

It is upstream:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=3a27dd1ce5de08e21e0266ddf00e6f1f843bfe8b
(and I just verified it didn't change between -rc3 and -rc4).

Jan

Re: patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug

[Robert Richter]

On 21.08.08 13:59:33, Jan Beulich wrote:
> Even worse - this would even try to access the MSR on non-AMD CPUs
> (currently probably prevented just by the fact that only AMD ones use
> family values of 0x10 or higher).

Ups, really worse. Code was AMD specific before and became
generic. Will fix this.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center
email: robert.richter@amd.com

Re: patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug

[Yinghai Lu]

On Thu, Aug 21, 2008 at 6:29 AM, Robert Richter <robert.richter@amd.com> wrote:
> On 21.08.08 13:59:33, Jan Beulich wrote:
>> Even worse - this would even try to access the MSR on non-AMD CPUs
>> (currently probably prevented just by the fact that only AMD ones use
>> family values of 0x10 or higher).
>
> Ups, really worse. Code was AMD specific before and became
> generic. Will fix this.
>

it seems you didn't address the concern about hotplug as you promised

http://lkml.org/lkml/2008/6/13/151

YH

Re: patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug

[Robert Richter]

On 21.08.08 09:25:37, Yinghai Lu wrote:
> On Thu, Aug 21, 2008 at 6:29 AM, Robert Richter <robert.richter@amd.com> wrote:
> > On 21.08.08 13:59:33, Jan Beulich wrote:
> >> Even worse - this would even try to access the MSR on non-AMD CPUs
> >> (currently probably prevented just by the fact that only AMD ones use
> >> family values of 0x10 or higher).
> >
> > Ups, really worse. Code was AMD specific before and became
> > generic. Will fix this.
> >
> 
> it seems you didn't address the concern about hotplug as you promised
> 
> http://lkml.org/lkml/2008/6/13/151

Yinghai, I know. You made a comment on this in reply to the patch and
this is still an open issue. Really, will fix this. :-)

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center
email: robert.richter@amd.com

Re: patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug

[Ingo Molnar]

* Robert Richter <robert.richter@amd.com> wrote:

> On 21.08.08 15:02:59, Ingo Molnar wrote:
> > 
> > * Jan Beulich <jbeulich@novell.com> wrote:
> > 
> > > Converting __cpuinit functions called out of init_amd() (and similar 
> > > others) to __init (and making them subject of xxx_initcall() handling 
> > > isn't valid, as they would no longer be called for hot plugged CPUs.
> > > 
> > > Further, since it's likely that in virtualized environments the MSR 
> > > write would at best be ignored, I'd also recommend using the 
> > > fault-safe accessors here *and* check that the bit actually got set 
> > > before setting PCI_HAS_IO_ECS (one would obviously have to BUG() when 
> > > hot-plugged CPUs fail to set the bit when those available at boot 
> > > successfully did so).
> > 
> > hm, which patch is this exactly, and in what tree? It's not upstream nor 
> > in -tip.
> 
> Probably this in the mainline kernel is meant:
> 
> commit 3a27dd1ce5de08e21e0266ddf00e6f1f843bfe8b
> Author: Robert Richter <robert.richter@amd.com>
> Date:   Thu Jun 12 20:19:23 2008 +0200
> 
>     x86: Move PCI IO ECS code to x86/pci
>     
>     "Form follows function". Code is now where it belongs to.
>     
>     Signed-off-by: Robert Richter <robert.richter@amd.com>
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> The problem is code like this:
> 
> +static int __init enable_pci_io_ecs(void)
> +{
> +       /* assume all cpus from fam10h have IO ECS */
> +        if (boot_cpu_data.x86 < 0x10)
> +               return 0;
> +       on_each_cpu(enable_pci_io_ecs_per_cpu, NULL, 1, 1);
> +       pci_probe |= PCI_HAS_IO_ECS;
> +       return 0;
> +}
> 
> It is only called during the setup and if the cpu is enabled. I intend 
> to rewrite the cpu setup code using cpu notifiers.

ok. Unfortunately the revert looks quite non-trivial for v2.6.27. Could 
we have some _really_ minimal fix for v2.6.27?

	Ingo