Re: [Qemu-devel] [PATCH v6 05/33] acpi: add aml_object_type

[Igor Mammedov]

On Mon, 9 Nov 2015 13:35:51 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> On Fri, Oct 30, 2015 at 01:55:59PM +0800, Xiao Guangrong wrote:
> > Implement ObjectType which is used by NVDIMM _DSM method in
> > later patch
> > 
> > Signed-off-by: Xiao Guangrong <address@hidden>
> 
> I had to go dig in the _DSM patch to see how it's used.
> And sure enough, callers have to know AML to make
> sense of it. So pls don't split out tiny patches like this.
> include the callee with the caller.
I'd prefer AML API patches as separate ones, as it makes
easier to review vs ACPI spec and also easier to reuse
in another series.
And once they are ok/reviewed we can merge them ahead of
users, so next series respins won't have to post the same
patches over and over and we won't have to review them
again every respin and others could use already merged API
instead of duplicating work that's been already done.

> 
> > ---
> >  hw/acpi/aml-build.c         | 8 ++++++++
> >  include/hw/acpi/aml-build.h | 1 +
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index efc06ab..9f792ab 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -1178,6 +1178,14 @@ Aml *aml_concatenate(Aml *source1, Aml *source2, Aml 
> > *target)
> >      return var;
> >  }
> >  
> > +/* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefObjectType */
> > +Aml *aml_object_type(Aml *object)
> > +{
> > +    Aml *var = aml_opcode(0x8E /* ObjectTypeOp */);
> > +    aml_append(var, object);
> > +    return var;
> > +}
> > +
> 
> It would be better to have a higher level API
> that can be used without knowning AML.
> For example:
> 
>       aml_object_type_is_package()
Higher level API is fine but it's better be done
on top of AML one, i.e. add it in addition to
 aml_object_type()


> 
> 
> 
> >  void
> >  build_header(GArray *linker, GArray *table_data,
> >               AcpiTableHeader *h, const char *sig, int len, uint8_t rev)
> > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > index 325782d..5b8a118 100644
> > --- a/include/hw/acpi/aml-build.h
> > +++ b/include/hw/acpi/aml-build.h
> > @@ -278,6 +278,7 @@ Aml *aml_derefof(Aml *arg);
> >  Aml *aml_sizeof(Aml *arg);
> >  Aml *aml_create_field(Aml *srcbuf, Aml *index, Aml *len, const char *name);
> >  Aml *aml_concatenate(Aml *source1, Aml *source2, Aml *target);
> > +Aml *aml_object_type(Aml *object);
> >  
> >  void
> >  build_header(GArray *linker, GArray *table_data,
> > -- 
> > 1.8.3.1
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to address@hidden
> More majordomo info at  http://vger.kernel.org/majordomo-info.html