[Qemu-devel] v2 revamp acpitable parsing and allow to specify complete (headerful) table

[Michael Tokarev]

This patch almost rewrites acpi_table_add() function
(but still leaves it using old get_param_value() interface).
The result is that it's now possible to specify whole table
(together with a header) in an external file, instead of just
data portion, with a new file= parameter, but at the same time
it's still possible to specify header fields as before.

Now with the checkpatch.pl formatting fixes, thanks to
Stefan Hajnoczi for suggestions.

Signed-off-by: Michael Tokarev <address@hidden>

 hw/acpi.c       |  269 +++++++++++++++++++++++++++++--------------------------
 qemu-options.hx |    7 ++-
 2 files changed, 147 insertions(+), 129 deletions(-)

diff --git a/hw/acpi.c b/hw/acpi.c
index 237526d..34ded76 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -22,18 +22,8 @@
 #include "qemu-kvm.h"
 #include "string.h"
 
-struct acpi_table_header
-{
-    char signature [4];    /* ACPI signature (4 ASCII characters) */
-    uint32_t length;          /* Length of table, in bytes, including header */
-    uint8_t revision;         /* ACPI Specification minor version # */
-    uint8_t checksum;         /* To make sum of entire table == 0 */
-    char oem_id [6];       /* OEM identification */
-    char oem_table_id [8]; /* OEM table identification */
-    uint32_t oem_revision;    /* OEM revision number */
-    char asl_compiler_id [4]; /* ASL compiler vendor ID */
-    uint32_t asl_compiler_revision; /* ASL compiler revision number */
-} __attribute__((packed));
+
+#define ACPI_TABLE_HDR_SIZE (4+4+1+1+6+8+4+4+4)
 
 char *acpi_tables;
 size_t acpi_tables_len;
