subject:"\[Qemu\-devel\] \[PATCH v2 01\/47\] acpi\: introduce AML composer aml

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

2015-02-05 Thread Marcel Apfelbaum


On 01/28/2015 09:56 AM, Michael S. Tsirkin wrote:

I've tried redo series with passing alloc list as first argument,
looks ugly as hell


I tried too. Not too bad at all. See below:

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f66da5d..820504a 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -491,14 +491,14 @@ static void acpi_set_pci_info(void)
  }
  }

-static void build_append_pcihp_notify_entry(AcpiAml *method, int slot)
+static void build_append_pcihp_notify_entry(AmlPool *p, AcpiAml *method, int 
slot)
  {
-AcpiAml if_ctx;
+AcpiAml *if_ctx;
  int32_t devfn = PCI_DEVFN(slot, 0);

-if_ctx = acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U  slot)));
-aml_append(if_ctx, acpi_notify(acpi_name(S%.02X, devfn), acpi_arg1()));
-aml_append(method, if_ctx);
+if_ctx = acpi_if(p, acpi_and(p, acpi_arg0(), acpi_int(p, 0x1U  slot)));
+aml_append(p, if_ctx, acpi_notify(p, acpi_name(p, S%.02X, devfn), 
acpi_arg1(p)));
+aml_append(p, method, if_ctx);
  }

  static void build_append_pci_bus_devices(AcpiAml *parent_scope, PCIBus *bus,

What exactly is the problem?  A tiny bit more verbose but the lifetime
of all objects is now explicit.

A matter of taste, but I personally don't like this p everywhere,
it makes everything hard to read.

I am still wondering about the static variable that will hold
all of the instances and will be explicitly freed at the end.

Thanks,
Marcel

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

2015-02-05 Thread Marcel Apfelbaum


On 01/28/2015 12:29 AM, Igor Mammedov wrote:

On Mon, 26 Jan 2015 18:17:55 +0200
Michael S. Tsirkin m...@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 imamm...@redhat.com wrote:


On Sat, 24 Jan 2015 18:33:50 +0200
Michael S. Tsirkin m...@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 m...@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();

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

2015-02-05 Thread Marcel Apfelbaum


On 01/28/2015 12:00 PM, Igor Mammedov wrote:

On Wed, 28 Jan 2015 09:56:26 +0200

[...]


Hence is suggesting at least to hide AmlPool internally in API
without exposing it to user. We can provide for user
init/free API to manage internal AmlPool manually, allowing
him to select when to do initialization and cleanup.

This I like the most.



Claudio, Marcel, Shannon,
As the first API users, could you give your feedback on the topic.

As a happy API user, I really like it as it is.
Since we do need to address the allocation/freeing issues
I like the internal AmlPool as I stated already in a few places :)

Thanks,
Marcel

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

2015-01-29 Thread Igor Mammedov

