Hi Ricardo,
Thanks for the review!
On 18/06/2022 01:38, Ricardo Koller wrote:
Hi Nikos,
On Fri, May 06, 2022 at 09:55:45PM +0100, Nikos Nikoleris wrote:
XSDT provides pointers to other ACPI tables much like RSDT. However,
contrary to RSDT that provides 32-bit addresses, XSDT provides 64-bit
pointers. ACPI requires that if XSDT is valid then it takes precedence
over RSDT.
Signed-off-by: Nikos Nikoleris <nikos.nikoleris@xxxxxxx>
---
lib/acpi.h | 6 ++++
lib/acpi.c | 103 ++++++++++++++++++++++++++++++++---------------------
2 files changed, 68 insertions(+), 41 deletions(-)
diff --git a/lib/acpi.h b/lib/acpi.h
index 42a2c16..d80b983 100644
--- a/lib/acpi.h
+++ b/lib/acpi.h
@@ -13,6 +13,7 @@
#define RSDP_SIGNATURE ACPI_SIGNATURE('R','S','D','P')
#define RSDT_SIGNATURE ACPI_SIGNATURE('R','S','D','T')
+#define XSDT_SIGNATURE ACPI_SIGNATURE('X','S','D','T')
#define FACP_SIGNATURE ACPI_SIGNATURE('F','A','C','P')
#define FACS_SIGNATURE ACPI_SIGNATURE('F','A','C','S')
@@ -56,6 +57,11 @@ struct rsdt_descriptor_rev1 {
u32 table_offset_entry[0];
} __attribute__ ((packed));
+struct acpi_table_xsdt {
+ ACPI_TABLE_HEADER_DEF
+ u64 table_offset_entry[1];
nit: This should be "[0]" to match the usage above (in rsdt).
I was about to suggest using an unspecified size "[]", but after reading
what the C standard says about it (below), now I'm not sure. was the
"[1]" needed for something that I'm missing?
106) The length is unspecified to allow for the fact that
implementations may give array members different
alignments according to their lengths.
+} __attribute__ ((packed));
+
struct fadt_descriptor_rev1
{
ACPI_TABLE_HEADER_DEF /* ACPI common table header */
diff --git a/lib/acpi.c b/lib/acpi.c
index de275ca..9b8700c 100644
--- a/lib/acpi.c
+++ b/lib/acpi.c
@@ -38,45 +38,66 @@ static struct rsdp_descriptor *get_rsdp(void)
void* find_acpi_table_addr(u32 sig)
nit: This one could also be fixed as well: "void *".
{
- struct rsdp_descriptor *rsdp;
- struct rsdt_descriptor_rev1 *rsdt;
- void *end;
- int i;
-
- /* FACS is special... */
- if (sig == FACS_SIGNATURE) {
- struct fadt_descriptor_rev1 *fadt;
- fadt = find_acpi_table_addr(FACP_SIGNATURE);
- if (!fadt) {
- return NULL;
- }
- return (void*)(ulong)fadt->firmware_ctrl;
- }
-
- rsdp = get_rsdp();
- if (rsdp == NULL) {
- printf("Can't find RSDP\n");
- return 0;
- }
-
- if (sig == RSDP_SIGNATURE) {
- return rsdp;
- }
-
- rsdt = (void*)(ulong)rsdp->rsdt_physical_address;
- if (!rsdt || rsdt->signature != RSDT_SIGNATURE)
- return 0;
-
- if (sig == RSDT_SIGNATURE) {
- return rsdt;
- }
-
- end = (void*)rsdt + rsdt->length;
- for (i=0; (void*)&rsdt->table_offset_entry[i] < end; i++) {
- struct acpi_table *t = (void*)(ulong)rsdt->table_offset_entry[i];
- if (t && t->signature == sig) {
- return t;
- }
- }
- return NULL;
+ struct rsdp_descriptor *rsdp;
+ struct rsdt_descriptor_rev1 *rsdt;
+ struct acpi_table_xsdt *xsdt = NULL;
+ void *end;
+ int i;
+
+ /* FACS is special... */
+ if (sig == FACS_SIGNATURE) {
+ struct fadt_descriptor_rev1 *fadt;
+
+ fadt = find_acpi_table_addr(FACP_SIGNATURE);
+ if (!fadt)
+ return NULL;
+
+ return (void*)(ulong)fadt->firmware_ctrl;
+ }
+
+ rsdp = get_rsdp();
+ if (rsdp == NULL) {
+ printf("Can't find RSDP\n");
+ return 0;
+ }
+
+ if (sig == RSDP_SIGNATURE)
+ return rsdp;
+
+ rsdt = (void *)(ulong)rsdp->rsdt_physical_address;
+ if (!rsdt || rsdt->signature != RSDT_SIGNATURE)
+ rsdt = NULL;
+
+ if (sig == RSDT_SIGNATURE)
+ return rsdt;
+
+ if (rsdp->revision > 1)
+ xsdt = (void *)(ulong)rsdp->xsdt_physical_address;
+ if (!xsdt || xsdt->signature != XSDT_SIGNATURE)
+ xsdt = NULL;
+
To simplify this function a bit, finding the xsdt could be moved to some
kind of init function.
That's a good point, I can add xsdt and rsdt as globals (like we do for
efi_rsdp) and then initialise them in set_efi_rsdp().
On the other hand, if we would like to avoid the global variables there
is only a handful of time we will be calling find_acpi_table_addr()
+ if (sig == XSDT_SIGNATURE)
+ return xsdt;
+
+ // APCI requires that we first try to use XSDT if it's valid,
+ // we use to find other tables, otherwise we use RSDT.
+ if (xsdt) {
+ end = (void *)(ulong)xsdt + xsdt->length;
+ for (i = 0; (void *)&xsdt->table_offset_entry[i] < end; i++) {
+ struct acpi_table *t =
+ (void *)xsdt->table_offset_entry[i];
+ if (t && t->signature == sig)
+ return t;
+ }
+ } else if (rsdt) {
+ end = (void *)rsdt + rsdt->length;
+ for (i = 0; (void *)&rsdt->table_offset_entry[i] < end; i++) {
+ struct acpi_table *t =
+ (void *)(ulong)rsdt->table_offset_entry[i];
+ if (t && t->signature == sig)
+ return t;
+ }
+ }
The two for-loops could be moved into some common function, or maybe a
macro to deal with the fact that it deals with two different structures
(the rsdt and the xsdt). This is my attempt at making it a function:
void *scan_system_descriptor_table(void *dt)
{
int i;
void *end;
/* XXX: Not sure if this is the nicest thing to do, but the rsdt
* and the xsdt have "length" and "table_offset_entry" at the
* same offsets. */
struct acpi_table_xsdt *xsdt = dt;
end = (void *)(ulong)xsdt + xsdt->length;
for (i = 0; &xsdt->table_offset_entry[i] < end; i++) {
struct acpi_table *t = (void *)xsdt->table_offset_entry[i];
if (t && t->signature == sig)
return t;
}
I remember trying something similar but it got a bit messy because
table_offset_entry is u32 in rsdt and u64 in xsdt. I'll check again if
there is a way we can improve this in v3.
Thanks,
Nikos
Thanks,
Ricardo
+
+ return NULL;
}
--
2.25.1
