[SeaBIOS] Re: [PATCH 7/9] acpi: add dsdt parser
On Wed, Apr 08, 2020 at 02:39:39PM +0200, Gerd Hoffmann wrote:
> > > +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_namestring(struct parse_state *s,
u8 *ptr, const char *item)
{
char *dst = s->name;
int offset = 0;
int i, count;
for (;;) {
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++;
continue;
case '^':
*(dst++) = '^';
offset++;
continue;
case 'A' ... 'Z':
case '_':
offset += parse_nameseg(ptr, &dst);
break;
default:
hex(ptr, 16, 3, __func__);
s->error = 1;
break;
}
break;
}
dprintf(5, "%s: %d %s '%s'\n", __func__, s->depth,
item, s->name);
return offset;
}
-Kevin
___
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org
[SeaBIOS] Re: [PATCH 7/9] acpi: add dsdt parser
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
[SeaBIOS] Re: [PATCH 7/9] acpi: add dsdt parser
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:
