[PATCHv3] acpi: make default DSDT optional

[Michael S. Tsirkin]

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 
---

Changes from v2:
       - fixed and extended the commit log addressing comments by Gerd,
         Laszlo, Paul. No functional changes.

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(-)

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
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

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) {

[Kevin O'Connor]

On Mon, Mar 18, 2013 at 01:00:42PM +0200, 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)

Okay.
> --- 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.

As Paul points out, it would really help if the help stated which
released version of QEMU is needed to turn this off.
> --- 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

As Paul points out, the #ifdef doesn't make sense as the symbol is
always defined.  However, lets avoid #ifs and let the build weed this
out.
> if (fadt && !fadt->dsdt) {
> if (fadt && !fadt->dsdt && CONFIG_ACPI_DSDT) {

It would be preferable to make the CONFIG_X be first in the if.

Thanks.
-Kevin

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

[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
> >
> > 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.

[Paul Menzel]

Am Montag, den 18.03.2013, 08:57 -0400 schrieb Kevin O'Connor:
> On Mon, Mar 18, 2013 at 01:00:42PM +0200, Michael S. Tsirkin wrote:
> > Since commit f7e4dd6c18ccfbaf6cd2f5eaaed2b77cabc8a406 QEMU loads its

Michael, thank you for putting this in. As commit hashes are hard to
memorize adding the summary to is useful in my opinion. For example for
this commit like this.

Since commit »acpi: autoload dsdt« (f7e4dd6c) [1] …

[…]

[1] http://git.qemu.org/?p=qemu.git;a=commit;h=f7e4dd6c18ccfbaf6cd2f5eaaed2b77ca...

[…]
> > --- 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.
>
> As Paul points out, it would really help if the help stated which
> released version of QEMU is needed to turn this off.

I just meant the commit message, but it is useful in Kconfig too. Kevin,
good suggestion.
> > --- 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
>
> As Paul points out, the #ifdef doesn't make sense as the symbol is
> always defined.  However, lets avoid #ifs and let the build weed this
> out.

That was Laszlo and not me. ;-)
> > if (fadt && !fadt->dsdt) {
> > if (fadt && !fadt->dsdt && CONFIG_ACPI_DSDT) {
>
> It would be preferable to make the CONFIG_X be first in the if.

Thanks,

Paul

[Michael S. Tsirkin]

On Mon, Mar 18, 2013 at 02:22:53PM +0100, Paul Menzel wrote:
> Am Montag, den 18.03.2013, 08:57 -0400 schrieb Kevin O'Connor:
> > On Mon, Mar 18, 2013 at 01:00:42PM +0200, Michael S. Tsirkin wrote:
> > > Since commit f7e4dd6c18ccfbaf6cd2f5eaaed2b77cabc8a406 QEMU loads its
>
> Michael, thank you for putting this in. As commit hashes are hard to
> memorize adding the summary to is useful in my opinion. For example for
> this commit like this.
>
> Since commit »acpi: autoload dsdt« (f7e4dd6c) [1] …
>
> […]
>
> [1] http://git.qemu.org/?p=qemu.git;a=commit;h=f7e4dd6c18ccfbaf6cd2f5eaaed2b77ca...
>
> […]

URLs are best avoided as servers come and go and git history is
immutable. Not sure what does the summary add - I think
the main thing you would want to do is check whether your
history includes a specific hash. You do it like this:
git log |grep f7e4dd6c18ccfbaf6cd2f5eaaed2b77cabc8a406
> > > --- 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.
> >
> > As Paul points out, it would really help if the help stated which
> > released version of QEMU is needed to turn this off.
>
> I just meant the commit message, but it is useful in Kconfig too. Kevin,
> good suggestion.

OK I put in the QEMU release version in Kconfig, that should be best I think?
> > > --- 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
> >
> > As Paul points out, the #ifdef doesn't make sense as the symbol is
> > always defined.  However, lets avoid #ifs and let the build weed this
> > out.
>
> That was Laszlo and not me. ;-)
> > > if (fadt && !fadt->dsdt) {
> > > if (fadt && !fadt->dsdt && CONFIG_ACPI_DSDT) {
> >
> > It would be preferable to make the CONFIG_X be first in the if.
>
> Thanks,
>
> Paul

[Paul Menzel]

Am Montag, den 18.03.2013, 16:17 +0200 schrieb Michael S. Tsirkin:
> On Mon, Mar 18, 2013 at 02:22:53PM +0100, Paul Menzel wrote:
> > Am Montag, den 18.03.2013, 08:57 -0400 schrieb Kevin O'Connor:
> > > On Mon, Mar 18, 2013 at 01:00:42PM +0200, Michael S. Tsirkin wrote:
> > > > Since commit f7e4dd6c18ccfbaf6cd2f5eaaed2b77cabc8a406 QEMU loads its
> >
> > Michael, thank you for putting this in. As commit hashes are hard to
> > memorize adding the summary to is useful in my opinion. For example for
> > this commit like this.
> >
> > Since commit »acpi: autoload dsdt« (f7e4dd6c) [1] …
> >
> > […]
> >
> > [1] http://git.qemu.org/?p=qemu.git;a=commit;h=f7e4dd6c18ccfbaf6cd2f5eaaed2b77ca...
> >
> > […]
>
> URLs are best avoided as servers come and go and git history is
> immutable. Not sure what does the summary add - I think
> the main thing you would want to do is check whether your
> history includes a specific hash.

Ok. In this case the version is good enough. But if you backport,
cherry-pick or do something different the hash changes too. So in my
opinion it is best to have both.
> You do it like this:
> git log |grep f7e4dd6c18ccfbaf6cd2f5eaaed2b77cabc8a406

$ git branch --contains f7e4dd6c18ccfbaf6cd2f5eaaed2b77cabc8a406
> > > > --- 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.
> > >
> > > As Paul points out, it would really help if the help stated which
> > > released version of QEMU is needed to turn this off.
> >
> > I just meant the commit message, but it is useful in Kconfig too. Kevin,
> > good suggestion.
>
> OK I put in the QEMU release version in Kconfig, that should be best I think?

Yes. Awesome!

[…]

Thanks,

Paul