[SeaBIOS] Re: [PATCH 7/9] acpi: add dsdt parser

[Kevin O'Connor]

On Fri, Apr 03, 2020 at 10:31:19AM +0200, Gerd Hoffmann wrote:
> Create a list of devices found in the DSDT table.  Add helper functions
> to find devices, walk the list and figure device informations like mmio
> ranges and irqs.
> Signed-off-by: Gerd Hoffmann
>
> src/util.h          |  10 +
>  src/fw/biostables.c | 622 ++++++++++++++++++++++++++++++++++++++++++++
>  src/post.c          |   2 +
>  src/Kconfig         |   7 +
>  4 files changed, 641 insertions(+)
>
> diff --git a/src/util.h b/src/util.h
> index 4f27fc307439..8ee0370492b8 100644
> --- a/src/util.h
> +++ b/src/util.h
> @@ -94,6 +94,16 @@ void display_uuid(void);
>  void copy_table(void *pos);
>  void smbios_setup(void);
>
> +struct acpi_device;
> +void acpi_dsdt_parse(void);
> +struct acpi_device *acpi_dsdt_find_string(struct acpi_device *prev, const char *hid);
> +struct acpi_device *acpi_dsdt_find_eisaid(struct acpi_device *prev, u16 eisaid);
> +char *acpi_dsdt_name(struct acpi_device *dev);
> +int acpi_dsdt_present_eisaid(u16 eisaid);
> +int acpi_dsdt_find_io(struct acpi_device *dev, u64 *min, u64 *max);
> +int acpi_dsdt_find_mem(struct acpi_device *dev, u64 *min, u64 *max);
> +int acpi_dsdt_find_irq(struct acpi_device *dev, u64 *irq);
>
> // fw/coreboot.c
>  extern const char *CBvendor, *CBpart;
>  struct cbfs_file;
> diff --git a/src/fw/biostables.c b/src/fw/biostables.c
> index 0d4fdb9c22e8..ebe1c90fca5e 100644
> --- a/src/fw/biostables.c
> +++ b/src/fw/biostables.c
> @@ -17,6 +17,7 @@
>  #include "std/smbios.h" // struct smbios_entry_point
>  #include "string.h" // memcpy
>  #include "util.h" // copy_table
> +#include "list.h" // hlist_*
>  #include "x86.h" // outb
>
> struct pir_header *PirAddr VARFSEG;
> @@ -509,3 +510,624 @@ copy_table(void *pos)
>      copy_acpi_rsdp(pos);
>      copy_smbios(pos);
>  }
>
> +/****************************************************************
> DSDT parser
> ****************************************************************/

I think this code is sufficiently large to demand it's own C file -
for example src/fw/dsdt_parser.c .
> +struct acpi_device {
> struct hlist_node node;
> char name[16];
> u8 *hid_aml;
> u8 *sta_aml;
> u8 *crs_data;
> int crs_size;
>
> +};
> +static struct hlist_head acpi_devices;

It would be good to add VARVERIFY32INIT to this global.
> +static int parse_error = 0;
> +static int parse_dumptree = 0;
> +static char parse_name[32];
> +static struct acpi_device *parse_dev;

I think it would be preferable to not use global variables for
temporary state.  I think the above could be moved into a new "struct
dsdt_parsing_state" and passed between functions.  (I suspect the "u8
*ptr" could be moved into that struct as well.)
> +static void parse_termlist(u8 *ptr, int offset, int pkglength);

