Skip to contentSkip to conversation

seabios: move acpi table formatting out of bios

RFC PATCH Superseded Michael S. Tsirkin 5 participants 24 messages Apr 25, 2013 – May 9, 2013 Kbuild

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
author
#5 lore raw

RFC PATCH 3/3 acpi: add an option to disable builtin tables

In reply to Michael S. Tsirkin

Serves to save a bit of memory, and is helpful for debugging (making sure tables come from qemu).

Memory stats:
Enabled:
Total size: 128776 Fixed: 59100 Free: 2296 (used 98.2% of 128KiB rom) Disabled:
Total size: 119836 Fixed: 58996 Free: 11236 (used 91.4% of 128KiB rom)

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 src/Kconfig | 12 +++++++++++-
 src/acpi.c  |  2 +-
 2 files changed, 12 insertions(+), 2 deletions(-)
src/Kconfig +11 -1
index 3c80132..1b54b83 100644
@@ -387,10 +387,20 @@ menu "BIOS Tables"
         default y
         help
             Support generation of ACPI tables.
+    config ACPI_BUILTIN
+        bool "Include built-in ACPI tables"
+        default y
+        depends on ACPI
+        help
+            Include built-in ACPI tables in BIOS.
+            Required for QEMU 1.5 and older.
+            This option can be disabled for QEMU 1.6 and newer
+            to save some space in the ROM file.
+            If unsure, say Y.
     config ACPI_DSDT
         bool "Include default ACPI DSDT"
         default y
-        depends on ACPI
+        depends on ACPI && ACPI_BUILTIN
         help
             Include default DSDT ACPI table in BIOS.
             Required for QEMU 1.3 and older.
src/acpi.c +1 -1
index 16ea9f4..b03b2ba 100644
@@ -664,7 +664,7 @@ acpi_setup(void)
 
     linker_link("/etc/linker-script");
 
-    if (!acpi_generate) {
+    if (!CONFIG_ACPI_BUILTIN || !acpi_generate) {
         return;
     }
Signature · 1 line
MST

Michael S. Tsirkin

author
#0 lore raw

cover seabios: move acpi table formatting out of bios

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled

    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

BIOS still has limited ability to parse the tables, for the following purposes:
- locate resume vector
- allocate RSDP in FSEG
- allocate FACS at an aligned address

Signature · 20 lines
MST


Michael S. Tsirkin (3):
  linker: utility to patch in-memory ROM files
  acpi: load and link tables from /etc/acpi/
  acpi: add an option to disable builtin tables

 Makefile     |  2 +-
 src/Kconfig  | 12 +++++++-
 src/acpi.c   | 67 +++++++++++++++++++++++++++++++++++++++++++-
 src/linker.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/linker.h | 50 +++++++++++++++++++++++++++++++++
 src/util.h   |  1 +
 6 files changed, 219 insertions(+), 3 deletions(-)
 create mode 100644 src/linker.c
 create mode 100644 src/linker.h

-- 
MST
author
#2 lore raw

RFC PATCH 2/3 acpi: load and link tables from /etc/acpi/

Load files in /etc/acpi/ and use for acpi tables.
Any files in this directory completely disable
generating and loading legacy acpi tables.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 src/acpi.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)
src/acpi.c +66 -1
index 58cd6d7..16ea9f4 100644
@@ -27,6 +27,7 @@
 #include "config.h" // CONFIG_*
 #include "paravirt.h" // RamSize
 #include "dev-q35.h"
+#include "linker.h"
 
 #include "acpi-dsdt.hex"
 

@@ -603,8 +604,72 @@ acpi_setup(void)
     if (! CONFIG_ACPI)
         return;
 
+    int acpi_generate = 1;
+
     dprintf(3, "init ACPI tables\n");
 
+    struct romfile_s *file = NULL;
+    for (;;) {
+        file = romfile_findprefix("/etc/acpi/", file);
+        if (!file)
+            break;
+
+        /*
+         * Disable ACPI table generation. All ACPI tables must come from
+         * etc/acpi/ romfile entries.
+         */
+        acpi_generate = 0;
+
+        if (!file->size)
+            continue;
+
+        void *data = malloc_high(file->size);
+        if (!data) {
+            warn_noalloc();
+            break;
+        }
+        int ret = file->copy(file, data, file->size);
+        if (ret < file->size) {
+            free(data);
+            continue;
+        }
+        /* Handle RSDP and FACS quirks */
+        if (file->size >= 8) {
+            struct rsdp_descriptor *rsdp = data;
+            struct acpi_table_header *hdr = data;
+            /* RSDP is in FSEG memory. */
+            if (rsdp->signature == cpu_to_le64(RSDP_SIGNATURE)) {
+                data = malloc_fseg(file->size);
+                if (!data) {
+                    warn_noalloc();
+                    break;
+                }
+                memcpy(data, rsdp, file->size);
+                free(rsdp);
+                /* Store Rsdp pointer for use by find_fadt. */
+                RsdpAddr = data;
+            } else if (hdr->signature == FACS_SIGNATURE) {
+                /* FACS is aligned to a 64 bit boundary. */
+                data = memalign_high(64, file->size);
+                if (!data) {
+                    warn_noalloc();
+                    break;
+                }
+                memcpy(data, hdr, file->size);
+                free(hdr);
+            }
+        }
+        file->data = data;
+    }
+
+    linker_link("/etc/linker-script");
+
+    if (!acpi_generate) {
+        return;
+    }
+
+    dprintf(3, "generate ACPI tables\n");
+
     // This code is hardcoded for PIIX4 Power Management device.
     struct pci_device *pci = pci_find_init_device(acpi_find_tbl, NULL);
     if (!pci)

@@ -630,7 +695,7 @@ acpi_setup(void)
     if (pci->device == PCI_DEVICE_ID_INTEL_ICH9_LPC)
         ACPI_INIT_TABLE(build_mcfg_q35());
 
