[coreboot] [PATCH] Seabios on Virtutech Simics x86-440bx model

[Magnus Christensson]

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.