From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53122) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yn0gC-0007Ir-Sf for qemu-devel@nongnu.org; Tue, 28 Apr 2015 04:15:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yn0g9-000670-CF for qemu-devel@nongnu.org; Tue, 28 Apr 2015 04:15:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43593) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yn0g9-00066r-1E for qemu-devel@nongnu.org; Tue, 28 Apr 2015 04:15:41 -0400 Date: Tue, 28 Apr 2015 10:15:29 +0200 From: Igor Mammedov Message-ID: <20150428101529.5feb4915@nial.brq.redhat.com> In-Reply-To: <553F3ACE.2060208@huawei.com> References: <1429104309-3844-1-git-send-email-zhaoshenglong@huawei.com> <1429104309-3844-14-git-send-email-zhaoshenglong@huawei.com> <20150428085412.5d215d01@nial.brq.redhat.com> <553F3ACE.2060208@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 13/20] hw/acpi/aml-build: Add ToUUID macro List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: 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, shannon.zhao@linaro.org On Tue, 28 Apr 2015 15:46:22 +0800 Shannon Zhao wrote: > On 2015/4/28 14:54, Igor Mammedov wrote: > > On Wed, 15 Apr 2015 21:25:02 +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 > >> --- > >> 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 > >> #include > >> #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, > > > > > > . > > > >