On Thu, 29 Jan 2015 15:46:32 +0800
Shannon Zhao zhaoshengl...@huawei.com wrote:

 On 2015/1/28 18:00, Igor Mammedov wrote:
  On Wed, 28 Jan 2015 09:56:26 +0200
  Michael S. Tsirkin m...@redhat.com wrote:
  
  I've tried redo series with passing alloc list as first argument,
  looks ugly as hell
 
  I tried too. Not too bad at all. See below:
 
  diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
  index f66da5d..820504a 100644
  --- a/hw/i386/acpi-build.c
  +++ b/hw/i386/acpi-build.c
  @@ -491,14 +491,14 @@ static void acpi_set_pci_info(void)
   }
   }
   
  -static void build_append_pcihp_notify_entry(AcpiAml *method, int slot)
  +static void build_append_pcihp_notify_entry(AmlPool *p, AcpiAml *method, 
  int slot)
   {
  -AcpiAml if_ctx;
  +AcpiAml *if_ctx;
   int32_t devfn = PCI_DEVFN(slot, 0);
   
  -if_ctx = acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U  slot)));
  -aml_append(if_ctx, acpi_notify(acpi_name(S%.02X, devfn), 
  acpi_arg1()));
  -aml_append(method, if_ctx);
  +if_ctx = acpi_if(p, acpi_and(p, acpi_arg0(), acpi_int(p, 0x1U  
  slot)));
  +aml_append(p, if_ctx, acpi_notify(p, acpi_name(p, S%.02X, devfn), 
  acpi_arg1(p)));
  +aml_append(p, method, if_ctx);
   }
   
   static void build_append_pci_bus_devices(AcpiAml *parent_scope, PCIBus 
  *bus,
 
  What exactly is the problem?  A tiny bit more verbose but the lifetime
  of all objects is now explicit.
  every usage of aml_foo()/build_append_pcihp_notify_entry() tags along
  extra pointer which is not really necessary for user to know. If possible
  user shouldn't care about it and concentrate on composing AML instead.
  
  Whole point of passing AmlPool and record every allocation is to avoid
  mistakes like:
  
  acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U  slot)));
  
  and forgetting to assign object returned by call anywhere,
  it's basically the same as calling malloc() without
  using result anywhere, however neither libc nor glib
  force user to pass allocator (in our case garbage collector)
  in every call that allocates memory. Let's just follow common
  convention here (#4) where an object is allocated by API call
  (i.e like object_new(FOO), gtk_widget_foo()).
  
  Hence is suggesting at least to hide AmlPool internally in API
  without exposing it to user. We can provide for user
  init/free API to manage internal AmlPool manually, allowing
  him to select when to do initialization and cleanup.
  
  Claudio, Marcel, Shannon,
  As the first API users, could you give your feedback on the topic.
  
 
 In my opinion, it's good to make users focused on ACPI table construction 
 through
 auto memory management. And it makes the code clear.
 
 PS:
 We're talking about use-after-free, like below example. But this example 
 really exist?
 During generating ACPI tables for virt machine, I don't encounter this case.
 
 For example:
   aml_append(a, b);
   aml_append(a, b);
Thanks for your reply,

yesterday we've came to a compromise, where API will stay as it is externally
only it will take all AML arguments by pointer. And we would introduce
allocation list internally inside API, User only would need to initialize it
before using API and free it after using API.

 
 Thanks,
 Shannon

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

2015-01-28 Thread Andrew Jones

On Wed, Jan 28, 2015 at 09:56:26AM +0200, Michael S. Tsirkin wrote:
  I've tried redo series with passing alloc list as first argument,
  looks ugly as hell
 
 I tried too. Not too bad at all. See below:

I'm not so sure. Looking at the version below, I find the
acpi_arg1(p) the most distracting. That API call creates
the simplest object, so should be the simplest looking. Actually,
you suggested that acpi_arg1(), a wrapper to make things look
even simpler, wasn't necessary, acpi_arg(1) would be fine. I
agree with that, but now we'd have acpi_arg(p, 1), which is
really starting to clutter an AML composition built with many
such calls.

 
 diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
 index f66da5d..820504a 100644
 --- a/hw/i386/acpi-build.c
 +++ b/hw/i386/acpi-build.c
 @@ -491,14 +491,14 @@ static void acpi_set_pci_info(void)
  }
  }
  
 -static void build_append_pcihp_notify_entry(AcpiAml *method, int slot)
 +static void build_append_pcihp_notify_entry(AmlPool *p, AcpiAml *method, int 
 slot)
  {
 -AcpiAml if_ctx;
 +AcpiAml *if_ctx;
  int32_t devfn = PCI_DEVFN(slot, 0);
  
 -if_ctx = acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U  slot)));
 -aml_append(if_ctx, acpi_notify(acpi_name(S%.02X, devfn), 
 acpi_arg1()));
 -aml_append(method, if_ctx);
 +if_ctx = acpi_if(p, acpi_and(p, acpi_arg0(), acpi_int(p, 0x1U  slot)));
