Re: [Qemu-ppc] [Qemu-devel] [PATCH v4 for-2.3 01/25] acpi: fix aml_equal term implementation

[Michael S. Tsirkin]

On Mon, Mar 09, 2015 at 01:26:40PM +0100, Igor Mammedov wrote:
> On Mon, 9 Mar 2015 12:04:51 +0100
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Mon, Mar 09, 2015 at 11:28:22AM +0100, Igor Mammedov wrote:
> > > On Sun,  8 Mar 2015 13:16:03 +0200
> > > Marcel Apfelbaum <address@hidden> wrote:
> > > 
> > > > The DefLEqual op does not have a target operand. Remove it.
> > > > 
> > > > Signed-off-by: Marcel Apfelbaum <address@hidden>
> > > Reviewed-by: Igor Mammedov <address@hidden>
> > > 
> > > > ---
> > > >  hw/acpi/aml-build.c | 1 -
> > > >  1 file changed, 1 deletion(-)
> > > > 
> > > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > > index 876cada..0d14561 100644
> > > > --- a/hw/acpi/aml-build.c
> > > > +++ b/hw/acpi/aml-build.c
> > > > @@ -542,7 +542,6 @@ Aml *aml_equal(Aml *arg1, Aml *arg2)
> > > >      Aml *var = aml_opcode(0x93 /* LequalOp */);
> > > >      aml_append(var, arg1);
> > > >      aml_append(var, arg2);
> > > > -    build_append_int(var->buf, 0x00); /* NullNameOp */
> > > It's just happens to work in case CPU and PCI hotplug because
> > > it LEqual was the only predicate in if block and NullNameOp
> > > was considered as part of inner code-block, like:
> > > if (LEqual(a, b)) {
> > >    NullName; // nop
> > >    ...
> > > }
> > 
> > So - maybe aml_if should get 3rd parameter - the command?
> it's not only one command it's block of AML code inside of 'if' scope.
> Adding 3rd argument would mean inventing another not defined by spec element
> like aml_block() where you could put AML items that are in block,
> I'd like to keep non spec items to minimum and not add them unless we have to.

In fact, it's TermList in spec, isn't it?
But I don't insist.

> 
> Then for consistence purposes we would add this 'aml_block' argument
> to other block constructs like 'device, scope, package, ...'
> So I think current way of defining context and then putting items in it
> is pretty clean way as opposed to doing it backwards, first defining
> elements somewhere and then passing that somewhere as argument to
> a AML block construct.


I think it's just a question of adding a convenience wrapper
for the most common case.

/* Convenience wrapper for when there's a single
 * term in TermList
 */
Aml *aml_if_then_1term(Aml *predicate, Aml *term)
{
    Aml *if_ctx = aml_if(predicate);

    aml_append(if_ctx, term);
    return if_ctx;
}