[SeaBIOS] [PATCHv3] acpi: make default DSDT optional

[Michael S. Tsirkin]

On Mon, Mar 18, 2013 at 02:03:42PM +0100, Laszlo Ersek wrote:
> On 03/18/13 12:00, Michael S. Tsirkin wrote:
> > Since commit f7e4dd6c18ccfbaf6cd2f5eaaed2b77cabc8a406 QEMU loads its
> > own copy of DSDT, so let's not build in PIIX.  This makes building in
> > the DSDT an option, default to on (built-in).
> > For the builds bundled with qemu (where we _know_ qemu is
> > new enougth that it actually works) we can flip the switch to 'n' (via
> > roms/config.seabios).  This way we save some space in the rom file and make
> > it easy to avoid shipping dead code in qemu.  At some point we might
> > be able to switch it off by default and then maybe remove altogether.
> > 
> > With CONFIG_ACPI_DSDT = y
> > Total size: 127348  Fixed: 58892  Free: 3724 (used 97.2% of 128KiB rom)
> > With CONFIG_ACPI_DSDT = n
> > Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
> > 
> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> > ---
> > 
> > Changes from v2:
> >        - fixed and extended the commit log addressing comments by Gerd,
> >          Laszlo, Paul. No functional changes.
> 
> Did you ignore my v2 comment below on purpose, or just miss it?

I missed it, sorry.

> > diff --git a/src/acpi.c b/src/acpi.c
> > index 119d1c1..69dd3aa 100644
> > --- a/src/acpi.c
> > +++ b/src/acpi.c
> > @@ -202,7 +202,11 @@ struct srat_memory_affinity
> >      u32    reserved3[2];
> >  } PACKED;
> >  
> > +#ifdef CONFIG_ACPI_DSDT
> >  #include "acpi-dsdt.hex"
> > +#else
> > +static u8 AmlCode[1];
> > +#endif
> 
> On 03/18/13 11:41, Laszlo Ersek wrote:
> > The macro CONFIG_ACPI_DSDT is always defined (you use it just below
> > in a controlling expression); its value is what varies.
> 
> In other words, "#ifdef CONFIG_ACPI_DSDT" always evaluates to true,
> independently of how the new config option is set. The space is probably
> saved only because gcc sees (with -fwhole-program) that there are no
> accesses to AmlCode[]. Ultimately the effect matches your high-level
> intent, but the above #ifdef code doesn't do what you expect it to do.
> 
> I think you should write
> 
>     #if CONFIG_ACPI_DSDT == 1
> 
> CONFIG_XXXX macros are not used in preprocessing directives very
> frequently, but see
> 
>     $ git grep CONFIG_ | grep '# *if'
> 
>     [...]
>     src/pirtable.c:#if CONFIG_PIRTABLE
>     src/pmm.c:#if CONFIG_PMM
>     src/pnpbios.c:#if CONFIG_PNPBIOS
>     src/romlayout.S:#if CONFIG_DISABLE_A20
>     src/romlayout.S:#if CONFIG_PCIBIOS
>     src/romlayout.S:#if CONFIG_ENTRY_EXTRASTACK
>     [...]
>     vgasrc/vgaentry.S:#if CONFIG_VGA_PCI == 1
> 
> "#if" and "#ifdef" are different.
> 
> Laszlo

Right. Thanks for the correction.