[v2,01/47] acpi: introduce AML composer aml_append()

Adds for dynamic AML creation, which will be used
for piecing ASL/AML primitives together and hiding
from user/caller details about how nested context
should be closed/packed leaving less space for
mistakes and necessity to know how AML should be
encoded, allowing user to concentrate on ASL
representation instead.

For example it will allow to create AML like this:

AcpiAml scope = acpi_scope("PCI0")
AcpiAml dev = acpi_device("PM")
    aml_append(&dev, acpi_name_decl("_ADR", acpi_int(addr)))
aml_append(&scope, dev);

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/acpi-build-utils.c         | 39 ++++++++++++++++++++++++++++++++++++++
 include/hw/acpi/acpi-build-utils.h | 16 ++++++++++++++++
 2 files changed, 55 insertions(+)

On Thu, Jan 22, 2015 at 02:49:45PM +0000, Igor Mammedov wrote:
> Adds for dynamic AML creation, which will be used
> for piecing ASL/AML primitives together and hiding
> from user/caller details about how nested context
> should be closed/packed leaving less space for
> mistakes and necessity to know how AML should be
> encoded, allowing user to concentrate on ASL
> representation instead.
> 
> For example it will allow to create AML like this:
> 
> AcpiAml scope = acpi_scope("PCI0")
> AcpiAml dev = acpi_device("PM")
>     aml_append(&dev, acpi_name_decl("_ADR", acpi_int(addr)))
> aml_append(&scope, dev);
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/acpi/acpi-build-utils.c         | 39 ++++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/acpi-build-utils.h | 16 ++++++++++++++++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> index 602e68c..547ecaa 100644
> --- a/hw/acpi/acpi-build-utils.c
> +++ b/hw/acpi/acpi-build-utils.c
> @@ -267,3 +267,42 @@ void build_append_int(GArray *table, uint32_t value)
>          build_append_value(table, value, 4);
>      }
>  }
> +
> +static void build_prepend_int(GArray *array, uint32_t value)
> +{
> +    GArray *data = build_alloc_array();
> +
> +    build_append_int(data, value);
> +    g_array_prepend_vals(array, data->data, data->len);
> +    build_free_array(data);
> +}
> +
> +void aml_append(AcpiAml *parent_ctx, AcpiAml child)
> +{
> +    switch (child.block_flags) {
> +    case EXT_PACKAGE:
> +        build_extop_package(child.buf, child.op);
> +        break;
> +
> +    case PACKAGE:
> +        build_package(child.buf, child.op);
> +        break;
> +
> +    case RES_TEMPLATE:
> +        build_append_byte(child.buf, 0x79); /* EndTag */
> +        /*
> +         * checksum operations is treated as succeeded if checksum
> +         * field is zero. [ACPI Spec 5.0, 6.4.2.9 End Tag]
> +         */
> +        build_append_byte(child.buf, 0);
> +        /* fall through, to pack resources in buffer */
> +    case BUFFER:
> +        build_prepend_int(child.buf, child.buf->len);
> +        build_package(child.buf, child.op);
> +        break;
> +    default:
> +        break;
> +    }
> +    build_append_array(parent_ctx->buf, child.buf);
> +    build_free_array(child.buf);
> +}
> diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> index 199f003..64e7ec3 100644
> --- a/include/hw/acpi/acpi-build-utils.h
> +++ b/include/hw/acpi/acpi-build-utils.h
> @@ -5,6 +5,22 @@
>  #include <glib.h>
>  #include "qemu/compiler.h"
>  
> +typedef enum {
> +    NON_BLOCK,
> +    PACKAGE,
> +    EXT_PACKAGE,
> +    BUFFER,
> +    RES_TEMPLATE,
> +} AcpiBlockFlags;

Please prefix values with ACPI_BUILD_ - don't pollute the
global namespace.
Same elsewhere: add build_ to functions, and Build to types.

This makes it clear these are not Acpi spec types,
but helpers to build Aml.

> +
> +typedef struct AcpiAml {
> +    GArray *buf;
> +    uint8_t op;
> +    AcpiBlockFlags block_flags;
> +} AcpiAml;
> +
> +void aml_append(AcpiAml *parent_ctx, AcpiAml child);
> +
>  GArray *build_alloc_array(void);
>  void build_free_array(GArray *array);
>  void build_prepend_byte(GArray *array, uint8_t val);
> -- 
> 1.8.3.1

On Thu, Jan 22, 2015 at 02:49:45PM +0000, Igor Mammedov wrote:
> Adds for dynamic AML creation, which will be used
> for piecing ASL/AML primitives together and hiding
> from user/caller details about how nested context
> should be closed/packed leaving less space for
> mistakes and necessity to know how AML should be
> encoded, allowing user to concentrate on ASL
> representation instead.
> 
> For example it will allow to create AML like this:
> 
> AcpiAml scope = acpi_scope("PCI0")
> AcpiAml dev = acpi_device("PM")
>     aml_append(&dev, acpi_name_decl("_ADR", acpi_int(addr)))
> aml_append(&scope, dev);
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/acpi/acpi-build-utils.c         | 39 ++++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/acpi-build-utils.h | 16 ++++++++++++++++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> index 602e68c..547ecaa 100644
> --- a/hw/acpi/acpi-build-utils.c
> +++ b/hw/acpi/acpi-build-utils.c
> @@ -267,3 +267,42 @@ void build_append_int(GArray *table, uint32_t value)
>          build_append_value(table, value, 4);
>      }
>  }
> +
> +static void build_prepend_int(GArray *array, uint32_t value)
> +{
> +    GArray *data = build_alloc_array();
> +
> +    build_append_int(data, value);
> +    g_array_prepend_vals(array, data->data, data->len);
> +    build_free_array(data);
> +}

I don't think prepend is generally justified:
it makes code hard to follow and debug.

Adding length is different: of course you need
to first have the package before you can add length.

We currently have build_prepend_package_length - just move it
to utils, and use everywhere.


> +
> +void aml_append(AcpiAml *parent_ctx, AcpiAml child)
> +{
> +    switch (child.block_flags) {
> +    case EXT_PACKAGE:
> +        build_extop_package(child.buf, child.op);
> +        break;
> +
> +    case PACKAGE:
> +        build_package(child.buf, child.op);
> +        break;
> +
> +    case RES_TEMPLATE:
> +        build_append_byte(child.buf, 0x79); /* EndTag */
> +        /*
> +         * checksum operations is treated as succeeded if checksum
> +         * field is zero. [ACPI Spec 5.0, 6.4.2.9 End Tag]
> +         */

Bad english. Let's quote verbatim:
	If the checksum field is zero, the
	resource data is treated as if the checksum operation succeeded.
Also best to quote earliest spec available so we know it's
compatible with old guests:
Spec 1.0b, 6.4.2.8 End Tag


> +        build_append_byte(child.buf, 0);
> +        /* fall through, to pack resources in buffer */
> +    case BUFFER:
> +        build_prepend_int(child.buf, child.buf->len);
> +        build_package(child.buf, child.op);
> +        break;
> +    default:
> +        break;
> +    }
> +    build_append_array(parent_ctx->buf, child.buf);
> +    build_free_array(child.buf);
> +}
> diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> index 199f003..64e7ec3 100644
> --- a/include/hw/acpi/acpi-build-utils.h
> +++ b/include/hw/acpi/acpi-build-utils.h
> @@ -5,6 +5,22 @@
>  #include <glib.h>
>  #include "qemu/compiler.h"
>  
> +typedef enum {
> +    NON_BLOCK,
> +    PACKAGE,
> +    EXT_PACKAGE,
> +    BUFFER,
> +    RES_TEMPLATE,
> +} AcpiBlockFlags;
> +
> +typedef struct AcpiAml {
> +    GArray *buf;
> +    uint8_t op;
> +    AcpiBlockFlags block_flags;
> +} AcpiAml;
> +
> +void aml_append(AcpiAml *parent_ctx, AcpiAml child);
> +
>  GArray *build_alloc_array(void);
>  void build_free_array(GArray *array);
>  void build_prepend_byte(GArray *array, uint8_t val);
> -- 
> 1.8.3.1

On Thu, Jan 22, 2015 at 02:49:45PM +0000, Igor Mammedov wrote:
> Adds for dynamic AML creation, which will be used
> for piecing ASL/AML primitives together and hiding
> from user/caller details about how nested context
> should be closed/packed leaving less space for
> mistakes and necessity to know how AML should be
> encoded, allowing user to concentrate on ASL
> representation instead.
> 
> For example it will allow to create AML like this:
> 
> AcpiAml scope = acpi_scope("PCI0")
> AcpiAml dev = acpi_device("PM")
>     aml_append(&dev, acpi_name_decl("_ADR", acpi_int(addr)))
> aml_append(&scope, dev);
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/acpi/acpi-build-utils.c         | 39 ++++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/acpi-build-utils.h | 16 ++++++++++++++++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> index 602e68c..547ecaa 100644
> --- a/hw/acpi/acpi-build-utils.c
> +++ b/hw/acpi/acpi-build-utils.c
> @@ -267,3 +267,42 @@ void build_append_int(GArray *table, uint32_t value)
>          build_append_value(table, value, 4);
>      }
>  }
> +
> +static void build_prepend_int(GArray *array, uint32_t value)
> +{
> +    GArray *data = build_alloc_array();
> +
> +    build_append_int(data, value);
> +    g_array_prepend_vals(array, data->data, data->len);
> +    build_free_array(data);
> +}
> +
> +void aml_append(AcpiAml *parent_ctx, AcpiAml child)
> +{
> +    switch (child.block_flags) {
> +    case EXT_PACKAGE:
> +        build_extop_package(child.buf, child.op);
> +        break;
> +
> +    case PACKAGE:
> +        build_package(child.buf, child.op);
> +        break;
> +
> +    case RES_TEMPLATE:
> +        build_append_byte(child.buf, 0x79); /* EndTag */
> +        /*
> +         * checksum operations is treated as succeeded if checksum
> +         * field is zero. [ACPI Spec 5.0, 6.4.2.9 End Tag]
> +         */
> +        build_append_byte(child.buf, 0);
> +        /* fall through, to pack resources in buffer */
> +    case BUFFER:
> +        build_prepend_int(child.buf, child.buf->len);
> +        build_package(child.buf, child.op);
> +        break;
> +    default:
> +        break;
> +    }
> +    build_append_array(parent_ctx->buf, child.buf);
> +    build_free_array(child.buf);

So looking at this, the API is a bit tricky to use
to avoid use after free.

For example:
	aml_append(&a, b);
	aml_append(&a, b);
this is use after free.

This is C, memory management should not be automatic.
So just move free out of there, get child by const pointer.
Same for alloc: split out allocation and init.
You can still return pointer back to caller,
this will allow chaining just like you do here.

We'll get:

	AcpiAml dev = aml_alloc();

	aml_append(&scope, acpi_device(&dev));

	aml_free(&dev);


which is more verbose, but makes it clear
there's no use after free, additionally,
compiler checks that you don't modify child
where not necessary.


> +}
> diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> index 199f003..64e7ec3 100644
> --- a/include/hw/acpi/acpi-build-utils.h
> +++ b/include/hw/acpi/acpi-build-utils.h
> @@ -5,6 +5,22 @@
>  #include <glib.h>
>  #include "qemu/compiler.h"
>  
> +typedef enum {
> +    NON_BLOCK,
> +    PACKAGE,
> +    EXT_PACKAGE,
> +    BUFFER,
> +    RES_TEMPLATE,
> +} AcpiBlockFlags;
> +
> +typedef struct AcpiAml {
> +    GArray *buf;
> +    uint8_t op;
> +    AcpiBlockFlags block_flags;
> +} AcpiAml;
> +
> +void aml_append(AcpiAml *parent_ctx, AcpiAml child);
> +
>  GArray *build_alloc_array(void);
>  void build_free_array(GArray *array);
>  void build_prepend_byte(GArray *array, uint8_t val);
> -- 
> 1.8.3.1

On Fri, 23 Jan 2015 10:03:03 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> > +typedef enum {
> > +    NON_BLOCK,
> > +    PACKAGE,
> > +    EXT_PACKAGE,
> > +    BUFFER,
> > +    RES_TEMPLATE,
> > +} AcpiBlockFlags;
> 
> Please prefix values with ACPI_BUILD_ - don't pollute the
> global namespace.
Could we use AML_ prefix instead?

> Same elsewhere: add build_ to functions, and Build to types.
Same here i.e. s/acpi_/aml_/ prefix in API calls?

> 
> This makes it clear these are not Acpi spec types,
> but helpers to build Aml.
> 
> > +
> > +typedef struct AcpiAml {
> > +    GArray *buf;
> > +    uint8_t op;
> > +    AcpiBlockFlags block_flags;
> > +} AcpiAml;
> > +
> > +void aml_append(AcpiAml *parent_ctx, AcpiAml child);
> > +
> >  GArray *build_alloc_array(void);
> >  void build_free_array(GArray *array);
> >  void build_prepend_byte(GArray *array, uint8_t val);
> > -- 
> > 1.8.3.1
>

On Fri, 23 Jan 2015 10:11:19 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jan 22, 2015 at 02:49:45PM +0000, Igor Mammedov wrote:
> > Adds for dynamic AML creation, which will be used
> > for piecing ASL/AML primitives together and hiding
> > from user/caller details about how nested context
> > should be closed/packed leaving less space for
> > mistakes and necessity to know how AML should be
> > encoded, allowing user to concentrate on ASL
> > representation instead.
> > 
> > For example it will allow to create AML like this:
> > 
> > AcpiAml scope = acpi_scope("PCI0")
> > AcpiAml dev = acpi_device("PM")
> >     aml_append(&dev, acpi_name_decl("_ADR", acpi_int(addr)))
> > aml_append(&scope, dev);
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/acpi/acpi-build-utils.c         | 39 ++++++++++++++++++++++++++++++++++++++
> >  include/hw/acpi/acpi-build-utils.h | 16 ++++++++++++++++
> >  2 files changed, 55 insertions(+)
> > 
> > diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> > index 602e68c..547ecaa 100644
> > --- a/hw/acpi/acpi-build-utils.c
> > +++ b/hw/acpi/acpi-build-utils.c
> > @@ -267,3 +267,42 @@ void build_append_int(GArray *table, uint32_t value)
> >          build_append_value(table, value, 4);
> >      }
> >  }
> > +
> > +static void build_prepend_int(GArray *array, uint32_t value)
> > +{
> > +    GArray *data = build_alloc_array();
> > +
> > +    build_append_int(data, value);
> > +    g_array_prepend_vals(array, data->data, data->len);
> > +    build_free_array(data);
> > +}
> 
> I don't think prepend is generally justified:
> it makes code hard to follow and debug.
> 
> Adding length is different: of course you need
> to first have the package before you can add length.
> 
> We currently have build_prepend_package_length - just move it
> to utils, and use everywhere.
[...]
> > +    case BUFFER:
> > +        build_prepend_int(child.buf, child.buf->len);
> > +        build_package(child.buf, child.op);
Buffer uses the same concept as package, but adds its own additional length.
Therefore I've added build_prepend_int(),
I can create build_buffer() and mimic build_package()
but it won't change picture.

As for moving to to another file, during all this series lowlevel
build_(some_aml_related_costruct_helper)s are moved into this file
and should be make static to hide from user lowlevel helpers
(including build_package).
That will leave only high level API available.

TODO for me: make sure that moved lowlevel helpers are static