^ forgot your p here
 +aml_append(p, if_ctx, acpi_notify(p, acpi_name(p, S%.02X, devfn), 
 acpi_arg1(p)));
 +aml_append(p, method, if_ctx);
  }
  
  static void build_append_pci_bus_devices(AcpiAml *parent_scope, PCIBus *bus,
 
 What exactly is the problem?  A tiny bit more verbose but the lifetime
 of all objects is now explicit.

It's probably just a personal preference thing. Igor and I prefer the
sequence of AML composing calls to appear as simple as possible, i.e.
develop the cleanest API as possible. To do this we need to find ways
to hide the memory management, which comes at a cost of using a model
that supports garbage collection, or adding a global variable to hide
the pool. Your preference appears to be to keep memory management as
simple and explicit as possible, at the expense of peppering each AML
build function with a bunch of 'p's. I agree with Igor that we should
get votes from the initial consumers of this API.

Thanks,
drew

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

2015-01-28 Thread Michael S. Tsirkin

On Wed, Jan 28, 2015 at 11:50:14AM +0100, Igor Mammedov wrote:
 On Wed, 28 Jan 2015 12:24:23 +0200
 Michael S. Tsirkin m...@redhat.com wrote:
 
  On Wed, Jan 28, 2015 at 11:00:23AM +0100, Igor Mammedov wrote:
   On Wed, 28 Jan 2015 09:56:26 +0200
   Michael S. Tsirkin m...@redhat.com wrote:
   
 I've tried redo series with passing alloc list as first argument,
 looks ugly as hell

I tried too. Not too bad at all. See below:

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f66da5d..820504a 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -491,14 +491,14 @@ static void acpi_set_pci_info(void)
 }
 }
 