-    struct romfile_s *file = NULL;
+    file = NULL;
     for (;;) {
         file = romfile_findprefix("acpi/", file);
         if (!file)
Signature · 1 line
MST
author
#1 lore raw

RFC PATCH 1/3 linker: utility to patch in-memory ROM files

In reply to Michael S. Tsirkin

Add ability for a ROM file to point to it's image in memory. When file is in memory, add utility that can patch it, storing pointers to one file within another file.

This is not a lot of code: together with the follow-up patch to load ACPI tables from ROM, we get:
Before:
Total size: 127880 Fixed: 59060 Free: 3192 (used 97.6% of 128KiB rom) After:
Total size: 128776 Fixed: 59100 Free: 2296 (used 98.2% of 128KiB rom)

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 Makefile     |  2 +-
 src/linker.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/linker.h | 50 +++++++++++++++++++++++++++++++++
 src/util.h   |  1 +
 4 files changed, 142 insertions(+), 1 deletion(-)
 create mode 100644 src/linker.c
 create mode 100644 src/linker.h
Makefile +1 -1
index 759bbbb..020fb2f 100644
@@ -18,7 +18,7 @@ SRC16=$(SRCBOTH) system.c disk.c font.c
 SRC32FLAT=$(SRCBOTH) post.c shadow.c memmap.c pmm.c coreboot.c boot.c \
     acpi.c smm.c mptable.c pirtable.c smbios.c pciinit.c optionroms.c mtrr.c \
     lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c \
-    biostables.c xen.c bmp.c romfile.c csm.c
+    biostables.c xen.c bmp.c romfile.c csm.c linker.c
 SRC32SEG=util.c output.c pci.c pcibios.c apm.c stacks.c
 
 # Default compiler flags
src/linker.c new file +90 -0

90 lines not on this page · raw message

src/linker.h new file +50 -0

50 lines not on this page · raw message

src/util.h +1 -0
index 996c29a..7b50c38 100644
@@ -436,6 +436,7 @@ struct romfile_s {
     char name[128];
     u32 size;
     int (*copy)(struct romfile_s *file, void *dest, u32 maxlen);
+    void *data;
 };
 void romfile_add(struct romfile_s *file);
 struct romfile_s *romfile_findprefix(const char *prefix, struct romfile_s *prev);
Signature · 1 line
MST
#6 lore raw
In reply to Michael S. Tsirkin

With ACPI moved out of SeaBIOS to QEMU, how will ACPI work when using SeaBIOS without QEMU?

Like if using SeaBIOS with Boch, KVM or Coreboot?

Quoted 62 lines · Michael S. Tsirkin

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled

    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB

rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

BIOS still has limited ability to parse the tables, for the following purposes:
- locate resume vector
- allocate RSDP in FSEG
- allocate FACS at an aligned address

--
MST

Michael S. Tsirkin (3):
linker: utility to patch in-memory ROM files
acpi: load and link tables from /etc/acpi/
acpi: add an option to disable builtin tables

 Makefile     |  2 +-
 src/Kconfig  | 12 +++++++-
 src/acpi.c   | 67 +++++++++++++++++++++++++++++++++++++++++++-
 src/linker.c | 90
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/linker.h | 50 +++++++++++++++++++++++++++++++++
 src/util.h   |  1 +
 6 files changed, 219 insertions(+), 3 deletions(-)
 create mode 100644 src/linker.c
 create mode 100644 src/linker.h

--
MST

_______________________________________________
SeaBIOS mailing list
[email protected]
http://www.seabios.org/mailman/listinfo/seabios

author
#7 lore raw
Quoted 4 lines · Fred .

With ACPI moved out of SeaBIOS to QEMU, how will ACPI work when using SeaBIOS without QEMU?

Like if using SeaBIOS with Boch, KVM or Coreboot?

KVM merged with QEMU, so it will use romfiles too. Others will have two options:

- keep using that tables in seabios. With time we will be able to drop qemu-specific stuff in the tables which should make life easier for Bochs/coreboot.
- add ACPI through romfiles like QEMU will do.

Which one will be chosen will be up to relevant projects, but note how they benefit in any case.

Quoted 66 lines · Michael S. Tsirkin

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled
    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

BIOS still has limited ability to parse the tables, for the following purposes:
        - locate resume vector
        - allocate RSDP in FSEG
        - allocate FACS at an aligned address

--
MST

Michael S. Tsirkin (3):
  linker: utility to patch in-memory ROM files
  acpi: load and link tables from /etc/acpi/
  acpi: add an option to disable builtin tables

 Makefile     |  2 +-
 src/Kconfig  | 12 +++++++-
 src/acpi.c   | 67 +++++++++++++++++++++++++++++++++++++++++++- src/linker.c | 90
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/linker.h | 50 +++++++++++++++++++++++++++++++++ src/util.h   |  1 +
 6 files changed, 219 insertions(+), 3 deletions(-) create mode 100644 src/linker.c
 create mode 100644 src/linker.h

--
MST

_______________________________________________
SeaBIOS mailing list
[email protected]
http://www.seabios.org/mailman/listinfo/seabios

#8 lore raw
Quoted 4 lines · Michael S. Tsirkin
Quoted 3 lines · Fred .

With ACPI moved out of SeaBIOS to QEMU, how will ACPI work when using SeaBIOS without QEMU?

Quoted 8 lines
Quoted 1 line

Like if using SeaBIOS with Boch, KVM or Coreboot?

KVM merged with QEMU, so it will use romfiles too. Others will have two options:

- keep using that tables in seabios. With time we will be able to drop qemu-specific stuff in the tables which should make life easier for Bochs/coreboot.

FYI - coreboot does not and never has used the acpi tables in seabios.

-Kevin

#3 lore raw

Re: [Qemu-devel] [PATCH RFC 2/3] acpi: load and link tables from /etc/acpi/

In reply to Michael S. Tsirkin

Not sure how much it counts, but I personally can agree with you on this direction :)

One note below:

Quoted 20 lines
src/acpi.c +14 -0
@@ -603,8 +604,72 @@ acpi_setup(void)
     if (! CONFIG_ACPI)
         return;
 
+    int acpi_generate = 1;
+
     dprintf(3, "init ACPI tables\n");
 
