[Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[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

[Qemu-devel] [PATCH RFC 1/3] linker: utility to patch in-memory ROM files

[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 <mst@redhat.com>
---
 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

diff --git a/Makefile b/Makefile
index 759bbbb..020fb2f 100644
--- a/Makefile
+++ b/Makefile
@@ -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
diff --git a/src/linker.c b/src/linker.c
new file mode 100644
index 0000000..223d2db
--- /dev/null
+++ b/src/linker.c
@@ -0,0 +1,90 @@
+#include "linker.h"
+#include "byteorder.h" // le64_to_cpu
+
+void linker_link(const char *name)
+{
+    struct linker_entry_s *entry;
+    int offset = 0;
+    int size, lsrc;
+    void *data = romfile_loadfile(name, &size);
+    struct romfile_s *src, *dst;
+    u32 dst_offset;
+    u64 val, buf;
+    if (!data)
+        return;
+
+    for (offset = 0; offset < size; offset += entry->size) {
+        entry = data + offset;
+        /* Entry must have some data. If not - skip it. */
+        if (entry->size <= sizeof *entry)
+            continue;
+        /* Check that entry fits in buffer. */
+        if (offset + entry->size > size) {
+            warn_internalerror();
+            break;
+        }
+        /* Skip any command that we don't recognize. */
+        if (entry->reserved1 || entry->reserved2)
+            continue;
+        if (entry->bytes != 1 &&
+            entry->bytes != 2 &&
+            entry->bytes != 4 &&
+            entry->bytes != 8)
+            continue;
+        if (entry->shift > 63)
+            continue;
+        if (entry->type >= LINKER_ENTRY_TYPE_MAX)
+            continue;
+        if (entry->format != LINKER_ENTRY_FORMAT_LE)
+            continue;
+        /* Last byte must be 0 */
+        if (entry->src_dst[entry->size - 1]) {
+            warn_internalerror();
+            continue;
+        }
+        lsrc = strlen(entry->src_dst);
+        if (!lsrc) {
+            warn_internalerror();
+            continue;
+        }
+        src = romfile_find(entry->src_dst);
+        if (!src) {
+            warn_internalerror();
+            continue;
+        }
+        if (!src->data) {
+            warn_internalerror();
+            continue;
+        }
+        if (lsrc + 1 +  sizeof *entry >= entry->size) {
+            warn_internalerror();
+            continue;
+        }
+        dst = romfile_find(entry->src_dst + lsrc + 1);
+        if (!dst) {
+            warn_internalerror();
+            continue;
+        }
+        if (!dst->data) {
+            warn_internalerror();
+            continue;
+        }
+        dst_offset = le32_to_cpu(entry->dst_offset);
+        if (dst->size < dst_offset + entry->bytes) {
+            warn_internalerror();
+            continue;
+        }
+        buf = 0;
+        memcpy(&buf, dst->data + dst_offset, entry->bytes);
+        val = ((u64)(unsigned long)src->data) >> entry->shift;
+        buf = le64_to_cpu(buf);
+        if (entry->type == LINKER_ENTRY_TYPE_ADD)
+            buf += val;
+        else
+            buf -= val;
+        buf = cpu_to_le64(val);
+        memcpy(dst->data + dst_offset, &buf, entry->bytes);
+    }
+
+    free(data);
+}
diff --git a/src/linker.h b/src/linker.h
new file mode 100644
index 0000000..d8f4a79
--- /dev/null
+++ b/src/linker.h
@@ -0,0 +1,50 @@
+#ifndef __LINKER_H
+#define __LINKER_H
+
+#include "types.h" // u8
+#include "util.h" // romfile_s
+
+/* ROM file linker interface. Linker uses little endian format */
+struct linker_entry_s {
+    u8 size; /* size in bytes including the header */
+    u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */
+    u8 shift; /* Shift source address right by this many bits. 0-63. */
+    u8 type;
+    u8 format;
+    u8 reserved1;
+    u16 reserved2;
+    u64 dst_offset; /* Offset in destination. Little endian format. */
+    /* Followed by source and destination, optionally padded by
+     * 0, up to the total of entry_len - 4 bytes.
+     * Strings are 0-terminated. */
+    char src_dst[];
+} PACKED;
+
+/* Note: align types must appear before all other types. */
+enum linker_entry_type {
+    /*
+     * increment value in DST by the address of source
+     * useful e.g. to fill in pointer values in ACPI tables
+     */
+
+    LINKER_ENTRY_TYPE_ADD = 0x0,
+    /*
+     * Decrement value in DST by the address of source
+     * useful e.g. to fix up checksum values in ACPI tables
+     */
+    LINKER_ENTRY_TYPE_SUB = 0x1,
+    /*
+     * The following move source so must come first,
+     * before sorce is used by other operations.
+     */
+    /* Last entry. Not a valid type. */
+    LINKER_ENTRY_TYPE_MAX,
+};
+
+enum linker_entry_format {
+    LINKER_ENTRY_FORMAT_LE = 0x0,
+};
+
+void linker_link(const char *name);
+
+#endif
diff --git a/src/util.h b/src/util.h
index 996c29a..7b50c38 100644
--- a/src/util.h
+++ b/src/util.h
@@ -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);
-- 
MST

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

[Michael S. Tsirkin]

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 <mst@redhat.com>
---
 src/acpi.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

diff --git a/src/acpi.c b/src/acpi.c
index 58cd6d7..16ea9f4 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -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)
-- 
MST

[Qemu-devel] [PATCH RFC 3/3] acpi: add an option to disable builtin tables

[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 <mst@redhat.com>
---
 src/Kconfig | 12 +++++++++++-
 src/acpi.c  |  2 +-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/Kconfig b/src/Kconfig
index 3c80132..1b54b83 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -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.
diff --git a/src/acpi.c b/src/acpi.c
index 16ea9f4..b03b2ba 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -664,7 +664,7 @@ acpi_setup(void)
 
     linker_link("/etc/linker-script");
 
-    if (!acpi_generate) {
+    if (!CONFIG_ACPI_BUILTIN || !acpi_generate) {
         return;
     }
 
-- 
MST

Re: [Qemu-devel] [SeaBIOS] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Fred .]

[-- Attachment #1: Type: text/plain, Size: 2413 bytes --]

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?


On Thu, Apr 25, 2013 at 11:02 AM, Michael S. Tsirkin <mst@redhat.com> wrote:

> 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
> SeaBIOS@seabios.org
> http://www.seabios.org/mailman/listinfo/seabios
>

[-- Attachment #2: Type: text/html, Size: 3100 bytes --]

Re: [Qemu-devel] [SeaBIOS] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Michael S. Tsirkin]

On Thu, Apr 25, 2013 at 08:19:48PM +0200, Fred . wrote:
> 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.

> 
> On Thu, Apr 25, 2013 at 11:02 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     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
>     SeaBIOS@seabios.org
>     http://www.seabios.org/mailman/listinfo/seabios
> 
> 

Re: [Qemu-devel] [SeaBIOS] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Kevin O'Connor]

On Fri, Apr 26, 2013 at 12:11:24AM +0300, Michael S. Tsirkin wrote:
> On Thu, Apr 25, 2013 at 08:19:48PM +0200, Fred . wrote:
> > 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.

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

-Kevin

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

[Laszlo Ersek]

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

One note below:

> @@ -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;
> +

[...]

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

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

[Michael S. Tsirkin]

On Mon, Apr 29, 2013 at 01:41:01PM +0200, Laszlo Ersek wrote:
> Not sure how much it counts, but I personally can agree with you on this
> direction :)
> 
> One note below:
> 
> > @@ -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;
> > +
> 
> [...]
> 
> > +    }
> > +
> > +    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

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.

-- 
MST

Re: [Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Michael S. Tsirkin]

On Thu, Apr 25, 2013 at 12:02:20PM +0300, Michael S. Tsirkin wrote:
> 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.

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?



> 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

Re: [Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Kevin O'Connor]

On Tue, May 07, 2013 at 09:00:48PM +0300, Michael S. Tsirkin wrote:
> On Thu, Apr 25, 2013 at 12:02:20PM +0300, Michael S. Tsirkin wrote:
> > 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.
> 
> 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

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

Re: [Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Gleb Natapov]

On Tue, May 07, 2013 at 07:01:13PM -0400, Kevin O'Connor wrote:
> On Tue, May 07, 2013 at 09:00:48PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Apr 25, 2013 at 12:02:20PM +0300, Michael S. Tsirkin wrote:
> > > 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.
> > 
> > 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
> 
> 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.

Re: [Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Michael S. Tsirkin]

On Wed, May 08, 2013 at 12:31:50PM +0300, Gleb Natapov wrote:
> On Tue, May 07, 2013 at 07:01:13PM -0400, Kevin O'Connor wrote:
> > On Tue, May 07, 2013 at 09:00:48PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Apr 25, 2013 at 12:02:20PM +0300, Michael S. Tsirkin wrote:
> > > > 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.
> > > 
> > > 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
> > 
> > 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.

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.

> --
> 			Gleb.

Re: [Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Gleb Natapov]

On Wed, May 08, 2013 at 01:29:12PM +0300, Michael S. Tsirkin wrote:
> On Wed, May 08, 2013 at 12:31:50PM +0300, Gleb Natapov wrote:
> > On Tue, May 07, 2013 at 07:01:13PM -0400, Kevin O'Connor wrote:
> > > On Tue, May 07, 2013 at 09:00:48PM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Apr 25, 2013 at 12:02:20PM +0300, Michael S. Tsirkin wrote:
> > > > > 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.
> > > > 
> > > > 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
> > > 
> > > 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.
> 
> 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?

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

> I don't care much.
> 
> > --
> > 			Gleb.

--
			Gleb.

Re: [Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Michael S. Tsirkin]

On Wed, May 08, 2013 at 01:34:59PM +0300, Gleb Natapov wrote:
> On Wed, May 08, 2013 at 01:29:12PM +0300, Michael S. Tsirkin wrote:
> > On Wed, May 08, 2013 at 12:31:50PM +0300, Gleb Natapov wrote:
> > > On Tue, May 07, 2013 at 07:01:13PM -0400, Kevin O'Connor wrote:
> > > > On Tue, May 07, 2013 at 09:00:48PM +0300, Michael S. Tsirkin wrote:
> > > > > On Thu, Apr 25, 2013 at 12:02:20PM +0300, Michael S. Tsirkin wrote:
> > > > > > 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.
> > > > > 
> > > > > 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
> > > > 
> > > > 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.
> > 
> > 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.

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

> > I don't care much.
> > 
> > > --
> > > 			Gleb.
> 
> --
> 			Gleb.

Re: [Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Gleb Natapov]

On Wed, May 08, 2013 at 01:43:25PM +0300, Michael S. Tsirkin wrote:
> On Wed, May 08, 2013 at 01:34:59PM +0300, Gleb Natapov wrote:
> > On Wed, May 08, 2013 at 01:29:12PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, May 08, 2013 at 12:31:50PM +0300, Gleb Natapov wrote:
> > > > On Tue, May 07, 2013 at 07:01:13PM -0400, Kevin O'Connor wrote:
> > > > > On Tue, May 07, 2013 at 09:00:48PM +0300, Michael S. Tsirkin wrote:
> > > > > > On Thu, Apr 25, 2013 at 12:02:20PM +0300, Michael S. Tsirkin wrote:
> > > > > > > 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.
> > > > > > 
> > > > > > 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
> > > > > 
> > > > > 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.
> > > 
> > > 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.
> 
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.

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

Re: [Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Michael S. Tsirkin]

On Wed, May 08, 2013 at 01:59:12PM +0300, Gleb Natapov wrote:
> On Wed, May 08, 2013 at 01:43:25PM +0300, Michael S. Tsirkin wrote:
> > On Wed, May 08, 2013 at 01:34:59PM +0300, Gleb Natapov wrote:
> > > On Wed, May 08, 2013 at 01:29:12PM +0300, Michael S. Tsirkin wrote:
> > > > On Wed, May 08, 2013 at 12:31:50PM +0300, Gleb Natapov wrote:
> > > > > On Tue, May 07, 2013 at 07:01:13PM -0400, Kevin O'Connor wrote:
> > > > > > On Tue, May 07, 2013 at 09:00:48PM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Thu, Apr 25, 2013 at 12:02:20PM +0300, Michael S. Tsirkin wrote:
> > > > > > > > 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.
> > > > > > > 
> > > > > > > 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
> > > > > > 
> > > > > > 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.
> > > > 
> > > > 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.
> > 
> 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.

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

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

-- 
MST

Re: [Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Gleb Natapov]

On Wed, May 08, 2013 at 02:07:24PM +0300, Michael S. Tsirkin wrote:
> On Wed, May 08, 2013 at 01:59:12PM +0300, Gleb Natapov wrote:
> > On Wed, May 08, 2013 at 01:43:25PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, May 08, 2013 at 01:34:59PM +0300, Gleb Natapov wrote:
> > > > On Wed, May 08, 2013 at 01:29:12PM +0300, Michael S. Tsirkin wrote:
> > > > > On Wed, May 08, 2013 at 12:31:50PM +0300, Gleb Natapov wrote:
> > > > > > On Tue, May 07, 2013 at 07:01:13PM -0400, Kevin O'Connor wrote:
> > > > > > > On Tue, May 07, 2013 at 09:00:48PM +0300, Michael S. Tsirkin wrote:
> > > > > > > > On Thu, Apr 25, 2013 at 12:02:20PM +0300, Michael S. Tsirkin wrote:
> > > > > > > > > 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.
> > > > > > > > 
> > > > > > > > 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
> > > > > > > 
> > > > > > > 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.
> > > > > 
> > > > > 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.
> > > 
> > 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.

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

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

Re: [Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Michael S. Tsirkin]

On Wed, May 08, 2013 at 02:35:44PM +0300, Gleb Natapov wrote:
> On Wed, May 08, 2013 at 02:07:24PM +0300, Michael S. Tsirkin wrote:
> > On Wed, May 08, 2013 at 01:59:12PM +0300, Gleb Natapov wrote:
> > > On Wed, May 08, 2013 at 01:43:25PM +0300, Michael S. Tsirkin wrote:
> > > > On Wed, May 08, 2013 at 01:34:59PM +0300, Gleb Natapov wrote:
> > > > > On Wed, May 08, 2013 at 01:29:12PM +0300, Michael S. Tsirkin wrote:
> > > > > > On Wed, May 08, 2013 at 12:31:50PM +0300, Gleb Natapov wrote:
> > > > > > > On Tue, May 07, 2013 at 07:01:13PM -0400, Kevin O'Connor wrote:
> > > > > > > > On Tue, May 07, 2013 at 09:00:48PM +0300, Michael S. Tsirkin wrote:
> > > > > > > > > On Thu, Apr 25, 2013 at 12:02:20PM +0300, Michael S. Tsirkin wrote:
> > > > > > > > > > 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.
> > > > > > > > > 
> > > > > > > > > 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
> > > > > > > > 
> > > > > > > > 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.
> > > > > > 
> > > > > > 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.
> > > > 
> > > 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.

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.

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

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

-- 
MST

Re: [Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Gleb Natapov]

On Wed, May 08, 2013 at 03:35:46PM +0300, Michael S. Tsirkin wrote:
> On Wed, May 08, 2013 at 02:35:44PM +0300, Gleb Natapov wrote:
> > On Wed, May 08, 2013 at 02:07:24PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, May 08, 2013 at 01:59:12PM +0300, Gleb Natapov wrote:
> > > > On Wed, May 08, 2013 at 01:43:25PM +0300, Michael S. Tsirkin wrote:
> > > > > On Wed, May 08, 2013 at 01:34:59PM +0300, Gleb Natapov wrote:
> > > > > > On Wed, May 08, 2013 at 01:29:12PM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Wed, May 08, 2013 at 12:31:50PM +0300, Gleb Natapov wrote:
> > > > > > > > On Tue, May 07, 2013 at 07:01:13PM -0400, Kevin O'Connor wrote:
> > > > > > > > > On Tue, May 07, 2013 at 09:00:48PM +0300, Michael S. Tsirkin wrote:
> > > > > > > > > > On Thu, Apr 25, 2013 at 12:02:20PM +0300, Michael S. Tsirkin wrote:
> > > > > > > > > > > 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.
> > > > > > > > > > 
> > > > > > > > > > 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
> > > > > > > > > 
> > > > > > > > > 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.
> > > > > > > 
> > > > > > > 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.
> > > > > 
> > > > 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.
> 
> 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.

> > > 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.
> 
> > > > > > > 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.
> > > 
> > > 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.
> 
I haven't seen his arguments against it.

--
			Gleb.

Re: [Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Michael S. Tsirkin]

On Tue, May 07, 2013 at 07:01:13PM -0400, Kevin O'Connor wrote:
> On Tue, May 07, 2013 at 09:00:48PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Apr 25, 2013 at 12:02:20PM +0300, Michael S. Tsirkin wrote:
> > > 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.
> > 
> > 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
> 
> 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.

-- 
MST

Re: [Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Kevin O'Connor]

On Wed, May 08, 2013 at 03:35:46PM +0300, Michael S. Tsirkin wrote:
> On Wed, May 08, 2013 at 02:35:44PM +0300, Gleb Natapov wrote:
> > On Wed, May 08, 2013 at 02:07:24PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, May 08, 2013 at 01:59:12PM +0300, Gleb Natapov wrote:
> > > > 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.
> > > 
> > > 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

Re: [Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Gleb Natapov]

On Wed, May 08, 2013 at 09:15:44PM +0300, Michael S. Tsirkin wrote:
> On Tue, May 07, 2013 at 07:01:13PM -0400, Kevin O'Connor wrote:
> > On Tue, May 07, 2013 at 09:00:48PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Apr 25, 2013 at 12:02:20PM +0300, Michael S. Tsirkin wrote:
> > > > 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.
> > > 
> > > 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
> > 
> > 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.
> 
We need to fix it anyway ;)

--
			Gleb.

Re: [Qemu-devel] [PATCH RFC 0/3] seabios: move acpi table formatting out of bios

[Gleb Natapov]

On Wed, May 08, 2013 at 06:55:22PM -0400, Kevin O'Connor wrote:
> On Wed, May 08, 2013 at 03:35:46PM +0300, Michael S. Tsirkin wrote:
> > On Wed, May 08, 2013 at 02:35:44PM +0300, Gleb Natapov wrote:
> > > On Wed, May 08, 2013 at 02:07:24PM +0300, Michael S. Tsirkin wrote:
> > > > On Wed, May 08, 2013 at 01:59:12PM +0300, Gleb Natapov wrote:
> > > > > 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.
> > > > 
> > > > 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.
> 
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.

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