diff mbox

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

Message ID 1432095595-12968-1-git-send-email-zhaoshenglong@huawei.com
State New
Headers show

Commit Message

Shannon Zhao May 20, 2015, 4:19 a.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>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/aml-build.c         | 112 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/acpi/aml-build.h |   1 +
 2 files changed, 113 insertions(+)

Comments

Igor Mammedov May 20, 2015, 10:57 a.m. UTC | #1 
On Wed, 20 May 2015 12:19:55 +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>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Please remove my RB in cases when changes in patch are not trivial.

> ---
>  hw/acpi/aml-build.c         | 112 ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/aml-build.h |   1 +
>  2 files changed, 113 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 7fcc44e..bdcbad2 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -28,6 +28,8 @@
>  #include "qemu/bswap.h"
>  #include "qemu/bitops.h"
>  #include "hw/acpi/bios-linker-loader.h"
> +#include "qapi/error.h"
> +#include "qemu-common.h"
>  
>  static GArray *build_alloc_array(void)
>  {
> @@ -955,6 +957,116 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
>                               addr_trans, len, flags);
>  }
>  
> +static void String2Byte(const char *str, uint8_t *byte_list)
> +{
> +    unsigned long long val;
> +    char *end;
> +    const char *start = str;
> +    int i, j = 0;
> +    Error *err = NULL;
> +
> +    if (strlen(str) != 36) {
> +        error_setg(&err, "Length of UUID string is not 36");
> +        goto error;
> +    }
> +
> +    val = strtoull(start, &end, 16);
> +    if ((end - start) != 8) {
> +        error_setg(&err, "Length of first part of UUID string is not 8");
> +        goto error;
> +    }
> +    for (i = 3; i >= 0; i--) {
> +        byte_list[j] = extract32(val, (8 * i), 8);
> +        j++;
> +    }
> +    start = end + 1;
> +
> +    val = strtoull(start, &end, 16);
> +    if ((end - start) != 4) {
> +        error_setg(&err, "Length of second part of UUID string is not 4");
> +        goto error;
> +    }
> +    for (i = 1; i >= 0; i--) {
> +        byte_list[j] = extract32(val, (8 * i), 8);
> +        j++;
> +    }
> +    start = end + 1;
> +
> +    val = strtoull(start, &end, 16);
> +    if ((end - start) != 4) {
> +        error_setg(&err, "Length of third part of UUID string is not 4");
> +        goto error;
> +    }
> +    for (i = 1; i >= 0; i--) {
> +        byte_list[j] = extract32(val, (8 * i), 8);
> +        j++;
> +    }
> +    start = end + 1;
> +
> +    val = strtoull(start, &end, 16);
> +    if ((end - start) != 4) {
> +        error_setg(&err, "Length of fourth part of UUID string is not 4");
> +        goto error;
> +    }
> +    for (i = 1; i >= 0; i--) {
> +        byte_list[j] = extract32(val, (8 * i), 8);
> +        j++;
> +    }
> +    start = end + 1;
> +
> +    val = strtoull(start, &end, 16);
> +    if ((end - start) != 12) {
> +        error_setg(&err, "Length of fifth part of UUID string is not 12");
> +        goto error;
> +    }
> +    for (i = 5; i >= 0; i--) {
> +        byte_list[j] = extract64(val, (8 * i), 8);
> +        j++;
> +    }
> +
> +    return;
> +
> +error:
> +    error_report_err(err);
> +    exit(1);
> +}
To me this looks just crazy,
I find the previous patch much easier to read/understand

maybe previous patch +separated asserts as was suggested
an earlier.


> +
> +/*
> + * 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);
> +    uint8_t byte_list[16];
> +
> +    String2Byte(uuid, byte_list);
> +
> +    build_append_byte(var->buf, byte_list[3]); /* dd - at offset 00 */
> +    build_append_byte(var->buf, byte_list[2]); /* cc - at offset 01 */
> +    build_append_byte(var->buf, byte_list[1]); /* bb - at offset 02 */
> +    build_append_byte(var->buf, byte_list[0]); /* aa - at offset 03 */
> +
> +    build_append_byte(var->buf, byte_list[5]); /* ff - at offset 04 */
> +    build_append_byte(var->buf, byte_list[4]); /* ee - at offset 05 */
> +
> +    build_append_byte(var->buf, byte_list[7]); /* hh - at offset 06 */
> +    build_append_byte(var->buf, byte_list[6]); /* gg - at offset 07 */
> +
> +    build_append_byte(var->buf, byte_list[8]); /* ii - at offset 08 */
> +    build_append_byte(var->buf, byte_list[9]); /* jj - at offset 09 */
> +
> +    build_append_byte(var->buf, byte_list[10]); /* kk - at offset 10 */
> +    build_append_byte(var->buf, byte_list[11]); /* ll - at offset 11 */
> +    build_append_byte(var->buf, byte_list[12]); /* mm - at offset 12 */
> +    build_append_byte(var->buf, byte_list[13]); /* nn - at offset 13 */
> +    build_append_byte(var->buf, byte_list[14]); /* oo - at offset 14 */
> +    build_append_byte(var->buf, byte_list[15]); /* pp - at offset 15 */
> +
> +    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 fac70ea..a873b46 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -257,6 +257,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,
Shannon Zhao May 20, 2015, 11:02 a.m. UTC | #2 
On 2015/5/20 18:57, Igor Mammedov wrote:
> On Wed, 20 May 2015 12:19:55 +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>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Please remove my RB in cases when changes in patch are not trivial.
> 

Ok, forgot this.

>> ---
>>  hw/acpi/aml-build.c         | 112 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/acpi/aml-build.h |   1 +
>>  2 files changed, 113 insertions(+)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 7fcc44e..bdcbad2 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -28,6 +28,8 @@
>>  #include "qemu/bswap.h"
>>  #include "qemu/bitops.h"
>>  #include "hw/acpi/bios-linker-loader.h"
>> +#include "qapi/error.h"
>> +#include "qemu-common.h"
>>  
>>  static GArray *build_alloc_array(void)
>>  {
>> @@ -955,6 +957,116 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
>>                               addr_trans, len, flags);
>>  }
>>  
>> +static void String2Byte(const char *str, uint8_t *byte_list)
>> +{
>> +    unsigned long long val;
>> +    char *end;
>> +    const char *start = str;
>> +    int i, j = 0;
>> +    Error *err = NULL;
>> +
>> +    if (strlen(str) != 36) {
>> +        error_setg(&err, "Length of UUID string is not 36");
>> +        goto error;
>> +    }
>> +
>> +    val = strtoull(start, &end, 16);
>> +    if ((end - start) != 8) {
>> +        error_setg(&err, "Length of first part of UUID string is not 8");
>> +        goto error;
>> +    }
>> +    for (i = 3; i >= 0; i--) {
>> +        byte_list[j] = extract32(val, (8 * i), 8);
>> +        j++;
>> +    }
>> +    start = end + 1;
>> +
>> +    val = strtoull(start, &end, 16);
>> +    if ((end - start) != 4) {
>> +        error_setg(&err, "Length of second part of UUID string is not 4");
>> +        goto error;
>> +    }
>> +    for (i = 1; i >= 0; i--) {
>> +        byte_list[j] = extract32(val, (8 * i), 8);
>> +        j++;
>> +    }
>> +    start = end + 1;
>> +
>> +    val = strtoull(start, &end, 16);
>> +    if ((end - start) != 4) {
>> +        error_setg(&err, "Length of third part of UUID string is not 4");
>> +        goto error;
>> +    }
>> +    for (i = 1; i >= 0; i--) {
>> +        byte_list[j] = extract32(val, (8 * i), 8);
>> +        j++;
>> +    }
>> +    start = end + 1;
>> +
>> +    val = strtoull(start, &end, 16);
>> +    if ((end - start) != 4) {
>> +        error_setg(&err, "Length of fourth part of UUID string is not 4");
>> +        goto error;
>> +    }
>> +    for (i = 1; i >= 0; i--) {
>> +        byte_list[j] = extract32(val, (8 * i), 8);
>> +        j++;
>> +    }
>> +    start = end + 1;
>> +
>> +    val = strtoull(start, &end, 16);
>> +    if ((end - start) != 12) {
>> +        error_setg(&err, "Length of fifth part of UUID string is not 12");
>> +        goto error;
>> +    }
>> +    for (i = 5; i >= 0; i--) {
>> +        byte_list[j] = extract64(val, (8 * i), 8);
>> +        j++;
>> +    }
>> +
>> +    return;
>> +
>> +error:
>> +    error_report_err(err);
>> +    exit(1);
>> +}
> To me this looks just crazy,
> I find the previous patch much easier to read/understand
> 
> maybe previous patch +separated asserts as was suggested
> an earlier.
> 

