From patchwork Wed Mar 16 13:47:01 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michael Tokarev X-Patchwork-Id: 87253 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 8DA61B6FD0 for ; Thu, 17 Mar 2011 00:48:23 +1100 (EST) Received: from localhost ([127.0.0.1]:58295 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pzr57-0006QK-GS for incoming@patchwork.ozlabs.org; Wed, 16 Mar 2011 09:48:09 -0400 Received: from [140.186.70.92] (port=53942 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pzr48-0006OH-9o for qemu-devel@nongnu.org; Wed, 16 Mar 2011 09:47:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pzr46-0005Kp-Gn for qemu-devel@nongnu.org; Wed, 16 Mar 2011 09:47:08 -0400 Received: from isrv.corpit.ru ([86.62.121.231]:49663) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pzr45-0005Kj-RL for qemu-devel@nongnu.org; Wed, 16 Mar 2011 09:47:06 -0400 Received: from [192.168.88.2] (mjt.vpn.tls.msk.ru [192.168.177.99]) by isrv.corpit.ru (Postfix) with ESMTP id AF68AA2670; Wed, 16 Mar 2011 16:47:03 +0300 (MSK) Message-ID: <4D80BF55.5010208@msgid.tls.msk.ru> Date: Wed, 16 Mar 2011 16:47:01 +0300 From: Michael Tokarev Organization: Telecom Service, JSC User-Agent: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.1.16) Gecko/20110307 Icedove/3.0.11 MIME-Version: 1.0 To: Isaku Yamahata Subject: [Qemu-devel] RFC: ACPI table loading References: <4D808D0B.302@msgid.tls.msk.ru> <20110316121039.GF16841@valinux.co.jp> In-Reply-To: <20110316121039.GF16841@valinux.co.jp> X-Enigmail-Version: 1.0.1 OpenPGP: id=804465C5 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 86.62.121.231 Cc: qemu-devel@nongnu.org X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 16.03.2011 15:10, Isaku Yamahata wrote: > On Wed, Mar 16, 2011 at 01:12:27PM +0300, Michael Tokarev wrote: >> 16.03.2011 12:29, Isaku Yamahata wrote: >>> Example: >>> qemu-system-x86_64 ... -M pc_q35 -acpitable 'load_header,data=roms/seabios/src/q35-acpi-dsdt.aml >> >> My question is unrelated to your q35 work, but I have a suggestion >> here: can we avoid this "load_header" thing please? I hacked this >> area locally a while back while trying to run OEM-licensed windows >> with a SLIC table in BIOS, and wanted to come with alternative >> approach. Now when you reminded me that again I'd rather finish >> that old thing and post a real patch... >> >> Here's my "idea". First, if there's no other options provided >> except of data=..., just treat it as "headerful", ie, complete with >> the header. Or alternatively (or at the same time), recognize >> "file=" the same way as "data=". We can go even further and load >> file/data first and patch in the other header field if specified, >> so it'll be possible to overwrite only certain parts of the header >> but load the rest of the table (complete with all other headers) >> from a file. >> >> Does it make sense? > > It sounds reasonable. As long as the patch is acceptable, > I'm willing to update the patch. > Let me summarize it. Your suggestion for -acpitable is > > existing behavior your suggested way > data= only > no sig=,... header is filled with zero headerful > (headerless) (new behaviour) > useless behavior > > with sig=... header is created header is created > (headerless) (headerless) I just implemented the whole thing, and refined it at the same time. With data= it works the same way as before, so no new behavour is introduced. Only with file= it is possible to specify whole table (with header) in a file, but other header fields specified on the command line (sig= etc) will work still, replacing the corresponding fields in the header read from the file. Something like the below, it's just an RFC, but it appears to work. /mjt Subject: rewamp acpitable parsing, and allow specifying complete file with headers 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. Signed-off-by: Michael Tokarev diff --git a/hw/acpi.c b/hw/acpi.c index 237526d..d12527e 100644 --- a/hw/acpi.c +++ b/hw/acpi.c @@ -22,18 +22,8 @@ #include "qemu-kvm.h" #include "string.h" -struct acpi_table_header -{ - char signature [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 */ - uint32_t oem_revision; /* OEM revision number */ - 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 (4+4+1+1+6+8+4+4+4) char *acpi_tables; size_t acpi_tables_len; @@ -50,153 +40,220 @@ static int acpi_checksum(const uint8_t *data, int len) int acpi_table_add(const char *t) { static const char *dfl_id = "QEMUQEMU"; - char buf[1024], *p, *f; - struct acpi_table_header acpi_hdr; + char buf[1024], *f, *p; unsigned long val; - uint32_t length; - struct acpi_table_header *acpi_hdr_p; - size_t off; - - 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); - } - if (get_param_value(buf, sizeof(buf), "rev", t)) { + size_t len, start; + bool has_header; + int changed; + + /*XXX fixme: this function uses obsolete argument parsing interface */ + /*XXX note: all 32bit accesses in there are misaligned */ + + if (get_param_value(buf, sizeof(buf), "data", t)) + { + has_header = 0; + } + else if (get_param_value(buf, sizeof(buf), "file", t)) + { + has_header = 1; + } + else { + has_header = 0; + buf[0] = '\0'; + } + + if (!acpi_tables) + { + acpi_tables_len = sizeof(uint16_t); + acpi_tables = qemu_mallocz(acpi_tables_len); + } + start = acpi_tables_len; + + len = sizeof(uint16_t) + ACPI_TABLE_HDR_SIZE; + acpi_tables = qemu_realloc(acpi_tables, acpi_tables_len + len); + acpi_tables_len += sizeof(uint16_t); + + if (!has_header) + { + memset(acpi_tables + acpi_tables_len, 0, ACPI_TABLE_HDR_SIZE); + acpi_tables_len += 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) + { + /*XXX fixme: report error */ + goto out; + } + + for(;;) + { + char data[8192]; + int r = read(fd, data, sizeof(data)); + if (r == 0) + { + break; + } + else if (r > 0) + { + acpi_tables = qemu_realloc(acpi_tables, acpi_tables_len + r); + memcpy(acpi_tables + acpi_tables_len, data, r); + acpi_tables_len += r; + } + else if (errno != EINTR) + { + /*XXX fixme: report error */ + close(fd); + goto out; + } + } + + close(fd); + } + + /* fill in the complete length of the table */ + len = acpi_tables_len - start - sizeof(uint16_t); + f = acpi_tables + start; + *(uint16_t*)f = cpu_to_le32(len); + f += sizeof(uint16_t); + + /* now fill in the header fields */ + changed = 0; + + /* 0..3, signature, string (4 bytes) */ + if (get_param_value(buf, sizeof(buf), "sig", t)) + { + strncpy(f + 0, buf, 4); + ++changed; + } + else if (!has_header) + { + strncpy(f + 0, dfl_id, 4); + } + + /* 4..7, length of the table, in bytes, including header (4 bytes) */ + + /* 8, ACPI specification minor version #, 1 byte */ + if (get_param_value(buf, sizeof(buf), "rev", t)) + { val = strtoul(buf, &p, 10); if (val > 255 || *p != '\0') - goto out; - } else { - val = 1; + goto out; /*XXX fixme: report error */ + f[8] = (uint8_t)val; + ++changed; + } + else if (!has_header) + { + f[8] = 1; } - 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); + /* 9, checksum of entire table (1 byte) */ + + /* 10..15 OEM identification (6 bytes) */ + if (get_param_value(buf, sizeof(buf), "oem_id", t)) + { + strncpy(f + 10, buf, 6); + ++changed; + } + else if (!has_header) + { + strncpy(f + 10, dfl_id, 6); } - 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); + /* 16..23 OEM table identifiaction, 8 bytes */ + if (get_param_value(buf, sizeof(buf), "oem_table_id", t)) + { + strncpy(f + 16, buf, 8); + ++changed; + } + else if (!has_header) + { + strncpy(f + 16, dfl_id, 8); } - if (get_param_value(buf, sizeof(buf), "oem_rev", t)) { + /* 24..27 OEM revision number, 4 bytes */ + if (get_param_value(buf, sizeof(buf), "oem_rev", t)) + { val = strtol(buf, &p, 10); if(*p != '\0') - goto out; - } else { - val = 1; + goto out; /*XXX fixme: report error */ + *(uint32_t*)(f + 24) = cpu_to_le32(val); + ++changed; + } else if (!has_header) + { + *(uint32_t*)(f + 24) = cpu_to_le32(1); } - 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); + /* 28..31 ASL compiler vendor ID (4 bytes) */ + if (get_param_value(buf, sizeof(buf), "asl_compiler_id", t)) + { + strncpy(f + 28, buf, 4); + ++changed; + } + else if (!has_header) + { + strncpy(f + 28, dfl_id, 4); } - if (get_param_value(buf, sizeof(buf), "asl_compiler_rev", t)) { + /* 32..35 ASL compiler revision number (4 bytes) */ + if (get_param_value(buf, sizeof(buf), "asl_compiler_rev", t)) + { val = strtol(buf, &p, 10); if(*p != '\0') - goto out; - } else { - val = 1; + goto out; /*XXX fixme: report error */ + *(uint32_t*)(f + 32) = cpu_to_le32(val); + ++changed; } - acpi_hdr.asl_compiler_revision = cpu_to_le32(val); - - if (!get_param_value(buf, sizeof(buf), "data", t)) { - buf[0] = '\0'; + else if (!has_header) { + *(uint32_t*)(f + 32) = cpu_to_le32(1); } - length = sizeof(acpi_hdr); + /* 4..7 length of the table including header, in bytes (4 bytes) */ + if (!has_header) + { + if (!changed) + fprintf(stderr, + "warning: acpi table specified with data=" + " but no table headers are provided, defaults are used\n"); + } + else + { + /* check if actual length is correct */ + val = le32_to_cpu(*(uint32_t*)(f + 4)); + if (val != len) + { + fprintf(stderr, + "warning: acpi table specified with file= has wrong length," + " header says %lu, actual size %u\n", + val, len); + ++changed; + } + } - 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; - } - length += s.st_size; - if (!n) - break; - *n = ':'; - f = n + 1; - } - - if (!acpi_tables) { - acpi_tables_len = sizeof(uint16_t); - acpi_tables = qemu_mallocz(acpi_tables_len); - } - 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); + /* fix table length */ + /* we may avoid putting length here if has_header is true */ + *(uint32_t*)(f + 4) = cpu_to_le32(len); + + /* 9 checksum (1 byte) */ + /* 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 */ + if (changed || !has_header || 1) + { + f[9] = 0; + f[9] = acpi_checksum((uint8_t*)f, len); + } - 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); - } - - 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); return 0; + out: - if (acpi_tables) { - qemu_free(acpi_tables); - acpi_tables = NULL; - } + acpi_tables_len = start; 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,