[Qemu-devel] [PATCH 00/11] build ACPI MADT for fw_cfg clients

[Qemu-devel] [PATCH 00/11] build ACPI MADT for fw_cfg clients

[Laszlo Ersek]

This series reworks the internals of the -acpitable command line option,
and if that option is not specified, produces the APIC (MADT) table
inside qemu, to be consumed over fw_cfg (alongside the DSDT).

checkpatch.pl raises quite a few complaints on the start-to-end diff of
this series, but all of them are clearly wrong.

I've just posted the seabios patches too
<http://www.seabios.org/pipermail/seabios/2013-March/005960.html>; there
I mentioned the following testing:

  I cross-tested {patched, unpatched} qemu with {patched, unpatched}
  seabios. Inside a RHEL-6 guest I dumped the RSDT, DSDT, and APIC
  (MADT) in all four cases, and compared the results. Patched qemu with
  unpatched seabios don't play nicely together (two APIC (MADT) tables
  present). Otherwise everything seemed fine to me.

  The CPU topology was set with

    -smp 3,maxcpus=16,sockets=2,cores=4,threads=2

  The table dumps / disassemblies are too big to include here, please
  find them at
  <http://people.redhat.com/~lersek/acpi_move/madt_tests.tar.gz>.

I also tested the following failure/warning modes from the qemu command
line:

(01) qemu-system-x86_64 -acpitable

     qemu-system-x86_64: -acpitable: requires an argument

(02) qemu-system-x86_64 -acpitable ''

     Wrong acpi table provided: '-acpitable' requires one of 'data' or
       'file'

(03) qemu-system-x86_64 -acpitable data=p,file=q

     Wrong acpi table provided: '-acpitable' requires one of 'data' or
       'file'

(04) qemu-system-x86_64 -acpitable data=''

     Wrong acpi table provided: '-acpitable' requires at least one
       pathname

(05) qemu-system-x86_64 -acpitable q

     Wrong acpi table provided: can't open file q: No such file or
       directory

(06) touch q
     qemu-system-x86_64 -acpitable q

     warning: ACPI table: no headers are specified [starts up]

(07) qemu-system-x86_64 -acpitable file=q

     Wrong acpi table provided: ACPI table claiming to have header is
       too short, available: 0, expected: 36

(08) head -c 36 /dev/zero >q
     qemu-system-x86_64 -acpitable file=q

     warning: ACPI table has wrong length, header says 0, actual size 36
       bytes [starts up]

(09) head -c 100000 /dev/zero >q
     qemu-system-x86_64 -acpitable file=q

     Wrong acpi table provided: ACPI table too big, requested: 100000,
       max: 65535

(10) qemu-system-x86_64 -acpitable data=q

     Wrong acpi table provided: ACPI table too big, requested: 100036,
       max: 65535

(11) # renamed "acpi-dsdt.aml" to something else
     qemu-system-x86_64

     WARNING: failed to find acpi-dsdt.aml [starts up]

(12) # recreated "acpi-dsdt.aml" with "hello, world" contents
     qemu-system-x86_64

     WARNING: failed to load
       /opt/qemu-upstream/share/qemu/acpi-dsdt.aml: ACPI table claiming
       to have header is too short, available: 13, expected: 36 [starts
       up]


Laszlo Ersek (11):
  strip some whitespace
  change element type from "char" to "unsigned char" in ACPI table data
  acpi_table_add(): report fatal errors through an internal Error
    object
  qapi schema: add AcpiTableOptions
  acpi_table_add(): accept QemuOpts and parse it with OptsVisitor
  acpi_table_add(): extract and reimplement internals
  like acpi_table_install(), acpi_table_add() should propagate Errors
  extract/unify the constant 0xfee00000 as APIC_DEFAULT_ADDRESS
  Introduce IO_APIC_DEFAULT_ADDRESS for 0xfec00000
  pc_acpi_init(): don't bail as soon as failing to find default DSDT
  i386/pc: build ACPI MADT for fw_cfg clients

 qapi-schema.json           |   58 +++++++
 hw/pc.h                    |    7 +-
 include/sysemu/arch_init.h |    3 +-
 target-i386/cpu.h          |    4 +-
 arch_init.c                |   13 +-
 hw/acpi.c                  |  357 ++++++++++++++++++++++++++------------------
 hw/apic_common.c           |    2 +-
 hw/i386/kvmvapic.c         |    2 -
 hw/i386/pc.c               |  160 +++++++++++++++++++-
 target-i386/cpu.c          |    4 +-
 vl.c                       |   12 +-
 11 files changed, 449 insertions(+), 173 deletions(-)

[Qemu-devel] [PATCH 01/11] strip some whitespace

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 target-i386/cpu.h |    2 +-
 arch_init.c       |    2 +-
 vl.c              |    8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 48f41ca..5284ebc 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -185,7 +185,7 @@
 #define HF2_VINTR_SHIFT      3 /* value of V_INTR_MASKING bit */
 
 #define HF2_GIF_MASK          (1 << HF2_GIF_SHIFT)
-#define HF2_HIF_MASK          (1 << HF2_HIF_SHIFT) 
+#define HF2_HIF_MASK          (1 << HF2_HIF_SHIFT)
 #define HF2_NMI_MASK          (1 << HF2_NMI_SHIFT)
 #define HF2_VINTR_MASK        (1 << HF2_VINTR_SHIFT)
 
diff --git a/arch_init.c b/arch_init.c
index 98e2bc6..761051b 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -160,7 +160,7 @@ int qemu_read_default_config_files(bool userconfig)
             return ret;
         }
     }
-    
+
     return 0;
 }
 
diff --git a/vl.c b/vl.c
index ce51e65..f2bd893 100644
--- a/vl.c
+++ b/vl.c
@@ -2530,7 +2530,7 @@ static int virtcon_parse(const char *devname)
         qemu_opt_set(bus_opts, "driver", "virtio-serial-s390");
     } else {
         qemu_opt_set(bus_opts, "driver", "virtio-serial-pci");
-    } 
+    }
 
     dev_opts = qemu_opts_create_nofail(device);
     qemu_opt_set(dev_opts, "driver", "virtconsole");
@@ -2582,7 +2582,7 @@ static int sclp_parse(const char *devname)
 }
 
 static int debugcon_parse(const char *devname)
