omarrx024: acpi_scan()

[BenLunt]

Hi omarrx024,

I have been looking over your ACPI code at https://github.com/omarrx024/lai/tree/master/src and https://omarrx024.github.io/docs/lai.html. The second URL has information about what the OS is to provide, namely, "acpi_scan()".

It looks like this function is suppose to return a "structure" filled with data such as:

Code: Select all

 typedef struct acpi_aml_t { // AML tables, DSDT and SSDT
  acpi_header_t header;
  bit8u *data;
} acpi_aml_t;

If so, does your code assume this is a memory location already filled with the header of the desired table as well as a memory buffer pointed to by 'data' holding the data of the table?

This looks funny to me.

For example, if this is the case, the "acpi_scan()" function will only be called for the DSDT and SSDT tables. Then it is suppose to return the AML code at these two tables. Will the "acpi_scan()" function ever be called for something else?

Also, it doesn't look like your code ever frees the memory used by both the acpi_aml_t structure, nor the data pointed to by "data". Is this the responsibility of the caller?

I guess I am just wondering why your definition of "acpi_scan()" says the the signature can be any ACPI table, though your code assumes it will return a type of acpi_aml_t which is only beneficial to the DSDT and SSDT tables.

Would you please explain a little more?

Thank you,
Ben

[BrightLight]

acpi_scan() is defined in kernel/acpi/tables.c, and it works with any table that has a pointer within the RSDT. So it works with SSDTs but not with the DSDT, which is why I also clarified in my document that you need to pass a pointer to the DSDT as an argument when initializing the namespace. This function returns a void pointer and not a pointer to an ACPI AML table, because not all ACPI tables are AML-encoded.

When it does return an AML table (i.e. when scanning for an SSDT,) the data can be used with this structure:

Code: Select all

typedef struct acpi_aml_t		// AML tables, DSDT and SSDT
{
	acpi_header_t header;
	uint8_t data[];
}__attribute__((packed)) acpi_aml_t;

Notice that "data" is an array, not a pointer, because the AML tables don't have pointers.

Will the "acpi_scan()" function ever be called for something else?

From within the AML interpreter itself, no. It will only be used to find AML-encoded tables, but the remaining of the non-AML ACPI code (SMP/APIC, HPET, etc) needs this function still, which is why it returns a void pointer. That code, of course, is inside the kernel and not in the AML interpreter.

Also, it doesn't look like your code ever frees the memory used by both the acpi_aml_t structure, nor the data pointed to by "data". Is this the responsibility of the caller?

Data is not a pointer, and acpi_aml_t doesn't allocate memory, it simply maps the ACPI tables into the virtual address space, so this doesn't waste much space. Anyway there are a few memory leaks I am aware of, and they will be fixed before a stable release.

[BenLunt]

When it does return an AML table (i.e. when scanning for an SSDT,) the data can be used with this structure:

Code: Select all

typedef struct acpi_aml_t		// AML tables, DSDT and SSDT
{
	acpi_header_t header;
	uint8_t data[];
}__attribute__((packed)) acpi_aml_t;

Notice that "data" is an array, not a pointer, because the AML tables don't have pointers.

Oh, I see. You are returning a pointer with that structure of data that will always start with a header, and then data[] is defined as the remainder of the table. It just so happens that in some tables, data is actually the AML data.

What got me is that

Code: Select all

	uint8_t data[];

is not necessarily legal code. A compiler might and usually will bark at an empty array.

To fix it, you can do

Code: Select all

	uint8_t data[1];

I saw a few more things that you might wish to investigate.
1) You return 0 for TRUE and 1 for FALSE in a lot of cases. This is actually completely opposite of the C standard, though it really doesn't matter. It is just preference. However, most will see a zero as a FALSE return and a non-zero (1) as a TRUE return.
2) You use pointer math on void pointers. This is not legal. A compiler will bark at a void pointer math. For example,

Code: Select all

  void *foo = malloc(1234);
  void *bar = foo + some_len;

should return an error, or at least a warning.

Anyway, the code looks good, well written, and clean. Good job.
Ben

[Korona]

The undefined-size array is a GNU extension to C and has the advantage that you don't need to do pointer arithmetic and that the size of the header is correct. Unless you want to compile with something other than GCC and clang, I would always prefer it to arrays of size 1.

[hexcoder]

The undefined-size array is a GNU extension to C and has the advantage that you don't need to do pointer arithmetic and that the size of the header is correct. Unless you want to compile with something other than GCC and clang, I would always prefer it to arrays of size 1.

Flexible array members were introduced in C99; they aren't GNU extensions.

[BenLunt]

omarrx024,

Just for your interest, here is an AML dump of one of my laptops.

http://www.fysnet.net/temp/aml_small_dell.txt

Hopefully you can easily convert it to a file or something and run it through your code. It is ACPI 2.0+ where I think yours is v1.0 only.

Just thought you might be interested, to to test your code or what not.

Thanks,
Ben

[BrightLight]

Hopefully you can easily convert it to a file or something and run it through your code.

What did you use to create this dump? The easiest way for what I'm doing is run this:

Code: Select all

acpidump > acpi.dat

And then attaching that dump file, because it can then be separated into different files with acpixtract, and disassembled with iasl. The package that contains these commands on Ubuntu/Debian at least is called acpica-tools.

It is ACPI 2.0+ where I think yours is v1.0 only.

ACPI AML tables have only ever had two versions, v1.0 and v2.0, and my AML interpreter is aware of ACPI 2.0 AML opcodes, which include 64-bit integer math.

[BenLunt]

I just dumped it from my OS, from a buffer I have the AML code stored in.

I only have so many machines to test mine with, and always like to have another "machine" to test on, so I thought you might like the same.

Ben

[BrightLight]

I just dumped it from my OS, from a buffer I have the AML code stored in.

I only have so many machines to test mine with, and always like to have another "machine" to test on, so I thought you might like the same.

Yeah, thanks.
I've written a small tool to give out the actual ACPI tables from your dumps, so that I can disassemble them into ASL. If you don't mind, do you have any other AML dumps from other PCs?

[BenLunt]

I just dumped it from my OS, from a buffer I have the AML code stored in.

I only have so many machines to test mine with, and always like to have another "machine" to test on, so I thought you might like the same.

Yeah, thanks.
I've written a small tool to give out the actual ACPI tables from your dumps, so that I can disassemble them into ASL. If you don't mind, do you have any other AML dumps from other PCs?

How about two more for now:

http://www.fysnet.net/temp/aml_1450_dell.txt (v1.0)
http://www.fysnet.net/temp/aml_1720_dell.txt (v2.0+)

Ben

[BenLunt]

P.S. I kept them as ascii dumps since you have gone to the time to create the little utility to convert them. However, if you prefer, I can send them as binary files too.

Anyway, if you don't mind, can you send me your AML file? Post it somewhere, here, or send it to me via email. (email address at the bottom of http://www.fysnet.net/osdesign_book_series.htm)

Thanks,
Ben

[BrightLight]

Anyway, if you don't mind, can you send me your AML file?

Sure, here's the DSDT and 6 SSDTs from my laptop, both AML binary and ASL disassembly.