[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RESEND PATCH 14/23] hw/acpi/aml-build: Add ToUUID macr
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [RESEND PATCH 14/23] hw/acpi/aml-build: Add ToUUID macro |
Date: |
Wed, 20 May 2015 15:41:33 +0200 |
On Wed, 20 May 2015 19:02:12 +0800
Shannon Zhao <address@hidden> wrote:
>
>
> On 2015/5/20 18:57, Igor Mammedov wrote:
> > On Wed, 20 May 2015 12:19:55 +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>
> >> Reviewed-by: Igor Mammedov <address@hidden>
> > 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,
> >
> >
> > .
> >
>
- [Qemu-devel] [PATCH v7 09/23] hw/arm/virt-acpi-build: Generate GTDT table, (continued)
[Qemu-devel] [PATCH v7 13/23] hw/acpi/aml-build: Make aml_buffer() definition consistent with the spec, Shannon Zhao, 2015/05/14
[Qemu-devel] [PATCH v7 16/23] hw/acpi/aml-build: Add aml_lnot() term, Shannon Zhao, 2015/05/14
[Qemu-devel] [PATCH v7 22/23] ACPI: split CONFIG_ACPI into 4 pieces, Shannon Zhao, 2015/05/14
[Qemu-devel] [PATCH v7 23/23] hw/arm/virt: Enable dynamic generation of ACPI v5.1 tables, Shannon Zhao, 2015/05/14
[Qemu-devel] [PATCH v7 15/23] hw/acpi/aml-build: Add aml_or() term, Shannon Zhao, 2015/05/14
[Qemu-devel] [PATCH v7 21/23] hw/arm/virt-acpi-build: Add PCIe controller in ACPI DSDT table, Shannon Zhao, 2015/05/14