| Message ID | 20110317101831.6F0CE1336C@gandalf.localdomain |
|---|---|
| State | New |
| Headers | show |
It looks good, except the unnecessary black line after return 0; Reviewed-by: Isaku Yamahata <yamahata@valinux.co.jp> On Thu, Mar 17, 2011 at 01:00:54PM +0300, 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. > > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > --- > 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'; > + } > + > + 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 [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.2.3 >
[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 <mjt@tls.msk.ru> Also Reviewed-by: Isaku Yamahata <yamahata@valinux.co.jp> 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'; > + } > + > + 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 [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,
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 <mjt@tls.msk.ru> > > Also > Reviewed-by: Isaku Yamahata <yamahata@valinux.co.jp> > > 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 [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, -- Gleb.
30.03.2011 18:26, Gleb Natapov wrote: > On Tue, Mar 29, 2011 at 10:41:11PM +0400, Michael Tokarev wrote: >> [Cc'ing Gleb since he - it seems - wrote the original code] Thank you for looking into this. >> 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. >>> [] >>> + 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. This whole thing needs to be replaced with proper option parsing. Currently, it completely ignores any unknown (such as mistyped) parameters among other things, or you can specify the same parameter twice without it noticing. So it needs more work when we'll decide which route to take with option parsing. I can rewrite it using QemuOpts, but I didn't want to because it appears their lifetime is very limited too. And sure, I can check for conflicting data= and file=, I just thought it's not really necessary having in mind all the above :) /mjt
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'; + } + + 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 [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,
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> --- hw/acpi.c | 285 +++++++++++++++++++++++++++++++------------------------ qemu-options.hx | 7 +- 2 files changed, 168 insertions(+), 124 deletions(-)