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

[Kevin O'Connor]

On Tue, May 10, 2016 at 02:13:35PM +0200, Igor Mammedov wrote:
> On Mon, 9 May 2016 11:12:25 -0400
> "Kevin O'Connor" <kevin at koconnor.net> 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 <imammedo at redhat.com>
> > > ---
> > >  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) {

Okay.  I think something like:

    ...
    if (romfile_loadint("etc/create-legacy-tables", 1)) {
        pirtable_setup();
        mptable_setup();
    }
    ...

would be better.  But, I'm also okay with the above MaxCountCPUs
solution.

Thanks.
-Kevin