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

[Marcel Apfelbaum <marcel@redhat.com>]

On 01/21/2015 11:09 AM, 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 <imammedo@redhat.com>
> Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> ---
> 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/acpi-build-utils.c         | 90 +++++++++++++++++++++++++++++++++++++-
>   hw/i386/acpi-build.c               | 35 +++++++--------
>   include/hw/acpi/acpi-build-utils.h |  2 +-
>   3 files changed, 106 insertions(+), 21 deletions(-)
>
> diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> index 79aa610..aed9066 100644
> --- a/hw/acpi/acpi-build-utils.c
> +++ b/hw/acpi/acpi-build-utils.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,94 @@ build_append_nameseg(GArray *array, const char *format, ...)
>       g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
>   }
>
> +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;
> +    }
> +    /*
> +     * 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:
> +        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);
> +        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 8ee15ab..e3ac1a14 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -283,7 +283,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;
> @@ -575,7 +575,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);
> @@ -703,24 +703,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 {
> @@ -823,7 +823,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 */
> @@ -847,12 +847,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 */
>           }
>
> @@ -878,11 +878,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");
>           }
>       }
>
> @@ -967,7 +964,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++) {
> @@ -987,7 +984,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-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> index e6a0b28..fd50625 100644
> --- a/include/hw/acpi/acpi-build-utils.h
> +++ b/include/hw/acpi/acpi-build-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);
>

Acked-by: Marcel Apfelbaum <marcel@redhat.com>