> > +        break;
> > +    default:
> > +        break;
> > +    }
> > +    build_append_array(parent_ctx->buf, child.buf);
> > +    build_free_array(child.buf);
> > +}
> > diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> > index 199f003..64e7ec3 100644
> > --- a/include/hw/acpi/acpi-build-utils.h
> > +++ b/include/hw/acpi/acpi-build-utils.h
> > @@ -5,6 +5,22 @@
> >  #include <glib.h>
> >  #include "qemu/compiler.h"
> >  
> > +typedef enum {
> > +    NON_BLOCK,
> > +    PACKAGE,
> > +    EXT_PACKAGE,
> > +    BUFFER,
> > +    RES_TEMPLATE,
> > +} AcpiBlockFlags;
> > +
> > +typedef struct AcpiAml {
> > +    GArray *buf;
> > +    uint8_t op;
> > +    AcpiBlockFlags block_flags;
> > +} AcpiAml;
> > +
> > +void aml_append(AcpiAml *parent_ctx, AcpiAml child);
> > +
> >  GArray *build_alloc_array(void);
> >  void build_free_array(GArray *array);
> >  void build_prepend_byte(GArray *array, uint8_t val);
> > -- 
> > 1.8.3.1

On Fri, Jan 23, 2015 at 11:35:29AM +0100, Igor Mammedov wrote:
> On Fri, 23 Jan 2015 10:11:19 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Jan 22, 2015 at 02:49:45PM +0000, Igor Mammedov wrote:
> > > Adds for dynamic AML creation, which will be used
> > > for piecing ASL/AML primitives together and hiding
> > > from user/caller details about how nested context
> > > should be closed/packed leaving less space for
> > > mistakes and necessity to know how AML should be
> > > encoded, allowing user to concentrate on ASL
> > > representation instead.
> > > 
> > > For example it will allow to create AML like this:
> > > 
> > > AcpiAml scope = acpi_scope("PCI0")
> > > AcpiAml dev = acpi_device("PM")
> > >     aml_append(&dev, acpi_name_decl("_ADR", acpi_int(addr)))
> > > aml_append(&scope, dev);
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  hw/acpi/acpi-build-utils.c         | 39 ++++++++++++++++++++++++++++++++++++++
> > >  include/hw/acpi/acpi-build-utils.h | 16 ++++++++++++++++
> > >  2 files changed, 55 insertions(+)
> > > 
> > > diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> > > index 602e68c..547ecaa 100644
> > > --- a/hw/acpi/acpi-build-utils.c
> > > +++ b/hw/acpi/acpi-build-utils.c
> > > @@ -267,3 +267,42 @@ void build_append_int(GArray *table, uint32_t value)
> > >          build_append_value(table, value, 4);
> > >      }
> > >  }
> > > +
> > > +static void build_prepend_int(GArray *array, uint32_t value)
> > > +{
> > > +    GArray *data = build_alloc_array();
> > > +
> > > +    build_append_int(data, value);
> > > +    g_array_prepend_vals(array, data->data, data->len);
> > > +    build_free_array(data);
> > > +}
> > 
> > I don't think prepend is generally justified:
> > it makes code hard to follow and debug.
> > 
> > Adding length is different: of course you need
> > to first have the package before you can add length.
> > 
> > We currently have build_prepend_package_length - just move it
> > to utils, and use everywhere.
> [...]
> > > +    case BUFFER:
> > > +        build_prepend_int(child.buf, child.buf->len);
> > > +        build_package(child.buf, child.op);
> Buffer uses the same concept as package, but adds its own additional length.
> Therefore I've added build_prepend_int(),
> I can create build_buffer() and mimic build_package()

Sounds good, pls do.
The point is to avoid generic prepend calls as an external API.

> but it won't change picture.

It's a better API - what is meant by picture?

> As for moving to to another file, during all this series lowlevel
> build_(some_aml_related_costruct_helper)s are moved into this file
> and should be make static to hide from user lowlevel helpers
> (including build_package).
> That will leave only high level API available.
> 
> TODO for me: make sure that moved lowlevel helpers are static
> 
> 
> > > +        break;
> > > +    default:
> > > +        break;
> > > +    }
> > > +    build_append_array(parent_ctx->buf, child.buf);
> > > +    build_free_array(child.buf);
> > > +}
> > > diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> > > index 199f003..64e7ec3 100644
> > > --- a/include/hw/acpi/acpi-build-utils.h
> > > +++ b/include/hw/acpi/acpi-build-utils.h
> > > @@ -5,6 +5,22 @@
> > >  #include <glib.h>
> > >  #include "qemu/compiler.h"
> > >  
> > > +typedef enum {
> > > +    NON_BLOCK,
> > > +    PACKAGE,
> > > +    EXT_PACKAGE,
> > > +    BUFFER,
> > > +    RES_TEMPLATE,
> > > +} AcpiBlockFlags;
> > > +
> > > +typedef struct AcpiAml {
> > > +    GArray *buf;
> > > +    uint8_t op;
> > > +    AcpiBlockFlags block_flags;
> > > +} AcpiAml;
> > > +
> > > +void aml_append(AcpiAml *parent_ctx, AcpiAml child);
> > > +
> > >  GArray *build_alloc_array(void);
> > >  void build_free_array(GArray *array);
> > >  void build_prepend_byte(GArray *array, uint8_t val);
> > > -- 
> > > 1.8.3.1

On Fri, Jan 23, 2015 at 11:03:54AM +0100, Igor Mammedov wrote:
> On Fri, 23 Jan 2015 10:03:03 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > > +typedef enum {
> > > +    NON_BLOCK,
> > > +    PACKAGE,
> > > +    EXT_PACKAGE,
> > > +    BUFFER,
> > > +    RES_TEMPLATE,
> > > +} AcpiBlockFlags;
> > 
> > Please prefix values with ACPI_BUILD_ - don't pollute the
> > global namespace.
> Could we use AML_ prefix instead?
> > Same elsewhere: add build_ to functions, and Build to types.
> Same here i.e. s/acpi_/aml_/ prefix in API calls?

OK.

> 
> > 
> > This makes it clear these are not Acpi spec types,
> > but helpers to build Aml.
> > 
> > > +
> > > +typedef struct AcpiAml {
> > > +    GArray *buf;
> > > +    uint8_t op;
> > > +    AcpiBlockFlags block_flags;
> > > +} AcpiAml;
> > > +
> > > +void aml_append(AcpiAml *parent_ctx, AcpiAml child);
> > > +
> > >  GArray *build_alloc_array(void);
> > >  void build_free_array(GArray *array);
> > >  void build_prepend_byte(GArray *array, uint8_t val);
> > > -- 
> > > 1.8.3.1
> >

On Fri, 23 Jan 2015 15:24:24 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jan 23, 2015 at 11:35:29AM +0100, Igor Mammedov wrote:
> > On Fri, 23 Jan 2015 10:11:19 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Thu, Jan 22, 2015 at 02:49:45PM +0000, Igor Mammedov wrote:
> > > > Adds for dynamic AML creation, which will be used
> > > > for piecing ASL/AML primitives together and hiding
> > > > from user/caller details about how nested context
> > > > should be closed/packed leaving less space for
> > > > mistakes and necessity to know how AML should be
> > > > encoded, allowing user to concentrate on ASL
> > > > representation instead.
> > > > 
> > > > For example it will allow to create AML like this:
> > > > 
> > > > AcpiAml scope = acpi_scope("PCI0")
> > > > AcpiAml dev = acpi_device("PM")
> > > >     aml_append(&dev, acpi_name_decl("_ADR", acpi_int(addr)))
> > > > aml_append(&scope, dev);
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >  hw/acpi/acpi-build-utils.c         | 39 ++++++++++++++++++++++++++++++++++++++
> > > >  include/hw/acpi/acpi-build-utils.h | 16 ++++++++++++++++
> > > >  2 files changed, 55 insertions(+)
> > > > 
> > > > diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> > > > index 602e68c..547ecaa 100644
> > > > --- a/hw/acpi/acpi-build-utils.c
> > > > +++ b/hw/acpi/acpi-build-utils.c
> > > > @@ -267,3 +267,42 @@ void build_append_int(GArray *table, uint32_t value)
> > > >          build_append_value(table, value, 4);
> > > >      }
> > > >  }
> > > > +
> > > > +static void build_prepend_int(GArray *array, uint32_t value)
> > > > +{
> > > > +    GArray *data = build_alloc_array();
> > > > +
> > > > +    build_append_int(data, value);
> > > > +    g_array_prepend_vals(array, data->data, data->len);
> > > > +    build_free_array(data);
> > > > +}
> > > 
> > > I don't think prepend is generally justified:
> > > it makes code hard to follow and debug.
> > > 
> > > Adding length is different: of course you need
> > > to first have the package before you can add length.
> > > 
> > > We currently have build_prepend_package_length - just move it
> > > to utils, and use everywhere.
> > [...]
> > > > +    case BUFFER:
> > > > +        build_prepend_int(child.buf, child.buf->len);
> > > > +        build_package(child.buf, child.op);
> > Buffer uses the same concept as package, but adds its own additional length.
> > Therefore I've added build_prepend_int(),
> > I can create build_buffer() and mimic build_package()
> 
> Sounds good, pls do.
> The point is to avoid generic prepend calls as an external API.
> 
> > but it won't change picture.
> 
> It's a better API - what is meant by picture?
build_prepend_int() is a static/non public function,
build_buffer() will also be static/non public function for use only by
API internals.

I pretty much hate long build_append_foo() names so I'm hiding all
lowlevel constructs and try to expose only high-level ASL ones.
Which makes me to think that we need to use asl_ prefix for API calls
instead of acpi_ or aml_.

> 
> > As for moving to to another file, during all this series lowlevel
> > build_(some_aml_related_costruct_helper)s are moved into this file
> > and should be make static to hide from user lowlevel helpers
> > (including build_package).
> > That will leave only high level API available.
> > 
> > TODO for me: make sure that moved lowlevel helpers are static
> > 
> > 
> > > > +        break;
> > > > +    default:
> > > > +        break;
> > > > +    }
> > > > +    build_append_array(parent_ctx->buf, child.buf);
> > > > +    build_free_array(child.buf);
> > > > +}
> > > > diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> > > > index 199f003..64e7ec3 100644
> > > > --- a/include/hw/acpi/acpi-build-utils.h
> > > > +++ b/include/hw/acpi/acpi-build-utils.h
> > > > @@ -5,6 +5,22 @@
> > > >  #include <glib.h>
> > > >  #include "qemu/compiler.h"
> > > >  
> > > > +typedef enum {
> > > > +    NON_BLOCK,
> > > > +    PACKAGE,
> > > > +    EXT_PACKAGE,
> > > > +    BUFFER,
> > > > +    RES_TEMPLATE,
> > > > +} AcpiBlockFlags;
> > > > +
> > > > +typedef struct AcpiAml {
> > > > +    GArray *buf;
> > > > +    uint8_t op;
> > > > +    AcpiBlockFlags block_flags;
> > > > +} AcpiAml;
> > > > +
> > > > +void aml_append(AcpiAml *parent_ctx, AcpiAml child);
> > > > +
> > > >  GArray *build_alloc_array(void);
> > > >  void build_free_array(GArray *array);
> > > >  void build_prepend_byte(GArray *array, uint8_t val);
> > > > -- 
> > > > 1.8.3.1

On Fri, Jan 23, 2015 at 02:40:30PM +0100, Igor Mammedov wrote:
> On Fri, 23 Jan 2015 15:24:24 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Jan 23, 2015 at 11:35:29AM +0100, Igor Mammedov wrote:
> > > On Fri, 23 Jan 2015 10:11:19 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Thu, Jan 22, 2015 at 02:49:45PM +0000, Igor Mammedov wrote:
> > > > > Adds for dynamic AML creation, which will be used
> > > > > for piecing ASL/AML primitives together and hiding
> > > > > from user/caller details about how nested context
> > > > > should be closed/packed leaving less space for
> > > > > mistakes and necessity to know how AML should be
> > > > > encoded, allowing user to concentrate on ASL
> > > > > representation instead.
> > > > > 
> > > > > For example it will allow to create AML like this:
> > > > > 
> > > > > AcpiAml scope = acpi_scope("PCI0")
> > > > > AcpiAml dev = acpi_device("PM")
> > > > >     aml_append(&dev, acpi_name_decl("_ADR", acpi_int(addr)))
> > > > > aml_append(&scope, dev);
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > ---
> > > > >  hw/acpi/acpi-build-utils.c         | 39 ++++++++++++++++++++++++++++++++++++++
> > > > >  include/hw/acpi/acpi-build-utils.h | 16 ++++++++++++++++
> > > > >  2 files changed, 55 insertions(+)
> > > > > 
> > > > > diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> > > > > index 602e68c..547ecaa 100644
> > > > > --- a/hw/acpi/acpi-build-utils.c
> > > > > +++ b/hw/acpi/acpi-build-utils.c
> > > > > @@ -267,3 +267,42 @@ void build_append_int(GArray *table, uint32_t value)
> > > > >          build_append_value(table, value, 4);
> > > > >      }
> > > > >  }
> > > > > +
> > > > > +static void build_prepend_int(GArray *array, uint32_t value)
> > > > > +{
> > > > > +    GArray *data = build_alloc_array();
> > > > > +
> > > > > +    build_append_int(data, value);
> > > > > +    g_array_prepend_vals(array, data->data, data->len);
> > > > > +    build_free_array(data);
> > > > > +}
> > > > 
> > > > I don't think prepend is generally justified:
> > > > it makes code hard to follow and debug.
> > > > 
> > > > Adding length is different: of course you need
> > > > to first have the package before you can add length.
> > > > 
> > > > We currently have build_prepend_package_length - just move it
> > > > to utils, and use everywhere.
> > > [...]
> > > > > +    case BUFFER:
> > > > > +        build_prepend_int(child.buf, child.buf->len);
> > > > > +        build_package(child.buf, child.op);
> > > Buffer uses the same concept as package, but adds its own additional length.
> > > Therefore I've added build_prepend_int(),
> > > I can create build_buffer() and mimic build_package()
> > 
> > Sounds good, pls do.
> > The point is to avoid generic prepend calls as an external API.
> > 
> > > but it won't change picture.
> > 
> > It's a better API - what is meant by picture?
> build_prepend_int() is a static/non public function,
> build_buffer() will also be static/non public function for use only by
> API internals.
> 
> I pretty much hate long build_append_foo() names so I'm hiding all
> lowlevel constructs and try to expose only high-level ASL ones.
> Which makes me to think that we need to use asl_ prefix for API calls
> instead of acpi_ or aml_.

This sounds wrong unless we either accept ASL input or
produce ASL output.

Igor, I think you are aiming a bit too high. Don't try to
write your own language, just use C. It does have
overhead like need to declare functions and variables,
and allocate/free memory, but they are well understood.


Your patches are almost there, they are pretty clean, the only issue I
think is this passing of AcpiAml by value, sometimes freeing buffer in
the process, sometimes not.

Just pass AcpiAml* everywhere, add APIs to allocate and free it
together with the internal buffer.
This makes it trivial to see that value is not misused:
just check it's between alloc and free - and that there are
no leaks - just check we call free on each value.
We can write a semantic patch to catch missing free calls,
it's easy.


> > 
> > > As for moving to to another file, during all this series lowlevel
> > > build_(some_aml_related_costruct_helper)s are moved into this file
> > > and should be make static to hide from user lowlevel helpers
> > > (including build_package).
> > > That will leave only high level API available.
> > > 
> > > TODO for me: make sure that moved lowlevel helpers are static
> > > 
> > > 
> > > > > +        break;
> > > > > +    default:
> > > > > +        break;
> > > > > +    }
> > > > > +    build_append_array(parent_ctx->buf, child.buf);
> > > > > +    build_free_array(child.buf);
> > > > > +}
> > > > > diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> > > > > index 199f003..64e7ec3 100644
> > > > > --- a/include/hw/acpi/acpi-build-utils.h
> > > > > +++ b/include/hw/acpi/acpi-build-utils.h
> > > > > @@ -5,6 +5,22 @@
> > > > >  #include <glib.h>
> > > > >  #include "qemu/compiler.h"
> > > > >  
> > > > > +typedef enum {
> > > > > +    NON_BLOCK,
> > > > > +    PACKAGE,
> > > > > +    EXT_PACKAGE,
> > > > > +    BUFFER,
> > > > > +    RES_TEMPLATE,
> > > > > +} AcpiBlockFlags;
> > > > > +
> > > > > +typedef struct AcpiAml {
> > > > > +    GArray *buf;
> > > > > +    uint8_t op;
> > > > > +    AcpiBlockFlags block_flags;
> > > > > +} AcpiAml;
> > > > > +
> > > > > +void aml_append(AcpiAml *parent_ctx, AcpiAml child);
> > > > > +
> > > > >  GArray *build_alloc_array(void);
> > > > >  void build_free_array(GArray *array);
> > > > >  void build_prepend_byte(GArray *array, uint8_t val);
> > > > > -- 
> > > > > 1.8.3.1

