From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:41315) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QnCXH-0003xj-0Q for qemu-devel@nongnu.org; Sat, 30 Jul 2011 12:37:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QnCXF-0007qx-7z for qemu-devel@nongnu.org; Sat, 30 Jul 2011 12:37:10 -0400 Received: from server514d.exghost.com ([72.32.253.69]:3509 helo=server514.appriver.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QnCXE-0007qq-W9 for qemu-devel@nongnu.org; Sat, 30 Jul 2011 12:37:09 -0400 References: <20110606083447.47A88527A@gandalf.tls.msk.ru> <4DF29AD2.2@msgid.tls.msk.ru> <4E205A57.2040309@virtualcomputer.com> <20110729014943.GK14976@valinux.co.jp> Content-Transfer-Encoding: quoted-printable From: "John Baboval" Content-Type: text/plain; charset="us-ascii" In-Reply-To: Message-ID: <43A19669-DD59-4794-AC0C-2543529588E5@virtualcomputer.com> Date: Sat, 30 Jul 2011 09:37:31 -0700 MIME-Version: 1.0 (iPad Mail 8K2) 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: Blue Swirl Cc: Isaku Yamahata , Michael Tokarev , qemu-devel@nongnu.org, Anthony Liguori Thanks. Somehow completely missed Blue's response back on the 15th. Glad to see this= in. =20 When using this for SLIC I had to patch the OEM ID fields in the other table= s to match at runtime in order to make windows licensing happy. But thats a B= IOS change, not something in QEMU.... On Jul 30, 2011, at 2:41 AM, "Blue Swirl" wrote: > Thanks, applied. >=20 > On Fri, Jul 29, 2011 at 4:49 AM, Isaku Yamahata w= rote: >> On Fri, Jul 15, 2011 at 07:51:43PM +0300, Blue Swirl wrote: >>> On Fri, Jul 15, 2011 at 6:18 PM, John Baboval >>> wrote: >>>> Is there something I can do to help take this patch the rest of the way= ? >>>>=20 >>>> I'd hate to see it die because of a style issue and a compiler warning.= >>>=20 >>> There's also suspicious missing 'break' statement. How about fixing >>> the issues and submitting the patch? >>=20 >> I fixed the compile error. >> I think the missing break is intentional, so added an comment there. Mich= ael? >> Blue, can you please take a look of it? >>=20 >>=20 >> =46rom 9a5e4158074ea251ab064a946927bcaf861f5c1e Mon Sep 17 00:00:00 2001 >> Message-Id: <9a5e4158074ea251ab064a946927bcaf861f5c1e.1311903724.git.yama= hata@valinux.co.jp> >> From: Michael Tokarev >> Date: Thu, 12 May 2011 18:44:17 +0400 >> Subject: [PATCH] revamp acpitable parsing and allow to specify complete (= headerful) table >>=20 >> =46rom Michael Tokarev >>=20 >> 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=3D parameter, but at the same time >> it's still possible to specify header fields as before. >>=20 >> Now with the checkpatch.pl formatting fixes, thanks to >> Stefan Hajnoczi for suggestions, with changes from >> Isaku Yamahata, and with my further refinements. >>=20 >> Signed-off-by: Michael Tokarev >> Cc:: Isaku Yamahata >> Cc: John Baboval >> Cc: Blue Swirl >>=20 >> --- >> 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(-) >>=20 >> 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" >>=20 >> -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 hea= der */ >> uint8_t revision; /* ACPI Specification minor version # */ >> uint8_t checksum; /* To make sum of entire table =3D=3D 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)); >>=20 >> +#define ACPI_TABLE_HDR_SIZE sizeof(struct acpi_table_header) >> +#define ACPI_TABLE_PFX_SIZE sizeof(uint16_t) /* size of the extra prefi= x */ >> + >> +static const char dfl_hdr[ACPI_TABLE_HDR_SIZE] =3D >> + "\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; >>=20 >> @@ -40,163 +51,198 @@ static int acpi_checksum(const uint8_t *data, int l= en) >> { >> int sum, i; >> sum =3D 0; >> - for(i =3D 0; i < len; i++) >> + for (i =3D 0; i < len; i++) { >> sum +=3D data[i]; >> + } >> return (-sum) & 0xff; >> } >>=20 >> +/* like strncpy() but zero-fills the tail of destination */ >> +static void strzcpy(char *dst, const char *src, size_t size) >> +{ >> + size_t len =3D strlen(src); >> + if (len >=3D size) { >> + len =3D 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 =3D "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 =3D 0; >> + r |=3D get_param_value(buf, sizeof(buf), "data", t) ? 1 : 0; >> + r |=3D get_param_value(buf, sizeof(buf), "file", t) ? 2 : 0; >> + switch (r) { >> + case 0: >> + buf[0] =3D '\0'; >> + /* fallthrough for default behavior */ >> + case 1: >> + has_header =3D false; >> + break; >> + case 2: >> + has_header =3D true; >> + break; >> + default: >> + fprintf(stderr, "acpitable: both data and file are specified\n")= ; >> + return -1; >> + } >>=20 >> - 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 =3D sizeof(uint16_t); >> + acpi_tables =3D qemu_mallocz(allen); >> } else { >> - strncpy(acpi_hdr.signature, dfl_id, 4); >> + allen =3D acpi_tables_len; >> } >> + >> + start =3D allen; >> + acpi_tables =3D qemu_realloc(acpi_tables, start + ACPI_TABLE_HDR_SIZ= E); >> + allen +=3D has_header ? ACPI_TABLE_PFX_SIZE : ACPI_TABLE_HDR_SIZE; >> + >> + /* now read in the data files, reallocating buffer as needed */ >> + >> + for (f =3D strtok(buf, ":"); f; f =3D strtok(NULL, ":")) { >> + int fd =3D open(f, O_RDONLY); >> + >> + if (fd < 0) { >> + fprintf(stderr, "can't open file %s: %s\n", f, strerror(errn= o)); >> + return -1; >> + } >> + >> + for (;;) { >> + char data[8192]; >> + r =3D read(fd, data, sizeof(data)); >> + if (r =3D=3D 0) { >> + break; >> + } else if (r > 0) { >> + acpi_tables =3D qemu_realloc(acpi_tables, allen + r); >> + memcpy(acpi_tables + allen, data, r); >> + allen +=3D r; >> + } else if (errno !=3D 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 =3D acpi_tables + start; /* start of the table */ >> + changed =3D 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 =3D allen - start - ACPI_TABLE_PFX_SIZE; >> + >> + hdr._length =3D 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 =3D le32_to_cpu(hdr.length); >> + if (val !=3D 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 =3D cpu_to_le32(len); >> + >> if (get_param_value(buf, sizeof(buf), "rev", t)) { >> - val =3D strtoul(buf, &p, 10); >> - if (val > 255 || *p !=3D '\0') >> - goto out; >> - } else { >> - val =3D 1; >> + val =3D strtoul(buf, &p, 0); >> + if (val > 255 || *p) { >> + fprintf(stderr, "acpitable: \"rev=3D%s\" is invalid\n", buf)= ; >> + return -1; >> + } >> + hdr.revision =3D (uint8_t)val; >> + ++changed; >> } >> - acpi_hdr.revision =3D (int8_t)val; >>=20 >> 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; >> } >>=20 >> 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; >> } >>=20 >> if (get_param_value(buf, sizeof(buf), "oem_rev", t)) { >> - val =3D strtol(buf, &p, 10); >> - if(*p !=3D '\0') >> - goto out; >> - } else { >> - val =3D 1; >> + val =3D strtol(buf, &p, 0); >> + if (*p) { >> + fprintf(stderr, "acpitable: \"oem_rev=3D%s\" is invalid\n", b= uf); >> + return -1; >> + } >> + hdr.oem_revision =3D cpu_to_le32(val); >> + ++changed; >> } >> - acpi_hdr.oem_revision =3D cpu_to_le32(val); >>=20 >> 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; >> } >>=20 >> if (get_param_value(buf, sizeof(buf), "asl_compiler_rev", t)) { >> - val =3D strtol(buf, &p, 10); >> - if(*p !=3D '\0') >> - goto out; >> - } else { >> - val =3D 1; >> - } >> - acpi_hdr.asl_compiler_revision =3D cpu_to_le32(val); >> - >> - if (!get_param_value(buf, sizeof(buf), "data", t)) { >> - buf[0] =3D '\0'; >> - } >> - >> - length =3D sizeof(acpi_hdr); >> - >> - f =3D buf; >> - while (buf[0]) { >> - struct stat s; >> - char *n =3D strchr(f, ':'); >> - if (n) >> - *n =3D '\0'; >> - if(stat(f, &s) < 0) { >> - fprintf(stderr, "Can't stat file '%s': %s\n", f, strerror(er= rno)); >> - goto out; >> + val =3D strtol(buf, &p, 0); >> + if (*p) { >> + fprintf(stderr, "acpitable: \"%s=3D%s\" is invalid\n", >> + "asl_compiler_rev", buf); >> + return -1; >> } >> - length +=3D s.st_size; >> - if (!n) >> - break; >> - *n =3D ':'; >> - f =3D n + 1; >> + hdr.asl_compiler_revision =3D cpu_to_le32(val); >> + ++changed; >> } >>=20 >> - if (!acpi_tables) { >> - acpi_tables_len =3D sizeof(uint16_t); >> - acpi_tables =3D qemu_mallocz(acpi_tables_len); >> + if (!has_header && !changed) { >> + fprintf(stderr, "warning: acpitable: no table headers are specif= ied\n"); >> } >> - acpi_tables =3D qemu_realloc(acpi_tables, >> - acpi_tables_len + sizeof(uint16_t) + leng= th); >> - p =3D acpi_tables + acpi_tables_len; >> - acpi_tables_len +=3D sizeof(uint16_t) + length; >> - >> - *(uint16_t*)p =3D cpu_to_le32(length); >> - p +=3D sizeof(uint16_t); >> - memcpy(p, &acpi_hdr, sizeof(acpi_hdr)); >> - off =3D sizeof(acpi_hdr); >> - >> - f =3D buf; >> - while (buf[0]) { >> - struct stat s; >> - int fd; >> - char *n =3D strchr(f, ':'); >> - if (n) >> - *n =3D '\0'; >> - fd =3D open(f, O_RDONLY); >> - >> - if(fd < 0) >> - goto out; >> - if(fstat(fd, &s) < 0) { >> - close(fd); >> - goto out; >> - } >>=20 >> - /* off < length is necessary because file size can be changed >> - under our foot */ >> - while(s.st_size && off < length) { >> - int r; >> - r =3D read(fd, p + off, s.st_size); >> - if (r > 0) { >> - off +=3D r; >> - s.st_size -=3D r; >> - } else if ((r < 0 && errno !=3D EINTR) || r =3D=3D 0) { >> - close(fd); >> - goto out; >> - } >> - } >>=20 >> - close(fd); >> - if (!n) >> - break; >> - f =3D 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 =3D 0; /* for checksum calculation */ >> + >> + /* put header back */ >> + memcpy(f, &hdr, sizeof(hdr)); >> + >> + if (changed || !has_header || 1) { >> + ((struct acpi_table_header *)f)->checksum =3D >> + acpi_checksum((uint8_t *)f + ACPI_TABLE_PFX_SIZE, len); >> } >>=20 >> - acpi_hdr_p =3D (struct acpi_table_header*)p; >> - acpi_hdr_p->length =3D cpu_to_le32(length); >> - acpi_hdr_p->checksum =3D acpi_checksum((uint8_t*)p, length); >> /* increase number of tables */ >> - (*(uint16_t*)acpi_tables) =3D >> - cpu_to_le32(le32_to_cpu(*(uint16_t*)acpi_tables) + 1); >> + (*(uint16_t *)acpi_tables) =3D >> + cpu_to_le32(le32_to_cpu(*(uint16_t *)acpi_tables) + 1); >> + >> + acpi_tables_len =3D allen; >> return 0; >> -out: >> - if (acpi_tables) { >> - qemu_free(acpi_tables); >> - acpi_tables =3D NULL; >> - } >> - return -1; >> + >> } >>=20 >> /* 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), optionall= y with PCI address >> ETEXI >>=20 >> DEF("acpitable", HAS_ARG, QEMU_OPTION_acpitable, >> - "-acpitable [sig=3Dstr][,rev=3Dn][,oem_id=3Dstr][,oem_table_id=3Dstr= ][,oem_rev=3Dn][,asl_compiler_id=3Dstr][,asl_compiler_rev=3Dn][,data=3Dfile1= [:file2]...]\n" >> + "-acpitable [sig=3Dstr][,rev=3Dn][,oem_id=3Dstr][,oem_table_id=3Dstr= ][,oem_rev=3Dn][,asl_compiler_id=3Dstr][,asl_compiler_rev=3Dn][,{data|file}=3D= file1[:file2]...]\n" >> " ACPI table description\n", QEMU_ARCH_I386) >> STEXI >> @item -acpitable [sig=3D@var{str}][,rev=3D@var{n}][,oem_id=3D@var{str}][= ,oem_table_id=3D@var{str}][,oem_rev=3D@var{n}] [,asl_compiler_id=3D@var{str}= ][,asl_compiler_rev=3D@var{n}][,data=3D@var{file1}[:@var{file2}]...] >> @findex -acpitable >> Add ACPI table with specified header fields and context from specified f= iles. >> +For file=3D, take whole ACPI table from the specified files, including a= ll >> +ACPI headers (possible overridden by other options). >> +For data=3D, only data >> +portion of the table is used, all header information is specified in the= >> +command line. >> ETEXI >>=20 >> DEF("smbios", HAS_ARG, QEMU_OPTION_smbios, >> -- >> 1.7.1.1 >>=20 >>=20 >>=20 >> -- >> yamahata >>=20