diff mbox

v2 revamp acpitable parsing and allow to specify complete (headerful) table

Message ID 20110317051940.GI16841@valinux.co.jp
State New
Headers show

Commit Message

Isaku Yamahata March 17, 2011, 5:19 a.m. UTC 
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>

Comments

Michael Tokarev March 17, 2011, 8:21 a.m. UTC | #1 
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
diff mbox

Patch

--- 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: