public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug
@ 2008-08-21 12:59 Jan Beulich
  2008-08-21 13:29 ` Robert Richter
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Jan Beulich @ 2008-08-21 12:59 UTC (permalink / raw)
  To: robert.richter, Ingo Molnar; +Cc: linux-kernel

>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


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

* Re: patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug
  2008-08-21 12:59 patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug Jan Beulich
@ 2008-08-21 13:29 ` Robert Richter
  2008-08-21 16:25   ` Yinghai Lu
  2008-08-22 18:23 ` [PATCH] x86: fix: do not run code in amd_bus.c on non-AMD CPUs Robert Richter
  2008-08-22 18:23 ` [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable Robert Richter
  2 siblings, 1 reply; 18+ messages in thread
From: Robert Richter @ 2008-08-21 13:29 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Ingo Molnar, linux-kernel

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


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

* Re: patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug
  2008-08-21 13:29 ` Robert Richter
@ 2008-08-21 16:25   ` Yinghai Lu
  2008-08-21 16:46     ` Robert Richter
  0 siblings, 1 reply; 18+ messages in thread
From: Yinghai Lu @ 2008-08-21 16:25 UTC (permalink / raw)
  To: Robert Richter; +Cc: Jan Beulich, Ingo Molnar, linux-kernel

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

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

* Re: patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug
  2008-08-21 16:25   ` Yinghai Lu
@ 2008-08-21 16:46     ` Robert Richter
  0 siblings, 0 replies; 18+ messages in thread
From: Robert Richter @ 2008-08-21 16:46 UTC (permalink / raw)
  To: Yinghai Lu; +Cc: Jan Beulich, Ingo Molnar, linux-kernel

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


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

