Re: [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
> >
> > 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