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 <shannon.z...@linaro.org>
>>
>> Add ToUUID macro, this is useful for generating PCIe ACPI table.
>>
>> Signed-off-by: Shannon Zhao <zhaoshengl...@huawei.com>
>> Signed-off-by: Shannon Zhao <shannon.z...@linaro.org>
>> ---
>> 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
>>
