I'm attaching patches for seabios to make it work on the Virtutech Simics x86-440bx model. Please let me know if there is some other list that is preferred for seabios patches.
Patches 1-6 and 9 are not really related to the Virtutech model at all, so those would be prime candidates to be included in the mainline version.
M.
On Tue, Nov 03, 2009 at 04:31:32PM +0100, Magnus Christensson wrote:
I'm attaching patches for seabios to make it work on the Virtutech Simics x86-440bx model. Please let me know if there is some other list that is preferred for seabios patches.
Patches 1-6 and 9 are not really related to the Virtutech model at all, so those would be prime candidates to be included in the mainline version.
Thanks Magnus.
Gleb I'm not sure if you're on the coreboot mailing list - can you take a look at these patches as well? A URL is at:
http://permalink.gmane.org/gmane.linux.bios/55487
-Kevin
On Tue, Nov 03, 2009 at 10:30:12PM -0500, Kevin O'Connor wrote:
On Tue, Nov 03, 2009 at 04:31:32PM +0100, Magnus Christensson wrote:
I'm attaching patches for seabios to make it work on the Virtutech Simics x86-440bx model. Please let me know if there is some other list that is preferred for seabios patches.
Patches 1-6 and 9 are not really related to the Virtutech model at all, so those would be prime candidates to be included in the mainline version.
l> Thanks Magnus.
Gleb I'm not sure if you're on the coreboot mailing list - can you take a look at these patches as well? A URL is at:
Can I get them in a mbox format somewhere? Want to tested them to be sure. From review: 1: OK
2: What is the reason for this? x86info --mptable on my 4 core AMD shows MP Table: # APIC ID Version State Family Model Step Flags # 0 0x10 BSP, usable 16 4 1 0x178bfbff # 1 0x10 AP, usable 16 4 1 0x178bfbff # 2 0x10 AP, usable 16 4 1 0x178bfbff # 3 0x10 AP, usable 16 4 1 0x178bfbff
3: And why is this? My mptable spec (from 1997) says that apic id is 8 bit and x86info --mptable on my Intel with 16 logical cpus shows: MP Table: # APIC ID Version State Family Model Step Flags # 0 0x15 BSP, usable 6 10 5 0x0381 # 16 0x15 AP, usable 6 10 5 0x0381 Interesting that on Intel only one entry per physical package.
4: I am not sure why we want to make it faster (waiting anyway) but OK.
5: OK. Wanted to add this by myself for a long time. We configure LINT0 inside KVM now, but this is BIOS job.
6: OK.
7: Don't like VIRTUTECH_IRQ0_OVERRIDE checking inside qemu_cfg_irq0_override(). What about creating irq0_override() in generic code that will call qemu_cfg_irq0_override() if needed. Setting VIRTUTECH_IRQ0_OVERRIDE to 1 by default is not a welcomed too :)
8: Do not set VIRTUTECH_PC_SHADOW to 1 by default please. Otherwise the code is not used by qemu so OK.
9: OK
10: USE_CMOS_BIOS_SMP_COUNT default to 1
-- Gleb.
On 11/04/2009 03:40 PM, Gleb Natapov wrote:
On Tue, Nov 03, 2009 at 10:30:12PM -0500, Kevin O'Connor wrote:
On Tue, Nov 03, 2009 at 04:31:32PM +0100, Magnus Christensson wrote:
I'm attaching patches for seabios to make it work on the Virtutech Simics x86-440bx model. Please let me know if there is some other list that is preferred for seabios patches.
Patches 1-6 and 9 are not really related to the Virtutech model at all, so those would be prime candidates to be included in the mainline version.
l> Thanks Magnus.
Gleb I'm not sure if you're on the coreboot mailing list - can you take a look at these patches as well? A URL is at:
Can I get them in a mbox format somewhere? Want to tested them to be sure. From review: 1: OK
2: What is the reason for this? x86info --mptable on my 4 core AMD shows MP Table: # APIC ID Version State Family Model Step Flags # 0 0x10 BSP, usable 16 4 1 0x178bfbff # 1 0x10 AP, usable 16 4 1 0x178bfbff # 2 0x10 AP, usable 16 4 1 0x178bfbff # 3 0x10 AP, usable 16 4 1 0x178bfbff
I can think of two reasons for only listing one CPU per package. The first would be performance, in that the OS needs to be aware of multi-threading in order not to slow down high priority tasks with stuff that is usually free when running on multiple processors (like spinlocks). The second reason would be that logical CPUs in the same package share some MSRs, and an OS that isn't aware of that may cease to work in such an environment.
Related link:
http://software.intel.com/en-us/articles/hyper-threading-implications-and-se...
3: And why is this? My mptable spec (from 1997) says that apic id is 8 bit and x86info --mptable on my Intel with 16 logical cpus shows: MP Table: # APIC ID Version State Family Model Step Flags # 0 0x15 BSP, usable 6 10 5 0x0381 # 16 0x15 AP, usable 6 10 5 0x0381 Interesting that on Intel only one entry per physical package.
I don't really remember the reason for that limit. I simply added stuff from our BIOS to SeaBIOS until they generated identical MPS tables. Feel free to ignore this patch.
4: I am not sure why we want to make it faster (waiting anyway) but OK.
Virtutech Simics is often run with scripted input, and time is then compressed whenever possible to boost overall performance.
5: OK. Wanted to add this by myself for a long time. We configure LINT0 inside KVM now, but this is BIOS job.
6: OK.
7: Don't like VIRTUTECH_IRQ0_OVERRIDE checking inside qemu_cfg_irq0_override(). What about creating irq0_override() in generic code that will call qemu_cfg_irq0_override() if needed. Setting VIRTUTECH_IRQ0_OVERRIDE to 1 by default is not a welcomed too :)
I'll add a QEMU compatible paravirt device for Simics, and then we can live without this patch.
8: Do not set VIRTUTECH_PC_SHADOW to 1 by default please. Otherwise the code is not used by qemu so OK.
9: OK
10: USE_CMOS_BIOS_SMP_COUNT default to 1
I'm attaching the (modified) patches.
M.
On Thu, Nov 05, 2009 at 10:30:49AM +0100, Magnus Christensson wrote:
On 11/04/2009 03:40 PM, Gleb Natapov wrote:
On Tue, Nov 03, 2009 at 10:30:12PM -0500, Kevin O'Connor wrote:
On Tue, Nov 03, 2009 at 04:31:32PM +0100, Magnus Christensson wrote:
I'm attaching patches for seabios to make it work on the Virtutech Simics x86-440bx model. Please let me know if there is some other list that is preferred for seabios patches.
Patches 1-6 and 9 are not really related to the Virtutech model at all, so those would be prime candidates to be included in the mainline version.
l> Thanks Magnus.
Gleb I'm not sure if you're on the coreboot mailing list - can you take a look at these patches as well? A URL is at:
Can I get them in a mbox format somewhere? Want to tested them to be sure. From review: 1: OK
2: What is the reason for this? x86info --mptable on my 4 core AMD shows MP Table: # APIC ID Version State Family Model Step Flags # 0 0x10 BSP, usable 16 4 1 0x178bfbff # 1 0x10 AP, usable 16 4 1 0x178bfbff # 2 0x10 AP, usable 16 4 1 0x178bfbff # 3 0x10 AP, usable 16 4 1 0x178bfbff
I can think of two reasons for only listing one CPU per package. The first would be performance, in that the OS needs to be aware of multi-threading in order not to slow down high priority tasks with stuff that is usually free when running on multiple processors (like spinlocks). The second reason would be that logical CPUs in the same package share some MSRs, and an OS that isn't aware of that may cease to work in such an environment.
Related link:
http://software.intel.com/en-us/articles/hyper-threading-implications-and-se...
This list talks about HT. Why not add entry for each core though? I am not aware of any MSRs shared between cores
From a41c206097e801de29668f99ae9b9342614c716d Mon Sep 17 00:00:00 2001
From: Magnus Christensson mch@virtutech.com Date: Tue, 3 Nov 2009 12:50:09 +0100 Subject: [PATCH 2/8] Only add the first logical CPU in each physical CPU to the MPS tables.
src/mptable.c | 26 ++++++++++++++++++++++---- 1 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/mptable.c b/src/mptable.c index 805fe1b..525188d 100644 --- a/src/mptable.c +++ b/src/mptable.c @@ -44,16 +44,32 @@ mptable_init(void) config->spec = 4; memcpy(config->oemid, CONFIG_CPUNAME8, sizeof(config->oemid)); memcpy(config->productid, "0.1 ", sizeof(config->productid));
config->entrycount = MaxCountCPUs + 2 + 16; config->lapic = BUILD_APIC_ADDR;
// CPU definitions. u32 cpuid_signature, ebx, ecx, cpuid_features; cpuid(1, &cpuid_signature, &ebx, &ecx, &cpuid_features); struct mpt_cpu *cpus = (void*)&config[1];
int i;
for (i = 0; i < MaxCountCPUs; i++) {
- int i, actual_cpu_count;
- for (i = 0, actual_cpu_count = 0; i < MaxCountCPUs; i++) { struct mpt_cpu *cpu = &cpus[i];
int log_cpus = (ebx >> 16) & 0xff;/* Only populate the MPS tables with the first logical CPU in eachpackage */if ((cpuid_features & (1 << 28)) &&log_cpus > 1 &&((log_cpus <= 2 && (i & 1) != 0) ||(log_cpus <= 4 && (i & 3) != 0) ||(log_cpus <= 8 && (i & 7) != 0) ||(log_cpus <= 16 && (i & 15) != 0) ||(log_cpus <= 32 && (i & 31) != 0) ||(log_cpus <= 64 && (i & 63) != 0) ||(log_cpus <= 128 && (i & 127) != 0)))continue;
Isn't this the same as (i & (log_cpus - 1) != 0)?
-- Gleb.
On 11/05/2009 10:54 AM, Gleb Natapov wrote:
On Thu, Nov 05, 2009 at 10:30:49AM +0100, Magnus Christensson wrote:
On 11/04/2009 03:40 PM, Gleb Natapov wrote:
On Tue, Nov 03, 2009 at 10:30:12PM -0500, Kevin O'Connor wrote:
On Tue, Nov 03, 2009 at 04:31:32PM +0100, Magnus Christensson wrote:
I'm attaching patches for seabios to make it work on the Virtutech Simics x86-440bx model. Please let me know if there is some other list that is preferred for seabios patches.
Patches 1-6 and 9 are not really related to the Virtutech model at all, so those would be prime candidates to be included in the mainline version.
l> Thanks Magnus.
Gleb I'm not sure if you're on the coreboot mailing list - can you take a look at these patches as well? A URL is at:
Can I get them in a mbox format somewhere? Want to tested them to be sure. From review: 1: OK
2: What is the reason for this? x86info --mptable on my 4 core AMD shows MP Table: # APIC ID Version State Family Model Step Flags # 0 0x10 BSP, usable 16 4 1 0x178bfbff # 1 0x10 AP, usable 16 4 1 0x178bfbff # 2 0x10 AP, usable 16 4 1 0x178bfbff # 3 0x10 AP, usable 16 4 1 0x178bfbff
I can think of two reasons for only listing one CPU per package. The first would be performance, in that the OS needs to be aware of multi-threading in order not to slow down high priority tasks with stuff that is usually free when running on multiple processors (like spinlocks). The second reason would be that logical CPUs in the same package share some MSRs, and an OS that isn't aware of that may cease to work in such an environment.
Related link:
http://software.intel.com/en-us/articles/hyper-threading-implications-and-se...
This list talks about HT. Why not add entry for each core though? I am not aware of any MSRs shared between cores
Some MSRs are shared between cores, for example lots of the memory check MSRs in Nehalem, although an OS that knows about MCs would probably also know something about how they are shared. Adding multiple cores to the MPS tables would probably not break anything. One reason why they are still filtered out in at least some cases could be that multiple cores and threads are flagged with the same bit (the HTT bit).
From a41c206097e801de29668f99ae9b9342614c716d Mon Sep 17 00:00:00 2001
From: Magnus Christenssonmch@virtutech.com Date: Tue, 3 Nov 2009 12:50:09 +0100 Subject: [PATCH 2/8] Only add the first logical CPU in each physical CPU to the MPS tables.
src/mptable.c | 26 ++++++++++++++++++++++---- 1 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/mptable.c b/src/mptable.c index 805fe1b..525188d 100644 --- a/src/mptable.c +++ b/src/mptable.c @@ -44,16 +44,32 @@ mptable_init(void) config->spec = 4; memcpy(config->oemid, CONFIG_CPUNAME8, sizeof(config->oemid)); memcpy(config->productid, "0.1 ", sizeof(config->productid));
config->entrycount = MaxCountCPUs + 2 + 16; config->lapic = BUILD_APIC_ADDR;
// CPU definitions. u32 cpuid_signature, ebx, ecx, cpuid_features; cpuid(1,&cpuid_signature,&ebx,&ecx,&cpuid_features); struct mpt_cpu *cpus = (void*)&config[1];
int i;
for (i = 0; i< MaxCountCPUs; i++) {
- int i, actual_cpu_count;
- for (i = 0, actual_cpu_count = 0; i< MaxCountCPUs; i++) { struct mpt_cpu *cpu =&cpus[i];
int log_cpus = (ebx>> 16)& 0xff;/* Only populate the MPS tables with the first logical CPU in eachpackage */if ((cpuid_features& (1<< 28))&&log_cpus> 1&&((log_cpus<= 2&& (i& 1) != 0) ||(log_cpus<= 4&& (i& 3) != 0) ||(log_cpus<= 8&& (i& 7) != 0) ||(log_cpus<= 16&& (i& 15) != 0) ||(log_cpus<= 32&& (i& 31) != 0) ||(log_cpus<= 64&& (i& 63) != 0) ||(log_cpus<= 128&& (i& 127) != 0)))continue;Isn't this the same as (i& (log_cpus - 1) != 0)?
Yes, as long as log_cpus is a power of 2. But not if it's for example 3.
M.
-- Gleb.
On Thu, Nov 05, 2009 at 11:24:17AM +0100, Magnus Christensson wrote:
On 11/05/2009 10:54 AM, Gleb Natapov wrote:
On Thu, Nov 05, 2009 at 10:30:49AM +0100, Magnus Christensson wrote:
On 11/04/2009 03:40 PM, Gleb Natapov wrote:
On Tue, Nov 03, 2009 at 10:30:12PM -0500, Kevin O'Connor wrote:
On Tue, Nov 03, 2009 at 04:31:32PM +0100, Magnus Christensson wrote:
I'm attaching patches for seabios to make it work on the Virtutech Simics x86-440bx model. Please let me know if there is some other list that is preferred for seabios patches.
Patches 1-6 and 9 are not really related to the Virtutech model at all, so those would be prime candidates to be included in the mainline version.
l> Thanks Magnus.
Gleb I'm not sure if you're on the coreboot mailing list - can you take a look at these patches as well? A URL is at:
Can I get them in a mbox format somewhere? Want to tested them to be sure. From review: 1: OK
2: What is the reason for this? x86info --mptable on my 4 core AMD shows MP Table: # APIC ID Version State Family Model Step Flags # 0 0x10 BSP, usable 16 4 1 0x178bfbff # 1 0x10 AP, usable 16 4 1 0x178bfbff # 2 0x10 AP, usable 16 4 1 0x178bfbff # 3 0x10 AP, usable 16 4 1 0x178bfbff
I can think of two reasons for only listing one CPU per package. The first would be performance, in that the OS needs to be aware of multi-threading in order not to slow down high priority tasks with stuff that is usually free when running on multiple processors (like spinlocks). The second reason would be that logical CPUs in the same package share some MSRs, and an OS that isn't aware of that may cease to work in such an environment.
Related link:
http://software.intel.com/en-us/articles/hyper-threading-implications-and-se...
This list talks about HT. Why not add entry for each core though? I am not aware of any MSRs shared between cores
Some MSRs are shared between cores, for example lots of the memory check MSRs in Nehalem, although an OS that knows about MCs would probably also know something about how they are shared. Adding multiple cores to the MPS tables would probably not break anything. One reason why they are still filtered out in at least some cases could be that multiple cores and threads are flagged with the same bit (the HTT bit).
OK. I don't think it is very important for modern guests anyway.
From a41c206097e801de29668f99ae9b9342614c716d Mon Sep 17 00:00:00 2001
From: Magnus Christenssonmch@virtutech.com Date: Tue, 3 Nov 2009 12:50:09 +0100 Subject: [PATCH 2/8] Only add the first logical CPU in each physical CPU to the MPS tables.
src/mptable.c | 26 ++++++++++++++++++++++---- 1 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/mptable.c b/src/mptable.c index 805fe1b..525188d 100644 --- a/src/mptable.c +++ b/src/mptable.c @@ -44,16 +44,32 @@ mptable_init(void) config->spec = 4; memcpy(config->oemid, CONFIG_CPUNAME8, sizeof(config->oemid)); memcpy(config->productid, "0.1 ", sizeof(config->productid));
config->entrycount = MaxCountCPUs + 2 + 16; config->lapic = BUILD_APIC_ADDR;
// CPU definitions. u32 cpuid_signature, ebx, ecx, cpuid_features; cpuid(1,&cpuid_signature,&ebx,&ecx,&cpuid_features); struct mpt_cpu *cpus = (void*)&config[1];
int i;
for (i = 0; i< MaxCountCPUs; i++) {
- int i, actual_cpu_count;
- for (i = 0, actual_cpu_count = 0; i< MaxCountCPUs; i++) { struct mpt_cpu *cpu =&cpus[i];
int log_cpus = (ebx>> 16)& 0xff;/* Only populate the MPS tables with the first logical CPU in eachpackage */if ((cpuid_features& (1<< 28))&&log_cpus> 1&&((log_cpus<= 2&& (i& 1) != 0) ||(log_cpus<= 4&& (i& 3) != 0) ||(log_cpus<= 8&& (i& 7) != 0) ||(log_cpus<= 16&& (i& 15) != 0) ||(log_cpus<= 32&& (i& 31) != 0) ||(log_cpus<= 64&& (i& 63) != 0) ||(log_cpus<= 128&& (i& 127) != 0)))continue;Isn't this the same as (i& (log_cpus - 1) != 0)?
Yes, as long as log_cpus is a power of 2. But not if it's for example 3.
How about this:
static inline unsigned long fls(unsigned long word) { asm("bsr %1,%0" : "=r" (word) : "rm" (word)); return word + 1; }
log_cpus = 1UL << fls(atoi(log_cpus) - 1)
(i & (log_cpus - 1) != 0)
-- Gleb.
On 11/05/2009 11:49 AM, Gleb Natapov wrote:
On Thu, Nov 05, 2009 at 11:24:17AM +0100, Magnus Christensson wrote:
On 11/05/2009 10:54 AM, Gleb Natapov wrote:
On Thu, Nov 05, 2009 at 10:30:49AM +0100, Magnus Christensson wrote:
On 11/04/2009 03:40 PM, Gleb Natapov wrote:
On Tue, Nov 03, 2009 at 10:30:12PM -0500, Kevin O'Connor wrote:
On Tue, Nov 03, 2009 at 04:31:32PM +0100, Magnus Christensson wrote:
> I'm attaching patches for seabios to make it work on the Virtutech > Simics x86-440bx model. Please let me know if there is some other list > that is preferred for seabios patches. > > Patches 1-6 and 9 are not really related to the Virtutech model at all, > so those would be prime candidates to be included in the mainline > version. >
l> Thanks Magnus.
Gleb I'm not sure if you're on the coreboot mailing list - can you take a look at these patches as well? A URL is at:
Can I get them in a mbox format somewhere? Want to tested them to be sure. From review: 1: OK
2: What is the reason for this? x86info --mptable on my 4 core AMD shows MP Table: # APIC ID Version State Family Model Step Flags # 0 0x10 BSP, usable 16 4 1 0x178bfbff # 1 0x10 AP, usable 16 4 1 0x178bfbff # 2 0x10 AP, usable 16 4 1 0x178bfbff # 3 0x10 AP, usable 16 4 1 0x178bfbff
I can think of two reasons for only listing one CPU per package. The first would be performance, in that the OS needs to be aware of multi-threading in order not to slow down high priority tasks with stuff that is usually free when running on multiple processors (like spinlocks). The second reason would be that logical CPUs in the same package share some MSRs, and an OS that isn't aware of that may cease to work in such an environment.
Related link:
http://software.intel.com/en-us/articles/hyper-threading-implications-and-se...
This list talks about HT. Why not add entry for each core though? I am not aware of any MSRs shared between cores
Some MSRs are shared between cores, for example lots of the memory check MSRs in Nehalem, although an OS that knows about MCs would probably also know something about how they are shared. Adding multiple cores to the MPS tables would probably not break anything. One reason why they are still filtered out in at least some cases could be that multiple cores and threads are flagged with the same bit (the HTT bit).
OK. I don't think it is very important for modern guests anyway.
Right. Anything modern should look at the ACPI tables.
From a41c206097e801de29668f99ae9b9342614c716d Mon Sep 17 00:00:00 2001
From: Magnus Christenssonmch@virtutech.com Date: Tue, 3 Nov 2009 12:50:09 +0100 Subject: [PATCH 2/8] Only add the first logical CPU in each physical CPU to the MPS tables.
src/mptable.c | 26 ++++++++++++++++++++++---- 1 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/mptable.c b/src/mptable.c index 805fe1b..525188d 100644 --- a/src/mptable.c +++ b/src/mptable.c @@ -44,16 +44,32 @@ mptable_init(void) config->spec = 4; memcpy(config->oemid, CONFIG_CPUNAME8, sizeof(config->oemid)); memcpy(config->productid, "0.1 ", sizeof(config->productid));
config->entrycount = MaxCountCPUs + 2 + 16; config->lapic = BUILD_APIC_ADDR;
// CPU definitions. u32 cpuid_signature, ebx, ecx, cpuid_features; cpuid(1,&cpuid_signature,&ebx,&ecx,&cpuid_features); struct mpt_cpu *cpus = (void*)&config[1];
int i;
for (i = 0; i< MaxCountCPUs; i++) {
- int i, actual_cpu_count;
- for (i = 0, actual_cpu_count = 0; i< MaxCountCPUs; i++) { struct mpt_cpu *cpu =&cpus[i];
int log_cpus = (ebx>> 16)& 0xff;/* Only populate the MPS tables with the first logical CPU in eachpackage */if ((cpuid_features& (1<< 28))&&log_cpus> 1&&((log_cpus<= 2&& (i& 1) != 0) ||(log_cpus<= 4&& (i& 3) != 0) ||(log_cpus<= 8&& (i& 7) != 0) ||(log_cpus<= 16&& (i& 15) != 0) ||(log_cpus<= 32&& (i& 31) != 0) ||(log_cpus<= 64&& (i& 63) != 0) ||(log_cpus<= 128&& (i& 127) != 0)))continue;Isn't this the same as (i& (log_cpus - 1) != 0)?
Yes, as long as log_cpus is a power of 2. But not if it's for example 3.
How about this:
static inline unsigned long fls(unsigned long word) { asm("bsr %1,%0" : "=r" (word) : "rm" (word)); return word + 1; }
log_cpus = 1UL<< fls(atoi(log_cpus) - 1)
(i& (log_cpus - 1) != 0)
Why atoi? Other than that it looks correct (assuming that the inline asm syntax is correct which I'm not fluent in).
M.
On Thu, Nov 05, 2009 at 02:02:09PM +0100, Magnus Christensson wrote:
On 11/05/2009 11:49 AM, Gleb Natapov wrote:
On Thu, Nov 05, 2009 at 11:24:17AM +0100, Magnus Christensson wrote:
On 11/05/2009 10:54 AM, Gleb Natapov wrote:
On Thu, Nov 05, 2009 at 10:30:49AM +0100, Magnus Christensson wrote:
On 11/04/2009 03:40 PM, Gleb Natapov wrote:
On Tue, Nov 03, 2009 at 10:30:12PM -0500, Kevin O'Connor wrote: >On Tue, Nov 03, 2009 at 04:31:32PM +0100, Magnus Christensson wrote: >>I'm attaching patches for seabios to make it work on the Virtutech >>Simics x86-440bx model. Please let me know if there is some other list >>that is preferred for seabios patches. >> >>Patches 1-6 and 9 are not really related to the Virtutech model at all, >>so those would be prime candidates to be included in the mainline >>version. l> Thanks Magnus. >Gleb I'm not sure if you're on the coreboot mailing list - can you >take a look at these patches as well? A URL is at: > >http://permalink.gmane.org/gmane.linux.bios/55487 > Can I get them in a mbox format somewhere? Want to tested them to be sure. From review: 1: OK
2: What is the reason for this? x86info --mptable on my 4 core AMD shows MP Table: # APIC ID Version State Family Model Step Flags # 0 0x10 BSP, usable 16 4 1 0x178bfbff # 1 0x10 AP, usable 16 4 1 0x178bfbff # 2 0x10 AP, usable 16 4 1 0x178bfbff # 3 0x10 AP, usable 16 4 1 0x178bfbff
I can think of two reasons for only listing one CPU per package. The first would be performance, in that the OS needs to be aware of multi-threading in order not to slow down high priority tasks with stuff that is usually free when running on multiple processors (like spinlocks). The second reason would be that logical CPUs in the same package share some MSRs, and an OS that isn't aware of that may cease to work in such an environment.
Related link:
http://software.intel.com/en-us/articles/hyper-threading-implications-and-se...
This list talks about HT. Why not add entry for each core though? I am not aware of any MSRs shared between cores
Some MSRs are shared between cores, for example lots of the memory check MSRs in Nehalem, although an OS that knows about MCs would probably also know something about how they are shared. Adding multiple cores to the MPS tables would probably not break anything. One reason why they are still filtered out in at least some cases could be that multiple cores and threads are flagged with the same bit (the HTT bit).
OK. I don't think it is very important for modern guests anyway.
Right. Anything modern should look at the ACPI tables.
From a41c206097e801de29668f99ae9b9342614c716d Mon Sep 17 00:00:00 2001
From: Magnus Christenssonmch@virtutech.com Date: Tue, 3 Nov 2009 12:50:09 +0100 Subject: [PATCH 2/8] Only add the first logical CPU in each physical CPU to the MPS tables.
src/mptable.c | 26 ++++++++++++++++++++++---- 1 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/mptable.c b/src/mptable.c index 805fe1b..525188d 100644 --- a/src/mptable.c +++ b/src/mptable.c @@ -44,16 +44,32 @@ mptable_init(void) config->spec = 4; memcpy(config->oemid, CONFIG_CPUNAME8, sizeof(config->oemid)); memcpy(config->productid, "0.1 ", sizeof(config->productid));
config->entrycount = MaxCountCPUs + 2 + 16; config->lapic = BUILD_APIC_ADDR;
// CPU definitions. u32 cpuid_signature, ebx, ecx, cpuid_features; cpuid(1,&cpuid_signature,&ebx,&ecx,&cpuid_features); struct mpt_cpu *cpus = (void*)&config[1];
int i;
for (i = 0; i< MaxCountCPUs; i++) {
- int i, actual_cpu_count;
- for (i = 0, actual_cpu_count = 0; i< MaxCountCPUs; i++) { struct mpt_cpu *cpu =&cpus[i];
int log_cpus = (ebx>> 16)& 0xff;/* Only populate the MPS tables with the first logical CPU in eachpackage */if ((cpuid_features& (1<< 28))&&log_cpus> 1&&((log_cpus<= 2&& (i& 1) != 0) ||(log_cpus<= 4&& (i& 3) != 0) ||(log_cpus<= 8&& (i& 7) != 0) ||(log_cpus<= 16&& (i& 15) != 0) ||(log_cpus<= 32&& (i& 31) != 0) ||(log_cpus<= 64&& (i& 63) != 0) ||(log_cpus<= 128&& (i& 127) != 0)))continue;Isn't this the same as (i& (log_cpus - 1) != 0)?
Yes, as long as log_cpus is a power of 2. But not if it's for example 3.
How about this:
static inline unsigned long fls(unsigned long word) { asm("bsr %1,%0" : "=r" (word) : "rm" (word)); return word + 1; }
log_cpus = 1UL<< fls(atoi(log_cpus) - 1)
(i& (log_cpus - 1) != 0)
Why atoi? Other than that it looks correct (assuming that the inline asm syntax is correct which I'm not fluent in).
atoi because of wrong cut&paste :) Drop it. Asm is from Linux so syntax is correct.
-- Gleb.
On 11/05/2009 02:04 PM, Gleb Natapov wrote:
On Thu, Nov 05, 2009 at 02:02:09PM +0100, Magnus Christensson wrote:
On 11/05/2009 11:49 AM, Gleb Natapov wrote:
On Thu, Nov 05, 2009 at 11:24:17AM +0100, Magnus Christensson wrote:
On 11/05/2009 10:54 AM, Gleb Natapov wrote:
On Thu, Nov 05, 2009 at 10:30:49AM +0100, Magnus Christensson wrote:
On 11/04/2009 03:40 PM, Gleb Natapov wrote:
> On Tue, Nov 03, 2009 at 10:30:12PM -0500, Kevin O'Connor wrote: > >> On Tue, Nov 03, 2009 at 04:31:32PM +0100, Magnus Christensson wrote: >> >>> I'm attaching patches for seabios to make it work on the Virtutech >>> Simics x86-440bx model. Please let me know if there is some other list >>> that is preferred for seabios patches. >>> >>> Patches 1-6 and 9 are not really related to the Virtutech model at all, >>> so those would be prime candidates to be included in the mainline >>> version. >>> > l> Thanks Magnus. > >> Gleb I'm not sure if you're on the coreboot mailing list - can you >> take a look at these patches as well? A URL is at: >> >> http://permalink.gmane.org/gmane.linux.bios/55487 >> >> > Can I get them in a mbox format somewhere? Want to tested them to be > sure. From review: > 1: > OK > > 2: > What is the reason for this? x86info --mptable on my 4 core AMD shows > MP Table: > # APIC ID Version State Family Model Step Flags > # 0 0x10 BSP, usable 16 4 1 0x178bfbff > # 1 0x10 AP, usable 16 4 1 0x178bfbff > # 2 0x10 AP, usable 16 4 1 0x178bfbff > # 3 0x10 AP, usable 16 4 1 0x178bfbff > I can think of two reasons for only listing one CPU per package. The first would be performance, in that the OS needs to be aware of multi-threading in order not to slow down high priority tasks with stuff that is usually free when running on multiple processors (like spinlocks). The second reason would be that logical CPUs in the same package share some MSRs, and an OS that isn't aware of that may cease to work in such an environment.
Related link:
http://software.intel.com/en-us/articles/hyper-threading-implications-and-se...
This list talks about HT. Why not add entry for each core though? I am not aware of any MSRs shared between cores
Some MSRs are shared between cores, for example lots of the memory check MSRs in Nehalem, although an OS that knows about MCs would probably also know something about how they are shared. Adding multiple cores to the MPS tables would probably not break anything. One reason why they are still filtered out in at least some cases could be that multiple cores and threads are flagged with the same bit (the HTT bit).
OK. I don't think it is very important for modern guests anyway.
Right. Anything modern should look at the ACPI tables.
> From a41c206097e801de29668f99ae9b9342614c716d Mon Sep 17 00:00:00 2001 > From: Magnus Christenssonmch@virtutech.com Date: Tue, 3 Nov 2009 12:50:09 +0100 Subject: [PATCH 2/8] Only add the first logical CPU in each physical CPU to the MPS tables.
src/mptable.c | 26 ++++++++++++++++++++++---- 1 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/mptable.c b/src/mptable.c index 805fe1b..525188d 100644 --- a/src/mptable.c +++ b/src/mptable.c @@ -44,16 +44,32 @@ mptable_init(void) config->spec = 4; memcpy(config->oemid, CONFIG_CPUNAME8, sizeof(config->oemid)); memcpy(config->productid, "0.1 ", sizeof(config->productid));
config->entrycount = MaxCountCPUs + 2 + 16; config->lapic = BUILD_APIC_ADDR;
// CPU definitions. u32 cpuid_signature, ebx, ecx, cpuid_features; cpuid(1,&cpuid_signature,&ebx,&ecx,&cpuid_features); struct mpt_cpu *cpus = (void*)&config[1];
int i;
for (i = 0; i< MaxCountCPUs; i++) {
- int i, actual_cpu_count;
- for (i = 0, actual_cpu_count = 0; i< MaxCountCPUs; i++) { struct mpt_cpu *cpu =&cpus[i];
int log_cpus = (ebx>> 16)& 0xff;/* Only populate the MPS tables with the first logical CPU in eachpackage */if ((cpuid_features& (1<< 28))&&log_cpus> 1&&((log_cpus<= 2&& (i& 1) != 0) ||(log_cpus<= 4&& (i& 3) != 0) ||(log_cpus<= 8&& (i& 7) != 0) ||(log_cpus<= 16&& (i& 15) != 0) ||(log_cpus<= 32&& (i& 31) != 0) ||(log_cpus<= 64&& (i& 63) != 0) ||(log_cpus<= 128&& (i& 127) != 0)))continue;Isn't this the same as (i& (log_cpus - 1) != 0)?
Yes, as long as log_cpus is a power of 2. But not if it's for example 3.
How about this:
static inline unsigned long fls(unsigned long word) { asm("bsr %1,%0" : "=r" (word) : "rm" (word)); return word + 1; }
log_cpus = 1UL<< fls(atoi(log_cpus) - 1)
(i& (log_cpus - 1) != 0)
Why atoi? Other than that it looks correct (assuming that the inline asm syntax is correct which I'm not fluent in).
atoi because of wrong cut&paste :) Drop it. Asm is from Linux so syntax is correct.
Ok. Changed patches attached.
M.
On Thu, Nov 05, 2009 at 03:00:45PM +0100, Magnus Christensson wrote:
Ok. Changed patches attached.
Looks good to me.
-- Gleb.
On Thu, Nov 05, 2009 at 03:00:45PM +0100, Magnus Christensson wrote:
Ok. Changed patches attached.
Thanks Magnus. I've committed patches 1-3 and 6. I have a question on patch 4:
@@ -91,6 +93,12 @@ smp_probe(void) u32 val = readl(APIC_SVR); writel(APIC_SVR, val | APIC_ENABLED);
- /* Set LINT0 as Ext_INT, level triggered */
- writel(APIC_LINT0, 0x8700);
- /* Set LINT1 as NMI, level triggered */
- writel(APIC_LINT1, 0x8400);
This will get run on real hardware when used with coreboot. Is this safe on all machines? If not, it should be wrapped in an if (!CONFIG_COREBOOT) clause.
I'll commit patch 5 once patch 4 is clear.
On patch 6 - I've been trying to avoid using #if statements - can you rework the patch with regular C if() clauses? Also, can you rename VIRTUTECH_PC_SHADOW to have a CONFIG_ prefix and place it with the rest of the CONFIG_ items in the config.h file. It would be nice if simutech could be auto-detected, but that's not a requirement.
Finally, on patch 7:
@@ -105,7 +105,7 @@ smp_probe(void) writel(APIC_ICR_LOW, 0x000C4600 | sipi_vector);
// Wait for other CPUs to process the SIPI.
- if (CONFIG_COREBOOT) {
- if (CONFIG_COREBOOT || !USE_CMOS_BIOS_SMP_COUNT) { msleep(10); } else { u8 cmos_smp_count = inb_cmos(CMOS_BIOS_SMP_COUNT);
I'm wondering if this should just say something like:
if (!kvm_para_available() && !qemu_para_available()) msleep(10); ...
That is, instead of defining a compile time parameter, I wonder if the default should be to msleep and only use the cmos method when qemu is detected - the cmos thing is really qemu specific anyway. Gleb - do you know a good way to determine if we're running on qemu?
-Kevin
On Mon, Nov 09, 2009 at 07:44:20PM -0500, Kevin O'Connor wrote:
That is, instead of defining a compile time parameter, I wonder if the default should be to msleep and only use the cmos method when qemu is detected - the cmos thing is really qemu specific anyway. Gleb - do you know a good way to determine if we're running on qemu?
Checking that qemu cfg is present.
-- Gleb.
On 11/10/2009 01:44 AM, Kevin O'Connor wrote:
On Thu, Nov 05, 2009 at 03:00:45PM +0100, Magnus Christensson wrote:
Ok. Changed patches attached.
Thanks Magnus. I've committed patches 1-3 and 6. I have a question on patch 4:
@@ -91,6 +93,12 @@ smp_probe(void) u32 val = readl(APIC_SVR); writel(APIC_SVR, val | APIC_ENABLED);
- /* Set LINT0 as Ext_INT, level triggered */
- writel(APIC_LINT0, 0x8700);
- /* Set LINT1 as NMI, level triggered */
- writel(APIC_LINT1, 0x8400);
This will get run on real hardware when used with coreboot. Is this safe on all machines? If not, it should be wrapped in an if (!CONFIG_COREBOOT) clause.
I'm not sure how work is divided between Coreboot and Seabios. Does Coreboot do all the machine specific initialization? Then the LINT LVTs should already have been initialized.
I'll commit patch 5 once patch 4 is clear.
We'd probably like to make that conditional if we skip the initialization. Something like:
if (!CONFIG_COREBOOT || (readl(APIC_LINT0) == <expected LINT0 value>) && readl(APIC_LINT1) == <expected LINT1 value>)) { add LINT entries }
On patch 6 - I've been trying to avoid using #if statements - can you rework the patch with regular C if() clauses? Also, can you rename VIRTUTECH_PC_SHADOW to have a CONFIG_ prefix and place it with the rest of the CONFIG_ items in the config.h file. It would be nice if simutech could be auto-detected, but that's not a requirement.
I'll rework that.
Finally, on patch 7:
@@ -105,7 +105,7 @@ smp_probe(void) writel(APIC_ICR_LOW, 0x000C4600 | sipi_vector);
// Wait for other CPUs to process the SIPI.
- if (CONFIG_COREBOOT) {
- if (CONFIG_COREBOOT || !USE_CMOS_BIOS_SMP_COUNT) { msleep(10); } else { u8 cmos_smp_count = inb_cmos(CMOS_BIOS_SMP_COUNT);
I'm wondering if this should just say something like:
if (!kvm_para_available()&& !qemu_para_available()) msleep(10); ...
That is, instead of defining a compile time parameter, I wonder if the default should be to msleep and only use the cmos method when qemu is detected - the cmos thing is really qemu specific anyway. Gleb - do you know a good way to determine if we're running on qemu?
Is KVM also using the QEMU port interface from paravirt.c? If so, we could do the following:
if (qemu_cfg_use_cmos_smp_count()) { u8 cmos_smp_count = inb_cmos(CMOS_BIOS_SMP_COUNT); while (cmos_smp_count + 1 != readl(&CountCPUs)) ; } else { msleep(10); }
What does the QEMU paravirt device return for an unknown request? Assuming 0, we need to invert the query to stay compatible (the Virtutech version of the paravirt device would return 1 to use msleep instead of the count in CMOS).
int qemu_cfg_use_cmos_smp_count(void) { u16 v; if (!qemu_cfg_present) return 0;
qemu_cfg_read_entry(&v, QEMU_CFG_SKIP_CMOS_SMP_COUNT, sizeof(v));
return !v; }
M.
Magnus Christensson wrote:
I'm not sure how work is divided between Coreboot and Seabios. Does Coreboot do all the machine specific initialization?
This is the idea.
Then the LINT LVTs should already have been initialized.
How are you using coreboot+SeaBIOS? Are you using the QEMU coreboot target? Maybe that isn't good enough for Simics?
//Peter
Peter Stuge wrote:
Magnus Christensson wrote:
I'm not sure how work is divided between Coreboot and Seabios. Does Coreboot do all the machine specific initialization?
This is the idea.
Ok.
Then the LINT LVTs should already have been initialized.
How are you using coreboot+SeaBIOS? Are you using the QEMU coreboot target? Maybe that isn't good enough for Simics?
I'm using naked SeaBIOS.
M.
On Tue, Nov 10, 2009 at 09:30:47AM +0100, Magnus Christensson wrote:
I'm not sure how work is divided between Coreboot and Seabios. Does Coreboot do all the machine specific initialization? Then the LINT LVTs should already have been initialized.
Yes. Coreboot is tasked with initializing machine specific hardware (eg, cpus, memory controllers, pci). Coreboot also provides the bios tables (eg, mptable, acpi).
Is KVM also using the QEMU port interface from paravirt.c?
Yes.
If so, we could do the following:
if (qemu_cfg_use_cmos_smp_count()) { u8 cmos_smp_count = inb_cmos(CMOS_BIOS_SMP_COUNT); while (cmos_smp_count + 1 != readl(&CountCPUs)) ; } else { msleep(10); }
That's okay with me, but it would require patching qemu to reserve a new qemu_cfg id for QEMU_CFG_SKIP_CMOS_SMP_COUNT.
What does the QEMU paravirt device return for an unknown request?
I believe it does.
-Kevin
-
Gleb Natapov -
Kevin O'Connor -
Magnus Christensson -
Peter Stuge