[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] v2 revamp acpitable parsing and allow to specify comple
|
From: |
Isaku Yamahata |
|
Subject: |
Re: [Qemu-devel] v2 revamp acpitable parsing and allow to specify complete (headerful) table |
|
Date: |
Thu, 17 Mar 2011 14:19:40 +0900 |
|
User-agent: |
Mutt/1.5.19 (2009-01-05) |
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 <address@hidden>
--- 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