I'm a little concerned about the unbounded recursion in this parsing
code.  The main SeaBIOS execution stack is pretty large, but nothing
stops the dsdt table from doing something goofy.  I think a sanity
check on recursion depth may be worthwhile.
> +static void hex(const u8 *ptr, int count, int lvl, const char *item)
> +{
> int l = 0, i;
> do {
>    dprintf(lvl, "%s: %04x:  ", item, l);
>    for (i = l; i < l+16; i += 4)
>        dprintf(lvl, "%02x %02x %02x %02x  ",
>                ptr[i+0], ptr[i+1], ptr[i+2], ptr[i+3]);
>    for (i = l; i < l+16; i++)
>        dprintf(lvl, "%c", (ptr[i] > 0x20 && ptr[i] < 0x80) ? ptr[i] : '.');
>    dprintf(lvl, "\n");
>    l += 16;
> } while (l < count);
>
> +}
>
> +static u64 parse_resource_int(u8 *ptr, int count)
> +{
> u64 value = 0;
> int index = 0;
> for (index = 0; index < count; index++)
>    value |= (u64)ptr[index] << (index * 8);
> return value;
>
> +}
>
> +static int parse_resource_bit(u8 *ptr, int count)
> +{
> int bit;
> for (bit = 0; bit < count*8; bit++)
>    if (ptr[bit/8] & (1 << (bit%8)))
>        return bit;
> return 0;
>
> +}
>
> +static int parse_resource(u8 *ptr, int length, int *type, u64 *min, u64 *max)
> +{
> int rname, rsize;
> u64 len;
> *type = -1;
> *min = 0;
> *max = 0;
> len = 0;
> if (!(ptr[0] & 0x80)) {
>    /* small resource */
>    rname = (ptr[0] >> 3) & 0x0f;
>    rsize = ptr[0] & 0x07;
>    rsize++;
>    switch (rname) {
>    case 0x04: /* irq */
>        *min = parse_resource_bit(ptr + 1, rsize);
>        *max = *min;
>        *type = 3;
>        break;
>    case 0x0f: /* end marker */
>        return 0;
>    case 0x08: /* io */
>        *min = parse_resource_int(ptr + 2, 2);
>        *max = parse_resource_int(ptr + 4, 2);
>        if (*min == *max) {
>            *max = *min + ptr[7] - 1;
>            *type = 1;
>        }
>        break;
>    case 0x09: /* fixed io */
>        *min = parse_resource_int(ptr + 2, 2);
>        *max = *min + ptr[4] - 1;
>        *type = 1;
>        break;
>    default:
>        dprintf(3, "%s: small: 0x%x (len %d)\n",
>                __func__, rname, rsize);
>        break;
>    }
> } else {
>    /* large resource */
>    rname = ptr[0] & 0x7f;
>    rsize = ptr[2] << 8 | ptr[1];
>    rsize += 3;
>    switch (rname) {
>    case 0x06: /* 32-bit Fixed Location Memory Range Descriptor */
>        *min = parse_resource_int(ptr + 4, 4);
>        len = parse_resource_int(ptr + 8, 4);
>        *max = *min + len - 1;
>        *type = 0;
>        break;
>    case 0x07: /* DWORD Address Space Descriptor */
>        *min = parse_resource_int(ptr + 10, 4);
>        *max = parse_resource_int(ptr + 14, 4);
>        *type = ptr[3];
>        break;
>    case 0x08: /* WORD Address Space Descriptor */
>        *min = parse_resource_int(ptr +  8, 2);
>        *max = parse_resource_int(ptr + 10, 2);
>        *type = ptr[3];
>        break;
>    case 0x09: /* irq */
>        *min = parse_resource_int(ptr +  5, 4);
>        *max = *min;
>        *type = 3;
>        break;
>    case 0x0a: /* QWORD Address Space Descriptor */
>        *min = parse_resource_int(ptr + 14, 8);
>        *max = parse_resource_int(ptr + 22, 8);
>        *type = ptr[3];
>        break;
>    default:
>        dprintf(3, "%s: large: 0x%x (len %d)\n", __func__, rname, rsize);
>        break;
>    }
> }
> return rsize;
>
> +}
>
> +static int find_resource(u8 *ptr, int len, int kind, u64 *min, u64 *max)
> +{
> int type, size, offset = 0;
> do {
>    size = parse_resource(ptr + offset, len - offset,
>                          &type, min, max);
>    if (kind == type)
>        return 0;
>    offset += size;
> } while (size > 0 && offset < len);
> return -1;
>
> +}
>
> +static int print_resources(const char *prefix, u8 *ptr, int len)
> +{
> static const char *typename[] = { "mem", "i/o", "bus" };
> int type, size, offset = 0;
> u64 min, max;
> do {
>    size = parse_resource(ptr + offset, len - offset,
>                          &type, &min, &max);
>    switch (type) {
>    case 0:
>    case 1:
>    case 2:
>        dprintf(1, "%s%s 0x%llx -> 0x%llx\n",
>                prefix, typename[type], min, max);
>        break;
>    case 3:
>        dprintf(1, "%sirq %lld\n", prefix, min);
>        break;
>    }
>    offset += size;
> } while (size > 0 && offset < len);
> return -1;
>
> +}
>
> +static int parse_nameseg(u8 *ptr, char **dst)
> +{
> if (dst && *dst) {
>    *(dst[0]++) = ptr[0];
>    if (ptr[1] != '_')
>        *(dst[0]++) = ptr[1];
>    if (ptr[2] != '_')
>        *(dst[0]++) = ptr[2];
>    if (ptr[3] != '_')
>        *(dst[0]++) = ptr[3];
>    *(dst[0]) = 0;
> }
> return 4;
>
> +}
>
> +static int parse_namestring(u8 *ptr, const char *item)
> +{
> char *dst = parse_name;
> int offset = 0;
> int i, count;
>
> +again:
> switch (ptr[offset]) {
> case 0: /* null name */
>    offset++;
>    *(dst++) = 0;
>    break;
> case 0x2e:
>    offset++;
>    offset += parse_nameseg(ptr + offset, &dst);
>    *(dst++) = '.';
>    offset += parse_nameseg(ptr + offset, &dst);
>    break;
> case 0x2f:
>    offset++;
>    count = ptr[offset];
>    offset++;
>    for (i = 0; i < count; i++) {
>        if (i)
>            *(dst++) = '.';
>        offset += parse_nameseg(ptr + offset, &dst);
>    }
>    break;
> case '\':
>    *(dst++) = '\\';
>    offset++;
>    goto again;
> case '^':
>    *(dst++) = '^';
>    offset++;
>    goto again;

I think this code would be more clear if it used "for (;;) {" and
"continue" instead of a backwards goto.
> case 'A' ... 'Z':
> case '_':
>    offset += parse_nameseg(ptr, &dst);
>    break;
> default:
>    hex(ptr, 16, 3, __func__);
>    parse_error = 1;
>    break;
> }
> dprintf(5, "%s: %s '%s'\n", __func__, item, parse_name);
> return offset;
>
> +}
>
> +static int parse_termarg_int(u8 *ptr, u64 *dst)
> +{
> u64 value;
> int offset = 1;
> switch (ptr[0]) {
> case 0x00: /* zero */
>    value = 0;
>    break;
> case 0x01: /* one */
>    value = 1;
>    break;
> case 0x0a: /* byte prefix */
>    value = ptr[1];
>    offset++;
>    break;
> case 0x0b: /* word prefix */
>    value = ptr[1] |
>        ((unsigned long)ptr[2] << 8);
>    offset += 2;
>    break;
> case 0x0c: /* dword prefix */
>    value = ptr[1] |
>        ((unsigned long)ptr[2] << 8) |
>        ((unsigned long)ptr[3] << 16) |
>        ((unsigned long)ptr[4] << 24);
>    offset += 4;
>    break;
> default:
>    value = 0;
>    hex(ptr, 16, 3, __func__);
>    parse_error = 1;
>    break;
> }
> if (dst)
>    *dst = value;
> dprintf(5, "%s: 0x%llx\n", __func__, value);
> return offset;
>
> +}
>
> +static int parse_pkglength(u8 *ptr, int *pkglength)
> +{
> int offset = 2;
> *pkglength = 0;
> switch (ptr[0] >> 6) {
> case 3:
>    *pkglength |= ptr[3] << 20;
>    offset++;
> case 2:
>    *pkglength |= ptr[2] << 12;
>    offset++;
> case 1:
>    *pkglength |= ptr[1] << 4;
>    *pkglength |= ptr[0] & 0x0f;
>    return offset;
> case 0:
> default:
>    *pkglength |= ptr[0] & 0x3f;
>    return 1;
> }
>
> +}
>
> +static int parse_pkg_common(u8 *ptr, const char *item, int *pkglength)
> +{
> int offset;
> offset = parse_pkglength(ptr, pkglength);
> offset += parse_namestring(ptr + offset, item);
> return offset;
>
> +}
>
> +static int parse_pkg_scope(u8 *ptr)
> +{
> int offset, pkglength;
> offset = parse_pkg_common(ptr, "skope", &pkglength);

skope?
> parse_termlist(ptr, offset, pkglength);
> return pkglength;
>
> +}
>
> +static int parse_pkg_device(u8 *ptr)
> +{
> int offset, pkglength;
> offset = parse_pkg_common(ptr, "device", &pkglength);
> parse_dev = malloc_high(sizeof(*parse_dev));

Shouldn't this be malloc_tmp() ?
> if (!parse_dev) {
>    warn_noalloc();
>    parse_error = 1;
>    return pkglength;
> }
> memset(parse_dev, 0, sizeof(*parse_dev));
> hlist_add_head(&parse_dev->node, &acpi_devices);
> strtcpy(parse_dev->name, parse_name, sizeof(parse_dev->name));
> parse_termlist(ptr, offset, pkglength);
> return pkglength;
>
> +}
>
> +static int parse_pkg_buffer(u8 *ptr)
> +{
> u64 blen;
> int pkglength, offset;
> offset = parse_pkglength(ptr, &pkglength);
> offset += parse_termarg_int(ptr + offset, &blen);
> if (strcmp(parse_name, "_CRS") == 0) {
>    parse_dev->crs_data = ptr + offset;
>    parse_dev->crs_size = blen;
> }
> return pkglength;
>
> +}
>
> +static int parse_pkg_skip(u8 *ptr, int op, int name)
> +{
> int pkglength, offset;
> char item[8];
> snprintf(item, sizeof(item), "op %x", op);
> offset = parse_pkglength(ptr, &pkglength);
> if (name) {
>    parse_namestring(ptr + offset, item);
> } else {
>    dprintf(5, "%s: %s (%d)\n", __func__, item, pkglength);
> }
> return pkglength;
>
> +}
>
> +static int parse_termobj(u8 *ptr)
> +{
> int offset = 1;
> switch (ptr[0]) {
> case 0x00: /* zero */
>    break;
> case 0x01: /* one */
>    break;
> case 0x08: /* name op */
>    offset += parse_namestring(ptr + offset, "name");
>    offset += parse_termobj(ptr + offset);
>    if (strcmp(parse_name, "_HID") == 0)
>        parse_dev->hid_aml = ptr;
>    if (strcmp(parse_name, "_STA") == 0)
>        parse_dev->sta_aml = ptr;
>    break;
> case 0x0a: /* byte prefix */
>    offset++;
>    break;
> case 0x0b: /* word prefix */
>    offset += 2;
>    break;
> case 0x0c: /* dword prefix */
>    offset += 4;
>    break;
> case 0x0d: /* string prefix */
>    while (ptr[offset])
>        offset++;
>    offset++;
>    break;
> case 0x10: /* scope op */
>    offset += parse_pkg_scope(ptr + offset);
>    break;
> case 0x11: /* buffer op */
>    offset += parse_pkg_buffer(ptr + offset);
>    break;
> case 0x12: /* package op */
> case 0x13: /* var package op */
>    offset += parse_pkg_skip(ptr + offset, ptr[0], 0);
>    break;
> case 0x14: /* method op */
>    offset += parse_pkg_skip(ptr + offset, ptr[0], 1);
>    if (strcmp(parse_name, "_STA") == 0)
>        parse_dev->sta_aml = ptr;
>    break;
> case 0x5b: /* ext op prefix */
>    offset++;
>    switch (ptr[1]) {
>    case 0x01: /* mutex op */
>        offset += parse_namestring(ptr + offset, "mutex");
>        offset++; /* sync flags */
>        break;
>    case 0x80: /* op region op */
>        offset += parse_namestring(ptr + offset, "op region");
>        offset++; /* region space */
>        offset += parse_termarg_int(ptr + offset, NULL);
>        offset += parse_termarg_int(ptr + offset, NULL);
>        break;
>    case 0x81: /* field op */
>    case 0x83: /* processor op */
>    case 0x84: /* power resource op */
>    case 0x85: /* thermal zone op */
>        offset += parse_pkg_skip(ptr + offset, 0x5b00 | ptr[1], 1);
>        break;
>    case 0x82: /* device op */
>        offset += parse_pkg_device(ptr + offset);
>        break;
>    default:
>        hex(ptr, 16, 3, __func__);
>        parse_error = 1;
>        break;
>    }
>    break;
> default:
>    hex(ptr, 16, 3, __func__);
>    parse_error = 1;
>    break;
> }
> return offset;
>
> +}
>
> +static void parse_termlist(u8 *ptr, int offset, int pkglength)
> +{
> for (;;) {
>    offset += parse_termobj(ptr + offset);
>    if (offset == pkglength)
>        return;
>    if (offset > pkglength) {
>        dprintf(1, "%s: overrun: %d/%d\n", __func__,
>                offset, pkglength);
>        parse_error = 1;
>        return;
>    }
>    if (parse_error) {
>        dprintf(1, "%s: parse error, skip from %d/%d\n", __func__,
>                offset, pkglength);
>        parse_error = 0;
>        return;
>    }
> }
>
> +}
>
> +static struct acpi_device *acpi_dsdt_find(struct acpi_device *prev, const u8 *aml, int size)

