Re: [SeaBIOS] [PATCH RFC 1/2] paravirt: disable MPTable in case of more than 255 CPUs

[Igor Mammedov]

On Mon, 9 May 2016 11:12:25 -0400
"Kevin O'Connor"  wrote:
> On Mon, May 09, 2016 at 11:43:53AM +0200, Igor Mammedov wrote:
> > MPTable doesn't support more than 254 CPUs and
> > QEMU supplies an alternative MADT table which
> > guest will use instead of it. So do not install
> > MPTable if more than 254 CPUs are provided.
> > Signed-off-by: Igor Mammedov
> >
> > src/fw/mptable.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/src/fw/mptable.c b/src/fw/mptable.c
> > index 47385cc..aec26f8 100644
> > --- a/src/fw/mptable.c
> > +++ b/src/fw/mptable.c
> > @@ -24,6 +24,10 @@ mptable_setup(void)
> >      if (! CONFIG_MPTABLE)
> >          return;
> >
> > if (romfile_loadint("etc/max-cpus", 0) > 255) {
> >    dprintf(1, "MPTable doesn't support more than 254 CPUs. Skip it.\n");
> >    return;
> >
> > }
> > dprintf(3, "init MPTable\n");
> >
> > // Config structure in temp area.
>
> Thanks.  I'd prefer to not make further modifications to fw/mptable.c
> though.  I think it would be better to do something like (totally
> untested):
>
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -154,8 +154,14 @@ qemu_platform_setup(void)
>      smp_setup();
>  // Create bios tables
> pirtable_setup();
> mptable_setup();
>
> if (romfile_find("etc/legacy-tables-loader")) {
>    // QEMU may specify the PIR and mptable (or leave empty for no tables)
>    romfile_loader_execute("etc/legacy-tables-loader");
>
> } else {
>    // Load the legacy internal tables
>    pirtable_setup();
>    mptable_setup();
>
> }
> smbios_setup();
>
> if (CONFIG_FW_ROMFILE_LOAD) {
>
> And then QEMU can create an empty fw_cfg file to suppress both the
> mptable and the pir table.  (Or, it can populate it to specify exactly
> what QEMU wants in those tables.)

I don't think QEMU would ever want to provide legacy tables
as Seabios creates them just fine for supported by them
amount of CPUs. It's just that mptable_setup() is broken
when there is more than 255 CPUs which leads to out of bound
read/write accesses when dealing with APIC ID > 255.

Considering QEMU not being interested in legacy tables,
how about following:

--- a/src/fw/paravirt.c
+++ b/src/fw/paravirt.c
@@ -154,8 +154,10 @@ qemu_platform_setup(void)
     smp_setup();

// Create bios tables
-    pirtable_setup();
-    mptable_setup();
+    if (MaxCountCPUs <= 255) {
+        pirtable_setup();
+        mptable_setup();
+    }
     smbios_setup();

if (CONFIG_FW_ROMFILE_LOAD) {
> -Kevin