From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44314) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QVADA-0002MC-Fq for qemu-devel@nongnu.org; Fri, 10 Jun 2011 18:29:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QVAD8-00060l-RP for qemu-devel@nongnu.org; Fri, 10 Jun 2011 18:29:52 -0400 Received: from isrv.corpit.ru ([86.62.121.231]:53959) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QVAD8-0005zX-Am for qemu-devel@nongnu.org; Fri, 10 Jun 2011 18:29:50 -0400 Message-ID: <4DF29AD2.2@msgid.tls.msk.ru> Date: Sat, 11 Jun 2011 02:29:38 +0400 From: Michael Tokarev MIME-Version: 1.0 References: <20110606083447.47A88527A@gandalf.tls.msk.ru> In-Reply-To: <20110606083447.47A88527A@gandalf.tls.msk.ru> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] v6 revamp acpitable parsing and allow to specify complete (headerful) table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Isaku Yamahata , Anthony Liguori I've given up on this one. Personally I don't need this stuff for my win7 guests since I can hack either bios or the O/S loader to include all the necessary verifications for the win7 activation to work. I tried to make this process to be legal (no hacks or "cracks" needed) and easy for others, but since noone is interested in this after 6 versions and 5 resends, I wont continue - what I am trying to achieve by pushing this so hard, after all? Thanks to everyone who gave (mostly code style) comments - at least I know the changes has been read by someone. Frustrated, /mjt 12.05.2011 18:44, 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. > > v5: rediffed against current qemu/master. > v6: fix one "} else {" coding style defect (noted by Blue Swirl) > > Signed-off-by: Michael Tokarev > --- > hw/acpi.c | 292 ++++++++++++++++++++++++++++++++----------------------- > qemu-options.hx | 7 +- > 2 files changed, 175 insertions(+), 124 deletions(-) > > diff --git a/hw/acpi.c b/hw/acpi.c > index ad40fb4..4316189 100644 > --- a/hw/acpi.c > +++ b/hw/acpi.c > @@ -22,17 +22,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; > > @@ -45,158 +57,192 @@ 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; > + > + 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'; > + 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; > + } > + > + 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; > + > } > > /* ACPI PM1a EVT */ > diff --git a/qemu-options.hx b/qemu-options.hx > index 82e085a..e639d5a 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -1014,12 +1014,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,