Patchwork Seabios on Virtutech Simics x86-440bx model

login
register
about
Submitter Magnus Christensson
Date 2009-11-05 14:00:45
Message ID <4AF2DA8D.1070302@virtutech.com>
Download mbox | patch
Permalink /patch/538/
State Not Applicable
Headers show

Comments

Magnus Christensson - 2009-11-05 14:00:45
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-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.
>    
Ok. Changed patches attached.

M.
Gleb Natapov - 2009-11-08 13:47:19
On Thu, Nov 05, 2009 at 03:00:45PM +0100, Magnus Christensson wrote:
> Ok. Changed patches attached.
Looks good to me.

--
			Gleb.
Kevin O'Connor - 2009-11-10 00:44:20
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
Gleb Natapov - 2009-11-10 08:18:04
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.
Magnus Christensson - 2009-11-10 08:30:47
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.
Peter Stuge - 2009-11-10 15:53:49
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
Magnus Christensson - 2009-11-11 12:48:37
Kevin O'Connor wrote:
> Hi Magnus,
>
> Is:
>
>     for (i = 0, actual_cpu_count = 0; i < MaxCountCPUs; i++) {
>         int log_cpus = (ebx >> 16) & 0xff;
>         log_cpus = 1UL << fls(log_cpus - 1); /* round up to power of 2 */
>         if ((cpuid_features & (1 << 28)) && (i & (log_cpus - 1)) != 0)
>             continue;
>
> equivalent to:
>
>     int log_cpus = 1;
>     if (cpuid_features & (1 << 28)) {
>         log_cpus = (ebx >> 16) & 0xff;
>         log_cpus = 1UL << fls(log_cpus - 1); /* round up to power of 2 */
>     }
>     for (i = 0, actual_cpu_count = 0; i < MaxCountCPUs; i+=log_cpus) {
>   
Yes, that should do the same thing.

M.
Magnus Christensson - 2009-11-11 12:59:01
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.
Kevin O'Connor - 2009-11-11 13:43:06
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
Kevin O'Connor - 2009-11-11 13:49:02
Hi Magnus,

Is:

    for (i = 0, actual_cpu_count = 0; i < MaxCountCPUs; i++) {
        int log_cpus = (ebx >> 16) & 0xff;
        log_cpus = 1UL << fls(log_cpus - 1); /* round up to power of 2 */
        if ((cpuid_features & (1 << 28)) && (i & (log_cpus - 1)) != 0)
            continue;

equivalent to:

    int log_cpus = 1;
    if (cpuid_features & (1 << 28)) {
        log_cpus = (ebx >> 16) & 0xff;
        log_cpus = 1UL << fls(log_cpus - 1); /* round up to power of 2 */
    }
    for (i = 0, actual_cpu_count = 0; i < MaxCountCPUs; i+=log_cpus) {

-Kevin
Peter Stuge - 2009-11-11 15:01:32
Magnus Christensson wrote:
>> 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.

Ah, ok. That makes sense. Thanks!


//Peter

Patch

From 478f7b800a35f58f718ce11c278506e011c0d853 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/9] 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