@@ -49,154 +39,177 @@ static int acpi_checksum(const uint8_t *data, int len)
 
 int acpi_table_add(const char *t)
 {
-    static const char *dfl_id = "QEMUQEMU";
+    static const char dfl_hdr[ACPI_TABLE_HDR_SIZE] =
+        "QEMU\0\0\0\0\1\0"       /* sig (4), len(4), revno (1), csum (1) */
+        "QEMUQUQEMUQEMU\1\0\0\0" /* OEM id (6), table (8), revno (4) */
+        "QEMU\1\0\0\0"           /* ASL compiler ID (4), version (4) */
+        ;
     char buf[1024], *p, *f;
-    struct acpi_table_header acpi_hdr;
     unsigned long val;
-    uint32_t length;
-    struct acpi_table_header *acpi_hdr_p;
-    size_t off;
+    size_t len, start;
+    bool has_header;
+    int changed;
 
-    memset(&acpi_hdr, 0, sizeof(acpi_hdr));
-  
-    if (get_param_value(buf, sizeof(buf), "sig", t)) {
-        strncpy(acpi_hdr.signature, buf, 4);
+    /*XXX fixme: this function uses obsolete argument parsing interface */
+    /*XXX note: all 32bit accesses in there are misaligned */
+
+    if (get_param_value(buf, sizeof(buf), "data", t)) {
+        has_header = 0;
+    } else if (get_param_value(buf, sizeof(buf), "file", t)) {
+        has_header = 1;
     } else {
-        strncpy(acpi_hdr.signature, dfl_id, 4);
+        has_header = 0;
+        buf[0] = '\0';
     }
-    if (get_param_value(buf, sizeof(buf), "rev", t)) {
-        val = strtoul(buf, &p, 10);
-        if (val > 255 || *p != '\0')
+
+    if (!acpi_tables) {
+        acpi_tables_len = sizeof(uint16_t);
+        acpi_tables = qemu_mallocz(acpi_tables_len);
+    }
+
+    start = acpi_tables_len;
+    len = sizeof(uint16_t) + ACPI_TABLE_HDR_SIZE;
+    acpi_tables = qemu_realloc(acpi_tables, acpi_tables_len + len);
+    acpi_tables_len += sizeof(uint16_t);
+
+    if (!has_header) {
+        memcpy(acpi_tables + acpi_tables_len, dfl_hdr, ACPI_TABLE_HDR_SIZE);
+        acpi_tables_len += ACPI_TABLE_HDR_SIZE;
+    }
+
+    /* now read in the data files, reallocating buffer as needed */
+
+    for (f = strtok(buf, ":"); f; f = strtok(NULL, ":")) {
+        int fd = open(f, O_RDONLY);
+
+        if (fd < 0) {
+            /*XXX fixme: report error */
             goto out;
-    } else {
-        val = 1;
+        }
+
+        for (;;) {
+            char data[8192];
+            int r = read(fd, data, sizeof(data));
+            if (r == 0) {
+                break;
+            } else if (r > 0) {
+                acpi_tables = qemu_realloc(acpi_tables, acpi_tables_len + r);
+                memcpy(acpi_tables + acpi_tables_len, data, r);
+                acpi_tables_len += r;
+            } else if (errno != EINTR) {
+                /*XXX fixme: report error */
+                close(fd);
+                goto out;
+            }
+        }
+
+        close(fd);
     }
-    acpi_hdr.revision = (int8_t)val;
 
+    /* fill in the complete length of the table */
+    len = acpi_tables_len - start - sizeof(uint16_t);
+    f = acpi_tables + start;
+    *(uint16_t *)f = cpu_to_le32(len);
+    f += sizeof(uint16_t);
+
+    /* now fill in the header fields */
+    changed = 0;
+
+    /* 0..3, signature, string (4 bytes) */
+    if (get_param_value(buf, sizeof(buf), "sig", t)) {
+        strncpy(f + 0, buf, 4);
+        ++changed;
+    }
+
+    /* 4..7, length of the table, in bytes, including header (4 bytes) */
+
+    /* 8, ACPI specification minor version #, 1 byte */
+    if (get_param_value(buf, sizeof(buf), "rev", t)) {
+        val = strtoul(buf, &p, 0);
+        if (val > 255 || *p) {
+            goto out;   /*XXX fixme: report error */
+        }
+        f[8] = (uint8_t)val;
+        ++changed;
+    }
+
+    /* 9, checksum of entire table (1 byte) */
+
+    /* 10..15 OEM identification (6 bytes) */
     if (get_param_value(buf, sizeof(buf), "oem_id", t)) {
-        strncpy(acpi_hdr.oem_id, buf, 6);
-    } else {
-        strncpy(acpi_hdr.oem_id, dfl_id, 6);
+        strncpy(f + 10, buf, 6);
+        ++changed;
     }
 
+    /* 16..23 OEM table identifiaction, 8 bytes */
     if (get_param_value(buf, sizeof(buf), "oem_table_id", t)) {
-        strncpy(acpi_hdr.oem_table_id, buf, 8);
-    } else {
-        strncpy(acpi_hdr.oem_table_id, dfl_id, 8);
+        strncpy(f + 16, buf, 8);
+        ++changed;
     }
 
+    /* 24..27 OEM revision number, 4 bytes */
     if (get_param_value(buf, sizeof(buf), "oem_rev", t)) {
-        val = strtol(buf, &p, 10);
-        if(*p != '\0')
-            goto out;
-    } else {
-        val = 1;
+        val = strtol(buf, &p, 0);
+        if (*p) {
+            goto out;   /*XXX fixme: report error */
+        }
+        *(uint32_t *)(f + 24) = cpu_to_le32(val);
+        ++changed;
     }
-    acpi_hdr.oem_revision = cpu_to_le32(val);
 
+    /* 28..31 ASL compiler vendor ID (4 bytes) */
     if (get_param_value(buf, sizeof(buf), "asl_compiler_id", t)) {
-        strncpy(acpi_hdr.asl_compiler_id, buf, 4);
-    } else {
-        strncpy(acpi_hdr.asl_compiler_id, dfl_id, 4);
+        strncpy(f + 28, buf, 4);
+        ++changed;
     }
 
+    /* 32..35 ASL compiler revision number (4 bytes) */
     if (get_param_value(buf, sizeof(buf), "asl_compiler_rev", t)) {
         val = strtol(buf, &p, 10);
-        if(*p != '\0')
-            goto out;
-    } else {
-        val = 1;
-    }
-    acpi_hdr.asl_compiler_revision = cpu_to_le32(val);
-    
-    if (!get_param_value(buf, sizeof(buf), "data", t)) {
-         buf[0] = '\0';
-    }
-
-    length = sizeof(acpi_hdr);
-
-    f = buf;
-    while (buf[0]) {
-        struct stat s;
-        char *n = strchr(f, ':');
-        if (n)
-            *n = '\0';
-        if(stat(f, &s) < 0) {
-            fprintf(stderr, "Can't stat file '%s': %s\n", f, strerror(errno));
-            goto out;
+        if (*p) {
+            goto out;   /*XXX fixme: report error */
         }
-        length += s.st_size;
-        if (!n)
-            break;
-        *n = ':';
-        f = n + 1;
+        *(uint32_t *)(f + 32) = cpu_to_le32(val);
+        ++changed;
     }
 
-    if (!acpi_tables) {
-        acpi_tables_len = sizeof(uint16_t);
-        acpi_tables = qemu_mallocz(acpi_tables_len);
-    }
-    acpi_tables = qemu_realloc(acpi_tables,
-                               acpi_tables_len + sizeof(uint16_t) + length);
-    p = acpi_tables + acpi_tables_len;
-    acpi_tables_len += sizeof(uint16_t) + length;
-
-    *(uint16_t*)p = cpu_to_le32(length);
-    p += sizeof(uint16_t);
-    memcpy(p, &acpi_hdr, sizeof(acpi_hdr));
-    off = sizeof(acpi_hdr);
-
-    f = buf;
-    while (buf[0]) {
-        struct stat s;
-        int fd;
-        char *n = strchr(f, ':');
-        if (n)
-            *n = '\0';
-        fd = open(f, O_RDONLY);
-
-        if(fd < 0)
-            goto out;
-        if(fstat(fd, &s) < 0) {
-            close(fd);
-            goto out;
+    /* 4..7 length of the table including header, in bytes (4 bytes) */
+    if (!has_header) {
+        if (!changed) {
+            fprintf(stderr,
+                "warning: acpi table specified with data="
+                " but no table headers are provided, defaults are used\n");
         }
-
-        /* off < length is necessary because file size can be changed
-           under our foot */
-        while(s.st_size && off < length) {
-            int r;
-            r = read(fd, p + off, s.st_size);
-            if (r > 0) {
-                off += r;
-                s.st_size -= r;
-            } else if ((r < 0 && errno != EINTR) || r == 0) {
-                close(fd);
-                goto out;
-            }
+    } else {
+        /* check if actual length is correct */
+        val = le32_to_cpu(*(uint32_t *)(f + 4));
+        if (val != len) {
+            fprintf(stderr,
+                "warning: acpi table specified with file= has wrong length,"
+                " header says %lu, actual size %u\n",
+                val, len);
+            ++changed;
         }
-
-        close(fd);
-        if (!n)
-            break;
-        f = n + 1;
     }
-    if (off < length) {
-        /* don't pass random value in process to guest */
-        memset(p + off, 0, length - off);
+
+    /* fix table length */
+    /* we may avoid putting length here if has_header is true */
+    *(uint32_t *)(f + 4) = cpu_to_le32(len);
+
+    /* 9 checksum (1 byte) */
+    /* 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 */
+    if (changed || !has_header || 1) {
+        f[9] = 0;
+        f[9] = acpi_checksum((uint8_t *)f, len);
     }
 
-    acpi_hdr_p = (struct acpi_table_header*)p;
-    acpi_hdr_p->length = cpu_to_le32(length);
-    acpi_hdr_p->checksum = acpi_checksum((uint8_t*)p, length);
     /* increase number of tables */
-    (*(uint16_t*)acpi_tables) =
-           cpu_to_le32(le32_to_cpu(*(uint16_t*)acpi_tables) + 1);
+    (*(uint16_t *)acpi_tables) =
+            cpu_to_le32(le32_to_cpu(*(uint16_t *)acpi_tables) + 1);
     return 0;
+
 out:
-    if (acpi_tables) {
-        qemu_free(acpi_tables);
-        acpi_tables = NULL;
-    }
+    acpi_tables_len = start;
     return -1;
 }
diff --git a/qemu-options.hx b/qemu-options.hx
index 18f54d2..e1d26b4 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -995,12 +995,17 @@ Enable virtio balloon device (default), optionally with 
PCI address
 ETEXI
 
 DEF("acpitable", HAS_ARG, QEMU_OPTION_acpitable,
-    "-acpitable 
[sig=str][,rev=n][,oem_id=str][,oem_table_id=str][,oem_rev=n][,asl_compiler_id=str][,asl_compiler_rev=n][,data=file1[:file2]...]\n"
+    "-acpitable 
[sig=str][,rev=n][,oem_id=str][,oem_table_id=str][,oem_rev=n][,asl_compiler_id=str][,asl_compiler_rev=n][,{data|file}=file1[:file2]...]\n"
     "                ACPI table description\n", QEMU_ARCH_I386)
 STEXI
 @item -acpitable 
address@hidden,address@hidden,address@hidden,address@hidden,address@hidden 
[,address@hidden,address@hidden,address@hidden:@var{file2}]...]
 @findex -acpitable
 Add ACPI table with specified header fields and context from specified files.
+For file=, take whole ACPI table from the specified files, including all
+ACPI headers (possible overridden by other options).
+For data=, only data
+portion of the table is used, all header information is specified in the
+command line.
 ETEXI
 
 DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,