+    struct romfile_s *file = NULL;
+    for (;;) {
+        file = romfile_findprefix("/etc/acpi/", file);
+        if (!file)
+            break;
+
+        /*
+         * Disable ACPI table generation. All ACPI tables must come from
+         * etc/acpi/ romfile entries.
+         */
+        acpi_generate = 0;
+

[...]

src/acpi.c acpi_setup(void) · line 675

Quoted 13 lines
src/acpi.c +10 -0
+    }
+
+    linker_link("/etc/linker-script");
+
+    if (!acpi_generate) {
+        return;
+    }
+
+    dprintf(3, "generate ACPI tables\n");
+
     // This code is hardcoded for PIIX4 Power Management device.
     struct pci_device *pci = pci_find_init_device(acpi_find_tbl, NULL);
     if (!pci)

Are you deliberately calling linker_link() independently from "acpi_generate"? My hunch is that linker_link() only makes sense if acpi_generate == 0 (ie. qemu has passed down at least one table to repoint some pointer to), but I may be missing a use case.

Thanks
Laszlo

author
#4 lore raw

Re: [Qemu-devel] [PATCH RFC 2/3] acpi: load and link tables from /etc/acpi/

Quoted 47 lines · Laszlo Ersek

Not sure how much it counts, but I personally can agree with you on this direction :)

One note below:

Quoted 20 lines
src/acpi.c +14 -0
@@ -603,8 +604,72 @@ acpi_setup(void)
     if (! CONFIG_ACPI)
         return;
 
+    int acpi_generate = 1;
+
     dprintf(3, "init ACPI tables\n");
 
+    struct romfile_s *file = NULL;
+    for (;;) {
+        file = romfile_findprefix("/etc/acpi/", file);
+        if (!file)
+            break;
+
+        /*
+         * Disable ACPI table generation. All ACPI tables must come from
+         * etc/acpi/ romfile entries.
+         */
+        acpi_generate = 0;
+

[...]

src/acpi.c acpi_setup(void) · line 675

Quoted 13 lines
src/acpi.c +10 -0
+    }
+
+    linker_link("/etc/linker-script");
+
+    if (!acpi_generate) {
+        return;
+    }
+
+    dprintf(3, "generate ACPI tables\n");
+
     // This code is hardcoded for PIIX4 Power Management device.
     struct pci_device *pci = pci_find_init_device(acpi_find_tbl, NULL);
     if (!pci)

Are you deliberately calling linker_link() independently from "acpi_generate"? My hunch is that linker_link() only makes sense if acpi_generate == 0 (ie. qemu has passed down at least one table to repoint some pointer to), but I may be missing a use case.

Quoted 2 lines

Thanks
Laszlo

In theory linker code is unrelated to acpi, and host can the linker to patch any romfile.
I could add if (!acpi_generate) but it just seems like adding code to remove something potentially useful.

Signature · 1 line
MST
author
#9 lore raw
In reply to Michael S. Tsirkin
Quoted 18 lines · Michael S. Tsirkin

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled

    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

Quoted 8 lines

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

After some thought, there are two additional
options worth considering, in that they simplify
bios code somewhat:

- bios could get size from qemu, allocate a buffer (e.g. could be one buffer for all tables) and pass the address to qemu.
qemu does all the patching

- further, qemu could do the copy of tables into
that address directly

thoughts?

Quoted 27 lines

BIOS still has limited ability to parse the tables, for the following purposes:
- locate resume vector
- allocate RSDP in FSEG
- allocate FACS at an aligned address

Signature · 20 lines
MST


Michael S. Tsirkin (3):
  linker: utility to patch in-memory ROM files
  acpi: load and link tables from /etc/acpi/
  acpi: add an option to disable builtin tables

 Makefile     |  2 +-
 src/Kconfig  | 12 +++++++-
 src/acpi.c   | 67 +++++++++++++++++++++++++++++++++++++++++++-
 src/linker.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/linker.h | 50 +++++++++++++++++++++++++++++++++
 src/util.h   |  1 +
 6 files changed, 219 insertions(+), 3 deletions(-)
 create mode 100644 src/linker.c
 create mode 100644 src/linker.h

-- 
MST
#10 lore raw
Quoted 32 lines · Michael S. Tsirkin
Quoted 18 lines · Michael S. Tsirkin

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled

    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

Quoted 8 lines

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

After some thought, there are two additional
options worth considering, in that they simplify
bios code somewhat:

Quoted 7 lines

- bios could get size from qemu, allocate a buffer (e.g. could be one buffer for all tables) and pass the address to qemu.
qemu does all the patching

- further, qemu could do the copy of tables into
that address directly

This seems more complex than necessary to me.

The important task is to get the tables generated in QEMU - I'd focus on getting the tables generated in QEMU (one table per fw_cfg "file"). Once that is done, the SeaBIOS side can be easily implemented, and we can add any enhancements on top if we feel it is necessary.

-Kevin

#11 lore raw
Quoted 41 lines · Kevin O'Connor
Quoted 32 lines · Michael S. Tsirkin
Quoted 18 lines · Michael S. Tsirkin

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled

    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

Quoted 8 lines

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

After some thought, there are two additional
options worth considering, in that they simplify
bios code somewhat:

Quoted 7 lines

- bios could get size from qemu, allocate a buffer (e.g. could be one buffer for all tables) and pass the address to qemu.
qemu does all the patching

- further, qemu could do the copy of tables into
that address directly

Quoted 7 lines

This seems more complex than necessary to me.

The important task is to get the tables generated in QEMU - I'd focus on getting the tables generated in QEMU (one table per fw_cfg "file"). Once that is done, the SeaBIOS side can be easily implemented, and we can add any enhancements on top if we feel it is necessary.

+1. This "copy of tables into that address directly" is just an ad-hoc PV isa DMA device in disguise. Such device was refused when libguestfs asked for it, and they wanted it for much better reason - performance. There is existing mechanism to pass data into firmware. Use it please.

--
Gleb.

author
#12 lore raw
Quoted 49 lines · Gleb Natapov
Quoted 41 lines · Kevin O'Connor
Quoted 32 lines · Michael S. Tsirkin
Quoted 18 lines · Michael S. Tsirkin

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled

    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

Quoted 8 lines

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

After some thought, there are two additional
options worth considering, in that they simplify
bios code somewhat:

Quoted 7 lines