Michael suggests add a pre-parse function to parse the string to bytes
list.

On 2015/5/19 16:37, Michael S. Tsirkin wrote:>>> 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.
>


> 
>> +
>> +/*
>> + * 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);
>> +    uint8_t byte_list[16];
>> +
>> +    String2Byte(uuid, byte_list);
>> +
>> +    build_append_byte(var->buf, byte_list[3]); /* dd - at offset 00 */
>> +    build_append_byte(var->buf, byte_list[2]); /* cc - at offset 01 */
>> +    build_append_byte(var->buf, byte_list[1]); /* bb - at offset 02 */
>> +    build_append_byte(var->buf, byte_list[0]); /* aa - at offset 03 */
>> +
>> +    build_append_byte(var->buf, byte_list[5]); /* ff - at offset 04 */
>> +    build_append_byte(var->buf, byte_list[4]); /* ee - at offset 05 */
>> +
>> +    build_append_byte(var->buf, byte_list[7]); /* hh - at offset 06 */
>> +    build_append_byte(var->buf, byte_list[6]); /* gg - at offset 07 */
>> +
>> +    build_append_byte(var->buf, byte_list[8]); /* ii - at offset 08 */
>> +    build_append_byte(var->buf, byte_list[9]); /* jj - at offset 09 */
>> +
>> +    build_append_byte(var->buf, byte_list[10]); /* kk - at offset 10 */
>> +    build_append_byte(var->buf, byte_list[11]); /* ll - at offset 11 */
>> +    build_append_byte(var->buf, byte_list[12]); /* mm - at offset 12 */
>> +    build_append_byte(var->buf, byte_list[13]); /* nn - at offset 13 */
>> +    build_append_byte(var->buf, byte_list[14]); /* oo - at offset 14 */
>> +    build_append_byte(var->buf, byte_list[15]); /* pp - at offset 15 */
>> +
>> +    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 fac70ea..a873b46 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -257,6 +257,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,
> 
> 
> .
>
Igor Mammedov May 20, 2015, 1:41 p.m. UTC | #3 
On Wed, 20 May 2015 19:02:12 +0800
Shannon Zhao <zhaoshenglong@huawei.com> wrote:

