Thanks.
Somehow completely missed Blue's response back on the 15th. Glad to see this
in.
When using this for SLIC I had to patch the OEM ID fields in the other tables
to match at runtime in order to make windows licensing happy. But thats a BIOS
change, not something in QEMU....
On Jul 30, 2011, at 2:41 AM, "Blue Swirl" <blauwir...@gmail.com> wrote:
> Thanks, applied.
>
> On Fri, Jul 29, 2011 at 4:49 AM, Isaku Yamahata <yamah...@valinux.co.jp>
> wrote:
>> On Fri, Jul 15, 2011 at 07:51:43PM +0300, Blue Swirl wrote:
>>> On Fri, Jul 15, 2011 at 6:18 PM, John Baboval
>>> <john.babo...@virtualcomputer.com> wrote:
>>>> Is there something I can do to help take this patch the rest of the way?
>>>>
>>>> I'd hate to see it die because of a style issue and a compiler warning.
>>>
>>> There's also suspicious missing 'break' statement. How about fixing
>>> the issues and submitting the patch?
>>
>> I fixed the compile error.
>> I think the missing break is intentional, so added an comment there. Michael?
>> Blue, can you please take a look of it?
>>
>>
>> From 9a5e4158074ea251ab064a946927bcaf861f5c1e Mon Sep 17 00:00:00 2001
>> Message-Id:
>> <9a5e4158074ea251ab064a946927bcaf861f5c1e.1311903724.git.yamah...@valinux.co.jp>
>> From: Michael Tokarev <m...@tls.msk.ru>
>> Date: Thu, 12 May 2011 18:44:17 +0400
>> Subject: [PATCH] revamp acpitable parsing and allow to specify complete
>> (headerful) table
>>
>> From Michael Tokarev <m...@tls.msk.ru>
>>
>> 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 <m...@tls.msk.ru>
>> Cc:: Isaku Yamahata <yamah...@valinux.co.jp>
>> Cc: John Baboval <john.babo...@virtualcomputer.com>
>> Cc: Blue Swirl <blauwir...@gmail.com>
>>
>> ---
>> v5: rediffed against current qemu/master.
>> v6: fix one "} else {" coding style defect (noted by Blue Swirl)
>> v7: style fix and added an comment for suspicious break
>> I think that the missing break of case 0 is intentional.
>> I added the fallthrough comment there.
>> ---
>> hw/acpi.c | 298
>> ++++++++++++++++++++++++++++++++-----------------------
>> qemu-options.hx | 7 +-
>> 2 files changed, 178 insertions(+), 127 deletions(-)
>>
>> diff --git a/hw/acpi.c b/hw/acpi.c
>> index ad40fb4..79ec66c 100644
>> --- a/hw/acpi.c
>> +++ b/hw/acpi.c
>> @@ -20,19 +20,30 @@
>> #include "pc.h"
>> #include "acpi.h"
>>
>> -struct acpi_table_header
>> -{
>> - char signature [4]; /* ACPI signature (4 ASCII characters) */
>> +struct acpi_table_header {
>> + 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;
>>
>> @@ -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;
>> - 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;
>> +
>> + 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:
>> + fprintf(stderr, "acpitable: both data and file are specified\n");
>> + return -1;
>> + }
>>
>> - memset(&acpi_hdr, 0, sizeof(acpi_hdr));
>> -
>> - if (get_param_value(buf, sizeof(buf), "sig", t)) {
>> - strncpy(acpi_hdr.signature, buf, 4);
>> + if (!acpi_tables) {
>> + allen = sizeof(uint16_t);
>> + acpi_tables = qemu_mallocz(allen);
>> } else {
>> - strncpy(acpi_hdr.signature, dfl_id, 4);
>> + 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);
>> +
>> + if (get_param_value(buf, sizeof(buf), "sig", t)) {
>> + 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 %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, 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;
>> +
>> }
>>
>> /* ACPI PM1a EVT */
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 1d57f64..74c0edb 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -1062,12 +1062,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,
>> --
>> 1.7.1.1
>>
>>
>>
>> --
>> yamahata
>>
