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

[Michael S. Tsirkin]

On Mon, Mar 18, 2013 at 11:41:44AM +0100, Laszlo Ersek wrote:
> comments in-line
> 
> On 03/18/13 11:12, Michael S. Tsirkin wrote:
> > QEMU now loads its own copy of DSDT, so let's not build in PIIX.
> > This leaves building in the DSDT an option, default to off.
> > If no one complains for a while, we'll be able to
> > remove this altogether.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> > ---
> > 
> > Changes from v1:
> > 	- default the new option to y to reduce
> > 	  disruption to existing users
> > 
> >  src/Kconfig | 6 ++++++
> >  src/acpi.c  | 7 ++++++-
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> The acpi_setup() function already picks the qemu-exported DSDT if there
> is one: fill_dsdt() links it into the FADT, and then we won't
> malloc_high() an area for our own copy.
> 
> What is the purpose of this patch? Is it to save space? I assume the
> static AmlCode array from "acpi-dsdt.hex" only occupies RAM as long as
> we don't start the boot loader / OS, so it is not important for runtime.
> Do we want to save space in the binary?

Yes.  Waste not, need not.

With default = y
Total size: 127348  Fixed: 58892  Free: 3724 (used 97.2% of 128KiB rom)
With default = n
Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)

So we get some breathing space here.  Increasing rom size is
problematic, is affects things like migration.

Also, in qemu I don't think we want to carry around code we never use.
It just results in confusion. At least, it confuses me.

> > diff --git a/src/Kconfig b/src/Kconfig
> > index 3141069..655ab1c 100644
> > --- a/src/Kconfig
> > +++ b/src/Kconfig
> > @@ -387,6 +387,12 @@ menu "BIOS Tables"
> >          default y
> >          help
> >              Support generation of ACPI tables.
> > +    config ACPI_DSDT
> > +        bool "Include default ACPI DSDT"
> > +        default y
> > +	depends on ACPI
> > +        help
> > +            Include default DSDT ACPI table in BIOS.
> >  endmenu
> >  
> >  source vgasrc/Kconfig
> 
> I think if ACPI_DSDT is off, then the Makefile could elect not to build
> "acpi-dsdt.hex" at all. (The "$(OUT)acpi.o" target should not depend on
> "acpi-dsdt.hex" in that case.) Anyway extra work is not a bug of course.
> (Plus I'm not sure how we could pass in the config value for ifeq.)
> 
> > 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
> 
> The macro CONFIG_ACPI_DSDT is always defined (you use it just below in a
> controlling expression); its value is what varies.
> 
> >  #include "acpi-dsdt.hex"
> > +#else
> > +static u8 AmlCode[1];
> > +#endif
> >  
> >  static void
> >  build_header(struct acpi_table_header *h, u32 sig, int len, u8 rev)
> > @@ -826,7 +830,8 @@ acpi_setup(void)
> >              break;
> >          }
> >      }
> > -    if (fadt && !fadt->dsdt) {
> > +
> > +    if (fadt && !fadt->dsdt && CONFIG_ACPI_DSDT) {
> >          /* default DSDT */
> >          void *dsdt = malloc_high(sizeof(AmlCode));
> >          if (!dsdt) {
> > 
> 
> For short-circuiting hygiene I'd put the config option first.
> 
> Laszlo