-static void build_append_pcihp_notify_entry(AcpiAml *method, int slot)
+static void build_append_pcihp_notify_entry(AmlPool *p, AcpiAml 
*method, int slot)
 {
-AcpiAml if_ctx;
+AcpiAml *if_ctx;
 int32_t devfn = PCI_DEVFN(slot, 0);
 
-if_ctx = acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U  slot)));
-aml_append(if_ctx, acpi_notify(acpi_name(S%.02X, devfn), 
acpi_arg1()));
-aml_append(method, if_ctx);
+if_ctx = acpi_if(p, acpi_and(p, acpi_arg0(), acpi_int(p, 0x1U  
slot)));
+aml_append(p, if_ctx, acpi_notify(p, acpi_name(p, S%.02X, 
devfn), acpi_arg1(p)));
+aml_append(p, method, if_ctx);
 }
 
 static void build_append_pci_bus_devices(AcpiAml *parent_scope, PCIBus 
*bus,

What exactly is the problem?  A tiny bit more verbose but the lifetime
of all objects is now explicit.
   every usage of aml_foo()/build_append_pcihp_notify_entry() tags along
   extra pointer which is not really necessary for user to know.
  
  It's necessary to make memory management explicit. Consider:
  
  alloc_pool();
  acpi_arg0();
  free_pool();
  acpi_arg1();
  
  Looks ok but isn't because acpi_arg1 silently allocates memory.
 with pool hidden inside API, acpi_arg1() would crash
 when accessing freed pool.
  p = alloc_pool();
  acpi_arg0(p);
  free_pool(p);
  acpi_arg1(p);
  
  It's obvious what's wrong here: p is used after it's freed.
 it's just like above but more verbose with the same end result.

  Come on, it's 3 characters per call.  I think it's a reasonable
  compromize.
 That characters will spread over all API usage and I don't really
 wish to invent garbage collection and then rewrite everything
 to a cleaner API afterwards.

If the cleaner API is just a removed parameter, a single sparse
patch will do it automatically.
Something like the following:

identifier func;
identifier call;
@@

-func(AmlPool *p, ...)
+func(...)
{
-call(p, ...)
+call(...)
}

 I admit that internal global pool is not the best thing,
 but that would be reasonable compromise to still get garbage
 collection without polluting API.

The issue is lifetime of objects being implicit in the API,
it doesn't look like a global pool fixes that.


 Otherwise lets use common pattern and go QOM way, which also takes
 care about use-after-free concern but lacks garbage collector.

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

2015-01-28 Thread Igor Mammedov

On Wed, 28 Jan 2015 09:56:26 +0200
Michael S. Tsirkin m...@redhat.com wrote:

  I've tried redo series with passing alloc list as first argument,
  looks ugly as hell
 
 I tried too. Not too bad at all. See below:
 
 diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
 index f66da5d..820504a 100644
 --- a/hw/i386/acpi-build.c
 +++ b/hw/i386/acpi-build.c
 @@ -491,14 +491,14 @@ static void acpi_set_pci_info(void)
  }
  }
  
 -static void build_append_pcihp_notify_entry(AcpiAml *method, int slot)
 +static void build_append_pcihp_notify_entry(AmlPool *p, AcpiAml *method, int 
 slot)
  {
 -AcpiAml if_ctx;
 +AcpiAml *if_ctx;
  int32_t devfn = PCI_DEVFN(slot, 0);
  
 -if_ctx = acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U  slot)));
 -aml_append(if_ctx, acpi_notify(acpi_name(S%.02X, devfn), 
 acpi_arg1()));
 -aml_append(method, if_ctx);
 +if_ctx = acpi_if(p, acpi_and(p, acpi_arg0(), acpi_int(p, 0x1U  slot)));
 +aml_append(p, if_ctx, acpi_notify(p, acpi_name(p, S%.02X, devfn), 
 acpi_arg1(p)));
 +aml_append(p, method, if_ctx);
  }
  
  static void build_append_pci_bus_devices(AcpiAml *parent_scope, PCIBus *bus,
 
 What exactly is the problem?  A tiny bit more verbose but the lifetime
 of all objects is now explicit.
every usage of aml_foo()/build_append_pcihp_notify_entry() tags along
extra pointer which is not really necessary for user to know. If possible
user shouldn't care about it and concentrate on composing AML instead.

Whole point of passing AmlPool and record every allocation is to avoid
mistakes like:

acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U  slot)));