-{   
+{
     QemuOpts *opts;
 
     if (!qemu_chr_new("debugcon", devname, NULL)) {
@@ -3704,8 +3704,8 @@ int main(int argc, char **argv, char **envp)
 			}
 			p += 8;
 			os_set_proc_name(p);
-		     }	
-		 }	
+		     }
+		 }
                 break;
             case QEMU_OPTION_prom_env:
                 if (nb_prom_envs >= MAX_PROM_ENVS) {
-- 
1.7.1

[Qemu-devel] [PATCH 02/11] change element type from "char" to "unsigned char" in ACPI table data

[Laszlo Ersek]

The data is binary, not textual.

Also, acpi_table_add() abuses the "char *f" pointer -- which normally
points to file names to load -- to poke into the table. Introduce "char
unsigned *table_start" for that purpose.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/pc.h   |    2 +-
 hw/acpi.c |   17 +++++++++--------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/pc.h b/hw/pc.h
index dbbd8cd..fe1768b 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -109,7 +109,7 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
 
 /* acpi.c */
 extern int acpi_enabled;
-extern char *acpi_tables;
+extern char unsigned *acpi_tables;
 extern size_t acpi_tables_len;
 
 void acpi_bios_init(void);
diff --git a/hw/acpi.c b/hw/acpi.c
index 53e47d5..c6320d5 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -41,14 +41,14 @@ struct acpi_table_header {
 #define ACPI_TABLE_HDR_SIZE sizeof(struct acpi_table_header)
 #define ACPI_TABLE_PFX_SIZE sizeof(uint16_t)  /* size of the extra prefix */
 
-static const char dfl_hdr[ACPI_TABLE_HDR_SIZE] =
+static const char unsigned dfl_hdr[ACPI_TABLE_HDR_SIZE] =
     "\0\0"                   /* fake _length (2) */
     "QEMU\0\0\0\0\1\0"       /* sig (4), len(4), revno (1), csum (1) */
     "QEMUQEQEMUQEMU\1\0\0\0" /* OEM id (6), table (8), revno (4) */
     "QEMU\1\0\0\0"           /* ASL compiler ID (4), version (4) */
     ;
 
-char *acpi_tables;
+char unsigned *acpi_tables;
 size_t acpi_tables_len;
 
 static int acpi_checksum(const uint8_t *data, int len)
@@ -71,6 +71,7 @@ int acpi_table_add(const char *t)
     int changed;
     int r;
     struct acpi_table_header hdr;
+    char unsigned *table_start;
 
     r = 0;
     r |= get_param_value(buf, sizeof(buf), "data", t) ? 1 : 0;
@@ -112,7 +113,7 @@ int acpi_table_add(const char *t)
         }
 
         for (;;) {
-            char data[8192];
+            char unsigned data[8192];
             r = read(fd, data, sizeof(data));
             if (r == 0) {
                 break;
@@ -133,11 +134,11 @@ int acpi_table_add(const char *t)
 
     /* now fill in the header fields */
 
-    f = acpi_tables + start;   /* start of the table */
+    table_start = acpi_tables + start;   /* start of the table */
     changed = 0;
 
     /* copy the header to temp place to align the fields */
-    memcpy(&hdr, has_header ? f : dfl_hdr, ACPI_TABLE_HDR_SIZE);
+    memcpy(&hdr, has_header ? table_start : dfl_hdr, ACPI_TABLE_HDR_SIZE);
 
     /* length of the table minus our prefix */
     len = allen - start - ACPI_TABLE_PFX_SIZE;
@@ -225,11 +226,11 @@ int acpi_table_add(const char *t)
     hdr.checksum = 0;    /* for checksum calculation */
 
     /* put header back */
-    memcpy(f, &hdr, sizeof(hdr));
+    memcpy(table_start, &hdr, sizeof(hdr));
 
     if (changed || !has_header || 1) {
-        ((struct acpi_table_header *)f)->checksum =
-            acpi_checksum((uint8_t *)f + ACPI_TABLE_PFX_SIZE, len);
+        ((struct acpi_table_header *)table_start)->checksum =
+            acpi_checksum((uint8_t *)table_start + ACPI_TABLE_PFX_SIZE, len);
     }
 
     /* increase number of tables */
-- 
1.7.1

[Qemu-devel] [PATCH 03/11] acpi_table_add(): report fatal errors through an internal Error object

[Laszlo Ersek]

The upcoming changes will need a cleanup section at the end of the
function, plus OptsVisitor reports errors via Error. For now keep
channeling any Errors to stderr.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/acpi.c |   33 +++++++++++++++++++--------------
 1 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/hw/acpi.c b/hw/acpi.c
index c6320d5..f34fcde 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -64,6 +64,7 @@ static int acpi_checksum(const uint8_t *data, int len)
 /* XXX fixme: this function uses obsolete argument parsing interface */
 int acpi_table_add(const char *t)
 {
+    Error *err = NULL;
     char buf[1024], *p, *f;
     unsigned long val;
     size_t len, start, allen;
@@ -87,8 +88,8 @@ int acpi_table_add(const char *t)
         has_header = true;
         break;
     default:
-        fprintf(stderr, "acpitable: both data and file are specified\n");
-        return -1;
+        error_setg(&err, "acpitable: both data and file are specified");
+        goto out;
     }
 
     if (!acpi_tables) {
@@ -108,8 +109,8 @@ int acpi_table_add(const char *t)
         int fd = open(f, O_RDONLY | O_BINARY);
 
         if (fd < 0) {
-            fprintf(stderr, "can't open file %s: %s\n", f, strerror(errno));
-            return -1;
+            error_setg(&err, "can't open file %s: %s", f, strerror(errno));
+            goto out;
         }
 
         for (;;) {
@@ -122,10 +123,10 @@ int acpi_table_add(const char *t)
                 memcpy(acpi_tables + allen, data, r);
                 allen += r;
             } else if (errno != EINTR) {
-                fprintf(stderr, "can't read file %s: %s\n",
-                        f, strerror(errno));
+                error_setg(&err, "can't read file %s: %s",
+                           f, strerror(errno));
                 close(fd);
-                return -1;
+                goto out;
             }
         }
 
@@ -169,8 +170,8 @@ int acpi_table_add(const char *t)
     if (get_param_value(buf, sizeof(buf), "rev", t)) {
         val = strtoul(buf, &p, 0);
         if (val > 255 || *p) {
-            fprintf(stderr, "acpitable: \"rev=%s\" is invalid\n", buf);
-            return -1;
+            error_setg(&err, "acpitable: \"rev=%s\" is invalid", buf);
+            goto out;
         }
         hdr.revision = (uint8_t)val;
         ++changed;
@@ -191,8 +192,8 @@ int acpi_table_add(const char *t)
     if (get_param_value(buf, sizeof(buf), "oem_rev", t)) {
         val = strtol(buf, &p, 0);
         if (*p) {
-            fprintf(stderr, "acpitable: \"oem_rev=%s\" is invalid\n", buf);
-            return -1;
+            error_setg(&err, "acpitable: \"oem_rev=%s\" is invalid", buf);
+            goto out;
         }
         hdr.oem_revision = cpu_to_le32(val);
         ++changed;
@@ -207,9 +208,9 @@ int acpi_table_add(const char *t)
     if (get_param_value(buf, sizeof(buf), "asl_compiler_rev", t)) {
         val = strtol(buf, &p, 0);
         if (*p) {
-            fprintf(stderr, "acpitable: \"%s=%s\" is invalid\n",
-                    "asl_compiler_rev", buf);
-            return -1;
+            error_setg(&err, "acpitable: \"%s=%s\" is invalid",
+                       "asl_compiler_rev", buf);
+            goto out;
         }
         hdr.asl_compiler_revision = cpu_to_le32(val);
         ++changed;
@@ -240,6 +241,10 @@ int acpi_table_add(const char *t)
     acpi_tables_len = allen;
     return 0;
 
+out:
+    fprintf(stderr, "%s\n", error_get_pretty(err));
+    error_free(err);
+    return -1;
 }
 
 static void acpi_notify_wakeup(Notifier *notifier, void *data)
-- 
1.7.1

[Qemu-devel] [PATCH 04/11] qapi schema: add AcpiTableOptions

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qapi-schema.json |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index fdaa9da..aae6767 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3442,3 +3442,61 @@
 # Since: 1.5
 ##
 { 'command': 'query-tpm', 'returns': ['TPMInfo'] }
+
+##
+# @AcpiTableOptions
+#
+# Specify an ACPI table on the command line to load.
+#
+# At most one of @file and @data can be specified. The list of files specified
+# by any one of them is loaded and concatenated in order. If both are omitted,
+# @data is implied.
+#
+# Other fields / optargs can be used to override fields of the generic ACPI
+# table header; refer to the ACPI specification 5.0, section 5.2.6 System
+# Description Table Header. If a header field is not overridden, then the
+# corresponding value from the concatenated blob is used (in case of @file), or
+# it is filled in with a hard-coded value (in case of @data).
+#
+# String fields are copied into the matching ACPI member from lowest address
+# upwards, and silently truncated / NUL-padded to length.
+#
+# @sig: #optional table signature / identifier (4 bytes)
+#
+# @rev: #optional table revision number (dependent on signature, 1 byte)
+#
+# @oem_id: #optional OEM identifier (6 bytes)
+#
+# @oem_table_id: #optional OEM table identifier (8 bytes)
+#
+# @oem_rev: #optional OEM-supplied revision number (4 bytes)
+#
+# @asl_compiler_id: #optional identifier of the utility that created the table
+#                   (4 bytes)
+#
+# @asl_compiler_rev: #optional revision number of the utility that created the
+#                    table (4 bytes)
+#
+# @file: #optional colon (:) separated list of pathnames to load and
+#        concatenate as table data. The resultant binary blob is expected to
+#        have an ACPI table header. At least one file is required. This field
+#        excludes @data.
+#
+# @data: #optional colon (:) separated list of pathnames to load and
+#        concatenate as table data. The resultant binary blob must not have an
+#        ACPI table header. At least one file is required. This field excludes
+#        @file.
+#
+# Since 1.5
+##
+{ 'type': 'AcpiTableOptions',
+  'data': {
+    '*sig':               'str',
+    '*rev':               'uint8',
+    '*oem_id':            'str',
+    '*oem_table_id':      'str',
+    '*oem_rev':           'uint32',
+    '*asl_compiler_id':   'str',
+    '*asl_compiler_rev':  'uint32',
+    '*file':              'str',
+    '*data':              'str' }}
-- 
1.7.1

[Qemu-devel] [PATCH 05/11] acpi_table_add(): accept QemuOpts and parse it with OptsVisitor

[Laszlo Ersek]

As one consequence, strtok() -- which modifies its argument -- is replaced
with g_strsplit().

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/pc.h                    |    2 +-
 include/sysemu/arch_init.h |    3 +-
 arch_init.c                |    4 +-
 hw/acpi.c                  |  139 ++++++++++++++++++++++++++------------------
 hw/i386/pc.c               |    9 +++-
 vl.c                       |    4 +-
 6 files changed, 98 insertions(+), 63 deletions(-)

diff --git a/hw/pc.h b/hw/pc.h
index fe1768b..7aaba3c 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -113,7 +113,7 @@ extern char unsigned *acpi_tables;
 extern size_t acpi_tables_len;
 
 void acpi_bios_init(void);
-int acpi_table_add(const char *table_desc);
+int acpi_table_add(const QemuOpts *opts);
 
 /* acpi_piix.c */
 
diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
index 5fc780c..24d8946 100644
--- a/include/sysemu/arch_init.h
+++ b/include/sysemu/arch_init.h
@@ -2,6 +2,7 @@
 #define QEMU_ARCH_INIT_H
 
 #include "qmp-commands.h"
+#include "qemu/option.h"
 
 enum {
     QEMU_ARCH_ALL = -1,
@@ -25,7 +26,7 @@ enum {
 extern const uint32_t arch_type;
 
 void select_soundhw(const char *optarg);
-void do_acpitable_option(const char *optarg);
+void do_acpitable_option(const QemuOpts *opts);
 void do_smbios_option(const char *optarg);
 void cpudef_init(void);
 int audio_available(void);
diff --git a/arch_init.c b/arch_init.c
index 761051b..d6170f7 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1105,10 +1105,10 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid)
     return 0;
 }
 
-void do_acpitable_option(const char *optarg)
+void do_acpitable_option(const QemuOpts *opts)
 {
 #ifdef TARGET_I386
-    if (acpi_table_add(optarg) < 0) {
+    if (acpi_table_add(opts) < 0) {
         fprintf(stderr, "Wrong acpi table provided\n");
         exit(1);
     }
diff --git a/hw/acpi.c b/hw/acpi.c
index f34fcde..b29dada 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -23,6 +23,10 @@
 #include "hw/pc.h"
 #include "hw/acpi.h"
 #include "monitor/monitor.h"
+#include "qemu/config-file.h"
+#include "qapi/opts-visitor.h"
+#include "qapi/dealloc-visitor.h"
+#include "qapi-visit.h"
 
 struct acpi_table_header {
     uint16_t _length;         /* our length, not actual part of the hdr */
@@ -51,6 +55,20 @@ static const char unsigned dfl_hdr[ACPI_TABLE_HDR_SIZE] =
 char unsigned *acpi_tables;
 size_t acpi_tables_len;
 
+static QemuOptsList qemu_acpi_opts = {
+    .name = "acpi",
+    .implied_opt_name = "data",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_acpi_opts.head),
+    .desc = { { 0 } } /* validated with OptsVisitor */
+};
+
+static void acpi_register_config(void)
+{
+    qemu_add_opts(&qemu_acpi_opts);
+}
+
+machine_init(acpi_register_config);
+
 static int acpi_checksum(const uint8_t *data, int len)
 {
     int sum, i;
@@ -61,12 +79,13 @@ static int acpi_checksum(const uint8_t *data, int len)
     return (-sum) & 0xff;
 }
 
-/* XXX fixme: this function uses obsolete argument parsing interface */
-int acpi_table_add(const char *t)
+int acpi_table_add(const QemuOpts *opts)
 {
+    AcpiTableOptions *hdrs = NULL;
     Error *err = NULL;
-    char buf[1024], *p, *f;
-    unsigned long val;
+    char **pathnames = NULL;
+    char **cur;
+
     size_t len, start, allen;
     bool has_header;
     int changed;
@@ -74,21 +93,26 @@ int acpi_table_add(const char *t)
     struct acpi_table_header hdr;
     char unsigned *table_start;
 
-    r = 0;
-    r |= get_param_value(buf, sizeof(buf), "data", t) ? 1 : 0;
-    r |= get_param_value(buf, sizeof(buf), "file", t) ? 2 : 0;
-    switch (r) {
-    case 0:
-        buf[0] = '\0';
-        /* fallthrough for default behavior */
-    case 1:
-        has_header = false;
-        break;
-    case 2:
-        has_header = true;
-        break;
-    default:
-        error_setg(&err, "acpitable: both data and file are specified");
+    {
+        OptsVisitor *ov;
+
+        ov = opts_visitor_new(opts);
+        visit_type_AcpiTableOptions(opts_get_visitor(ov), &hdrs, NULL, &err);
+        opts_visitor_cleanup(ov);
+    }
+
+    if (err) {
+        goto out;
+    }
+    if (hdrs->has_file == hdrs->has_data) {
+        error_setg(&err, "'-acpitable' requires one of 'data' or 'file'");
+        goto out;
+    }
+    has_header = hdrs->has_file;
+
+    pathnames = g_strsplit(hdrs->has_file ? hdrs->file : hdrs->data, ":", 0);
+    if (pathnames == NULL || pathnames[0] == NULL) {
+        error_setg(&err, "'-acpitable' requires at least one pathname");
         goto out;
     }
 
@@ -105,11 +129,11 @@ int acpi_table_add(const char *t)
 
     /* now read in the data files, reallocating buffer as needed */
 
-    for (f = strtok(buf, ":"); f; f = strtok(NULL, ":")) {
-        int fd = open(f, O_RDONLY | O_BINARY);
+    for (cur = pathnames; *cur; ++cur) {
+        int fd = open(*cur, O_RDONLY | O_BINARY);
 
         if (fd < 0) {
-            error_setg(&err, "can't open file %s: %s", f, strerror(errno));
+            error_setg(&err, "can't open file %s: %s", *cur, strerror(errno));
             goto out;
         }
 
@@ -124,7 +148,7 @@ int acpi_table_add(const char *t)
                 allen += r;
             } else if (errno != EINTR) {
                 error_setg(&err, "can't read file %s: %s",
-                           f, strerror(errno));
+                           *cur, strerror(errno));
                 close(fd);
                 goto out;
             }
@@ -146,14 +170,16 @@ int acpi_table_add(const char *t)
 
     hdr._length = cpu_to_le16(len);
 
-    if (get_param_value(buf, sizeof(buf), "sig", t)) {
+    if (hdrs->has_sig) {
         /* strncpy is justified: the field need not be NUL-terminated. */
-        strncpy(hdr.sig, buf, sizeof(hdr.sig));
+        strncpy(hdr.sig, hdrs->sig, sizeof(hdr.sig));
         ++changed;
     }
 
     /* length of the table including header, in bytes */
     if (has_header) {
+        unsigned long val;
+
         /* check if actual length is correct */
         val = le32_to_cpu(hdr.length);
         if (val != len) {
@@ -167,52 +193,38 @@ int acpi_table_add(const char *t)
     /* we may avoid putting length here if has_header is true */
     hdr.length = cpu_to_le32(len);
 
-    if (get_param_value(buf, sizeof(buf), "rev", t)) {
-        val = strtoul(buf, &p, 0);
-        if (val > 255 || *p) {
-            error_setg(&err, "acpitable: \"rev=%s\" is invalid", buf);
-            goto out;
-        }
-        hdr.revision = (uint8_t)val;
+    if (hdrs->has_rev) {
+        hdr.revision = hdrs->rev;
         ++changed;
     }
 
-    if (get_param_value(buf, sizeof(buf), "oem_id", t)) {
+    if (hdrs->has_oem_id) {
         /* strncpy is justified: the field need not be NUL-terminated. */
-        strncpy(hdr.oem_id, buf, sizeof(hdr.oem_id));
+        strncpy(hdr.oem_id, hdrs->oem_id, sizeof(hdr.oem_id));
         ++changed;
     }
 
-    if (get_param_value(buf, sizeof(buf), "oem_table_id", t)) {
+    if (hdrs->has_oem_table_id) {
         /* strncpy is justified: the field need not be NUL-terminated. */
-        strncpy(hdr.oem_table_id, buf, sizeof(hdr.oem_table_id));
+        strncpy(hdr.oem_table_id, hdrs->oem_table_id,
+                sizeof(hdr.oem_table_id));
         ++changed;
     }
 
-    if (get_param_value(buf, sizeof(buf), "oem_rev", t)) {
-        val = strtol(buf, &p, 0);
-        if (*p) {
-            error_setg(&err, "acpitable: \"oem_rev=%s\" is invalid", buf);
-            goto out;
-        }
-        hdr.oem_revision = cpu_to_le32(val);
+    if (hdrs->has_oem_rev) {
+        hdr.oem_revision = cpu_to_le32(hdrs->oem_rev);
         ++changed;
     }
 
-    if (get_param_value(buf, sizeof(buf), "asl_compiler_id", t)) {
+    if (hdrs->has_asl_compiler_id) {
         /* strncpy is justified: the field need not be NUL-terminated. */
-        strncpy(hdr.asl_compiler_id, buf, sizeof(hdr.asl_compiler_id));
+        strncpy(hdr.asl_compiler_id, hdrs->asl_compiler_id,
+                sizeof(hdr.asl_compiler_id));
         ++changed;
     }
 
-    if (get_param_value(buf, sizeof(buf), "asl_compiler_rev", t)) {
-        val = strtol(buf, &p, 0);
-        if (*p) {
-            error_setg(&err, "acpitable: \"%s=%s\" is invalid",
-                       "asl_compiler_rev", buf);
-            goto out;
-        }
-        hdr.asl_compiler_revision = cpu_to_le32(val);
+    if (hdrs->has_asl_compiler_rev) {
+        hdr.asl_compiler_revision = cpu_to_le32(hdrs->asl_compiler_rev);
         ++changed;
     }
 
@@ -239,12 +251,25 @@ int acpi_table_add(const char *t)
         cpu_to_le32(le32_to_cpu(*(uint16_t *)acpi_tables) + 1);
 
     acpi_tables_len = allen;
-    return 0;
 
 out:
-    fprintf(stderr, "%s\n", error_get_pretty(err));
-    error_free(err);
-    return -1;
+    g_strfreev(pathnames);
+
+    if (hdrs != NULL) {
+        QapiDeallocVisitor *dv;
+
+        dv = qapi_dealloc_visitor_new();
+        visit_type_AcpiTableOptions(qapi_dealloc_get_visitor(dv), &hdrs, NULL,
+                                    NULL);
+        qapi_dealloc_visitor_cleanup(dv);
+    }
+
+    if (err) {
+        fprintf(stderr, "%s\n", error_get_pretty(err));
+        error_free(err);
+        return -1;
+    }
+    return 0;
 }
 
 static void acpi_notify_wakeup(Notifier *notifier, void *data)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ed7d9ba..bba19e4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -51,6 +51,7 @@
 #include "exec/address-spaces.h"
 #include "sysemu/arch_init.h"
 #include "qemu/bitmap.h"
+#include "qemu/config-file.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -886,6 +887,7 @@ void pc_cpus_init(const char *cpu_model)
 void pc_acpi_init(const char *default_dsdt)
 {
     char *filename = NULL, *arg = NULL;
+    QemuOpts *opts;
 
     if (acpi_tables != NULL) {
         /* manually set via -acpitable, leave it alone */
@@ -899,7 +901,12 @@ void pc_acpi_init(const char *default_dsdt)
     }
 
     arg = g_strdup_printf("file=%s", filename);
-    if (acpi_table_add(arg) != 0) {
+
+    /* creates a deep copy of "arg" */
+    opts = qemu_opts_parse(qemu_find_opts("acpi"), arg, 0);
+    g_assert(opts != NULL);
+
+    if (acpi_table_add(opts) != 0) {
         fprintf(stderr, "WARNING: failed to load %s\n", filename);
     }
     g_free(arg);
diff --git a/vl.c b/vl.c
index f2bd893..75c5b60 100644
--- a/vl.c
+++ b/vl.c
@@ -3556,7 +3556,9 @@ int main(int argc, char **argv, char **envp)
                 break;
             }
             case QEMU_OPTION_acpitable:
-                do_acpitable_option(optarg);
+                opts = qemu_opts_parse(qemu_find_opts("acpi"), optarg, 1);
+                g_assert(opts != NULL);
+                do_acpitable_option(opts);
                 break;
             case QEMU_OPTION_smbios:
                 do_smbios_option(optarg);
-- 
1.7.1

[Qemu-devel] [PATCH 06/11] acpi_table_add(): extract and reimplement internals

[Laszlo Ersek]

The new function acpi_table_install() installs any blob the caller passes
in. In the next patches this function will be promoted from helper role to
extern.

Reimplementing the logic should make it easier to understand. It also
removes a buffer overflow when

    has_header &&
    cumulative_file_size < ACPI_TABLE_HDR_SIZE - ACPI_TABLE_PFX_SIZE

(In that case the g_realloc() call in the read() loop used to shrink the
"acpi_tables" array, causing an out-of-bounds read access when copying the
header out of "acpi_tables".)

The new code isn't more daring alignment-wise than its predecessor:
"acpi_table_header" is packed, and the uint32_t fields are at offsets 6,
26, and 34.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/acpi.c |  284 +++++++++++++++++++++++++++++++++++--------------------------
 1 files changed, 163 insertions(+), 121 deletions(-)

diff --git a/hw/acpi.c b/hw/acpi.c
index b29dada..ce408da 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -30,7 +30,7 @@
 
 struct acpi_table_header {
     uint16_t _length;         /* our length, not actual part of the hdr */
-                              /* XXX why we have 2 length fields here? */
+                              /* allows easier parsing for fw_cfg clients */
     char sig[4];              /* ACPI signature (4 ASCII characters) */
     uint32_t length;          /* Length of table, in bytes, including header */
     uint8_t revision;         /* ACPI Specification minor version # */
@@ -45,8 +45,7 @@ struct acpi_table_header {
 #define ACPI_TABLE_HDR_SIZE sizeof(struct acpi_table_header)
 #define ACPI_TABLE_PFX_SIZE sizeof(uint16_t)  /* size of the extra prefix */
 
-static const char unsigned dfl_hdr[ACPI_TABLE_HDR_SIZE] =
-    "\0\0"                   /* fake _length (2) */
+static const char unsigned dfl_hdr[ACPI_TABLE_HDR_SIZE - ACPI_TABLE_PFX_SIZE] =
     "QEMU\0\0\0\0\1\0"       /* sig (4), len(4), revno (1), csum (1) */
     "QEMUQEQEMUQEMU\1\0\0\0" /* OEM id (6), table (8), revno (4) */
     "QEMU\1\0\0\0"           /* ASL compiler ID (4), version (4) */
@@ -79,19 +78,165 @@ static int acpi_checksum(const uint8_t *data, int len)
     return (-sum) & 0xff;
 }
 
+
+/* Install a copy of the ACPI table specified in @blob.
+ *
+ * If @has_header is set, @blob starts with the System Description Table Header
+ * structure. Otherwise, "dfl_hdr" is prepended. In any case, each header field
+ * is optionally overwritten from @hdrs.
+ *
+ * It is valid to call this function with
+ * (@blob == NULL && bloblen == 0 && !has_header).
+ *
+ * @hdrs->file and @hdrs->data are ignored.
+ *
+ * SIZE_MAX is considered "infinity" in this function.
+ *
+ * The number of tables that can be installed is not limited, but the 16-bit
+ * counter at the beginning of "acpi_tables" wraps around after UINT16_MAX.
+ */
+static void acpi_table_install(const char unsigned *blob, size_t bloblen,
+                               bool has_header,
+                               const struct AcpiTableOptions *hdrs,
+                               Error **errp)
+{
+    size_t body_start;
+    const char unsigned *hdr_src;
+    size_t body_size, acpi_payload_size;
+    struct acpi_table_header *ext_hdr;
+    unsigned changed_fields;
+
+    /* Calculate where the ACPI table body starts within the blob, plus where
+     * to copy the ACPI table header from.
+     */
+    if (has_header) {
+        /*   _length             | ACPI header in blob | blob body
+         *   ^^^^^^^^^^^^^^^^^^^   ^^^^^^^^^^^^^^^^^^^   ^^^^^^^^^
+         *   ACPI_TABLE_PFX_SIZE     sizeof dfl_hdr      body_size
+         *                           == body_start
+         *
+         *                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+         *                           acpi_payload_size == bloblen
+         */
+        body_start = sizeof dfl_hdr;
+
+        if (bloblen < body_start) {
+            error_setg(errp, "ACPI table claiming to have header is too "
+                       "short, available: %zu, expected: %zu", bloblen,
+                       body_start);
+            return;
+        }
+        hdr_src = blob;
+    } else {
+        /*   _length             | ACPI header in template | blob body
+         *   ^^^^^^^^^^^^^^^^^^^   ^^^^^^^^^^^^^^^^^^^^^^^   ^^^^^^^^^^
+         *   ACPI_TABLE_PFX_SIZE       sizeof dfl_hdr        body_size
+         *                                                   == bloblen
+         *
+         *                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+         *                                  acpi_payload_size
+         */
+        body_start = 0;
+        hdr_src = dfl_hdr;
+    }
+    body_size = bloblen - body_start;
+    acpi_payload_size = sizeof dfl_hdr + body_size;
+
+    if (acpi_payload_size > UINT16_MAX) {
+        error_setg(errp, "ACPI table too big, requested: %zu, max: %u",
+                   acpi_payload_size, (unsigned)UINT16_MAX);
+        return;
+    }
+
+    /* We won't fail from here on. Initialize / extend the globals. */
+    if (acpi_tables == NULL) {
+        acpi_tables_len = sizeof(uint16_t);
+        acpi_tables = g_malloc0(acpi_tables_len);
+    }
+
+    acpi_tables = g_realloc(acpi_tables, acpi_tables_len +
+                                         ACPI_TABLE_PFX_SIZE +
+                                         sizeof dfl_hdr + body_size);
+
+    ext_hdr = (struct acpi_table_header *)(acpi_tables + acpi_tables_len);
+    acpi_tables_len += ACPI_TABLE_PFX_SIZE;
+
+    memcpy(acpi_tables + acpi_tables_len, hdr_src, sizeof dfl_hdr);
+    acpi_tables_len += sizeof dfl_hdr;
+
+    if (blob != NULL) {
+        memcpy(acpi_tables + acpi_tables_len, blob + body_start, body_size);
+        acpi_tables_len += body_size;
+    }
+
+    /* increase number of tables */
+    cpu_to_le16wu((uint16_t *)acpi_tables,
+                  le16_to_cpupu((uint16_t *)acpi_tables) + 1u);
+
+    /* Update the header fields. The strings need not be NUL-terminated. */
+    changed_fields = 0;
+    ext_hdr->_length = cpu_to_le16(acpi_payload_size);
+
+    if (hdrs->has_sig) {
+        strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
+        ++changed_fields;
+    }
+
+    if (has_header && le32_to_cpu(ext_hdr->length) != acpi_payload_size) {
+        fprintf(stderr,
+                "warning: ACPI table has wrong length, header says "
+                "%" PRIu32 ", actual size %zu bytes\n",
+                le32_to_cpu(ext_hdr->length), acpi_payload_size);
+    }
+    ext_hdr->length = cpu_to_le32(acpi_payload_size);
+
+    if (hdrs->has_rev) {
+        ext_hdr->revision = hdrs->rev;
+        ++changed_fields;
+    }
+
+    ext_hdr->checksum = 0;
+
+    if (hdrs->has_oem_id) {
+        strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id);
+        ++changed_fields;
+    }
+    if (hdrs->has_oem_table_id) {
+        strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id,
+                sizeof ext_hdr->oem_table_id);
+        ++changed_fields;
+    }
+    if (hdrs->has_oem_rev) {
+        ext_hdr->oem_revision = cpu_to_le32(hdrs->oem_rev);
+        ++changed_fields;
+    }
+    if (hdrs->has_asl_compiler_id) {
+        strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id,
+                sizeof ext_hdr->asl_compiler_id);
+        ++changed_fields;
+    }
+    if (hdrs->has_asl_compiler_rev) {
+        ext_hdr->asl_compiler_revision = cpu_to_le32(hdrs->asl_compiler_rev);
+        ++changed_fields;
+    }
+
+    if (!has_header && changed_fields == 0) {
+        fprintf(stderr, "warning: ACPI table: no headers are specified\n");
+    }
+
+    /* recalculate checksum */
+    ext_hdr->checksum = acpi_checksum((const char unsigned *)ext_hdr +
+                                      ACPI_TABLE_PFX_SIZE, acpi_payload_size);
+}
+
 int acpi_table_add(const QemuOpts *opts)
 {
     AcpiTableOptions *hdrs = NULL;
     Error *err = NULL;
     char **pathnames = NULL;
     char **cur;
-
-    size_t len, start, allen;
-    bool has_header;
-    int changed;
-    int r;
-    struct acpi_table_header hdr;
-    char unsigned *table_start;
+    size_t bloblen = 0;
+    char unsigned *blob = NULL;
 
     {
         OptsVisitor *ov;
@@ -108,7 +253,6 @@ int acpi_table_add(const QemuOpts *opts)
         error_setg(&err, "'-acpitable' requires one of 'data' or 'file'");
         goto out;
     }
-    has_header = hdrs->has_file;
 
     pathnames = g_strsplit(hdrs->has_file ? hdrs->file : hdrs->data, ":", 0);
     if (pathnames == NULL || pathnames[0] == NULL) {
@@ -116,19 +260,7 @@ int acpi_table_add(const QemuOpts *opts)
         goto out;
     }
 
-    if (!acpi_tables) {
-        allen = sizeof(uint16_t);
-        acpi_tables = g_malloc0(allen);
-    } else {
-        allen = acpi_tables_len;
-    }
-
-    start = allen;
-    acpi_tables = g_realloc(acpi_tables, start + ACPI_TABLE_HDR_SIZE);
-    allen += has_header ? ACPI_TABLE_PFX_SIZE : ACPI_TABLE_HDR_SIZE;
-
     /* now read in the data files, reallocating buffer as needed */
-
     for (cur = pathnames; *cur; ++cur) {
         int fd = open(*cur, O_RDONLY | O_BINARY);
 
@@ -139,13 +271,15 @@ int acpi_table_add(const QemuOpts *opts)
 
         for (;;) {
             char unsigned data[8192];
-            r = read(fd, data, sizeof(data));
+            ssize_t r;
+
+            r = read(fd, data, sizeof data);
             if (r == 0) {
                 break;
             } else if (r > 0) {
-                acpi_tables = g_realloc(acpi_tables, allen + r);
-                memcpy(acpi_tables + allen, data, r);
-                allen += r;
+                blob = g_realloc(blob, bloblen + r);
+                memcpy(blob + bloblen, data, r);
+                bloblen += r;
             } else if (errno != EINTR) {
                 error_setg(&err, "can't read file %s: %s",
                            *cur, strerror(errno));
@@ -157,102 +291,10 @@ int acpi_table_add(const QemuOpts *opts)
         close(fd);
     }
 
-    /* now fill in the header fields */
-
-    table_start = acpi_tables + start;   /* start of the table */
-    changed = 0;
-
-    /* copy the header to temp place to align the fields */
-    memcpy(&hdr, has_header ? table_start : dfl_hdr, ACPI_TABLE_HDR_SIZE);
-
-    /* length of the table minus our prefix */
-    len = allen - start - ACPI_TABLE_PFX_SIZE;
-
-    hdr._length = cpu_to_le16(len);
-
-    if (hdrs->has_sig) {
-        /* strncpy is justified: the field need not be NUL-terminated. */
-        strncpy(hdr.sig, hdrs->sig, sizeof(hdr.sig));
-        ++changed;
-    }
-
-    /* length of the table including header, in bytes */
-    if (has_header) {
-        unsigned long val;
-
-        /* check if actual length is correct */
-        val = le32_to_cpu(hdr.length);
-        if (val != len) {
-            fprintf(stderr,
-                "warning: acpitable has wrong length,"
-                " header says %lu, actual size %zu bytes\n",
-                val, len);
-            ++changed;
-        }
-    }
-    /* we may avoid putting length here if has_header is true */
-    hdr.length = cpu_to_le32(len);
-
-    if (hdrs->has_rev) {
-        hdr.revision = hdrs->rev;
-        ++changed;
-    }
-
-    if (hdrs->has_oem_id) {
-        /* strncpy is justified: the field need not be NUL-terminated. */
-        strncpy(hdr.oem_id, hdrs->oem_id, sizeof(hdr.oem_id));
-        ++changed;
-    }
-
-    if (hdrs->has_oem_table_id) {
-        /* strncpy is justified: the field need not be NUL-terminated. */
-        strncpy(hdr.oem_table_id, hdrs->oem_table_id,
-                sizeof(hdr.oem_table_id));
-        ++changed;
-    }
-
-    if (hdrs->has_oem_rev) {
-        hdr.oem_revision = cpu_to_le32(hdrs->oem_rev);
-        ++changed;
-    }
-
-    if (hdrs->has_asl_compiler_id) {
-        /* strncpy is justified: the field need not be NUL-terminated. */
-        strncpy(hdr.asl_compiler_id, hdrs->asl_compiler_id,
-                sizeof(hdr.asl_compiler_id));
-        ++changed;
-    }
-
-    if (hdrs->has_asl_compiler_rev) {
-        hdr.asl_compiler_revision = cpu_to_le32(hdrs->asl_compiler_rev);
-        ++changed;
-    }
-
-    if (!has_header && !changed) {
-        fprintf(stderr, "warning: acpitable: no table headers are specified\n");
-    }
-
-
-    /* now calculate checksum of the table, complete with the header */
-    /* we may as well leave checksum intact if has_header is true */
-    /* alternatively there may be a way to set cksum to a given value */
-    hdr.checksum = 0;    /* for checksum calculation */
-
-    /* put header back */
-    memcpy(table_start, &hdr, sizeof(hdr));
-
-    if (changed || !has_header || 1) {
-        ((struct acpi_table_header *)table_start)->checksum =
-            acpi_checksum((uint8_t *)table_start + ACPI_TABLE_PFX_SIZE, len);
-    }
-
-    /* increase number of tables */
-    (*(uint16_t *)acpi_tables) =
-        cpu_to_le32(le32_to_cpu(*(uint16_t *)acpi_tables) + 1);
-
-    acpi_tables_len = allen;
+    acpi_table_install(blob, bloblen, hdrs->has_file, hdrs, &err);
 
 out:
+    g_free(blob);
     g_strfreev(pathnames);
 
     if (hdrs != NULL) {
-- 
1.7.1

[Qemu-devel] [PATCH 07/11] like acpi_table_install(), acpi_table_add() should propagate Errors

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/pc.h      |    2 +-
 arch_init.c  |    9 +++++++--
 hw/acpi.c    |    9 ++-------
 hw/i386/pc.c |    8 ++++++--
 4 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/hw/pc.h b/hw/pc.h
index 7aaba3c..b8574a2 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -113,7 +113,7 @@ extern char unsigned *acpi_tables;
 extern size_t acpi_tables_len;
 
 void acpi_bios_init(void);
-int acpi_table_add(const QemuOpts *opts);
+void acpi_table_add(const QemuOpts *opts, Error **errp);
 
 /* acpi_piix.c */
 
diff --git a/arch_init.c b/arch_init.c
index d6170f7..42e38d3 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1108,8 +1108,13 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid)
 void do_acpitable_option(const QemuOpts *opts)
 {
 #ifdef TARGET_I386
-    if (acpi_table_add(opts) < 0) {
-        fprintf(stderr, "Wrong acpi table provided\n");
+    Error *err = NULL;
+
+    acpi_table_add(opts, &err);
+    if (err) {
+        fprintf(stderr, "Wrong acpi table provided: %s\n",
+                error_get_pretty(err));
+        error_free(err);
         exit(1);
     }
 #endif
diff --git a/hw/acpi.c b/hw/acpi.c
index ce408da..e7a213c 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -229,7 +229,7 @@ static void acpi_table_install(const char unsigned *blob, size_t bloblen,
                                       ACPI_TABLE_PFX_SIZE, acpi_payload_size);
 }
 
-int acpi_table_add(const QemuOpts *opts)
+void acpi_table_add(const QemuOpts *opts, Error **errp)
 {
     AcpiTableOptions *hdrs = NULL;
     Error *err = NULL;
@@ -306,12 +306,7 @@ out:
         qapi_dealloc_visitor_cleanup(dv);
     }
 
-    if (err) {
-        fprintf(stderr, "%s\n", error_get_pretty(err));
-        error_free(err);
-        return -1;
-    }
-    return 0;
+    error_propagate(errp, err);
 }
 
 static void acpi_notify_wakeup(Notifier *notifier, void *data)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bba19e4..c0a1bff 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -888,6 +888,7 @@ void pc_acpi_init(const char *default_dsdt)
 {
     char *filename = NULL, *arg = NULL;
     QemuOpts *opts;
+    Error *err = NULL;
 
     if (acpi_tables != NULL) {
         /* manually set via -acpitable, leave it alone */
@@ -906,8 +907,11 @@ void pc_acpi_init(const char *default_dsdt)
     opts = qemu_opts_parse(qemu_find_opts("acpi"), arg, 0);
     g_assert(opts != NULL);
 
-    if (acpi_table_add(opts) != 0) {
-        fprintf(stderr, "WARNING: failed to load %s\n", filename);
+    acpi_table_add(opts, &err);
+    if (err) {
+        fprintf(stderr, "WARNING: failed to load %s: %s\n", filename,
+                error_get_pretty(err));
+        error_free(err);
     }
     g_free(arg);
     g_free(filename);
-- 
1.7.1

[Qemu-devel] [PATCH 08/11] extract/unify the constant 0xfee00000 as APIC_DEFAULT_ADDRESS

[Laszlo Ersek]

A common dependency of the constant's current users:
- hw/apic_common.c
- hw/i386/kvmvapic.c
- target-i386/cpu.c
is "target-i386/cpu.h".

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 target-i386/cpu.h  |    2 ++
 hw/apic_common.c   |    2 +-
 hw/i386/kvmvapic.c |    2 --
 target-i386/cpu.c  |    4 +---
 4 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 5284ebc..069a2e2 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1267,4 +1267,6 @@ const char *get_register_name_32(unsigned int reg);
 uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index);
 void enable_compat_apic_id_mode(void);
 
+#define APIC_DEFAULT_ADDRESS 0xfee00000
+
 #endif /* CPU_I386_H */
diff --git a/hw/apic_common.c b/hw/apic_common.c
index d0c2616..3798509 100644
--- a/hw/apic_common.c
+++ b/hw/apic_common.c
@@ -218,7 +218,7 @@ static void apic_reset_common(DeviceState *d)
     bool bsp;
 
     bsp = cpu_is_bsp(s->cpu);
-    s->apicbase = 0xfee00000 |
+    s->apicbase = APIC_DEFAULT_ADDRESS |
         (bsp ? MSR_IA32_APICBASE_BSP : 0) | MSR_IA32_APICBASE_ENABLE;
 
     s->vapic_paddr = 0;
diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index c151c95..cc95e5c 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -13,8 +13,6 @@
 #include "sysemu/kvm.h"
 #include "hw/apic_internal.h"
 
-#define APIC_DEFAULT_ADDRESS    0xfee00000
-
 #define VAPIC_IO_PORT           0x7e
 
 #define VAPIC_CPU_SHIFT         7
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a0640db..764eb65 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2048,8 +2048,6 @@ static void mce_init(X86CPU *cpu)
     }
 }
 
-#define MSI_ADDR_BASE 0xfee00000
-
 #ifndef CONFIG_USER_ONLY
 static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
 {
@@ -2089,7 +2087,7 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
            on the global memory bus. */
         /* XXX: what if the base changes? */
         sysbus_mmio_map_overlap(SYS_BUS_DEVICE(env->apic_state), 0,
-                                MSI_ADDR_BASE, 0x1000);
+                                APIC_DEFAULT_ADDRESS, 0x1000);
         apic_mapped = 1;
     }
 }
-- 
1.7.1

[Qemu-devel] [PATCH 09/11] Introduce IO_APIC_DEFAULT_ADDRESS for 0xfec00000

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/pc.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c0a1bff..23f9800 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -72,6 +72,8 @@
 #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
 #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
 
+#define IO_APIC_DEFAULT_ADDRESS 0xfec00000
+
 #define E820_NR_ENTRIES		16
 
 struct e820_entry {
@@ -1166,7 +1168,7 @@ void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name)
     }
     qdev_init_nofail(dev);
     d = SYS_BUS_DEVICE(dev);
-    sysbus_mmio_map(d, 0, 0xfec00000);
+    sysbus_mmio_map(d, 0, IO_APIC_DEFAULT_ADDRESS);
 
     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
         gsi_state->ioapic_irq[i] = qdev_get_gpio_in(dev, i);
-- 
1.7.1

[Qemu-devel] [PATCH 10/11] pc_acpi_init(): don't bail as soon as failing to find default DSDT

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/pc.c |   33 +++++++++++++++++----------------
 1 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 23f9800..0a5d4d4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -888,9 +888,7 @@ void pc_cpus_init(const char *cpu_model)
 
 void pc_acpi_init(const char *default_dsdt)
 {
-    char *filename = NULL, *arg = NULL;
-    QemuOpts *opts;
-    Error *err = NULL;
+    char *filename;
 
     if (acpi_tables != NULL) {
         /* manually set via -acpitable, leave it alone */
@@ -900,23 +898,26 @@ void pc_acpi_init(const char *default_dsdt)
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, default_dsdt);
     if (filename == NULL) {
         fprintf(stderr, "WARNING: failed to find %s\n", default_dsdt);
-        return;
-    }
+    } else {
+        char *arg;
+        QemuOpts *opts;
+        Error *err = NULL;
 
-    arg = g_strdup_printf("file=%s", filename);
+        arg = g_strdup_printf("file=%s", filename);
 
-    /* creates a deep copy of "arg" */
-    opts = qemu_opts_parse(qemu_find_opts("acpi"), arg, 0);
-    g_assert(opts != NULL);
+        /* creates a deep copy of "arg" */
+        opts = qemu_opts_parse(qemu_find_opts("acpi"), arg, 0);
+        g_assert(opts != NULL);
 
-    acpi_table_add(opts, &err);
-    if (err) {
-        fprintf(stderr, "WARNING: failed to load %s: %s\n", filename,
-                error_get_pretty(err));
-        error_free(err);
+        acpi_table_add(opts, &err);
+        if (err) {
+            fprintf(stderr, "WARNING: failed to load %s: %s\n", filename,
+                    error_get_pretty(err));
+            error_free(err);
+        }
+        g_free(arg);
+        g_free(filename);
     }
-    g_free(arg);
-    g_free(filename);
 }
 
 void *pc_memory_init(MemoryRegion *system_memory,
-- 
1.7.1

[Qemu-devel] [PATCH 11/11] i386/pc: build ACPI MADT for fw_cfg clients

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/pc.h      |    3 +
 hw/acpi.c    |    7 +--
 hw/i386/pc.c |  128 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 134 insertions(+), 4 deletions(-)

diff --git a/hw/pc.h b/hw/pc.h
index b8574a2..2a4358f 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -113,6 +113,9 @@ extern char unsigned *acpi_tables;
 extern size_t acpi_tables_len;
 
 void acpi_bios_init(void);
+void acpi_table_install(const char unsigned *blob, size_t bloblen,
+                        bool has_header, const struct AcpiTableOptions *hdrs,
+                        Error **errp);
 void acpi_table_add(const QemuOpts *opts, Error **errp);
 
 /* acpi_piix.c */
diff --git a/hw/acpi.c b/hw/acpi.c
index e7a213c..58b5afd 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -95,10 +95,9 @@ static int acpi_checksum(const uint8_t *data, int len)
  * The number of tables that can be installed is not limited, but the 16-bit
  * counter at the beginning of "acpi_tables" wraps around after UINT16_MAX.
  */
-static void acpi_table_install(const char unsigned *blob, size_t bloblen,
-                               bool has_header,
-                               const struct AcpiTableOptions *hdrs,
-                               Error **errp)
+void acpi_table_install(const char unsigned *blob, size_t bloblen,
+                        bool has_header, const struct AcpiTableOptions *hdrs,
+                        Error **errp)
 {
     size_t body_start;
     const char unsigned *hdr_src;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0a5d4d4..331eb60 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -886,6 +886,132 @@ void pc_cpus_init(const char *cpu_model)
     }
 }
 
+static void pc_acpi_madt(void)
+{
+    typedef struct {
+        uint8_t    type;
+        uint8_t    length;
+    } QEMU_PACKED AcpiSubHdr;
+
+    struct {
+        uint32_t   lapic_addr; /* Local Interrupt Controller Address */
+        uint32_t   flags;      /* Multiple APIC flags */
+    } QEMU_PACKED *madt;
+    struct {
+        AcpiSubHdr hdr;
+        uint8_t    processor_id; /* ACPI Processor ID */
+        uint8_t    apic_id;      /* APIC ID */
+        uint32_t   flags;        /* LOcal APIC flags */
+    } QEMU_PACKED *lapic;
+    struct {
+        AcpiSubHdr hdr;
+        uint8_t    io_apic_id;   /* The I/O APIC's ID */
+        uint8_t    reserved;     /* constant zero */
+        uint32_t   io_apic_addr; /* 32-bit physical address to access */
+        uint32_t   gsi_base;     /* interrupt inputs start here */
+    } QEMU_PACKED *io_apic;
+    struct {
+        AcpiSubHdr hdr;
+        uint8_t    bus;    /* constant zero: ISA */
+        uint8_t    source; /* this bus-relative interrupt source... */
+        uint32_t   gsi;    /* ... will signal this global system interrupt */
+        uint16_t   flags;  /* MPS INTI Flags: Polarity, Trigger Mode */
+    } QEMU_PACKED *int_src_ovr;
+    struct {
+        AcpiSubHdr hdr;
+        uint8_t    processor_id; /* ACPI Processor ID */
+        uint16_t   flags;        /* MPS INTI Flags: Polarity, Trigger Mode */
+        uint8_t    lint;         /* LAPIC interrupt input for NMI */
+    } QEMU_PACKED *lapic_nmi;
+
+    static const AcpiTableOptions hdrs = {
+        .has_sig              = true,
+        .sig                  = (char *)"APIC",
+        .has_oem_id           = true,
+        .oem_id               = (char *)"QEMU  ",
+        .has_oem_table_id     = true,
+        .oem_table_id         = (char *)"QEMUMADT"
+    };
+    static const uint8_t pci_isa_irq[] = { 5, 9, 10, 11 };
+
+    unsigned num_lapic, num_int_src_ovr, i;
+    size_t blob_size;
+    char unsigned *blob;
+    Error *err = NULL;
+
+    /* see note on FW_CFG_MAX_CPUS in bochs_bios_init() */
+    num_lapic = pc_apic_id_limit(max_cpus);
+    num_int_src_ovr = sizeof pci_isa_irq + kvm_allows_irq0_override();
+
+    blob_size = (sizeof *madt)        * 1               +
+                (sizeof *lapic)       * num_lapic       +
+                (sizeof *io_apic)     * 1               +
+                (sizeof *int_src_ovr) * num_int_src_ovr +
+                (sizeof *lapic_nmi)   * 1;
+    blob      = g_malloc(blob_size);
+
+    madt        = (void *)blob;
+    lapic       = (void *)(madt        + 1              );
+    io_apic     = (void *)(lapic       + num_lapic      );
+    int_src_ovr = (void *)(io_apic     + 1              );
+    lapic_nmi   = (void *)(int_src_ovr + num_int_src_ovr);
+
+    madt->lapic_addr = cpu_to_le32(APIC_DEFAULT_ADDRESS);
+    madt->flags      = cpu_to_le32(1); /* PCAT_COMPAT */
+
+    /* create a Local APIC structure for each possible APIC ID */
+    for (i = 0; i < num_lapic; ++i) {
+        lapic[i].hdr.type     = 0; /* Processor Local APIC */
+        lapic[i].hdr.length   = sizeof *lapic;
+        lapic[i].processor_id = i;
+        lapic[i].apic_id      = i;
+        lapic[i].flags        = cpu_to_le32(0); /* disabled */
+    }
+    /* enable the CPUs with a CPU index in the [0..smp_cpus-1] range */
+    for (i = 0; i < smp_cpus; ++i) {
+        lapic[x86_cpu_apic_id_from_index(i)].flags = cpu_to_le32(1);
+    }
+
+    io_apic->hdr.type     = 1; /* I/O APIC */
+    io_apic->hdr.length   = sizeof *io_apic;
+    io_apic->io_apic_id   = 0;
+    io_apic->reserved     = 0;
+    io_apic->io_apic_addr = cpu_to_le32(IO_APIC_DEFAULT_ADDRESS);
+    io_apic->gsi_base     = cpu_to_le32(0);
+
+    for (i = 0; i < sizeof pci_isa_irq; ++i) {
+        int_src_ovr[i].hdr.type   = 2; /* Interrupt Source Override */
+        int_src_ovr[i].hdr.length = sizeof *int_src_ovr;
+        int_src_ovr[i].bus        = 0;
+        int_src_ovr[i].source     = pci_isa_irq[i];
+        int_src_ovr[i].gsi        = cpu_to_le32(pci_isa_irq[i]);
+        int_src_ovr[i].flags      = cpu_to_le16(0xd);
+                                    /* active high, level-triggered */
+    }
+    if (kvm_allows_irq0_override()) {
+        int_src_ovr[i].hdr.type   = 2; /* Interrupt Source Override */
+        int_src_ovr[i].hdr.length = sizeof *int_src_ovr;
+        int_src_ovr[i].bus        = 0;
+        int_src_ovr[i].source     = 0;
+        int_src_ovr[i].gsi        = cpu_to_le32(2);
+        int_src_ovr[i].flags      = cpu_to_le16(0); /* conforms to bus spec */
+    }
+
+    lapic_nmi->hdr.type     = 4; /* Local APIC NMI */
+    lapic_nmi->hdr.length   = sizeof *lapic_nmi;
+    lapic_nmi->processor_id = 0xff; /* all processors */
+    lapic_nmi->flags        = cpu_to_le16(0); /* conforms to bus spec */
+    lapic_nmi->lint         = 1; /* NMI connected to LAPIC input LINT1 */
+
+    acpi_table_install(blob, blob_size, false, &hdrs, &err);
+    if (err) {
+        fprintf(stderr, "WARNING: failed to install MADT: %s\n",
+                error_get_pretty(err));
+        error_free(err);
+    }
+    g_free(blob);
+}
+
 void pc_acpi_init(const char *default_dsdt)
 {
     char *filename;
@@ -918,6 +1044,8 @@ void pc_acpi_init(const char *default_dsdt)
         g_free(arg);
         g_free(filename);
     }
+
+    pc_acpi_madt();
 }
 
 void *pc_memory_init(MemoryRegion *system_memory,
-- 
1.7.1

Re: [Qemu-devel] [PATCH 02/11] change element type from "char" to "unsigned char" in ACPI table data

[Eric Blake]

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

On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
> The data is binary, not textual.
> 
> Also, acpi_table_add() abuses the "char *f" pointer -- which normally
> points to file names to load -- to poke into the table. Introduce "char
> unsigned *table_start" for that purpose.
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---

> @@ -112,7 +113,7 @@ int acpi_table_add(const char *t)
>          }
>  
>          for (;;) {
> -            char data[8192];
> +            char unsigned data[8192];

Although this spelling of the type is valid C, it is not typical
convention prior to your patch:

$ git grep 'unsigned char' | wc
    508    3130   34115
$ git grep 'char unsigned' | wc
      0       0       0

> @@ -225,11 +226,11 @@ int acpi_table_add(const char *t)
>      hdr.checksum = 0;    /* for checksum calculation */
>  
>      /* put header back */
> -    memcpy(f, &hdr, sizeof(hdr));
> +    memcpy(table_start, &hdr, sizeof(hdr));
>  
>      if (changed || !has_header || 1) {
> -        ((struct acpi_table_header *)f)->checksum =
> -            acpi_checksum((uint8_t *)f + ACPI_TABLE_PFX_SIZE, len);
> +        ((struct acpi_table_header *)table_start)->checksum =
> +            acpi_checksum((uint8_t *)table_start + ACPI_TABLE_PFX_SIZE, len);

Now that table_start is an unsigned char *, do you still need the cast
to uint8_t*?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 04/11] qapi schema: add AcpiTableOptions

[Eric Blake]

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

On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qapi-schema.json |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++

No counterpart change to qmp-commands.hx showing a valid usage?

>  1 files changed, 58 insertions(+), 0 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index fdaa9da..aae6767 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3442,3 +3442,61 @@
>  # Since: 1.5
>  ##
>  { 'command': 'query-tpm', 'returns': ['TPMInfo'] }
> +
> +##
> +# @AcpiTableOptions
> +#
> +# Specify an ACPI table on the command line to load.
> +#

> +#
> +# @oem_id: #optional OEM identifier (6 bytes)

s/oem_id/oem-id/

In general, new QMP interfaces should use '-', not '_'.

> +#
> +# @file: #optional colon (:) separated list of pathnames to load and
> +#        concatenate as table data. The resultant binary blob is expected to
> +#        have an ACPI table header. At least one file is required. This field
> +#        excludes @data.
> +#

Ewwww.  This should be '*file' : [ 'str' ] (that is, use a JSON array of
file names, not a single string).  If you have to reparse a JSON
argument to break it into parts, then you are using the wrong interface;
not to mention that I might (perversely) want to pass in a file name
that contains a colon as part of its name.

> +# @data: #optional colon (:) separated list of pathnames to load and
> +#        concatenate as table data. The resultant binary blob must not have an
> +#        ACPI table header. At least one file is required. This field excludes
> +#        @file.

Again, JSON array, not flat string.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 02/11] change element type from "char" to "unsigned char" in ACPI table data

[Laszlo Ersek]

On 03/21/13 00:40, Eric Blake wrote:
> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
>> The data is binary, not textual.
>>
>> Also, acpi_table_add() abuses the "char *f" pointer -- which normally
>> points to file names to load -- to poke into the table. Introduce "char
>> unsigned *table_start" for that purpose.
>>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
> 
>> @@ -112,7 +113,7 @@ int acpi_table_add(const char *t)
>>          }
>>  
>>          for (;;) {
>> -            char data[8192];
>> +            char unsigned data[8192];
> 
> Although this spelling of the type is valid C, it is not typical
> convention prior to your patch:
> 
> $ git grep 'unsigned char' | wc
>     508    3130   34115
> $ git grep 'char unsigned' | wc
>       0       0       0

Will fix if a respin is required! :)

>> @@ -225,11 +226,11 @@ int acpi_table_add(const char *t)
>>      hdr.checksum = 0;    /* for checksum calculation */
>>  
>>      /* put header back */
>> -    memcpy(f, &hdr, sizeof(hdr));
>> +    memcpy(table_start, &hdr, sizeof(hdr));
>>  
>>      if (changed || !has_header || 1) {
>> -        ((struct acpi_table_header *)f)->checksum =
>> -            acpi_checksum((uint8_t *)f + ACPI_TABLE_PFX_SIZE, len);
>> +        ((struct acpi_table_header *)table_start)->checksum =
>> +            acpi_checksum((uint8_t *)table_start + ACPI_TABLE_PFX_SIZE, len);
> 
> Now that table_start is an unsigned char *, do you still need the cast
> to uint8_t*?

Hrmpf. Nope. Thankfully this code is going all away later on in the series.

Thanks for reviewing it!
Laszlo

Re: [Qemu-devel] [PATCH 04/11] qapi schema: add AcpiTableOptions

[Laszlo Ersek]

On 03/21/13 00:45, Eric Blake wrote:
> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  qapi-schema.json |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> No counterpart change to qmp-commands.hx showing a valid usage?

This is not a qmp command, but a productive (ie. by-design)
repurposement of the schema. Please see commit

    eb7ee2cb qapi: introduce OptsVisitor

and the somewhat heated
- http://thread.gmane.org/gmane.comp.emulators.qemu/193702/focus=194579
- http://thread.gmane.org/gmane.comp.emulators.qemu/193702/focus=194585


>> +#
>> +# @oem_id: #optional OEM identifier (6 bytes)
> 
> s/oem_id/oem-id/
> 
> In general, new QMP interfaces should use '-', not '_'.

Indeed! I think this warrants a respin.

>> +#
>> +# @file: #optional colon (:) separated list of pathnames to load and
>> +#        concatenate as table data. The resultant binary blob is expected to
>> +#        have an ACPI table header. At least one file is required. This field
>> +#        excludes @data.
>> +#
> 
> Ewwww.  This should be '*file' : [ 'str' ] (that is, use a JSON array of
> file names, not a single string).  If you have to reparse a JSON
> argument to break it into parts, then you are using the wrong interface;
> not to mention that I might (perversely) want to pass in a file name
> that contains a colon as part of its name.

Again (referring back to the links above), the schema here is structured
so that it accepts the same "-acpitable ..." command line options that
used to work before.

The parsing of that option pre-series, in acpi_table_add(), is "XXX
fixme: this function uses obsolete argument parsing interface". Since
I'm reworking that here, I think it's reasonable to fix that up as well.
The choice is between the traditional QemuOpts functions and
OptsVisitor. I'd like to benefit from the range validation built into
the latter, plus I'd like to demonstrate that OptsVisitor is usable.

(BTW OptsVisitor does support repeating option arguments, and it indeed
turns them into JSON lists, but utilizing that here would break the
current '-acpitable ...' format.)

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 04/11] qapi schema: add AcpiTableOptions

[Laszlo Ersek]

On 03/21/13 01:31, Laszlo Ersek wrote:
> On 03/21/13 00:45, Eric Blake wrote:
>> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:

>>> +#
>>> +# @oem_id: #optional OEM identifier (6 bytes)
>>
>> s/oem_id/oem-id/
>>
>> In general, new QMP interfaces should use '-', not '_'.
> 
> Indeed! I think this warrants a respin.

Actually it doesn't, apologies -- I got confused for a minute. Again,
since I aim to match the existing option format, I must keep the same
spelling.

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH 04/11] qapi schema: add AcpiTableOptions

[Paolo Bonzini]

Il 21/03/2013 11:41, Laszlo Ersek ha scritto:
> On 03/21/13 01:31, Laszlo Ersek wrote:
>> On 03/21/13 00:45, Eric Blake wrote:
>>> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
> 
>>>> +#
>>>> +# @oem_id: #optional OEM identifier (6 bytes)
>>>
>>> s/oem_id/oem-id/
>>>
>>> In general, new QMP interfaces should use '-', not '_'.
>>
>> Indeed! I think this warrants a respin.
> 
> Actually it doesn't, apologies -- I got confused for a minute. Again,
> since I aim to match the existing option format, I must keep the same
> spelling.

We should make a list of places where we have mixed conventions, accept
both, and mass-convert to dash...

Paolo

Re: [Qemu-devel] [PATCH 04/11] qapi schema: add AcpiTableOptions

[Michael S. Tsirkin]

On Thu, Mar 21, 2013 at 12:51:34PM +0100, Paolo Bonzini wrote:
> Il 21/03/2013 11:41, Laszlo Ersek ha scritto:
> > On 03/21/13 01:31, Laszlo Ersek wrote:
> >> On 03/21/13 00:45, Eric Blake wrote:
> >>> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
> > 
> >>>> +#
> >>>> +# @oem_id: #optional OEM identifier (6 bytes)
> >>>
> >>> s/oem_id/oem-id/
> >>>
> >>> In general, new QMP interfaces should use '-', not '_'.
> >>
> >> Indeed! I think this warrants a respin.
> > 
> > Actually it doesn't, apologies -- I got confused for a minute. Again,
> > since I aim to match the existing option format, I must keep the same
> > spelling.
> 
> We should make a list of places where we have mixed conventions, accept
> both, and mass-convert to dash...
> 
> Paolo

Anthony used to nack all mass conversions since they mess up the git
history. Since the movement of headers went in, I gather this position
has been relaxed...

-- 
MST

Re: [Qemu-devel] [PATCH 04/11] qapi schema: add AcpiTableOptions

[Laszlo Ersek]

On 03/21/13 13:36, Michael S. Tsirkin wrote:
> On Thu, Mar 21, 2013 at 12:51:34PM +0100, Paolo Bonzini wrote:
>> Il 21/03/2013 11:41, Laszlo Ersek ha scritto:
>>> On 03/21/13 01:31, Laszlo Ersek wrote:
>>>> On 03/21/13 00:45, Eric Blake wrote:
>>>>> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
>>>
>>>>>> +#
>>>>>> +# @oem_id: #optional OEM identifier (6 bytes)
>>>>>
>>>>> s/oem_id/oem-id/
>>>>>
>>>>> In general, new QMP interfaces should use '-', not '_'.
>>>>
>>>> Indeed! I think this warrants a respin.
>>>
>>> Actually it doesn't, apologies -- I got confused for a minute. Again,
>>> since I aim to match the existing option format, I must keep the same
>>> spelling.
>>
>> We should make a list of places where we have mixed conventions, accept
>> both, and mass-convert to dash...
>>
>> Paolo
> 
> Anthony used to nack all mass conversions since they mess up the git
> history. Since the movement of headers went in, I gather this position
> has been relaxed...
> 

Paolo, what are you suggesting precisely?

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 04/11] qapi schema: add AcpiTableOptions

[Paolo Bonzini]

Il 21/03/2013 13:42, Laszlo Ersek ha scritto:
> On 03/21/13 13:36, Michael S. Tsirkin wrote:
>> On Thu, Mar 21, 2013 at 12:51:34PM +0100, Paolo Bonzini wrote:
>>> Il 21/03/2013 11:41, Laszlo Ersek ha scritto:
>>>> On 03/21/13 01:31, Laszlo Ersek wrote:
>>>>> On 03/21/13 00:45, Eric Blake wrote:
>>>>>> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
>>>>
>>>>>>> +#
>>>>>>> +# @oem_id: #optional OEM identifier (6 bytes)
>>>>>>
>>>>>> s/oem_id/oem-id/
>>>>>>
>>>>>> In general, new QMP interfaces should use '-', not '_'.
>>>>>
>>>>> Indeed! I think this warrants a respin.
>>>>
>>>> Actually it doesn't, apologies -- I got confused for a minute. Again,
>>>> since I aim to match the existing option format, I must keep the same
>>>> spelling.
>>>
>>> We should make a list of places where we have mixed conventions, accept
>>> both, and mass-convert to dash...
>>>
>>> Paolo
>>
>> Anthony used to nack all mass conversions since they mess up the git
>> history. Since the movement of headers went in, I gather this position
>> has been relaxed...
>>
> 
> Paolo, what are you suggesting precisely?

Using something like

int strcmp_dash(const char *a, const char *b)
{
    char p, q;
    for (;; a++, b++) {
        p = *a == '_' ? '-' : *a;
        q = *b == '_' ? '-' : *b;
        a++, b++;
    } while (p == q && p != 0);
    return p - q;
}

in QemuOpts, the monitor, etc.  The mass conversion isn't really
necessary, just for cleanliness.

Paolo

Re: [Qemu-devel] [PATCH 00/11] build ACPI MADT for fw_cfg clients

[Laszlo Ersek]

On 03/21/13 00:23, Laszlo Ersek wrote:
> This series reworks the internals of the -acpitable command line option,
> and if that option is not specified, produces the APIC (MADT) table
> inside qemu, to be consumed over fw_cfg (alongside the DSDT).

> Laszlo Ersek (11):
>   strip some whitespace
>   change element type from "char" to "unsigned char" in ACPI table data
>   acpi_table_add(): report fatal errors through an internal Error
>     object
>   qapi schema: add AcpiTableOptions
>   acpi_table_add(): accept QemuOpts and parse it with OptsVisitor
>   acpi_table_add(): extract and reimplement internals
>   like acpi_table_install(), acpi_table_add() should propagate Errors
>   extract/unify the constant 0xfee00000 as APIC_DEFAULT_ADDRESS
>   Introduce IO_APIC_DEFAULT_ADDRESS for 0xfec00000
>   pc_acpi_init(): don't bail as soon as failing to find default DSDT
>   i386/pc: build ACPI MADT for fw_cfg clients

Any chance patches 01 to 09 could be considered? Esp. 06 which removes
an out-of-bounds access (an innocent-looking one, admittedly).

I'm OK too if the series is dropped (patch 11 was the main motivation,
but the interface that it extends was deemed unsuitable going forward on
the seabios list). I'd just like to hear the maintainer with
jurisdiction say the NAK. ("Too expensive even to review for too little
gain" is a good reason.)

Thanks
Laszlo

Re: [Qemu-devel] [PATCH 04/11] qapi schema: add AcpiTableOptions

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 21/03/2013 13:42, Laszlo Ersek ha scritto:
>> On 03/21/13 13:36, Michael S. Tsirkin wrote:
>>> On Thu, Mar 21, 2013 at 12:51:34PM +0100, Paolo Bonzini wrote:
>>>> Il 21/03/2013 11:41, Laszlo Ersek ha scritto:
>>>>> On 03/21/13 01:31, Laszlo Ersek wrote:
>>>>>> On 03/21/13 00:45, Eric Blake wrote:
>>>>>>> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
>>>>>
>>>>>>>> +#
>>>>>>>> +# @oem_id: #optional OEM identifier (6 bytes)
>>>>>>>
>>>>>>> s/oem_id/oem-id/
>>>>>>>
>>>>>>> In general, new QMP interfaces should use '-', not '_'.
>>>>>>
>>>>>> Indeed! I think this warrants a respin.
>>>>>
>>>>> Actually it doesn't, apologies -- I got confused for a minute. Again,
>>>>> since I aim to match the existing option format, I must keep the same
>>>>> spelling.
>>>>
>>>> We should make a list of places where we have mixed conventions, accept
>>>> both, and mass-convert to dash...
>>>>
>>>> Paolo
>>>
>>> Anthony used to nack all mass conversions since they mess up the git
>>> history. Since the movement of headers went in, I gather this position
>>> has been relaxed...
>>>
>> 
>> Paolo, what are you suggesting precisely?
>
> Using something like
>
> int strcmp_dash(const char *a, const char *b)
> {
>     char p, q;
>     for (;; a++, b++) {
>         p = *a == '_' ? '-' : *a;
>         q = *b == '_' ? '-' : *b;
>         a++, b++;
>     } while (p == q && p != 0);
>     return p - q;
> }
>
> in QemuOpts, the monitor, etc.  The mass conversion isn't really
> necessary, just for cleanliness.

I had thought we already did this for qemu-option.c but it doesn't
appear that we do...

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [PATCH 04/11] qapi schema: add AcpiTableOptions

[Anthony Liguori]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Thu, Mar 21, 2013 at 12:51:34PM +0100, Paolo Bonzini wrote:
>> Il 21/03/2013 11:41, Laszlo Ersek ha scritto:
>> > On 03/21/13 01:31, Laszlo Ersek wrote:
>> >> On 03/21/13 00:45, Eric Blake wrote:
>> >>> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
>> > 
>> >>>> +#
>> >>>> +# @oem_id: #optional OEM identifier (6 bytes)
>> >>>
>> >>> s/oem_id/oem-id/
>> >>>
>> >>> In general, new QMP interfaces should use '-', not '_'.
>> >>
>> >> Indeed! I think this warrants a respin.
>> > 
>> > Actually it doesn't, apologies -- I got confused for a minute. Again,
>> > since I aim to match the existing option format, I must keep the same
>> > spelling.
>> 
>> We should make a list of places where we have mixed conventions, accept
>> both, and mass-convert to dash...
>> 
>> Paolo
>
> Anthony used to nack all mass conversions since they mess up the git
> history. Since the movement of headers went in, I gather this position
> has been relaxed...

It's all about the value-to-cost ratio.  Running indent to remove
invisible space at the end of lines or sed'ing a way a '_t' just because
people like to quote standards too much offers very little value at a
high cost.

Significantly improving the code layout OTOH adds a lot of value and
justifies the impact.

Regards,

Anthony Liguori

>
> -- 
> MST

Re: [Qemu-devel] [PATCH 00/11] build ACPI MADT for fw_cfg clients

[Anthony Liguori]

Laszlo Ersek <lersek@redhat.com> writes:

> On 03/21/13 00:23, Laszlo Ersek wrote:
>> This series reworks the internals of the -acpitable command line option,
>> and if that option is not specified, produces the APIC (MADT) table
>> inside qemu, to be consumed over fw_cfg (alongside the DSDT).
>
>> Laszlo Ersek (11):
>>   strip some whitespace
>>   change element type from "char" to "unsigned char" in ACPI table data
>>   acpi_table_add(): report fatal errors through an internal Error
>>     object
>>   qapi schema: add AcpiTableOptions
>>   acpi_table_add(): accept QemuOpts and parse it with OptsVisitor
>>   acpi_table_add(): extract and reimplement internals
>>   like acpi_table_install(), acpi_table_add() should propagate Errors
>>   extract/unify the constant 0xfee00000 as APIC_DEFAULT_ADDRESS
>>   Introduce IO_APIC_DEFAULT_ADDRESS for 0xfec00000
>>   pc_acpi_init(): don't bail as soon as failing to find default DSDT
>>   i386/pc: build ACPI MADT for fw_cfg clients
>
> Any chance patches 01 to 09 could be considered? Esp. 06 which removes
> an out-of-bounds access (an innocent-looking one, admittedly).
>
> I'm OK too if the series is dropped (patch 11 was the main motivation,
> but the interface that it extends was deemed unsuitable going forward on
> the seabios list). I'd just like to hear the maintainer with
> jurisdiction say the NAK. ("Too expensive even to review for too little
> gain" is a good reason.)

The whole thing looks pretty nice to me.  I'll merge the full series in
a day or so unless anyone objects.

This is an under maintained area of QEMU so it's very nice to see
improvements!

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

>
> Thanks
> Laszlo

Re: [Qemu-devel] [PATCH 00/11] build ACPI MADT for fw_cfg clients

[Laszlo Ersek]

(adding Kevin)

On 04/03/13 22:05, Anthony Liguori wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
>> On 03/21/13 00:23, Laszlo Ersek wrote:
>>> This series reworks the internals of the -acpitable command line option,
>>> and if that option is not specified, produces the APIC (MADT) table
>>> inside qemu, to be consumed over fw_cfg (alongside the DSDT).
>>
>>> Laszlo Ersek (11):
>>>   strip some whitespace
>>>   change element type from "char" to "unsigned char" in ACPI table data
>>>   acpi_table_add(): report fatal errors through an internal Error
>>>     object
>>>   qapi schema: add AcpiTableOptions
>>>   acpi_table_add(): accept QemuOpts and parse it with OptsVisitor
>>>   acpi_table_add(): extract and reimplement internals
>>>   like acpi_table_install(), acpi_table_add() should propagate Errors
>>>   extract/unify the constant 0xfee00000 as APIC_DEFAULT_ADDRESS
>>>   Introduce IO_APIC_DEFAULT_ADDRESS for 0xfec00000
>>>   pc_acpi_init(): don't bail as soon as failing to find default DSDT
>>>   i386/pc: build ACPI MADT for fw_cfg clients
>>
>> Any chance patches 01 to 09 could be considered? Esp. 06 which removes
>> an out-of-bounds access (an innocent-looking one, admittedly).
>>
>> I'm OK too if the series is dropped (patch 11 was the main motivation,
>> but the interface that it extends was deemed unsuitable going forward on
>> the seabios list). I'd just like to hear the maintainer with
>> jurisdiction say the NAK. ("Too expensive even to review for too little
>> gain" is a good reason.)
> 
> The whole thing looks pretty nice to me.

That's awesome, thank you very much!

>  I'll merge the full series in
> a day or so unless anyone objects.

For transparency's sake: Kevin, this is where you'd object to patch 11:
it adds an MADT to the existing fw_cfg blob, which, combined with an
older (=current) SeaBIOS, leads to a duplicated MADT; see also the blurb
in 00/11 which quotes that from
<http://www.seabios.org/pipermail/seabios/2013-March/005960.html>. (I'm
adding this paragraph because you can argue this better; I yet have to
go through your detailed design
<http://www.seabios.org/pipermail/seabios/2013-March/006020.html> once
more.)

Anyway, if you could tolerate such a duplicate MADT for a very short
time, I'd send a qemu patch that reworks pc_acpi_madt() (in 11/11), such
that it's final call is not made to acpi_table_install() -- ie.
appending the MADT to the existing fw_cfg ACPI blob --, but installs it
as a separate fw_cfg file as you're suggesting (/etc/acpi/APIC or so) --
the existing "-acpitable" switch would still benefit from 01-10, and I'd
salvage the meat of 11/11 as well. I'd then attempt to write a SeaBIOS
patch too for /etc/acpi/APIC. Would that be acceptable for you?

> This is an under maintained area of QEMU so it's very nice to see
> improvements!
> 
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Can't thank enough for your quick response, Anthony.

Laszlo

Re: [Qemu-devel] [PATCH 00/11] build ACPI MADT for fw_cfg clients

[Kevin O'Connor]

On Thu, Apr 04, 2013 at 09:52:31AM +0200, Laszlo Ersek wrote:
> On 04/03/13 22:05, Anthony Liguori wrote:
> > Laszlo Ersek <lersek@redhat.com> writes:
> >> Any chance patches 01 to 09 could be considered? Esp. 06 which removes
> >> an out-of-bounds access (an innocent-looking one, admittedly).
> >>
> >> I'm OK too if the series is dropped (patch 11 was the main motivation,
> >> but the interface that it extends was deemed unsuitable going forward on
> >> the seabios list). I'd just like to hear the maintainer with
> >> jurisdiction say the NAK. ("Too expensive even to review for too little
> >> gain" is a good reason.)
> > 
> > The whole thing looks pretty nice to me.
> 
> That's awesome, thank you very much!
> 
> >  I'll merge the full series in
> > a day or so unless anyone objects.
> 
> For transparency's sake: Kevin, this is where you'd object to patch 11:
> it adds an MADT to the existing fw_cfg blob, which, combined with an
> older (=current) SeaBIOS, leads to a duplicated MADT; see also the blurb
> in 00/11 which quotes that from

Right.  I don't think we should commit patch 11 as that would cause
the current QEMU/SeaBIOS to incorrectly create two MADT tables.  We
should instead create the new MADT in a separate fw_cfg entry.

The other patches in the series look sane to me.

-Kevin

Re: [Qemu-devel] [PATCH 00/11] build ACPI MADT for fw_cfg clients

[Laszlo Ersek]

On 04/05/13 01:22, Kevin O'Connor wrote:
> On Thu, Apr 04, 2013 at 09:52:31AM +0200, Laszlo Ersek wrote:
>> On 04/03/13 22:05, Anthony Liguori wrote:
>>> Laszlo Ersek <lersek@redhat.com> writes:
>>>> Any chance patches 01 to 09 could be considered? Esp. 06 which removes
>>>> an out-of-bounds access (an innocent-looking one, admittedly).
>>>>
>>>> I'm OK too if the series is dropped (patch 11 was the main motivation,
>>>> but the interface that it extends was deemed unsuitable going forward on
>>>> the seabios list). I'd just like to hear the maintainer with
>>>> jurisdiction say the NAK. ("Too expensive even to review for too little
>>>> gain" is a good reason.)
>>>
>>> The whole thing looks pretty nice to me.
>>
>> That's awesome, thank you very much!
>>
>>>  I'll merge the full series in
>>> a day or so unless anyone objects.
>>
>> For transparency's sake: Kevin, this is where you'd object to patch 11:
>> it adds an MADT to the existing fw_cfg blob, which, combined with an
>> older (=current) SeaBIOS, leads to a duplicated MADT; see also the blurb
>> in 00/11 which quotes that from
> 
> Right.  I don't think we should commit patch 11 as that would cause
> the current QEMU/SeaBIOS to incorrectly create two MADT tables.  We
> should instead create the new MADT in a separate fw_cfg entry.
> 
> The other patches in the series look sane to me.

Anthony committed 01-10/11, I'm going to rework & post 11/11 as a
separate patch. Many thanks.

Laszlo

Re: [Qemu-devel] [PATCH 00/11] build ACPI MADT for fw_cfg clients

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori