Antwort: [PATCH v2 25/39] acpi: Put table-setup code in its own function
Wolfgang Wallner
wolfgang.wallner at br-automation.com
Wed Mar 11 15:33:15 CET 2020
Hi Simon,
-----"Simon Glass" <sjg at chromium.org> schrieb: -----
>
> We always write three basic tables to ACPI at the start. Move this into
> its own function, along with acpi_fill_header(), so we can write a test
> for this code.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
> Changes in v2: None
>
> arch/x86/lib/acpi_table.c | 77 +----------------------------------
> include/acpi_table.h | 10 +++++
> lib/acpi/acpi_table.c | 86 ++++++++++++++++++++++++++++++++++++++-
> test/dm/acpi.c | 55 ++++++++++++++++++++++++-
> 4 files changed, 148 insertions(+), 80 deletions(-)
>
> diff --git a/arch/x86/lib/acpi_table.c b/arch/x86/lib/acpi_table.c
> index 9168119547..83b92e2a4c 100644
> --- a/arch/x86/lib/acpi_table.c
> +++ b/arch/x86/lib/acpi_table.c
> @@ -31,58 +31,6 @@ extern const unsigned char AmlCode[];
> /* ACPI RSDP address to be used in boot parameters */
> static ulong acpi_rsdp_addr;
>
> -static void acpi_write_rsdp(struct acpi_rsdp *rsdp, struct acpi_rsdt *rsdt,
> - struct acpi_xsdt *xsdt)
> -{
> - memset(rsdp, 0, sizeof(struct acpi_rsdp));
> -
> - memcpy(rsdp->signature, RSDP_SIG, 8);
> - memcpy(rsdp->oem_id, OEM_ID, 6);
> -
> - rsdp->length = sizeof(struct acpi_rsdp);
> - rsdp->rsdt_address = (u32)rsdt;
> -
> - rsdp->xsdt_address = (u64)(u32)xsdt;
> - rsdp->revision = ACPI_RSDP_REV_ACPI_2_0;
> -
> - /* Calculate checksums */
> - rsdp->checksum = table_compute_checksum((void *)rsdp, 20);
> - rsdp->ext_checksum = table_compute_checksum((void *)rsdp,
> - sizeof(struct acpi_rsdp));
> -}
> -
> -static void acpi_write_rsdt(struct acpi_rsdt *rsdt)
> -{
> - struct acpi_table_header *header = &(rsdt->header);
> -
> - /* Fill out header fields */
> - acpi_fill_header(header, "RSDT");
> - header->length = sizeof(struct acpi_rsdt);
> - header->revision = 1;
> -
> - /* Entries are filled in later, we come with an empty set */
> -
> - /* Fix checksum */
> - header->checksum = table_compute_checksum((void *)rsdt,
> - sizeof(struct acpi_rsdt));
> -}
> -
> -static void acpi_write_xsdt(struct acpi_xsdt *xsdt)
> -{
> - struct acpi_table_header *header = &(xsdt->header);
> -
> - /* Fill out header fields */
> - acpi_fill_header(header, "XSDT");
> - header->length = sizeof(struct acpi_xsdt);
> - header->revision = 1;
> -
> - /* Entries are filled in later, we come with an empty set */
> -
> - /* Fix checksum */
> - header->checksum = table_compute_checksum((void *)xsdt,
> - sizeof(struct acpi_xsdt));
> -}
> -
> static void acpi_create_facs(struct acpi_facs *facs)
> {
> memset((void *)facs, 0, sizeof(struct acpi_facs));
> @@ -402,7 +350,6 @@ static void acpi_create_spcr(struct acpi_spcr *spcr)
> ulong write_acpi_tables(ulong start_addr)
> {
> struct acpi_ctx sctx, *ctx = &sctx;
> - struct acpi_xsdt *xsdt;
> struct acpi_facs *facs;
> struct acpi_table_header *dsdt;
> struct acpi_fadt *fadt;
> @@ -415,32 +362,10 @@ ulong write_acpi_tables(ulong start_addr)
> int i;
>
> start = map_sysmem(start_addr, 0);
> - ctx->current = start;
> -
> - /* Align ACPI tables to 16 byte */
> - acpi_align(ctx);
>
> debug("ACPI: Writing ACPI tables at %lx\n", start_addr);
>
> - /* We need at least an RSDP and an RSDT Table */
> - ctx->rsdp = ctx->current;
> - acpi_inc_align(ctx, sizeof(struct acpi_rsdp));
> - ctx->rsdt = ctx->current;
> - acpi_inc_align(ctx, sizeof(struct acpi_rsdt));
> - xsdt = ctx->current;
> - acpi_inc_align(ctx, sizeof(struct acpi_xsdt));
> - /*
> - * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> - * boundary (Windows checks this, but Linux does not).
> - */
> - acpi_align_large(ctx);
> -
> - /* clear all table memory */
> - memset((void *)start, 0, ctx->current - start);
> -
> - acpi_write_rsdp(ctx->rsdp, ctx->rsdt, xsdt);
> - acpi_write_rsdt(ctx->rsdt);
> - acpi_write_xsdt(xsdt);
> + acpi_setup_base_tables(ctx, start);
>
> debug("ACPI: * FACS\n");
> facs = ctx->current;
> diff --git a/include/acpi_table.h b/include/acpi_table.h
> index 2131484880..f500f0d3fe 100644
> --- a/include/acpi_table.h
> +++ b/include/acpi_table.h
> @@ -564,6 +564,16 @@ void acpi_inc_align(struct acpi_ctx *ctx, uint amount);
> */
> int acpi_add_table(struct acpi_ctx *ctx, void *table);
>
> +/**
> + * acpi_setup_base_tables() - Set up context along with RSDP, RSDT and XDST
typo: XSDT
> + *
> + * Set up the context with the given start position. Some basic tables are
> + * always needed, so set them up as well.
> + *
> + * @ctx: Context to set up
> + */
> +void acpi_setup_base_tables(struct acpi_ctx *ctx, void *start);
> +
> #endif /* !__ACPI__*/
>
> #include <asm/acpi_table.h>
> diff --git a/lib/acpi/acpi_table.c b/lib/acpi/acpi_table.c
> index 00e80ac39a..9f452a65ce 100644
> --- a/lib/acpi/acpi_table.c
> +++ b/lib/acpi/acpi_table.c
> @@ -157,7 +157,7 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table)
>
> /* Re-calculate checksum */
> rsdt->header.checksum = 0;
> - rsdt->header.checksum = table_compute_checksum((u8 *)rsdt,
> + rsdt->header.checksum = table_compute_checksum(rsdt,
> rsdt->header.length);
>
> /*
> @@ -175,8 +175,90 @@ int acpi_add_table(struct acpi_ctx *ctx, void *table)
> /* Re-calculate checksum */
> xsdt->header.checksum = 0;
> xsdt->header.checksum =
> - table_compute_checksum((u8 *)xsdt, xsdt->header.length);
> + table_compute_checksum(xsdt, xsdt->header.length);
> }
>
> return 0;
> }
> +
> +static void acpi_write_rsdp(struct acpi_rsdp *rsdp, struct acpi_rsdt *rsdt,
> + struct acpi_xsdt *xsdt)
> +{
> + memset(rsdp, 0, sizeof(struct acpi_rsdp));
> +
> + memcpy(rsdp->signature, RSDP_SIG, 8);
> + memcpy(rsdp->oem_id, OEM_ID, 6);
> +
> + rsdp->length = sizeof(struct acpi_rsdp);
> + rsdp->rsdt_address = map_to_sysmem(rsdt);
> +
> + rsdp->xsdt_address = map_to_sysmem(xsdt);
> + rsdp->revision = ACPI_RSDP_REV_ACPI_2_0;
> +
> + /* Calculate checksums */
> + rsdp->checksum = table_compute_checksum(rsdp, 20);
> + rsdp->ext_checksum = table_compute_checksum(rsdp,
> + sizeof(struct acpi_rsdp));
> +}
> +
> +static void acpi_write_rsdt(struct acpi_rsdt *rsdt)
> +{
> + struct acpi_table_header *header = &rsdt->header;
> +
> + /* Fill out header fields */
> + acpi_fill_header(header, "RSDT");
> + header->length = sizeof(struct acpi_rsdt);
> + header->revision = 1;
> +
> + /* Entries are filled in later, we come with an empty set */
> +
> + /* Fix checksum */
> + header->checksum = table_compute_checksum(rsdt,
> + sizeof(struct acpi_rsdt));
> +}
> +
> +static void acpi_write_xsdt(struct acpi_xsdt *xsdt)
> +{
> + struct acpi_table_header *header = &xsdt->header;
> +
> + /* Fill out header fields */
> + acpi_fill_header(header, "XSDT");
> + header->length = sizeof(struct acpi_xsdt);
> + header->revision = 1;
> +
> + /* Entries are filled in later, we come with an empty set */
> +
> + /* Fix checksum */
> + header->checksum = table_compute_checksum(xsdt,
> + sizeof(struct acpi_xsdt));
> +}
> +
> +void acpi_setup_base_tables(struct acpi_ctx *ctx, void *start)
> +{
> + struct acpi_xsdt *xsdt;
> +
> + ctx->current = start;
> +
> + /* Align ACPI tables to 16 byte */
> + acpi_align(ctx);
> +
> + /* We need at least an RSDP and an RSDT Table */
> + ctx->rsdp = ctx->current;
> + acpi_inc_align(ctx, sizeof(struct acpi_rsdp));
> + ctx->rsdt = ctx->current;
> + acpi_inc_align(ctx, sizeof(struct acpi_rsdt));
> + xsdt = ctx->current;
> + acpi_inc_align(ctx, sizeof(struct acpi_xsdt));
> + /*
> + * Per ACPI spec, the FACS table address must be aligned to a 64 byte
> + * boundary (Windows checks this, but Linux does not).
> + */
> + acpi_align_large(ctx);
Having this call inside of acpi_setup_base_tables() creates the requirement
that the FACS is set up directly after the base tables. With the current
code this is satisfied, but it might be overlooked by future refactoring.
Maybe move this call out directly in front of the setup of FACS?
> +
> + /* clear all table memory */
> + memset((void *)start, '\0', ctx->current - start);
> +
> + acpi_write_rsdp(ctx->rsdp, ctx->rsdt, xsdt);
> + acpi_write_rsdt(ctx->rsdt);
> + acpi_write_xsdt(xsdt);
> +}
> diff --git a/test/dm/acpi.c b/test/dm/acpi.c
> index a2a57a29a6..bb66c7229c 100644
> --- a/test/dm/acpi.c
> +++ b/test/dm/acpi.c
> @@ -9,6 +9,9 @@
> #include <common.h>
> #include <acpi_table.h>
> #include <dm.h>
> +#include <malloc.h>
> +#include <mapmem.h>
> +#include <tables_csum.h>
> #include <version.h>
> #include <dm/acpi.h>
> #include <dm/test.h>
> @@ -138,9 +141,9 @@ static int dm_test_acpi_write_tables(struct unit_test_state *uts)
> buf = malloc(BUF_SIZE);
> ut_assertnonnull(buf);
>
> - ctx.current = buf;
> + acpi_setup_base_tables(&ctx, buf);
> + dmar = ctx.current;
> ut_assertok(acpi_write_dev_tables(&ctx));
> - dmar = buf;
>
> /*
> * We should have two dmar tables, one for each "denx,u-boot-acpi-test"
> @@ -153,6 +156,11 @@ static int dm_test_acpi_write_tables(struct unit_test_state *uts)
> ut_asserteq(DMAR_INTR_REMAP, dmar[1].flags);
> ut_asserteq(32 - 1, dmar[1].host_address_width);
>
> + /* Check that the pointers were added correctly */
> + ut_asserteq(map_to_sysmem(dmar), ctx.rsdt->entry[0]);
> + ut_asserteq(map_to_sysmem(dmar + 1), ctx.rsdt->entry[1]);
> + ut_asserteq(0, ctx.rsdt->entry[2]);
> +
> return 0;
> }
> DM_TEST(dm_test_acpi_write_tables, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
> @@ -184,3 +192,46 @@ static int dm_test_acpi_basic(struct unit_test_state *uts)
> return 0;
> }
> DM_TEST(dm_test_acpi_basic, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
> +
> +/* Test acpi_setup_base_tables */
> +static int dm_test_acpi_setup_base_tables(struct unit_test_state *uts)
> +{
> + struct acpi_rsdp *rsdp;
> + struct acpi_rsdt *rsdt;
> + struct acpi_xsdt *xsdt;
> + struct acpi_ctx ctx;
> + void *buf, *end;
> +
> + /* Use an unaligned address deliberately */
> + buf = memalign(64, BUF_SIZE);
Maybe I'm missing something, but as far as I understand it, the comment
contradicts the code (aligning to 64 bytes makes it an aligned address,
doesn't it?).
> + ut_assertnonnull(buf);
> + acpi_setup_base_tables(&ctx, buf + 4);
> +
> + rsdp = buf + 16;
> + ut_asserteq_ptr(rsdp, ctx.rsdp);
> + ut_assertok(memcmp(RSDP_SIG, rsdp->signature, sizeof(rsdp->signature)));
> + ut_asserteq(sizeof(*rsdp), rsdp->length);
> + ut_assertok(table_compute_checksum(rsdp, 20));
> + ut_assertok(table_compute_checksum(rsdp, sizeof(*rsdp)));
> +
> + rsdt = PTR_ALIGN((void *)rsdp + sizeof(*rsdp), 16);
> + ut_asserteq_ptr(rsdt, ctx.rsdt);
> + ut_assertok(memcmp("RSDT", rsdt->header.signature, ACPI_NAME_LEN));
> + ut_asserteq(sizeof(*rsdt), rsdt->header.length);
> + ut_assertok(table_compute_checksum(rsdt, sizeof(*rsdt)));
> +
> + xsdt = PTR_ALIGN((void *)rsdt + sizeof(*rsdt), 16);
> + ut_assertok(memcmp("XSDT", xsdt->header.signature, ACPI_NAME_LEN));
> + ut_asserteq(sizeof(*xsdt), xsdt->header.length);
> + ut_assertok(table_compute_checksum(xsdt, sizeof(*xsdt)));
> +
> + end = PTR_ALIGN((void *)xsdt + sizeof(*xsdt), 64);
> + ut_asserteq_ptr(end, ctx.current);
> +
> + ut_asserteq(map_to_sysmem(rsdt), rsdp->rsdt_address);
> + ut_asserteq(map_to_sysmem(xsdt), rsdp->xsdt_address);
> +
> + return 0;
> +}
> +DM_TEST(dm_test_acpi_setup_base_tables,
> + DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
> --
> 2.25.1.481.gfbce0eb801-goog
regards, Wolfgang
More information about the U-Boot
mailing list