On Fri, 23 Jan 2015 15:55:11 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jan 23, 2015 at 02:40:30PM +0100, Igor Mammedov wrote:
> > On Fri, 23 Jan 2015 15:24:24 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Fri, Jan 23, 2015 at 11:35:29AM +0100, Igor Mammedov wrote:
> > > > On Fri, 23 Jan 2015 10:11:19 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Thu, Jan 22, 2015 at 02:49:45PM +0000, Igor Mammedov wrote:
> > > > > > Adds for dynamic AML creation, which will be used
> > > > > > for piecing ASL/AML primitives together and hiding
> > > > > > from user/caller details about how nested context
> > > > > > should be closed/packed leaving less space for
> > > > > > mistakes and necessity to know how AML should be
> > > > > > encoded, allowing user to concentrate on ASL
> > > > > > representation instead.
> > > > > > 
> > > > > > For example it will allow to create AML like this:
> > > > > > 
> > > > > > AcpiAml scope = acpi_scope("PCI0")
> > > > > > AcpiAml dev = acpi_device("PM")
> > > > > >     aml_append(&dev, acpi_name_decl("_ADR", acpi_int(addr)))
> > > > > > aml_append(&scope, dev);
> > > > > > 
> > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > ---
> > > > > >  hw/acpi/acpi-build-utils.c         | 39 ++++++++++++++++++++++++++++++++++++++
> > > > > >  include/hw/acpi/acpi-build-utils.h | 16 ++++++++++++++++
> > > > > >  2 files changed, 55 insertions(+)
> > > > > > 
> > > > > > diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> > > > > > index 602e68c..547ecaa 100644
> > > > > > --- a/hw/acpi/acpi-build-utils.c
> > > > > > +++ b/hw/acpi/acpi-build-utils.c
> > > > > > @@ -267,3 +267,42 @@ void build_append_int(GArray *table, uint32_t value)
> > > > > >          build_append_value(table, value, 4);
> > > > > >      }
> > > > > >  }
> > > > > > +
> > > > > > +static void build_prepend_int(GArray *array, uint32_t value)
> > > > > > +{
> > > > > > +    GArray *data = build_alloc_array();
> > > > > > +
> > > > > > +    build_append_int(data, value);
> > > > > > +    g_array_prepend_vals(array, data->data, data->len);
> > > > > > +    build_free_array(data);
> > > > > > +}
> > > > > 
> > > > > I don't think prepend is generally justified:
> > > > > it makes code hard to follow and debug.
> > > > > 
> > > > > Adding length is different: of course you need
> > > > > to first have the package before you can add length.
> > > > > 
> > > > > We currently have build_prepend_package_length - just move it
> > > > > to utils, and use everywhere.
> > > > [...]
> > > > > > +    case BUFFER:
> > > > > > +        build_prepend_int(child.buf, child.buf->len);
> > > > > > +        build_package(child.buf, child.op);
> > > > Buffer uses the same concept as package, but adds its own additional length.
> > > > Therefore I've added build_prepend_int(),
> > > > I can create build_buffer() and mimic build_package()
> > > 
> > > Sounds good, pls do.
> > > The point is to avoid generic prepend calls as an external API.
> > > 
> > > > but it won't change picture.
> > > 
> > > It's a better API - what is meant by picture?
> > build_prepend_int() is a static/non public function,
> > build_buffer() will also be static/non public function for use only by
> > API internals.
> > 
> > I pretty much hate long build_append_foo() names so I'm hiding all
> > lowlevel constructs and try to expose only high-level ASL ones.
> > Which makes me to think that we need to use asl_ prefix for API calls
> > instead of acpi_ or aml_.
> 
> This sounds wrong unless we either accept ASL input or
> produce ASL output.
> 
> Igor, I think you are aiming a bit too high. Don't try to
> write your own language, just use C. It does have
> overhead like need to declare functions and variables,
> and allocate/free memory, but they are well understood.
I refuse to give up on cleaner and simpler API yet :)

> 
> 
> Your patches are almost there, they are pretty clean, the only issue I
> think is this passing of AcpiAml by value, sometimes freeing buffer in
> the process, sometimes not.
Currently buffer is allocated by API and is always freed whenever
it's passed to another API function.
That's why it makes user not to care about memory mgmt.

The only limitation of it is if you store AcpiAml return value into some
variable you are responsible to use it only once for passing to another API
function. Reusing this variable's value (pass it to API function second time)
would cause cause use-after-free and freeing-freed bugs.
Like this:
AcpiAml table = acpi_definition_block("SSDT",...);
AcpiAml scope = acpi_scope("PCI0");
aml_append(&table, scope); // <- here scope becomes invalid
// a bug
aml_append(&table, scope); // use-after-free + freeing-freed bugs

There are several approaches to look for resolving above issues:
1. Adopt and use memory mgmt model used by GTK+
   in nutshell: http://www.cs.hunter.cuny.edu/~sweiss/course_materials/csci493.70/lecture_notes/GTK_memory_mngmt.pdf
   In particular adopt behavior of GInitiallyUnowned usage model

   that will allow to keep convenient chained call style and if necessary
   reuse objects returned by API by explicitly referencing/dereferencing
   them if needed.

2. It's possible to drop freeing inside API completely and
   record(store in list) every new object inside a table context.
   When table is constructed, list of created objects could be
   safely freed.
   With that it would be safe to reuse every AcpiAml object
   and avoid free-after-use issues with limitation that created
   AcpiAml objects shouldn't be used after table was closed.
   It should cover all practical use of API, i.e. no cross
   table AcpiAml objects.

3. talloc implementation Amit've mentioned,
   perhaps it might work since it allows to set destructors for
   managed pointers. With this we might get clear abort when
   dereferencing freed pointer see talloc_set()

> 
> Just pass AcpiAml* everywhere, add APIs to allocate and free it
> together with the internal buffer.
> This makes it trivial to see that value is not misused:
> just check it's between alloc and free - and that there are
> no leaks - just check we call free on each value.
> We can write a semantic patch to catch missing free calls,
> it's easy.
> 
> 
> > > 
> > > > As for moving to to another file, during all this series lowlevel
> > > > build_(some_aml_related_costruct_helper)s are moved into this file
> > > > and should be make static to hide from user lowlevel helpers
> > > > (including build_package).
> > > > That will leave only high level API available.
> > > > 
> > > > TODO for me: make sure that moved lowlevel helpers are static
> > > > 
> > > > 
> > > > > > +        break;
> > > > > > +    default:
> > > > > > +        break;
> > > > > > +    }
> > > > > > +    build_append_array(parent_ctx->buf, child.buf);
> > > > > > +    build_free_array(child.buf);
> > > > > > +}
> > > > > > diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> > > > > > index 199f003..64e7ec3 100644
> > > > > > --- a/include/hw/acpi/acpi-build-utils.h
> > > > > > +++ b/include/hw/acpi/acpi-build-utils.h
> > > > > > @@ -5,6 +5,22 @@
> > > > > >  #include <glib.h>
> > > > > >  #include "qemu/compiler.h"
> > > > > >  
> > > > > > +typedef enum {
> > > > > > +    NON_BLOCK,
> > > > > > +    PACKAGE,
> > > > > > +    EXT_PACKAGE,
> > > > > > +    BUFFER,
> > > > > > +    RES_TEMPLATE,
> > > > > > +} AcpiBlockFlags;
> > > > > > +
> > > > > > +typedef struct AcpiAml {
> > > > > > +    GArray *buf;
> > > > > > +    uint8_t op;
> > > > > > +    AcpiBlockFlags block_flags;
> > > > > > +} AcpiAml;
> > > > > > +
> > > > > > +void aml_append(AcpiAml *parent_ctx, AcpiAml child);
> > > > > > +
> > > > > >  GArray *build_alloc_array(void);
> > > > > >  void build_free_array(GArray *array);
> > > > > >  void build_prepend_byte(GArray *array, uint8_t val);
> > > > > > -- 
> > > > > > 1.8.3.1
>

On Fri, Jan 23, 2015 at 06:56:20PM +0100, Igor Mammedov wrote:
> On Fri, 23 Jan 2015 15:55:11 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Jan 23, 2015 at 02:40:30PM +0100, Igor Mammedov wrote:
> > > On Fri, 23 Jan 2015 15:24:24 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Fri, Jan 23, 2015 at 11:35:29AM +0100, Igor Mammedov wrote:
> > > > > On Fri, 23 Jan 2015 10:11:19 +0200
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > 
> > > > > > On Thu, Jan 22, 2015 at 02:49:45PM +0000, Igor Mammedov wrote:
> > > > > > > Adds for dynamic AML creation, which will be used
> > > > > > > for piecing ASL/AML primitives together and hiding
> > > > > > > from user/caller details about how nested context
> > > > > > > should be closed/packed leaving less space for
> > > > > > > mistakes and necessity to know how AML should be
> > > > > > > encoded, allowing user to concentrate on ASL
> > > > > > > representation instead.
> > > > > > > 
> > > > > > > For example it will allow to create AML like this:
> > > > > > > 
> > > > > > > AcpiAml scope = acpi_scope("PCI0")
> > > > > > > AcpiAml dev = acpi_device("PM")
> > > > > > >     aml_append(&dev, acpi_name_decl("_ADR", acpi_int(addr)))
> > > > > > > aml_append(&scope, dev);
> > > > > > > 
> > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > > ---
> > > > > > >  hw/acpi/acpi-build-utils.c         | 39 ++++++++++++++++++++++++++++++++++++++
> > > > > > >  include/hw/acpi/acpi-build-utils.h | 16 ++++++++++++++++
> > > > > > >  2 files changed, 55 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> > > > > > > index 602e68c..547ecaa 100644
> > > > > > > --- a/hw/acpi/acpi-build-utils.c
> > > > > > > +++ b/hw/acpi/acpi-build-utils.c
> > > > > > > @@ -267,3 +267,42 @@ void build_append_int(GArray *table, uint32_t value)
> > > > > > >          build_append_value(table, value, 4);
> > > > > > >      }
> > > > > > >  }
> > > > > > > +
> > > > > > > +static void build_prepend_int(GArray *array, uint32_t value)
> > > > > > > +{
> > > > > > > +    GArray *data = build_alloc_array();
> > > > > > > +
> > > > > > > +    build_append_int(data, value);
> > > > > > > +    g_array_prepend_vals(array, data->data, data->len);
> > > > > > > +    build_free_array(data);
> > > > > > > +}
> > > > > > 
> > > > > > I don't think prepend is generally justified:
> > > > > > it makes code hard to follow and debug.
> > > > > > 
> > > > > > Adding length is different: of course you need
> > > > > > to first have the package before you can add length.
> > > > > > 
> > > > > > We currently have build_prepend_package_length - just move it
> > > > > > to utils, and use everywhere.
> > > > > [...]
> > > > > > > +    case BUFFER:
> > > > > > > +        build_prepend_int(child.buf, child.buf->len);
> > > > > > > +        build_package(child.buf, child.op);
> > > > > Buffer uses the same concept as package, but adds its own additional length.
> > > > > Therefore I've added build_prepend_int(),
> > > > > I can create build_buffer() and mimic build_package()
> > > > 
> > > > Sounds good, pls do.
> > > > The point is to avoid generic prepend calls as an external API.
> > > > 
> > > > > but it won't change picture.
> > > > 
> > > > It's a better API - what is meant by picture?
> > > build_prepend_int() is a static/non public function,
> > > build_buffer() will also be static/non public function for use only by
> > > API internals.
> > > 
> > > I pretty much hate long build_append_foo() names so I'm hiding all
> > > lowlevel constructs and try to expose only high-level ASL ones.
> > > Which makes me to think that we need to use asl_ prefix for API calls
> > > instead of acpi_ or aml_.
> > 
> > This sounds wrong unless we either accept ASL input or
> > produce ASL output.
> > 
> > Igor, I think you are aiming a bit too high. Don't try to
> > write your own language, just use C. It does have
> > overhead like need to declare functions and variables,
> > and allocate/free memory, but they are well understood.
> I refuse to give up on cleaner and simpler API yet :)
> 
> > 
> > 
> > Your patches are almost there, they are pretty clean, the only issue I
> > think is this passing of AcpiAml by value, sometimes freeing buffer in
> > the process, sometimes not.
> Currently buffer is allocated by API and is always freed whenever
> it's passed to another API function.
> That's why it makes user not to care about memory mgmt.
> 
> The only limitation of it is if you store AcpiAml return value into some
> variable you are responsible to use it only once for passing to another API
> function. Reusing this variable's value (pass it to API function second time)
> would cause cause use-after-free and freeing-freed bugs.
> Like this:
> AcpiAml table = acpi_definition_block("SSDT",...);
> AcpiAml scope = acpi_scope("PCI0");
> aml_append(&table, scope); // <- here scope becomes invalid
> // a bug
> aml_append(&table, scope); // use-after-free + freeing-freed bugs
> 
> There are several approaches to look for resolving above issues:
> 1. Adopt and use memory mgmt model used by GTK+
>    in nutshell: http://www.cs.hunter.cuny.edu/~sweiss/course_materials/csci493.70/lecture_notes/GTK_memory_mngmt.pdf
>    In particular adopt behavior of GInitiallyUnowned usage model
> 
>    that will allow to keep convenient chained call style and if necessary
>    reuse objects returned by API by explicitly referencing/dereferencing
>    them if needed.

Hmm, it's still easy to misuse. I think I prefer option 2 below.

> 2. It's possible to drop freeing inside API completely and
>    record(store in list) every new object inside a table context.
>    When table is constructed, list of created objects could be
>    safely freed.
>    With that it would be safe to reuse every AcpiAml object
>    and avoid free-after-use issues with limitation that created
>    AcpiAml objects shouldn't be used after table was closed.
>    It should cover all practical use of API, i.e. no cross
>    table AcpiAml objects.

So each aml_alloc function gets pointer to this list,
and adds the new element there.
Eventually we do free_all to free all elements,
so there isn't even an aml_free to mis-use.

Good idea! I think this will address the issue.


> 3. talloc implementation Amit've mentioned,
>    perhaps it might work since it allows to set destructors for
>    managed pointers. With this we might get clear abort when
>    dereferencing freed pointer see talloc_set()


I think it's a separate discussion. Maybe talloc is a good
allocator to use in qemu, but using a separate allocator
just for acpi generation would be an overkill.

