[v7,14/23] hw/acpi/aml-build: Add ToUUID macro

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         | 46 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/acpi/aml-build.h |  1 +
 2 files changed, 47 insertions(+)

On Thu, May 14, 2015 at 05:19:33PM +0800, Shannon Zhao 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         | 46 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/aml-build.h |  1 +
>  2 files changed, 47 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 6197896..0e4d35f 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -956,6 +956,52 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
>                               addr_trans, len, flags);
>  }
>  
> +static uint8_t Hex2Byte(const char *src)
> +{
> +    int hi = Hex2Digit(*src++);
> +    int lo = Hex2Digit(*src);


Just src[0] src[1] would be clearer.

> +
> +    g_assert(((hi >= 0) && (hi <= 15)) && ((lo >= 0) && (lo <= 15)));

Pls use simple assert everywhere, it does not matter
and rest of acpi uses plain assert.

> +    return (hi << 4) | lo;
> +}

Really should be lower-case. We need to fix Hex2Digit too,
but since Hex2Digit is broken, I'll let this one in
and we can fix both with a patch on top.

> +
> +/*
> + * ACPI 3.0: 17.5.124 ToUUID (Convert String to UUID Macro)
> + * e.g. UUID: aabbccdd-eeff-gghh-iijj-kkllmmnnoopp
> + * call aml_touuid("aabbccdd-eeff-gghh-iijj-kkllmmnnoopp");
> + */
> +Aml *aml_touuid(const char *uuid)
> +{
> +    Aml *var = aml_bundle(0x11 /* BufferOp */, AML_BUFFER);
> +
> +    /* format: aabbccdd-eeff-gghh-iijj-kkllmmnnoopp */

This documents the argument, so really belongs near the
function's head.

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

Does something else validate the parameter?
It's not friendly to handle errors by assert.
Why doesn't that something else give us a pre-parse structure then?


> +
> +    build_append_byte(var->buf, Hex2Byte(uuid + 6)); /* dd - at offset 00 */
> +    build_append_byte(var->buf, Hex2Byte(uuid + 4)); /* cc - at offset 01 */
> +    build_append_byte(var->buf, Hex2Byte(uuid + 2)); /* bb - at offset 02 */
> +    build_append_byte(var->buf, Hex2Byte(uuid + 0)); /* aa - at offset 03 */
> +
> +    build_append_byte(var->buf, Hex2Byte(uuid + 11)); /* ff - at offset 04 */
> +    build_append_byte(var->buf, Hex2Byte(uuid + 9)); /* ee - at offset 05 */
> +
> +    build_append_byte(var->buf, Hex2Byte(uuid + 16)); /* hh - at offset 06 */
> +    build_append_byte(var->buf, Hex2Byte(uuid + 14)); /* gg - at offset 07 */
> +
> +    build_append_byte(var->buf, Hex2Byte(uuid + 19)); /* ii - at offset 08 */
> +    build_append_byte(var->buf, Hex2Byte(uuid + 21)); /* jj - at offset 09 */
> +
> +    build_append_byte(var->buf, Hex2Byte(uuid + 24)); /* kk - at offset 10 */
> +    build_append_byte(var->buf, Hex2Byte(uuid + 26)); /* ll - at offset 11 */
> +    build_append_byte(var->buf, Hex2Byte(uuid + 28)); /* mm - at offset 12 */
> +    build_append_byte(var->buf, Hex2Byte(uuid + 30)); /* nn - at offset 13 */
> +    build_append_byte(var->buf, Hex2Byte(uuid + 32)); /* oo - at offset 14 */
> +    build_append_byte(var->buf, Hex2Byte(uuid + 34)); /* pp - at offset 15 */

Can we use strtoull?


> +
> +    return var;
> +}
> +
>  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 615f3c1..f5e764c 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -265,6 +265,7 @@ Aml *aml_buffer(int buffer_size, uint8_t *byte_list);
>  Aml *aml_resource_template(void);
>  Aml *aml_field(const char *name, AmlAccessType type, AmlUpdateRule rule);
>  Aml *aml_varpackage(uint32_t num_elements);
> +Aml *aml_touuid(const char *uuid);
>  
>  void
>  build_header(GArray *linker, GArray *table_data,
> -- 
> 2.1.0
>

On 05/18/15 18:17, Michael S. Tsirkin wrote:
> On Thu, May 14, 2015 at 05:19:33PM +0800, Shannon Zhao 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         | 46 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/acpi/aml-build.h |  1 +
>>  2 files changed, 47 insertions(+)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 6197896..0e4d35f 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -956,6 +956,52 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
>>                               addr_trans, len, flags);
>>  }
>>  
>> +static uint8_t Hex2Byte(const char *src)
>> +{
>> +    int hi = Hex2Digit(*src++);
>> +    int lo = Hex2Digit(*src);
> 
> 
> Just src[0] src[1] would be clearer.
> 
>> +
>> +    g_assert(((hi >= 0) && (hi <= 15)) && ((lo >= 0) && (lo <= 15)));
> 
> Pls use simple assert everywhere, it does not matter
> and rest of acpi uses plain assert.

In addition, please break up conjunctions into different lines:

  assert(a && b);

should be

  assert(a);
  assert(b);

If one of "a" and "b" fails, the second form gives more info.

(Quite from the sidelines, but I think this is a reasonable, generic
style hint.)

> 
>> +    return (hi << 4) | lo;
>> +}
> 
> Really should be lower-case. We need to fix Hex2Digit too,
> but since Hex2Digit is broken, I'll let this one in
> and we can fix both with a patch on top.
> 
>> +
>> +/*
>> + * ACPI 3.0: 17.5.124 ToUUID (Convert String to UUID Macro)
>> + * e.g. UUID: aabbccdd-eeff-gghh-iijj-kkllmmnnoopp
>> + * call aml_touuid("aabbccdd-eeff-gghh-iijj-kkllmmnnoopp");
>> + */
>> +Aml *aml_touuid(const char *uuid)
>> +{
>> +    Aml *var = aml_bundle(0x11 /* BufferOp */, AML_BUFFER);
>> +
>> +    /* format: aabbccdd-eeff-gghh-iijj-kkllmmnnoopp */
> 
> This documents the argument, so really belongs near the
> function's head.
> 
>> +    g_assert((strlen(uuid) == 36) && (uuid[8] == '-') && (uuid[13] == '-')
>> +              && (uuid[18] == '-') && (uuid[23] == '-'));

Ditto (assuming this assert() stays at all, based on Michael's feedback).

Thanks
Laszlo

> Does something else validate the parameter?
> It's not friendly to handle errors by assert.
> Why doesn't that something else give us a pre-parse structure then?
> 
> 
>> +
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 6)); /* dd - at offset 00 */
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 4)); /* cc - at offset 01 */
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 2)); /* bb - at offset 02 */
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 0)); /* aa - at offset 03 */
>> +
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 11)); /* ff - at offset 04 */
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 9)); /* ee - at offset 05 */
>> +
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 16)); /* hh - at offset 06 */
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 14)); /* gg - at offset 07 */
>> +
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 19)); /* ii - at offset 08 */
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 21)); /* jj - at offset 09 */
>> +
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 24)); /* kk - at offset 10 */
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 26)); /* ll - at offset 11 */
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 28)); /* mm - at offset 12 */
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 30)); /* nn - at offset 13 */
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 32)); /* oo - at offset 14 */
>> +    build_append_byte(var->buf, Hex2Byte(uuid + 34)); /* pp - at offset 15 */
> 
> Can we use strtoull?
> 
> 
>> +
>> +    return var;
>> +}
>> +
>>  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 615f3c1..f5e764c 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -265,6 +265,7 @@ Aml *aml_buffer(int buffer_size, uint8_t *byte_list);
>>  Aml *aml_resource_template(void);
>>  Aml *aml_field(const char *name, AmlAccessType type, AmlUpdateRule rule);
>>  Aml *aml_varpackage(uint32_t num_elements);
>> +Aml *aml_touuid(const char *uuid);
>>  
>>  void
>>  build_header(GArray *linker, GArray *table_data,
>> -- 
>> 2.1.0
>>

On 2015/5/19 0:17, Michael S. Tsirkin wrote:
> On Thu, May 14, 2015 at 05:19:33PM +0800, Shannon Zhao 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         | 46 +++++++++++++++++++++++++++++++++++++++++++++
>> >  include/hw/acpi/aml-build.h |  1 +
>> >  2 files changed, 47 insertions(+)
>> > 
>> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> > index 6197896..0e4d35f 100644
>> > --- a/hw/acpi/aml-build.c
>> > +++ b/hw/acpi/aml-build.c
>> > @@ -956,6 +956,52 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
>> >                               addr_trans, len, flags);
>> >  }
>> >  
>> > +static uint8_t Hex2Byte(const char *src)
>> > +{
>> > +    int hi = Hex2Digit(*src++);
>> > +    int lo = Hex2Digit(*src);
> 
> Just src[0] src[1] would be clearer.
> 
>> > +
>> > +    g_assert(((hi >= 0) && (hi <= 15)) && ((lo >= 0) && (lo <= 15)));
> Pls use simple assert everywhere, it does not matter
> and rest of acpi uses plain assert.
> 
>> > +    return (hi << 4) | lo;
>> > +}
> Really should be lower-case. We need to fix Hex2Digit too,
> but since Hex2Digit is broken, I'll let this one in
> and we can fix both with a patch on top.
> 
>> > +
>> > +/*
>> > + * ACPI 3.0: 17.5.124 ToUUID (Convert String to UUID Macro)
>> > + * e.g. UUID: aabbccdd-eeff-gghh-iijj-kkllmmnnoopp
>> > + * call aml_touuid("aabbccdd-eeff-gghh-iijj-kkllmmnnoopp");
>> > + */
>> > +Aml *aml_touuid(const char *uuid)
>> > +{
>> > +    Aml *var = aml_bundle(0x11 /* BufferOp */, AML_BUFFER);
>> > +
>> > +    /* format: aabbccdd-eeff-gghh-iijj-kkllmmnnoopp */
> This documents the argument, so really belongs near the
> function's head.
> 
>> > +    g_assert((strlen(uuid) == 36) && (uuid[8] == '-') && (uuid[13] == '-')
>> > +              && (uuid[18] == '-') && (uuid[23] == '-'));
> Does something else validate the parameter?
> It's not friendly to handle errors by assert.
> Why doesn't that something else give us a pre-parse structure then?
> 

a pre-parse structure?

> 
>> > +
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 6)); /* dd - at offset 00 */
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 4)); /* cc - at offset 01 */
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 2)); /* bb - at offset 02 */
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 0)); /* aa - at offset 03 */
>> > +
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 11)); /* ff - at offset 04 */
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 9)); /* ee - at offset 05 */
>> > +
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 16)); /* hh - at offset 06 */
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 14)); /* gg - at offset 07 */
>> > +
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 19)); /* ii - at offset 08 */
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 21)); /* jj - at offset 09 */
>> > +
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 24)); /* kk - at offset 10 */
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 26)); /* ll - at offset 11 */
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 28)); /* mm - at offset 12 */
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 30)); /* nn - at offset 13 */
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 32)); /* oo - at offset 14 */
>> > +    build_append_byte(var->buf, Hex2Byte(uuid + 34)); /* pp - at offset 15 */
> Can we use strtoull?
> 

