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 <m...@tls.msk.ru> > > Also > Reviewed-by: Isaku Yamahata <yamah...@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.