> 
> > 
> > Just pass AcpiAml* everywhere, add APIs to allocate and free it
> > together with the internal buffer.
> > This makes it trivial to see that value is not misused:
> > just check it's between alloc and free - and that there are
> > no leaks - just check we call free on each value.
> > We can write a semantic patch to catch missing free calls,
> > it's easy.
> > 
> > 
> > > > 
> > > > > As for moving to to another file, during all this series lowlevel
> > > > > build_(some_aml_related_costruct_helper)s are moved into this file
> > > > > and should be make static to hide from user lowlevel helpers
> > > > > (including build_package).
> > > > > That will leave only high level API available.
> > > > > 
> > > > > TODO for me: make sure that moved lowlevel helpers are static
> > > > > 
> > > > > 
> > > > > > > +        break;
> > > > > > > +    default:
> > > > > > > +        break;
> > > > > > > +    }
> > > > > > > +    build_append_array(parent_ctx->buf, child.buf);
> > > > > > > +    build_free_array(child.buf);
> > > > > > > +}
> > > > > > > diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> > > > > > > index 199f003..64e7ec3 100644
> > > > > > > --- a/include/hw/acpi/acpi-build-utils.h
> > > > > > > +++ b/include/hw/acpi/acpi-build-utils.h
> > > > > > > @@ -5,6 +5,22 @@
> > > > > > >  #include <glib.h>
> > > > > > >  #include "qemu/compiler.h"
> > > > > > >  
> > > > > > > +typedef enum {
> > > > > > > +    NON_BLOCK,
> > > > > > > +    PACKAGE,
> > > > > > > +    EXT_PACKAGE,
> > > > > > > +    BUFFER,
> > > > > > > +    RES_TEMPLATE,
> > > > > > > +} AcpiBlockFlags;
> > > > > > > +
> > > > > > > +typedef struct AcpiAml {
> > > > > > > +    GArray *buf;
> > > > > > > +    uint8_t op;
> > > > > > > +    AcpiBlockFlags block_flags;
> > > > > > > +} AcpiAml;
> > > > > > > +
> > > > > > > +void aml_append(AcpiAml *parent_ctx, AcpiAml child);
> > > > > > > +
> > > > > > >  GArray *build_alloc_array(void);
> > > > > > >  void build_free_array(GArray *array);
> > > > > > >  void build_prepend_byte(GArray *array, uint8_t val);
> > > > > > > -- 
> > > > > > > 1.8.3.1
> >

On Sat, 24 Jan 2015 18:33:50 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jan 23, 2015 at 06:56:20PM +0100, Igor Mammedov wrote:
> > On Fri, 23 Jan 2015 15:55:11 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Fri, Jan 23, 2015 at 02:40:30PM +0100, Igor Mammedov wrote:
> > > > On Fri, 23 Jan 2015 15:24:24 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Fri, Jan 23, 2015 at 11:35:29AM +0100, Igor Mammedov wrote:
> > > > > > On Fri, 23 Jan 2015 10:11:19 +0200
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > 
> > > > > > > On Thu, Jan 22, 2015 at 02:49:45PM +0000, Igor Mammedov wrote:
> > > > > > > > Adds for dynamic AML creation, which will be used
> > > > > > > > for piecing ASL/AML primitives together and hiding
> > > > > > > > from user/caller details about how nested context
> > > > > > > > should be closed/packed leaving less space for
> > > > > > > > mistakes and necessity to know how AML should be
> > > > > > > > encoded, allowing user to concentrate on ASL
> > > > > > > > representation instead.
> > > > > > > > 
> > > > > > > > For example it will allow to create AML like this:
> > > > > > > > 
> > > > > > > > AcpiAml scope = acpi_scope("PCI0")
> > > > > > > > AcpiAml dev = acpi_device("PM")
> > > > > > > >     aml_append(&dev, acpi_name_decl("_ADR", acpi_int(addr)))
> > > > > > > > aml_append(&scope, dev);
> > > > > > > > 
> > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > > > ---
> > > > > > > >  hw/acpi/acpi-build-utils.c         | 39 ++++++++++++++++++++++++++++++++++++++
> > > > > > > >  include/hw/acpi/acpi-build-utils.h | 16 ++++++++++++++++
> > > > > > > >  2 files changed, 55 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> > > > > > > > index 602e68c..547ecaa 100644
> > > > > > > > --- a/hw/acpi/acpi-build-utils.c
> > > > > > > > +++ b/hw/acpi/acpi-build-utils.c
> > > > > > > > @@ -267,3 +267,42 @@ void build_append_int(GArray *table, uint32_t value)
> > > > > > > >          build_append_value(table, value, 4);
> > > > > > > >      }
> > > > > > > >  }
> > > > > > > > +
> > > > > > > > +static void build_prepend_int(GArray *array, uint32_t value)
> > > > > > > > +{
> > > > > > > > +    GArray *data = build_alloc_array();
> > > > > > > > +
> > > > > > > > +    build_append_int(data, value);
> > > > > > > > +    g_array_prepend_vals(array, data->data, data->len);
> > > > > > > > +    build_free_array(data);
> > > > > > > > +}
> > > > > > > 
> > > > > > > I don't think prepend is generally justified:
> > > > > > > it makes code hard to follow and debug.
> > > > > > > 
> > > > > > > Adding length is different: of course you need
> > > > > > > to first have the package before you can add length.
> > > > > > > 
> > > > > > > We currently have build_prepend_package_length - just move it
> > > > > > > to utils, and use everywhere.
> > > > > > [...]
> > > > > > > > +    case BUFFER:
> > > > > > > > +        build_prepend_int(child.buf, child.buf->len);
> > > > > > > > +        build_package(child.buf, child.op);
> > > > > > Buffer uses the same concept as package, but adds its own additional length.
> > > > > > Therefore I've added build_prepend_int(),
> > > > > > I can create build_buffer() and mimic build_package()
> > > > > 
> > > > > Sounds good, pls do.
> > > > > The point is to avoid generic prepend calls as an external API.
> > > > > 
> > > > > > but it won't change picture.
> > > > > 
> > > > > It's a better API - what is meant by picture?
> > > > build_prepend_int() is a static/non public function,
> > > > build_buffer() will also be static/non public function for use only by
> > > > API internals.
> > > > 
> > > > I pretty much hate long build_append_foo() names so I'm hiding all
> > > > lowlevel constructs and try to expose only high-level ASL ones.
> > > > Which makes me to think that we need to use asl_ prefix for API calls
> > > > instead of acpi_ or aml_.
> > > 
> > > This sounds wrong unless we either accept ASL input or
> > > produce ASL output.
> > > 
> > > Igor, I think you are aiming a bit too high. Don't try to
> > > write your own language, just use C. It does have
> > > overhead like need to declare functions and variables,
> > > and allocate/free memory, but they are well understood.
> > I refuse to give up on cleaner and simpler API yet :)
> > 
> > > 
> > > 
> > > Your patches are almost there, they are pretty clean, the only issue I
> > > think is this passing of AcpiAml by value, sometimes freeing buffer in
> > > the process, sometimes not.
> > Currently buffer is allocated by API and is always freed whenever
> > it's passed to another API function.
> > That's why it makes user not to care about memory mgmt.
> > 
> > The only limitation of it is if you store AcpiAml return value into some
> > variable you are responsible to use it only once for passing to another API
> > function. Reusing this variable's value (pass it to API function second time)
> > would cause cause use-after-free and freeing-freed bugs.
> > Like this:
> > AcpiAml table = acpi_definition_block("SSDT",...);
> > AcpiAml scope = acpi_scope("PCI0");
> > aml_append(&table, scope); // <- here scope becomes invalid
> > // a bug
> > aml_append(&table, scope); // use-after-free + freeing-freed bugs
> > 
> > There are several approaches to look for resolving above issues:
> > 1. Adopt and use memory mgmt model used by GTK+
> >    in nutshell: http://www.cs.hunter.cuny.edu/~sweiss/course_materials/csci493.70/lecture_notes/GTK_memory_mngmt.pdf
> >    In particular adopt behavior of GInitiallyUnowned usage model
> > 
> >    that will allow to keep convenient chained call style and if necessary
> >    reuse objects returned by API by explicitly referencing/dereferencing
> >    them if needed.
> 
> Hmm, it's still easy to misuse. I think I prefer option 2 below.
That's basically what we have/use in QOM with object_new(FOO) + object_unref()
I have no idea why we invented our own Object infrastructure
when we could just use GObject one from already used glib.

> 
> > 2. It's possible to drop freeing inside API completely and
> >    record(store in list) every new object inside a table context.
> >    When table is constructed, list of created objects could be
> >    safely freed.
> >    With that it would be safe to reuse every AcpiAml object
> >    and avoid free-after-use issues with limitation that created
> >    AcpiAml objects shouldn't be used after table was closed.
> >    It should cover all practical use of API, i.e. no cross
> >    table AcpiAml objects.
> 
> So each aml_alloc function gets pointer to this list,
> and adds the new element there.
> Eventually we do free_all to free all elements,
> so there isn't even an aml_free to mis-use.
I'm thinking a little bit different about implementation though.
I still don't like the use of explicit alloc/free being called
by API user since it doesn't allow chained API calls and
I think it's unnecessary complication see below why.

Here is what's true about current API and a I'd like to with it:

  1. Every API call (except aml_append) makes aml_alloc(), it's just
     like a wrapper about object_new(FOO). (current + new impl.)

  2 Every API call that takes AML type as input argument
  2.1 consumes (frees) it (current impl.)
      (it's easy to fix use after free concern too,
       just pass AML by pointer and zero-out memory before it's freed
       and assert whenever one of input arguments is not correct,
       i.e. it was reused second time)
      There is no need for following steps after this one.
  2.2 takes ownership of GInitiallyUnowned and adds it to its list
      of its children.
  3. Free children when AML object is destroyed (i.e. ref count zero)
     That way when toplevel table object (definition block in 42/47)
     is added to ACPI blob we can unref it, which will cause
     its whole children tree freed, except for AML objects where
     API user explicitly took extra reference (i.e. wanted them
     to reuse in another table)

I'd prefer:
 *  2.1 way to address your current concern of use-after-free
    as the most simplest one (no reuse is possible however)
or
 * follow already used by QEMU QOM/GObject pattern of
   implicit alloc/free

since they allow to construct AML in a more simple/manageable way i.e.
 
  aml_append(method,
      aml_store(aml_string("foo"), aml_local(0)))
  );

v.s. explicit headache of alloc/free, which doesn't fix
     use-after-free anyway and just adds more boiler plate
     plus makes code har to read read

  str = aml_alloc();
  aml_string(str, "foo");
  loc0 = aml_alloc();
  aml_local(loc0, 0);
  store = aml_alloc();
  aml_store(store, str, loc0);
  aml_append(method, store);
  aml_free(store);
  aml_free(loc0);
  aml_free(str);

> 
> Good idea! I think this will address the issue.
> 
> 
> > 3. talloc implementation Amit've mentioned,
> >    perhaps it might work since it allows to set destructors for
> >    managed pointers. With this we might get clear abort when
> >    dereferencing freed pointer see talloc_set()
> 
> 
> I think it's a separate discussion. Maybe talloc is a good
> allocator to use in qemu, but using a separate allocator
> just for acpi generation would be an overkill.
> 
> > 
> > > 
> > > Just pass AcpiAml* everywhere, add APIs to allocate and free it
> > > together with the internal buffer.
> > > This makes it trivial to see that value is not misused:
> > > just check it's between alloc and free - and that there are
> > > no leaks - just check we call free on each value.
> > > We can write a semantic patch to catch missing free calls,
> > > it's easy.
> > > 
> > > 
> > > > > 
> > > > > > As for moving to to another file, during all this series lowlevel
> > > > > > build_(some_aml_related_costruct_helper)s are moved into this file
> > > > > > and should be make static to hide from user lowlevel helpers
> > > > > > (including build_package).
> > > > > > That will leave only high level API available.
> > > > > > 
> > > > > > TODO for me: make sure that moved lowlevel helpers are static
> > > > > > 
> > > > > > 
> > > > > > > > +        break;
> > > > > > > > +    default:
> > > > > > > > +        break;
> > > > > > > > +    }
> > > > > > > > +    build_append_array(parent_ctx->buf, child.buf);
> > > > > > > > +    build_free_array(child.buf);
> > > > > > > > +}
> > > > > > > > diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> > > > > > > > index 199f003..64e7ec3 100644
> > > > > > > > --- a/include/hw/acpi/acpi-build-utils.h
> > > > > > > > +++ b/include/hw/acpi/acpi-build-utils.h
> > > > > > > > @@ -5,6 +5,22 @@
> > > > > > > >  #include <glib.h>
> > > > > > > >  #include "qemu/compiler.h"
> > > > > > > >  
> > > > > > > > +typedef enum {
> > > > > > > > +    NON_BLOCK,
> > > > > > > > +    PACKAGE,
> > > > > > > > +    EXT_PACKAGE,
> > > > > > > > +    BUFFER,
> > > > > > > > +    RES_TEMPLATE,
> > > > > > > > +} AcpiBlockFlags;
> > > > > > > > +
> > > > > > > > +typedef struct AcpiAml {
> > > > > > > > +    GArray *buf;
> > > > > > > > +    uint8_t op;
> > > > > > > > +    AcpiBlockFlags block_flags;
> > > > > > > > +} AcpiAml;
> > > > > > > > +
> > > > > > > > +void aml_append(AcpiAml *parent_ctx, AcpiAml child);
> > > > > > > > +
> > > > > > > >  GArray *build_alloc_array(void);
> > > > > > > >  void build_free_array(GArray *array);
> > > > > > > >  void build_prepend_byte(GArray *array, uint8_t val);
> > > > > > > > -- 
> > > > > > > > 1.8.3.1
> > >

> v.s. explicit headache of alloc/free, which doesn't fix
>      use-after-free anyway and just adds more boiler plate
>      plus makes code har to read read
> 
>   str = aml_alloc();
>   aml_string(str, "foo");
>   loc0 = aml_alloc();
>   aml_local(loc0, 0);
>   store = aml_alloc();
>   aml_store(store, str, loc0);
>   aml_append(method, store);
>   aml_free(store);
>   aml_free(loc0);
>   aml_free(str);

Looks like I wasn't clear.  This is what I propose:

void aml_add_method(AmlPool *pool, AmlBlob *aml)
{
   AmlBlob *str = aml_alloc(pool);
   aml_string(str, "foo");
   loc0 = aml_alloc(pool);
   aml_local(loc0, 0);
   AmlBob *store = aml_alloc(pool);
   aml_store(store, str, loc0);
   aml_append(method, store);
}


So just propagare AmlPool* everywhere, don't free.

Then at top level:

	AmlPool *pool = aml_pool_alloc();

	AmlSsdt = aml_add_ssdt(pool, ....);

	....

	aml_pool_free(pool);



So from API perspective, this is very close to
what you posted, with just two changes:

	- pass pool parameter everywhere
	- have an extra alloc/free in only one place.

Happy?

