[v5,13/20] hw/acpi/aml-build: Add ToUUID macro

Message ID	1429104309-3844-14-git-send-email-zhaoshenglong@huawei.com
State	New
Headers	show Return-Path: <qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org> From: Shannon Zhao <zhaoshenglong@huawei.com> To: <qemu-devel@nongnu.org>, <peter.maydell@linaro.org>, <pbonzini@redhat.com>, <christoffer.dall@linaro.org>, <a.spyridakis@virtualopensystems.com>, <claudio.fontana@huawei.com>, <imammedo@redhat.com>, <hanjun.guo@linaro.org>, <mst@redhat.com>, <lersek@redhat.com>, <alex.bennee@linaro.org> Date: Wed, 15 Apr 2015 21:25:02 +0800 Message-ID: <1429104309-3844-14-git-send-email-zhaoshenglong@huawei.com> In-Reply-To: <1429104309-3844-1-git-send-email-zhaoshenglong@huawei.com> References: <1429104309-3844-1-git-send-email-zhaoshenglong@huawei.com> MIME-Version: 1.0 Content-Type: text/plain Cc: hangaohuai@huawei.com, shannon.zhao@linaro.org, peter.huangpeng@huawei.com, zhaoshenglong@huawei.com Subject: [Qemu-devel] [PATCH v5 13/20] hw/acpi/aml-build: Add ToUUID macro Precedence: list Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org

Shannon Zhao April 15, 2015, 1:25 p.m. UTC

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(+)

Igor Mammedov April 28, 2015, 6:54 a.m. UTC | #1

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,

Shannon Zhao April 28, 2015, 7:46 a.m. UTC | #2

On 2015/4/28 14:54, Igor Mammedov wrote:
> 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?
> 

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); ?

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

Shannon Zhao April 28, 2015, 8:08 a.m. UTC | #3

On 2015/4/28 14:54, Igor Mammedov wrote:
> 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 */
> ...