* [PATCH] x86: fix: do not run code in amd_bus.c on non-AMD CPUs
  2008-08-21 12:59 patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug Jan Beulich
  2008-08-21 13:29 ` Robert Richter
@ 2008-08-22 18:23 ` Robert Richter
  2008-08-22 18:47   ` Yinghai Lu
  2008-08-22 18:23 ` [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable Robert Richter
  2 siblings, 1 reply; 18+ messages in thread
From: Robert Richter @ 2008-08-22 18:23 UTC (permalink / raw)
  To: Ingo Molnar, Thomas Gleixner
  Cc: LKML, Robert Richter, Jan Beulich, Yinghai Lu

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



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

* [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable
  2008-08-21 12:59 patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug Jan Beulich
  2008-08-21 13:29 ` Robert Richter
  2008-08-22 18:23 ` [PATCH] x86: fix: do not run code in amd_bus.c on non-AMD CPUs Robert Richter
@ 2008-08-22 18:23 ` Robert Richter
  2008-08-22 18:56   ` Yinghai Lu
  2008-08-23 15:37   ` Ingo Molnar
  2 siblings, 2 replies; 18+ messages in thread
From: Robert Richter @ 2008-08-22 18:23 UTC (permalink / raw)
  To: Ingo Molnar, Thomas Gleixner
  Cc: LKML, Robert Richter, Jan Beulich, Yinghai Lu

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



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

* Re: [PATCH] x86: fix: do not run code in amd_bus.c on non-AMD CPUs
  2008-08-22 18:23 ` [PATCH] x86: fix: do not run code in amd_bus.c on non-AMD CPUs Robert Richter
@ 2008-08-22 18:47   ` Yinghai Lu
  2008-08-22 18:51     ` Robert Richter
  0 siblings, 1 reply; 18+ messages in thread
From: Yinghai Lu @ 2008-08-22 18:47 UTC (permalink / raw)
  To: Robert Richter; +Cc: Ingo Molnar, Thomas Gleixner, LKML, Jan Beulich

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

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

* Re: [PATCH] x86: fix: do not run code in amd_bus.c on non-AMD CPUs
  2008-08-22 18:47   ` Yinghai Lu
@ 2008-08-22 18:51     ` Robert Richter
  2008-08-22 18:57       ` Yinghai Lu
  0 siblings, 1 reply; 18+ messages in thread
From: Robert Richter @ 2008-08-22 18:51 UTC (permalink / raw)
  To: Yinghai Lu; +Cc: Ingo Molnar, Thomas Gleixner, LKML, Jan Beulich

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


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

* Re: [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable
  2008-08-22 18:23 ` [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable Robert Richter
@ 2008-08-22 18:56   ` Yinghai Lu
  2008-08-22 19:07     ` Robert Richter
  2008-08-23 15:37   ` Ingo Molnar
  1 sibling, 1 reply; 18+ messages in thread
From: Yinghai Lu @ 2008-08-22 18:56 UTC (permalink / raw)
  To: Robert Richter; +Cc: Ingo Molnar, Thomas Gleixner, LKML, Jan Beulich

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

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

* Re: [PATCH] x86: fix: do not run code in amd_bus.c on non-AMD CPUs
  2008-08-22 18:51     ` Robert Richter
@ 2008-08-22 18:57       ` Yinghai Lu
  2008-08-22 19:13         ` Robert Richter
  0 siblings, 1 reply; 18+ messages in thread
From: Yinghai Lu @ 2008-08-22 18:57 UTC (permalink / raw)
  To: Robert Richter; +Cc: Ingo Molnar, Thomas Gleixner, LKML, Jan Beulich

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

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

* Re: [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable
  2008-08-22 18:56   ` Yinghai Lu
@ 2008-08-22 19:07     ` Robert Richter
  2008-08-22 19:10       ` Yinghai Lu
  0 siblings, 1 reply; 18+ messages in thread
From: Robert Richter @ 2008-08-22 19:07 UTC (permalink / raw)
  To: Yinghai Lu; +Cc: Ingo Molnar, Thomas Gleixner, LKML, Jan Beulich

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


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

* Re: [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable
  2008-08-22 19:07     ` Robert Richter
@ 2008-08-22 19:10       ` Yinghai Lu
  2008-08-22 19:20         ` Robert Richter
  0 siblings, 1 reply; 18+ messages in thread
From: Yinghai Lu @ 2008-08-22 19:10 UTC (permalink / raw)
  To: Robert Richter; +Cc: Ingo Molnar, Thomas Gleixner, LKML, Jan Beulich

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

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

* Re: [PATCH] x86: fix: do not run code in amd_bus.c on non-AMD CPUs
  2008-08-22 18:57       ` Yinghai Lu
@ 2008-08-22 19:13         ` Robert Richter
  0 siblings, 0 replies; 18+ messages in thread
From: Robert Richter @ 2008-08-22 19:13 UTC (permalink / raw)
  To: Yinghai Lu; +Cc: Ingo Molnar, Thomas Gleixner, LKML, Jan Beulich

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


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

* Re: [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable
  2008-08-22 19:10       ` Yinghai Lu
@ 2008-08-22 19:20         ` Robert Richter
  0 siblings, 0 replies; 18+ messages in thread
From: Robert Richter @ 2008-08-22 19:20 UTC (permalink / raw)
  To: Yinghai Lu; +Cc: Ingo Molnar, Thomas Gleixner, LKML, Jan Beulich

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


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

* Re: [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable
  2008-08-22 18:23 ` [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable Robert Richter
  2008-08-22 18:56   ` Yinghai Lu
@ 2008-08-23 15:37   ` Ingo Molnar
  2008-08-23 15:39     ` Ingo Molnar
  2008-08-25  9:00     ` [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable (-v2) Robert Richter
  1 sibling, 2 replies; 18+ messages in thread
From: Ingo Molnar @ 2008-08-23 15:37 UTC (permalink / raw)
  To: Robert Richter; +Cc: Thomas Gleixner, LKML, Jan Beulich, Yinghai Lu


* 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

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

* Re: [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable
  2008-08-23 15:37   ` Ingo Molnar
@ 2008-08-23 15:39     ` Ingo Molnar
  2008-08-25  9:00     ` [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable (-v2) Robert Richter
  1 sibling, 0 replies; 18+ messages in thread
From: Ingo Molnar @ 2008-08-23 15:39 UTC (permalink / raw)
  To: Robert Richter; +Cc: Thomas Gleixner, LKML, Jan Beulich, Yinghai Lu


* 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

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

* [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable (-v2)
  2008-08-23 15:37   ` Ingo Molnar
  2008-08-23 15:39     ` Ingo Molnar
@ 2008-08-25  9:00     ` Robert Richter
  2008-08-25  9:12       ` Ingo Molnar
  1 sibling, 1 reply; 18+ messages in thread
From: Robert Richter @ 2008-08-25  9:00 UTC (permalink / raw)
  To: Ingo Molnar, Thomas Gleixner
  Cc: LKML, Robert Richter, Jan Beulich, Yinghai Lu

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



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

* Re: [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable (-v2)
  2008-08-25  9:00     ` [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable (-v2) Robert Richter
@ 2008-08-25  9:12       ` Ingo Molnar
  0 siblings, 0 replies; 18+ messages in thread
From: Ingo Molnar @ 2008-08-25  9:12 UTC (permalink / raw)
  To: Robert Richter; +Cc: Thomas Gleixner, LKML, Jan Beulich, Yinghai Lu


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

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

end of thread, other threads:[~2008-08-25  9:13 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-08-21 12:59 patch "x86: MOVE PCI IO ECS code to x86/pci" breaks CPU hotplug Jan Beulich
2008-08-21 13:29 ` Robert Richter
2008-08-21 16:25   ` Yinghai Lu
2008-08-21 16:46     ` Robert Richter
2008-08-22 18:23 ` [PATCH] x86: fix: do not run code in amd_bus.c on non-AMD CPUs Robert Richter
2008-08-22 18:47   ` Yinghai Lu
2008-08-22 18:51     ` Robert Richter
2008-08-22 18:57       ` Yinghai Lu
2008-08-22 19:13         ` Robert Richter
2008-08-22 18:23 ` [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable Robert Richter
2008-08-22 18:56   ` Yinghai Lu
2008-08-22 19:07     ` Robert Richter
2008-08-22 19:10       ` Yinghai Lu
2008-08-22 19:20         ` Robert Richter
2008-08-23 15:37   ` Ingo Molnar
2008-08-23 15:39     ` Ingo Molnar
2008-08-25  9:00     ` [PATCH] x86: fix: make PCI ECS for AMD CPUs hotplug capable (-v2) Robert Richter
2008-08-25  9:12       ` Ingo Molnar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox