Re: [Qemu-devel] [PATCH v2 06/47] acpi: add acpi_name() & acpi_name_decl() term

[Michael S. Tsirkin]

On Thu, Jan 22, 2015 at 02:49:50PM +0000, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
>  hw/acpi/acpi-build-utils.c         | 24 ++++++++++++++++++++++++
>  include/hw/acpi/acpi-build-utils.h |  3 +++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> index 40a1769..1bda2ec 100644
> --- a/hw/acpi/acpi-build-utils.c
> +++ b/hw/acpi/acpi-build-utils.c
> @@ -314,6 +314,30 @@ static AcpiAml aml_allocate_internal(uint8_t op, 
> AcpiBlockFlags flags)
>      return var;
>  }
>  
> +/*
> + * help to construct NameString, which return AcpiAml object
> + * for using with other aml_append or other acpi_* terms

Here and elsewhere: I can't parse this header text.
I'm guessing you just mean "construct NameString",
and that's it?

Also, most other places use build_append_namestring -
so when should acpi_name be used instead?
This should be made clear here in the comment.

> + */
> +AcpiAml GCC_FMT_ATTR(1, 2) acpi_name(const char *name_format, ...)
> +{

This isn't really a name. It just appends a string.  So rename this
acpi_string and then the below one adding a name can be named acpi_name?

Also, in many places one must use only one nameseg.
I think a separate api that actually validates
that it's one segment is better than silently failing.
Do we ever use it for more than 1 segment?
If not, maybe the right thing to do is
to use build_append_nameseg and call this one acpi_nameseg.


> +    va_list ap;
> +    AcpiAml var = aml_allocate_internal(0, NON_BLOCK);

0 hard coded? What does it mean?
Same elsewhere.

> +    va_start(ap, name_format);
> +    build_append_namestringv(var.buf, name_format, ap);
> +    va_end(ap);
> +    return var;
> +
> +/* ACPI 5.0: 20.2.5.1 Namespace Modifier Objects Encoding: DefName */

Let's quote the earliest spec which documents each object:
one year from now 5.0 will not be the latest.
Applies here and elsewhere.
In most places this will be 1.0b.
Where the construct is newer, this will automatically
document which guests support it.

> +AcpiAml acpi_name_decl(const char *name, AcpiAml val)
> +{
> +    AcpiAml var = aml_allocate_internal(0, NON_BLOCK);
> +    build_append_byte(var.buf, 0x08);

Pls add comment documenting what 0x08 is here.

> +    build_append_namestring(var.buf, "%s", name);
> +    aml_append(&var, val);
> +    return var;
> +}
> +
>  /* ACPI 5.0: 20.2.5.3 Type 1 Opcodes Encoding: DefIfElse */
>  AcpiAml acpi_if(AcpiAml predicate)
>  {
> diff --git a/include/hw/acpi/acpi-build-utils.h 
> b/include/hw/acpi/acpi-build-utils.h
> index 177f9ed..868cfa5 100644
> --- a/include/hw/acpi/acpi-build-utils.h
> +++ b/include/hw/acpi/acpi-build-utils.h
> @@ -21,6 +21,9 @@ typedef struct AcpiAml {
>  
>  void aml_append(AcpiAml *parent_ctx, AcpiAml child);
>  
> +/* non block ASL object primitives */

what does it mean that it's a "non block primitive"?
I didn't find this concept in the spec.

> +AcpiAml GCC_FMT_ATTR(1, 2) acpi_name(const char *name_format, ...);
> +AcpiAml acpi_name_decl(const char *name, AcpiAml val);
>  /* Block ASL object primitives */
>  AcpiAml acpi_if(AcpiAml predicate);
>  AcpiAml acpi_method(const char *name, int arg_count);
> -- 
> 1.8.3.1