[patch 0/4] x86: vSMP updates

[patch 0/4] x86: vSMP updates

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

[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 <kiran@scalex86.org>

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;

[patch 2/4] x86: Fix build breakage when PCI is define and PARAVIRT is not

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

Signed-off-by: Ravikiran Thirumalai <kiran@scalex86.org>

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;

[patch 3/4] x86: vSMP: Use pvops only if platform has the capability to support it

[Ravikiran G Thirumalai]

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 <kiran@scalex86.org>

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

Re: [patch 1/4] x86: vSMP: Fix is_vsmp_box()

[Yinghai Lu]

On Thu, Mar 20, 2008 at 12:39 AM, Ravikiran G Thirumalai
<kiran@scalex86.org> 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 <kiran@scalex86.org>
>
>  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

[patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems

[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 <kiran@scalex86.org>

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

Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems

[Yinghai Lu]

On Thu, Mar 20, 2008 at 12:45 AM, Ravikiran G Thirumalai
<kiran@scalex86.org> wrote:
> Indicate TSCs are unreliable as time sources if the platform is
>  a multi chassi ScaleMP vSMPowered machine.
>
>  Signed-off-by: Ravikiran Thirumalai <kiran@scalex86.org>
>
>  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

Re: [patch 1/4] x86: vSMP: Fix is_vsmp_box()

[Ravikiran G Thirumalai]

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
><kiran@scalex86.org> 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 <kiran@scalex86.org>
>>
>>  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

Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems

[Ravikiran G Thirumalai]

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
><kiran@scalex86.org> wrote:
>> Indicate TSCs are unreliable as time sources if the platform is
>>  a multi chassi ScaleMP vSMPowered machine.
>>
>>  Signed-off-by: Ravikiran Thirumalai <kiran@scalex86.org>
>>
>>  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

Re: [patch 2/4] x86: Fix build breakage when PCI is define and PARAVIRT is not

[Jeremy Fitzhardinge]

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 <kiran@scalex86.org>
>
> 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

Re: [patch 2/4] x86: Fix build breakage when PCI is define and PARAVIRT is not

[Yinghai Lu]

On Thu, Mar 20, 2008 at 9:30 PM, Jeremy Fitzhardinge <jeremy@goop.org> 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 <kiran@scalex86.org>
>  >
>  > 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

Re: [patch 0/4] x86: vSMP updates

[Ingo Molnar]

* Ravikiran G Thirumalai <kiran@scalex86.org> 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

Re: [patch 1/4] x86: vSMP: Fix is_vsmp_box()

[Ingo Molnar]

* Ravikiran G Thirumalai <kiran@scalex86.org> 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 <mingo@elte.hu>
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 <mingo@elte.hu>
---
 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;

Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems

[Ingo Molnar]

* Ravikiran G Thirumalai <kiran@scalex86.org> 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

Re: [patch 1/4] x86: vSMP: Fix is_vsmp_box()

[Yinghai Lu]

On Fri, Mar 21, 2008 at 2:11 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
>  * Ravikiran G Thirumalai <kiran@scalex86.org> 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 <mingo@elte.hu>
>  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

Re: [patch 1/4] x86: vSMP: Fix is_vsmp_box()

[Ravikiran G Thirumalai]

On Fri, Mar 21, 2008 at 10:11:39AM +0100, Ingo Molnar wrote:
>
>* Ravikiran G Thirumalai <kiran@scalex86.org> 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 <mingo@elte.hu>
>---
> 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;

Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems

[Ravikiran G Thirumalai]

On Fri, Mar 21, 2008 at 10:15:42AM +0100, Ingo Molnar wrote:
>
>* Ravikiran G Thirumalai <kiran@scalex86.org> 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

Re: [patch 2/4] x86: Fix build breakage when PCI is define and PARAVIRT is not

[Ravikiran G Thirumalai]

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

Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems

[Ingo Molnar]

* Ravikiran G Thirumalai <kiran@scalex86.org> 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

Re: [patch 2/4] x86: Fix build breakage when PCI is define and PARAVIRT is not

[Jeremy Fitzhardinge]

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

Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems

[Ravikiran G Thirumalai]

On Fri, Mar 21, 2008 at 07:58:21PM +0100, Ingo Molnar wrote:
>
>* Ravikiran G Thirumalai <kiran@scalex86.org> 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 <kiran@scalex86.org>

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;
 

Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems

[Ingo Molnar]

* Ravikiran G Thirumalai <kiran@scalex86.org> 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

Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems

[Ravikiran G Thirumalai]

On Sat, Mar 22, 2008 at 09:02:51PM +0100, Ingo Molnar wrote:
>
>* Ravikiran G Thirumalai <kiran@scalex86.org> 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 <kiran@scalex86.org>

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;
 

Re: [patch 4/4] x86: apic_is_clustered_box to indicate unsynched TSC's on multiboard vSMP systems

[Ingo Molnar]

* Ravikiran G Thirumalai <kiran@scalex86.org> 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