> 
> 
> On 2015/5/20 18:57, Igor Mammedov wrote:
> > On Wed, 20 May 2015 12:19:55 +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>
> >> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > Please remove my RB in cases when changes in patch are not trivial.
> > 
> 
> Ok, forgot this.
> 
> >> ---
> >>  hw/acpi/aml-build.c         | 112 ++++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/acpi/aml-build.h |   1 +
> >>  2 files changed, 113 insertions(+)
> >>
> >> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> >> index 7fcc44e..bdcbad2 100644
> >> --- a/hw/acpi/aml-build.c
> >> +++ b/hw/acpi/aml-build.c
> >> @@ -28,6 +28,8 @@
> >>  #include "qemu/bswap.h"
> >>  #include "qemu/bitops.h"
> >>  #include "hw/acpi/bios-linker-loader.h"
> >> +#include "qapi/error.h"
> >> +#include "qemu-common.h"
> >>  
> >>  static GArray *build_alloc_array(void)
> >>  {
> >> @@ -955,6 +957,116 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
> >>                               addr_trans, len, flags);
> >>  }
> >>  
> >> +static void String2Byte(const char *str, uint8_t *byte_list)
> >> +{
> >> +    unsigned long long val;
> >> +    char *end;
> >> +    const char *start = str;
> >> +    int i, j = 0;
> >> +    Error *err = NULL;
> >> +
> >> +    if (strlen(str) != 36) {
> >> +        error_setg(&err, "Length of UUID string is not 36");
> >> +        goto error;
> >> +    }
> >> +
> >> +    val = strtoull(start, &end, 16);
> >> +    if ((end - start) != 8) {
> >> +        error_setg(&err, "Length of first part of UUID string is not 8");
> >> +        goto error;
> >> +    }
> >> +    for (i = 3; i >= 0; i--) {
> >> +        byte_list[j] = extract32(val, (8 * i), 8);
> >> +        j++;
> >> +    }
> >> +    start = end + 1;
> >> +
> >> +    val = strtoull(start, &end, 16);
> >> +    if ((end - start) != 4) {
> >> +        error_setg(&err, "Length of second part of UUID string is not 4");
> >> +        goto error;
> >> +    }
> >> +    for (i = 1; i >= 0; i--) {
> >> +        byte_list[j] = extract32(val, (8 * i), 8);
> >> +        j++;
> >> +    }
> >> +    start = end + 1;
> >> +
> >> +    val = strtoull(start, &end, 16);
> >> +    if ((end - start) != 4) {
> >> +        error_setg(&err, "Length of third part of UUID string is not 4");
> >> +        goto error;
> >> +    }
> >> +    for (i = 1; i >= 0; i--) {
> >> +        byte_list[j] = extract32(val, (8 * i), 8);
> >> +        j++;
> >> +    }
> >> +    start = end + 1;
> >> +
> >> +    val = strtoull(start, &end, 16);
> >> +    if ((end - start) != 4) {
> >> +        error_setg(&err, "Length of fourth part of UUID string is not 4");
> >> +        goto error;
> >> +    }
> >> +    for (i = 1; i >= 0; i--) {
> >> +        byte_list[j] = extract32(val, (8 * i), 8);
> >> +        j++;
> >> +    }
> >> +    start = end + 1;
> >> +
> >> +    val = strtoull(start, &end, 16);
> >> +    if ((end - start) != 12) {
> >> +        error_setg(&err, "Length of fifth part of UUID string is not 12");
> >> +        goto error;
> >> +    }
> >> +    for (i = 5; i >= 0; i--) {
> >> +        byte_list[j] = extract64(val, (8 * i), 8);
> >> +        j++;
> >> +    }
> >> +
> >> +    return;
> >> +
> >> +error:
> >> +    error_report_err(err);
> >> +    exit(1);
> >> +}
> > To me this looks just crazy,
> > I find the previous patch much easier to read/understand
> > 
> > maybe previous patch +separated asserts as was suggested
> > an earlier.
> > 
> 
> Michael suggests add a pre-parse function to parse the string to bytes
> list.
As you see result is more complex than original approach,
without clear benefits.