and forgetting to assign object returned by call anywhere,
it's basically the same as calling malloc() without
using result anywhere, however neither libc nor glib
force user to pass allocator (in our case garbage collector)
in every call that allocates memory. Let's just follow common
convention here (#4) where an object is allocated by API call
(i.e like object_new(FOO), gtk_widget_foo()).

Hence is suggesting at least to hide AmlPool internally in API
without exposing it to user. We can provide for user
init/free API to manage internal AmlPool manually, allowing
him to select when to do initialization and cleanup.

Claudio, Marcel, Shannon,
As the first API users, could you give your feedback on the topic.

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

2015-01-28 Thread Claudio Fontana

Hello Igor,

It's quite difficult to understand all the different options, there have been 
quite a few flying around.
I'll comment this mail in particular, ignoring for the moment all the other 
exchanges
(about QOM etc).

On 28.01.2015 11:00, Igor Mammedov wrote:
 On Wed, 28 Jan 2015 09:56:26 +0200
 Michael S. Tsirkin m...@redhat.com wrote:
 
 I've tried redo series with passing alloc list as first argument,
 looks ugly as hell

 I tried too. Not too bad at all. See below:

 diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
 index f66da5d..820504a 100644
 --- a/hw/i386/acpi-build.c
 +++ b/hw/i386/acpi-build.c
 @@ -491,14 +491,14 @@ static void acpi_set_pci_info(void)
  }
  }
  
 -static void build_append_pcihp_notify_entry(AcpiAml *method, int slot)
 +static void build_append_pcihp_notify_entry(AmlPool *p, AcpiAml *method, 
 int slot)
  {
 -AcpiAml if_ctx;
 +AcpiAml *if_ctx;
  int32_t devfn = PCI_DEVFN(slot, 0);
  
 -if_ctx = acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U  slot)));
 -aml_append(if_ctx, acpi_notify(acpi_name(S%.02X, devfn), 
 acpi_arg1()));
 -aml_append(method, if_ctx);
 +if_ctx = acpi_if(p, acpi_and(p, acpi_arg0(), acpi_int(p, 0x1U  
 slot)));
 +aml_append(p, if_ctx, acpi_notify(p, acpi_name(p, S%.02X, devfn), 
 acpi_arg1(p)));
 +aml_append(p, method, if_ctx);
  }
  
  static void build_append_pci_bus_devices(AcpiAml *parent_scope, PCIBus *bus,

 What exactly is the problem?  A tiny bit more verbose but the lifetime
 of all objects is now explicit.

I think both options are ok.
The extra parameter is just basically passed around if I understand correctly, 
that doesn't seem terrible.

 every usage of aml_foo()/build_append_pcihp_notify_entry() tags along
 extra pointer which is not really necessary for user to know. If possible
 user shouldn't care about it and concentrate on composing AML instead.
 
 Whole point of passing AmlPool and record every allocation is to avoid
 mistakes like:
 
 acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U  slot)));
 
 and forgetting to assign object returned by call anywhere,
 it's basically the same as calling malloc() without
 using result anywhere, however neither libc nor glib
 force user to pass allocator (in our case garbage collector)
 in every call that allocates memory. Let's just follow common
 convention here (#4) where an object is allocated by API call
 (i.e like object_new(FOO), gtk_widget_foo()).
 
 Hence is suggesting at least to hide AmlPool internally in API
 without exposing it to user. We can provide for user
 init/free API to manage internal AmlPool manually, allowing
 him to select when to do initialization and cleanup.
 
 Claudio, Marcel, Shannon,
 As the first API users, could you give your feedback on the topic.
 

Personally I find as mentioned both options ok.

Your (Igor's) proposal looks better when staring at the code which uses the 
API,
Michael's suggestion is to avoid any confusion around when an object is 
allocated / freed, and that's a valid point.

Sorry for being so neutral, but really both options seem to have their merits 
and both seem substantially fine to me.

Ciao,

Claudio

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

2015-01-28 Thread Igor Mammedov

On Wed, 28 Jan 2015 12:24:23 +0200
Michael S. Tsirkin m...@redhat.com wrote:

 On Wed, Jan 28, 2015 at 11:00:23AM +0100, Igor Mammedov wrote:
  On Wed, 28 Jan 2015 09:56:26 +0200
  Michael S. Tsirkin m...@redhat.com wrote:
  
I've tried redo series with passing alloc list as first argument,
looks ugly as hell
   
   I tried too. Not too bad at all. See below:
   
   diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
   index f66da5d..820504a 100644
   --- a/hw/i386/acpi-build.c
   +++ b/hw/i386/acpi-build.c
   @@ -491,14 +491,14 @@ static void acpi_set_pci_info(void)
}
}

   -static void build_append_pcihp_notify_entry(AcpiAml *method, int slot)
   +static void build_append_pcihp_notify_entry(AmlPool *p, AcpiAml *method, 
   int slot)
{
   -AcpiAml if_ctx;
   +AcpiAml *if_ctx;
int32_t devfn = PCI_DEVFN(slot, 0);

   -if_ctx = acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U  slot)));
   -aml_append(if_ctx, acpi_notify(acpi_name(S%.02X, devfn), 
   acpi_arg1()));
   -aml_append(method, if_ctx);
   +if_ctx = acpi_if(p, acpi_and(p, acpi_arg0(), acpi_int(p, 0x1U  
   slot)));
   +aml_append(p, if_ctx, acpi_notify(p, acpi_name(p, S%.02X, devfn), 
   acpi_arg1(p)));
   +aml_append(p, method, if_ctx);
}

static void build_append_pci_bus_devices(AcpiAml *parent_scope, PCIBus 
   *bus,
   
   What exactly is the problem?  A tiny bit more verbose but the lifetime
   of all objects is now explicit.
  every usage of aml_foo()/build_append_pcihp_notify_entry() tags along
  extra pointer which is not really necessary for user to know.
 
 It's necessary to make memory management explicit. Consider:
 
 alloc_pool();
 acpi_arg0();
 free_pool();
 acpi_arg1();
 
 Looks ok but isn't because acpi_arg1 silently allocates memory.
with pool hidden inside API, acpi_arg1() would crash
when accessing freed pool.

 p = alloc_pool();
 acpi_arg0(p);
 free_pool(p);
 acpi_arg1(p);
 
 It's obvious what's wrong here: p is used after it's freed.
it's just like above but more verbose with the same end result.
 
 Come on, it's 3 characters per call.  I think it's a reasonable
 compromize.
That characters will spread over all API usage and I don't really
wish to invent garbage collection and then rewrite everything
to a cleaner API afterwards.
I admit that internal global pool is not the best thing,
but that would be reasonable compromise to still get garbage
collection without polluting API.
Otherwise lets use common pattern and go QOM way, which also takes
care about use-after-free concern but lacks garbage collector.

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

2015-01-28 Thread Michael S. Tsirkin

On Wed, Jan 28, 2015 at 11:00:23AM +0100, Igor Mammedov wrote:
 On Wed, 28 Jan 2015 09:56:26 +0200
 Michael S. Tsirkin m...@redhat.com wrote:
 
   I've tried redo series with passing alloc list as first argument,
   looks ugly as hell
  
  I tried too. Not too bad at all. See below:
  
  diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
  index f66da5d..820504a 100644
  --- a/hw/i386/acpi-build.c
  +++ b/hw/i386/acpi-build.c
  @@ -491,14 +491,14 @@ static void acpi_set_pci_info(void)
   }
   }
   
  -static void build_append_pcihp_notify_entry(AcpiAml *method, int slot)
  +static void build_append_pcihp_notify_entry(AmlPool *p, AcpiAml *method, 
  int slot)
   {
  -AcpiAml if_ctx;
  +AcpiAml *if_ctx;
   int32_t devfn = PCI_DEVFN(slot, 0);
   
  -if_ctx = acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U  slot)));
  -aml_append(if_ctx, acpi_notify(acpi_name(S%.02X, devfn), 
  acpi_arg1()));
  -aml_append(method, if_ctx);
  +if_ctx = acpi_if(p, acpi_and(p, acpi_arg0(), acpi_int(p, 0x1U  
  slot)));
  +aml_append(p, if_ctx, acpi_notify(p, acpi_name(p, S%.02X, devfn), 
  acpi_arg1(p)));
  +aml_append(p, method, if_ctx);
   }
   
   static void build_append_pci_bus_devices(AcpiAml *parent_scope, PCIBus 
  *bus,
  
  What exactly is the problem?  A tiny bit more verbose but the lifetime
  of all objects is now explicit.
 every usage of aml_foo()/build_append_pcihp_notify_entry() tags along
 extra pointer which is not really necessary for user to know.

It's necessary to make memory management explicit. Consider:

alloc_pool();
acpi_arg0();
free_pool();
acpi_arg1();

Looks ok but isn't because acpi_arg1 silently allocates memory.

p = alloc_pool();
acpi_arg0(p);
free_pool(p);
acpi_arg1(p);

It's obvious what's wrong here: p is used after it's freed.

Come on, it's 3 characters per call.  I think it's a reasonable
compromize.

-- 
MST

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

2015-01-28 Thread Shannon Zhao

On 2015/1/28 18:00, Igor Mammedov wrote:
 On Wed, 28 Jan 2015 09:56:26 +0200
 Michael S. Tsirkin m...@redhat.com wrote:
 
 I've tried redo series with passing alloc list as first argument,
 looks ugly as hell

 I tried too. Not too bad at all. See below:

 diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
 index f66da5d..820504a 100644
 --- a/hw/i386/acpi-build.c
 +++ b/hw/i386/acpi-build.c
 @@ -491,14 +491,14 @@ static void acpi_set_pci_info(void)
  }
  }
  
 -static void build_append_pcihp_notify_entry(AcpiAml *method, int slot)
 +static void build_append_pcihp_notify_entry(AmlPool *p, AcpiAml *method, 
 int slot)
  {
 -AcpiAml if_ctx;
 +AcpiAml *if_ctx;
  int32_t devfn = PCI_DEVFN(slot, 0);
  
 -if_ctx = acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U  slot)));
 -aml_append(if_ctx, acpi_notify(acpi_name(S%.02X, devfn), 
 acpi_arg1()));
 -aml_append(method, if_ctx);
 +if_ctx = acpi_if(p, acpi_and(p, acpi_arg0(), acpi_int(p, 0x1U  
 slot)));
 +aml_append(p, if_ctx, acpi_notify(p, acpi_name(p, S%.02X, devfn), 
 acpi_arg1(p)));
 +aml_append(p, method, if_ctx);
  }
  
  static void build_append_pci_bus_devices(AcpiAml *parent_scope, PCIBus *bus,

 What exactly is the problem?  A tiny bit more verbose but the lifetime
 of all objects is now explicit.
 every usage of aml_foo()/build_append_pcihp_notify_entry() tags along
 extra pointer which is not really necessary for user to know. If possible
 user shouldn't care about it and concentrate on composing AML instead.
 
 Whole point of passing AmlPool and record every allocation is to avoid
 mistakes like:
 
 acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U  slot)));
 
 and forgetting to assign object returned by call anywhere,
 it's basically the same as calling malloc() without
 using result anywhere, however neither libc nor glib
 force user to pass allocator (in our case garbage collector)
 in every call that allocates memory. Let's just follow common
 convention here (#4) where an object is allocated by API call
 (i.e like object_new(FOO), gtk_widget_foo()).
 
 Hence is suggesting at least to hide AmlPool internally in API
 without exposing it to user. We can provide for user
 init/free API to manage internal AmlPool manually, allowing
 him to select when to do initialization and cleanup.
 
 Claudio, Marcel, Shannon,
 As the first API users, could you give your feedback on the topic.
 

In my opinion, it's good to make users focused on ACPI table construction 
through
auto memory management. And it makes the code clear.

PS:
We're talking about use-after-free, like below example. But this example really 
exist?
During generating ACPI tables for virt machine, I don't encounter this case.

For example:
aml_append(a, b);
aml_append(a, b);

Thanks,
Shannon

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

2015-01-27 Thread Igor Mammedov

On Mon, 26 Jan 2015 18:17:55 +0200
Michael S. Tsirkin m...@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 imamm...@redhat.com wrote:

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

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

2015-01-27 Thread Michael S. Tsirkin

 I've tried redo series with passing alloc list as first argument,
 looks ugly as hell

I tried too. Not too bad at all. See below:

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f66da5d..820504a 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -491,14 +491,14 @@ static void acpi_set_pci_info(void)
 }
 }
 