On Mon, 26 Jan 2015 10:57:21 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Sat, 24 Jan 2015 18:33:50 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Jan 23, 2015 at 06:56:20PM +0100, Igor Mammedov wrote:
> > > On Fri, 23 Jan 2015 15:55:11 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
[...]
> > > I refuse to give up on cleaner and simpler API yet :)
> > > 
> > > > 
> > > > 
> > > > Your patches are almost there, they are pretty clean, the only issue I
> > > > think is this passing of AcpiAml by value, sometimes freeing buffer in
> > > > the process, sometimes not.
> > > Currently buffer is allocated by API and is always freed whenever
> > > it's passed to another API function.
> > > That's why it makes user not to care about memory mgmt.
> > > 
> > > The only limitation of it is if you store AcpiAml return value into some
> > > variable you are responsible to use it only once for passing to another API
> > > function. Reusing this variable's value (pass it to API function second time)
> > > would cause cause use-after-free and freeing-freed bugs.
> > > Like this:
> > > AcpiAml table = acpi_definition_block("SSDT",...);
> > > AcpiAml scope = acpi_scope("PCI0");
> > > aml_append(&table, scope); // <- here scope becomes invalid
> > > // a bug
> > > aml_append(&table, scope); // use-after-free + freeing-freed bugs
> > > 
> > > There are several approaches to look for resolving above issues:
> > > 1. Adopt and use memory mgmt model used by GTK+
> > >    in nutshell: http://www.cs.hunter.cuny.edu/~sweiss/course_materials/csci493.70/lecture_notes/GTK_memory_mngmt.pdf
> > >    In particular adopt behavior of GInitiallyUnowned usage model
> > > 
> > >    that will allow to keep convenient chained call style and if necessary
> > >    reuse objects returned by API by explicitly referencing/dereferencing
> > >    them if needed.
> > 
> > Hmm, it's still easy to misuse. I think I prefer option 2 below.
> That's basically what we have/use in QOM with object_new(FOO) + object_unref()
> I have no idea why we invented our own Object infrastructure
> when we could just use GObject one from already used glib.
> 
> > 
> > > 2. It's possible to drop freeing inside API completely and
> > >    record(store in list) every new object inside a table context.
> > >    When table is constructed, list of created objects could be
> > >    safely freed.
> > >    With that it would be safe to reuse every AcpiAml object
> > >    and avoid free-after-use issues with limitation that created
> > >    AcpiAml objects shouldn't be used after table was closed.
> > >    It should cover all practical use of API, i.e. no cross
> > >    table AcpiAml objects.
> > 
> > So each aml_alloc function gets pointer to this list,
> > and adds the new element there.
> > Eventually we do free_all to free all elements,
> > so there isn't even an aml_free to mis-use.
> I'm thinking a little bit different about implementation though.
> I still don't like the use of explicit alloc/free being called
> by API user since it doesn't allow chained API calls and
> I think it's unnecessary complication see below why.
> 
> Here is what's true about current API and a I'd like to with it:
> 
>   1. Every API call (except aml_append) makes aml_alloc(), it's just
>      like a wrapper about object_new(FOO). (current + new impl.)
> 
>   2 Every API call that takes AML type as input argument
>   2.1 consumes (frees) it (current impl.)
>       (it's easy to fix use after free concern too,
>        just pass AML by pointer and zero-out memory before it's freed
>        and assert whenever one of input arguments is not correct,
>        i.e. it was reused second time)
>       There is no need for following steps after this one.
>   2.2 takes ownership of GInitiallyUnowned and adds it to its list
>       of its children.
>   3. Free children when AML object is destroyed (i.e. ref count zero)
>      That way when toplevel table object (definition block in 42/47)
>      is added to ACPI blob we can unref it, which will cause
>      its whole children tree freed, except for AML objects where
>      API user explicitly took extra reference (i.e. wanted them
>      to reuse in another table)
> 
> I'd prefer:
>  *  2.1 way to address your current concern of use-after-free
>     as the most simplest one (no reuse is possible however)
> or
>  * follow already used by QEMU QOM/GObject pattern of
>    implicit alloc/free
> 
> since they allow to construct AML in a more simple/manageable way i.e.
>  
>   aml_append(method,
>       aml_store(aml_string("foo"), aml_local(0)))
>   );
> 
> v.s. explicit headache of alloc/free, which doesn't fix
>      use-after-free anyway and just adds more boiler plate
>      plus makes code har to read read
> 
>   str = aml_alloc();
>   aml_string(str, "foo");
>   loc0 = aml_alloc();
>   aml_local(loc0, 0);
>   store = aml_alloc();
>   aml_store(store, str, loc0);
>   aml_append(method, store);
>   aml_free(store);
>   aml_free(loc0);
>   aml_free(str);

Here is a compromise what I and Michael came to on a phone call:

Externally API usage would look like:

AmlAllocList *p = some_list_alloc();

Aml *ssdt = aml_def_block(p, "SSDT", ...);
Aml *dev = aml_device(p, "PCI0");
aml_append(dev,
    aml_name_def(p, "_STA", aml_int(p, 0xF /* present */))
);
aml_append(ssdt, dev);

aml_append(acpi_tables_blob, ssdt);

free_aml_alloc_list(p);


Each of aml_foo() will take other Aml arguments by pointer.
Also every aml_foo(), except of aml_append() will allocate
Aml struct and return pointer to it and also add this pointer
into AmlAllocList which is passed as first argument to each
aml_foo() call.
aml_append() becomes nondestructive function and just adds
child(2nd arg) to the parent context (1st arg).

After API user is done with building table and pushed it
into tables blob, he/she calls free_aml_alloc_list() to free
all Aml objects created during process of building the table
content.

> 
> > 
> > Good idea! I think this will address the issue.
> > 
> > 
> > > 3. talloc implementation Amit've mentioned,
> > >    perhaps it might work since it allows to set destructors for
> > >    managed pointers. With this we might get clear abort when
> > >    dereferencing freed pointer see talloc_set()
> > 
> > 
> > I think it's a separate discussion. Maybe talloc is a good
> > allocator to use in qemu, but using a separate allocator
> > just for acpi generation would be an overkill.
> > 
> > > 
> > > > 
> > > > Just pass AcpiAml* everywhere, add APIs to allocate and free it
> > > > together with the internal buffer.
> > > > This makes it trivial to see that value is not misused:
> > > > just check it's between alloc and free - and that there are
> > > > no leaks - just check we call free on each value.
> > > > We can write a semantic patch to catch missing free calls,
> > > > it's easy.
> > > > 
> > > > 
> > > > > > 
> > > > > > > As for moving to to another file, during all this series lowlevel
> > > > > > > build_(some_aml_related_costruct_helper)s are moved into this file
> > > > > > > and should be make static to hide from user lowlevel helpers
> > > > > > > (including build_package).
> > > > > > > That will leave only high level API available.
> > > > > > > 
> > > > > > > TODO for me: make sure that moved lowlevel helpers are static
> > > > > > > 
> > > > > > >

On Mon, Jan 26, 2015 at 04:09:20PM +0100, Igor Mammedov wrote:
> On Mon, 26 Jan 2015 10:57:21 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Sat, 24 Jan 2015 18:33:50 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Fri, Jan 23, 2015 at 06:56:20PM +0100, Igor Mammedov wrote:
> > > > On Fri, 23 Jan 2015 15:55:11 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> [...]
> > > > I refuse to give up on cleaner and simpler API yet :)
> > > > 
> > > > > 
> > > > > 
> > > > > Your patches are almost there, they are pretty clean, the only issue I
> > > > > think is this passing of AcpiAml by value, sometimes freeing buffer in
> > > > > the process, sometimes not.
> > > > Currently buffer is allocated by API and is always freed whenever
> > > > it's passed to another API function.
> > > > That's why it makes user not to care about memory mgmt.
> > > > 
> > > > The only limitation of it is if you store AcpiAml return value into some
> > > > variable you are responsible to use it only once for passing to another API
> > > > function. Reusing this variable's value (pass it to API function second time)
> > > > would cause cause use-after-free and freeing-freed bugs.
> > > > Like this:
> > > > AcpiAml table = acpi_definition_block("SSDT",...);
> > > > AcpiAml scope = acpi_scope("PCI0");
> > > > aml_append(&table, scope); // <- here scope becomes invalid
> > > > // a bug
> > > > aml_append(&table, scope); // use-after-free + freeing-freed bugs
> > > > 
> > > > There are several approaches to look for resolving above issues:
> > > > 1. Adopt and use memory mgmt model used by GTK+
> > > >    in nutshell: http://www.cs.hunter.cuny.edu/~sweiss/course_materials/csci493.70/lecture_notes/GTK_memory_mngmt.pdf
> > > >    In particular adopt behavior of GInitiallyUnowned usage model
> > > > 
> > > >    that will allow to keep convenient chained call style and if necessary
> > > >    reuse objects returned by API by explicitly referencing/dereferencing
> > > >    them if needed.
> > > 
> > > Hmm, it's still easy to misuse. I think I prefer option 2 below.
> > That's basically what we have/use in QOM with object_new(FOO) + object_unref()
> > I have no idea why we invented our own Object infrastructure
> > when we could just use GObject one from already used glib.
> > 
> > > 
> > > > 2. It's possible to drop freeing inside API completely and
> > > >    record(store in list) every new object inside a table context.
> > > >    When table is constructed, list of created objects could be
> > > >    safely freed.
> > > >    With that it would be safe to reuse every AcpiAml object
> > > >    and avoid free-after-use issues with limitation that created
> > > >    AcpiAml objects shouldn't be used after table was closed.
> > > >    It should cover all practical use of API, i.e. no cross
> > > >    table AcpiAml objects.
> > > 
> > > So each aml_alloc function gets pointer to this list,
> > > and adds the new element there.
> > > Eventually we do free_all to free all elements,
> > > so there isn't even an aml_free to mis-use.
> > I'm thinking a little bit different about implementation though.
> > I still don't like the use of explicit alloc/free being called
> > by API user since it doesn't allow chained API calls and
> > I think it's unnecessary complication see below why.
> > 
> > Here is what's true about current API and a I'd like to with it:
> > 
> >   1. Every API call (except aml_append) makes aml_alloc(), it's just
> >      like a wrapper about object_new(FOO). (current + new impl.)
> > 
> >   2 Every API call that takes AML type as input argument
> >   2.1 consumes (frees) it (current impl.)
> >       (it's easy to fix use after free concern too,
> >        just pass AML by pointer and zero-out memory before it's freed
> >        and assert whenever one of input arguments is not correct,
> >        i.e. it was reused second time)
> >       There is no need for following steps after this one.
> >   2.2 takes ownership of GInitiallyUnowned and adds it to its list
> >       of its children.
> >   3. Free children when AML object is destroyed (i.e. ref count zero)
> >      That way when toplevel table object (definition block in 42/47)
> >      is added to ACPI blob we can unref it, which will cause
> >      its whole children tree freed, except for AML objects where
> >      API user explicitly took extra reference (i.e. wanted them
> >      to reuse in another table)
> > 
> > I'd prefer:
> >  *  2.1 way to address your current concern of use-after-free
> >     as the most simplest one (no reuse is possible however)
> > or
> >  * follow already used by QEMU QOM/GObject pattern of
> >    implicit alloc/free
> > 
> > since they allow to construct AML in a more simple/manageable way i.e.
> >  
> >   aml_append(method,
> >       aml_store(aml_string("foo"), aml_local(0)))
> >   );
> > 
> > v.s. explicit headache of alloc/free, which doesn't fix
> >      use-after-free anyway and just adds more boiler plate
> >      plus makes code har to read read
> > 
> >   str = aml_alloc();
> >   aml_string(str, "foo");
> >   loc0 = aml_alloc();
> >   aml_local(loc0, 0);
> >   store = aml_alloc();
> >   aml_store(store, str, loc0);
> >   aml_append(method, store);
> >   aml_free(store);
> >   aml_free(loc0);
> >   aml_free(str);
> 
> Here is a compromise what I and Michael came to on a phone call:
> 
> Externally API usage would look like:
> 
> AmlAllocList *p = some_list_alloc();
> 
> Aml *ssdt = aml_def_block(p, "SSDT", ...);
> Aml *dev = aml_device(p, "PCI0");
> aml_append(dev,
>     aml_name_def(p, "_STA", aml_int(p, 0xF /* present */))
> );
> aml_append(ssdt, dev);
> 
> aml_append(acpi_tables_blob, ssdt);
> 
> free_aml_alloc_list(p);
> 
> 
> Each of aml_foo() will take other Aml arguments by pointer.
> Also every aml_foo(), except of aml_append() will allocate
> Aml struct and return pointer to it and also add this pointer
> into AmlAllocList which is passed as first argument to each
> aml_foo() call.
> aml_append() becomes nondestructive function and just adds
> child(2nd arg) to the parent context (1st arg).
> 
> After API user is done with building table and pushed it
> into tables blob, he/she calls free_aml_alloc_list() to free
> all Aml objects created during process of building the table
> content.

Hmm, passing 'p' around somewhat muddies an otherwise clean
interface, but the concern with aml_append silently freeing
memory still accessible by the caller is definitely valid. I
only wonder how things would look with Igor's option 2.2 above.
The caller still only needs to free the final table, but it
also becomes safe to use the same object references multiple
times before freeing the table. Using QOM also seems reasonable
to me, as it appears it's the accepted way to do garbage
collection in QEMU. Is it possible to do 2.2 with QOM?

> 
> > 
> > > 
> > > Good idea! I think this will address the issue.
> > > 
> > > 
> > > > 3. talloc implementation Amit've mentioned,
> > > >    perhaps it might work since it allows to set destructors for
> > > >    managed pointers. With this we might get clear abort when
> > > >    dereferencing freed pointer see talloc_set()
> > > 
> > > 
> > > I think it's a separate discussion. Maybe talloc is a good
> > > allocator to use in qemu, but using a separate allocator
> > > just for acpi generation would be an overkill.
> > > 
> > > > 
> > > > > 
> > > > > Just pass AcpiAml* everywhere, add APIs to allocate and free it
> > > > > together with the internal buffer.
> > > > > This makes it trivial to see that value is not misused:
> > > > > just check it's between alloc and free - and that there are
> > > > > no leaks - just check we call free on each value.
> > > > > We can write a semantic patch to catch missing free calls,
> > > > > it's easy.
> > > > > 
> > > > > 
> > > > > > > 
> > > > > > > > As for moving to to another file, during all this series lowlevel
> > > > > > > > build_(some_aml_related_costruct_helper)s are moved into this file
> > > > > > > > and should be make static to hide from user lowlevel helpers
> > > > > > > > (including build_package).
> > > > > > > > That will leave only high level API available.
> > > > > > > > 
> > > > > > > > TODO for me: make sure that moved lowlevel helpers are static
> > > > > > > > 
> > > > > > > > 
> 
>

