qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named


From: Ben Warren
Subject: Re: [Qemu-devel] [PATCH v4 1/9] ACPI: Add a function for building named qword entries
Date: Wed, 25 Jan 2017 09:36:52 -0800

Hi Laszlo,

> On Jan 24, 2017, at 7:55 PM, Laszlo Ersek <address@hidden> wrote:
> 
> Hi Ben,
> 
> sorry about being late to reviewing this series. I hope I can now spend
> more time on it.
> 
> - Please do not try to address my comments immediately. It's very
> possible (even likely) that Igor, MST and myself could have different
> opinions on things, so first please await agreement between your reviewers.
> 
Thanks for the very detailed review.  I’ll give it a couple of days and then 
start work on the suggested changes.

> - I think you should have CC'd Igor and Michael directly. I'm adding
> them to this reply; hopefully that will be enough for them to monitor
> this series.
> 
> - I'll likely be unable to review everything with 100% coverage; so
> addressing (or sufficiently refuting) my comments might not guarantee
> that the next version will be merged at once.
> 
> With all that said:
> 
> On 01/25/17 02:43, address@hidden <mailto:address@hidden> wrote:
>> From: Ben Warren <address@hidden <mailto:address@hidden>>
>> 
>> This is initially used to patch a 64-bit address into the VM Generation ID 
>> SSDT
> 
> (1) I think this commit message line is overlong; I think we wrap at 74
> chars or so. Not critical, but worth mentioning.
> 
>> 
>> Signed-off-by: Ben Warren <address@hidden>
>> ---
>> hw/acpi/aml-build.c         | 28 ++++++++++++++++++++++++++++
>> include/hw/acpi/aml-build.h |  4 ++++
>> 2 files changed, 32 insertions(+)
>> 
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index b2a1e40..dc4edc2 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -285,6 +285,34 @@ build_append_named_dword(GArray *array, const char 
>> *name_format, ...)
>>     return offset;
>> }
>> 
>> +/*
>> + * Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a qword,
>> + * and return the offset to 0x00000000 for runtime patching.
>> + *
>> + * Warning: runtime patching is best avoided. Only use this as
>> + * a replacement for DataTableRegion (for guests that don't
>> + * support it).
>> + */
> 
> (2) Since we're adding a qword (8-byte integer), the hexadecimal
> constant should have 16 nibbles: 0x0000000000000000. (After copying the
> comment from build_append_named_dword(), it should be actualized.)
> 
> (3) Normally the functions in this file that create AML operators carry
> a note about the ACPI spec version and exact location that defines the
> operator. I see that commit f20354910893 ("acpi: add
> build_append_named_dword, returning an offset in buffer", 2016-03-01)
> missed that too.
> 
> Unless this tradition has been willfully abandoned, I suggest that you
> add the right reference here, and also (in retrospect) to
> build_append_named_dword().
> 
>> +int
>> +build_append_named_qword(GArray *array, const char *name_format, ...)
>> +{
>> +    int offset;
>> +    va_list ap;
>> +
>> +    build_append_byte(array, 0x08); /* NameOp */
>> +    va_start(ap, name_format);
>> +    build_append_namestringv(array, name_format, ap);
>> +    va_end(ap);
>> +
>> +    build_append_byte(array, 0x0E); /* QWordPrefix */
>> +
>> +    offset = array->len;
>> +    build_append_int_noprefix(array, 0x00000000, 8);
> 
> (4) I guess the constant should be updated here too, for consistency
> with the comment.
> 
> The rest looks okay. (I didn't verify 0x0E == QWordPrefix specifically,
> because an error there would break the functionality immediately, and
> you'd notice.)
> 
> Thanks!
> Laszlo
> 
>> +    assert(array->len == offset + 8);
>> +
>> +    return offset;
>> +}
>> +
>> static GPtrArray *alloc_list;
>> 
>> static Aml *aml_alloc(void)
>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index 559326c..dbf63cf 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -385,6 +385,10 @@ int
>> build_append_named_dword(GArray *array, const char *name_format, ...)
>> GCC_FMT_ATTR(2, 3);
>> 
>> +int
>> +build_append_named_qword(GArray *array, const char *name_format, ...)
>> +GCC_FMT_ATTR(2, 3);
>> +
>> void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>>                        uint64_t len, int node, MemoryAffinityFlags flags);

—Ben

Attachment: smime.p7s
Description: S/MIME cryptographic signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]