revamp acpitable parsing and allow to specify complete (headerful) table

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, with changes from
Isaku Yamahata, and with my further refinements.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
Cc: Isaku Yamahata <yamahata@valinux.co.jp>
Cc: John Baboval <john.baboval@virtualcomputer.com>
Cc: Blue Swirl <blauwirbel@gmail.com>
[yamahata@valinux.co.jp: fix compile error, comment fallthrough]
Signed-off-by: Blue Swirl <blauwirbel@gmail.com>

hw/acpi.c

@@ -20,19 +20,30 @@
 #include "pc.h"
 #include "acpi.h"
 
-struct acpi_table_header
+struct acpi_table_header {
-{
+    uint16_t _length;         /* our length, not actual part of the hdr */
-    char signature [4];    /* ACPI signature (4 ASCII characters) */
+                              /* XXX why we have 2 length fields here? */
+    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 # */
     uint8_t checksum;         /* To make sum of entire table == 0 */
-    char oem_id [6];       /* OEM identification */
+    char oem_id[6];           /* OEM identification */
-    char oem_table_id [8]; /* OEM table 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 */
+    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 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] =
+    "\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;
 size_t acpi_tables_len;
 
@@ -40,163 +51,198 @@ static int acpi_checksum(const uint8_t *data, int len)
 {
     int sum, i;
     sum = 0;
-    for(i = 0; i < len; i++)
+    for (i = 0; i < len; i++) {
         sum += data[i];
+    }
     return (-sum) & 0xff;
 }
 
+/* like strncpy() but zero-fills the tail of destination */
+static void strzcpy(char *dst, const char *src, size_t size)
+{
+    size_t len = strlen(src);
+    if (len >= size) {
+        len = size;
+    } else {
+      memset(dst + len, 0, size - len);
+    }
+    memcpy(dst, src, len);
+}
+
+/* XXX fixme: this function uses obsolete argument parsing interface */
 int acpi_table_add(const char *t)
 {
-    static const char *dfl_id = "QEMUQEMU";
     char buf[1024], *p, *f;
-    struct acpi_table_header acpi_hdr;
     unsigned long val;
-    uint32_t length;
+    size_t len, start, allen;
-    struct acpi_table_header *acpi_hdr_p;
+    bool has_header;
-    size_t off;
+    int changed;
+    int r;
+    struct acpi_table_header hdr;
 
-    memset(&acpi_hdr, 0, sizeof(acpi_hdr));
+    r = 0;
-  
+    r |= get_param_value(buf, sizeof(buf), "data", t) ? 1 : 0;
-    if (get_param_value(buf, sizeof(buf), "sig", t)) {
+    r |= get_param_value(buf, sizeof(buf), "file", t) ? 2 : 0;
-        strncpy(acpi_hdr.signature, buf, 4);
+    switch (r) {
-    } else {
+    case 0:
-        strncpy(acpi_hdr.signature, dfl_id, 4);
+        buf[0] = '\0';
-    }
+        /* fallthrough for default behavior */
-    if (get_param_value(buf, sizeof(buf), "rev", t)) {
+    case 1:
-        val = strtoul(buf, &p, 10);
+        has_header = false;
-        if (val > 255 || *p != '\0')
+        break;
-            goto out;
+    case 2:
-    } else {
+        has_header = true;
-        val = 1;
+        break;
-    }
+    default:
-    acpi_hdr.revision = (int8_t)val;
+        fprintf(stderr, "acpitable: both data and file are specified\n");
-
+        return -1;
-    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);
-    }
-
-    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);
-    }
-
-    if (get_param_value(buf, sizeof(buf), "oem_rev", t)) {
-        val = strtol(buf, &p, 10);
-        if(*p != '\0')
-            goto out;
-    } else {
-        val = 1;
-    }
-    acpi_hdr.oem_revision = cpu_to_le32(val);
-
-    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);
-    }
-
-    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;
-        }
-        length += s.st_size;
-        if (!n)
-            break;
-        *n = ':';
-        f = n + 1;
     }
 
     if (!acpi_tables) {
-        acpi_tables_len = sizeof(uint16_t);
+        allen = sizeof(uint16_t);
-        acpi_tables = qemu_mallocz(acpi_tables_len);
+        acpi_tables = qemu_mallocz(allen);
+    } else {
+        allen = 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);
+    start = allen;
-    p += sizeof(uint16_t);
+    acpi_tables = qemu_realloc(acpi_tables, start + ACPI_TABLE_HDR_SIZE);
-    memcpy(p, &acpi_hdr, sizeof(acpi_hdr));
+    allen += has_header ? ACPI_TABLE_PFX_SIZE : ACPI_TABLE_HDR_SIZE;
-    off = sizeof(acpi_hdr);
 
