Re: [Qemu-devel] [PATCH v6 06/33] acpi: add aml_method_serialized

[Michael S. Tsirkin]

On Fri, Oct 30, 2015 at 01:56:00PM +0800, Xiao Guangrong wrote:
> It avoid explicit Mutex and will be used by NVDIMM ACPI
> 
> Signed-off-by: Xiao Guangrong <address@hidden>

I'd rather you squashed these utility patches in with where
the code is used. This is just making it harder to review
as I have to jump back and forth.

> ---
>  hw/acpi/aml-build.c         | 26 ++++++++++++++++++++++++--
>  include/hw/acpi/aml-build.h |  1 +
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 9f792ab..8bee8b2 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -696,14 +696,36 @@ Aml *aml_while(Aml *predicate)
>  }
>  
>  /* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefMethod */
> -Aml *aml_method(const char *name, int arg_count)
> +static Aml *__aml_method(const char *name, int arg_count, bool serialized)

Please don't prefix names with __.
what should you call this?
For example, you can call it aml_method_serialized.

>  {
>      Aml *var = aml_bundle(0x14 /* MethodOp */, AML_PACKAGE);
> +    int methodflags;
> +
> +    /*
> +     * MethodFlags:
> +     *   bit 0-2: ArgCount (0-7)
> +     *   bit 3: SerializeFlag
> +     *     0: NotSerialized
> +     *     1: Serialized
> +     *   bit 4-7: reserved (must be 0)
> +     */
> +    assert(!(arg_count & ~7));

Or shorter assert(arg_count < 8);

> +    methodflags = arg_count | (serialized << 3);
>      build_append_namestring(var->buf, "%s", name);
> -    build_append_byte(var->buf, arg_count); /* MethodFlags: ArgCount */
> +    build_append_byte(var->buf, methodflags);
>      return var;
>  }
>  
> +Aml *aml_method(const char *name, int arg_count)
> +{
> +    return __aml_method(name, arg_count, false);
> +}
> +
> +Aml *aml_method_serialized(const char *name, int arg_count)
> +{
> +    return __aml_method(name, arg_count, true);
> +}
> +
>  /* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefDevice */
>  Aml *aml_device(const char *name_format, ...)
>  {
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 5b8a118..00cf40e 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -263,6 +263,7 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed 
> min_fixed,
>  Aml *aml_scope(const char *name_format, ...) GCC_FMT_ATTR(1, 2);
>  Aml *aml_device(const char *name_format, ...) GCC_FMT_ATTR(1, 2);
>  Aml *aml_method(const char *name, int arg_count);
> +Aml *aml_method_serialized(const char *name, int arg_count);
>  Aml *aml_if(Aml *predicate);
>  Aml *aml_else(void);
>  Aml *aml_while(Aml *predicate);
> -- 
> 1.8.3.1