Re: [Qemu-devel] RFC: ACPI table loading

[Isaku Yamahata <yamahata@valinux.co.jp>]

The behavior seems reasonable. So I cleaned it up for upstream merge.

thanks,

>From cd336e50ebda63ecd84f8172dcf4d4456059d615 Mon Sep 17 00:00:00 2001
Message-Id: <cd336e50ebda63ecd84f8172dcf4d4456059d615.1300332741.git.yamahata@valinux.co.jp>
From: Isaku Yamahata <yamahata@valinux.co.jp>
Date: Thu, 17 Mar 2011 12:28:52 +0900
Subject: [PATCH] acpi: rewamp acpitable parsing, and allow specifying complete file with headers

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.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
Changes v2:
- rebased to 31d3c9b8c15d7b42f508d5fc2adc4abb7c732b70
- style fix
- eliminated unaligned access
- replace magic numbers with symbolic constants
- error report
---
 hw/acpi.c |  294 ++++++++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 184 insertions(+), 110 deletions(-)

diff --git a/hw/acpi.c b/hw/acpi.c
index 8071e7b..b058e20 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -32,6 +32,30 @@ struct acpi_table_header
     uint32_t asl_compiler_revision; /* ASL compiler revision number */
 } __attribute__((packed));
 
+#define ACPI_TABLE_OFF(x)       (offsetof(struct acpi_table_header, x))
+#define ACPI_TABLE_SIZE(x)      (sizeof(((struct acpi_table_header*)0)->x))
+
+#define ACPI_TABLE_SIG_OFF              ACPI_TABLE_OFF(signature)
+#define ACPI_TABLE_SIG_SIZE             ACPI_TABLE_SIZE(signature)
+#define ACPI_TABLE_LEN_OFF              ACPI_TABLE_OFF(length)
+#define ACPI_TABLE_LEN_SIZE             ACPI_TABLE_SIZE(length)
+#define ACPI_TABLE_REV_OFF              ACPI_TABLE_OFF(revision)
+#define ACPI_TABLE_REV_SIZE             ACPI_TABLE_SIZE(revision)
+#define ACPI_TABLE_CSUM_OFF             ACPI_TABLE_OFF(checksum)
+#define ACPI_TABLE_CSUM_SIZE            ACPI_TABLE_SIZE(checksum)
+#define ACPI_TABLE_OEM_ID_OFF           ACPI_TABLE_OFF(oem_id)
+#define ACPI_TABLE_OEM_ID_SIZE          ACPI_TABLE_SIZE(oem_id)
+#define ACPI_TABLE_OEM_TABLE_ID_OFF     ACPI_TABLE_OFF(oem_table_id)
+#define ACPI_TABLE_OEM_TABLE_ID_SIZE    ACPI_TABLE_SIZE(oem_table_id)
+#define ACPI_TABLE_OEM_REV_OFF          ACPI_TABLE_OFF(oem_revision)
+#define ACPI_TABLE_OEM_REV_SIZE         ACPI_TABLE_SIZE(oem_revision)
+#define ACPI_TABLE_ASL_COMPILER_ID_OFF  ACPI_TABLE_OFF(asl_compiler_id)
+#define ACPI_TABLE_ASL_COMPILER_ID_SIZE ACPI_TABLE_SIZE(asl_compiler_id)
+#define ACPI_TABLE_ASL_COMPILER_REV_OFF ACPI_TABLE_OFF(asl_compiler_revision)
+#define ACPI_TABLE_ASL_COMPILER_REV_SIZE ACPI_TABLE_SIZE(asl_compiler_revision)
+
+#define ACPI_TABLE_HDR_SIZE sizeof(struct acpi_table_header)
+
 char *acpi_tables;
 size_t acpi_tables_len;
 
