Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()

[Igor Mammedov <imammedo@redhat.com>]

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
>