login
Search
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 systemsThe patches have been tested against 2.6.25-rc5-mm1 on vSMPowered and non
vSMPowered x86 boxes.Please apply
Thanks,
Kiran
--
thanks Ravikiran, applied them (with one fix).
Ingo
--
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
--
On Thu, Mar 20, 2008 at 12:45 AM, Ravikiran G Thirumalai
you still call that for intel box.
YH
--
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
--
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
--
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
--
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
--
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;--
yeah.
since this is an "impossible" scenario, just stick in a WARN_ON_ONCE().
Ingo
--
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;--
thanks, applied.
Ingo
--
Re-arrange set_vsmp_pv_ops so that pv_ops are set only if
the platform has capability to support paravirtualized irq opsSigned-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);
--
- 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();
-#endifdmi_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 <as...
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), andRather than doing this, I'd propose putting
#ifndef CONFIG_X86_VSMP
static inline int is_vsmp_box(void)
{
return 0;
}
#endifin a header, and then making the definition in vsmp_64.c unconditional
This was almost right, except it should have been in a #ifdef
CONFIG_X86_VSMP block.J
--
Well, you could have a vSMPowered system without PARAVIRT, yes. But a non
Well, vSMPowered bits in the kernel serves two objectives:
a) Internode cacheline size
b) paravirt irq opsA 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
--
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 sizethat 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
--
agreed
http://lkml.org/lkml/2008/3/18/76
YH
--
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;
--
On Thu, Mar 20, 2008 at 12:39 AM, Ravikiran G Thirumalai
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
--
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
--
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 2008code 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.afterSigned-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;
--
Wow! good to know that avoiding one call shaved so many bytes.
Agreed, the below patch is better.Thanks,
--
good evidence.
YH
--
