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

[Gleb Natapov]

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 Christensson
> > > > > 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.