Re: [Qemu-devel] [PATCH v2] Allow additions of ACPI tables from command line

[Anthony Liguori <aliguori@us.ibm.com>]

Gleb Natapov wrote:
> This is needed to dynamically add SLIC tables with Windows
> activation keys.
>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/hw/acpi.c b/hw/acpi.c
> index 4338d02..7175598 100644
> --- a/hw/acpi.c
> +++ b/hw/acpi.c
> @@ -561,3 +561,170 @@ void qemu_system_powerdown(void)
>      }
>  }
>  #endif
> +
> +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 */
> +};
>   

I don't have a lot of confidence that this table is going to be padded 
correctly by all compilers on all architectures.  I'd suggest explicit 
padding.

> +int acpi_table_add(const char *t)
> +{
> +    static const char *dfl_id = "QEMUQEMU";
> +    char buf[1024], *p, *f;
> +    struct acpi_table_header acpi_hdr;
> +    unsigned long val;
> +    size_t off;
> +
> +    memset(&acpi_hdr, 0, sizeof(acpi_hdr));
> +  
> +    if (get_param_value(buf, sizeof(buf), "sig", t)) {
> +        strncpy(acpi_hdr.signature, buf, 4);
> +    } else {
> +        strncpy(acpi_hdr.signature, dfl_id, 4);
> +    }
> +    if (get_param_value(buf, sizeof(buf), "rev", t)) {
> +        val = strtoul(buf, &p, 10);
> +        if (val > 255 || *p != '\0')
> +            goto out;
> +    } else {
> +        val = 1;
> +    }
> +    acpi_hdr.revision = (int8_t)val;
>   

You're filling this table out in host endianness, not guest endianness.  
The table gets passed directly to the guest's BIOS though.

> +    if (get_param_value(buf, sizeof(buf), "oem_id", t)) {
> +        strncpy(acpi_hdr.oem_id, buf, 6);
>   

is oem_id supposed to be NULL terminated or just NULL padded?

Regards,

Anthony Liguori