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 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.
Done.
New versions should come later today.
take care,
Gerd
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org
