| Message ID | 20240911062511.494855-14-patrick.rudolph@9elements.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Implement ACPI on aarch64 | expand |
> On platforms that do not have usable DRAM below 4GiB, like QEMU sbsa, > the RSDT cannot be used. Allow both RSDT and XSDT to be null and only > fill those tables that are present in acpi_add_table(). I'm not sure what ACPI 2.0 from the subject has to do with the above, eg ACPI only started supporting aarch64 with ACPI 5.1 > Fixes a crash on QEMU sbsa. > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Tom Rini <trini@konsulko.com> > --- > lib/acpi/acpi_table.c | 95 ++++++++++++++++++++++++++----------------- > 1 file changed, 57 insertions(+), 38 deletions(-) > > diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c > index 4f5cfe522c..8aab41212a 100644 > --- a/lib/acpi/acpi_table.c > +++ b/lib/acpi/acpi_table.c > @@ -157,51 +157,70 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table) > struct acpi_rsdt *rsdt; > struct acpi_xsdt *xsdt; > > - /* The RSDT is mandatory while the XSDT is not */ > - rsdt = ctx->rsdt; > - > - /* This should always be MAX_ACPI_TABLES */ > - entries_num = ARRAY_SIZE(rsdt->entry); > - > - for (i = 0; i < entries_num; i++) { > - if (rsdt->entry[i] == 0) > - break; > - } > - > - if (i >= entries_num) { > - log_err("ACPI: Error: too many tables\n"); > - return -E2BIG; > - } > + /* On legacy x86 platforms the RSDT is mandatory while the XSDT is not. > + * On other platforms there might be no memory below 4GiB, thus RSDT is NULL. > + */ > + if (ctx->rsdt) { > + rsdt = ctx->rsdt; > > - /* Add table to the RSDT */ > - rsdt->entry[i] = nomap_to_sysmem(table); > + /* This should always be MAX_ACPI_TABLES */ > + entries_num = ARRAY_SIZE(rsdt->entry); > > - /* Fix RSDT length or the kernel will assume invalid entries */ > - rsdt->header.length = sizeof(struct acpi_table_header) + > - (sizeof(u32) * (i + 1)); > + for (i = 0; i < entries_num; i++) { > + if (rsdt->entry[i] == 0) > + break; > + } > > - /* Re-calculate checksum */ > - rsdt->header.checksum = 0; > - rsdt->header.checksum = table_compute_checksum((u8 *)rsdt, > - rsdt->header.length); > + if (i >= entries_num) { > + log_err("ACPI: Error: too many tables\n"); > + return -E2BIG; > + } > > - /* > - * And now the same thing for the XSDT. We use the same index as for > - * now we want the XSDT and RSDT to always be in sync in U-Boot > - */ > - xsdt = ctx->xsdt; > + /* Add table to the RSDT */ > + rsdt->entry[i] = nomap_to_sysmem(table); > > - /* Add table to the XSDT */ > - xsdt->entry[i] = nomap_to_sysmem(table); > + /* Fix RSDT length or the kernel will assume invalid entries */ > + rsdt->header.length = sizeof(struct acpi_table_header) + > + (sizeof(u32) * (i + 1)); > > - /* Fix XSDT length */ > - xsdt->header.length = sizeof(struct acpi_table_header) + > - (sizeof(u64) * (i + 1)); > + /* Re-calculate checksum */ > + rsdt->header.checksum = 0; > + rsdt->header.checksum = table_compute_checksum((u8 *)rsdt, > + rsdt->header.length); > + } > > - /* Re-calculate checksum */ > - xsdt->header.checksum = 0; > - xsdt->header.checksum = table_compute_checksum((u8 *)xsdt, > - xsdt->header.length); > + if (ctx->xsdt) { > + /* > + * And now the same thing for the XSDT. We use the same index as for > + * now we want the XSDT and RSDT to always be in sync in U-Boot > + */ > + xsdt = ctx->xsdt; > + > + /* This should always be MAX_ACPI_TABLES */ > + entries_num = ARRAY_SIZE(xsdt->entry); > + > + for (i = 0; i < entries_num; i++) { > + if (xsdt->entry[i] == 0) > + break; > + } > + > + if (i >= entries_num) { > + log_err("ACPI: Error: too many tables\n"); > + return -E2BIG; > + } > + > + /* Add table to the XSDT */ > + xsdt->entry[i] = nomap_to_sysmem(table); > + > + /* Fix XSDT length */ > + xsdt->header.length = sizeof(struct acpi_table_header) + > + (sizeof(u64) * (i + 1)); > + > + /* Re-calculate checksum */ > + xsdt->header.checksum = 0; > + xsdt->header.checksum = table_compute_checksum((u8 *)xsdt, > + xsdt->header.length); > + } > > return 0; > } > -- > 2.46.0 >
Hi Patrick, On Wed, 11 Sept 2024 at 03:46, Peter Robinson <pbrobinson@gmail.com> wrote: > > > On platforms that do not have usable DRAM below 4GiB, like QEMU sbsa, > > the RSDT cannot be used. Allow both RSDT and XSDT to be null and only > > fill those tables that are present in acpi_add_table(). > > I'm not sure what ACPI 2.0 from the subject has to do with the above, > eg ACPI only started supporting aarch64 with ACPI 5.1 Should perhaps be something like 'Support platforms without an RSDR'? > > > Fixes a crash on QEMU sbsa. > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Tom Rini <trini@konsulko.com> > > --- > > lib/acpi/acpi_table.c | 95 ++++++++++++++++++++++++++----------------- > > 1 file changed, 57 insertions(+), 38 deletions(-) > > Reviewed-by: Simon Glass <sjg@chromium.org> I was trying to see if this patch could be split into something which refactors the code and then another patch to fix the bug, but I can't see it. Regards, Simon
On Wed, Sep 11, 2024 at 11:46 AM Peter Robinson <pbrobinson@gmail.com> wrote: > > > On platforms that do not have usable DRAM below 4GiB, like QEMU sbsa, > > the RSDT cannot be used. Allow both RSDT and XSDT to be null and only > > fill those tables that are present in acpi_add_table(). > > I'm not sure what ACPI 2.0 from the subject has to do with the above, > eg ACPI only started supporting aarch64 with ACPI 5.1 > The change is unrelated to the architecture. ACPI 2.0 made XSDT mandatory and RSDT optional. Since x86 is backwards compatible and always had memory below 4GiB, tables would always fit into RSDT and XSDT, but on platforms where tables are stored above 4GiB the address would not fit into RSDT, only in XSDT. I'll update the commit message to better explain why this change was done. > > Fixes a crash on QEMU sbsa. > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Tom Rini <trini@konsulko.com> > > --- > > lib/acpi/acpi_table.c | 95 ++++++++++++++++++++++++++----------------- > > 1 file changed, 57 insertions(+), 38 deletions(-) > > > > diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c > > index 4f5cfe522c..8aab41212a 100644 > > --- a/lib/acpi/acpi_table.c > > +++ b/lib/acpi/acpi_table.c > > @@ -157,51 +157,70 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table) > > struct acpi_rsdt *rsdt; > > struct acpi_xsdt *xsdt; > > > > - /* The RSDT is mandatory while the XSDT is not */ > > - rsdt = ctx->rsdt; > > - > > - /* This should always be MAX_ACPI_TABLES */ > > - entries_num = ARRAY_SIZE(rsdt->entry); > > - > > - for (i = 0; i < entries_num; i++) { > > - if (rsdt->entry[i] == 0) > > - break; > > - } > > - > > - if (i >= entries_num) { > > - log_err("ACPI: Error: too many tables\n"); > > - return -E2BIG; > > - } > > + /* On legacy x86 platforms the RSDT is mandatory while the XSDT is not. > > + * On other platforms there might be no memory below 4GiB, thus RSDT is NULL. > > + */ > > + if (ctx->rsdt) { > > + rsdt = ctx->rsdt; > > > > - /* Add table to the RSDT */ > > - rsdt->entry[i] = nomap_to_sysmem(table); > > + /* This should always be MAX_ACPI_TABLES */ > > + entries_num = ARRAY_SIZE(rsdt->entry); > > > > - /* Fix RSDT length or the kernel will assume invalid entries */ > > - rsdt->header.length = sizeof(struct acpi_table_header) + > > - (sizeof(u32) * (i + 1)); > > + for (i = 0; i < entries_num; i++) { > > + if (rsdt->entry[i] == 0) > > + break; > > + } > > > > - /* Re-calculate checksum */ > > - rsdt->header.checksum = 0; > > - rsdt->header.checksum = table_compute_checksum((u8 *)rsdt, > > - rsdt->header.length); > > + if (i >= entries_num) { > > + log_err("ACPI: Error: too many tables\n"); > > + return -E2BIG; > > + } > > > > - /* > > - * And now the same thing for the XSDT. We use the same index as for > > - * now we want the XSDT and RSDT to always be in sync in U-Boot > > - */ > > - xsdt = ctx->xsdt; > > + /* Add table to the RSDT */ > > + rsdt->entry[i] = nomap_to_sysmem(table); > > > > - /* Add table to the XSDT */ > > - xsdt->entry[i] = nomap_to_sysmem(table); > > + /* Fix RSDT length or the kernel will assume invalid entries */ > > + rsdt->header.length = sizeof(struct acpi_table_header) + > > + (sizeof(u32) * (i + 1)); > > > > - /* Fix XSDT length */ > > - xsdt->header.length = sizeof(struct acpi_table_header) + > > - (sizeof(u64) * (i + 1)); > > + /* Re-calculate checksum */ > > + rsdt->header.checksum = 0; > > + rsdt->header.checksum = table_compute_checksum((u8 *)rsdt, > > + rsdt->header.length); > > + } > > > > - /* Re-calculate checksum */ > > - xsdt->header.checksum = 0; > > - xsdt->header.checksum = table_compute_checksum((u8 *)xsdt, > > - xsdt->header.length); > > + if (ctx->xsdt) { > > + /* > > + * And now the same thing for the XSDT. We use the same index as for > > + * now we want the XSDT and RSDT to always be in sync in U-Boot > > + */ > > + xsdt = ctx->xsdt; > > + > > + /* This should always be MAX_ACPI_TABLES */ > > + entries_num = ARRAY_SIZE(xsdt->entry); > > + > > + for (i = 0; i < entries_num; i++) { > > + if (xsdt->entry[i] == 0) > > + break; > > + } > > + > > + if (i >= entries_num) { > > + log_err("ACPI: Error: too many tables\n"); > > + return -E2BIG; > > + } > > + > > + /* Add table to the XSDT */ > > + xsdt->entry[i] = nomap_to_sysmem(table); > > + > > + /* Fix XSDT length */ > > + xsdt->header.length = sizeof(struct acpi_table_header) + > > + (sizeof(u64) * (i + 1)); > > + > > + /* Re-calculate checksum */ > > + xsdt->header.checksum = 0; > > + xsdt->header.checksum = table_compute_checksum((u8 *)xsdt, > > + xsdt->header.length); > > + } > > > > return 0; > > } > > -- > > 2.46.0 > >
diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c index 4f5cfe522c..8aab41212a 100644 --- a/lib/acpi/acpi_table.c +++ b/lib/acpi/acpi_table.c @@ -157,51 +157,70 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table) struct acpi_rsdt *rsdt; struct acpi_xsdt *xsdt; - /* The RSDT is mandatory while the XSDT is not */ - rsdt = ctx->rsdt; - - /* This should always be MAX_ACPI_TABLES */ - entries_num = ARRAY_SIZE(rsdt->entry); - - for (i = 0; i < entries_num; i++) { - if (rsdt->entry[i] == 0) - break; - } - - if (i >= entries_num) { - log_err("ACPI: Error: too many tables\n"); - return -E2BIG; - } + /* On legacy x86 platforms the RSDT is mandatory while the XSDT is not. + * On other platforms there might be no memory below 4GiB, thus RSDT is NULL. + */ + if (ctx->rsdt) { + rsdt = ctx->rsdt; - /* Add table to the RSDT */ - rsdt->entry[i] = nomap_to_sysmem(table); + /* This should always be MAX_ACPI_TABLES */ + entries_num = ARRAY_SIZE(rsdt->entry); - /* Fix RSDT length or the kernel will assume invalid entries */ - rsdt->header.length = sizeof(struct acpi_table_header) + - (sizeof(u32) * (i + 1)); + for (i = 0; i < entries_num; i++) { + if (rsdt->entry[i] == 0) + break; + } - /* Re-calculate checksum */ - rsdt->header.checksum = 0; - rsdt->header.checksum = table_compute_checksum((u8 *)rsdt, - rsdt->header.length); + if (i >= entries_num) { + log_err("ACPI: Error: too many tables\n"); + return -E2BIG; + } - /* - * And now the same thing for the XSDT. We use the same index as for - * now we want the XSDT and RSDT to always be in sync in U-Boot - */ - xsdt = ctx->xsdt; + /* Add table to the RSDT */ + rsdt->entry[i] = nomap_to_sysmem(table); - /* Add table to the XSDT */ - xsdt->entry[i] = nomap_to_sysmem(table); + /* Fix RSDT length or the kernel will assume invalid entries */ + rsdt->header.length = sizeof(struct acpi_table_header) + + (sizeof(u32) * (i + 1)); - /* Fix XSDT length */ - xsdt->header.length = sizeof(struct acpi_table_header) + - (sizeof(u64) * (i + 1)); + /* Re-calculate checksum */ + rsdt->header.checksum = 0; + rsdt->header.checksum = table_compute_checksum((u8 *)rsdt, + rsdt->header.length); + } - /* Re-calculate checksum */ - xsdt->header.checksum = 0; - xsdt->header.checksum = table_compute_checksum((u8 *)xsdt, - xsdt->header.length); + if (ctx->xsdt) { + /* + * And now the same thing for the XSDT. We use the same index as for + * now we want the XSDT and RSDT to always be in sync in U-Boot + */ + xsdt = ctx->xsdt; + + /* This should always be MAX_ACPI_TABLES */ + entries_num = ARRAY_SIZE(xsdt->entry); + + for (i = 0; i < entries_num; i++) { + if (xsdt->entry[i] == 0) + break; + } + + if (i >= entries_num) { + log_err("ACPI: Error: too many tables\n"); + return -E2BIG; + } + + /* Add table to the XSDT */ + xsdt->entry[i] = nomap_to_sysmem(table); + + /* Fix XSDT length */ + xsdt->header.length = sizeof(struct acpi_table_header) + + (sizeof(u64) * (i + 1)); + + /* Re-calculate checksum */ + xsdt->header.checksum = 0; + xsdt->header.checksum = table_compute_checksum((u8 *)xsdt, + xsdt->header.length); + } return 0; }
On platforms that do not have usable DRAM below 4GiB, like QEMU sbsa, the RSDT cannot be used. Allow both RSDT and XSDT to be null and only fill those tables that are present in acpi_add_table(). Fixes a crash on QEMU sbsa. Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> Cc: Simon Glass <sjg@chromium.org> Cc: Tom Rini <trini@konsulko.com> --- lib/acpi/acpi_table.c | 95 ++++++++++++++++++++++++++----------------- 1 file changed, 57 insertions(+), 38 deletions(-)