Re: [Qemu-devel] [PATCH v6 3/5] acpi: add build_append_namestring() helper

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jan 28, 2015 at 04:44:09PM +0100, Igor Mammedov wrote:
> On Wed, 28 Jan 2015 17:16:17 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jan 28, 2015 at 02:34:48PM +0000, Igor Mammedov wrote:
> > > Use build_append_namestring() instead of build_append_nameseg()
> > > So user won't have to care whether name is NameSeg, NamePath or
> > > NameString.
> > > 
> > > See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
> > > 
> > 
> > typo
> will fix
> 
> > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> > > Acked-by: Marcel Apfelbaum <marcel@redhat.com>
> > > ---
> > > v4:
> > >  * fix codding style of ACPI_DualNamePrefix & ACPI_MultiNamePrefix
> > >    it's imposible to use literals as suggested due to
> > >    g_array_append_val() requiring reference value
> > > 
> > > v3:
> > >  assert on wrong Segcount earlier and extend condition to
> > >   seg_count > 0 && seg_count <= 255
> > >  as suggested by Claudio Fontana <claudio.fontana@huawei.com>
> > > ---
> > >  hw/acpi/aml-build.c         | 92 ++++++++++++++++++++++++++++++++++++++++++++-
> > >  hw/i386/acpi-build.c        | 35 ++++++++---------
> > >  include/hw/acpi/aml-build.h |  2 +-
> > >  3 files changed, 108 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > index b28c1f4..ed4ab56 100644
> > > --- a/hw/acpi/aml-build.c
> > > +++ b/hw/acpi/aml-build.c
> > > @@ -52,7 +52,7 @@ void build_append_array(GArray *array, GArray *val)
> > >  
> > >  #define ACPI_NAMESEG_LEN 4
> > >  
> > > -void GCC_FMT_ATTR(2, 3)
> > > +static void GCC_FMT_ATTR(2, 3)
> > >  build_append_nameseg(GArray *array, const char *format, ...)
> > >  {
> > >      /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > > @@ -71,6 +71,96 @@ build_append_nameseg(GArray *array, const char *format, ...)
> > >      g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
> > >  }
> > >  
> > > +static void
> > > +build_append_namestringv(GArray *array, const char *format, va_list ap)
> > > +{
> > > +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > > +    char *s;
> > > +    int len;
> > > +    va_list va_len;
> > > +    char **segs;
> > > +    char **segs_iter;
> > > +    int seg_count = 0;
> > > +
> > > +    va_copy(va_len, ap);
> > > +    len = vsnprintf(NULL, 0, format, va_len);
> > > +    va_end(va_len);
> > > +    len += 1;
> > > +    s = g_new(typeof(*s), len);
> > > +
> > > +    len = vsnprintf(s, len, format, ap);
> > > +
> > > +    segs = g_strsplit(s, ".", 0);
> > > +    g_free(s);
> > > +
> > > +    /* count segments */
> > > +    segs_iter = segs;
> > > +    while (*segs_iter) {
> > > +        ++segs_iter;
> > > +        ++seg_count;
> > >
> > How about we split out formatting?
> > Make +build_append_namestringv acceptconst char **segs, int nsegs,
> > put all code above to build_append_namestring.
> ok
> 
> > 
> > > +    /*
> > > +     * ACPI 5.0 spec: 20.2.2 Name Objects Encoding:
> > > +     * "SegCount can be from 1 to 255"
> > > +     */
> > > +    assert(seg_count > 0 && seg_count <= 255);
> > > +
> > > +    /* handle RootPath || PrefixPath */
> > > +    s = *segs;
> > > +    while (*s == '\\' || *s == '^') {
> > > +        g_array_append_val(array, *s);
> > > +        ++s;
> > > +    }
> > > +
> > > +    switch (seg_count) {
> > > +    case 1:
> > > +        if (!*s) { /* NullName */
> > > +            const uint8_t nullpath = 0;
> > > +            g_array_append_val(array, nullpath);
> > > +        } else {
> > > +            build_append_nameseg(array, "%s", s);
> > > +        }
> > > +        break;
> > > +
> > > +    case 2: {
> > > +        const uint8_t prefix_opcode  = 0x2E; /* DualNamePrefix */
> > 
> > const is pretty bogus here.
> it's like in above block: const uint8_t nullpath = 0;

It's bogus there as well.

> > 
> > > +
> > > +        g_array_append_val(array, prefix_opcode);
> > > +        build_append_nameseg(array, "%s", s);
> > 
> > So nameseg only ever gets %s now?
> > In that case, move varg parsing out of there,
> > make it simply assept char*
> ok
> 
> > 
> > > +        build_append_nameseg(array, "%s", segs[1]);
> > > +        break;
> > > +    }
> > > +    default: {
> > > +        const uint8_t prefix_opcode = 0x2F; /* MultiNamePrefix */
> > 
> > And here.
> > 
> > > +        g_array_append_val(array, prefix_opcode);
> > > +        g_array_append_val(array, seg_count);
> > > +
> > > +        /* handle the 1st segment manually due to prefix/root path */
> > > +        build_append_nameseg(array, "%s", s);
> > > +
> > > +        /* add the rest of segments */
> > > +        segs_iter = segs + 1;
> > > +        while (*segs_iter) {
> > > +            build_append_nameseg(array, "%s", *segs_iter);
> > > +            ++segs_iter;
> > > +        }
> > > +        break;
> > > +    } /* default */
> > > +    } /* switch (seg_count) */
> > 
> > And the only reason for the extra {} is the local variable -
> > just declare it at top of function and assign here, then you
> > won't have these weird double }} things, and you won't need to
> > add comments after } which is really confusing IMO.
> > 
> > Or drop the variable:
> > 
> > 	g_array_append_val(array, 0x2F);  /* MultiNamePrefix */
> g_array_append_val() doesn't work with literals, hence variable.

OK but there's no need for extra {}, just add uint8_t opcode
at the top scope, and reuse.


> 
> > 
> > is just as clear.
> > 
> > 
> > > +    g_strfreev(segs);
> > > +}
> > > +
> > > +void GCC_FMT_ATTR(2, 3)
> > > +build_append_namestring(GArray *array, const char *format, ...)
> > > +{
> > > +    va_list ap;
> > > +
> > > +    va_start(ap, format);
> > > +    build_append_namestringv(array, format, ap);
> > > +    va_end(ap);
> > > +}
> > > +
> > >  /* 5.4 Definition Block Encoding */
> > >  enum {
> > >      PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index a92d008..380799b 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -282,7 +282,7 @@ static GArray *build_alloc_method(const char *name, uint8_t arg_count)
> > >  {
> > >      GArray *method = build_alloc_array();
> > >  
> > > -    build_append_nameseg(method, "%s", name);
> > > +    build_append_namestring(method, "%s", name);
> > >      build_append_byte(method, arg_count); /* MethodFlags: ArgCount */
> > >  
> > >      return method;
> > > @@ -574,7 +574,7 @@ build_append_notify_method(GArray *device, const char *name,
> > >  
> > >      for (i = 0; i < count; i++) {
> > >          GArray *target = build_alloc_array();
> > > -        build_append_nameseg(target, format, i);
> > > +        build_append_namestring(target, format, i);
> > >          assert(i < 256); /* Fits in 1 byte */
> > >          build_append_notify_target_ifequal(method, target, i, 1);
> > >          build_free_array(target);
> > > @@ -702,24 +702,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > >  
> > >      if (bus->parent_dev) {
> > >          op = 0x82; /* DeviceOp */
> > > -        build_append_nameseg(bus_table, "S%.02X",
> > > +        build_append_namestring(bus_table, "S%.02X",
> > >                               bus->parent_dev->devfn);
> > >          build_append_byte(bus_table, 0x08); /* NameOp */
> > > -        build_append_nameseg(bus_table, "_SUN");
> > > +        build_append_namestring(bus_table, "_SUN");
> > >          build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
> > >          build_append_byte(bus_table, 0x08); /* NameOp */
> > > -        build_append_nameseg(bus_table, "_ADR");
> > > +        build_append_namestring(bus_table, "_ADR");
> > >          build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
> > >                             PCI_FUNC(bus->parent_dev->devfn), 4);
> > >      } else {
> > >          op = 0x10; /* ScopeOp */;
> > > -        build_append_nameseg(bus_table, "PCI0");
> > > +        build_append_namestring(bus_table, "PCI0");
> > >      }
> > >  
> > >      bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
> > >      if (bsel) {
> > >          build_append_byte(bus_table, 0x08); /* NameOp */
> > > -        build_append_nameseg(bus_table, "BSEL");
> > > +        build_append_namestring(bus_table, "BSEL");
> > >          build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> > >          memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> > >      } else {
> > > @@ -822,7 +822,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > >              build_append_int(notify, 0x1U << i);
> > >              build_append_byte(notify, 0x00); /* NullName */
> > >              build_append_byte(notify, 0x86); /* NotifyOp */
> > > -            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
> > > +            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
> > >              build_append_byte(notify, 0x69); /* Arg1Op */
> > >  
> > >              /* Pack it up */
> > > @@ -846,12 +846,12 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > >          if (bsel) {
> > >              build_append_byte(method, 0x70); /* StoreOp */
> > >              build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
> > > -            build_append_nameseg(method, "BNUM");
> > > -            build_append_nameseg(method, "DVNT");
> > > -            build_append_nameseg(method, "PCIU");
> > > +            build_append_namestring(method, "BNUM");
> > > +            build_append_namestring(method, "DVNT");
> > > +            build_append_namestring(method, "PCIU");
> > >              build_append_int(method, 1); /* Device Check */
> > > -            build_append_nameseg(method, "DVNT");
> > > -            build_append_nameseg(method, "PCID");
> > > +            build_append_namestring(method, "DVNT");
> > > +            build_append_namestring(method, "PCID");
> > >              build_append_int(method, 3); /* Eject Request */
> > >          }
> > >  
> > > @@ -877,11 +877,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > >           * At the moment this is not needed for root as we have a single root.
> > >           */
> > >          if (bus->parent_dev) {
> > > -            build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
> > > -            build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
> > > -            build_append_nameseg(parent->notify_table, "S%.02X",
> > > +            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
> > >                                   bus->parent_dev->devfn);
> > 
> > Pls align continuation at opening (.
> ok
> 
> > 
> > > -            build_append_nameseg(parent->notify_table, "PCNT");
> > >          }
> > >      }
> > >  
> > > @@ -949,7 +946,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > >          GArray *sb_scope = build_alloc_array();
> > >          uint8_t op = 0x10; /* ScopeOp */
> > >  
> > > -        build_append_nameseg(sb_scope, "_SB");
> > > +        build_append_namestring(sb_scope, "_SB");
> > >  
> > >          /* build Processor object for each processor */
> > >          for (i = 0; i < acpi_cpus; i++) {
> > > @@ -969,7 +966,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > >  
> > >          /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
> > >          build_append_byte(sb_scope, 0x08); /* NameOp */
> > > -        build_append_nameseg(sb_scope, "CPON");
> > > +        build_append_namestring(sb_scope, "CPON");
> > >  
> > >          {
> > >              GArray *package = build_alloc_array();
> > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > > index e6a0b28..fd50625 100644
> > > --- a/include/hw/acpi/aml-build.h
> > > +++ b/include/hw/acpi/aml-build.h
> > > @@ -12,7 +12,7 @@ void build_append_byte(GArray *array, uint8_t val);
> > >  void build_append_array(GArray *array, GArray *val);
> > >  
> > >  void GCC_FMT_ATTR(2, 3)
> > > -build_append_nameseg(GArray *array, const char *format, ...);
> > > +build_append_namestring(GArray *array, const char *format, ...);
> > >  
> > >  void build_prepend_package_length(GArray *package, unsigned min_bytes);
> > >  void build_package(GArray *package, uint8_t op, unsigned min_bytes);
> > > -- 
> > > 1.8.3.1
> >