From patchwork Thu Mar 17 05:19:40 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Isaku Yamahata X-Patchwork-Id: 87336 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 4D65CB6FED for ; Thu, 17 Mar 2011 16:21:05 +1100 (EST) Received: from localhost ([127.0.0.1]:39527 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q05du-0005ar-I5 for incoming@patchwork.ozlabs.org; Thu, 17 Mar 2011 01:21:02 -0400 Received: from [140.186.70.92] (port=59306 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q05cg-0005Xw-2r for qemu-devel@nongnu.org; Thu, 17 Mar 2011 01:19:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q05ce-0005ac-1W for qemu-devel@nongnu.org; Thu, 17 Mar 2011 01:19:45 -0400 Received: from mail.valinux.co.jp ([210.128.90.3]:49867) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q05cd-0005ZY-Cd for qemu-devel@nongnu.org; Thu, 17 Mar 2011 01:19:43 -0400 Received: from ps.local.valinux.co.jp (vagw.valinux.co.jp [210.128.90.14]) by mail.valinux.co.jp (Postfix) with SMTP id 109D4188F8; Thu, 17 Mar 2011 14:19:41 +0900 (JST) Received: (nullmailer pid 2341 invoked by uid 1000); Thu, 17 Mar 2011 05:19:40 -0000 Date: Thu, 17 Mar 2011 14:19:40 +0900 From: Isaku Yamahata To: Michael Tokarev Subject: Re: [Qemu-devel] v2 revamp acpitable parsing and allow to specify complete (headerful) table Message-ID: <20110317051940.GI16841@valinux.co.jp> References: <20110316203927.6B9C11336C@gandalf.localdomain> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20110316203927.6B9C11336C@gandalf.localdomain> User-Agent: Mutt/1.5.19 (2009-01-05) X-Virus-Scanned: clamav-milter 0.95.2 at va-mail.local.valinux.co.jp X-Virus-Status: Clean X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-Received-From: 210.128.90.3 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 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 --- 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: