On Tue, Dec 09, 2014 at 11:39:39AM +0100, Igor Mammedov wrote:
> On Mon, 8 Dec 2014 22:21:20 +0200
> "Michael S. Tsirkin" <m...@redhat.com> wrote:
>
> > On Mon, Dec 08, 2014 at 04:08:05PM +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
> > >
> > > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> >
> > Well that's wasteful since most places do have simple 4 byte names.
> What do you mean under wasteful?
I think I misunderstood what this patch does:
you mean "select the correct name type based on
the string used"?
I'll have to re-read it, sorry about the noise.
> > How about we replace where this is needed?
> >
> >
> > > ---
> > > hw/acpi/acpi_gen_utils.c | 86
> > > +++++++++++++++++++++++++++++++++++++++-
> > > hw/i386/acpi-build.c | 35 ++++++++--------
> > > include/hw/acpi/acpi_gen_utils.h | 2 +-
> > > 3 files changed, 102 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> > > index 1583b35..31e2694 100644
> > > --- a/hw/acpi/acpi_gen_utils.c
> > > +++ b/hw/acpi/acpi_gen_utils.c
> > > @@ -31,7 +31,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 */
> > > @@ -53,6 +53,90 @@ build_append_nameseg(GArray *array, const char
> > > *format, ...)
> > > }
> > > }
> > >
> > > +static const uint8_t ACPI_DualNamePrefix = 0x2E;
> > > +static const uint8_t ACPI_MultiNamePrefix = 0x2F;
> > > +
> > > +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;
> > > + }
> > > +
> > > + /* 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:
> > > + g_array_append_val(array, ACPI_DualNamePrefix);
> > > + build_append_nameseg(array, "%s", s);
> > > + build_append_nameseg(array, "%s", segs[1]);
> > > + break;
> > > +
> > > + default:
> > > + g_array_append_val(array, ACPI_MultiNamePrefix);
> > > + assert(seg_count <= 0xFF); /* ByteData Max */
> > > + 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;
> > > + }
> > > + 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 870e4a0..0f6202d 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -273,7 +273,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;
> > > @@ -565,7 +565,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);
> > > @@ -693,24 +693,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 {
> > > @@ -813,7 +813,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 */
> > > @@ -837,12 +837,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 */
> > > }
> > >
> > > @@ -868,11 +868,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);
> > > - build_append_nameseg(parent->notify_table, "PCNT");
> > > }
> > > }
> > >
> > > @@ -940,7 +937,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++) {
> > > @@ -960,7 +957,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/acpi_gen_utils.h
> > > b/include/hw/acpi/acpi_gen_utils.h
> > > index e6a0b28..fd50625 100644
> > > --- a/include/hw/acpi/acpi_gen_utils.h
> > > +++ b/include/hw/acpi/acpi_gen_utils.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
