[PATCHv2] acpi: make default DSDT optional

[Michael S. Tsirkin]

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

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

[Gerd Hoffmann]

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

Commit message still says "default off".

Other than that it looks good to me.

cheers,
  Gerd

[Paul Menzel]

Dear Michael,

Am Montag, den 18.03.2013, 12:12 +0200 schrieb Michael S. Tsirkin:
> QEMU now loads its own copy of DSDT, so let's not build in PIIX.

for non-QEMU developers could you please add a version string or commit
so it is known since when QEMU supports this?
> 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(-)
>
> 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

Whitespace? Everything else seems to use spaces.
>    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) {

Thanks,

Paul

[Laszlo Ersek]

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

[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

[Peter Stuge]

Michael S. Tsirkin wrote:
> Also, in qemu I don't think we want to carry around code we never use.

I'm sure you know that the SeaBIOS world is a lot more than just QEMU,
so probably it makes sense to not change whatever the default was
before your change. I get the impression that you have.

//Peter