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

[Igor Mammedov]

On Tue, 28 Apr 2015 15:46:22 +0800
Shannon Zhao <address@hidden> wrote:

> On 2015/4/28 14:54, Igor Mammedov wrote:
> > On Wed, 15 Apr 2015 21:25:02 +0800
> > Shannon Zhao <address@hidden> wrote:
> > 
> >> From: Shannon Zhao <address@hidden>
> >>
> >> Add ToUUID macro, this is useful for generating PCIe ACPI table.
> >>
> >> Signed-off-by: Shannon Zhao <address@hidden>
> >> Signed-off-by: Shannon Zhao <address@hidden>
> >> ---
> >>  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?
> > 
> 
> strtol needs stdlib.h. I should include it directly rather than
> including qemu-common.h.
> 
> >>  #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 */
> 
> use build_append_byte(var->buf, HEX2BYTE(uuid + 7); ?
> 
> > build_append_byte(var->buf, HEX2BYTE(uuid[2]); /* cc */
> 
> use build_append_byte(var->buf, HEX2BYTE(uuid + 5); ?
if you mean hyphens /-/ then they are not encoded,
but you surely can add checks for them to make sure that
UUID format is as expected.

> 
> > ...
> > 
> > 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,
> > 
> > 
> > .
> > 
> 
>