On Mon, Jan 26, 2015 at 04:34:01PM +0100, Andrew Jones wrote:
> On Mon, Jan 26, 2015 at 04:09:20PM +0100, Igor Mammedov wrote:
> > On Mon, 26 Jan 2015 10:57:21 +0100
> > Igor Mammedov <imammedo@redhat.com> wrote:
> > 
> > > On Sat, 24 Jan 2015 18:33:50 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Fri, Jan 23, 2015 at 06:56:20PM +0100, Igor Mammedov wrote:
> > > > > On Fri, 23 Jan 2015 15:55:11 +0200
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > 
> > [...]
> > > > > I refuse to give up on cleaner and simpler API yet :)
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > Your patches are almost there, they are pretty clean, the only issue I
> > > > > > think is this passing of AcpiAml by value, sometimes freeing buffer in
> > > > > > the process, sometimes not.
> > > > > Currently buffer is allocated by API and is always freed whenever
> > > > > it's passed to another API function.
> > > > > That's why it makes user not to care about memory mgmt.
> > > > > 
> > > > > The only limitation of it is if you store AcpiAml return value into some
> > > > > variable you are responsible to use it only once for passing to another API
> > > > > function. Reusing this variable's value (pass it to API function second time)
> > > > > would cause cause use-after-free and freeing-freed bugs.
> > > > > Like this:
> > > > > AcpiAml table = acpi_definition_block("SSDT",...);
> > > > > AcpiAml scope = acpi_scope("PCI0");
> > > > > aml_append(&table, scope); // <- here scope becomes invalid
> > > > > // a bug
> > > > > aml_append(&table, scope); // use-after-free + freeing-freed bugs
> > > > > 
> > > > > There are several approaches to look for resolving above issues:
> > > > > 1. Adopt and use memory mgmt model used by GTK+
> > > > >    in nutshell: http://www.cs.hunter.cuny.edu/~sweiss/course_materials/csci493.70/lecture_notes/GTK_memory_mngmt.pdf
> > > > >    In particular adopt behavior of GInitiallyUnowned usage model
> > > > > 
> > > > >    that will allow to keep convenient chained call style and if necessary
> > > > >    reuse objects returned by API by explicitly referencing/dereferencing
> > > > >    them if needed.
> > > > 
> > > > Hmm, it's still easy to misuse. I think I prefer option 2 below.
> > > That's basically what we have/use in QOM with object_new(FOO) + object_unref()
> > > I have no idea why we invented our own Object infrastructure
> > > when we could just use GObject one from already used glib.
> > > 
> > > > 
> > > > > 2. It's possible to drop freeing inside API completely and
> > > > >    record(store in list) every new object inside a table context.
> > > > >    When table is constructed, list of created objects could be
> > > > >    safely freed.
> > > > >    With that it would be safe to reuse every AcpiAml object
> > > > >    and avoid free-after-use issues with limitation that created
> > > > >    AcpiAml objects shouldn't be used after table was closed.
> > > > >    It should cover all practical use of API, i.e. no cross
> > > > >    table AcpiAml objects.
> > > > 
> > > > So each aml_alloc function gets pointer to this list,
> > > > and adds the new element there.
> > > > Eventually we do free_all to free all elements,
> > > > so there isn't even an aml_free to mis-use.
> > > I'm thinking a little bit different about implementation though.
> > > I still don't like the use of explicit alloc/free being called
> > > by API user since it doesn't allow chained API calls and
> > > I think it's unnecessary complication see below why.
> > > 
> > > Here is what's true about current API and a I'd like to with it:
> > > 
> > >   1. Every API call (except aml_append) makes aml_alloc(), it's just
> > >      like a wrapper about object_new(FOO). (current + new impl.)
> > > 
> > >   2 Every API call that takes AML type as input argument
> > >   2.1 consumes (frees) it (current impl.)
> > >       (it's easy to fix use after free concern too,
> > >        just pass AML by pointer and zero-out memory before it's freed
> > >        and assert whenever one of input arguments is not correct,
> > >        i.e. it was reused second time)
> > >       There is no need for following steps after this one.
> > >   2.2 takes ownership of GInitiallyUnowned and adds it to its list
> > >       of its children.
> > >   3. Free children when AML object is destroyed (i.e. ref count zero)
> > >      That way when toplevel table object (definition block in 42/47)
> > >      is added to ACPI blob we can unref it, which will cause
> > >      its whole children tree freed, except for AML objects where
> > >      API user explicitly took extra reference (i.e. wanted them
> > >      to reuse in another table)
> > > 
> > > I'd prefer:
> > >  *  2.1 way to address your current concern of use-after-free
> > >     as the most simplest one (no reuse is possible however)
> > > or
> > >  * follow already used by QEMU QOM/GObject pattern of
> > >    implicit alloc/free
> > > 
> > > since they allow to construct AML in a more simple/manageable way i.e.
> > >  
> > >   aml_append(method,
> > >       aml_store(aml_string("foo"), aml_local(0)))
> > >   );
> > > 
> > > v.s. explicit headache of alloc/free, which doesn't fix
> > >      use-after-free anyway and just adds more boiler plate
> > >      plus makes code har to read read
> > > 
> > >   str = aml_alloc();
> > >   aml_string(str, "foo");
> > >   loc0 = aml_alloc();
> > >   aml_local(loc0, 0);
> > >   store = aml_alloc();
> > >   aml_store(store, str, loc0);
> > >   aml_append(method, store);
> > >   aml_free(store);
> > >   aml_free(loc0);
> > >   aml_free(str);
> > 
> > Here is a compromise what I and Michael came to on a phone call:
> > 
> > Externally API usage would look like:
> > 
> > AmlAllocList *p = some_list_alloc();
> > 
> > Aml *ssdt = aml_def_block(p, "SSDT", ...);
> > Aml *dev = aml_device(p, "PCI0");
> > aml_append(dev,
> >     aml_name_def(p, "_STA", aml_int(p, 0xF /* present */))
> > );
> > aml_append(ssdt, dev);
> > 
> > aml_append(acpi_tables_blob, ssdt);
> > 
> > free_aml_alloc_list(p);
> > 
> > 
> > Each of aml_foo() will take other Aml arguments by pointer.
> > Also every aml_foo(), except of aml_append() will allocate
> > Aml struct and return pointer to it and also add this pointer
> > into AmlAllocList which is passed as first argument to each
> > aml_foo() call.
> > aml_append() becomes nondestructive function and just adds
> > child(2nd arg) to the parent context (1st arg).
> > 
> > After API user is done with building table and pushed it
> > into tables blob, he/she calls free_aml_alloc_list() to free
> > all Aml objects created during process of building the table
> > content.
> 
> Hmm, passing 'p' around somewhat muddies an otherwise clean
> interface, but the concern with aml_append silently freeing
> memory still accessible by the caller is definitely valid. I
> only wonder how things would look with Igor's option 2.2 above.
> The caller still only needs to free the final table, but it
> also becomes safe to use the same object references multiple
> times before freeing the table. Using QOM also seems reasonable
> to me, as it appears it's the accepted way to do garbage
> collection in QEMU. Is it possible to do 2.2 with QOM?

I'd rather not go there: QOM was really invented for introspection, and
for long-lived heavy-weight objects.  And to me, code using QOM is
harder to understand than simple alloc/free. It's worth it where we need
the features it offers, e.g. it has run-time checks where we previously
just did a cast. But in this case I'd rather use something simpler and
with compile-time checks.



> > 
> > > 
> > > > 
> > > > Good idea! I think this will address the issue.
> > > > 
> > > > 
> > > > > 3. talloc implementation Amit've mentioned,
> > > > >    perhaps it might work since it allows to set destructors for
> > > > >    managed pointers. With this we might get clear abort when
> > > > >    dereferencing freed pointer see talloc_set()
> > > > 
> > > > 
> > > > I think it's a separate discussion. Maybe talloc is a good
> > > > allocator to use in qemu, but using a separate allocator
> > > > just for acpi generation would be an overkill.
> > > > 
> > > > > 
> > > > > > 
> > > > > > Just pass AcpiAml* everywhere, add APIs to allocate and free it
> > > > > > together with the internal buffer.
> > > > > > This makes it trivial to see that value is not misused:
> > > > > > just check it's between alloc and free - and that there are
> > > > > > no leaks - just check we call free on each value.
> > > > > > We can write a semantic patch to catch missing free calls,
> > > > > > it's easy.
> > > > > > 
> > > > > > 
> > > > > > > > 
> > > > > > > > > As for moving to to another file, during all this series lowlevel
> > > > > > > > > build_(some_aml_related_costruct_helper)s are moved into this file
> > > > > > > > > and should be make static to hide from user lowlevel helpers
> > > > > > > > > (including build_package).
> > > > > > > > > That will leave only high level API available.
> > > > > > > > > 
> > > > > > > > > TODO for me: make sure that moved lowlevel helpers are static
> > > > > > > > > 
> > > > > > > > > 
> > 
> >

On Mon, 26 Jan 2015 18:17:55 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jan 26, 2015 at 04:34:01PM +0100, Andrew Jones wrote:
> > On Mon, Jan 26, 2015 at 04:09:20PM +0100, Igor Mammedov wrote:
> > > On Mon, 26 Jan 2015 10:57:21 +0100
> > > Igor Mammedov <imammedo@redhat.com> wrote:
> > > 
> > > > On Sat, 24 Jan 2015 18:33:50 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Fri, Jan 23, 2015 at 06:56:20PM +0100, Igor Mammedov wrote:
> > > > > > On Fri, 23 Jan 2015 15:55:11 +0200
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > 
> > > [...]
> > > > > > I refuse to give up on cleaner and simpler API yet :)
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > Your patches are almost there, they are pretty clean, the
> > > > > > > only issue I think is this passing of AcpiAml by value,
> > > > > > > sometimes freeing buffer in the process, sometimes not.
> > > > > > Currently buffer is allocated by API and is always freed
> > > > > > whenever it's passed to another API function.
> > > > > > That's why it makes user not to care about memory mgmt.
> > > > > > 
> > > > > > The only limitation of it is if you store AcpiAml return
> > > > > > value into some variable you are responsible to use it only
> > > > > > once for passing to another API function. Reusing this
> > > > > > variable's value (pass it to API function second time)
> > > > > > would cause cause use-after-free and freeing-freed bugs.
> > > > > > Like this: AcpiAml table =
> > > > > > acpi_definition_block("SSDT",...); AcpiAml scope =
> > > > > > acpi_scope("PCI0"); aml_append(&table, scope); // <- here
> > > > > > scope becomes invalid // a bug
> > > > > > aml_append(&table, scope); // use-after-free +
> > > > > > freeing-freed bugs
> > > > > > 
> > > > > > There are several approaches to look for resolving above
> > > > > > issues: 1. Adopt and use memory mgmt model used by GTK+
> > > > > >    in nutshell:
> > > > > > http://www.cs.hunter.cuny.edu/~sweiss/course_materials/csci493.70/lecture_notes/GTK_memory_mngmt.pdf
> > > > > > In particular adopt behavior of GInitiallyUnowned usage
> > > > > > model
> > > > > > 
> > > > > >    that will allow to keep convenient chained call style
> > > > > > and if necessary reuse objects returned by API by
> > > > > > explicitly referencing/dereferencing them if needed.
> > > > > 
> > > > > Hmm, it's still easy to misuse. I think I prefer option 2
> > > > > below.
> > > > That's basically what we have/use in QOM with object_new(FOO) +
> > > > object_unref() I have no idea why we invented our own Object
> > > > infrastructure when we could just use GObject one from already
> > > > used glib.
> > > > 
> > > > > 
> > > > > > 2. It's possible to drop freeing inside API completely and
> > > > > >    record(store in list) every new object inside a table
> > > > > > context. When table is constructed, list of created objects
> > > > > > could be safely freed.
> > > > > >    With that it would be safe to reuse every AcpiAml object
> > > > > >    and avoid free-after-use issues with limitation that
> > > > > > created AcpiAml objects shouldn't be used after table was
> > > > > > closed. It should cover all practical use of API, i.e. no
> > > > > > cross table AcpiAml objects.
> > > > > 
> > > > > So each aml_alloc function gets pointer to this list,
> > > > > and adds the new element there.
> > > > > Eventually we do free_all to free all elements,
> > > > > so there isn't even an aml_free to mis-use.
> > > > I'm thinking a little bit different about implementation though.
> > > > I still don't like the use of explicit alloc/free being called
> > > > by API user since it doesn't allow chained API calls and
> > > > I think it's unnecessary complication see below why.
> > > > 
> > > > Here is what's true about current API and a I'd like to with it:
> > > > 
> > > >   1. Every API call (except aml_append) makes aml_alloc(), it's
> > > > just like a wrapper about object_new(FOO). (current + new impl.)
> > > > 
> > > >   2 Every API call that takes AML type as input argument
> > > >   2.1 consumes (frees) it (current impl.)
> > > >       (it's easy to fix use after free concern too,
> > > >        just pass AML by pointer and zero-out memory before it's
> > > > freed and assert whenever one of input arguments is not correct,
> > > >        i.e. it was reused second time)
> > > >       There is no need for following steps after this one.
> > > >   2.2 takes ownership of GInitiallyUnowned and adds it to its
> > > > list of its children.
> > > >   3. Free children when AML object is destroyed (i.e. ref count
> > > > zero) That way when toplevel table object (definition block in
> > > > 42/47) is added to ACPI blob we can unref it, which will cause
> > > >      its whole children tree freed, except for AML objects where
> > > >      API user explicitly took extra reference (i.e. wanted them
> > > >      to reuse in another table)
> > > > 
> > > > I'd prefer:
> > > >  *  2.1 way to address your current concern of use-after-free
> > > >     as the most simplest one (no reuse is possible however)
> > > > or
> > > >  * follow already used by QEMU QOM/GObject pattern of
> > > >    implicit alloc/free
> > > > 
> > > > since they allow to construct AML in a more simple/manageable
> > > > way i.e. 
> > > >   aml_append(method,
> > > >       aml_store(aml_string("foo"), aml_local(0)))
> > > >   );
> > > > 
> > > > v.s. explicit headache of alloc/free, which doesn't fix
> > > >      use-after-free anyway and just adds more boiler plate
> > > >      plus makes code har to read read
> > > > 
> > > >   str = aml_alloc();
> > > >   aml_string(str, "foo");
> > > >   loc0 = aml_alloc();
> > > >   aml_local(loc0, 0);
> > > >   store = aml_alloc();
> > > >   aml_store(store, str, loc0);
> > > >   aml_append(method, store);
> > > >   aml_free(store);
> > > >   aml_free(loc0);
> > > >   aml_free(str);
> > > 
> > > Here is a compromise what I and Michael came to on a phone call:
> > > 
> > > Externally API usage would look like:
> > > 
> > > AmlAllocList *p = some_list_alloc();
> > > 
> > > Aml *ssdt = aml_def_block(p, "SSDT", ...);
> > > Aml *dev = aml_device(p, "PCI0");
> > > aml_append(dev,
> > >     aml_name_def(p, "_STA", aml_int(p, 0xF /* present */))
> > > );
> > > aml_append(ssdt, dev);
> > > 
> > > aml_append(acpi_tables_blob, ssdt);
> > > 
> > > free_aml_alloc_list(p);
> > > 
> > > 
> > > Each of aml_foo() will take other Aml arguments by pointer.
> > > Also every aml_foo(), except of aml_append() will allocate
> > > Aml struct and return pointer to it and also add this pointer
> > > into AmlAllocList which is passed as first argument to each
> > > aml_foo() call.
> > > aml_append() becomes nondestructive function and just adds
> > > child(2nd arg) to the parent context (1st arg).
> > > 
> > > After API user is done with building table and pushed it
> > > into tables blob, he/she calls free_aml_alloc_list() to free
> > > all Aml objects created during process of building the table
> > > content.
> > 
> > Hmm, passing 'p' around somewhat muddies an otherwise clean
> > interface, but the concern with aml_append silently freeing
> > memory still accessible by the caller is definitely valid. I
I've tried redo series with passing alloc list as first argument,
looks ugly as hell and I still doubt that explicit recording of every
allocation is necessary. I'd rather use QOM tree for AML objects.

If we still don't want to use QOM and considering that ACPI
tables are build in one thread/function, I'd prefer to hide
allocation list inside API making it static variable for now. With
external  init/free_all API to allow explicitly do this operations
before/after table is build. It would still provide explicit
alloc/free but would keep AML API clean/simple since we won't have to
pass alloc list to every API function which are called quite a lot
times. It also would allow transparently for users switch from this
allocation scheme to QOM one when such need arises.

> > only wonder how things would look with Igor's option 2.2 above.
> > The caller still only needs to free the final table, but it
> > also becomes safe to use the same object references multiple
> > times before freeing the table. Using QOM also seems reasonable
> > to me, as it appears it's the accepted way to do garbage
> > collection in QEMU. Is it possible to do 2.2 with QOM?
> 
> I'd rather not go there: QOM was really invented for introspection,
> and for long-lived heavy-weight objects.  And to me, code using QOM is
> harder to understand than simple alloc/free. It's worth it where we
> need the features it offers, e.g. it has run-time checks where we
> previously just did a cast. But in this case I'd rather use something
> simpler and with compile-time checks.

I'll post example on top of v2 that does it QOM way as Drew suggested
for discussion. Resut looks relatively simple and keeps API clean,
it replaces alloc/free with widely used object_new/object_unref
and allows to free table tree including all its children in one call
object_unref(table).
It also allows to extend generic AML object to types like AML tables
blob (there is patch in example) or AML table (haven't tried yet but
see need for it) and automatically enforces type checks whenever we do
casts, and we have to do casts to access type specific fields
internally. At the same time it keeps API simple and uniform (single
typed Aml* as parameters/return values) from user POV.
Making type for every AML object probably would be overkill right now
but we could do it for generic AML object, AML table and AML tables
blob objects, that helps to generalize table building functions that
has been just copied from x86 to ARM series in latest ACPI for ARM
series. Later if there would be need it would be possible to extend
AML types internally without touching/breaking API users.

