Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()
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()
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()
On 01/28/2015 12:29 AM, Igor Mammedov wrote: On Mon, 26 Jan 2015 18:17:55 +0200 "Michael S. Tsirkin" 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 wrote: On Sat, 24 Jan 2015 18:33:50 +0200 "Michael S. Tsirkin" 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" wrote: [...] I refuse to give up on cleaner and simpler API yet :) Your patches are almost there, they are pretty clean, the only issue I think is this passing of AcpiAml by value, sometimes freeing buffer in the process, sometimes not. Currently buffer is allocated by API and is always freed whenever it's passed to another API function. That's why it makes user not to care about memory mgmt. The only limitation of it is if you store AcpiAml return value into some variable you are responsible to use it only once for passing to another API function. Reusing this variable's value (pass it to API function second time) would cause cause use-after-free and freeing-freed bugs. Like this: AcpiAml table = acpi_definition_block("SSDT",...); AcpiAml scope = acpi_scope("PCI0"); aml_append(&table, scope); // <- here scope becomes invalid // a bug aml_append(&table, scope); // use-after-free + freeing-freed bugs There are several approaches to look for resolving above issues: 1. Adopt and use memory mgmt model used by GTK+ in nutshell: http://www.cs.hunter.cuny.edu/~sweiss/course_materials/csci493.70/lecture_notes/GTK_memory_mngmt.pdf In particular adopt behavior of GInitiallyUnowned usage model that will allow to keep convenient chained call style and if necessary reuse objects returned by API by explicitly referencing/dereferencing them if needed. Hmm, it's still easy to misuse. I think I prefer option 2 below. That's basically what we have/use in QOM with object_new(FOO) + object_unref() I have no idea why we invented our own Object infrastructure when we could just use GObject one from already used glib. 2. It's possible to drop freeing inside API completely and record(store in list) every new object inside a table context. When table is constructed, list of created objects could be safely freed. With that it would be safe to reuse every AcpiAml object and avoid free-after-use issues with limitation that created AcpiAml objects shouldn't be used after table was closed. It should cover all practical use of API, i.e. no cross table AcpiAml objects. So each aml_alloc function gets pointer to this list, and adds the new element there. Eventually we do free_all to free all elements, so there isn't even an aml_free to mis-use. I'm thinking a little bit different about implementation though. I still don't like the use of explicit alloc/free being called by API user since it doesn't allow chained API calls and I think it's unnecessary complication see below why. Here is what's true about current API and a I'd like to with it: 1. Every API call (except aml_append) makes aml_alloc(), it's just like a wrapper about object_new(FOO). (current + new impl.) 2 Every API call that takes AML type as input argument 2.1 consumes (frees) it (current impl.) (it's easy to fix use after free concern too, just pass AML by pointer and zero-out memory before it's freed and assert whenever one of input arguments is not correct, i.e. it was reused second time) There is no need for following steps after this one. 2.2 takes ownership of GInitiallyUnowned and adds it to its list of its children. 3. Free children when AML object is destroyed (i.e. ref count zero) That way when toplevel table object (definition block in 42/47) is added to ACPI blob we can unref it, which will cause its whole children tree freed, except for AML objects where API user explicitly took extra reference (i.e. wanted them to reuse in another table) I'd prefer: * 2.1 way to address your current concern of use-after-free as the most simplest one (no reuse is possible however) or * follow already used by QEMU QOM/GObject pattern of implicit alloc/free since they allow to construct AML in a more simple/manageable way i.e. aml_append(method, aml_store(aml_string("foo"), aml_local(0))) ); v.s. explicit headache of alloc/free, which doesn't fix use-after-free anyway and just adds more boiler plate plus makes code har to read read str = aml_alloc(); aml_string(str, "foo"); loc0 = aml_alloc(); aml_local(loc0, 0); store = aml_alloc(); aml_store(store, str, loc0); aml_append(method, store); aml_free(store); aml_free(loc0); aml_free(str); Here is a compromise what I and Michael came to on a phone call: Externally API usage would look like: AmlAllocList *p = some_list_alloc(); Aml *ssdt = aml_def_block(p, "SSDT", ...); Aml *de
Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()
On Thu, 29 Jan 2015 15:46:32 +0800 Shannon Zhao wrote: > On 2015/1/28 18:00, Igor Mammedov wrote: > > On Wed, 28 Jan 2015 09:56:26 +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: > >> > >> 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()
On 2015/1/28 18:00, Igor Mammedov wrote: > On Wed, 28 Jan 2015 09:56:26 +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: >> >> 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()
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" 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" 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()
On Wed, 28 Jan 2015 12:24:23 +0200 "Michael S. Tsirkin" 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" 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()
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()
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" 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()
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" 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()
On Wed, 28 Jan 2015 09:56:26 +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: > > 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()
> 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()
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" 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 wrote: > > > > > > > > > On Sat, 24 Jan 2015 18:33:50 +0200 > > > > > "Michael S. Tsirkin" 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" 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 child
Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()
On Mon, 26 Jan 2015 18:17:55 +0200 "Michael S. Tsirkin" 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 wrote: > > > > > > > On Sat, 24 Jan 2015 18:33:50 +0200 > > > > "Michael S. Tsirkin" 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" 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-fr
Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()
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 wrote: > > > > > On Sat, 24 Jan 2015 18:33:50 +0200 > > > "Michael S. Tsirkin" 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" 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 > > >impli
Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()
On Mon, Jan 26, 2015 at 04:09:20PM +0100, Igor Mammedov wrote: > On Mon, 26 Jan 2015 10:57:21 +0100 > Igor Mammedov wrote: > > > On Sat, 24 Jan 2015 18:33:50 +0200 > > "Michael S. Tsirkin" 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" 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
Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()
On Mon, 26 Jan 2015 10:57:21 +0100 Igor Mammedov wrote: > On Sat, 24 Jan 2015 18:33:50 +0200 > "Michael S. Tsirkin" 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" 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_fr
Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()
> 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()
On Sat, 24 Jan 2015 18:33:50 +0200 "Michael S. Tsirkin" 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" 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" 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" 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 > > > > > > > > --- > > > > > > > > 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 valu
Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()
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" 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" 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" 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 > > > > > > > --- > > > > > > > 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
Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()
On Fri, 23 Jan 2015 15:55:11 +0200 "Michael S. Tsirkin" 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" 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" 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 > > > > > > --- > > > > > > 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 par
Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()
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" 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" 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 > > > > > --- > > > > > 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 @@ > > > >
Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()
On Fri, 23 Jan 2015 15:24:24 +0200 "Michael S. Tsirkin" 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" 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 > > > > --- > > > > 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 > > > > #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()
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" 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 > >
Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()
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" 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 > > > --- > > > 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 > > > #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()
On Fri, 23 Jan 2015 10:11:19 +0200 "Michael S. Tsirkin" 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 > > --- > > 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 > > #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()
On Fri, 23 Jan 2015 10:03:03 +0200 "Michael S. Tsirkin" 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()
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 > --- > 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 > #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()
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 > --- > 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 > #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()
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 > --- > 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 > #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
[Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append()
Adds for dynamic AML creation, which will be used for piecing ASL/AML primitives together and hiding from user/caller details about how nested context should be closed/packed leaving less space for mistakes and necessity to know how AML should be encoded, allowing user to concentrate on ASL representation instead. For example it will allow to create AML like this: AcpiAml scope = acpi_scope("PCI0") AcpiAml dev = acpi_device("PM") aml_append(&dev, acpi_name_decl("_ADR", acpi_int(addr))) aml_append(&scope, dev); Signed-off-by: Igor Mammedov --- 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 #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