Igor and I have a discussion on this. I used strtol before and we got an
agreement on current approach.

On Tue, May 19, 2015 at 08:41:39AM +0800, Shannon Zhao wrote:
> 
> 
> On 2015/5/19 0:17, Michael S. Tsirkin wrote:
> > On Thu, May 14, 2015 at 05:19:33PM +0800, Shannon Zhao 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         | 46 +++++++++++++++++++++++++++++++++++++++++++++
> >> >  include/hw/acpi/aml-build.h |  1 +
> >> >  2 files changed, 47 insertions(+)
> >> > 
> >> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> >> > index 6197896..0e4d35f 100644
> >> > --- a/hw/acpi/aml-build.c
> >> > +++ b/hw/acpi/aml-build.c
> >> > @@ -956,6 +956,52 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
> >> >                               addr_trans, len, flags);
> >> >  }
> >> >  
> >> > +static uint8_t Hex2Byte(const char *src)
> >> > +{
> >> > +    int hi = Hex2Digit(*src++);
> >> > +    int lo = Hex2Digit(*src);
> > 
> > Just src[0] src[1] would be clearer.
> > 
> >> > +
> >> > +    g_assert(((hi >= 0) && (hi <= 15)) && ((lo >= 0) && (lo <= 15)));
> > Pls use simple assert everywhere, it does not matter
> > and rest of acpi uses plain assert.
> > 
> >> > +    return (hi << 4) | lo;
> >> > +}
> > Really should be lower-case. We need to fix Hex2Digit too,
> > but since Hex2Digit is broken, I'll let this one in
> > and we can fix both with a patch on top.
> > 
> >> > +
> >> > +/*
> >> > + * ACPI 3.0: 17.5.124 ToUUID (Convert String to UUID Macro)
> >> > + * e.g. UUID: aabbccdd-eeff-gghh-iijj-kkllmmnnoopp
> >> > + * call aml_touuid("aabbccdd-eeff-gghh-iijj-kkllmmnnoopp");
> >> > + */
> >> > +Aml *aml_touuid(const char *uuid)
> >> > +{
> >> > +    Aml *var = aml_bundle(0x11 /* BufferOp */, AML_BUFFER);
> >> > +
> >> > +    /* format: aabbccdd-eeff-gghh-iijj-kkllmmnnoopp */
> > This documents the argument, so really belongs near the
> > function's head.
> > 
> >> > +    g_assert((strlen(uuid) == 36) && (uuid[8] == '-') && (uuid[13] == '-')
> >> > +              && (uuid[18] == '-') && (uuid[23] == '-'));
> > Does something else validate the parameter?
> > It's not friendly to handle errors by assert.
> > Why doesn't that something else give us a pre-parse structure then?
> > 
> 
> a pre-parse structure?

Yes - have caller validate and parse the string, on failure report
it to user cleanly, on success give us a byte array
avoiding need to call Hex2Byte.


> > 
> >> > +
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 6)); /* dd - at offset 00 */
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 4)); /* cc - at offset 01 */
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 2)); /* bb - at offset 02 */
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 0)); /* aa - at offset 03 */
> >> > +
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 11)); /* ff - at offset 04 */
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 9)); /* ee - at offset 05 */
> >> > +
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 16)); /* hh - at offset 06 */
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 14)); /* gg - at offset 07 */
> >> > +
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 19)); /* ii - at offset 08 */
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 21)); /* jj - at offset 09 */
> >> > +
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 24)); /* kk - at offset 10 */
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 26)); /* ll - at offset 11 */
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 28)); /* mm - at offset 12 */
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 30)); /* nn - at offset 13 */
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 32)); /* oo - at offset 14 */
> >> > +    build_append_byte(var->buf, Hex2Byte(uuid + 34)); /* pp - at offset 15 */
> > Can we use strtoull?
> > 
> 
> Igor and I have a discussion on this. I used strtol before and we got an
> agreement on current approach.
> 
> -- 
> Shannon