qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Igor Mammedov
Subject: Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()
Date: Mon, 26 Jan 2015 10:57:21 +0100

On Sat, 24 Jan 2015 18:33:50 +0200
"Michael S. Tsirkin" <address@hidden> 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" <address@hidden> 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" <address@hidden> 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" <address@hidden> 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 <address@hidden>
> > > > > > > > ---
> > > > > > > >  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
> > > 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]