[v4,for-2.3,01/25] acpi: fix aml_equal term implementation

The DefLEqual op does not have a target operand. Remove it.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/acpi/aml-build.c | 1 -
 1 file changed, 1 deletion(-)

On Sun,  8 Mar 2015 13:16:03 +0200
Marcel Apfelbaum <marcel@redhat.com> wrote:

> The DefLEqual op does not have a target operand. Remove it.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  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
   ...
}
>      return var;
>  }
>

On Mon, Mar 09, 2015 at 11:28:22AM +0100, Igor Mammedov wrote:
> On Sun,  8 Mar 2015 13:16:03 +0200
> Marcel Apfelbaum <marcel@redhat.com> wrote:
> 
> > The DefLEqual op does not have a target operand. Remove it.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
> > ---
> >  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?

> >      return var;
> >  }
> >

On Mon, 9 Mar 2015 12:04:51 +0100
"Michael S. Tsirkin" <mst@redhat.com> 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 <marcel@redhat.com> wrote:
> > 
> > > The DefLEqual op does not have a target operand. Remove it.
> > > 
> > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > > ---
> > >  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.

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.

> 
> > >      return var;
> > >  }
> > >

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" <mst@redhat.com> 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 <marcel@redhat.com> wrote:
> > > 
> > > > The DefLEqual op does not have a target operand. Remove it.
> > > > 
> > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > > 
> > > > ---
> > > >  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;
}