> 
> On 2015/5/19 16:37, Michael S. Tsirkin wrote:>>> 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.
> >
> 
> 
> > 
> >> +
> >> +/*
> >> + * 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);
> >> +    uint8_t byte_list[16];
> >> +
> >> +    String2Byte(uuid, byte_list);
> >> +
> >> +    build_append_byte(var->buf, byte_list[3]); /* dd - at offset 00 */
> >> +    build_append_byte(var->buf, byte_list[2]); /* cc - at offset 01 */
> >> +    build_append_byte(var->buf, byte_list[1]); /* bb - at offset 02 */
> >> +    build_append_byte(var->buf, byte_list[0]); /* aa - at offset 03 */
> >> +
> >> +    build_append_byte(var->buf, byte_list[5]); /* ff - at offset 04 */
> >> +    build_append_byte(var->buf, byte_list[4]); /* ee - at offset 05 */
> >> +
> >> +    build_append_byte(var->buf, byte_list[7]); /* hh - at offset 06 */
> >> +    build_append_byte(var->buf, byte_list[6]); /* gg - at offset 07 */
> >> +
> >> +    build_append_byte(var->buf, byte_list[8]); /* ii - at offset 08 */
> >> +    build_append_byte(var->buf, byte_list[9]); /* jj - at offset 09 */
> >> +
> >> +    build_append_byte(var->buf, byte_list[10]); /* kk - at offset 10 */
> >> +    build_append_byte(var->buf, byte_list[11]); /* ll - at offset 11 */
> >> +    build_append_byte(var->buf, byte_list[12]); /* mm - at offset 12 */
> >> +    build_append_byte(var->buf, byte_list[13]); /* nn - at offset 13 */
> >> +    build_append_byte(var->buf, byte_list[14]); /* oo - at offset 14 */
> >> +    build_append_byte(var->buf, byte_list[15]); /* pp - at offset 15 */
> >> +
> >> +    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 fac70ea..a873b46 100644
> >> --- a/include/hw/acpi/aml-build.h
> >> +++ b/include/hw/acpi/aml-build.h
> >> @@ -257,6 +257,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,
> > 
> > 
> > .
> > 
>
Shannon Zhao May 20, 2015, 1:46 p.m. UTC | #4 
On 2015/5/20 21:41, Igor Mammedov wrote:
> On Wed, 20 May 2015 19:02:12 +0800
> Shannon Zhao <zhaoshenglong@huawei.com> wrote:
>
>>
>>
>> On 2015/5/20 18:57, Igor Mammedov wrote:
>>> On Wed, 20 May 2015 12:19:55 +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>
>>>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>> Please remove my RB in cases when changes in patch are not trivial.
>>>
>>
>> Ok, forgot this.
>>
>>>> ---
>>>>   hw/acpi/aml-build.c         | 112 ++++++++++++++++++++++++++++++++++++++++++++
>>>>   include/hw/acpi/aml-build.h |   1 +
>>>>   2 files changed, 113 insertions(+)
>>>>
>>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>>> index 7fcc44e..bdcbad2 100644
>>>> --- a/hw/acpi/aml-build.c
>>>> +++ b/hw/acpi/aml-build.c
>>>> @@ -28,6 +28,8 @@
>>>>   #include "qemu/bswap.h"
>>>>   #include "qemu/bitops.h"
>>>>   #include "hw/acpi/bios-linker-loader.h"
>>>> +#include "qapi/error.h"
>>>> +#include "qemu-common.h"
>>>>
>>>>   static GArray *build_alloc_array(void)
>>>>   {
>>>> @@ -955,6 +957,116 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
>>>>                                addr_trans, len, flags);
>>>>   }
>>>>
>>>> +static void String2Byte(const char *str, uint8_t *byte_list)
>>>> +{
>>>> +    unsigned long long val;
>>>> +    char *end;
>>>> +    const char *start = str;
>>>> +    int i, j = 0;
>>>> +    Error *err = NULL;
>>>> +
>>>> +    if (strlen(str) != 36) {
>>>> +        error_setg(&err, "Length of UUID string is not 36");
>>>> +        goto error;
>>>> +    }
>>>> +
>>>> +    val = strtoull(start, &end, 16);
>>>> +    if ((end - start) != 8) {
>>>> +        error_setg(&err, "Length of first part of UUID string is not 8");
>>>> +        goto error;
>>>> +    }
>>>> +    for (i = 3; i >= 0; i--) {
>>>> +        byte_list[j] = extract32(val, (8 * i), 8);
>>>> +        j++;
>>>> +    }
>>>> +    start = end + 1;
>>>> +
>>>> +    val = strtoull(start, &end, 16);
>>>> +    if ((end - start) != 4) {
>>>> +        error_setg(&err, "Length of second part of UUID string is not 4");
>>>> +        goto error;
>>>> +    }
>>>> +    for (i = 1; i >= 0; i--) {
>>>> +        byte_list[j] = extract32(val, (8 * i), 8);
>>>> +        j++;
>>>> +    }
>>>> +    start = end + 1;
>>>> +
>>>> +    val = strtoull(start, &end, 16);
>>>> +    if ((end - start) != 4) {
>>>> +        error_setg(&err, "Length of third part of UUID string is not 4");
>>>> +        goto error;
>>>> +    }
>>>> +    for (i = 1; i >= 0; i--) {
>>>> +        byte_list[j] = extract32(val, (8 * i), 8);
>>>> +        j++;
>>>> +    }
>>>> +    start = end + 1;
>>>> +
>>>> +    val = strtoull(start, &end, 16);
>>>> +    if ((end - start) != 4) {
>>>> +        error_setg(&err, "Length of fourth part of UUID string is not 4");
>>>> +        goto error;
>>>> +    }
>>>> +    for (i = 1; i >= 0; i--) {
>>>> +        byte_list[j] = extract32(val, (8 * i), 8);
>>>> +        j++;
>>>> +    }
>>>> +    start = end + 1;
>>>> +
>>>> +    val = strtoull(start, &end, 16);
>>>> +    if ((end - start) != 12) {
>>>> +        error_setg(&err, "Length of fifth part of UUID string is not 12");
>>>> +        goto error;
>>>> +    }
>>>> +    for (i = 5; i >= 0; i--) {
>>>> +        byte_list[j] = extract64(val, (8 * i), 8);
>>>> +        j++;
>>>> +    }
>>>> +
>>>> +    return;
>>>> +
>>>> +error:
>>>> +    error_report_err(err);
>>>> +    exit(1);
>>>> +}
>>> To me this looks just crazy,
>>> I find the previous patch much easier to read/understand
>>>
>>> maybe previous patch +separated asserts as was suggested
>>> an earlier.
>>>
>>
>> Michael suggests add a pre-parse function to parse the string to bytes
>> list.
> As you see result is more complex than original approach,
> without clear benefits.
>