- bios could get size from qemu, allocate a buffer (e.g. could be one buffer for all tables) and pass the address to qemu.
qemu does all the patching

- further, qemu could do the copy of tables into
that address directly

Quoted 7 lines

This seems more complex than necessary to me.

The important task is to get the tables generated in QEMU - I'd focus on getting the tables generated in QEMU (one table per fw_cfg "file"). Once that is done, the SeaBIOS side can be easily implemented, and we can add any enhancements on top if we feel it is necessary.

Quoted 4 lines

+1. This "copy of tables into that address directly" is just an ad-hoc PV isa DMA device in disguise. Such device was refused when libguestfs asked for it, and they wanted it for much better reason - performance. There is existing mechanism to pass data into firmware. Use it please.

Yes I can code it up using FW_CFG for now.

One issue with QEMU_CFG_FILE_DIR is that it's broken wrt migration, unless we pass in very small bits of data which we
can guarantee never changes across qemu versions.

Off-list, I suggested fixing it and migrating file content, but Anthony thinks it's a bad idea.

I don't care much.

Quoted 2 lines

--
Gleb.

#13 lore raw
Quoted 55 lines · Michael S. Tsirkin
Quoted 49 lines · Gleb Natapov
Quoted 41 lines · Kevin O'Connor
Quoted 32 lines · Michael S. Tsirkin
Quoted 18 lines · Michael S. Tsirkin

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled

    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

Quoted 8 lines

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

After some thought, there are two additional
options worth considering, in that they simplify
bios code somewhat:

Quoted 7 lines

- bios could get size from qemu, allocate a buffer (e.g. could be one buffer for all tables) and pass the address to qemu.
qemu does all the patching

- further, qemu could do the copy of tables into
that address directly

Quoted 7 lines

This seems more complex than necessary to me.

The important task is to get the tables generated in QEMU - I'd focus on getting the tables generated in QEMU (one table per fw_cfg "file"). Once that is done, the SeaBIOS side can be easily implemented, and we can add any enhancements on top if we feel it is necessary.

Quoted 4 lines

+1. This "copy of tables into that address directly" is just an ad-hoc PV isa DMA device in disguise. Such device was refused when libguestfs asked for it, and they wanted it for much better reason - performance. There is existing mechanism to pass data into firmware. Use it please.

Quoted 6 lines

Yes I can code it up using FW_CFG for now.

One issue with QEMU_CFG_FILE_DIR is that it's broken wrt migration, unless we pass in very small bits of data which we
can guarantee never changes across qemu versions.

Shouldn't we guaranty that ACPI tables do not change for the same machine type anyway?

Quoted 3 lines

Off-list, I suggested fixing it and migrating file content, but Anthony thinks it's a bad idea.

Why is this a bad idea to fix device migration?

Quoted 4 lines

I don't care much.

Quoted 2 lines

--
Gleb.

--
Gleb.

author
#14 lore raw
Quoted 56 lines · Gleb Natapov
Quoted 55 lines · Michael S. Tsirkin
Quoted 53 lines · Gleb Natapov
Quoted 41 lines · Kevin O'Connor
Quoted 32 lines · Michael S. Tsirkin
Quoted 18 lines · Michael S. Tsirkin

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled

    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

Quoted 8 lines

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

After some thought, there are two additional
options worth considering, in that they simplify
bios code somewhat:

Quoted 7 lines

- bios could get size from qemu, allocate a buffer (e.g. could be one buffer for all tables) and pass the address to qemu.
qemu does all the patching

- further, qemu could do the copy of tables into
that address directly

Quoted 7 lines

This seems more complex than necessary to me.

The important task is to get the tables generated in QEMU - I'd focus on getting the tables generated in QEMU (one table per fw_cfg "file"). Once that is done, the SeaBIOS side can be easily implemented, and we can add any enhancements on top if we feel it is necessary.

+1. This "copy of tables into that address directly" is just an ad-hoc PV isa DMA device in disguise. Such device was refused when libguestfs asked for it, and they wanted it for much better reason - performance. There is existing mechanism to pass data into firmware. Use it please.

Quoted 8 lines
Quoted 6 lines

Yes I can code it up using FW_CFG for now.

One issue with QEMU_CFG_FILE_DIR is that it's broken wrt migration, unless we pass in very small bits of data which we
can guarantee never changes across qemu versions.

Shouldn't we guaranty that ACPI tables do not change for the same machine type anyway?

That's not practical. They are too big to stay completely unchanged.

Quoted 4 lines
Quoted 3 lines

Off-list, I suggested fixing it and migrating file content, but Anthony thinks it's a bad idea.

Why is this a bad idea to fix device migration?

You misunderstand I think.
Question is whether we should be putting so much info in fw_cfg. If we keep fw_cfg for small things we don't need to migrate it. In that case ACPI tables have to be passed in using some other mechanism.

Quoted 7 lines
Quoted 4 lines

I don't care much.

Quoted 2 lines

--
Gleb.

--
Gleb.

#15 lore raw
Quoted 59 lines · Michael S. Tsirkin
Quoted 58 lines · Gleb Natapov
Quoted 57 lines · Michael S. Tsirkin
Quoted 49 lines · Gleb Natapov
Quoted 41 lines · Kevin O'Connor
Quoted 32 lines · Michael S. Tsirkin
Quoted 18 lines · Michael S. Tsirkin

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled

    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

Quoted 8 lines

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

After some thought, there are two additional
options worth considering, in that they simplify
bios code somewhat:

Quoted 7 lines

- bios could get size from qemu, allocate a buffer (e.g. could be one buffer for all tables) and pass the address to qemu.
qemu does all the patching

- further, qemu could do the copy of tables into
that address directly

Quoted 7 lines

This seems more complex than necessary to me.

The important task is to get the tables generated in QEMU - I'd focus on getting the tables generated in QEMU (one table per fw_cfg "file"). Once that is done, the SeaBIOS side can be easily implemented, and we can add any enhancements on top if we feel it is necessary.

Quoted 4 lines

+1. This "copy of tables into that address directly" is just an ad-hoc PV isa DMA device in disguise. Such device was refused when libguestfs asked for it, and they wanted it for much better reason - performance. There is existing mechanism to pass data into firmware. Use it please.

