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

[Laszlo Ersek]

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.

- 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).
> + */

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