* [Qemu-devel] v2 revamp acpitable parsing and allow to specify complete (headerful) table
@ 2011-03-16 20:39 Michael Tokarev
2011-03-17 5:19 ` Isaku Yamahata
0 siblings, 1 reply; 3+ messages in thread
From: Michael Tokarev @ 2011-03-16 20:39 UTC (permalink / raw)
To: qemu-devel
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.
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
hw/acpi.c | 269 +++++++++++++++++++++++++++++--------------------------
qemu-options.hx | 7 ++-
2 files changed, 147 insertions(+), 129 deletions(-)
diff --git a/hw/acpi.c b/hw/acpi.c
index 237526d..34ded76 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;
@@ -49,154 +39,177 @@ static int acpi_checksum(const uint8_t *data, int len)
int acpi_table_add(const char *t)
{
- static const char *dfl_id = "QEMUQEMU";
+ static const char dfl_hdr[ACPI_TABLE_HDR_SIZE] =
+ "QEMU\0\0\0\0\1\0" /* sig (4), len(4), revno (1), csum (1) */
+ "QEMUQUQEMUQEMU\1\0\0\0" /* OEM id (6), table (8), revno (4) */
+ "QEMU\1\0\0\0" /* ASL compiler ID (4), version (4) */
+ ;
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;
+ bool has_header;
+ int changed;
- memset(&acpi_hdr, 0, sizeof(acpi_hdr));
-
- if (get_param_value(buf, sizeof(buf), "sig", t)) {
- strncpy(acpi_hdr.signature, buf, 4);
+ /*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 {
- strncpy(acpi_hdr.signature, dfl_id, 4);
+ has_header = 0;
+ buf[0] = '\0';
}
- if (get_param_value(buf, sizeof(buf), "rev", t)) {
- val = strtoul(buf, &p, 10);
- if (val > 255 || *p != '\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) {
+ memcpy(acpi_tables + acpi_tables_len, dfl_hdr, 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;
- } else {
- val = 1;
+ }
+
+ 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);
}
- acpi_hdr.revision = (int8_t)val;
+ /* 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;
+ }
+
+ /* 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, 0);
+ if (val > 255 || *p) {
+ goto out; /*XXX fixme: report error */
+ }
+ f[8] = (uint8_t)val;
+ ++changed;
+ }
+
+ /* 9, checksum of entire table (1 byte) */
+
+ /* 10..15 OEM identification (6 bytes) */
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);
+ strncpy(f + 10, buf, 6);
+ ++changed;
}
+ /* 16..23 OEM table identifiaction, 8 bytes */
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);
+ strncpy(f + 16, buf, 8);
+ ++changed;
}
+ /* 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;
+ val = strtol(buf, &p, 0);
+ if (*p) {
+ goto out; /*XXX fixme: report error */
+ }
+ *(uint32_t *)(f + 24) = cpu_to_le32(val);
+ ++changed;
}
- acpi_hdr.oem_revision = cpu_to_le32(val);
+ /* 28..31 ASL compiler vendor ID (4 bytes) */
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);
+ strncpy(f + 28, buf, 4);
+ ++changed;
}
+ /* 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;
- }
- 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;
+ if (*p) {
+ goto out; /*XXX fixme: report error */
}
- length += s.st_size;
- if (!n)
- break;
- *n = ':';
- f = n + 1;
+ *(uint32_t *)(f + 32) = cpu_to_le32(val);
+ ++changed;
}
- 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);
-
- if(fd < 0)
- goto out;
- if(fstat(fd, &s) < 0) {
- close(fd);
- goto out;
+ /* 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");
}
-
- /* 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;
- }
+ } 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;
}
-
- 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);
+
+ /* 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);
}
- 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);
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,
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] v2 revamp acpitable parsing and allow to specify complete (headerful) table
2011-03-16 20:39 [Qemu-devel] v2 revamp acpitable parsing and allow to specify complete (headerful) table Michael Tokarev
@ 2011-03-17 5:19 ` Isaku Yamahata
2011-03-17 8:21 ` Michael Tokarev
0 siblings, 1 reply; 3+ messages in thread
From: Isaku Yamahata @ 2011-03-17 5:19 UTC (permalink / raw)
To: Michael Tokarev; +Cc: qemu-devel
Ouch. You already clean it up.
Here is my diff from your patch. Can you please merged into the patch?
changes are
- eliminate unaligned access
- error report
- replace magic number with symbolic constants
- unconverted strtol(base=10)
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
--- qemu-acpi-load-other-0/hw/acpi.c 2011-03-17 14:02:07.000000000 +0900
+++ qemu-acpi-load-0/hw/acpi.c 2011-03-17 14:14:39.000000000 +0900
@@ -19,8 +19,42 @@
#include "pc.h"
#include "acpi.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_OFF(x) (offsetof(struct acpi_table_header, x))
+#define ACPI_TABLE_SIZE(x) (sizeof(((struct acpi_table_header*)0)->x))
+
+#define ACPI_TABLE_SIG_OFF ACPI_TABLE_OFF(signature)
+#define ACPI_TABLE_SIG_SIZE ACPI_TABLE_SIZE(signature)
+#define ACPI_TABLE_LEN_OFF ACPI_TABLE_OFF(length)
+#define ACPI_TABLE_LEN_SIZE ACPI_TABLE_SIZE(length)
+#define ACPI_TABLE_REV_OFF ACPI_TABLE_OFF(revision)
+#define ACPI_TABLE_REV_SIZE ACPI_TABLE_SIZE(revision)
+#define ACPI_TABLE_CSUM_OFF ACPI_TABLE_OFF(checksum)
+#define ACPI_TABLE_CSUM_SIZE ACPI_TABLE_SIZE(checksum)
+#define ACPI_TABLE_OEM_ID_OFF ACPI_TABLE_OFF(oem_id)
+#define ACPI_TABLE_OEM_ID_SIZE ACPI_TABLE_SIZE(oem_id)
+#define ACPI_TABLE_OEM_TABLE_ID_OFF ACPI_TABLE_OFF(oem_table_id)
+#define ACPI_TABLE_OEM_TABLE_ID_SIZE ACPI_TABLE_SIZE(oem_table_id)
+#define ACPI_TABLE_OEM_REV_OFF ACPI_TABLE_OFF(oem_revision)
+#define ACPI_TABLE_OEM_REV_SIZE ACPI_TABLE_SIZE(oem_revision)
+#define ACPI_TABLE_ASL_COMPILER_ID_OFF ACPI_TABLE_OFF(asl_compiler_id)
+#define ACPI_TABLE_ASL_COMPILER_ID_SIZE ACPI_TABLE_SIZE(asl_compiler_id)
+#define ACPI_TABLE_ASL_COMPILER_REV_OFF ACPI_TABLE_OFF(asl_compiler_revision)
+#define ACPI_TABLE_ASL_COMPILER_REV_SIZE ACPI_TABLE_SIZE(asl_compiler_revision)
-#define ACPI_TABLE_HDR_SIZE (4+4+1+1+6+8+4+4+4)
+#define ACPI_TABLE_HDR_SIZE sizeof(struct acpi_table_header)
char *acpi_tables;
size_t acpi_tables_len;
@@ -34,6 +68,7 @@ static int acpi_checksum(const uint8_t *
return (-sum) & 0xff;
}
+/* XXX fixme: this function uses obsolete argument parsing interface */
int acpi_table_add(const char *t)
{
static const char dfl_hdr[ACPI_TABLE_HDR_SIZE] =
@@ -43,17 +78,16 @@ int acpi_table_add(const char *t)
;
char buf[1024], *p, *f;
unsigned long val;
+ uint16_t val16;
+ uint32_t val32;
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;
+ has_header = false;
} else if (get_param_value(buf, sizeof(buf), "file", t)) {
- has_header = 1;
+ has_header = true;
} else {
has_header = 0;
buf[0] = '\0';
@@ -80,7 +114,7 @@ int acpi_table_add(const char *t)
int fd = open(f, O_RDONLY);
if (fd < 0) {
- /*XXX fixme: report error */
+ fprintf(stderr, "can't open file %s: %s\n", f, strerror(errno));
goto out;
}
@@ -94,7 +128,8 @@ int acpi_table_add(const char *t)
memcpy(acpi_tables + acpi_tables_len, data, r);
acpi_tables_len += r;
} else if (errno != EINTR) {
- /*XXX fixme: report error */
+ fprintf(stderr, "can't read file %s: %s\n",
+ f, strerror(errno));
close(fd);
goto out;
}
@@ -106,7 +141,8 @@ int acpi_table_add(const char *t)
/* 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);
+ val16 = cpu_to_le16(len);
+ memcpy(f, &val16, sizeof(uint16_t));
f += sizeof(uint16_t);
/* now fill in the header fields */
@@ -114,7 +150,7 @@ int acpi_table_add(const char *t)
/* 0..3, signature, string (4 bytes) */
if (get_param_value(buf, sizeof(buf), "sig", t)) {
- strncpy(f + 0, buf, 4);
+ strncpy(f + ACPI_TABLE_SIG_OFF, buf, ACPI_TABLE_SIG_SIZE);
++changed;
}
@@ -124,23 +160,26 @@ int acpi_table_add(const char *t)
if (get_param_value(buf, sizeof(buf), "rev", t)) {
val = strtoul(buf, &p, 0);
if (val > 255 || *p) {
- goto out; /*XXX fixme: report error */
+ fprintf(stderr, "invalid acpi rev.\n");
+ goto out;
}
- f[8] = (uint8_t)val;
+ f[ACPI_TABLE_REV_OFF] = (uint8_t)val;
++changed;
}
- /* 9, checksum of entire table (1 byte) */
+ /* 9, checksum of entire table (1 byte)
+ this will be processed after all the headers are modified */
/* 10..15 OEM identification (6 bytes) */
if (get_param_value(buf, sizeof(buf), "oem_id", t)) {
- strncpy(f + 10, buf, 6);
+ strncpy(f + ACPI_TABLE_OEM_ID_OFF, buf, ACPI_TABLE_OEM_ID_SIZE);
++changed;
}
/* 16..23 OEM table identifiaction, 8 bytes */
if (get_param_value(buf, sizeof(buf), "oem_table_id", t)) {
- strncpy(f + 16, buf, 8);
+ strncpy(f + ACPI_TABLE_OEM_TABLE_ID_OFF, buf,
+ ACPI_TABLE_OEM_TABLE_ID_SIZE);
++changed;
}
@@ -148,25 +187,31 @@ int acpi_table_add(const char *t)
if (get_param_value(buf, sizeof(buf), "oem_rev", t)) {
val = strtol(buf, &p, 0);
if (*p) {
- goto out; /*XXX fixme: report error */
+ fprintf(stderr, "invalid acpi oem_rev.\n");
+ goto out;
}
- *(uint32_t *)(f + 24) = cpu_to_le32(val);
+ val32 = cpu_to_le32(val);
+ memcpy(f + ACPI_TABLE_OEM_REV_OFF, &val32, ACPI_TABLE_OEM_REV_SIZE);
++changed;
}
/* 28..31 ASL compiler vendor ID (4 bytes) */
if (get_param_value(buf, sizeof(buf), "asl_compiler_id", t)) {
- strncpy(f + 28, buf, 4);
+ strncpy(f + ACPI_TABLE_ASL_COMPILER_ID_OFF, buf,
+ ACPI_TABLE_ASL_COMPILER_ID_SIZE);
++changed;
}
/* 32..35 ASL compiler revision number (4 bytes) */
if (get_param_value(buf, sizeof(buf), "asl_compiler_rev", t)) {
- val = strtol(buf, &p, 10);
+ val = strtol(buf, &p, 0);
if (*p) {
- goto out; /*XXX fixme: report error */
+ fprintf(stderr, "invalid acpi asl_compiler_rev.\n");
+ goto out;
}
- *(uint32_t *)(f + 32) = cpu_to_le32(val);
+ val32 = cpu_to_le32(val);
+ memcpy(f + ACPI_TABLE_ASL_COMPILER_REV_OFF, &val32,
+ ACPI_TABLE_ASL_COMPILER_REV_SIZE);
++changed;
}
@@ -179,11 +224,12 @@ int acpi_table_add(const char *t)
}
} else {
/* check if actual length is correct */
- val = le32_to_cpu(*(uint32_t *)(f + 4));
+ memcpy(&val32, f + ACPI_TABLE_LEN_OFF, ACPI_TABLE_LEN_SIZE);
+ val = le32_to_cpu(val32);
if (val != len) {
fprintf(stderr,
"warning: acpi table specified with file= has wrong length,"
- " header says %lu, actual size %u\n",
+ " header says %lu, actual size %zu\n",
val, len);
++changed;
}
@@ -191,19 +237,20 @@ int acpi_table_add(const char *t)
/* fix table length */
/* we may avoid putting length here if has_header is true */
- *(uint32_t *)(f + 4) = cpu_to_le32(len);
+ val32 = cpu_to_le32(len);
+ memcpy(f + ACPI_TABLE_LEN_OFF, &val32, ACPI_TABLE_LEN_SIZE);
/* 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);
+ f[ACPI_TABLE_CSUM_OFF] = 0;
+ f[ACPI_TABLE_CSUM_OFF] = acpi_checksum((uint8_t*)f, len);
}
/* 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_le16(le16_to_cpu(*(uint16_t*)acpi_tables) + 1);
return 0;
out:
--
yamahata
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] v2 revamp acpitable parsing and allow to specify complete (headerful) table
2011-03-17 5:19 ` Isaku Yamahata
@ 2011-03-17 8:21 ` Michael Tokarev
0 siblings, 0 replies; 3+ messages in thread
From: Michael Tokarev @ 2011-03-17 8:21 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: qemu-devel
17.03.2011 08:19, Isaku Yamahata wrote:
> Ouch. You already clean it up.
Please excuse me for this. My first try was just an RFC to show the
"basic idea" - as if it's so much large idea :), it wasn't my intention
to ask for comments about the code itself, I said "_something_ of
this theme". And I should have Cc'd you on my real submission too,
obviously, which I somehow overlooked. I should be more careful.
> Here is my diff from your patch. Can you please merged into the patch?
> changes are
> - eliminate unaligned access
> - error report
I intentionally did not implement reporting, because it needs to be
done using generic "error reporting" infrastructure which I haven't
learned yet ;)
> - replace magic number with symbolic constants
> - unconverted strtol(base=10)
Aha, one more case which I didn't spot, thanks! :)
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
>
> --- qemu-acpi-load-other-0/hw/acpi.c 2011-03-17 14:02:07.000000000 +0900
> +++ qemu-acpi-load-0/hw/acpi.c 2011-03-17 14:14:39.000000000 +0900
> @@ -19,8 +19,42 @@
> #include "pc.h"
> #include "acpi.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_OFF(x) (offsetof(struct acpi_table_header, x))
> +#define ACPI_TABLE_SIZE(x) (sizeof(((struct acpi_table_header*)0)->x))
> +
> +#define ACPI_TABLE_SIG_OFF ACPI_TABLE_OFF(signature)
> +#define ACPI_TABLE_SIG_SIZE ACPI_TABLE_SIZE(signature)
Um. This is jus too much. I've a better idea, with less code: after
loading the table, do a memcpy() into a local acpi_table_header
structure, perform all stuff there without the need of these #defines,
and copy it back at the end right before the checksum.
This way, we eliminate the defines, eliminate unaligned access, and
eliminate the need for these uint16 variables too.
I'll cook it in a few minutes, is that ok?
It's just too much (IMHO anyway) for such a little function which
is used so unfrequently :)
> /* 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_le16(le16_to_cpu(*(uint16_t*)acpi_tables) + 1);
> return 0;
This is something about which checkpatch.pl complains, telling
there should be a space before "*" in (uint16_t*) ;)
Thank you!
/mjt
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-03-17 8:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-16 20:39 [Qemu-devel] v2 revamp acpitable parsing and allow to specify complete (headerful) table Michael Tokarev
2011-03-17 5:19 ` Isaku Yamahata
2011-03-17 8:21 ` Michael Tokarev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).