From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56929) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YuPsU-0002ic-3U for qemu-devel@nongnu.org; Mon, 18 May 2015 14:35:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YuPsQ-00007b-RI for qemu-devel@nongnu.org; Mon, 18 May 2015 14:35:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46744) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YuPsQ-00007Q-Iq for qemu-devel@nongnu.org; Mon, 18 May 2015 14:34:58 -0400 Message-ID: <555A30C2.1000206@redhat.com> Date: Mon, 18 May 2015 20:34:42 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1431595182-7552-1-git-send-email-zhaoshenglong@huawei.com> <1431595182-7552-15-git-send-email-zhaoshenglong@huawei.com> <20150518180850-mutt-send-email-mst@redhat.com> In-Reply-To: <20150518180850-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 14/23] 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, "Michael S. Tsirkin" , a.spyridakis@virtualopensystems.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, peter.huangpeng@huawei.com, hanjun.guo@linaro.org, imammedo@redhat.com, pbonzini@redhat.com, alex.bennee@linaro.org, christoffer.dall@linaro.org, shannon.zhao@linaro.org 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 >> >> 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 | 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 >>