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

[Michael S. Tsirkin]

On Wed, Jan 25, 2017 at 09:36:52AM -0800, Ben Warren wrote:
> 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 wrote:
> 
>         From: Ben Warren <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).
>         + */

only one comment: QWords first appeared in ACPI 2.0 and
XP does not like them. Not strictly a blocker as people can
avoid using the feature, but nice to have.
Will either UEFI or seabios allocate
memory outside 4G range? If not you do not need a qword.




> 
>     (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
>