| Message ID | 1422455690-13950-4-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > 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. > + /* > + * 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. > + > + 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* > + 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 */ 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 (. > - 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
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; > > > + > > + 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. > > 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 >
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 > >
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 > > > 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. Can't do this, AML API calls which accept format string will use build_append_namestringv()
On Fri, Jan 30, 2015 at 12:46:41PM +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 > > > > > 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. > Can't do this, AML API calls which accept format string will > use build_append_namestringv() OK but I still think it's a good idea to split this out, and have a static function that accepts an array of segments.
On Sat, 31 Jan 2015 19:05:47 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Jan 30, 2015 at 12:46:41PM +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 > > > > > > > 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. > > Can't do this, AML API calls which accept format string will > > use build_append_namestringv() > > OK but I still think it's a good idea to split this out, > and have a static function that accepts an array of segments. It would be a static function with a single call site from another static function. I see no point is doing it, on contrary it would be confusing why do we have function if we do not reuse it. I'd split out the moment we actually need it. > >
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; + } + /* + * 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 */ + + g_array_append_val(array, prefix_opcode); + build_append_nameseg(array, "%s", s); + build_append_nameseg(array, "%s", segs[1]); + break; + } + default: { + const uint8_t prefix_opcode = 0x2F; /* MultiNamePrefix */ + + 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) */ + 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); - 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);