Re: [Qemu-devel] v2 revamp acpitable parsing and allow to specify complete (headerful) table

[Michael Tokarev <mjt@tls.msk.ru>]

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