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

[PATCH] x86: fix: do not run code in amd_bus.c on non-AMD CPUs

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

This patch adds cpu vendor check to the postcore_initcalls.

Cc: Jan Beulich <jbeulich@novell.com>
Cc: Yinghai Lu <yhlu.kernel@gmail.com>
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/pci/amd_bus.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/x86/pci/amd_bus.c b/arch/x86/pci/amd_bus.c
index dbf5323..4a6f1a6 100644
--- a/arch/x86/pci/amd_bus.c
+++ b/arch/x86/pci/amd_bus.c
@@ -555,9 +555,11 @@ static int __init early_fill_mp_bus_info(void)
 	return 0;
 }
 
-postcore_initcall(early_fill_mp_bus_info);
+#else  /* !CONFIG_X86_64 */
 
-#endif
+static int __init early_fill_mp_bus_info(void) { return 0; }
+
+#endif /* !CONFIG_X86_64 */
 
 /* common 32/64 bit code */
 
@@ -583,4 +585,15 @@ static int __init enable_pci_io_ecs(void)
 	return 0;
 }
 
-postcore_initcall(enable_pci_io_ecs);
+static int __init amd_postcore_init(void)
+{
+	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
+		return 0;
+
+	early_fill_mp_bus_info();
+	enable_pci_io_ecs();
+
+	return 0;
+}
+
+postcore_initcall(amd_postcore_init);
-- 
1.5.6.4

[PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable

[Robert Richter]

Until now, PCI ECS setup was performed at boot time only and for cpus
that are enabled then. This patch fixes this and adds cpu hotplug.

Tests sequence (check if ECS bit is set when bringing cpu online again):

 # ( perl -e 'sysseek(STDIN, 0xC001001F, 0)'; hexdump -n 8 -e '2/4 "%08x " "\n"' )   < /dev/cpu/1/msr
 00000008 00404010
 # ( perl -e 'sysseek(STDOUT, 0xC001001F, 0); print pack "l*", 8, 0x00400010' ) > /dev/cpu/1/msr
 # ( perl -e 'sysseek(STDIN, 0xC001001F, 0)'; hexdump -n 8 -e '2/4 "%08x " "\n"' )   < /dev/cpu/1/msr
 00000008 00400010
 # echo 0 > /sys/devices/system/cpu/cpu1/online
 # echo 1 > /sys/devices/system/cpu/cpu1/online
 # ( perl -e 'sysseek(STDIN, 0xC001001F, 0)'; hexdump -n 8 -e '2/4 "%08x " "\n"' )   < /dev/cpu/1/msr
 00000008 00404010

Cc: Jan Beulich <jbeulich@novell.com>
Cc: Yinghai Lu <yhlu.kernel@gmail.com>
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/pci/amd_bus.c |   35 +++++++++++++++++++++++++++++++----
 1 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/arch/x86/pci/amd_bus.c b/arch/x86/pci/amd_bus.c
index 4a6f1a6..6a0fca7 100644
--- a/arch/x86/pci/amd_bus.c
+++ b/arch/x86/pci/amd_bus.c
@@ -1,6 +1,7 @@
 #include <linux/init.h>
 #include <linux/pci.h>
 #include <linux/topology.h>
+#include <linux/cpu.h>
 #include "pci.h"
 
 #ifdef CONFIG_X86_64
@@ -565,7 +566,7 @@ static int __init early_fill_mp_bus_info(void) { return 0; }
 
 #define ENABLE_CF8_EXT_CFG      (1ULL << 46)
 
-static void enable_pci_io_ecs_per_cpu(void *unused)
+static void enable_pci_io_ecs(void *unused)
 {
 	u64 reg;
 	rdmsrl(MSR_AMD64_NB_CFG, reg);
@@ -575,13 +576,39 @@ static void enable_pci_io_ecs_per_cpu(void *unused)
 	}
 }
 
-static int __init enable_pci_io_ecs(void)
+static int __cpuinit amd_cpu_notify(struct notifier_block *self,
+				    unsigned long action, void *hcpu)
 {
+	int cpu = (long)hcpu;
+	switch(action) {
+	case CPU_ONLINE:
+	case CPU_ONLINE_FROZEN:
+		smp_call_function_single(cpu, enable_pci_io_ecs, NULL, 0);
+		break;
+	default:
+		break;
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block __cpuinitdata amd_cpu_notifier = {
+	.notifier_call	= amd_cpu_notify,
+};
+
+static int __init pci_io_ecs_init(void)
+{
+	int cpu;
+
 	/* 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);
+
+	register_cpu_notifier(&amd_cpu_notifier);
+	for_each_online_cpu(cpu)
+		amd_cpu_notify(&amd_cpu_notifier, (unsigned long)CPU_ONLINE,
+			       (void *)(long)cpu);
 	pci_probe |= PCI_HAS_IO_ECS;
+
 	return 0;
 }
 
@@ -591,7 +618,7 @@ static int __init amd_postcore_init(void)
 		return 0;
 
 	early_fill_mp_bus_info();
-	enable_pci_io_ecs();
+	pci_io_ecs_init();
 
 	return 0;
 }
-- 
1.5.6.4

Re: [PATCH] x86: fix: do not run code in amd_bus.c on non-AMD CPUs

[Yinghai Lu]

On Fri, Aug 22, 2008 at 11:23 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).
>
> This patch adds cpu vendor check to the postcore_initcalls.
>
> Cc: Jan Beulich <jbeulich@novell.com>
> Cc: Yinghai Lu <yhlu.kernel@gmail.com>
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  arch/x86/pci/amd_bus.c |   19 ++++++++++++++++---
>  1 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/pci/amd_bus.c b/arch/x86/pci/amd_bus.c
> index dbf5323..4a6f1a6 100644
> --- a/arch/x86/pci/amd_bus.c
> +++ b/arch/x86/pci/amd_bus.c
> @@ -555,9 +555,11 @@ static int __init early_fill_mp_bus_info(void)
>        return 0;
>  }
>
> -postcore_initcall(early_fill_mp_bus_info);
> +#else  /* !CONFIG_X86_64 */
>
> -#endif
> +static int __init early_fill_mp_bus_info(void) { return 0; }
> +
> +#endif /* !CONFIG_X86_64 */
>
>  /* common 32/64 bit code */
>
> @@ -583,4 +585,15 @@ static int __init enable_pci_io_ecs(void)
>        return 0;
>  }
>
> -postcore_initcall(enable_pci_io_ecs);
> +static int __init amd_postcore_init(void)
> +{
> +       if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> +               return 0;
> +
> +       early_fill_mp_bus_info();
> +       enable_pci_io_ecs();
> +
> +       return 0;
> +}
> +
> +postcore_initcall(amd_postcore_init);

1. early_fill_mp_bus_info has checking with vendor/deviceid in itself.

2. you didn't address: when cpu is hot plug in later,
enable_pci_to_ecs_per_cpu will not be called.
( maxcpus and addtional_cpus)

YH

Re: [PATCH] x86: fix: do not run code in amd_bus.c on non-AMD CPUs

[Robert Richter]

On 22.08.08 11:47:45, Yinghai Lu wrote:
> 1. early_fill_mp_bus_info has checking with vendor/deviceid in itself.

Right, but scanning the pci bus could be skipped for non-AMD cpus. If
you don't like this check for your function I will resubmit a new
patch.

> 2. you didn't address: when cpu is hot plug in later,
> enable_pci_to_ecs_per_cpu will not be called.
> ( maxcpus and addtional_cpus)

Next patch?

-Robert

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

Re: [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable

[Yinghai Lu]

On Fri, Aug 22, 2008 at 11:23 AM, Robert Richter <robert.richter@amd.com> wrote:
> Until now, PCI ECS setup was performed at boot time only and for cpus
> that are enabled then. This patch fixes this and adds cpu hotplug.
>
> Tests sequence (check if ECS bit is set when bringing cpu online again):
>
>  # ( perl -e 'sysseek(STDIN, 0xC001001F, 0)'; hexdump -n 8 -e '2/4 "%08x " "\n"' )   < /dev/cpu/1/msr
>  00000008 00404010
>  # ( perl -e 'sysseek(STDOUT, 0xC001001F, 0); print pack "l*", 8, 0x00400010' ) > /dev/cpu/1/msr
>  # ( perl -e 'sysseek(STDIN, 0xC001001F, 0)'; hexdump -n 8 -e '2/4 "%08x " "\n"' )   < /dev/cpu/1/msr
>  00000008 00400010
>  # echo 0 > /sys/devices/system/cpu/cpu1/online
>  # echo 1 > /sys/devices/system/cpu/cpu1/online
>  # ( perl -e 'sysseek(STDIN, 0xC001001F, 0)'; hexdump -n 8 -e '2/4 "%08x " "\n"' )   < /dev/cpu/1/msr
>  00000008 00404010
>
> Cc: Jan Beulich <jbeulich@novell.com>
> Cc: Yinghai Lu <yhlu.kernel@gmail.com>
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  arch/x86/pci/amd_bus.c |   35 +++++++++++++++++++++++++++++++----
>  1 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/pci/amd_bus.c b/arch/x86/pci/amd_bus.c
> index 4a6f1a6..6a0fca7 100644
> --- a/arch/x86/pci/amd_bus.c
> +++ b/arch/x86/pci/amd_bus.c
> @@ -1,6 +1,7 @@
>  #include <linux/init.h>
>  #include <linux/pci.h>
>  #include <linux/topology.h>
> +#include <linux/cpu.h>
>  #include "pci.h"
>
>  #ifdef CONFIG_X86_64
> @@ -565,7 +566,7 @@ static int __init early_fill_mp_bus_info(void) { return 0; }
>
>  #define ENABLE_CF8_EXT_CFG      (1ULL << 46)
>
> -static void enable_pci_io_ecs_per_cpu(void *unused)
> +static void enable_pci_io_ecs(void *unused)
>  {
>        u64 reg;
>        rdmsrl(MSR_AMD64_NB_CFG, reg);
> @@ -575,13 +576,39 @@ static void enable_pci_io_ecs_per_cpu(void *unused)
>        }
>  }
>
> -static int __init enable_pci_io_ecs(void)
> +static int __cpuinit amd_cpu_notify(struct notifier_block *self,
> +                                   unsigned long action, void *hcpu)
>  {
> +       int cpu = (long)hcpu;
> +       switch(action) {
> +       case CPU_ONLINE:
> +       case CPU_ONLINE_FROZEN:
> +               smp_call_function_single(cpu, enable_pci_io_ecs, NULL, 0);
> +               break;
> +       default:
> +               break;
> +       }
> +       return NOTIFY_OK;
> +}
> +
> +static struct notifier_block __cpuinitdata amd_cpu_notifier = {
> +       .notifier_call  = amd_cpu_notify,
> +};
> +
> +static int __init pci_io_ecs_init(void)
> +{
> +       int cpu;
> +
>        /* 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);
> +
> +       register_cpu_notifier(&amd_cpu_notifier);
> +       for_each_online_cpu(cpu)
> +               amd_cpu_notify(&amd_cpu_notifier, (unsigned long)CPU_ONLINE,
> +                              (void *)(long)cpu);

wonder if those two lines should be reversed.

>        pci_probe |= PCI_HAS_IO_ECS;
> +
>        return 0;
>  }
>
> @@ -591,7 +618,7 @@ static int __init amd_postcore_init(void)
>                return 0;
>
>        early_fill_mp_bus_info();
> -       enable_pci_io_ecs();
> +       pci_io_ecs_init();
>
>        return 0;
>  }
> --
> 1.5.6.4
>
>
>

Re: [PATCH] x86: fix: do not run code in amd_bus.c on non-AMD CPUs

[Yinghai Lu]

On Fri, Aug 22, 2008 at 11:51 AM, Robert Richter <robert.richter@amd.com> wrote:
> On 22.08.08 11:47:45, Yinghai Lu wrote:
>> 1. early_fill_mp_bus_info has checking with vendor/deviceid in itself.
>
> Right, but scanning the pci bus could be skipped for non-AMD cpus. If
> you don't like this check for your function I will resubmit a new
> patch.

just fine, if you check vendor before that, we should remove check
vendor again in later function.

YH

Re: [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable

[Robert Richter]

On 22.08.08 11:56:08, Yinghai Lu wrote:

[...]

> > +static int __init pci_io_ecs_init(void)
> > +{
> > +       int cpu;
> > +
> >        /* 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);
> > +
> > +       register_cpu_notifier(&amd_cpu_notifier);
> > +       for_each_online_cpu(cpu)
> > +               amd_cpu_notify(&amd_cpu_notifier, (unsigned long)CPU_ONLINE,
> > +                              (void *)(long)cpu);
> 
> wonder if those two lines should be reversed.

Do you mean setting PCI_HAS_IO_ECS before for_each_online_cpu(...)?
PCI_HAS_IO_ECS will be used only in pci_direct_init(). PCI is
initialized in a later init stage, so it doesn't matter. My intention
was to set the bit after the setup of all online cpus is finished.

-Robert

> 
> >        pci_probe |= PCI_HAS_IO_ECS;
> > +
> >        return 0;
> >  }
> >
> > @@ -591,7 +618,7 @@ static int __init amd_postcore_init(void)
> >                return 0;
> >
> >        early_fill_mp_bus_info();
> > -       enable_pci_io_ecs();
> > +       pci_io_ecs_init();
> >
> >        return 0;
> >  }
> > --
> > 1.5.6.4
> >
> >
> >
> 

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

Re: [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable

[Yinghai Lu]

On Fri, Aug 22, 2008 at 12:07 PM, Robert Richter <robert.richter@amd.com> wrote:
> On 22.08.08 11:56:08, Yinghai Lu wrote:
>
> [...]
>
>> > +static int __init pci_io_ecs_init(void)
>> > +{
>> > +       int cpu;
>> > +
>> >        /* 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);
>> > +
>> > +       register_cpu_notifier(&amd_cpu_notifier);
>> > +       for_each_online_cpu(cpu)
>> > +               amd_cpu_notify(&amd_cpu_notifier, (unsigned long)CPU_ONLINE,
>> > +                              (void *)(long)cpu);
>>
>> wonder if those two lines should be reversed.
>
> Do you mean setting PCI_HAS_IO_ECS before for_each_online_cpu(...)?
> PCI_HAS_IO_ECS will be used only in pci_direct_init(). PCI is
> initialized in a later init stage, so it doesn't matter. My intention
> was to set the bit after the setup of all online cpus is finished.


==>
+       for_each_online_cpu(cpu)
+               amd_cpu_notify(&amd_cpu_notifier, (unsigned long)CPU_ONLINE,
+                              (void *)(long)cpu);
+       register_cpu_notifier(&amd_cpu_notifier);

YH

Re: [PATCH] x86: fix: do not run code in amd_bus.c on non-AMD CPUs

[Robert Richter]

On 22.08.08 11:57:39, Yinghai Lu wrote:
> On Fri, Aug 22, 2008 at 11:51 AM, Robert Richter <robert.richter@amd.com> wrote:
> > On 22.08.08 11:47:45, Yinghai Lu wrote:
> >> 1. early_fill_mp_bus_info has checking with vendor/deviceid in itself.
> >
> > Right, but scanning the pci bus could be skipped for non-AMD cpus. If
> > you don't like this check for your function I will resubmit a new
> > patch.
> 
> just fine, if you check vendor before that, we should remove check
> vendor again in later function.

Right. There are no more checks in, only the PCI vendor check that can
not be removed.

-Robert

> 
> YH
> 

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

Re: [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable

[Robert Richter]

On 22.08.08 12:10:04, Yinghai Lu wrote:
> On Fri, Aug 22, 2008 at 12:07 PM, Robert Richter <robert.richter@amd.com> wrote:
> > On 22.08.08 11:56:08, Yinghai Lu wrote:
> >
> > [...]
> >
> >> > +static int __init pci_io_ecs_init(void)
> >> > +{
> >> > +       int cpu;
> >> > +
> >> >        /* 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);
> >> > +
> >> > +       register_cpu_notifier(&amd_cpu_notifier);
> >> > +       for_each_online_cpu(cpu)
> >> > +               amd_cpu_notify(&amd_cpu_notifier, (unsigned long)CPU_ONLINE,
> >> > +                              (void *)(long)cpu);
> >>
> >> wonder if those two lines should be reversed.
> >
> > Do you mean setting PCI_HAS_IO_ECS before for_each_online_cpu(...)?
> > PCI_HAS_IO_ECS will be used only in pci_direct_init(). PCI is
> > initialized in a later init stage, so it doesn't matter. My intention
> > was to set the bit after the setup of all online cpus is finished.
> 
> 
> ==>
> +       for_each_online_cpu(cpu)
> +               amd_cpu_notify(&amd_cpu_notifier, (unsigned long)CPU_ONLINE,
> +                              (void *)(long)cpu);
> +       register_cpu_notifier(&amd_cpu_notifier);

Hmm, most code in the kernel registers first. Theoretical, with this
code, if a cpu would go online in between, it wouldn't be initialized.

-Robert

> 
> YH
> 

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

Re: [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable

[Ingo Molnar]

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

> Until now, PCI ECS setup was performed at boot time only and for cpus 
> that are enabled then. This patch fixes this and adds cpu hotplug.

this patch does not apply cleanly to -git.

	Ingo

Re: [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> * Robert Richter <robert.richter@amd.com> wrote:
> 
> > Until now, PCI ECS setup was performed at boot time only and for 
> > cpus that are enabled then. This patch fixes this and adds cpu 
> > hotplug.
> 
> this patch does not apply cleanly to -git.

i see, it depends on the other patch. Applied both to tip/x86/urgent.

	Ingo

[PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable (-v2)

[Robert Richter]

Until now, PCI ECS setup was performed at boot time only and for cpus
that are enabled then. This patch fixes this and adds cpu hotplug.

Tests sequence (check if ECS bit is set when bringing cpu online again):

 # ( perl -e 'sysseek(STDIN, 0xC001001F, 0)'; hexdump -n 8 -e '2/4 "%08x " "\n"' )   < /dev/cpu/1/msr
 00000008 00404010
 # ( perl -e 'sysseek(STDOUT, 0xC001001F, 0); print pack "l*", 8, 0x00400010' ) > /dev/cpu/1/msr
 # ( perl -e 'sysseek(STDIN, 0xC001001F, 0)'; hexdump -n 8 -e '2/4 "%08x " "\n"' )   < /dev/cpu/1/msr
 00000008 00400010
 # echo 0 > /sys/devices/system/cpu/cpu1/online
 # echo 1 > /sys/devices/system/cpu/cpu1/online
 # ( perl -e 'sysseek(STDIN, 0xC001001F, 0)'; hexdump -n 8 -e '2/4 "%08x " "\n"' )   < /dev/cpu/1/msr
 00000008 00404010

Cc: Jan Beulich <jbeulich@novell.com>
Cc: Yinghai Lu <yhlu.kernel@gmail.com>
Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/pci/amd_bus.c |   35 +++++++++++++++++++++++++++++++----
 1 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/arch/x86/pci/amd_bus.c b/arch/x86/pci/amd_bus.c
index 4a6f1a6..22e0576 100644
--- a/arch/x86/pci/amd_bus.c
+++ b/arch/x86/pci/amd_bus.c
@@ -1,6 +1,7 @@
 #include <linux/init.h>
 #include <linux/pci.h>
 #include <linux/topology.h>
+#include <linux/cpu.h>
 #include "pci.h"
 
 #ifdef CONFIG_X86_64
@@ -565,7 +566,7 @@ static int __init early_fill_mp_bus_info(void) { return 0; }
 
 #define ENABLE_CF8_EXT_CFG      (1ULL << 46)
 
-static void enable_pci_io_ecs_per_cpu(void *unused)
+static void enable_pci_io_ecs(void *unused)
 {
 	u64 reg;
 	rdmsrl(MSR_AMD64_NB_CFG, reg);
@@ -575,13 +576,39 @@ static void enable_pci_io_ecs_per_cpu(void *unused)
 	}
 }
 
-static int __init enable_pci_io_ecs(void)
+static int __cpuinit amd_cpu_notify(struct notifier_block *self,
+				    unsigned long action, void *hcpu)
 {
+	int cpu = (long)hcpu;
+	switch (action) {
+	case CPU_ONLINE:
+	case CPU_ONLINE_FROZEN:
+		smp_call_function_single(cpu, enable_pci_io_ecs, NULL, 0);
+		break;
+	default:
+		break;
+	}
+	return NOTIFY_OK;
+}
+
+static struct notifier_block __cpuinitdata amd_cpu_notifier = {
+	.notifier_call	= amd_cpu_notify,
+};
+
+static int __init pci_io_ecs_init(void)
+{
+	int cpu;
+
 	/* 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);
+
+	register_cpu_notifier(&amd_cpu_notifier);
+	for_each_online_cpu(cpu)
+		amd_cpu_notify(&amd_cpu_notifier, (unsigned long)CPU_ONLINE,
+			       (void *)(long)cpu);
 	pci_probe |= PCI_HAS_IO_ECS;
+
 	return 0;
 }
 
@@ -591,7 +618,7 @@ static int __init amd_postcore_init(void)
 		return 0;
 
 	early_fill_mp_bus_info();
-	enable_pci_io_ecs();
+	pci_io_ecs_init();
 
 	return 0;
 }
-- 
1.5.6.4

Re: [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable (-v2)

[Ingo Molnar]

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

> Until now, PCI ECS setup was performed at boot time only and for cpus 
> that are enabled then. This patch fixes this and adds cpu hotplug.

i already had v1 applied to tip/x86/urgent, so i took the -v2 delta and 
queued it up in tip/x86/cleanups - see the commit below.

	Ingo

--------------->
>From ed21763e7b0b3fb50e4efd9d4bc17ef5b035d304 Mon Sep 17 00:00:00 2001
From: Robert Richter <robert.richter@amd.com>
Date: Fri, 22 Aug 2008 20:23:38 +0200
Subject: [PATCH] x86: cleanup in amd_cpu_notify()

small coding style fix.

Signed-off-by: Robert Richter <robert.richter@amd.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/pci/amd_bus.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/pci/amd_bus.c b/arch/x86/pci/amd_bus.c
index 6a0fca7..22e0576 100644
--- a/arch/x86/pci/amd_bus.c
+++ b/arch/x86/pci/amd_bus.c
@@ -580,7 +580,7 @@ static int __cpuinit amd_cpu_notify(struct notifier_block *self,
 				    unsigned long action, void *hcpu)
 {
 	int cpu = (long)hcpu;
-	switch(action) {
+	switch (action) {
 	case CPU_ONLINE:
 	case CPU_ONLINE_FROZEN:
 		smp_call_function_single(cpu, enable_pci_io_ecs, NULL, 0);