@@ -44,156 +68,206 @@ 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)
 {
     static const char *dfl_id = "QEMUQEMU";
-    char buf[1024], *p, *f;
-    struct acpi_table_header acpi_hdr;
+    char buf[1024], *f, *p;
     unsigned long val;
-    uint32_t length;
-    struct acpi_table_header *acpi_hdr_p;
-    size_t off;
+    uint16_t val16;
+    uint32_t val32;
+    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);
+    if (get_param_value(buf, sizeof(buf), "data", t)) {
+        has_header = false;
+    } else if (get_param_value(buf, sizeof(buf), "file", t)) {
+        has_header = true;
     } else {
-        strncpy(acpi_hdr.signature, dfl_id, 4);
+        has_header = 0;
+        buf[0] = '\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) {
+        memset(acpi_tables + acpi_tables_len, 0, 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) {
+            fprintf(stderr, "can't open file %s: %s\n", f, strerror(errno));
+            goto out;
+        }
+
+        for(;;) {
+            char data[8192];
+            ssize_t 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) {
+                fprintf(stderr, "can't read file %s: %s\n",
+                        f, strerror(errno));
+                close(fd);
+                goto out;
+            }
+        }
+
+        close(fd);
+    }
+
+    /* fill in the complete length of the table */
+    len = acpi_tables_len - start - sizeof(uint16_t);
+    f = acpi_tables + start;
+    val16 = cpu_to_le16(len);
+    memcpy(f, &val16, sizeof(uint16_t));
+    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 + ACPI_TABLE_SIG_OFF, buf, ACPI_TABLE_SIG_SIZE);
+        ++changed;
+    } else if (!has_header) {
+        strncpy(f + ACPI_TABLE_SIG_OFF, dfl_id, ACPI_TABLE_SIG_SIZE);
+    }
+
+    /* 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, 10);
-        if (val > 255 || *p != '\0')
+        if (val > 255 || *p != '\0') {
+            fprintf(stderr, "invalid acpi rev.\n");
             goto out;
-    } else {
-        val = 1;
+        }
+        f[ACPI_TABLE_REV_OFF] = (uint8_t)val;
+        ++changed;
+    } else if (!has_header) {
+        f[ACPI_TABLE_REV_OFF] = 1;
     }
-    acpi_hdr.revision = (int8_t)val;
 
+    /* 9, checksum of entire table (1 byte)
+       this will be processed after all the headers are modified */
+
+    /* 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 + ACPI_TABLE_OEM_ID_OFF, buf, ACPI_TABLE_OEM_ID_SIZE);
+        ++changed;
+    } else if (!has_header) {
+        strncpy(f + ACPI_TABLE_OEM_ID_OFF, dfl_id, ACPI_TABLE_OEM_ID_SIZE);
     }
 
+    /* 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 + ACPI_TABLE_OEM_TABLE_ID_OFF, buf,
+                ACPI_TABLE_OEM_TABLE_ID_SIZE);
+        ++changed;
+    } else if (!has_header) {
+        strncpy(f + ACPI_TABLE_OEM_TABLE_ID_OFF, dfl_id,
+                ACPI_TABLE_OEM_TABLE_ID_SIZE);
     }
 
+    /* 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')
+        if(*p != '\0') {
+            fprintf(stderr, "invalid acpi oem_rev.\n");
             goto out;
-    } else {
-        val = 1;
+        }
+        val32 = cpu_to_le32(val);
+        memcpy(f + ACPI_TABLE_OEM_REV_OFF, &val32, ACPI_TABLE_OEM_REV_SIZE);
+        ++changed;
+    } else if (!has_header) {
+        val32 = cpu_to_le32(1);
+        memcpy(f + ACPI_TABLE_OEM_REV_OFF, &val32, ACPI_TABLE_OEM_REV_SIZE);
     }
-    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 + ACPI_TABLE_ASL_COMPILER_ID_OFF, buf,
+                ACPI_TABLE_ASL_COMPILER_ID_SIZE);
+        ++changed;
+    } else if (!has_header) {
+        strncpy(f + ACPI_TABLE_ASL_COMPILER_ID_OFF, dfl_id,
+                ACPI_TABLE_ASL_COMPILER_ID_SIZE);
     }
 
+    /* 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));
+        if(*p != '\0') {
+            fprintf(stderr, "invalid acpi asl_compiler_rev.\n");
             goto out;
         }
-        length += s.st_size;
-        if (!n)
-            break;
-        *n = ':';
-        f = n + 1;
-    }
+        val32 = cpu_to_le32(val);
+        memcpy(f + ACPI_TABLE_ASL_COMPILER_REV_OFF, &val32,
+               ACPI_TABLE_ASL_COMPILER_REV_SIZE);
+        ++changed;
+    } else if (!has_header) {
+        val32 = cpu_to_le32(1);
+        memcpy(f + ACPI_TABLE_ASL_COMPILER_REV_OFF, &val32,
+               ACPI_TABLE_ASL_COMPILER_REV_SIZE);
 
-    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;
-        }
 
-        /* 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;
-            }
+    /* 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");
+        }
+    } else {
+        /* check if actual length is correct */
+        memcpy(&val32, f + ACPI_TABLE_LEN_OFF, ACPI_TABLE_LEN_SIZE);
+        val = le32_to_cpu(val32);
+        if (val != len) {
+            fprintf(stderr,
+                    "warning: acpi table specified with file= has wrong length,"
+                    " header says %lu, actual size %zu\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 */
+    val32 = cpu_to_le32(len);
+    memcpy(f + ACPI_TABLE_LEN_OFF, &val32, ACPI_TABLE_LEN_SIZE);
+
+    /* 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[ACPI_TABLE_CSUM_OFF] = 0;
+        f[ACPI_TABLE_CSUM_OFF] = 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);
+        cpu_to_le16(le16_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;
 }
-- 
1.7.1.1




On Wed, Mar 16, 2011 at 04:47:01PM +0300, Michael Tokarev wrote:
> 16.03.2011 15:10, Isaku Yamahata wrote:
> > On Wed, Mar 16, 2011 at 01:12:27PM +0300, Michael Tokarev wrote:
> >> 16.03.2011 12:29, Isaku Yamahata wrote:
> 
> >>> Example:
> >>> qemu-system-x86_64 ... -M pc_q35 -acpitable 'load_header,data=roms/seabios/src/q35-acpi-dsdt.aml
> >>
> >> My question is unrelated to your q35 work, but I have a suggestion
> >> here: can we avoid this "load_header" thing please?  I hacked this
> >> area locally a while back while trying to run OEM-licensed windows
> >> with a SLIC table in BIOS, and wanted to come with alternative
> >> approach.  Now when you reminded me that again I'd rather finish
> >> that old thing and post a real patch...
> >>
> >> Here's my "idea".  First, if there's no other options provided
> >> except of data=..., just treat it as "headerful", ie, complete with
> >> the header.  Or alternatively (or at the same time), recognize
> >> "file=" the same way as "data=".  We can go even further and load
> >> file/data first and patch in the other header field if specified,
> >> so it'll be possible to overwrite only certain parts of the header
> >> but load the rest of the table (complete with all other headers)
> >> from a file.
> >>
> >> Does it make sense?
> > 
> > It sounds reasonable. As long as the patch is acceptable,
> > I'm willing to update the patch.
> > Let me summarize it. Your suggestion for -acpitable is
> >   
> >                 existing behavior               your suggested way
> > data= only
> > no sig=,...     header is filled with zero      headerful
> >                 (headerless)                    (new behaviour)
> >                 useless behavior
> > 
> > with sig=...    header is created               header is created
> >                 (headerless)                    (headerless)
> 
> I just implemented the whole thing, and refined it at the same
> time.  With data= it works the same way as before, so no
> new behavour is introduced.  Only with file= it is possible
> to specify whole table (with header) in a file, but other
> header fields specified on the command line (sig= etc) will
> work still, replacing the corresponding fields in the header
> read from the file.
> 
> Something like the below, it's just an RFC, but it appears
> to work.
> 
> /mjt
> 
> Subject: rewamp acpitable parsing, and allow specifying complete file with headers
> 
> 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.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> 
> diff --git a/hw/acpi.c b/hw/acpi.c
> index 237526d..d12527e 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;
> @@ -50,153 +40,220 @@ static int acpi_checksum(const uint8_t *data, int len)
>  int acpi_table_add(const char *t)
>  {
>      static const char *dfl_id = "QEMUQEMU";
> -    char buf[1024], *p, *f;
> -    struct acpi_table_header acpi_hdr;
> +    char buf[1024], *f, *p;
>      unsigned long val;
> -    uint32_t length;
> -    struct acpi_table_header *acpi_hdr_p;
> -    size_t off;
> -
> -    memset(&acpi_hdr, 0, sizeof(acpi_hdr));
> -
> -    if (get_param_value(buf, sizeof(buf), "sig", t)) {
> -        strncpy(acpi_hdr.signature, buf, 4);
> -    } else {
> -        strncpy(acpi_hdr.signature, dfl_id, 4);
> -    }
> -    if (get_param_value(buf, sizeof(buf), "rev", t)) {
> +    size_t len, start;
> +    bool has_header;
> +    int changed;
> +
> +    /*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 {
> +	has_header = 0;
> +	buf[0] = '\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)
> +    {
> +       memset(acpi_tables + acpi_tables_len, 0, 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;
> +	}
> +
> +	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);
> +    }
> +
> +    /* 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;
> +    }
> +    else if (!has_header)
> +    {
> +        strncpy(f + 0, dfl_id, 4);
> +    }
> +
> +    /* 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, 10);
>          if (val > 255 || *p != '\0')
> -            goto out;
> -    } else {
> -        val = 1;
> +            goto out;	/*XXX fixme: report error */
> +	f[8] = (uint8_t)val;
> +	++changed;
> +    }
> +    else if (!has_header)
> +    {
> +	f[8] = 1;
>      }
> -    acpi_hdr.revision = (int8_t)val;
> 
> -    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);
> +    /* 9, checksum of entire table (1 byte) */
> +
> +    /* 10..15 OEM identification (6 bytes) */
> +    if (get_param_value(buf, sizeof(buf), "oem_id", t))
> +    {
> +        strncpy(f + 10, buf, 6);
> +	++changed;
> +    }
> +    else if (!has_header)
> +    {
> +        strncpy(f + 10, 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);
> +    /* 16..23 OEM table identifiaction, 8 bytes */
> +    if (get_param_value(buf, sizeof(buf), "oem_table_id", t))
> +    {
> +        strncpy(f + 16, buf, 8);
> +	++changed;
> +    }
> +    else if (!has_header)
> +    {
> +        strncpy(f + 16, dfl_id, 8);
>      }
> 
> -    if (get_param_value(buf, sizeof(buf), "oem_rev", t)) {
> +    /* 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;
> +            goto out;	/*XXX fixme: report error */
> +	*(uint32_t*)(f + 24) = cpu_to_le32(val);
> +	++changed;
> +    } else if (!has_header)
> +    {
> +	*(uint32_t*)(f + 24) = cpu_to_le32(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);
> +    /* 28..31 ASL compiler vendor ID (4 bytes) */
> +    if (get_param_value(buf, sizeof(buf), "asl_compiler_id", t))
> +    {
> +        strncpy(f + 28, buf, 4);
> +        ++changed;
> +    }
> +    else if (!has_header)
> +    {
> +        strncpy(f + 28, dfl_id, 4);
>      }
> 
> -    if (get_param_value(buf, sizeof(buf), "asl_compiler_rev", t)) {
> +    /* 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;
> +            goto out;	/*XXX fixme: report error */
> +	*(uint32_t*)(f + 32) = cpu_to_le32(val);
> +	++changed;
>      }
> -    acpi_hdr.asl_compiler_revision = cpu_to_le32(val);
> -
> -    if (!get_param_value(buf, sizeof(buf), "data", t)) {
> -         buf[0] = '\0';
> +    else if (!has_header) {
> +	*(uint32_t*)(f + 32) = cpu_to_le32(1);
>      }
> 
> -    length = sizeof(acpi_hdr);
> +    /* 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");
> +    }
> +    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;
> +	}
> +    }
> 
> -    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);
> -        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);
> +    /* 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);
> +    }
> 
> -        if(fd < 0)
> -            goto out;
> -        if(fstat(fd, &s) < 0) {
> -            close(fd);
> -            goto out;
> -        }
> -
> -        /* 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;
> -            }
> -        }
> -
> -        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;
> -    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);
>      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 [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,
> 

-- 
yamahata