From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44920) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yv4Kb-0002Jo-FU for qemu-devel@nongnu.org; Wed, 20 May 2015 09:46:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yv4KY-0007Xs-MB for qemu-devel@nongnu.org; Wed, 20 May 2015 09:46:45 -0400 Received: from mail-pd0-f182.google.com ([209.85.192.182]:33614) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yv4KY-0007Xk-EJ for qemu-devel@nongnu.org; Wed, 20 May 2015 09:46:42 -0400 Received: by pdbqa5 with SMTP id qa5so69602395pdb.0 for ; Wed, 20 May 2015 06:46:41 -0700 (PDT) Message-ID: <555C903B.4050002@linaro.org> Date: Wed, 20 May 2015 21:46:35 +0800 From: Shannon Zhao MIME-Version: 1.0 References: <1431595182-7552-15-git-send-email-zhaoshenglong@huawei.com> <1432095595-12968-1-git-send-email-zhaoshenglong@huawei.com> <20150520125704.6d4167cf@nial.brq.redhat.com> <555C69B4.1010507@huawei.com> <20150520154133.4542e3ca@nial.brq.redhat.com> In-Reply-To: <20150520154133.4542e3ca@nial.brq.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RESEND PATCH 14/23] hw/acpi/aml-build: Add ToUUID macro List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov , Shannon Zhao Cc: peter.maydell@linaro.org, hangaohuai@huawei.com, mst@redhat.com, a.spyridakis@virtualopensystems.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, peter.huangpeng@huawei.com, alex.bennee@linaro.org, hanjun.guo@linaro.org, pbonzini@redhat.com, lersek@redhat.com, christoffer.dall@linaro.org On 2015/5/20 21:41, Igor Mammedov wrote: > On Wed, 20 May 2015 19:02:12 +0800 > Shannon Zhao wrote: > >> >> >> On 2015/5/20 18:57, Igor Mammedov wrote: >>> On Wed, 20 May 2015 12:19:55 +0800 >>> Shannon Zhao wrote: >>> >>>> From: Shannon Zhao >>>> >>>> Add ToUUID macro, this is useful for generating PCIe ACPI table. >>>> >>>> Signed-off-by: Shannon Zhao >>>> Signed-off-by: Shannon Zhao >>>> Reviewed-by: Igor Mammedov >>> 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, >>> >>> >>> . >>> >> > -- Shannon