> 
> 
> 
> > > 
> > > > 
> > > > > 
> > > > > Good idea! I think this will address the issue.
> > > > > 
> > > > > 
> > > > > > 3. talloc implementation Amit've mentioned,
> > > > > >    perhaps it might work since it allows to set destructors
> > > > > > for managed pointers. With this we might get clear abort
> > > > > > when dereferencing freed pointer see talloc_set()
> > > > > 
> > > > > 
> > > > > I think it's a separate discussion. Maybe talloc is a good
> > > > > allocator to use in qemu, but using a separate allocator
> > > > > just for acpi generation would be an overkill.
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > Just pass AcpiAml* everywhere, add APIs to allocate and
> > > > > > > free it together with the internal buffer.
> > > > > > > This makes it trivial to see that value is not misused:
> > > > > > > just check it's between alloc and free - and that there
> > > > > > > are no leaks - just check we call free on each value.
> > > > > > > We can write a semantic patch to catch missing free calls,
> > > > > > > it's easy.
> > > > > > > 
> > > > > > > 
> > > > > > > > > 
> > > > > > > > > > As for moving to to another file, during all this
> > > > > > > > > > series lowlevel
> > > > > > > > > > build_(some_aml_related_costruct_helper)s are moved
> > > > > > > > > > into this file and should be make static to hide
> > > > > > > > > > from user lowlevel helpers (including
> > > > > > > > > > build_package). That will leave only high level API
> > > > > > > > > > available.
> > > > > > > > > > 
> > > > > > > > > > TODO for me: make sure that moved lowlevel helpers
> > > > > > > > > > are static
> > > > > > > > > > 
> > > > > > > > > > 
> > > 
> > >

On Tue, Jan 27, 2015 at 11:29:09PM +0100, Igor Mammedov wrote:
> On Mon, 26 Jan 2015 18:17:55 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Jan 26, 2015 at 04:34:01PM +0100, Andrew Jones wrote:
> > > On Mon, Jan 26, 2015 at 04:09:20PM +0100, Igor Mammedov wrote:
> > > > On Mon, 26 Jan 2015 10:57:21 +0100
> > > > Igor Mammedov <imammedo@redhat.com> wrote:
> > > > 
> > > > > On Sat, 24 Jan 2015 18:33:50 +0200
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > 
> > > > > > On Fri, Jan 23, 2015 at 06:56:20PM +0100, Igor Mammedov wrote:
> > > > > > > On Fri, 23 Jan 2015 15:55:11 +0200
> > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > 
> > > > [...]
> > > > > > > I refuse to give up on cleaner and simpler API yet :)
> > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Your patches are almost there, they are pretty clean, the
> > > > > > > > only issue I think is this passing of AcpiAml by value,
> > > > > > > > sometimes freeing buffer in the process, sometimes not.
> > > > > > > Currently buffer is allocated by API and is always freed
> > > > > > > whenever it's passed to another API function.
> > > > > > > That's why it makes user not to care about memory mgmt.
> > > > > > > 
> > > > > > > The only limitation of it is if you store AcpiAml return
> > > > > > > value into some variable you are responsible to use it only
> > > > > > > once for passing to another API function. Reusing this
> > > > > > > variable's value (pass it to API function second time)
> > > > > > > would cause cause use-after-free and freeing-freed bugs.
> > > > > > > Like this: AcpiAml table =
> > > > > > > acpi_definition_block("SSDT",...); AcpiAml scope =
> > > > > > > acpi_scope("PCI0"); aml_append(&table, scope); // <- here
> > > > > > > scope becomes invalid // a bug
> > > > > > > aml_append(&table, scope); // use-after-free +
> > > > > > > freeing-freed bugs
> > > > > > > 
> > > > > > > There are several approaches to look for resolving above
> > > > > > > issues: 1. Adopt and use memory mgmt model used by GTK+
> > > > > > >    in nutshell:
> > > > > > > http://www.cs.hunter.cuny.edu/~sweiss/course_materials/csci493.70/lecture_notes/GTK_memory_mngmt.pdf
> > > > > > > In particular adopt behavior of GInitiallyUnowned usage
> > > > > > > model
> > > > > > > 
> > > > > > >    that will allow to keep convenient chained call style
> > > > > > > and if necessary reuse objects returned by API by
> > > > > > > explicitly referencing/dereferencing them if needed.
> > > > > > 
> > > > > > Hmm, it's still easy to misuse. I think I prefer option 2
> > > > > > below.
> > > > > That's basically what we have/use in QOM with object_new(FOO) +
> > > > > object_unref() I have no idea why we invented our own Object
> > > > > infrastructure when we could just use GObject one from already
> > > > > used glib.
> > > > > 
> > > > > > 
> > > > > > > 2. It's possible to drop freeing inside API completely and
> > > > > > >    record(store in list) every new object inside a table
> > > > > > > context. When table is constructed, list of created objects
> > > > > > > could be safely freed.
> > > > > > >    With that it would be safe to reuse every AcpiAml object
> > > > > > >    and avoid free-after-use issues with limitation that
> > > > > > > created AcpiAml objects shouldn't be used after table was
> > > > > > > closed. It should cover all practical use of API, i.e. no
> > > > > > > cross table AcpiAml objects.
> > > > > > 
> > > > > > So each aml_alloc function gets pointer to this list,
> > > > > > and adds the new element there.
> > > > > > Eventually we do free_all to free all elements,
> > > > > > so there isn't even an aml_free to mis-use.
> > > > > I'm thinking a little bit different about implementation though.
> > > > > I still don't like the use of explicit alloc/free being called
> > > > > by API user since it doesn't allow chained API calls and
> > > > > I think it's unnecessary complication see below why.
> > > > > 
> > > > > Here is what's true about current API and a I'd like to with it:
> > > > > 
> > > > >   1. Every API call (except aml_append) makes aml_alloc(), it's
> > > > > just like a wrapper about object_new(FOO). (current + new impl.)
> > > > > 
> > > > >   2 Every API call that takes AML type as input argument
> > > > >   2.1 consumes (frees) it (current impl.)
> > > > >       (it's easy to fix use after free concern too,
> > > > >        just pass AML by pointer and zero-out memory before it's
> > > > > freed and assert whenever one of input arguments is not correct,
> > > > >        i.e. it was reused second time)
> > > > >       There is no need for following steps after this one.
> > > > >   2.2 takes ownership of GInitiallyUnowned and adds it to its
> > > > > list of its children.
> > > > >   3. Free children when AML object is destroyed (i.e. ref count
> > > > > zero) That way when toplevel table object (definition block in
> > > > > 42/47) is added to ACPI blob we can unref it, which will cause
> > > > >      its whole children tree freed, except for AML objects where
> > > > >      API user explicitly took extra reference (i.e. wanted them
> > > > >      to reuse in another table)
> > > > > 
> > > > > I'd prefer:
> > > > >  *  2.1 way to address your current concern of use-after-free
> > > > >     as the most simplest one (no reuse is possible however)
> > > > > or
> > > > >  * follow already used by QEMU QOM/GObject pattern of
> > > > >    implicit alloc/free
> > > > > 
> > > > > since they allow to construct AML in a more simple/manageable
> > > > > way i.e. 
> > > > >   aml_append(method,
> > > > >       aml_store(aml_string("foo"), aml_local(0)))
> > > > >   );
> > > > > 
> > > > > v.s. explicit headache of alloc/free, which doesn't fix
> > > > >      use-after-free anyway and just adds more boiler plate
> > > > >      plus makes code har to read read
> > > > > 
> > > > >   str = aml_alloc();
> > > > >   aml_string(str, "foo");
> > > > >   loc0 = aml_alloc();
> > > > >   aml_local(loc0, 0);
> > > > >   store = aml_alloc();
> > > > >   aml_store(store, str, loc0);
> > > > >   aml_append(method, store);
> > > > >   aml_free(store);
> > > > >   aml_free(loc0);
> > > > >   aml_free(str);
> > > > 
> > > > Here is a compromise what I and Michael came to on a phone call:
> > > > 
> > > > Externally API usage would look like:
> > > > 
> > > > AmlAllocList *p = some_list_alloc();
> > > > 
> > > > Aml *ssdt = aml_def_block(p, "SSDT", ...);
> > > > Aml *dev = aml_device(p, "PCI0");
> > > > aml_append(dev,
> > > >     aml_name_def(p, "_STA", aml_int(p, 0xF /* present */))
> > > > );
> > > > aml_append(ssdt, dev);
> > > > 
> > > > aml_append(acpi_tables_blob, ssdt);
> > > > 
> > > > free_aml_alloc_list(p);
> > > > 
> > > > 
> > > > Each of aml_foo() will take other Aml arguments by pointer.
> > > > Also every aml_foo(), except of aml_append() will allocate
> > > > Aml struct and return pointer to it and also add this pointer
> > > > into AmlAllocList which is passed as first argument to each
> > > > aml_foo() call.
> > > > aml_append() becomes nondestructive function and just adds
> > > > child(2nd arg) to the parent context (1st arg).
> > > > 
> > > > After API user is done with building table and pushed it
> > > > into tables blob, he/she calls free_aml_alloc_list() to free
> > > > all Aml objects created during process of building the table
> > > > content.
> > > 
> > > Hmm, passing 'p' around somewhat muddies an otherwise clean
> > > interface, but the concern with aml_append silently freeing
> > > memory still accessible by the caller is definitely valid. I
> I've tried redo series with passing alloc list as first argument,
> looks ugly as hell

Just make names shorter, something like "p" will do the trick.
It's hard to argue about subjective ugly/not ugly.

Have you seen Rusty's design manifesto?
You current code is at best at level 2:
read the implementation and you will get it right.
Moving to QOM gets you to level 4:
follow common convention and you'll get it right.
But alloc/free is at level 7:
the obvious use is (probably) the correct one.

> and I still doubt that explicit recording of every
> allocation is necessary. I'd rather use QOM tree for AML objects.
> If we still don't want to use QOM and considering that ACPI
> tables are build in one thread/function, I'd prefer to hide
> allocation list inside API making it static variable for now. With
> external  init/free_all API to allow explicitly do this operations
> before/after table is build. It would still provide explicit
> alloc/free but would keep AML API clean/simple since we won't have to
> pass alloc list to every API function which are called quite a lot
> times. It also would allow transparently for users switch from this
> allocation scheme to QOM one when such need arises.
>
> > > only wonder how things would look with Igor's option 2.2 above.
> > > The caller still only needs to free the final table, but it
> > > also becomes safe to use the same object references multiple
> > > times before freeing the table. Using QOM also seems reasonable
> > > to me, as it appears it's the accepted way to do garbage
> > > collection in QEMU. Is it possible to do 2.2 with QOM?
> > 
> > I'd rather not go there: QOM was really invented for introspection,
> > and for long-lived heavy-weight objects.  And to me, code using QOM is
> > harder to understand than simple alloc/free. It's worth it where we
> > need the features it offers, e.g. it has run-time checks where we
> > previously just did a cast. But in this case I'd rather use something
> > simpler and with compile-time checks.
> 
> I'll post example on top of v2 that does it QOM way as Drew suggested
> for discussion. Resut looks relatively simple and keeps API clean,
> it replaces alloc/free with widely used object_new/object_unref
> and allows to free table tree including all its children in one call
> object_unref(table).

Having just tried to fix QOM induced memory leaks, I'm not impressed.
Basically, the rule that people seem to agree on is that
it's always a mistake to create QOM objects in response
to guest activity, and this is what we are doing here, after all.

> It also allows to extend generic AML object to types like AML tables
> blob (there is patch in example) or AML table (haven't tried yet but
> see need for it) and automatically enforces type checks whenever we do
> casts, and we have to do casts to access type specific fields
> internally. At the same time it keeps API simple and uniform (single
> typed Aml* as parameters/return values) from user POV.
> Making type for every AML object probably would be overkill right now
> but we could do it for generic AML object, AML table and AML tables
> blob objects, that helps to generalize table building functions that
> has been just copied from x86 to ARM series in latest ACPI for ARM
> series. Later if there would be need it would be possible to extend
> AML types internally without touching/breaking API users.
> > 
> > 
> > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > Good idea! I think this will address the issue.
> > > > > > 
> > > > > > 
> > > > > > > 3. talloc implementation Amit've mentioned,
> > > > > > >    perhaps it might work since it allows to set destructors
> > > > > > > for managed pointers. With this we might get clear abort
> > > > > > > when dereferencing freed pointer see talloc_set()
> > > > > > 
> > > > > > 
> > > > > > I think it's a separate discussion. Maybe talloc is a good
> > > > > > allocator to use in qemu, but using a separate allocator
> > > > > > just for acpi generation would be an overkill.
> > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > Just pass AcpiAml* everywhere, add APIs to allocate and
> > > > > > > > free it together with the internal buffer.
> > > > > > > > This makes it trivial to see that value is not misused:
> > > > > > > > just check it's between alloc and free - and that there
> > > > > > > > are no leaks - just check we call free on each value.
> > > > > > > > We can write a semantic patch to catch missing free calls,
> > > > > > > > it's easy.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > > As for moving to to another file, during all this
> > > > > > > > > > > series lowlevel
> > > > > > > > > > > build_(some_aml_related_costruct_helper)s are moved
> > > > > > > > > > > into this file and should be make static to hide
> > > > > > > > > > > from user lowlevel helpers (including
> > > > > > > > > > > build_package). That will leave only high level API
> > > > > > > > > > > available.
> > > > > > > > > > > 
> > > > > > > > > > > TODO for me: make sure that moved lowlevel helpers
> > > > > > > > > > > are static
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > 
> > > >

Example demonstrates use of QOM object for building AML
objects tree and freeing it explicitly when requested.

Top level ACPI tables blob object is only partially
switched to object model now but I hope it demostrates
the point of absracting code and hiding parts of code
that could be done without user intervention.

Pathes 1/2/8 are just a convertion boiler plate which
will go away on respin.


Igor Mammedov (13):
  convert to passing AcpiAml by pointers
  make toplevel ACPI tables blob a pointer
  qom: add support for weak referenced object: aka UnownedObject
  acpi: make AcpiAml an OQM object
  acpi: use TYPE_AML_OBJECT inside of AML API
  acpi: use TYPE_AML_OBJECT for toplevel ACPI tables blob
  acpi: make toplevel ACPI tables blob a dedicated object
  i386: acpi: hack not yet converted tables calls to deal with
    table_data being a pointer
  acpi: add aml_blob() helper
  i386: acpi: add DSDT table using AML API
  acpi: acpi_add_table() to common cross target file
  acpi: prepare for API internal collection of RSDT entries
  i386: acpi: mark SSDT as RSDT entry so API would add entry to RSDT
    automatically

 hw/acpi/acpi-build-utils.c         | 537 +++++++++++++++++++++----------------
 hw/i386/acpi-build.c               | 388 +++++++++++++--------------
 include/hw/acpi/acpi-build-utils.h | 119 +++++---
 include/qom/object.h               |  20 ++
 qom/object.c                       |  20 +-
 5 files changed, 620 insertions(+), 464 deletions(-)

On Wed, Jan 28, 2015 at 10:03:24AM +0000, Igor Mammedov wrote:
> 
> Example demonstrates use of QOM object for building AML
> objects tree and freeing it explicitly when requested.
> 
> Top level ACPI tables blob object is only partially
> switched to object model now but I hope it demostrates
> the point of absracting code and hiding parts of code
> that could be done without user intervention.
> 
> Pathes 1/2/8 are just a convertion boiler plate which
> will go away on respin.
> 
> 
> Igor Mammedov (13):
>   convert to passing AcpiAml by pointers
>   make toplevel ACPI tables blob a pointer
>   qom: add support for weak referenced object: aka UnownedObject
>   acpi: make AcpiAml an OQM object
>   acpi: use TYPE_AML_OBJECT inside of AML API
>   acpi: use TYPE_AML_OBJECT for toplevel ACPI tables blob
>   acpi: make toplevel ACPI tables blob a dedicated object
>   i386: acpi: hack not yet converted tables calls to deal with
>     table_data being a pointer
>   acpi: add aml_blob() helper
>   i386: acpi: add DSDT table using AML API
>   acpi: acpi_add_table() to common cross target file
>   acpi: prepare for API internal collection of RSDT entries
>   i386: acpi: mark SSDT as RSDT entry so API would add entry to RSDT
>     automatically
> 
>  hw/acpi/acpi-build-utils.c         | 537 +++++++++++++++++++++----------------
>  hw/i386/acpi-build.c               | 388 +++++++++++++--------------
>  include/hw/acpi/acpi-build-utils.h | 119 +++++---
>  include/qom/object.h               |  20 ++
>  qom/object.c                       |  20 +-
>  5 files changed, 620 insertions(+), 464 deletions(-)
> 
> -- 
> 1.8.3.1
>

