Re: [Qemu-devel] [PATCH] v3 revamp acpitable parsing and allow to specify complete (headerful) table

[Gleb Natapov]

On Tue, Mar 29, 2011 at 10:41:11PM +0400, Michael Tokarev wrote:
> [Cc'ing Gleb since he - it seems - wrote the original code]
> 
> 17.03.2011 13:00, Michael Tokarev wrote:
> > 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.
> 
> Obviously after an almost complete rewrite, it's nice to have
> some way to ensure the new code does not break things.  I
> tested this by ensuring that the new code produces the same
> ACPI table as the old code did (modulo the new parameter).
> 
> I did a few (not all) combinations of various subparameters
> (sig=foo rev=N oem_id=bar  oem_table_id=baz etc), booting
> linux with old and new code and comparing the resulting
> table from /sys/firmware/acpi/tables/foo.
> 
> Later on, I took that fake table from within guest and
> specified it with -acpitable file=, and specified one or
> two extra fields and compared two tables - this file=
> plus added extra with the original way, replacing
> the added fields with their new values.  The result
> was the same.
> 
> With this patch we're one step closer (and one step
> left still) to be able to run the same pre-installed
> windows image either natively or in kvm/qemu this
> way:
> 
>   kvm -hda /dev/sda -acpitable file=/sys/firmware/acpi/tables/SLIC
> 
> (and choosing to boot windows installed on your hdd,
> obviously) -- windows activation/registration is not
> affected by running it in kvm - _provided_ both steps
> are completed, one is this acpitable thing and another
> is identification for seabios -- this one:
> http://article.gmane.org/gmane.comp.emulators.qemu/97348
> 
> > Signed-off-by: Michael Tokarev <address@hidden>
> 
> Also
> Reviewed-by: Isaku Yamahata <address@hidden>
> 
> Thanks!
> 
> >  hw/acpi.c       |  285 
> > +++++++++++++++++++++++++++++++------------------------
> >  qemu-options.hx |    7 +-
> >  2 files changed, 168 insertions(+), 124 deletions(-)
> > 
> > diff --git a/hw/acpi.c b/hw/acpi.c
> > index 237526d..4cc0311 100644
> > --- a/hw/acpi.c
> > +++ b/hw/acpi.c
> > @@ -15,6 +15,7 @@
> >   * You should have received a copy of the GNU Lesser General Public
> >   * License along with this library; if not, see 
> > <http://www.gnu.org/licenses/>
> >   */
> > +
> >  #include "hw.h"
> >  #include "pc.h"
> >  #include "acpi.h"
> > @@ -24,17 +25,29 @@
> >  
> >  struct acpi_table_header
> >  {
> > -    char signature [4];    /* ACPI signature (4 ASCII characters) */
> > +    uint16_t _length;         /* our length, not actual part of the hdr */
> > +                              /* 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_table_id [8]; /* OEM table identification */
> > +    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 */
> > +    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;
> >  
> > @@ -47,156 +60,182 @@ static int acpi_checksum(const uint8_t *data, int len)
> >      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;
> > -    struct acpi_table_header *acpi_hdr_p;
> > -    size_t off;
> > +    size_t len, start, allen;
> > +    bool has_header;
> > +    int changed;
> > +    int r;
> > +    struct acpi_table_header hdr;
> > +
> > +    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 {
> > +        has_header = false;
> > +        buf[0] = '\0';
> > +    }
Shouldn't we print some kind of error if user specify data and file
param? Otherwise looks good to me.

> > +
> > +    if (!acpi_tables) {
> > +        allen = sizeof(uint16_t);
> > +        acpi_tables = qemu_mallocz(allen);
> > +    }
> > +    else {
> > +        allen = acpi_tables_len;
> > +    }
> > +
> > +    start = allen;
> > +    acpi_tables = qemu_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 (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));
> > +            return -1;
> > +        }
> > +
> > +        for (;;) {
> > +            char data[8192];
> > +            r = read(fd, data, sizeof(data));
> > +            if (r == 0) {
> > +                break;
> > +            } else if (r > 0) {
> > +                acpi_tables = qemu_realloc(acpi_tables, allen + r);
> > +                memcpy(acpi_tables + allen, data, r);
> > +                allen += r;
> > +            } else if (errno != EINTR) {
> > +                fprintf(stderr, "can't read file %s: %s\n",
> > +                        f, strerror(errno));
> > +                close(fd);
> > +                return -1;
> > +            }
> > +        }
> > +
> > +        close(fd);
> > +    }
> > +
> > +    /* now fill in the header fields */
> > +
> > +    f = 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);
> > +
> > +    /* length of the table minus our prefix */
> > +    len = allen - start - ACPI_TABLE_PFX_SIZE;
> > +
> > +    hdr._length = cpu_to_le16(len);
> >  
> > -    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);
> > +        strzcpy(hdr.sig, buf, sizeof(hdr.sig));
> > +        ++changed;
> > +    }
> > +
> > +    /* 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 %u 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, 10);
> > -        if (val > 255 || *p != '\0')
> > -            goto out;
> > -    } else {
> > -        val = 1;
> > +        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;
> >      }
> > -    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);
> > +        strzcpy(hdr.oem_id, buf, sizeof(hdr.oem_id));
> > +        ++changed;
> >      }
> >  
> >      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);
> > +        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, 10);
> > -        if(*p != '\0')
> > -            goto out;
> > -    } else {
> > -        val = 1;
> > +        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;
> >      }
> > -    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);
> > +        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, 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;
> > +        val = strtol(buf, &p, 0);
> > +        if (*p) {
> > +            fprintf(stderr, "acpitable: \"%s=%s\" is invalid\n",
> > +                    "asl_compiler_rev", buf);
> > +            return -1;
> >          }
> > -        length += s.st_size;
> > -        if (!n)
> > -            break;
> > -        *n = ':';
> > -        f = n + 1;
> > +        hdr.asl_compiler_revision = cpu_to_le32(val);
> > +        ++changed;
> >      }
> >  
> > -    if (!acpi_tables) {
> > -        acpi_tables_len = sizeof(uint16_t);
> > -        acpi_tables = qemu_mallocz(acpi_tables_len);
> > +    if (!has_header && !changed) {
> > +        fprintf(stderr, "warning: acpitable: no table headers are 
> > specified\n");
> >      }
> > -    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;
> > -            }
> > -        }
> >  
> > -        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);
> > +    /* 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);
> >      }
> >  
> > -    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);
> > +
> > +    acpi_tables_len = allen;
> >      return 0;
> > -out:
> > -    if (acpi_tables) {
> > -        qemu_free(acpi_tables);
> > -        acpi_tables = NULL;
> > -    }
> > -    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,

--
                        Gleb.