This code should be wrapped to 80 characters.  (I know there's a bunch
of places where I goofed at this in the past, but I think going
forward we should try to keep to 80 characters.)
> +{
> struct acpi_device *dev;
> struct hlist_node *node;
> if (!prev)
>    node = acpi_devices.first;
> else
>    node = prev->node.next;
> for (; node != NULL; node = dev->node.next) {
>    dev = container_of(node, struct acpi_device, node);
>    if (!aml)
>        return dev;
>    if (!dev->hid_aml)
>        continue;
>    if (memcmp(dev->hid_aml + 5, aml, size) == 0)
>        return dev;
> }
> return NULL;
>
> +}
>
> +static int acpi_dsdt_present(struct acpi_device *dev)
> +{
> if (!dev)
>    return 0; /* no */
> if (!dev->sta_aml)
>    return 1; /* yes */
> if (dev->sta_aml[0] == 0x14)
>    return -1; /* unknown (can't evaluate method) */
> if (dev->sta_aml[0] == 0x08) {
>    u64 value = 0;
>    parse_termarg_int(dev->sta_aml + 5, &value);
>    if (value == 0)
>        return 0; /* no */
>    else
>        return 1; /* yes */
> }
> return -1; /* unknown (should not happen) */
>
> +}
>
> +/****************************************************************
> DSDT parser, public interface
> ****************************************************************/
>
> +struct acpi_device *acpi_dsdt_find_string(struct acpi_device *prev, const char *hid)
> +{
> if (!CONFIG_ACPI_PARSE)
>    return NULL;
> u8 aml[10];
> int len = snprintf((char*)aml, sizeof(aml), "\x0d%s", hid);
> return acpi_dsdt_find(prev, aml, len);
>
> +}
>
> +struct acpi_device *acpi_dsdt_find_eisaid(struct acpi_device *prev, u16 eisaid)
> +{
> if (!CONFIG_ACPI_PARSE)
>    return NULL;
> u8 aml[] = {
>    0x0c, 0x41, 0xd0,
>    eisaid >> 8,
>    eisaid & 0xff
> };
> return acpi_dsdt_find(prev, aml, 5);
>
> +}
>
> +char *acpi_dsdt_name(struct acpi_device *dev)
> +{
> if (!CONFIG_ACPI_PARSE || !dev)
>    return NULL;
> return dev->name;
>
> +}
>
> +int acpi_dsdt_find_io(struct acpi_device *dev, u64 *min, u64 *max)
> +{
> if (!CONFIG_ACPI_PARSE || !dev || !dev->crs_data)
>    return -1;
> return find_resource(dev->crs_data, dev->crs_size,
>                     1 /* I/O */, min, max);
>
> +}
>
> +int acpi_dsdt_find_mem(struct acpi_device *dev, u64 *min, u64 *max)
> +{
> if (!CONFIG_ACPI_PARSE || !dev || !dev->crs_data)
>    return -1;
> return find_resource(dev->crs_data, dev->crs_size,
>                     0 /* mem */, min, max);
>
> +}
>
> +int acpi_dsdt_find_irq(struct acpi_device *dev, u64 *irq)
> +{
> u64 max;
> if (!CONFIG_ACPI_PARSE || !dev || !dev->crs_data)
>    return -1;
> return find_resource(dev->crs_data, dev->crs_size,
>                     3 /* irq */, irq, &max);
>
> +}
>
> +int acpi_dsdt_present_eisaid(u16 eisaid)
> +{
> if (!CONFIG_ACPI_PARSE)
>    return -1; /* unknown */
> struct acpi_device *dev = acpi_dsdt_find_eisaid(NULL, eisaid);
> return acpi_dsdt_present(dev);
>
> +}
>
> +void acpi_dsdt_parse(void)
> +{
> if (!CONFIG_ACPI_PARSE)
>    return;
> struct fadt_descriptor_rev1 *fadt = find_acpi_table(FACP_SIGNATURE);
> if (!fadt)
>    return;
> u8 *dsdt = (void*)(fadt->dsdt);
> if (!dsdt)
>    return;
> u32 length = *(u32*)(dsdt + 4);
> u32 offset = 0x24;
> dprintf(1, "ACPI: parse DSDT at %p (len %d)\n", dsdt, length);
> parse_termlist(dsdt, offset, length);
> if (parse_dumptree) {
>    struct acpi_device *dev;
>    dprintf(1, "ACPI: dumping dsdt devices\n");
>    for (dev = acpi_dsdt_find(NULL, NULL, 0);
>         dev != NULL;
>         dev = acpi_dsdt_find(dev, NULL, 0)) {
>        dprintf(1, "    %s", acpi_dsdt_name(dev));
>        if (dev->hid_aml)
>            dprintf(1, ", hid");
>        if (dev->sta_aml)
>            dprintf(1, ", sta (0x%x)", dev->sta_aml[0]);
>        if (dev->crs_data)
>            dprintf(1, ", crs");
>        dprintf(1, "\n");
>        if (dev->crs_data)
>            print_resources("        ", dev->crs_data, dev->crs_size);
>    }
> }
>
> +}
> diff --git a/src/post.c b/src/post.c
> index f93106a1c9c9..febdc0859764 100644
> --- a/src/post.c
> +++ b/src/post.c
> @@ -149,6 +149,8 @@ platform_hardware_setup(void)
>      qemu_platform_setup();
>      coreboot_platform_setup();
> acpi_dsdt_parse();

Instead of adding this to post.c, could we add the call to
find_acpi_features()?  (And arrange for qemu_platform_setup() to call
find_acpi_features() or directly call acpi_dsdt_parse().)
> // Setup timers and periodic clock interrupt
>    timer_setup();
>    clock_setup();
>
> diff --git a/src/Kconfig b/src/Kconfig
> index 6606ce4d46c9..ab36e676bf12 100644
> --- a/src/Kconfig
> +++ b/src/Kconfig
> @@ -524,6 +524,13 @@ menu "BIOS Tables"
>              This option can be disabled for QEMU 1.6 and older
>              to save some space in the ROM file.
>              If unsure, say Y.
> config ACPI_PARSE
>    bool "Include ACPI DSDT parser."
>    default n
>    help
>        Support parsing ACPI DSDT for device probing.
>        Needed to find virtio-mmio devices.
>        If unsure, say N.

If we're going to add a dsdt parser then I think it should default to
enabled.

-Kevin
> endmenu
> source vgasrc/Kconfig
>
> 2.18.2
> _______________________________________________
> SeaBIOS mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]