Thanks for doing this. It's satisfied my curiosity as to how
much boilerplate we'd need for the conversion. I looked mainly
at the first half of this series, as it appears the second half
is an add-on, not directly relevant to the explicit memory
management vs. object model memory management discussion. The
additional code, and need for QOM knowledge, appears pretty low.
So, as this would allow developers focused on ACPI table
construction to almost completely avoid doing any memory
management themselves, then, FWIW, it looks like a good trade
off to me.

drew

On 01/28/2015 12:29 AM, Igor Mammedov wrote:
> On Mon, 26 Jan 2015 18:17:55 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Mon, Jan 26, 2015 at 04:34:01PM +0100, Andrew Jones wrote:
>>> On Mon, Jan 26, 2015 at 04:09:20PM +0100, Igor Mammedov wrote:
>>>> On Mon, 26 Jan 2015 10:57:21 +0100
>>>> Igor Mammedov <imammedo@redhat.com> wrote:
>>>>
>>>>> On Sat, 24 Jan 2015 18:33:50 +0200
>>>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>>>
>>>>>> On Fri, Jan 23, 2015 at 06:56:20PM +0100, Igor Mammedov wrote:
>>>>>>> On Fri, 23 Jan 2015 15:55:11 +0200
>>>>>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>>>>>
>>>> [...]
>>>>>>> I refuse to give up on cleaner and simpler API yet :)
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Your patches are almost there, they are pretty clean, the
>>>>>>>> only issue I think is this passing of AcpiAml by value,
>>>>>>>> sometimes freeing buffer in the process, sometimes not.
>>>>>>> Currently buffer is allocated by API and is always freed
>>>>>>> whenever it's passed to another API function.
>>>>>>> That's why it makes user not to care about memory mgmt.
>>>>>>>
>>>>>>> The only limitation of it is if you store AcpiAml return
>>>>>>> value into some variable you are responsible to use it only
>>>>>>> once for passing to another API function. Reusing this
>>>>>>> variable's value (pass it to API function second time)
>>>>>>> would cause cause use-after-free and freeing-freed bugs.
>>>>>>> Like this: AcpiAml table =
>>>>>>> acpi_definition_block("SSDT",...); AcpiAml scope =
>>>>>>> acpi_scope("PCI0"); aml_append(&table, scope); // <- here
>>>>>>> scope becomes invalid // a bug
>>>>>>> aml_append(&table, scope); // use-after-free +
>>>>>>> freeing-freed bugs
>>>>>>>
>>>>>>> There are several approaches to look for resolving above
>>>>>>> issues: 1. Adopt and use memory mgmt model used by GTK+
>>>>>>>     in nutshell:
>>>>>>> http://www.cs.hunter.cuny.edu/~sweiss/course_materials/csci493.70/lecture_notes/GTK_memory_mngmt.pdf
>>>>>>> In particular adopt behavior of GInitiallyUnowned usage
>>>>>>> model
>>>>>>>
>>>>>>>     that will allow to keep convenient chained call style
>>>>>>> and if necessary reuse objects returned by API by
>>>>>>> explicitly referencing/dereferencing them if needed.
>>>>>>
>>>>>> Hmm, it's still easy to misuse. I think I prefer option 2
>>>>>> below.
>>>>> That's basically what we have/use in QOM with object_new(FOO) +
>>>>> object_unref() I have no idea why we invented our own Object
>>>>> infrastructure when we could just use GObject one from already
>>>>> used glib.
>>>>>
>>>>>>
>>>>>>> 2. It's possible to drop freeing inside API completely and
>>>>>>>     record(store in list) every new object inside a table
>>>>>>> context. When table is constructed, list of created objects
>>>>>>> could be safely freed.
>>>>>>>     With that it would be safe to reuse every AcpiAml object
>>>>>>>     and avoid free-after-use issues with limitation that
>>>>>>> created AcpiAml objects shouldn't be used after table was
>>>>>>> closed. It should cover all practical use of API, i.e. no
>>>>>>> cross table AcpiAml objects.
>>>>>>
>>>>>> So each aml_alloc function gets pointer to this list,
>>>>>> and adds the new element there.
>>>>>> Eventually we do free_all to free all elements,
>>>>>> so there isn't even an aml_free to mis-use.
>>>>> I'm thinking a little bit different about implementation though.
>>>>> I still don't like the use of explicit alloc/free being called
>>>>> by API user since it doesn't allow chained API calls and
>>>>> I think it's unnecessary complication see below why.
>>>>>
>>>>> Here is what's true about current API and a I'd like to with it:
>>>>>
>>>>>    1. Every API call (except aml_append) makes aml_alloc(), it's
>>>>> just like a wrapper about object_new(FOO). (current + new impl.)
>>>>>
>>>>>    2 Every API call that takes AML type as input argument
>>>>>    2.1 consumes (frees) it (current impl.)
>>>>>        (it's easy to fix use after free concern too,
>>>>>         just pass AML by pointer and zero-out memory before it's
>>>>> freed and assert whenever one of input arguments is not correct,
>>>>>         i.e. it was reused second time)
>>>>>        There is no need for following steps after this one.
>>>>>    2.2 takes ownership of GInitiallyUnowned and adds it to its
>>>>> list of its children.
>>>>>    3. Free children when AML object is destroyed (i.e. ref count
>>>>> zero) That way when toplevel table object (definition block in
>>>>> 42/47) is added to ACPI blob we can unref it, which will cause
>>>>>       its whole children tree freed, except for AML objects where
>>>>>       API user explicitly took extra reference (i.e. wanted them
>>>>>       to reuse in another table)
>>>>>
>>>>> I'd prefer:
>>>>>   *  2.1 way to address your current concern of use-after-free
>>>>>      as the most simplest one (no reuse is possible however)
>>>>> or
>>>>>   * follow already used by QEMU QOM/GObject pattern of
>>>>>     implicit alloc/free
>>>>>
>>>>> since they allow to construct AML in a more simple/manageable
>>>>> way i.e.
>>>>>    aml_append(method,
>>>>>        aml_store(aml_string("foo"), aml_local(0)))
>>>>>    );
>>>>>
>>>>> v.s. explicit headache of alloc/free, which doesn't fix
>>>>>       use-after-free anyway and just adds more boiler plate
>>>>>       plus makes code har to read read
>>>>>
>>>>>    str = aml_alloc();
>>>>>    aml_string(str, "foo");
>>>>>    loc0 = aml_alloc();
>>>>>    aml_local(loc0, 0);
>>>>>    store = aml_alloc();
>>>>>    aml_store(store, str, loc0);
>>>>>    aml_append(method, store);
>>>>>    aml_free(store);
>>>>>    aml_free(loc0);
>>>>>    aml_free(str);
>>>>
>>>> Here is a compromise what I and Michael came to on a phone call:
>>>>
>>>> Externally API usage would look like:
>>>>
>>>> AmlAllocList *p = some_list_alloc();
>>>>
>>>> Aml *ssdt = aml_def_block(p, "SSDT", ...);
>>>> Aml *dev = aml_device(p, "PCI0");
>>>> aml_append(dev,
>>>>      aml_name_def(p, "_STA", aml_int(p, 0xF /* present */))
>>>> );
>>>> aml_append(ssdt, dev);
>>>>
>>>> aml_append(acpi_tables_blob, ssdt);
>>>>
>>>> free_aml_alloc_list(p);
>>>>
>>>>
>>>> Each of aml_foo() will take other Aml arguments by pointer.
>>>> Also every aml_foo(), except of aml_append() will allocate
>>>> Aml struct and return pointer to it and also add this pointer
>>>> into AmlAllocList which is passed as first argument to each
>>>> aml_foo() call.
>>>> aml_append() becomes nondestructive function and just adds
>>>> child(2nd arg) to the parent context (1st arg).
>>>>
>>>> After API user is done with building table and pushed it
>>>> into tables blob, he/she calls free_aml_alloc_list() to free
>>>> all Aml objects created during process of building the table
>>>> content.
>>>
>>> Hmm, passing 'p' around somewhat muddies an otherwise clean
>>> interface, but the concern with aml_append silently freeing
>>> memory still accessible by the caller is definitely valid. I
> I've tried redo series with passing alloc list as first argument,
> looks ugly as hell
I don't like that either.

  and I still doubt that explicit recording of every
> allocation is necessary.
This would be nice since is simple, it doesn't hurt
and it releases us from the use-after-free problem.


  I'd rather use QOM tree for AML objects.
If you must :(, it looks like an overkill since those objects will live
a very short life and your proposed table does the trick.
Keeping references and a whole tree seems unnecessary.

>
> If we still don't want to use QOM and considering that ACPI
> tables are build in one thread/function, I'd prefer to hide
> allocation list inside API making it static variable for now.
Why not, would be an internal static variable acceptable?
Do we have to worry of multiple threads for this?
If not, I would suggest going with that and close the day.

My opinion, of course.
Thanks,
Marcel

[...]

On 01/28/2015 02:44 PM, Andrew Jones wrote:
> On Wed, Jan 28, 2015 at 10:03:24AM +0000, Igor Mammedov wrote:
>>
>> Example demonstrates use of QOM object for building AML
>> objects tree and freeing it explicitly when requested.
>>
>> Top level ACPI tables blob object is only partially
>> switched to object model now but I hope it demostrates
>> the point of absracting code and hiding parts of code
>> that could be done without user intervention.
>>
>> Pathes 1/2/8 are just a convertion boiler plate which
>> will go away on respin.
>>
>>
>> Igor Mammedov (13):
>>    convert to passing AcpiAml by pointers
>>    make toplevel ACPI tables blob a pointer
>>    qom: add support for weak referenced object: aka UnownedObject
>>    acpi: make AcpiAml an OQM object
>>    acpi: use TYPE_AML_OBJECT inside of AML API
>>    acpi: use TYPE_AML_OBJECT for toplevel ACPI tables blob
>>    acpi: make toplevel ACPI tables blob a dedicated object
>>    i386: acpi: hack not yet converted tables calls to deal with
>>      table_data being a pointer
>>    acpi: add aml_blob() helper
>>    i386: acpi: add DSDT table using AML API
>>    acpi: acpi_add_table() to common cross target file
>>    acpi: prepare for API internal collection of RSDT entries
>>    i386: acpi: mark SSDT as RSDT entry so API would add entry to RSDT
>>      automatically
>>
>>   hw/acpi/acpi-build-utils.c         | 537 +++++++++++++++++++++----------------
>>   hw/i386/acpi-build.c               | 388 +++++++++++++--------------
>>   include/hw/acpi/acpi-build-utils.h | 119 +++++---
>>   include/qom/object.h               |  20 ++
>>   qom/object.c                       |  20 +-
>>   5 files changed, 620 insertions(+), 464 deletions(-)
>>
>> --
>> 1.8.3.1
>>
>
> Thanks for doing this.
Thanks also fomr me! It is really  interesting to see this.

  It's satisfied my curiosity as to how
> much boilerplate we'd need for the conversion. I looked mainly
> at the first half of this series, as it appears the second half
> is an add-on, not directly relevant to the explicit memory
> management vs. object model memory management discussion. The
> additional code, and need for QOM knowledge, appears pretty low.
> So, as this would allow developers focused on ACPI table
> construction to almost completely avoid doing any memory
> management themselves, then, FWIW, it looks like a good trade
> off to me.

While I am not against this approach, maybe a simpler idea of keeping
an internal static map of allocated vars and freeing it in the end would be enough.

That being said, this approach gives us the opportunity to create QOM objects
for all AML constructs as AcpiArg for example:
     AcpiAml arg = object_new(ACPI_ARG_TYPE);
and handling the implementation inside the corresponding init function.

Thanks,
Marcel

>
> drew
>

On Thu, 05 Feb 2015 16:28:03 +0200
Marcel Apfelbaum <marcel@redhat.com> wrote:

> On 01/28/2015 02:44 PM, Andrew Jones wrote:
> > On Wed, Jan 28, 2015 at 10:03:24AM +0000, Igor Mammedov wrote:
> >>
> >> Example demonstrates use of QOM object for building AML
> >> objects tree and freeing it explicitly when requested.
> >>
> >> Top level ACPI tables blob object is only partially
> >> switched to object model now but I hope it demostrates
> >> the point of absracting code and hiding parts of code
> >> that could be done without user intervention.
> >>
> >> Pathes 1/2/8 are just a convertion boiler plate which
> >> will go away on respin.
> >>
> >>
> >> Igor Mammedov (13):
> >>    convert to passing AcpiAml by pointers
> >>    make toplevel ACPI tables blob a pointer
> >>    qom: add support for weak referenced object: aka UnownedObject
> >>    acpi: make AcpiAml an OQM object
> >>    acpi: use TYPE_AML_OBJECT inside of AML API
> >>    acpi: use TYPE_AML_OBJECT for toplevel ACPI tables blob
> >>    acpi: make toplevel ACPI tables blob a dedicated object
> >>    i386: acpi: hack not yet converted tables calls to deal with
> >>      table_data being a pointer
> >>    acpi: add aml_blob() helper
> >>    i386: acpi: add DSDT table using AML API
> >>    acpi: acpi_add_table() to common cross target file
> >>    acpi: prepare for API internal collection of RSDT entries
> >>    i386: acpi: mark SSDT as RSDT entry so API would add entry to RSDT
> >>      automatically
> >>
> >>   hw/acpi/acpi-build-utils.c         | 537 +++++++++++++++++++++----------------
> >>   hw/i386/acpi-build.c               | 388 +++++++++++++--------------
> >>   include/hw/acpi/acpi-build-utils.h | 119 +++++---
> >>   include/qom/object.h               |  20 ++
> >>   qom/object.c                       |  20 +-
> >>   5 files changed, 620 insertions(+), 464 deletions(-)
> >>
> >> --
> >> 1.8.3.1
> >>
> >
> > Thanks for doing this.
> Thanks also fomr me! It is really  interesting to see this.
> 
>   It's satisfied my curiosity as to how
> > much boilerplate we'd need for the conversion. I looked mainly
> > at the first half of this series, as it appears the second half
> > is an add-on, not directly relevant to the explicit memory
> > management vs. object model memory management discussion. The
> > additional code, and need for QOM knowledge, appears pretty low.
> > So, as this would allow developers focused on ACPI table
> > construction to almost completely avoid doing any memory
> > management themselves, then, FWIW, it looks like a good trade
> > off to me.
> 
> While I am not against this approach, maybe a simpler idea of keeping
> an internal static map of allocated vars and freeing it in the end would be enough.
> 
> That being said, this approach gives us the opportunity to create QOM objects
> for all AML constructs as AcpiArg for example:
>      AcpiAml arg = object_new(ACPI_ARG_TYPE);
> and handling the implementation inside the corresponding init function.
That was my idea bind using QOM,
later we could slowly split different AML constructs into different types
and e.t.c., which helps to abstract code and override with inherited
target specific types.

But we could switch to QOM later when we have to without affecting users,
since external API will stay the same.

> 
> Thanks,
> Marcel
> 
> >
> > drew
> >
> 
>