-    f = buf;
+    /* now read in the data files, reallocating buffer as needed */
-    while (buf[0]) {
-        struct stat s;
-        int fd;
-        char *n = strchr(f, ':');
-        if (n)
-            *n = '\0';
-        fd = open(f, O_RDONLY);
 
-        if(fd < 0)
+    for (f = strtok(buf, ":"); f; f = strtok(NULL, ":")) {
-            goto out;
+        int fd = open(f, O_RDONLY);
-        if(fstat(fd, &s) < 0) {
+
-            close(fd);
+        if (fd < 0) {
-            goto out;
+            fprintf(stderr, "can't open file %s: %s\n", f, strerror(errno));
+            return -1;
         }
 
-        /* off < length is necessary because file size can be changed
+        for (;;) {
-           under our foot */
+            char data[8192];
-        while(s.st_size && off < length) {
+            r = read(fd, data, sizeof(data));
-            int r;
+            if (r == 0) {
-            r = read(fd, p + off, s.st_size);
+                break;
-            if (r > 0) {
+            } else if (r > 0) {
-                off += r;
+                acpi_tables = qemu_realloc(acpi_tables, allen + r);
-                s.st_size -= r;
+                memcpy(acpi_tables + allen, data, r);
-            } else if ((r < 0 && errno != EINTR) || r == 0) {
+                allen += r;
+            } else if (errno != EINTR) {
+                fprintf(stderr, "can't read file %s: %s\n",
+                        f, strerror(errno));
                 close(fd);
-                goto out;
+                return -1;
             }
         }
 
         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);
     }
 
-    acpi_hdr_p = (struct acpi_table_header*)p;
+    /* now fill in the header fields */
-    acpi_hdr_p->length = cpu_to_le32(length);
+
-    acpi_hdr_p->checksum = acpi_checksum((uint8_t*)p, length);
+    f = acpi_tables + start;   /* start of the table */
-    /* increase number of tables */
+    changed = 0;
-    (*(uint16_t*)acpi_tables) =
+
-	    cpu_to_le32(le32_to_cpu(*(uint16_t*)acpi_tables) + 1);
+    /* copy the header to temp place to align the fields */
-    return 0;
+    memcpy(&hdr, has_header ? f : dfl_hdr, ACPI_TABLE_HDR_SIZE);
-out:
+
-    if (acpi_tables) {
+    /* length of the table minus our prefix */
-        qemu_free(acpi_tables);
+    len = allen - start - ACPI_TABLE_PFX_SIZE;
-        acpi_tables = NULL;
+
+    hdr._length = cpu_to_le16(len);
+
+    if (get_param_value(buf, sizeof(buf), "sig", t)) {
+        strzcpy(hdr.sig, buf, sizeof(hdr.sig));
+        ++changed;
     }
-    return -1;
+
+    /* length of the table including header, in bytes */
+    if (has_header) {
+        /* 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 (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;
+        }
+        hdr.revision = (uint8_t)val;
+        ++changed;
+    }
+
+    if (get_param_value(buf, sizeof(buf), "oem_id", t)) {
+        strzcpy(hdr.oem_id, buf, sizeof(hdr.oem_id));
+        ++changed;
+    }
+
+    if (get_param_value(buf, sizeof(buf), "oem_table_id", t)) {
+        strzcpy(hdr.oem_table_id, buf, sizeof(hdr.oem_table_id));
+        ++changed;
+    }
+
+    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;
+        }
+        hdr.oem_revision = cpu_to_le32(val);
+        ++changed;
+    }
+
+    if (get_param_value(buf, sizeof(buf), "asl_compiler_id", t)) {
+        strzcpy(hdr.asl_compiler_id, buf, sizeof(hdr.asl_compiler_id));
+        ++changed;
+    }
+
+    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;
+        }
+        hdr.asl_compiler_revision = cpu_to_le32(val);
+        ++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(f, &hdr, sizeof(hdr));
+
+    if (changed || !has_header || 1) {
+        ((struct acpi_table_header *)f)->checksum =
+            acpi_checksum((uint8_t *)f + 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;
+    return 0;
+
 }
 
 /* ACPI PM1a EVT */

qemu-options.hx

@@ -1074,12 +1074,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 [sig=@var{str}][,rev=@var{n}][,oem_id=@var{str}][,oem_table_id=@var{str}][,oem_rev=@var{n}] [,asl_compiler_id=@var{str}][,asl_compiler_rev=@var{n}][,data=@var{file1}[:@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,