Patchwork Seabios on Virtutech Simics x86-440bx model

login
register
about
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

Magnus Christensson - 2009-11-05 09:30:49
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/

> 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.
Gleb Natapov - 2009-11-05 09:54:47
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.
Magnus Christensson - 2009-11-05 10:24:17
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.
>
>
Gleb Natapov - 2009-11-05 10:49:53
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.
Magnus Christensson - 2009-11-05 13:02:09
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.
Gleb Natapov - 2009-11-05 13:04:39
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