use build_append_byte(var->buf, HEX2BYTE(uuid + (3 * 2)); /* dd */ ?

Igor Mammedov April 28, 2015, 8:15 a.m. UTC | #4

On Tue, 28 Apr 2015 15:46:22 +0800
Shannon Zhao <zhaoshenglong@huawei.com> wrote:

> On 2015/4/28 14:54, Igor Mammedov wrote:
> > 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?
> > 
> 
> 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,
> > 
> > 
> > .
> > 
> 
>

Shannon Zhao April 28, 2015, 8:52 a.m. UTC | #5

On 2015/4/28 16:15, Igor Mammedov wrote:
>>> 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.
> 

I mean uuid[3] points to b not dd. Maybe use following way:

static uint8_t Hex2Byte(char *src)
{
    int hi = Hex2Digit(*src++);
    int lo = Hex2Digit(*src);

    if ((hi < 0) || (lo < 0))
        return -1;

    return (hi << 4) | lo;
}

g_assert((strlen(uuid) == 36) && (uuid[8] == '-') && (uuid[13] == '-')
          && (uuid[18] == '-') && (uuid[23] == '-'));

build_append_byte(var->buf, Hex2Byte(uuid + (3 * 2))); /* dd */
build_append_byte(var->buf, Hex2Byte(uuid + (2 * 2))); /* cc */
build_append_byte(var->buf, Hex2Byte(uuid + (1 * 2))); /* bb */
build_append_byte(var->buf, Hex2Byte(uuid + (0 * 2))); /* aa */

build_append_byte(var->buf, Hex2Byte(uuid + (5 * 2 + 1))); /* ff */
build_append_byte(var->buf, Hex2Byte(uuid + (4 * 2 + 1))); /* ee */

build_append_byte(var->buf, Hex2Byte(uuid + (7 * 2 + 2))); /* hh */
build_append_byte(var->buf, Hex2Byte(uuid + (6 * 2 + 2))); /* gg */

build_append_byte(var->buf, Hex2Byte(uuid + (8 * 2 + 3))); /* ii */
build_append_byte(var->buf, Hex2Byte(uuid + (9 * 2 + 3))); /* jj */

build_append_byte(var->buf, Hex2Byte(uuid + (10 * 2 + 4))); /* kk */
build_append_byte(var->buf, Hex2Byte(uuid + (11 * 2 + 4))); /* ll */
build_append_byte(var->buf, Hex2Byte(uuid + (12 * 2 + 4))); /* mm */
build_append_byte(var->buf, Hex2Byte(uuid + (13 * 2 + 4))); /* nn */
build_append_byte(var->buf, Hex2Byte(uuid + (14 * 2 + 4))); /* oo */
build_append_byte(var->buf, Hex2Byte(uuid + (15 * 2 + 4))); /* pp */

Igor Mammedov April 28, 2015, 9:35 a.m. UTC | #6

On Tue, 28 Apr 2015 16:52:19 +0800
Shannon Zhao <zhaoshenglong@huawei.com> wrote:

> On 2015/4/28 16:15, Igor Mammedov wrote:
> >>> 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.
> > 
> 
> I mean uuid[3] points to b not dd. Maybe use following way:
> 
> static uint8_t Hex2Byte(char *src)
or even better:
Hex2Byte(char *src, byte_idx)
  and do pointer arithmetic inside

[...]
> build_append_byte(var->buf, Hex2Byte(uuid + (3 * 2))); /* dd */
build_append_byte(var->buf, Hex2Byte(uuid, 3)); /* dd - at offset 00 */
build_append_byte(var->buf, Hex2Byte(uuid, 2)); /* cc - at offset 01 */
...

Shannon Zhao April 28, 2015, 9:48 a.m. UTC | #7

On 2015/4/28 17:35, Igor Mammedov wrote:
> On Tue, 28 Apr 2015 16:52:19 +0800
> Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> 
>> On 2015/4/28 16:15, Igor Mammedov wrote:
>>>>> 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.
>>>
>>
>> I mean uuid[3] points to b not dd. Maybe use following way:
>>
>> static uint8_t Hex2Byte(char *src)
> or even better:
> Hex2Byte(char *src, byte_idx)
>   and do pointer arithmetic inside
> 
> [...]
>> build_append_byte(var->buf, Hex2Byte(uuid + (3 * 2))); /* dd */
> build_append_byte(var->buf, Hex2Byte(uuid, 3)); /* dd - at offset 00 */
> build_append_byte(var->buf, Hex2Byte(uuid, 2)); /* cc - at offset 01 */
> ...
> 
Yes, it's better to first four bytes. But there are hyphens /-/, for
offset 04, 05 and etc it doesn't work. We can't use following expression:

build_append_byte(var->buf, Hex2Byte(uuid, 5)); /* ff - at offset 04 */
build_append_byte(var->buf, Hex2Byte(uuid, 4)); /* ee - at offset 05 */
...

Shannon Zhao April 29, 2015, 1:41 p.m. UTC | #8

On 2015/4/28 17:48, Shannon Zhao wrote:
> On 2015/4/28 17:35, Igor Mammedov wrote:
>> On Tue, 28 Apr 2015 16:52:19 +0800
>> Shannon Zhao <zhaoshenglong@huawei.com> wrote:
>>
>>> On 2015/4/28 16:15, Igor Mammedov wrote:
>>>>>> 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.
>>>>
>>>
>>> I mean uuid[3] points to b not dd. Maybe use following way:
>>>
>>> static uint8_t Hex2Byte(char *src)
>> or even better:
>> Hex2Byte(char *src, byte_idx)
>>    and do pointer arithmetic inside
>>
>> [...]
>>> build_append_byte(var->buf, Hex2Byte(uuid + (3 * 2))); /* dd */
>> build_append_byte(var->buf, Hex2Byte(uuid, 3)); /* dd - at offset 00 */
>> build_append_byte(var->buf, Hex2Byte(uuid, 2)); /* cc - at offset 01 */
>> ...
>>
> Yes, it's better to first four bytes. But there are hyphens /-/, for
> offset 04, 05 and etc it doesn't work. We can't use following expression:
>
> build_append_byte(var->buf, Hex2Byte(uuid, 5)); /* ff - at offset 04 */
> build_append_byte(var->buf, Hex2Byte(uuid, 4)); /* ee - at offset 05 */
> ...
>
>

So about the implementation of this macro, I think I'd like to use 
following approach. This lets Hex2Byte do what it should only do and 
still has a clear look of UUID. What do you think about?

static uint8_t Hex2Byte(char *src)
{
     int hi = Hex2Digit(*src++);
     int lo = Hex2Digit(*src);

     if ((hi < 0) || (lo < 0))
         return -1;

     return (hi << 4) | lo;
}

g_assert((strlen(uuid) == 36) && (uuid[8] == '-') && (uuid[13] == '-')
           && (uuid[18] == '-') && (uuid[23] == '-'));

build_append_byte(var->buf, Hex2Byte(uuid + (3 * 2))); /* dd */
build_append_byte(var->buf, Hex2Byte(uuid + (2 * 2))); /* cc */
build_append_byte(var->buf, Hex2Byte(uuid + (1 * 2))); /* bb */
build_append_byte(var->buf, Hex2Byte(uuid + (0 * 2))); /* aa */

build_append_byte(var->buf, Hex2Byte(uuid + (5 * 2 + 1))); /* ff */
build_append_byte(var->buf, Hex2Byte(uuid + (4 * 2 + 1))); /* ee */

build_append_byte(var->buf, Hex2Byte(uuid + (7 * 2 + 2))); /* hh */
build_append_byte(var->buf, Hex2Byte(uuid + (6 * 2 + 2))); /* gg */

build_append_byte(var->buf, Hex2Byte(uuid + (8 * 2 + 3))); /* ii */
build_append_byte(var->buf, Hex2Byte(uuid + (9 * 2 + 3))); /* jj */

build_append_byte(var->buf, Hex2Byte(uuid + (10 * 2 + 4))); /* kk */
build_append_byte(var->buf, Hex2Byte(uuid + (11 * 2 + 4))); /* ll */
build_append_byte(var->buf, Hex2Byte(uuid + (12 * 2 + 4))); /* mm */
build_append_byte(var->buf, Hex2Byte(uuid + (13 * 2 + 4))); /* nn */
build_append_byte(var->buf, Hex2Byte(uuid + (14 * 2 + 4))); /* oo */
build_append_byte(var->buf, Hex2Byte(uuid + (15 * 2 + 4))); /* pp */

Igor Mammedov May 4, 2015, 9:22 a.m. UTC | #9

On Wed, 29 Apr 2015 21:41:39 +0800
Shannon Zhao <shannon.zhao@linaro.org> wrote:

> 
> 
> On 2015/4/28 17:48, Shannon Zhao wrote:
> > On 2015/4/28 17:35, Igor Mammedov wrote:
> >> On Tue, 28 Apr 2015 16:52:19 +0800
> >> Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> >>
> >>> On 2015/4/28 16:15, Igor Mammedov wrote:
> >>>>>> 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.
> >>>>
> >>>
> >>> I mean uuid[3] points to b not dd. Maybe use following way:
> >>>
> >>> static uint8_t Hex2Byte(char *src)
> >> or even better:
> >> Hex2Byte(char *src, byte_idx)
> >>    and do pointer arithmetic inside
> >>
> >> [...]
> >>> build_append_byte(var->buf, Hex2Byte(uuid + (3 * 2))); /* dd */
> >> build_append_byte(var->buf, Hex2Byte(uuid, 3)); /* dd - at offset 00 */
> >> build_append_byte(var->buf, Hex2Byte(uuid, 2)); /* cc - at offset 01 */
> >> ...
> >>
> > Yes, it's better to first four bytes. But there are hyphens /-/, for
> > offset 04, 05 and etc it doesn't work. We can't use following expression:
> >
> > build_append_byte(var->buf, Hex2Byte(uuid, 5)); /* ff - at offset 04 */
> > build_append_byte(var->buf, Hex2Byte(uuid, 4)); /* ee - at offset 05 */
> > ...
> >
> >
> 
> So about the implementation of this macro, I think I'd like to use 
> following approach. This lets Hex2Byte do what it should only do and 
> still has a clear look of UUID. What do you think about?
> 
> static uint8_t Hex2Byte(char *src)
> {
>      int hi = Hex2Digit(*src++);
>      int lo = Hex2Digit(*src);
> 
>      if ((hi < 0) || (lo < 0))
>          return -1;
just make Hex2Digit() assert on wrong input.

> 
>      return (hi << 4) | lo;
> }
> 
> g_assert((strlen(uuid) == 36) && (uuid[8] == '-') && (uuid[13] == '-')
>            && (uuid[18] == '-') && (uuid[23] == '-'));
> 
> build_append_byte(var->buf, Hex2Byte(uuid + (3 * 2))); /* dd */
                                               ^^^^^
I'd make it one number, instead of forcing reader to do math
every time he/she looks at this code.


> build_append_byte(var->buf, Hex2Byte(uuid + (2 * 2))); /* cc */
> build_append_byte(var->buf, Hex2Byte(uuid + (1 * 2))); /* bb */
> build_append_byte(var->buf, Hex2Byte(uuid + (0 * 2))); /* aa */
> 
> build_append_byte(var->buf, Hex2Byte(uuid + (5 * 2 + 1))); /* ff */
> build_append_byte(var->buf, Hex2Byte(uuid + (4 * 2 + 1))); /* ee */
> 
> build_append_byte(var->buf, Hex2Byte(uuid + (7 * 2 + 2))); /* hh */
> build_append_byte(var->buf, Hex2Byte(uuid + (6 * 2 + 2))); /* gg */
> 
> build_append_byte(var->buf, Hex2Byte(uuid + (8 * 2 + 3))); /* ii */
> build_append_byte(var->buf, Hex2Byte(uuid + (9 * 2 + 3))); /* jj */
> 
> build_append_byte(var->buf, Hex2Byte(uuid + (10 * 2 + 4))); /* kk */
> build_append_byte(var->buf, Hex2Byte(uuid + (11 * 2 + 4))); /* ll */
> build_append_byte(var->buf, Hex2Byte(uuid + (12 * 2 + 4))); /* mm */
> build_append_byte(var->buf, Hex2Byte(uuid + (13 * 2 + 4))); /* nn */
> build_append_byte(var->buf, Hex2Byte(uuid + (14 * 2 + 4))); /* oo */
> build_append_byte(var->buf, Hex2Byte(uuid + (15 * 2 + 4))); /* pp */

Shannon Zhao May 4, 2015, 9:30 a.m. UTC | #10

On 2015/5/4 17:22, Igor Mammedov wrote:
> On Wed, 29 Apr 2015 21:41:39 +0800
> Shannon Zhao <shannon.zhao@linaro.org> wrote:
> 
>>
>>
>> On 2015/4/28 17:48, Shannon Zhao wrote:
>>> On 2015/4/28 17:35, Igor Mammedov wrote:
>>>> On Tue, 28 Apr 2015 16:52:19 +0800
>>>> Shannon Zhao <zhaoshenglong@huawei.com> wrote:
>>>>
>>>>> On 2015/4/28 16:15, Igor Mammedov wrote:
>>>>>>>> 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.
>>>>>>
>>>>>
>>>>> I mean uuid[3] points to b not dd. Maybe use following way:
>>>>>
>>>>> static uint8_t Hex2Byte(char *src)
>>>> or even better:
>>>> Hex2Byte(char *src, byte_idx)
>>>>    and do pointer arithmetic inside
>>>>
>>>> [...]
>>>>> build_append_byte(var->buf, Hex2Byte(uuid + (3 * 2))); /* dd */
>>>> build_append_byte(var->buf, Hex2Byte(uuid, 3)); /* dd - at offset 00 */
>>>> build_append_byte(var->buf, Hex2Byte(uuid, 2)); /* cc - at offset 01 */
>>>> ...
>>>>
>>> Yes, it's better to first four bytes. But there are hyphens /-/, for
>>> offset 04, 05 and etc it doesn't work. We can't use following expression:
>>>
>>> build_append_byte(var->buf, Hex2Byte(uuid, 5)); /* ff - at offset 04 */
>>> build_append_byte(var->buf, Hex2Byte(uuid, 4)); /* ee - at offset 05 */
>>> ...
>>>
>>>
>>
>> So about the implementation of this macro, I think I'd like to use 
>> following approach. This lets Hex2Byte do what it should only do and 
>> still has a clear look of UUID. What do you think about?
>>
>> static uint8_t Hex2Byte(char *src)
>> {
>>      int hi = Hex2Digit(*src++);
>>      int lo = Hex2Digit(*src);
>>
>>      if ((hi < 0) || (lo < 0))
>>          return -1;
> just make Hex2Digit() assert on wrong input.
> 

Ok.

>>
>>      return (hi << 4) | lo;
>> }
>>
>> g_assert((strlen(uuid) == 36) && (uuid[8] == '-') && (uuid[13] == '-')
>>            && (uuid[18] == '-') && (uuid[23] == '-'));
>>
>> build_append_byte(var->buf, Hex2Byte(uuid + (3 * 2))); /* dd */
>                                                ^^^^^
> I'd make it one number, instead of forcing reader to do math
> every time he/she looks at this code.
> 

Ok, will do this. And about other patches in this series could you offer
your comments?

Thanks,
Shannon

> 
>> build_append_byte(var->buf, Hex2Byte(uuid + (2 * 2))); /* cc */
>> build_append_byte(var->buf, Hex2Byte(uuid + (1 * 2))); /* bb */
>> build_append_byte(var->buf, Hex2Byte(uuid + (0 * 2))); /* aa */
>>
>> build_append_byte(var->buf, Hex2Byte(uuid + (5 * 2 + 1))); /* ff */
>> build_append_byte(var->buf, Hex2Byte(uuid + (4 * 2 + 1))); /* ee */
>>
>> build_append_byte(var->buf, Hex2Byte(uuid + (7 * 2 + 2))); /* hh */
>> build_append_byte(var->buf, Hex2Byte(uuid + (6 * 2 + 2))); /* gg */
>>
>> build_append_byte(var->buf, Hex2Byte(uuid + (8 * 2 + 3))); /* ii */
>> build_append_byte(var->buf, Hex2Byte(uuid + (9 * 2 + 3))); /* jj */
>>
>> build_append_byte(var->buf, Hex2Byte(uuid + (10 * 2 + 4))); /* kk */
>> build_append_byte(var->buf, Hex2Byte(uuid + (11 * 2 + 4))); /* ll */
>> build_append_byte(var->buf, Hex2Byte(uuid + (12 * 2 + 4))); /* mm */
>> build_append_byte(var->buf, Hex2Byte(uuid + (13 * 2 + 4))); /* nn */
>> build_append_byte(var->buf, Hex2Byte(uuid + (14 * 2 + 4))); /* oo */
>> build_append_byte(var->buf, Hex2Byte(uuid + (15 * 2 + 4))); /* pp */
>

Igor Mammedov May 4, 2015, 10:53 a.m. UTC | #11

On Mon, 04 May 2015 17:30:20 +0800
Shannon Zhao <shannon.zhao@linaro.org> wrote:

> 
> 
> On 2015/5/4 17:22, Igor Mammedov wrote:
> > On Wed, 29 Apr 2015 21:41:39 +0800
> > Shannon Zhao <shannon.zhao@linaro.org> wrote:
> > 
> >>
> >>
> >> On 2015/4/28 17:48, Shannon Zhao wrote:
> >>> On 2015/4/28 17:35, Igor Mammedov wrote:
> >>>> On Tue, 28 Apr 2015 16:52:19 +0800
> >>>> Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> >>>>
> >>>>> On 2015/4/28 16:15, Igor Mammedov wrote:
> >>>>>>>> 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.
> >>>>>>
> >>>>>
> >>>>> I mean uuid[3] points to b not dd. Maybe use following way:
> >>>>>
> >>>>> static uint8_t Hex2Byte(char *src)
> >>>> or even better:
> >>>> Hex2Byte(char *src, byte_idx)
> >>>>    and do pointer arithmetic inside
> >>>>
> >>>> [...]
> >>>>> build_append_byte(var->buf, Hex2Byte(uuid + (3 * 2))); /* dd */
> >>>> build_append_byte(var->buf, Hex2Byte(uuid, 3)); /* dd - at offset 00 */
> >>>> build_append_byte(var->buf, Hex2Byte(uuid, 2)); /* cc - at offset 01 */
> >>>> ...
> >>>>
> >>> Yes, it's better to first four bytes. But there are hyphens /-/, for
> >>> offset 04, 05 and etc it doesn't work. We can't use following expression:
> >>>
> >>> build_append_byte(var->buf, Hex2Byte(uuid, 5)); /* ff - at offset 04 */
> >>> build_append_byte(var->buf, Hex2Byte(uuid, 4)); /* ee - at offset 05 */
> >>> ...
> >>>
> >>>
> >>
> >> So about the implementation of this macro, I think I'd like to use 
> >> following approach. This lets Hex2Byte do what it should only do and 
> >> still has a clear look of UUID. What do you think about?
> >>
> >> static uint8_t Hex2Byte(char *src)
> >> {
> >>      int hi = Hex2Digit(*src++);
> >>      int lo = Hex2Digit(*src);
> >>
> >>      if ((hi < 0) || (lo < 0))
> >>          return -1;
> > just make Hex2Digit() assert on wrong input.
> > 
> 
> Ok.
> 
> >>
> >>      return (hi << 4) | lo;
> >> }
> >>
> >> g_assert((strlen(uuid) == 36) && (uuid[8] == '-') && (uuid[13] == '-')
> >>            && (uuid[18] == '-') && (uuid[23] == '-'));
> >>
> >> build_append_byte(var->buf, Hex2Byte(uuid + (3 * 2))); /* dd */
> >                                                ^^^^^
> > I'd make it one number, instead of forcing reader to do math
> > every time he/she looks at this code.
> > 
> 
> Ok, will do this. And about other patches in this series could you offer
> your comments?
the rest of AML patches looks good, I'll give them RB on next respin.

> 
> Thanks,
> Shannon
> 
> > 
> >> build_append_byte(var->buf, Hex2Byte(uuid + (2 * 2))); /* cc */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (1 * 2))); /* bb */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (0 * 2))); /* aa */
> >>
> >> build_append_byte(var->buf, Hex2Byte(uuid + (5 * 2 + 1))); /* ff */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (4 * 2 + 1))); /* ee */
> >>
> >> build_append_byte(var->buf, Hex2Byte(uuid + (7 * 2 + 2))); /* hh */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (6 * 2 + 2))); /* gg */
> >>
> >> build_append_byte(var->buf, Hex2Byte(uuid + (8 * 2 + 3))); /* ii */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (9 * 2 + 3))); /* jj */
> >>
> >> build_append_byte(var->buf, Hex2Byte(uuid + (10 * 2 + 4))); /* kk */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (11 * 2 + 4))); /* ll */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (12 * 2 + 4))); /* mm */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (13 * 2 + 4))); /* nn */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (14 * 2 + 4))); /* oo */
> >> build_append_byte(var->buf, Hex2Byte(uuid + (15 * 2 + 4))); /* pp */
> > 
>

[v5,13/20] hw/acpi/aml-build: Add ToUUID macro

Commit Message

Comments

Patch