-static void build_append_pcihp_notify_entry(AcpiAml *method, int slot)
+static void build_append_pcihp_notify_entry(AmlPool *p, AcpiAml *method, int 
slot)
 {
-AcpiAml if_ctx;
+AcpiAml *if_ctx;
 int32_t devfn = PCI_DEVFN(slot, 0);
 
-if_ctx = acpi_if(acpi_and(acpi_arg0(), acpi_int(0x1U  slot)));
-aml_append(if_ctx, acpi_notify(acpi_name(S%.02X, devfn), acpi_arg1()));
-aml_append(method, if_ctx);
+if_ctx = acpi_if(p, acpi_and(p, acpi_arg0(), acpi_int(p, 0x1U  slot)));
+aml_append(p, if_ctx, acpi_notify(p, acpi_name(p, S%.02X, devfn), 
acpi_arg1(p)));
+aml_append(p, method, if_ctx);
 }
 
 static void build_append_pci_bus_devices(AcpiAml *parent_scope, PCIBus *bus,

What exactly is the problem?  A tiny bit more verbose but the lifetime
of all objects is now explicit.


-- 
MST

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

2015-01-27 Thread Michael S. Tsirkin

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 m...@redhat.com wrote:

On Sat, 24 Jan 2015 18:33:50 +0200
Michael S. Tsirkin m...@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 m...@redhat.com wrote:

[...]
I refuse to give up on cleaner and simpler API yet :)

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

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.)

