Submitter | Magnus Christensson |
---|---|
Date | 2009-11-05 09:30:49 |
Message ID | <4AF29B49.7090001@virtutech.com> |
Download | mbox | patch |
Permalink | /patch/537/ |
State | Not Applicable |
Headers | show |
Comments
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-setup-on-microsoft-operating-systems/ > 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 each > + package */ > + 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: >>>> >>>> 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-setup-on-microsoft-operating-systems/ >> >> > 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 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 each >> + package */ >> + 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: > >>>> > >>>>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-setup-on-microsoft-operating-systems/ > >> > >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 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 each > >>+ package */ > >>+ 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: >>>>>> >>>>>> 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-setup-on-microsoft-operating-systems/ >>>> >>>> >>> 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 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 each >>>> + package */ >>>> + 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-setup-on-microsoft-operating-systems/ > >>>> > >>>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 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 each > >>>>+ package */ > >>>>+ 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.
Patch
From cd8e82ad346be99987773bbf6c22829402cd6bd5 Mon Sep 17 00:00:00 2001 From: Magnus Christensson <mch@virtutech.com> Date: Thu, 5 Nov 2009 10:26:40 +0100 Subject: [PATCH 8/8] Add USE_CMOS_BIOS_SMP_COUNT option. If disabled, we wait 10ms instead of relying on the cpu count stored in memory. --- src/config.h | 1 + src/smp.c | 2 +- 2 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/config.h b/src/config.h index f2c08ff..e9d63e7 100644 --- a/src/config.h +++ b/src/config.h @@ -191,5 +191,6 @@ /* Options for running on the Virtutech Simics x86-440bx machine model */ #define VIRTUTECH_PC_SHADOW 0 +#define USE_CMOS_BIOS_SMP_COUNT 1 #endif // config.h diff --git a/src/smp.c b/src/smp.c index b0852f8..caec5f1 100644 --- a/src/smp.c +++ b/src/smp.c @@ -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); -- 1.6.2.5