x86: vSMP updates
cover x86: vSMP updates
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
PATCH 1/4 x86: vSMP: Fix is_vsmp_box()
is_vsmp_box() currently does not work on vSMPowered systems, as pci cfg space is not read correctly -- This patch fixes it.
arch/x86/kernel/vsmp_64.c +4 -2
@@ -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;
Re: [patch 1/4] x86: vSMP: Fix is_vsmp_box()
Quoted 10 lines · Ravikiran G Thirumalai
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.c2008-03-19 13:30:35.116766719 -0700
+++linux.git.trees/arch/x86/kernel/vsmp_64.c2008-03-19 13:39:20.074685590 -0700
@@ -84,8 +84,10 @@ intis_vsmp_box(void)
Quoted 12 lines
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
Re: [patch 1/4] x86: vSMP: Fix is_vsmp_box()
Quoted 23 lines · Yinghai Lu
Quoted 21 lines · Ravikiran G Thirumalai
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.c2008-03-19 13:30:35.116766719 -0700
+++linux.git.trees/arch/x86/kernel/vsmp_64.c2008-03-19 13:39:20.074685590 -0700
@@ -84,8 +84,10 @@ intis_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;
Quoted 4 lines
Quoted 1 line
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
Re: [patch 1/4] x86: vSMP: Fix is_vsmp_box()
Quoted 9 lines · Ravikiran G Thirumalai
Quoted 9 lines
Quoted 9 lines
/* 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;
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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--- arch/x86/kernel/vsmp_64.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
arch/x86/kernel/vsmp_64.c +2 -4
@@ -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;
Re: [patch 1/4] x86: vSMP: Fix is_vsmp_box()
Quoted 49 lines · Ingo Molnar
Quoted 9 lines · Ravikiran G Thirumalai
Quoted 9 lines
Quoted 9 lines
/* 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;Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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
Quoted 7 lines
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
Re: [patch 1/4] x86: vSMP: Fix is_vsmp_box()
In reply to Ingo MolnarQuoted 23 lines · Ingo Molnar
Quoted 9 lines · Ravikiran G Thirumalai
Quoted 9 lines
Quoted 9 lines
/* 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;Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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
Quoted 7 lines
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
Quoted 23 lines
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
===================================================================arch/x86/kernel/vsmp_64.c +2
-4@@ -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;
PATCH 2/4 x86: Fix build breakage when PCI is define and PARAVIRT is not
In reply to Ravikiran G Thirumalai- 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.
arch/x86/kernel/Makefile +1 -1
@@ -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
arch/x86/kernel/setup_64.c +0 -2
@@ -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 ();
arch/x86/kernel/vsmp_64.c +43 -29
93 lines not on this page · raw message
include/asm-x86/apic.h +2 -5
@@ -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: [patch 2/4] x86: Fix build breakage when PCI is define and PARAVIRT is not
Quoted 10 lines · Ravikiran G Thirumalai
- 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.
- splitvsmp_initto useis_vsmp_box()andset_vsmp_pv_ops()set_vsmp_pv_opswill 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.
arch/x86/kernel/Makefile obj-$(CONFIG_KEXEC) += relocate_kernel_ · line 64
Quoted 13 lines
Signed-off-by Ravikiran Thirumalai <[email protected]>Index: linux.git.trees/arch/x86/kernel/Makefile
===================================================================arch/x86/kernel/Makefile +1
-1@@ -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)?
Quoted 99 lines
arch/x86/kernel/Makefile +0
-0obj-$(CONFIG_KPROBES) += kprobes.o obj-$(CONFIG_MODULES) += module_$(BITS).oobj-$(CONFIG_ACPI_SRAT) += srat_32.o
Index:linux.git.trees/arch/x86/kernel/setup_64.c
===================================================================arch/x86/kernel/setup_64.c +0
-2@@ -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
===================================================================arch/x86/kernel/vsmp_64.c +27
-29@@ -19,6 +19,7 @@#include <asm/io.h> #include <asm/paravirt.h> +#if definedCONFIG_PCI && definedCONFIG_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 ;
arch/x86/kernel/vsmp_64.c void __init vsmp_init(void) · line 137
Quoted 10 lines
arch/x86/kernel/vsmp_64.c +9
-0+ + 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).
Quoted 10 lines
arch/x86/kernel/vsmp_64.c +7
-0+ +void __init vsmp_init (void ) +{ +if (!is_vsmp_box ()) +return ; + +set_vsmp_pv_ops ();return ;}
Similarly with this.
Quoted 14 lines
Index:
linux.git.trees/include/asm-x86/apic.h
===================================================================include/asm-x86/apic.h +0
-1@@ -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
include/asm-x86/apic.h extern unsigned boot_cpu_id; · line 64
Quoted 10 lines
include/asm-x86/apic.h +2
-4#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
Re: [patch 2/4] x86: Fix build breakage when PCI is define and PARAVIRT is not
Quoted 197 lines · Jeremy Fitzhardinge
Quoted 10 lines · Ravikiran G Thirumalai
- 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.
- splitvsmp_initto useis_vsmp_box()andset_vsmp_pv_ops()set_vsmp_pv_opswill 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.
arch/x86/kernel/Makefile 2008-03-19 13:46:14.400935607 -0700 obj-$(CONFIG_KEXEC) += relocate_kernel_ · line 64
Quoted 13 lines
Signed-off-by Ravikiran Thirumalai <[email protected]>Index: linux.git.trees/arch/x86/kernel/Makefile
===================================================================arch/x86/kernel/Makefile 2008-03-19 13:46:14.400935607 -0700 renamed from arch/x86/kernel/Makefile 2008-03-19 13:39:29.000000000 -0700 +1
-1@@ -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.oCouldn't this be make obj-$(CONFIG_X86_VSMP)?
Quoted 99 lines
arch/x86/kernel/Makefile 2008-03-19 13:46:14.400935607 -0700 renamed from arch/x86/kernel/Makefile 2008-03-19 13:39:29.000000000 -0700 +0
-0obj-$(CONFIG_KPROBES) += kprobes.o obj-$(CONFIG_MODULES) += module_$(BITS).oobj-$(CONFIG_ACPI_SRAT) += srat_32.o
Index:linux.git.trees/arch/x86/kernel/setup_64.c
===================================================================arch/x86/kernel/setup_64.c 2008-03-19 13:57:43.951337318 -0700 renamed from arch/x86/kernel/setup_64.c 2008-03-19 13:39:29.000000000 -0700 +0
-2@@ -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
===================================================================arch/x86/kernel/vsmp_64.c 2008-03-19 20:53:34.267707163 -0700 renamed from arch/x86/kernel/vsmp_64.c 2008-03-19 13:39:50.000000000 -0700 +27
-29@@ -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;arch/x86/kernel/vsmp_64.c 2008-03-19 20:53:34.267707163 -0700 void __init vsmp_init(void) · line 137
Quoted 10 lines
arch/x86/kernel/vsmp_64.c 2008-03-19 20:53:34.267707163 -0700 renamed from arch/x86/kernel/vsmp_64.c 2008-03-19 13:39:50.000000000 -0700 +9
-0+ + return vsmp; +} +#else +int is_vsmp_box(void) +{ + return 0; +} +#endifRather than doing this, I'd propose putting
#ifndef
CONFIG_X86_VSMP
static inline intis_vsmp_box(void){ return 0;}
#endifin a header, and then making the definition in
vsmp_64.cunconditional (or rather, the whole of vsmp_64.o conditional onCONFIG_X86_VSMP).Quoted 10 lines
arch/x86/kernel/vsmp_64.c 2008-03-19 20:53:34.267707163 -0700 renamed from arch/x86/kernel/vsmp_64.c 2008-03-19 13:39:50.000000000 -0700 +7
-0+ +void __init vsmp_init(void) +{ + if (!is_vsmp_box()) + return; + + set_vsmp_pv_ops(); return;}
Similarly with this.
Quoted 21 lines
Index:
linux.git.trees/include/asm-x86/apic.h
===================================================================include/asm-x86/apic.h 2008-03-19 13:57:14.550893819 -0700 renamed from include/asm-x86/apic.h 2008-03-19 13:39:29.000000000 -0700 +0
-5@@ -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
Quoted 8 lines
include/asm-x86/apic.h 2008-03-19 13:57:14.550893819 -0700 extern unsigned boot_cpu_id; · line 64
Quoted 3 lines
include/asm-x86/apic.h 2008-03-19 13:57:14.550893819 -0700 renamed from include/asm-x86/apic.h 2008-03-19 13:39:29.000000000 -0700 +2
-0+extern int is_vsmp_box(void); +This was almost right, except it should have been in a #ifdef
CONFIG_X86_VSMPblock.
agreed
http://lkml.org/lkml/2008/3/18/76
YH
Re: [patch 2/4] x86: Fix build breakage when PCI is define and PARAVIRT is not
In reply to Jeremy FitzhardingeQuoted 12 lines · Jeremy Fitzhardinge
Quoted 10 lines · Ravikiran G Thirumalai
- 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.
- splitvsmp_initto useis_vsmp_box()andset_vsmp_pv_ops()set_vsmp_pv_opswill do nothing if PCI is not enabled in the config.
Quoted 3 lines
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.
Quoted 4 lines
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
Re: [patch 2/4] x86: Fix build breakage when PCI is define and PARAVIRT is not
Quoted 4 lines · Ravikiran G Thirumalai
Well, vSMPowered bits in the kernel serves two objectives:
a) Internode cacheline size
b) paravirt irq ops
Quoted 9 lines
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_VSMPchooses 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
PATCH 3/4 x86: vSMP: Use pvops only if platform has the capability to support it
In reply to Ravikiran G ThirumalaiRe-arrange set_vsmp_pv_ops so that pv_ops are set only if the platform has capability to support paravirtualized irq ops
arch/x86/kernel/vsmp_64.c +7 -7
@@ -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 );
PATCH 4/4 x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems
In reply to Ravikiran G ThirumalaiIndicate TSCs are unreliable as time sources if the platform is a multi chassi ScaleMP vSMPowered machine.
arch/x86/kernel/apic_64.c +7 -1
@@ -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
Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems
Quoted 15 lines · Ravikiran G Thirumalai
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.c2008-03-19 20:42:28.187659497 -0700
+++linux.git.trees/arch/x86/kernel/apic_64.c2008-03-19 21:24:17.705515003 -0700
@@ -1208,7 +1208,7 @@__cpuinitint 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())
Quoted 1 line
return 0;
why?
Quoted 11 lines
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
Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems
Quoted 27 lines · Yinghai Lu
Quoted 15 lines · Ravikiran G Thirumalai
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.c2008-03-19 20:42:28.187659497 -0700
+++linux.git.trees/arch/x86/kernel/apic_64.c2008-03-19 21:24:17.705515003 -0700
@@ -1208,7 +1208,7 @@__cpuinitint 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())Quoted 1 line
return 0;why?
Quoted 6 lines
bios_cpu_apicid = x86_bios_cpu_apicid_early_ptr; @@ -1248,6 +1248,12 @@ __cpuinit int apic_is_clustered_box(void ++zeros; }
Quoted 7 lines
Quoted 5 lines
+ /* 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
Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems
Quoted 9 lines · Ravikiran G Thirumalai
Quoted 3 lines
Quoted 3 lines
- 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
Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems
Quoted 13 lines · Ingo Molnar
Quoted 9 lines · Ravikiran G Thirumalai
Quoted 3 lines
Quoted 3 lines
- 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.
Quoted 8 lines
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, explicitdetect_vsmp_box()call early during bootup which did the PCI config space access, andis_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
Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems
Quoted 5 lines · Ravikiran G Thirumalai
As for the observation about probing the pci space early during the bootup, we call
vsmp_init()much earlier during the bootup, which callsis_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
Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems
Quoted 9 lines · Ingo Molnar
Quoted 5 lines · Ravikiran G Thirumalai
As for the observation about probing the pci space early during the bootup, we call
vsmp_init()much earlier during the bootup, which callsis_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.
Quoted 8 lines
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()intovsmp_init()and keep a pure flag returnis_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]>
arch/x86/kernel/vsmp_64.c +19 -9
@@ -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 ;
Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems
Quoted 1 line · Ravikiran G Thirumalai
OK. Something like this?
yeah.
small nit:
arch/x86/kernel/vsmp_64.c static void __init set_vsmp_pv_ops(void) · line 134
Quoted 9 lines
arch/x86/kernel/vsmp_64.c +8
-0+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
Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems
Quoted 9 lines · Ingo Molnar
Quoted 1 line · Ravikiran G Thirumalai
OK. Something like this?
yeah.
small nit:
Quoted 12 lines
Review
Quoted 9 lines
(quoted diff) +8
-0+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]>
arch/x86/kernel/vsmp_64.c +19 -9
@@ -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 ;
Ingo Molnar
AppliedRe: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems
Quoted 5 lines · Ravikiran G Thirumalai
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
Quoted 11 lines · Ravikiran G Thirumalai
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 whenCONFIG_PCIis enabled, butCONFIG_PARAVIRTis not
3. Set paravirt ops only if the platform has capability to recognize paravirtualized irq operations
4. Updateapic_is_clustered_box()to indicate unsynched TSCs on multiboard vSMPowered systems
Quoted 2 lines
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