Re: [Qemu-devel] [PATCH 07/74] acpi: aml: add helper for Opcode Arg2 Arg2 [Dst] AML pattern

[Igor Mammedov]

On Thu, 10 Dec 2015 09:59:16 +0800
Shannon Zhao <address@hidden> wrote:

> 
> 
> On 2015/12/10 7:41, Igor Mammedov wrote:
> > Currently AML API doesn't compose terms in form of
> > following pattern:
> > 
> >    Opcode Arg2 Arg2 [Dst]
> > 
> > but ASL used in piix4/q35 DSDT ACPI tables uses that
> > form, so for clean conversion of it, AML API should
> > be able to handle an optional 'Dst' argumet used there.
> > 
> > Since above pattern is used by arithmetic/bit ops,
> > introduce helper that they could reuse.
> > It reduces code duplication in existing 5 aml_foo()
> > functions and also will prevent more duplication
> > when exiting functions are extended to support
> > optional 'Dst' argument.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  hw/acpi/aml-build.c | 61
> > ++++++++++++++++++++++++++++------------------------- 1 file
> > changed, 32 insertions(+), 29 deletions(-)
> > 
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index a6e4c54..22015d2 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -436,44 +436,55 @@ Aml *aml_store(Aml *val, Aml *target)
> >      return var;
> >  }
> >  
> > -/* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefAnd */
> > -Aml *aml_and(Aml *arg1, Aml *arg2)
> > +/**
> > + * build_opcode_2arg_dst:
> > + * @op: 1-byte opcode
> > + * @arg1: 1st operand
> > + * @arg2: 2nd operand
> > + * @dst: optional target to store to, set to NULL if it's not
> > required
> > + *
> > + * An internal helper to compose AML terms that have
> > + *   "Op Operand Operand Target"
> > + * pattern.
> > + *
> > + * Returns: The newly allocated and composed according to patter
> > Aml object.
> > + */
> > +static Aml *
> > +build_opcode_2arg_dst(uint8_t op, Aml *arg1, Aml *arg2, Aml *dst)
> >  {
> > -    Aml *var = aml_opcode(0x7B /* AndOp */);
> > +    Aml *var = aml_opcode(op);
> >      aml_append(var, arg1);
> >      aml_append(var, arg2);
> > -    build_append_byte(var->buf, 0x00 /* NullNameOp */);
> > +    if (dst) {
> > +        aml_append(var, dst);
> > +    } else {
> > +        build_append_byte(var->buf, 0x00 /* NullNameOp */);
> > +    }
> >      return var;
> >  }
> > 
> This looks good. Maybe you could add a helper like
> build_opcode_2arg(), then aml_lor() and aml_lgreater() could use this.

If it reduces number code size, it could be made on top of this
series.

If AML API patches ('acpi:' prefixed) would have 'Reviewed-by:' we
could merge them first, before the PC:DSTD conversion patches are
reviewed so that AML API could be improved/extendend further without
conflicts.

That would also make series smaller on respin.