Page 1 of 1
omarrx024: acpi_scan()
Posted: Fri Apr 06, 2018 7:40 pm
by 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
Re: omarrx024: acpi_scan()
Posted: Sat Apr 07, 2018 5:30 am
by 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.
BenLunt wrote: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.
BenLunt wrote: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.
Re: omarrx024: acpi_scan()
Posted: Sat Apr 07, 2018 9:55 am
by BenLunt
omarrx024 wrote:
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
is not necessarily legal code. A compiler might and usually will bark at an empty array.
To fix it, you can do
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
Re: omarrx024: acpi_scan()
Posted: Sat Apr 07, 2018 10:14 am
by 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.
Re: omarrx024: acpi_scan()
Posted: Sat Apr 07, 2018 3:38 pm
by hexcoder
Korona wrote: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.
Re: omarrx024: acpi_scan()
Posted: Sat Apr 07, 2018 4:12 pm
by 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
Re: omarrx024: acpi_scan()
Posted: Sat Apr 07, 2018 4:38 pm
by BrightLight
BenLunt wrote: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:
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.
BenLunt wrote: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.
Re: omarrx024: acpi_scan()
Posted: Sat Apr 07, 2018 4:42 pm
by 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
Re: omarrx024: acpi_scan()
Posted: Sun Apr 08, 2018 4:50 am
by BrightLight
BenLunt wrote: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?
Re: omarrx024: acpi_scan()
Posted: Sun Apr 08, 2018 1:30 pm
by BenLunt
omarrx024 wrote:BenLunt wrote: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
Re: omarrx024: acpi_scan()
Posted: Sun Apr 08, 2018 7:50 pm
by 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
Re: omarrx024: acpi_scan()
Posted: Mon Apr 09, 2018 4:02 am
by BrightLight
BenLunt wrote: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.