Yes I can code it up using FW_CFG for now.

Quoted 9 lines
Quoted 6 lines
Quoted 4 lines

One issue with QEMU_CFG_FILE_DIR is that it's broken wrt migration, unless we pass in very small bits of data which we
can guarantee never changes across qemu versions.

Shouldn't we guaranty that ACPI tables do not change for the same machine type anyway?

That's not practical. They are too big to stay completely unchanged.

I will not be surprised if this will cause us problem somehow. Guest will see new tables only after reboot/resume from S4 so damage is limited, but one thing that comes to mind is table's size change. If they grow from one version to the other after resuming a guest from S4 on new QEMU version part of the tables may be corrupted.

Quoted 11 lines
Quoted 4 lines
Quoted 3 lines

Off-list, I suggested fixing it and migrating file content, but Anthony thinks it's a bad idea.

Why is this a bad idea to fix device migration?

You misunderstand I think.
Question is whether we should be putting so much info in fw_cfg. If we keep fw_cfg for small things we don't need to migrate it. In that case ACPI tables have to be passed in using some other mechanism.

Where this notion that fw_cfg is only for a small things is coming from? I can assure you this was not the case when the device was introduced. In fact it is used today for not so small things like bootindex splash screen bitmaps, option rom loading and kernel/initrd loading. Some of those are bigger then ACPI tables will ever be. And they all should be migrated, so fw_cfg should be fixed anyway.

--
Gleb.

author
#16 lore raw
Quoted 67 lines · Gleb Natapov
Quoted 66 lines · Michael S. Tsirkin
Quoted 64 lines · Gleb Natapov
Quoted 55 lines · Michael S. Tsirkin
Quoted 49 lines · Gleb Natapov
Quoted 41 lines · Kevin O'Connor
Quoted 32 lines · Michael S. Tsirkin
Quoted 18 lines · Michael S. Tsirkin

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled

    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

Quoted 8 lines

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

After some thought, there are two additional
options worth considering, in that they simplify
bios code somewhat:

Quoted 7 lines

- bios could get size from qemu, allocate a buffer (e.g. could be one buffer for all tables) and pass the address to qemu.
qemu does all the patching

- further, qemu could do the copy of tables into
that address directly

Quoted 7 lines

This seems more complex than necessary to me.

The important task is to get the tables generated in QEMU - I'd focus on getting the tables generated in QEMU (one table per fw_cfg "file"). Once that is done, the SeaBIOS side can be easily implemented, and we can add any enhancements on top if we feel it is necessary.

Quoted 4 lines

+1. This "copy of tables into that address directly" is just an ad-hoc PV isa DMA device in disguise. Such device was refused when libguestfs asked for it, and they wanted it for much better reason - performance. There is existing mechanism to pass data into firmware. Use it please.

Quoted 6 lines

Yes I can code it up using FW_CFG for now.

One issue with QEMU_CFG_FILE_DIR is that it's broken wrt migration, unless we pass in very small bits of data which we
can guarantee never changes across qemu versions.

Shouldn't we guaranty that ACPI tables do not change for the same machine type anyway?

Quoted 7 lines
Quoted 2 lines

That's not practical. They are too big to stay completely unchanged.

I will not be surprised if this will cause us problem somehow. Guest will see new tables only after reboot/resume from S4 so damage is limited, but one thing that comes to mind is table's size change. If they grow from one version to the other after resuming a guest from S4 on new QEMU version part of the tables may be corrupted.

Why would it be corrupted?

In any case, FACS has a hardware signature value for just such a case. If we know VM can not be resumed on new QEMU, we can change the signature and it will cold-boot instead.

Quoted 18 lines
Quoted 11 lines
Quoted 4 lines
Quoted 3 lines

Off-list, I suggested fixing it and migrating file content, but Anthony thinks it's a bad idea.

Why is this a bad idea to fix device migration?

You misunderstand I think.
Question is whether we should be putting so much info in fw_cfg. If we keep fw_cfg for small things we don't need to migrate it. In that case ACPI tables have to be passed in using some other mechanism.

Where this notion that fw_cfg is only for a small things is coming from? I can assure you this was not the case when the device was introduced. In fact it is used today for not so small things like bootindex splash screen bitmaps, option rom loading and kernel/initrd loading. Some of those are bigger then ACPI tables will ever be. And they all should be migrated, so fw_cfg should be fixed anyway.

Quoted 2 lines

--
Gleb.

I'm not arguing with that. Convince Anthony please.

Signature · 1 line
MST
#17 lore raw
Quoted 70 lines · Michael S. Tsirkin
Quoted 69 lines · Gleb Natapov
Quoted 68 lines · Michael S. Tsirkin
Quoted 56 lines · Gleb Natapov
Quoted 55 lines · Michael S. Tsirkin
Quoted 53 lines · Gleb Natapov
Quoted 41 lines · Kevin O'Connor
Quoted 32 lines · Michael S. Tsirkin
Quoted 18 lines · Michael S. Tsirkin

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled

    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

Quoted 8 lines

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

After some thought, there are two additional
options worth considering, in that they simplify
bios code somewhat:

Quoted 7 lines

- bios could get size from qemu, allocate a buffer (e.g. could be one buffer for all tables) and pass the address to qemu.
qemu does all the patching

- further, qemu could do the copy of tables into
that address directly

Quoted 7 lines

This seems more complex than necessary to me.

The important task is to get the tables generated in QEMU - I'd focus on getting the tables generated in QEMU (one table per fw_cfg "file"). Once that is done, the SeaBIOS side can be easily implemented, and we can add any enhancements on top if we feel it is necessary.

+1. This "copy of tables into that address directly" is just an ad-hoc PV isa DMA device in disguise. Such device was refused when libguestfs asked for it, and they wanted it for much better reason - performance. There is existing mechanism to pass data into firmware. Use it please.

Quoted 8 lines
Quoted 6 lines

Yes I can code it up using FW_CFG for now.

One issue with QEMU_CFG_FILE_DIR is that it's broken wrt migration, unless we pass in very small bits of data which we
can guarantee never changes across qemu versions.

