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

[Andrew Jones]

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

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

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

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

Thanks,
drew