Hi Ingo, Andrew,
Here are a few updates to the vSMP architecture bits in the x86 tree.
This is a set of 4 patches which do the following:
1. Fix is_vsmp_box() to actually detect if the system is a vSMPowered box
2. Fix build issue when CONFIG_PCI is enabled, but CONFIG_PARAVIRT is not
3. Set paravirt ops only if the platform has capability to recognize
paravirtualized irq operations
4. Update apic_is_clustered_box() to indicate unsynched TSCs on multiboard
vSMPowered systems
The patches have been tested against 2.6.25-rc5-mm1 on vSMPowered and non
vSMPowered x86 boxes.
Please apply
Thanks,
Kiran
is_vsmp_box() currently does not work on vSMPowered systems, as pci cfg
space is not read correctly -- This patch fixes it.
Signed-off-by: Ravikiran Thirumalai <[email protected]>
Index: linux.git.trees/arch/x86/kernel/vsmp_64.c
===================================================================
--- linux.git.trees.orig/arch/x86/kernel/vsmp_64.c 2008-03-19 13:30:35.116766719 -0700
+++ linux.git.trees/arch/x86/kernel/vsmp_64.c 2008-03-19 13:39:20.074685590 -0700
@@ -84,8 +84,10 @@ int is_vsmp_box(void)
return vsmp;
/* Check if we are running on a ScaleMP vSMP box */
- if (read_pci_config(0, 0x1f, 0, PCI_VENDOR_ID) ==
- (PCI_VENDOR_ID_SCALEMP || (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16)))
+ if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
+ PCI_VENDOR_ID_SCALEMP) &&
+ (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
+ PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
vsmp = 1;
return vsmp;
- Fix the the build breakage when PARAVIRT is defined
but PCI is not
This fixes problem reported at:
http://marc.info/?l=linux-kernel&m=120525966600698&w=2
- Make is_vsmp_box() available even when PARAVIRT is not defined.
This is needed to determine if tsc's are reliable as a time source
even when PARAVIRT is not defined.
- split vsmp_init to use is_vsmp_box() and set_vsmp_pv_ops()
set_vsmp_pv_ops will do nothing if PCI is not enabled in the config.
Signed-off-by: Ravikiran Thirumalai <[email protected]>
Index: linux.git.trees/arch/x86/kernel/Makefile
===================================================================
--- linux.git.trees.orig/arch/x86/kernel/Makefile 2008-03-19 13:39:29.000000000 -0700
+++ linux.git.trees/arch/x86/kernel/Makefile 2008-03-19 13:46:14.400935607 -0700
@@ -60,7 +60,7 @@ obj-$(CONFIG_KEXEC) += relocate_kernel_
obj-$(CONFIG_CRASH_DUMP) += crash_dump_$(BITS).o
obj-$(CONFIG_X86_NUMAQ) += numaq_32.o
obj-$(CONFIG_X86_SUMMIT_NUMA) += summit_32.o
-obj-$(CONFIG_PARAVIRT) += vsmp_64.o
+obj-y += vsmp_64.o
obj-$(CONFIG_KPROBES) += kprobes.o
obj-$(CONFIG_MODULES) += module_$(BITS).o
obj-$(CONFIG_ACPI_SRAT) += srat_32.o
Index: linux.git.trees/arch/x86/kernel/setup_64.c
===================================================================
--- linux.git.trees.orig/arch/x86/kernel/setup_64.c 2008-03-19 13:39:29.000000000 -0700
+++ linux.git.trees/arch/x86/kernel/setup_64.c 2008-03-19 13:57:43.951337318 -0700
@@ -353,9 +353,7 @@ void __init setup_arch(char **cmdline_p)
if (efi_enabled)
efi_init();
-#ifdef CONFIG_PARAVIRT
vsmp_init();
-#endif
dmi_scan_machine();
Index: linux.git.trees/arch/x86/kernel/vsmp_64.c
===================================================================
--- linux.git.trees.orig/arch/x86/kernel/vsmp_64.c 2008-03-19 13:39:50.000000000 -0700
+++ linux.git.trees/arch/x86/kernel/vsmp_64.c 2008-03-19 20:53:34.267707163 -0700
@@ -19,6 +19,7 @@
#include <asm/io.h>
#include <asm/paravirt.h>
+#if defined CONFIG_PCI && defined CONFIG_PARAVIRT
/*
* Interrupt control on vSMPowered systems:
* ~AC is a shadow of IF. If IF is 'on' AC should be 'off'
@@ -72,39 +73,11 @@ static unsigned __init vsmp_patch(u8 typ
}
-static int vsmp = -1;
-
-int is_vsmp_box(void)
-{
- if (vsmp != -1)
- return vsmp;
-
- vsmp = 0;
- if (!early_pci_allowed())
- return vsmp;
-
- /* Check if we are running on a ScaleMP vSMP box */
- if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
- PCI_VENDOR_ID_SCALEMP) &&
- (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
- PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
- vsmp = 1;
-
- return vsmp;
-}
-
-void __init vsmp_init(void)
+static void __init set_vsmp_pv_ops(void)
{
void *address;
unsigned int cap, ctl, cfg;
- if (!is_vsmp_box())
- return;
-
- if (!early_pci_allowed())
- return;
-
- /* If we are, use the distinguished irq functions */
pv_irq_ops.irq_disable = vsmp_irq_disable;
pv_irq_ops.irq_enable = vsmp_irq_enable;
pv_irq_ops.save_fl = vsmp_save_fl;
@@ -127,5 +100,46 @@ void __init vsmp_init(void)
}
early_iounmap(address, 8);
+}
+#else
+static void __init set_vsmp_pv_ops(void)
+{
+}
+#endif
+
+#ifdef CONFIG_PCI
+static int vsmp = -1;
+
+int is_vsmp_box(void)
+{
+ if (vsmp != -1)
+ return vsmp;
+
+ vsmp = 0;
+ if (!early_pci_allowed())
+ return vsmp;
+
+ /* Check if we are running on a ScaleMP vSMP box */
+ if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
+ PCI_VENDOR_ID_SCALEMP) &&
+ (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
+ PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
+ vsmp = 1;
+
+ return vsmp;
+}
+#else
+int is_vsmp_box(void)
+{
+ return 0;
+}
+#endif
+
+void __init vsmp_init(void)
+{
+ if (!is_vsmp_box())
+ return;
+
+ set_vsmp_pv_ops();
return;
}
Index: linux.git.trees/include/asm-x86/apic.h
===================================================================
--- linux.git.trees.orig/include/asm-x86/apic.h 2008-03-19 13:39:29.000000000 -0700
+++ linux.git.trees/include/asm-x86/apic.h 2008-03-19 13:57:14.550893819 -0700
@@ -51,19 +51,16 @@ extern unsigned boot_cpu_id;
*/
#ifdef CONFIG_PARAVIRT
#include <asm/paravirt.h>
-extern int is_vsmp_box(void);
#else
#define apic_write native_apic_write
#define apic_write_atomic native_apic_write_atomic
#define apic_read native_apic_read
#define setup_boot_clock setup_boot_APIC_clock
#define setup_secondary_clock setup_secondary_APIC_clock
-static int inline is_vsmp_box(void)
-{
- return 0;
-}
#endif
+extern int is_vsmp_box(void);
+
static inline void native_apic_write(unsigned long reg, u32 v)
{
*((volatile u32 *)(APIC_BASE + reg)) = v;
Re-arrange set_vsmp_pv_ops so that pv_ops are set only if
the platform has capability to support paravirtualized irq ops
Signed-off-by: Ravikiran Thirumalai <[email protected]>
Index: linux.git.trees/arch/x86/kernel/vsmp_64.c
===================================================================
--- linux.git.trees.orig/arch/x86/kernel/vsmp_64.c 2008-03-19 20:53:34.267707163 -0700
+++ linux.git.trees/arch/x86/kernel/vsmp_64.c 2008-03-19 21:21:47.853254516 -0700
@@ -78,12 +78,6 @@ static void __init set_vsmp_pv_ops(void)
void *address;
unsigned int cap, ctl, cfg;
- pv_irq_ops.irq_disable = vsmp_irq_disable;
- pv_irq_ops.irq_enable = vsmp_irq_enable;
- pv_irq_ops.save_fl = vsmp_save_fl;
- pv_irq_ops.restore_fl = vsmp_restore_fl;
- pv_init_ops.patch = vsmp_patch;
-
/* set vSMP magic bits to indicate vSMP capable kernel */
cfg = read_pci_config(0, 0x1f, 0, PCI_BASE_ADDRESS_0);
address = early_ioremap(cfg, 8);
@@ -92,7 +86,13 @@ static void __init set_vsmp_pv_ops(void)
printk(KERN_INFO "vSMP CTL: capabilities:0x%08x control:0x%08x\n",
cap, ctl);
if (cap & ctl & (1 << 4)) {
- /* Turn on vSMP IRQ fastpath handling (see system.h) */
+ /* Setup irq ops and turn on vSMP IRQ fastpath handling */
+ pv_irq_ops.irq_disable = vsmp_irq_disable;
+ pv_irq_ops.irq_enable = vsmp_irq_enable;
+ pv_irq_ops.save_fl = vsmp_save_fl;
+ pv_irq_ops.restore_fl = vsmp_restore_fl;
+ pv_init_ops.patch = vsmp_patch;
+
ctl &= ~(1 << 4);
writel(ctl, address + 4);
ctl = readl(address + 4);
On Thu, Mar 20, 2008 at 12:39 AM, Ravikiran G Thirumalai
<[email protected]> wrote:
> is_vsmp_box() currently does not work on vSMPowered systems, as pci cfg
> space is not read correctly -- This patch fixes it.
>
> Signed-off-by: Ravikiran Thirumalai <[email protected]>
>
> Index: linux.git.trees/arch/x86/kernel/vsmp_64.c
> ===================================================================
> --- linux.git.trees.orig/arch/x86/kernel/vsmp_64.c 2008-03-19 13:30:35.116766719 -0700
> +++ linux.git.trees/arch/x86/kernel/vsmp_64.c 2008-03-19 13:39:20.074685590 -0700
> @@ -84,8 +84,10 @@ int is_vsmp_box(void)
> return vsmp;
>
> /* Check if we are running on a ScaleMP vSMP box */
> - if (read_pci_config(0, 0x1f, 0, PCI_VENDOR_ID) ==
> - (PCI_VENDOR_ID_SCALEMP || (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16)))
> + if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
> + PCI_VENDOR_ID_SCALEMP) &&
> + (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
> + PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
> vsmp = 1;
>
> return vsmp;
why read two times
or
(PCI_VENDOR_ID_SCALEMP || (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16)))
should be
(PCI_VENDOR_ID_SCALEMP | (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16)))
YH
Indicate TSCs are unreliable as time sources if the platform is
a multi chassi ScaleMP vSMPowered machine.
Signed-off-by: Ravikiran Thirumalai <[email protected]>
Index: linux.git.trees/arch/x86/kernel/apic_64.c
===================================================================
--- linux.git.trees.orig/arch/x86/kernel/apic_64.c 2008-03-19 20:42:28.187659497 -0700
+++ linux.git.trees/arch/x86/kernel/apic_64.c 2008-03-19 21:24:17.705515003 -0700
@@ -1208,7 +1208,7 @@ __cpuinit int apic_is_clustered_box(void
* will be [4, 0x23] or [8, 0x27] could be thought to
* vsmp box still need checking...
*/
- if (!is_vsmp_box() && (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
+ if ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) && !is_vsmp_box())
return 0;
bios_cpu_apicid = x86_bios_cpu_apicid_early_ptr;
@@ -1248,6 +1248,12 @@ __cpuinit int apic_is_clustered_box(void
++zeros;
}
+ /* ScaleMP vSMPowered boxes have one cluster per board and TSCs are
+ * not guaranteed to be synced between boards
+ */
+ if (is_vsmp_box() && clusters > 1)
+ return 1;
+
/*
* If clusters > 2, then should be multi-chassis.
* May have to revisit this when multi-core + hyperthreaded CPUs come
On Thu, Mar 20, 2008 at 12:45 AM, Ravikiran G Thirumalai
<[email protected]> wrote:
> Indicate TSCs are unreliable as time sources if the platform is
> a multi chassi ScaleMP vSMPowered machine.
>
> Signed-off-by: Ravikiran Thirumalai <[email protected]>
>
> Index: linux.git.trees/arch/x86/kernel/apic_64.c
> ===================================================================
> --- linux.git.trees.orig/arch/x86/kernel/apic_64.c 2008-03-19 20:42:28.187659497 -0700
> +++ linux.git.trees/arch/x86/kernel/apic_64.c 2008-03-19 21:24:17.705515003 -0700
> @@ -1208,7 +1208,7 @@ __cpuinit int apic_is_clustered_box(void
> * will be [4, 0x23] or [8, 0x27] could be thought to
> * vsmp box still need checking...
> */
> - if (!is_vsmp_box() && (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
> + if ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) && !is_vsmp_box())
> return 0;
why?
>
> bios_cpu_apicid = x86_bios_cpu_apicid_early_ptr;
> @@ -1248,6 +1248,12 @@ __cpuinit int apic_is_clustered_box(void
> ++zeros;
> }
>
> + /* ScaleMP vSMPowered boxes have one cluster per board and TSCs are
> + * not guaranteed to be synced between boards
> + */
> + if (is_vsmp_box() && clusters > 1)
> + return 1;
you still call that for intel box.
YH
On Thu, Mar 20, 2008 at 12:44:37AM -0700, Yinghai Lu wrote:
>On Thu, Mar 20, 2008 at 12:39 AM, Ravikiran G Thirumalai
><[email protected]> wrote:
>> is_vsmp_box() currently does not work on vSMPowered systems, as pci cfg
>> space is not read correctly -- This patch fixes it.
>>
>> Signed-off-by: Ravikiran Thirumalai <[email protected]>
>>
>> Index: linux.git.trees/arch/x86/kernel/vsmp_64.c
>> ===================================================================
>> --- linux.git.trees.orig/arch/x86/kernel/vsmp_64.c 2008-03-19 13:30:35.116766719 -0700
>> +++ linux.git.trees/arch/x86/kernel/vsmp_64.c 2008-03-19 13:39:20.074685590 -0700
>> @@ -84,8 +84,10 @@ int is_vsmp_box(void)
>> return vsmp;
>>
>> /* Check if we are running on a ScaleMP vSMP box */
>> - if (read_pci_config(0, 0x1f, 0, PCI_VENDOR_ID) ==
>> - (PCI_VENDOR_ID_SCALEMP || (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16)))
>> + if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
>> + PCI_VENDOR_ID_SCALEMP) &&
>> + (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
>> + PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
>> vsmp = 1;
>>
>> return vsmp;
>
>why read two times
>
Well, the pci cfg space read happens just _once_ during the boot, as
the result is cached in a static flag. The above code is better readable.
So readability is better than micro-optimization here.
Thanks,
Kiran
On Thu, Mar 20, 2008 at 12:53:25AM -0700, Yinghai Lu wrote:
>On Thu, Mar 20, 2008 at 12:45 AM, Ravikiran G Thirumalai
><[email protected]> wrote:
>> Indicate TSCs are unreliable as time sources if the platform is
>> a multi chassi ScaleMP vSMPowered machine.
>>
>> Signed-off-by: Ravikiran Thirumalai <[email protected]>
>>
>> Index: linux.git.trees/arch/x86/kernel/apic_64.c
>> ===================================================================
>> --- linux.git.trees.orig/arch/x86/kernel/apic_64.c 2008-03-19 20:42:28.187659497 -0700
>> +++ linux.git.trees/arch/x86/kernel/apic_64.c 2008-03-19 21:24:17.705515003 -0700
>> @@ -1208,7 +1208,7 @@ __cpuinit int apic_is_clustered_box(void
>> * will be [4, 0x23] or [8, 0x27] could be thought to
>> * vsmp box still need checking...
>> */
>> - if (!is_vsmp_box() && (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
>> + if ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) && !is_vsmp_box())
>> return 0;
>
>why?
>
>>
>> bios_cpu_apicid = x86_bios_cpu_apicid_early_ptr;
>> @@ -1248,6 +1248,12 @@ __cpuinit int apic_is_clustered_box(void
>> ++zeros;
>> }
>>
>> + /* ScaleMP vSMPowered boxes have one cluster per board and TSCs are
>> + * not guaranteed to be synced between boards
>> + */
>> + if (is_vsmp_box() && clusters > 1)
>> + return 1;
>
>you still call that for intel box.
Yes. The first call is to state that TSC's are synced if it is an AMD box
and if it is _not_ a vSMPowered box (for the apic id lifting case on Sun
boxes), the second call is for _any_ vSMPowered system with more than one
board -- TSC's are not guaranteed to be synched in that case. Both these
calls are needed.
Thanks,
Kiran
Ravikiran G Thirumalai wrote:
> - Fix the the build breakage when PARAVIRT is defined
> but PCI is not
> This fixes problem reported at:
> http://marc.info/?l=linux-kernel&m=120525966600698&w=2
> - Make is_vsmp_box() available even when PARAVIRT is not defined.
> This is needed to determine if tsc's are reliable as a time source
> even when PARAVIRT is not defined.
> - split vsmp_init to use is_vsmp_box() and set_vsmp_pv_ops()
> set_vsmp_pv_ops will do nothing if PCI is not enabled in the config.
>
I'm a bit confused by all the config dependencies. I had assumed that
VSMP depended on both PCI and PARAVIRT. Is this not actually true? You
can have a VSMP system without either or both of PCI/PARAVIRT?
The structure of the code suggests that at the very least VSMP depends
on PCI (it never makes sense to enable VSMP on a non-PCI system), and
therefore a number of your config decisions can just be predicated on VSMP.
> Signed-off-by: Ravikiran Thirumalai <[email protected]>
>
> Index: linux.git.trees/arch/x86/kernel/Makefile
> ===================================================================
> --- linux.git.trees.orig/arch/x86/kernel/Makefile 2008-03-19 13:39:29.000000000 -0700
> +++ linux.git.trees/arch/x86/kernel/Makefile 2008-03-19 13:46:14.400935607 -0700
> @@ -60,7 +60,7 @@ obj-$(CONFIG_KEXEC) += relocate_kernel_
> obj-$(CONFIG_CRASH_DUMP) += crash_dump_$(BITS).o
> obj-$(CONFIG_X86_NUMAQ) += numaq_32.o
> obj-$(CONFIG_X86_SUMMIT_NUMA) += summit_32.o
> -obj-$(CONFIG_PARAVIRT) += vsmp_64.o
> +obj-y += vsmp_64.o
>
Couldn't this be make obj-$(CONFIG_X86_VSMP)?
> obj-$(CONFIG_KPROBES) += kprobes.o
> obj-$(CONFIG_MODULES) += module_$(BITS).o
> obj-$(CONFIG_ACPI_SRAT) += srat_32.o
> Index: linux.git.trees/arch/x86/kernel/setup_64.c
> ===================================================================
> --- linux.git.trees.orig/arch/x86/kernel/setup_64.c 2008-03-19 13:39:29.000000000 -0700
> +++ linux.git.trees/arch/x86/kernel/setup_64.c 2008-03-19 13:57:43.951337318 -0700
> @@ -353,9 +353,7 @@ void __init setup_arch(char **cmdline_p)
> if (efi_enabled)
> efi_init();
>
> -#ifdef CONFIG_PARAVIRT
> vsmp_init();
> -#endif
>
> dmi_scan_machine();
>
> Index: linux.git.trees/arch/x86/kernel/vsmp_64.c
> ===================================================================
> --- linux.git.trees.orig/arch/x86/kernel/vsmp_64.c 2008-03-19 13:39:50.000000000 -0700
> +++ linux.git.trees/arch/x86/kernel/vsmp_64.c 2008-03-19 20:53:34.267707163 -0700
> @@ -19,6 +19,7 @@
> #include <asm/io.h>
> #include <asm/paravirt.h>
>
> +#if defined CONFIG_PCI && defined CONFIG_PARAVIRT
> /*
> * Interrupt control on vSMPowered systems:
> * ~AC is a shadow of IF. If IF is 'on' AC should be 'off'
> @@ -72,39 +73,11 @@ static unsigned __init vsmp_patch(u8 typ
>
> }
>
> -static int vsmp = -1;
> -
> -int is_vsmp_box(void)
> -{
> - if (vsmp != -1)
> - return vsmp;
> -
> - vsmp = 0;
> - if (!early_pci_allowed())
> - return vsmp;
> -
> - /* Check if we are running on a ScaleMP vSMP box */
> - if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
> - PCI_VENDOR_ID_SCALEMP) &&
> - (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
> - PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
> - vsmp = 1;
> -
> - return vsmp;
> -}
> -
> -void __init vsmp_init(void)
> +static void __init set_vsmp_pv_ops(void)
> {
> void *address;
> unsigned int cap, ctl, cfg;
>
> - if (!is_vsmp_box())
> - return;
> -
> - if (!early_pci_allowed())
> - return;
> -
> - /* If we are, use the distinguished irq functions */
> pv_irq_ops.irq_disable = vsmp_irq_disable;
> pv_irq_ops.irq_enable = vsmp_irq_enable;
> pv_irq_ops.save_fl = vsmp_save_fl;
> @@ -127,5 +100,46 @@ void __init vsmp_init(void)
> }
>
> early_iounmap(address, 8);
> +}
> +#else
> +static void __init set_vsmp_pv_ops(void)
> +{
> +}
> +#endif
> +
> +#ifdef CONFIG_PCI
> +static int vsmp = -1;
> +
> +int is_vsmp_box(void)
> +{
> + if (vsmp != -1)
> + return vsmp;
> +
> + vsmp = 0;
> + if (!early_pci_allowed())
> + return vsmp;
> +
> + /* Check if we are running on a ScaleMP vSMP box */
> + if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
> + PCI_VENDOR_ID_SCALEMP) &&
> + (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
> + PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
> + vsmp = 1;
> +
> + return vsmp;
> +}
> +#else
> +int is_vsmp_box(void)
> +{
> + return 0;
> +}
> +#endif
>
Rather than doing this, I'd propose putting
#ifndef CONFIG_X86_VSMP
static inline int is_vsmp_box(void)
{
return 0;
}
#endif
in a header, and then making the definition in vsmp_64.c unconditional
(or rather, the whole of vsmp_64.o conditional on CONFIG_X86_VSMP).
> +
> +void __init vsmp_init(void)
> +{
> + if (!is_vsmp_box())
> + return;
> +
> + set_vsmp_pv_ops();
> return;
> }
>
Similarly with this.
> Index: linux.git.trees/include/asm-x86/apic.h
> ===================================================================
> --- linux.git.trees.orig/include/asm-x86/apic.h 2008-03-19 13:39:29.000000000 -0700
> +++ linux.git.trees/include/asm-x86/apic.h 2008-03-19 13:57:14.550893819 -0700
> @@ -51,19 +51,16 @@ extern unsigned boot_cpu_id;
> */
> #ifdef CONFIG_PARAVIRT
> #include <asm/paravirt.h>
> -extern int is_vsmp_box(void);
> #else
> #define apic_write native_apic_write
> #define apic_write_atomic native_apic_write_atomic
> #define apic_read native_apic_read
> #define setup_boot_clock setup_boot_APIC_clock
> #define setup_secondary_clock setup_secondary_APIC_clock
> -static int inline is_vsmp_box(void)
> -{
> - return 0;
> -}
> #endif
>
> +extern int is_vsmp_box(void);
> +
>
This was almost right, except it should have been in a #ifdef
CONFIG_X86_VSMP block.
J
On Thu, Mar 20, 2008 at 9:30 PM, Jeremy Fitzhardinge <[email protected]> wrote:
> Ravikiran G Thirumalai wrote:
> > - Fix the the build breakage when PARAVIRT is defined
> > but PCI is not
> > This fixes problem reported at:
> > http://marc.info/?l=linux-kernel&m=120525966600698&w=2
> > - Make is_vsmp_box() available even when PARAVIRT is not defined.
> > This is needed to determine if tsc's are reliable as a time source
> > even when PARAVIRT is not defined.
> > - split vsmp_init to use is_vsmp_box() and set_vsmp_pv_ops()
> > set_vsmp_pv_ops will do nothing if PCI is not enabled in the config.
> >
>
> I'm a bit confused by all the config dependencies. I had assumed that
> VSMP depended on both PCI and PARAVIRT. Is this not actually true? You
> can have a VSMP system without either or both of PCI/PARAVIRT?
>
> The structure of the code suggests that at the very least VSMP depends
> on PCI (it never makes sense to enable VSMP on a non-PCI system), and
> therefore a number of your config decisions can just be predicated on VSMP.
>
>
> > Signed-off-by: Ravikiran Thirumalai <[email protected]>
> >
> > Index: linux.git.trees/arch/x86/kernel/Makefile
> > ===================================================================
> > --- linux.git.trees.orig/arch/x86/kernel/Makefile 2008-03-19 13:39:29.000000000 -0700
> > +++ linux.git.trees/arch/x86/kernel/Makefile 2008-03-19 13:46:14.400935607 -0700
> > @@ -60,7 +60,7 @@ obj-$(CONFIG_KEXEC) += relocate_kernel_
> > obj-$(CONFIG_CRASH_DUMP) += crash_dump_$(BITS).o
> > obj-$(CONFIG_X86_NUMAQ) += numaq_32.o
> > obj-$(CONFIG_X86_SUMMIT_NUMA) += summit_32.o
> > -obj-$(CONFIG_PARAVIRT) += vsmp_64.o
> > +obj-y += vsmp_64.o
> >
>
> Couldn't this be make obj-$(CONFIG_X86_VSMP)?
>
>
>
> > obj-$(CONFIG_KPROBES) += kprobes.o
> > obj-$(CONFIG_MODULES) += module_$(BITS).o
> > obj-$(CONFIG_ACPI_SRAT) += srat_32.o
> > Index: linux.git.trees/arch/x86/kernel/setup_64.c
> > ===================================================================
> > --- linux.git.trees.orig/arch/x86/kernel/setup_64.c 2008-03-19 13:39:29.000000000 -0700
> > +++ linux.git.trees/arch/x86/kernel/setup_64.c 2008-03-19 13:57:43.951337318 -0700
> > @@ -353,9 +353,7 @@ void __init setup_arch(char **cmdline_p)
> > if (efi_enabled)
> > efi_init();
> >
> > -#ifdef CONFIG_PARAVIRT
> > vsmp_init();
> > -#endif
> >
> > dmi_scan_machine();
> >
> > Index: linux.git.trees/arch/x86/kernel/vsmp_64.c
> > ===================================================================
> > --- linux.git.trees.orig/arch/x86/kernel/vsmp_64.c 2008-03-19 13:39:50.000000000 -0700
> > +++ linux.git.trees/arch/x86/kernel/vsmp_64.c 2008-03-19 20:53:34.267707163 -0700
> > @@ -19,6 +19,7 @@
> > #include <asm/io.h>
> > #include <asm/paravirt.h>
> >
> > +#if defined CONFIG_PCI && defined CONFIG_PARAVIRT
> > /*
> > * Interrupt control on vSMPowered systems:
> > * ~AC is a shadow of IF. If IF is 'on' AC should be 'off'
> > @@ -72,39 +73,11 @@ static unsigned __init vsmp_patch(u8 typ
> >
> > }
> >
> > -static int vsmp = -1;
> > -
> > -int is_vsmp_box(void)
> > -{
> > - if (vsmp != -1)
> > - return vsmp;
> > -
> > - vsmp = 0;
> > - if (!early_pci_allowed())
> > - return vsmp;
> > -
> > - /* Check if we are running on a ScaleMP vSMP box */
> > - if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
> > - PCI_VENDOR_ID_SCALEMP) &&
> > - (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
> > - PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
> > - vsmp = 1;
> > -
> > - return vsmp;
> > -}
> > -
> > -void __init vsmp_init(void)
> > +static void __init set_vsmp_pv_ops(void)
> > {
> > void *address;
> > unsigned int cap, ctl, cfg;
> >
> > - if (!is_vsmp_box())
> > - return;
> > -
> > - if (!early_pci_allowed())
> > - return;
> > -
> > - /* If we are, use the distinguished irq functions */
> > pv_irq_ops.irq_disable = vsmp_irq_disable;
> > pv_irq_ops.irq_enable = vsmp_irq_enable;
> > pv_irq_ops.save_fl = vsmp_save_fl;
> > @@ -127,5 +100,46 @@ void __init vsmp_init(void)
> > }
> >
> > early_iounmap(address, 8);
> > +}
> > +#else
> > +static void __init set_vsmp_pv_ops(void)
> > +{
> > +}
> > +#endif
> > +
> > +#ifdef CONFIG_PCI
> > +static int vsmp = -1;
> > +
> > +int is_vsmp_box(void)
> > +{
> > + if (vsmp != -1)
> > + return vsmp;
> > +
> > + vsmp = 0;
> > + if (!early_pci_allowed())
> > + return vsmp;
> > +
> > + /* Check if we are running on a ScaleMP vSMP box */
> > + if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
> > + PCI_VENDOR_ID_SCALEMP) &&
> > + (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
> > + PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
> > + vsmp = 1;
> > +
> > + return vsmp;
> > +}
> > +#else
> > +int is_vsmp_box(void)
> > +{
> > + return 0;
> > +}
> > +#endif
> >
>
> Rather than doing this, I'd propose putting
>
> #ifndef CONFIG_X86_VSMP
> static inline int is_vsmp_box(void)
> {
> return 0;
> }
> #endif
>
> in a header, and then making the definition in vsmp_64.c unconditional
> (or rather, the whole of vsmp_64.o conditional on CONFIG_X86_VSMP).
>
>
> > +
> > +void __init vsmp_init(void)
> > +{
> > + if (!is_vsmp_box())
> > + return;
> > +
> > + set_vsmp_pv_ops();
> > return;
> > }
> >
>
> Similarly with this.
>
>
> > Index: linux.git.trees/include/asm-x86/apic.h
> > ===================================================================
> > --- linux.git.trees.orig/include/asm-x86/apic.h 2008-03-19 13:39:29.000000000 -0700
> > +++ linux.git.trees/include/asm-x86/apic.h 2008-03-19 13:57:14.550893819 -0700
> > @@ -51,19 +51,16 @@ extern unsigned boot_cpu_id;
> > */
> > #ifdef CONFIG_PARAVIRT
> > #include <asm/paravirt.h>
> > -extern int is_vsmp_box(void);
> > #else
> > #define apic_write native_apic_write
> > #define apic_write_atomic native_apic_write_atomic
> > #define apic_read native_apic_read
> > #define setup_boot_clock setup_boot_APIC_clock
> > #define setup_secondary_clock setup_secondary_APIC_clock
> > -static int inline is_vsmp_box(void)
> > -{
> > - return 0;
> > -}
> > #endif
> >
> > +extern int is_vsmp_box(void);
> > +
> >
>
> This was almost right, except it should have been in a #ifdef
> CONFIG_X86_VSMP block.
>
>
agreed
http://lkml.org/lkml/2008/3/18/76
YH
* Ravikiran G Thirumalai <[email protected]> wrote:
> Hi Ingo, Andrew,
> Here are a few updates to the vSMP architecture bits in the x86 tree.
> This is a set of 4 patches which do the following:
>
> 1. Fix is_vsmp_box() to actually detect if the system is a vSMPowered box
> 2. Fix build issue when CONFIG_PCI is enabled, but CONFIG_PARAVIRT is not
> 3. Set paravirt ops only if the platform has capability to recognize
> paravirtualized irq operations
> 4. Update apic_is_clustered_box() to indicate unsynched TSCs on multiboard
> vSMPowered systems
>
> The patches have been tested against 2.6.25-rc5-mm1 on vSMPowered and
> non vSMPowered x86 boxes.
thanks Ravikiran, applied them (with one fix).
Ingo
* Ravikiran G Thirumalai <[email protected]> wrote:
> >> /* Check if we are running on a ScaleMP vSMP box */
> >> - if (read_pci_config(0, 0x1f, 0, PCI_VENDOR_ID) ==
> >> - (PCI_VENDOR_ID_SCALEMP || (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16)))
> >> + if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
> >> + PCI_VENDOR_ID_SCALEMP) &&
> >> + (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
> >> + PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
> >> vsmp = 1;
> >>
> >> return vsmp;
> >
> >why read two times
> >
>
> Well, the pci cfg space read happens just _once_ during the boot, as
> the result is cached in a static flag. The above code is better
> readable. So readability is better than micro-optimization here.
i think the patch below results in even more readable and a bit smaller
code - because it's such a simple check?
OTOH, you are right in general, for example in mmconf-fam10h_64.c's
get_fam10h_pci_mmconf_base() function, we do this:
id = read_pci_config(bus, slot, 0, PCI_VENDOR_ID);
vendor = id & 0xffff;
device = (id>>16) & 0xffff;
if (pci_probes[i].vendor == vendor &&
pci_probes[i].device == device) {
here it would indeed be cleaner to simply do:
vendor = read_pci_config_16(bus, slot, 0, PCI_VENDOR_ID);
device = read_pci_config_16(bus, slot, 0, PCI_DEVICE_ID);
instead of open-coding the 16-bit splitting.
Ingo
------------>
Subject: x86: vsmp fix x86 vsmp fix is vsmp box cleanup
From: Ingo Molnar <[email protected]>
Date: Fri Mar 21 09:55:06 CET 2008
code got a bit smaller:
arch/x86/kernel/vsmp_64.o:
text data bss dec hex filename
205 4 0 209 d1 vsmp_64.o.before
181 4 0 185 b9 vsmp_64.o.after
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/vsmp_64.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
Index: linux-x86.q/arch/x86/kernel/vsmp_64.c
===================================================================
--- linux-x86.q.orig/arch/x86/kernel/vsmp_64.c
+++ linux-x86.q/arch/x86/kernel/vsmp_64.c
@@ -120,10 +120,8 @@ int is_vsmp_box(void)
return vsmp;
/* Check if we are running on a ScaleMP vSMP box */
- if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
- PCI_VENDOR_ID_SCALEMP) &&
- (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
- PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
+ if (read_pci_config(0, 0x1f, 0, PCI_VENDOR_ID) ==
+ (PCI_VENDOR_ID_SCALEMP | (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16)))
vsmp = 1;
return vsmp;
* Ravikiran G Thirumalai <[email protected]> wrote:
> >> - if (!is_vsmp_box() && (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
> >> + if ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) && !is_vsmp_box())
> >> return 0;
> Yes. The first call is to state that TSC's are synced if it is an AMD
> box and if it is _not_ a vSMPowered box (for the apic id lifting case
> on Sun boxes), the second call is for _any_ vSMPowered system with
> more than one board -- TSC's are not guaranteed to be synched in that
> case. Both these calls are needed.
i suspect there are two questions here: firstly, your change only flips
the condition around which should not have any effect _normally_. But
the thing is that is_vsmp_box() has side-effects: it reads the PCI
config space and sets a flag. It would be cleaner if there was a
separate, explicit detect_vsmp_box() call early during bootup which did
the PCI config space access, and is_vsmp_box() would only return the
current state of the vsmp flag. Then your above change would be
unnecessary.
Ingo
On Fri, Mar 21, 2008 at 2:11 AM, Ingo Molnar <[email protected]> wrote:
>
> * Ravikiran G Thirumalai <[email protected]> wrote:
>
>
> > >> /* Check if we are running on a ScaleMP vSMP box */
> > >> - if (read_pci_config(0, 0x1f, 0, PCI_VENDOR_ID) ==
> > >> - (PCI_VENDOR_ID_SCALEMP || (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16)))
> > >> + if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
> > >> + PCI_VENDOR_ID_SCALEMP) &&
> > >> + (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
> > >> + PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
> > >> vsmp = 1;
> > >>
> > >> return vsmp;
> > >
> > >why read two times
> > >
> >
> > Well, the pci cfg space read happens just _once_ during the boot, as
> > the result is cached in a static flag. The above code is better
> > readable. So readability is better than micro-optimization here.
>
> i think the patch below results in even more readable and a bit smaller
> code - because it's such a simple check?
>
> OTOH, you are right in general, for example in mmconf-fam10h_64.c's
> get_fam10h_pci_mmconf_base() function, we do this:
>
> id = read_pci_config(bus, slot, 0, PCI_VENDOR_ID);
>
> vendor = id & 0xffff;
> device = (id>>16) & 0xffff;
> if (pci_probes[i].vendor == vendor &&
> pci_probes[i].device == device) {
>
> here it would indeed be cleaner to simply do:
>
> vendor = read_pci_config_16(bus, slot, 0, PCI_VENDOR_ID);
> device = read_pci_config_16(bus, slot, 0, PCI_DEVICE_ID);
>
> instead of open-coding the 16-bit splitting.
>
> Ingo
>
> ------------>
> Subject: x86: vsmp fix x86 vsmp fix is vsmp box cleanup
> From: Ingo Molnar <[email protected]>
> Date: Fri Mar 21 09:55:06 CET 2008
>
> code got a bit smaller:
>
> arch/x86/kernel/vsmp_64.o:
>
> text data bss dec hex filename
> 205 4 0 209 d1 vsmp_64.o.before
> 181 4 0 185 b9 vsmp_64.o.after
good evidence.
YH
On Fri, Mar 21, 2008 at 10:11:39AM +0100, Ingo Molnar wrote:
>
>* Ravikiran G Thirumalai <[email protected]> wrote:
>
>> >> /* Check if we are running on a ScaleMP vSMP box */
>> >> - if (read_pci_config(0, 0x1f, 0, PCI_VENDOR_ID) ==
>> >> - (PCI_VENDOR_ID_SCALEMP || (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16)))
>> >> + if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
>> >> + PCI_VENDOR_ID_SCALEMP) &&
>> >> + (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
>> >> + PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
>> >> vsmp = 1;
>> >>
>> >> return vsmp;
>> >
>> >why read two times
>> >
>>
>> Well, the pci cfg space read happens just _once_ during the boot, as
>> the result is cached in a static flag. The above code is better
>> readable. So readability is better than micro-optimization here.
>
>i think the patch below results in even more readable and a bit smaller
>
>code got a bit smaller:
>
>arch/x86/kernel/vsmp_64.o:
>
> text data bss dec hex filename
> 205 4 0 209 d1 vsmp_64.o.before
> 181 4 0 185 b9 vsmp_64.o.after
Wow! good to know that avoiding one call shaved so many bytes.
Agreed, the below patch is better.
Thanks,
Kiran
>
>Signed-off-by: Ingo Molnar <[email protected]>
>---
> arch/x86/kernel/vsmp_64.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
>Index: linux-x86.q/arch/x86/kernel/vsmp_64.c
>===================================================================
>--- linux-x86.q.orig/arch/x86/kernel/vsmp_64.c
>+++ linux-x86.q/arch/x86/kernel/vsmp_64.c
>@@ -120,10 +120,8 @@ int is_vsmp_box(void)
> return vsmp;
>
> /* Check if we are running on a ScaleMP vSMP box */
>- if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
>- PCI_VENDOR_ID_SCALEMP) &&
>- (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
>- PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
>+ if (read_pci_config(0, 0x1f, 0, PCI_VENDOR_ID) ==
>+ (PCI_VENDOR_ID_SCALEMP | (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16)))
> vsmp = 1;
>
> return vsmp;
On Fri, Mar 21, 2008 at 10:15:42AM +0100, Ingo Molnar wrote:
>
>* Ravikiran G Thirumalai <[email protected]> wrote:
>
>> >> - if (!is_vsmp_box() && (boot_cpu_data.x86_vendor == X86_VENDOR_AMD))
>> >> + if ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) && !is_vsmp_box())
>> >> return 0;
>
>> Yes. The first call is to state that TSC's are synced if it is an AMD
>> box and if it is _not_ a vSMPowered box (for the apic id lifting case
>> on Sun boxes), the second call is for _any_ vSMPowered system with
>> more than one board -- TSC's are not guaranteed to be synched in that
>> case. Both these calls are needed.
>
>i suspect there are two questions here: firstly, your change only flips
>the condition around which should not have any effect _normally_. But
>the thing is that is_vsmp_box() has side-effects: it reads the PCI
>config space and sets a flag. It would be cleaner if there was a
>separate, explicit detect_vsmp_box() call early during bootup which did
>the PCI config space access, and is_vsmp_box() would only return the
>current state of the vsmp flag. Then your above change would be
>unnecessary.
The above change -- the flipping of conditions -- is not _really_ necessary.
The condition and call is needed however. The flip was done while
experimenting with the patch, since most vSMPowered machines today are mostly
intel, the call to is_vsmp_box() could be avoided here on intel boxes.
However this is only a trivial micro-optimization, as, we call
is_vsmp_box() again even for intel boxes now.
As for the observation about probing the pci space early during the bootup,
we call vsmp_init() much earlier during the bootup, which calls is_vsmp_box(),
does the pci probing and caches the result in the flag, as you suggest.
So the call in the above diff context does not access the pci config space
as is.
Thanks,
Kiran
On Thu, Mar 20, 2008 at 09:30:09PM -0700, Jeremy Fitzhardinge wrote:
>Ravikiran G Thirumalai wrote:
>>- Fix the the build breakage when PARAVIRT is defined
>> but PCI is not
>> This fixes problem reported at:
>> http://marc.info/?l=linux-kernel&m=120525966600698&w=2
>>- Make is_vsmp_box() available even when PARAVIRT is not defined.
>> This is needed to determine if tsc's are reliable as a time source
>> even when PARAVIRT is not defined.
>>- split vsmp_init to use is_vsmp_box() and set_vsmp_pv_ops()
>> set_vsmp_pv_ops will do nothing if PCI is not enabled in the config.
>>
>
>I'm a bit confused by all the config dependencies. I had assumed that
>VSMP depended on both PCI and PARAVIRT. Is this not actually true? You
>can have a VSMP system without either or both of PCI/PARAVIRT?
Well, you could have a vSMPowered system without PARAVIRT, yes. But a non
PCI vSMPowered system does not exist to my knowledge.
>
>The structure of the code suggests that at the very least VSMP depends
>on PCI (it never makes sense to enable VSMP on a non-PCI system), and
>therefore a number of your config decisions can just be predicated on VSMP.
Well, vSMPowered bits in the kernel serves two objectives:
a) Internode cacheline size
b) paravirt irq ops
A vSMPowered machine can boot without either, but both affect performance.
Both these bits are not interdependent. The paravirt ops
need the PARAVIRT infrastructure and that is all that is needed.
The internode cacheline size needs a compile time definition that's all.
CONFIG_X86_VSMP chooses both. However, there is no reason to have paravirt
irq ops depend on the Internode cacheline size. More so since pv ops
has the capability to detect the system type dynamically and using
appropriate pv ops.
Thanks,
Kiran
* Ravikiran G Thirumalai <[email protected]> wrote:
> As for the observation about probing the pci space early during the
> bootup, we call vsmp_init() much earlier during the bootup, which
> calls is_vsmp_box(), does the pci probing and caches the result in the
> flag, as you suggest. So the call in the above diff context does not
> access the pci config space as is.
ah, i see - indeed - the trick with -1 :-)
my point remains though: if you initialize VSMP in a separate function
anyway then please move this PCI config space access from is_vsmp_box()
into vsmp_init() and keep a pure flag return is_vsmp_box(). That way
there can be no question at all whether there are (or can be) any
side-effects of that function.
Ingo
Ravikiran G Thirumalai wrote:
> Well, vSMPowered bits in the kernel serves two objectives:
> a) Internode cacheline size
> b) paravirt irq ops
>
> A vSMPowered machine can boot without either, but both affect performance.
> Both these bits are not interdependent. The paravirt ops
> need the PARAVIRT infrastructure and that is all that is needed.
> The internode cacheline size needs a compile time definition that's all.
> CONFIG_X86_VSMP chooses both. However, there is no reason to have paravirt
> irq ops depend on the Internode cacheline size. More so since pv ops
> has the capability to detect the system type dynamically and using
> appropriate pv ops.
>
So are you saying that X86_VSMP is just to select the internode
cacheline size, and is independent of the pvops side of things?
If so, I think it would be worth adding a separate config variable so
that you have the following:
VSMP (depends on PCI) - master selector for all vsmp-related code;
enables vsmp detection code
VSMP && PARAVIRT - installs paravirt irq ops on a vsmp system
VSMP_CACHELINE_SIZE (depends on VSMP) - selects internode cacheline size
that way vsmp_64.o can just depend on VSMP, and at the very least
contains the query code, and the other features are independently
selectable.
J
On Fri, Mar 21, 2008 at 07:58:21PM +0100, Ingo Molnar wrote:
>
>* Ravikiran G Thirumalai <[email protected]> wrote:
>
>> As for the observation about probing the pci space early during the
>> bootup, we call vsmp_init() much earlier during the bootup, which
>> calls is_vsmp_box(), does the pci probing and caches the result in the
>> flag, as you suggest. So the call in the above diff context does not
>> access the pci config space as is.
>
>ah, i see - indeed - the trick with -1 :-)
>
>my point remains though: if you initialize VSMP in a separate function
>anyway then please move this PCI config space access from is_vsmp_box()
>into vsmp_init() and keep a pure flag return is_vsmp_box(). That way
>there can be no question at all whether there are (or can be) any
>side-effects of that function.
>
OK. Something like this?
vSMP detection: Access pci config space early in boot to detect if the
system is a vSMPowered box, and cache the result in a flag, so that
is_vsmp_box() retrieves the value of the flag always.
Signed-off-by; Ravikiran Thirumalai <[email protected]>
Index: linux.git.trees/arch/x86/kernel/vsmp_64.c
===================================================================
--- linux.git.trees.orig/arch/x86/kernel/vsmp_64.c 2008-03-21 13:15:17.000000000 -0700
+++ linux.git.trees/arch/x86/kernel/vsmp_64.c 2008-03-22 11:58:42.313580356 -0700
@@ -108,25 +108,34 @@ static void __init set_vsmp_pv_ops(void)
#endif
#ifdef CONFIG_PCI
-static int vsmp = -1;
+static int is_vsmp = -1;
-int is_vsmp_box(void)
+static void __init detect_vsmp_box(void)
{
- if (vsmp != -1)
- return vsmp;
+ is_vsmp = 0;
- vsmp = 0;
if (!early_pci_allowed())
- return vsmp;
+ return;
- /* Check if we are running on a ScaleMP vSMP box */
+ /* Check if we are running on a ScaleMP vSMPowered box */
if (read_pci_config(0, 0x1f, 0, PCI_VENDOR_ID) ==
(PCI_VENDOR_ID_SCALEMP | (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16)))
- vsmp = 1;
+ is_vsmp = 1;
+}
- return vsmp;
+int is_vsmp_box(void)
+{
+ if (is_vsmp != -1)
+ return is_vsmp;
+ else {
+ printk("ScaleMP vSMPowered system detection called too early!\n");
+ return 0;
+ }
}
#else
+static int __init detect_vsmp_box(void)
+{
+}
int is_vsmp_box(void)
{
return 0;
@@ -135,6 +144,7 @@ int is_vsmp_box(void)
void __init vsmp_init(void)
{
+ detect_vsmp_box();
if (!is_vsmp_box())
return;
* Ravikiran G Thirumalai <[email protected]> wrote:
> OK. Something like this?
yeah.
small nit:
> +int is_vsmp_box(void)
> +{
> + if (is_vsmp != -1)
> + return is_vsmp;
> + else {
> + printk("ScaleMP vSMPowered system detection called too early!\n");
> + return 0;
> + }
> }
since this is an "impossible" scenario, just stick in a WARN_ON_ONCE().
Ingo
On Sat, Mar 22, 2008 at 09:02:51PM +0100, Ingo Molnar wrote:
>
>* Ravikiran G Thirumalai <[email protected]> wrote:
>
>> OK. Something like this?
>
>yeah.
>
>small nit:
>
>> +int is_vsmp_box(void)
>> +{
>> + if (is_vsmp != -1)
>> + return is_vsmp;
>> + else {
>> + printk("ScaleMP vSMPowered system detection called too early!\n");
>> + return 0;
>> + }
>> }
>
>since this is an "impossible" scenario, just stick in a WARN_ON_ONCE().
>
Sure. Here's the modified patch.
vSMP detection: Access pci config space early in boot to detect if the
system is a vSMPowered box, and cache the result in a flag, so that
is_vsmp_box() retrieves the value of the flag always.
Signed-off-by; Ravikiran Thirumalai <[email protected]>
Index: linux.git.trees/arch/x86/kernel/vsmp_64.c
===================================================================
--- linux.git.trees.orig/arch/x86/kernel/vsmp_64.c 2008-03-21 13:15:17.000000000 -0700
+++ linux.git.trees/arch/x86/kernel/vsmp_64.c 2008-03-24 13:57:09.387437626 -0700
@@ -108,25 +108,34 @@ static void __init set_vsmp_pv_ops(void)
#endif
#ifdef CONFIG_PCI
-static int vsmp = -1;
+static int is_vsmp = -1;
-int is_vsmp_box(void)
+static void __init detect_vsmp_box(void)
{
- if (vsmp != -1)
- return vsmp;
+ is_vsmp = 0;
- vsmp = 0;
if (!early_pci_allowed())
- return vsmp;
+ return;
- /* Check if we are running on a ScaleMP vSMP box */
+ /* Check if we are running on a ScaleMP vSMPowered box */
if (read_pci_config(0, 0x1f, 0, PCI_VENDOR_ID) ==
(PCI_VENDOR_ID_SCALEMP | (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16)))
- vsmp = 1;
+ is_vsmp = 1;
+}
- return vsmp;
+int is_vsmp_box(void)
+{
+ if (is_vsmp != -1)
+ return is_vsmp;
+ else {
+ WARN_ON_ONCE(1);
+ return 0;
+ }
}
#else
+static int __init detect_vsmp_box(void)
+{
+}
int is_vsmp_box(void)
{
return 0;
@@ -135,6 +144,7 @@ int is_vsmp_box(void)
void __init vsmp_init(void)
{
+ detect_vsmp_box();
if (!is_vsmp_box())
return;
* Ravikiran G Thirumalai <[email protected]> wrote:
> Sure. Here's the modified patch.
>
> vSMP detection: Access pci config space early in boot to detect if the
> system is a vSMPowered box, and cache the result in a flag, so that
> is_vsmp_box() retrieves the value of the flag always.
thanks, applied.
Ingo