Shouldn't we guaranty that ACPI tables do not change for the same machine type anyway?

That's not practical. They are too big to stay completely unchanged.

Quoted 8 lines
Quoted 5 lines

I will not be surprised if this will cause us problem somehow. Guest will see new tables only after reboot/resume from S4 so damage is limited, but one thing that comes to mind is table's size change. If they grow from one version to the other after resuming a guest from S4 on new QEMU version part of the tables may be corrupted.

Why would it be corrupted?

Because ACPI tables are stored in the memory marked as "ACPI data" (IIRC seabios mark them as reserved). Guests do not save reserved memory during S4 (don't know about "ACPI data", but if guest does not copy tables from it to another location I doubt it saves the memory, anyway ACPI spec does not mandate it), so what happens on resume if the memory grows? Part of it, that was not marked as reserved before S4, is re-written with whatever data guest had there and rest contains now corrupted ACPI tables that BIOS put there during boot. We can hope that guest is smart enough to see that memory map changed and refuse to resume or we can put ACPI tables into NVS memory which has to be saved and restored during S4 by OSPM.

Quoted 4 lines

In any case, FACS has a hardware signature value for just such a case. If we know VM can not be resumed on new QEMU, we can change the signature and it will cold-boot instead.

Nice, does Linux check it?

Quoted 18 lines
Quoted 18 lines
Quoted 11 lines
Quoted 4 lines
Quoted 3 lines

Off-list, I suggested fixing it and migrating file content, but Anthony thinks it's a bad idea.

Why is this a bad idea to fix device migration?

You misunderstand I think.
Question is whether we should be putting so much info in fw_cfg. If we keep fw_cfg for small things we don't need to migrate it. In that case ACPI tables have to be passed in using some other mechanism.

Where this notion that fw_cfg is only for a small things is coming from? I can assure you this was not the case when the device was introduced. In fact it is used today for not so small things like bootindex splash screen bitmaps, option rom loading and kernel/initrd loading. Some of those are bigger then ACPI tables will ever be. And they all should be migrated, so fw_cfg should be fixed anyway.

Quoted 5 lines
Quoted 2 lines

--
Gleb.

I'm not arguing with that. Convince Anthony please.

Convince him in what? That fw_cfg is broken vrt migration and there are cases that will fail _today_ without any ACPI related changes? This is knows for ages.

--
Gleb.

author
#18 lore raw
Quoted 79 lines · Gleb Natapov
Quoted 70 lines · Michael S. Tsirkin
Quoted 69 lines · Gleb Natapov
Quoted 68 lines · Michael S. Tsirkin
Quoted 56 lines · Gleb Natapov
Quoted 55 lines · Michael S. Tsirkin
Quoted 53 lines · Gleb Natapov
Quoted 41 lines · Kevin O'Connor
Quoted 32 lines · Michael S. Tsirkin
Quoted 18 lines · Michael S. Tsirkin

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled

    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

Quoted 8 lines

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

After some thought, there are two additional
options worth considering, in that they simplify
bios code somewhat:

Quoted 7 lines

- bios could get size from qemu, allocate a buffer (e.g. could be one buffer for all tables) and pass the address to qemu.
qemu does all the patching

- further, qemu could do the copy of tables into
that address directly

Quoted 7 lines

This seems more complex than necessary to me.

The important task is to get the tables generated in QEMU - I'd focus on getting the tables generated in QEMU (one table per fw_cfg "file"). Once that is done, the SeaBIOS side can be easily implemented, and we can add any enhancements on top if we feel it is necessary.

+1. This "copy of tables into that address directly" is just an ad-hoc PV isa DMA device in disguise. Such device was refused when libguestfs asked for it, and they wanted it for much better reason - performance. There is existing mechanism to pass data into firmware. Use it please.

Quoted 8 lines
Quoted 6 lines

Yes I can code it up using FW_CFG for now.

One issue with QEMU_CFG_FILE_DIR is that it's broken wrt migration, unless we pass in very small bits of data which we
can guarantee never changes across qemu versions.

Shouldn't we guaranty that ACPI tables do not change for the same machine type anyway?

That's not practical. They are too big to stay completely unchanged.

Quoted 8 lines
Quoted 5 lines

I will not be surprised if this will cause us problem somehow. Guest will see new tables only after reboot/resume from S4 so damage is limited, but one thing that comes to mind is table's size change. If they grow from one version to the other after resuming a guest from S4 on new QEMU version part of the tables may be corrupted.

Why would it be corrupted?

Quoted 11 lines

Because ACPI tables are stored in the memory marked as "ACPI data" (IIRC seabios mark them as reserved). Guests do not save reserved memory during S4 (don't know about "ACPI data", but if guest does not copy tables from it to another location I doubt it saves the memory, anyway ACPI spec does not mandate it), so what happens on resume if the memory grows? Part of it, that was not marked as reserved before S4, is re-written with whatever data guest had there and rest contains now corrupted ACPI tables that BIOS put there during boot. We can hope that guest is smart enough to see that memory map changed and refuse to resume or we can put ACPI tables into NVS memory which has to be saved and restored during S4 by OSPM.

This easily solvable by asking for memory with a solid margin.

But what you write above sounds strange: would not it apply to any memory allocated with malloc_high? If so, a minor change in BIOS e.g. where we allocate a bit more, less, or in a different order, will break suspend.

Quoted 5 lines
Quoted 4 lines

In any case, FACS has a hardware signature value for just such a case. If we know VM can not be resumed on new QEMU, we can change the signature and it will cold-boot instead.

Nice, does Linux check it?

If not we can fix it.

Quoted 21 lines
Quoted 21 lines
Quoted 20 lines
Quoted 11 lines
Quoted 4 lines
Quoted 3 lines

Off-list, I suggested fixing it and migrating file content, but Anthony thinks it's a bad idea.

Why is this a bad idea to fix device migration?

You misunderstand I think.
Question is whether we should be putting so much info in fw_cfg. If we keep fw_cfg for small things we don't need to migrate it. In that case ACPI tables have to be passed in using some other mechanism.

Where this notion that fw_cfg is only for a small things is coming from? I can assure you this was not the case when the device was introduced. In fact it is used today for not so small things like bootindex splash screen bitmaps, option rom loading and kernel/initrd loading. Some of those are bigger then ACPI tables will ever be. And they all should be migrated, so fw_cfg should be fixed anyway.

--
Gleb.

Quoted 8 lines
Quoted 2 lines

I'm not arguing with that. Convince Anthony please.

Convince him in what? That fw_cfg is broken vrt migration and there are cases that will fail _today_ without any ACPI related changes? This is knows for ages.

--
Gleb.

That we should use fw_cfg to load acpi tables.

Signature · 1 line
MST
#19 lore raw
Quoted 92 lines · Michael S. Tsirkin
Quoted 79 lines · Gleb Natapov
Quoted 70 lines · Michael S. Tsirkin
Quoted 69 lines · Gleb Natapov
Quoted 68 lines · Michael S. Tsirkin
Quoted 56 lines · Gleb Natapov
Quoted 55 lines · Michael S. Tsirkin
Quoted 53 lines · Gleb Natapov
Quoted 41 lines · Kevin O'Connor
Quoted 32 lines · Michael S. Tsirkin
Quoted 18 lines · Michael S. Tsirkin

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled

    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

Quoted 8 lines

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

After some thought, there are two additional
options worth considering, in that they simplify
bios code somewhat:

Quoted 7 lines

- bios could get size from qemu, allocate a buffer (e.g. could be one buffer for all tables) and pass the address to qemu.
qemu does all the patching

- further, qemu could do the copy of tables into
that address directly

Quoted 7 lines

This seems more complex than necessary to me.

The important task is to get the tables generated in QEMU - I'd focus on getting the tables generated in QEMU (one table per fw_cfg "file"). Once that is done, the SeaBIOS side can be easily implemented, and we can add any enhancements on top if we feel it is necessary.

+1. This "copy of tables into that address directly" is just an ad-hoc PV isa DMA device in disguise. Such device was refused when libguestfs asked for it, and they wanted it for much better reason - performance. There is existing mechanism to pass data into firmware. Use it please.

Quoted 8 lines
Quoted 6 lines

Yes I can code it up using FW_CFG for now.

One issue with QEMU_CFG_FILE_DIR is that it's broken wrt migration, unless we pass in very small bits of data which we
can guarantee never changes across qemu versions.

Shouldn't we guaranty that ACPI tables do not change for the same machine type anyway?

That's not practical. They are too big to stay completely unchanged.

Quoted 8 lines
Quoted 5 lines

I will not be surprised if this will cause us problem somehow. Guest will see new tables only after reboot/resume from S4 so damage is limited, but one thing that comes to mind is table's size change. If they grow from one version to the other after resuming a guest from S4 on new QEMU version part of the tables may be corrupted.

Why would it be corrupted?

Quoted 11 lines

Because ACPI tables are stored in the memory marked as "ACPI data" (IIRC seabios mark them as reserved). Guests do not save reserved memory during S4 (don't know about "ACPI data", but if guest does not copy tables from it to another location I doubt it saves the memory, anyway ACPI spec does not mandate it), so what happens on resume if the memory grows? Part of it, that was not marked as reserved before S4, is re-written with whatever data guest had there and rest contains now corrupted ACPI tables that BIOS put there during boot. We can hope that guest is smart enough to see that memory map changed and refuse to resume or we can put ACPI tables into NVS memory which has to be saved and restored during S4 by OSPM.

Quoted 8 lines

This easily solvable by asking for memory with a solid margin.

But what you write above sounds strange: would not it apply to any memory allocated with malloc_high? If so, a minor change in BIOS e.g. where we allocate a bit more, less, or in a different order, will break suspend.

Good point. I really do not know how S4 resume handles memory map changes. The only sane way that I see is to refuse resume if memory map changes.

Quoted 31 lines
Quoted 5 lines
Quoted 4 lines

In any case, FACS has a hardware signature value for just such a case. If we know VM can not be resumed on new QEMU, we can change the signature and it will cold-boot instead.

Nice, does Linux check it?

If not we can fix it.

Quoted 23 lines
Quoted 23 lines
Quoted 18 lines
Quoted 11 lines
Quoted 4 lines
Quoted 3 lines

Off-list, I suggested fixing it and migrating file content, but Anthony thinks it's a bad idea.

Why is this a bad idea to fix device migration?

You misunderstand I think.
Question is whether we should be putting so much info in fw_cfg. If we keep fw_cfg for small things we don't need to migrate it. In that case ACPI tables have to be passed in using some other mechanism.

Where this notion that fw_cfg is only for a small things is coming from? I can assure you this was not the case when the device was introduced. In fact it is used today for not so small things like bootindex splash screen bitmaps, option rom loading and kernel/initrd loading. Some of those are bigger then ACPI tables will ever be. And they all should be migrated, so fw_cfg should be fixed anyway.

Quoted 2 lines

--
Gleb.

I'm not arguing with that. Convince Anthony please.

Quoted 9 lines
Quoted 6 lines

Convince him in what? That fw_cfg is broken vrt migration and there are cases that will fail _today_ without any ACPI related changes? This is knows for ages.

--
Gleb.

That we should use fw_cfg to load acpi tables.

I haven't seen his arguments against it.

--
Gleb.

author
#22 lore raw
In reply to Kevin O'Connor
Quoted 41 lines · Kevin O'Connor
Quoted 32 lines · Michael S. Tsirkin
Quoted 18 lines · Michael S. Tsirkin

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled

    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

Quoted 8 lines

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

After some thought, there are two additional
options worth considering, in that they simplify
bios code somewhat:

Quoted 7 lines

- bios could get size from qemu, allocate a buffer (e.g. could be one buffer for all tables) and pass the address to qemu.
qemu does all the patching

- further, qemu could do the copy of tables into
that address directly

Quoted 8 lines

This seems more complex than necessary to me.

The important task is to get the tables generated in QEMU - I'd focus on getting the tables generated in QEMU (one table per fw_cfg "file"). Once that is done, the SeaBIOS side can be easily implemented, and we can add any enhancements on top if we feel it is necessary.

-Kevin

I have kind of done this, though only compile-tested for now - still need to update the bios with the new linker interface along the lines suggested by you.

If you want to see how the code looks like check out

git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git acpi the code is in
hw/i386/acpi-build.c
and
hw/i386/bios-linker-loader.c

the history is all messed up now, I'll clean it up shortly.

That said, this uses fw_cfg so for this to be acceptable, we need to fix migration with big fw_cfg files.

Signature · 1 line
MST
#20 lore raw
In reply to Michael S. Tsirkin
Quoted 10 lines · Michael S. Tsirkin
Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Michael S. Tsirkin
Quoted 6 lines · Gleb Natapov

Where this notion that fw_cfg is only for a small things is coming from? I can assure you this was not the case when the device was introduced. In fact it is used today for not so small things like bootindex splash screen bitmaps, option rom loading and kernel/initrd loading. Some of those are bigger then ACPI tables will ever be. And they all should be migrated, so fw_cfg should be fixed anyway.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

I'm not arguing with that. Convince Anthony please.

Convince him in what? That fw_cfg is broken vrt migration and there are cases that will fail _today_ without any ACPI related changes? This is knows for ages.

That we should use fw_cfg to load acpi tables.

I'm confused.

ACPI tables are not large. At most we're talking about 100K of data total.

I don't see what migration has to do with using fw_cfg to pass acpi tables - the content is only read at startup. There may be an issue for the corner case of VM restarts, but if so it's nothing new. If the content of a fw_cfg entry changes during a guest reboot it is going to have the same impact regardless of whether it's the "irq0-override" entry / "numa-nodes" entry - or if it's the "madt" entry / "srat" entry, etc. So, I don't see how fw_cfg would suddenly not be suitable.

Again, I recommend that ACPI (and mptable, smbios, pir) be generated in qemu and that the content be passed to SeaBIOS using one fw_cfg "file" per table.

-Kevin

#23 lore raw
In reply to Michael S. Tsirkin
Quoted 63 lines · Michael S. Tsirkin
Quoted 41 lines · Kevin O'Connor
Quoted 32 lines · Michael S. Tsirkin
Quoted 18 lines · Michael S. Tsirkin

Untested yet, but I thought I'd share the
BIOS bits so we can agree on direction.

In particular check out ROM sizes:
- Before patchset with DSDT enabled

    Total size: 127880  Fixed: 59060  Free: 3192 (used 97.6% of 128KiB rom)
- Before patchset with DSDT disabled
    Total size: 122844  Fixed: 58884  Free: 8228 (used 93.7% of 128KiB rom)
- After patchset:
    Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
- Legacy disabled at build time:
    Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

As can be seen from this, most size savings come from dropping DSDT, but we do save a bit by removing other tables. Of course the real reason to move tables to QEMU is so that ACPI can better match hardware.

Quoted 8 lines

This patchset adds an option to move all code for formatting acpi tables out of BIOS. With this, QEMU has full control over the table layout. All tables are loaded from the new "/etc/acpi/" directory. Any entries in this directory cause BIOS to disable
ACPI table generation completely.
A generic linker script, controlled by QEMU, is
loaded from "/etc/linker-script". It is used to
patch in table pointers and checksums.

After some thought, there are two additional
options worth considering, in that they simplify
bios code somewhat:

Quoted 7 lines

- bios could get size from qemu, allocate a buffer (e.g. could be one buffer for all tables) and pass the address to qemu.
qemu does all the patching

- further, qemu could do the copy of tables into
that address directly

Quoted 8 lines

This seems more complex than necessary to me.

The important task is to get the tables generated in QEMU - I'd focus on getting the tables generated in QEMU (one table per fw_cfg "file"). Once that is done, the SeaBIOS side can be easily implemented, and we can add any enhancements on top if we feel it is necessary.

-Kevin

I have kind of done this, though only compile-tested for now - still need to update the bios with the new linker interface along the lines suggested by you.

If you want to see how the code looks like check out

git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git acpi the code is in
hw/i386/acpi-build.c
and
hw/i386/bios-linker-loader.c

Quoted 5 lines

the history is all messed up now, I'll clean it up shortly.

That said, this uses fw_cfg so for this to be acceptable, we need to fix migration with big fw_cfg files.

We need to fix it anyway ;)

--
Gleb.

#21 lore raw
In reply to Kevin O'Connor
Quoted 24 lines · Kevin O'Connor
Quoted 10 lines · Michael S. Tsirkin
Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Michael S. Tsirkin
Quoted 6 lines · Gleb Natapov

Where this notion that fw_cfg is only for a small things is coming from? I can assure you this was not the case when the device was introduced. In fact it is used today for not so small things like bootindex splash screen bitmaps, option rom loading and kernel/initrd loading. Some of those are bigger then ACPI tables will ever be. And they all should be migrated, so fw_cfg should be fixed anyway.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

I'm not arguing with that. Convince Anthony please.

Convince him in what? That fw_cfg is broken vrt migration and there are cases that will fail _today_ without any ACPI related changes? This is knows for ages.

That we should use fw_cfg to load acpi tables.

I'm confused.

ACPI tables are not large. At most we're talking about 100K of data total.

Quoted 9 lines

I don't see what migration has to do with using fw_cfg to pass acpi tables - the content is only read at startup. There may be an issue for the corner case of VM restarts, but if so it's nothing new. If the content of a fw_cfg entry changes during a guest reboot it is going to have the same impact regardless of whether it's the "irq0-override" entry / "numa-nodes" entry - or if it's the "madt" entry / "srat" entry, etc. So, I don't see how fw_cfg would suddenly not be suitable.

If the tables at the source of the migration and the destination differs and migration happens while seabios is reading them guest will have corrupted ACPI tables at the destination. The problem is not new. The same is true for reading option rom or splash screen or bootindex file, basically anything that we read via fw_cfg interface and it can be different between two qemu version. The window where the bug may happen is very small, so we never saw such problem in practice to my knowledge and the fix should be simple too: migrate fw_cfg that is been read during migration.

Quoted 4 lines

Again, I recommend that ACPI (and mptable, smbios, pir) be generated in qemu and that the content be passed to SeaBIOS using one fw_cfg "file" per table.

+1 again.

--
Gleb.