Yeah, let's go back then.

>>
>> On 2015/5/19 16:37, Michael S. Tsirkin wrote:>>> 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.
>>>
>>
>>
>>>
>>>> +
>>>> +/*
>>>> + * 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);
>>>> +    uint8_t byte_list[16];
>>>> +
>>>> +    String2Byte(uuid, byte_list);
>>>> +
>>>> +    build_append_byte(var->buf, byte_list[3]); /* dd - at offset 00 */
>>>> +    build_append_byte(var->buf, byte_list[2]); /* cc - at offset 01 */
>>>> +    build_append_byte(var->buf, byte_list[1]); /* bb - at offset 02 */
>>>> +    build_append_byte(var->buf, byte_list[0]); /* aa - at offset 03 */
>>>> +
>>>> +    build_append_byte(var->buf, byte_list[5]); /* ff - at offset 04 */
>>>> +    build_append_byte(var->buf, byte_list[4]); /* ee - at offset 05 */
>>>> +
>>>> +    build_append_byte(var->buf, byte_list[7]); /* hh - at offset 06 */
>>>> +    build_append_byte(var->buf, byte_list[6]); /* gg - at offset 07 */
>>>> +
>>>> +    build_append_byte(var->buf, byte_list[8]); /* ii - at offset 08 */
>>>> +    build_append_byte(var->buf, byte_list[9]); /* jj - at offset 09 */
>>>> +
>>>> +    build_append_byte(var->buf, byte_list[10]); /* kk - at offset 10 */
>>>> +    build_append_byte(var->buf, byte_list[11]); /* ll - at offset 11 */
>>>> +    build_append_byte(var->buf, byte_list[12]); /* mm - at offset 12 */
>>>> +    build_append_byte(var->buf, byte_list[13]); /* nn - at offset 13 */
>>>> +    build_append_byte(var->buf, byte_list[14]); /* oo - at offset 14 */
>>>> +    build_append_byte(var->buf, byte_list[15]); /* pp - at offset 15 */
>>>> +
>>>> +    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 fac70ea..a873b46 100644
>>>> --- a/include/hw/acpi/aml-build.h
>>>> +++ b/include/hw/acpi/aml-build.h
>>>> @@ -257,6 +257,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,
>>>
>>>
>>> .
>>>
>>
>
diff mbox

Patch

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 7fcc44e..bdcbad2 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -28,6 +28,8 @@ 
 #include "qemu/bswap.h"
 #include "qemu/bitops.h"
 #include "hw/acpi/bios-linker-loader.h"
+#include "qapi/error.h"
+#include "qemu-common.h"
 
 static GArray *build_alloc_array(void)
 {
@@ -955,6 +957,116 @@  Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
                              addr_trans, len, flags);
 }
 
+static void String2Byte(const char *str, uint8_t *byte_list)
+{
+    unsigned long long val;
+    char *end;
+    const char *start = str;
+    int i, j = 0;
+    Error *err = NULL;
+
+    if (strlen(str) != 36) {
+        error_setg(&err, "Length of UUID string is not 36");
+        goto error;
+    }
+
+    val = strtoull(start, &end, 16);
+    if ((end - start) != 8) {
+        error_setg(&err, "Length of first part of UUID string is not 8");
+        goto error;
+    }
+    for (i = 3; i >= 0; i--) {
+        byte_list[j] = extract32(val, (8 * i), 8);
+        j++;
+    }
+    start = end + 1;
+
+    val = strtoull(start, &end, 16);
+    if ((end - start) != 4) {
+        error_setg(&err, "Length of second part of UUID string is not 4");
+        goto error;
+    }
+    for (i = 1; i >= 0; i--) {
+        byte_list[j] = extract32(val, (8 * i), 8);
+        j++;
+    }
+    start = end + 1;
+
+    val = strtoull(start, &end, 16);
+    if ((end - start) != 4) {
+        error_setg(&err, "Length of third part of UUID string is not 4");
+        goto error;
+    }
+    for (i = 1; i >= 0; i--) {
+        byte_list[j] = extract32(val, (8 * i), 8);
+        j++;
+    }
+    start = end + 1;
+
+    val = strtoull(start, &end, 16);
+    if ((end - start) != 4) {
+        error_setg(&err, "Length of fourth part of UUID string is not 4");
+        goto error;
+    }
+    for (i = 1; i >= 0; i--) {
+        byte_list[j] = extract32(val, (8 * i), 8);
+        j++;
+    }
+    start = end + 1;
+
+    val = strtoull(start, &end, 16);
+    if ((end - start) != 12) {
+        error_setg(&err, "Length of fifth part of UUID string is not 12");
+        goto error;
+    }
+    for (i = 5; i >= 0; i--) {
+        byte_list[j] = extract64(val, (8 * i), 8);
+        j++;
+    }
+
+    return;
+
+error:
+    error_report_err(err);
+    exit(1);
+}
+
+/*
+ * 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);
+    uint8_t byte_list[16];
+
+    String2Byte(uuid, byte_list);
+
+    build_append_byte(var->buf, byte_list[3]); /* dd - at offset 00 */
+    build_append_byte(var->buf, byte_list[2]); /* cc - at offset 01 */
+    build_append_byte(var->buf, byte_list[1]); /* bb - at offset 02 */
+    build_append_byte(var->buf, byte_list[0]); /* aa - at offset 03 */
+
+    build_append_byte(var->buf, byte_list[5]); /* ff - at offset 04 */
+    build_append_byte(var->buf, byte_list[4]); /* ee - at offset 05 */
+
+    build_append_byte(var->buf, byte_list[7]); /* hh - at offset 06 */
+    build_append_byte(var->buf, byte_list[6]); /* gg - at offset 07 */
+
+    build_append_byte(var->buf, byte_list[8]); /* ii - at offset 08 */
+    build_append_byte(var->buf, byte_list[9]); /* jj - at offset 09 */
+
+    build_append_byte(var->buf, byte_list[10]); /* kk - at offset 10 */
+    build_append_byte(var->buf, byte_list[11]); /* ll - at offset 11 */
+    build_append_byte(var->buf, byte_list[12]); /* mm - at offset 12 */
+    build_append_byte(var->buf, byte_list[13]); /* nn - at offset 13 */
+    build_append_byte(var->buf, byte_list[14]); /* oo - at offset 14 */
+    build_append_byte(var->buf, byte_list[15]); /* pp - at offset 15 */
+
+    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 fac70ea..a873b46 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -257,6 +257,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,