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

[Laszlo Ersek]

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?

> 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