since they allow to construct AML in a more simple/manageable
way i.e.

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

2015-01-26 Thread Andrew Jones

On Mon, Jan 26, 2015 at 04:09:20PM +0100, Igor Mammedov wrote:
 On Mon, 26 Jan 2015 10:57:21 +0100
 Igor Mammedov imamm...@redhat.com wrote:
 
  On Sat, 24 Jan 2015 18:33:50 +0200
  Michael S. Tsirkin m...@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 m...@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);

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

2015-01-26 Thread Igor Mammedov

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

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

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

2015-01-26 Thread Michael S. Tsirkin

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

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)

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();

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

2015-01-26 Thread Igor Mammedov

On Sat, 24 Jan 2015 18:33:50 +0200
Michael S. Tsirkin m...@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 m...@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 m...@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 m...@redhat.com wrote:
  
   On Thu, Jan 22, 2015 at 02:49:45PM +, 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 imamm...@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:

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

2015-01-26 Thread Michael S. Tsirkin

 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?

-- 
MST

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

2015-01-24 Thread Michael S. Tsirkin

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 m...@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 m...@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 m...@redhat.com wrote:
 
  On Thu, Jan 22, 2015 at 02:49:45PM +, 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 imamm...@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

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

2015-01-23 Thread Igor Mammedov

On Fri, 23 Jan 2015 15:55:11 +0200
Michael S. Tsirkin m...@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 m...@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 m...@redhat.com wrote:

 On Thu, Jan 22, 2015 at 02:49:45PM +, 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 imamm...@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

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

2015-01-23 Thread Igor Mammedov

On Fri, 23 Jan 2015 15:24:24 +0200
Michael S. Tsirkin m...@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 m...@redhat.com wrote:
  
   On Thu, Jan 22, 2015 at 02:49:45PM +, 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 imamm...@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

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

2015-01-23 Thread Michael S. Tsirkin

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 m...@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 m...@redhat.com wrote:
   
On Thu, Jan 22, 2015 at 02:49:45PM +, 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 imamm...@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

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

2015-01-23 Thread Michael S. Tsirkin

On Thu, Jan 22, 2015 at 02:49:45PM +, 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 imamm...@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

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

2015-01-23 Thread Michael S. Tsirkin

On Thu, Jan 22, 2015 at 02:49:45PM +, 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 imamm...@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

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

2015-01-23 Thread Michael S. Tsirkin

On Thu, Jan 22, 2015 at 02:49:45PM +, 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 imamm...@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

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

2015-01-23 Thread Igor Mammedov

On Fri, 23 Jan 2015 10:03:03 +0200
Michael S. Tsirkin m...@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

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

2015-01-23 Thread Igor Mammedov

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

 On Thu, Jan 22, 2015 at 02:49:45PM +, 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 imamm...@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

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

2015-01-23 Thread Michael S. Tsirkin

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 m...@redhat.com wrote:
 
  On Thu, Jan 22, 2015 at 02:49:45PM +, 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 imamm...@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

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

2015-01-23 Thread Michael S. Tsirkin

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 m...@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

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

2015-01-22 Thread Igor Mammedov

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 imamm...@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;
+
+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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

31 matches

Site Navigation

Mail list logo

Footer information