Hi,
+/****************************************************************
- DSDT parser
- ****************************************************************/
I think this code is sufficiently large to demand it's own C file - for example src/fw/dsdt_parser.c .
Done.
+static struct hlist_head acpi_devices;
It would be good to add VARVERIFY32INIT to this global.
Done.
+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.
Done.
(I suspect the "u8 *ptr" could be moved into that struct as well.)
Not that easy with the current position/offset tracking, specifically the parse-something(ptr + offset, ...); calls are tricky.
+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.
Added.
+again:
- switch (ptr[offset]) {
- case 0: /* null name */
offset++;*(dst++) = 0;break;
[ ... ]
- case '^':
*(dst++) = '^';offset++;goto again;I think this code would be more clear if it used "for (;;) {" and "continue" instead of a backwards goto.
Hmm, doesn't help that much due to for + switch nesting. I would need either an additional state variable or use goto to jump from inside switch out of the for loop. Both ways don't make things more clear compared to the current state ...
+static int parse_pkg_scope(u8 *ptr) +{
- int offset, pkglength;
- offset = parse_pkg_common(ptr, "skope", &pkglength);
skope?
Tyops. Fixed.
+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() ?
Yes, device list is not needed any more after post.
+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.)
Done.
- 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().)
Done (calling acpi_dsdt_parse directly, with qemu we don't need to search the tables for pm_timer).
- config ACPI_PARSE
bool "Include ACPI DSDT parser."default nhelpSupport 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.
Done.
New versions should come later today.
take care, Gerd