Re: [Qemu-devel] [PATCH v5 13/20] hw/acpi/aml-build: Add ToUUID macro

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 15 Apr 2015 21:25:02 +0800
Shannon Zhao <zhaoshenglong@huawei.com> wrote:

> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Add ToUUID macro, this is useful for generating PCIe ACPI table.
> 
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  hw/acpi/aml-build.c         | 40 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/aml-build.h |  1 +
>  2 files changed, 41 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index b99bb13..316d5a5 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -25,6 +25,7 @@
>  #include <stdbool.h>
>  #include <string.h>
>  #include "hw/acpi/aml-build.h"
> +#include "qemu-common.h"
why do you need this hunk?

>  #include "qemu/bswap.h"
>  #include "qemu/bitops.h"
>  #include "hw/acpi/bios-linker-loader.h"
> @@ -948,6 +949,45 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
>                               addr_trans, len, flags);
>  }
>  
> +/*
> + * ACPI 3.0: 17.5.124 ToUUID (Convert String to UUID Macro)
> + * e.g. UUID: E5C937D0-3553-4d7a-9117-EA4D19C3434D
> + * call aml_touuid("E5C937D0-3553-4d7a-9117-EA4D19C3434D");
> + */
> +Aml *aml_touuid(const char *uuid)
> +{
> +    int i;
> +    long int val;
unsigned long long int ???


> +    char *end;
> +    const char *start = uuid;
> +    Aml *UUID = aml_buffer();
s/UUID/var/

> +
> +    val = strtol(start, &end, 16);
may be use strtoull()

> +    g_assert((end - start) == 8);
> +    build_append_int_noprefix(UUID->buf, val, 4);
> +    start = end + 1;
> +    val = strtol(start, &end, 16);
> +    g_assert((end - start) == 4);
> +    build_append_int_noprefix(UUID->buf, val, 2);
> +    start = end + 1;
> +    val = strtol(start, &end, 16);
> +    g_assert((end - start) == 4);
> +    build_append_int_noprefix(UUID->buf, val, 2);
this corresponds to -gghh- part of UUID according to spec
it would be better if you add pattern mentioned in spec
in this function and then put comments marking places
which handle specific part of it. 

> +    start = end + 1;
> +    val = strtol(start, &end, 16);
> +    g_assert((end - start) == 4);
> +    build_append_int_noprefix(UUID->buf, (val >> 8) & 0xFF, 1);
> +    build_append_int_noprefix(UUID->buf, val & 0xFF, 1);
add comment to it that according to spec bytes here are flipped around
that's why special treatment.

> +    start = end + 1;
> +    val = strtol(start, &end, 16);
> +    g_assert((end - start) == 12);
> +    for (i = 40; i >= 0; i -= 8) {
> +        build_append_int_noprefix(UUID->buf, (val >> i) & 0xFF, 1);
> +    }
> +
btw:
whole thing might be simpler if an intermediate conversion is avoided,
just pack buffer as in spec byte by byte:

/* format: aabbccdd-eeff-gghh-iijj-kkllmmnnoopp */
assert(strlen(uuid) == ...);
build_append_byte(var->buf, HEX2BYTE(uuid[3]); /* dd */
build_append_byte(var->buf, HEX2BYTE(uuid[2]); /* cc */
...

easy to validate just by looking at "UUID Buffer Format" table in spec

> +    return UUID;
> +}
> +
>  void
>  build_header(GArray *linker, GArray *table_data,
>               AcpiTableHeader *h, const char *sig, int len, uint8_t rev)
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index d1b9fe7..b41fd0c 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -259,6 +259,7 @@ Aml *aml_buffer(void);
>  Aml *aml_resource_template(void);
>  Aml *aml_field(const char *name, AmlFieldFlags flags);
>  Aml *aml_varpackage(uint32_t num_elements);
> +Aml *aml_touuid(const char *uuid);
>  
>  void
>  build_header(GArray *linker, GArray *table_data,