Hi Simon,
On Thu, 19 Sept 2024 at 17:10, Simon Glass <s...@chromium.org> wrote:
>
> Hi Patrick,
>
> On Wed, 18 Sept 2024 at 17:22, Patrick Rudolph
> <patrick.rudo...@9elements.com> wrote:
> >
> > Allocate memory for ACPI tables inside the efi_loader and write out
> > the tables similar to SMBIOS tables. When ACPI is enabled and wasn't
> > installed in other places, install the ACPI table in EFI. Since EFI
> > is necessary to pass the ACPI table location when FDT isn't used,
> > there's no need to install it separately.
> >
> > When CONFIG_BLOBLIST_TABLES is set the tables will be stored in a
> > bloblist. The tables are still passed to the OS using EFI.
> >
> > This allows non x86 platforms to boot using ACPI only in case the
> > EFI loader is being used.
> >
> > TEST: Booted QEMU SBSA (no QFW) using EFI and ACPI only.
> >
> > Signed-off-by: Patrick Rudolph <patrick.rudo...@9elements.com>
> > Cc: Simon Glass <s...@chromium.org>
> > Cc: Tom Rini <tr...@konsulko.com>
> > ---
> > Changelog v3:
> > - Drop memalign and use efi_allocate_pages
> > - Use log_debug instead of debug
> > - Clarify commit message
> > - Skip writing ACPI tables on sandbox
> > - Rename function
> > - Add function comment
> > ---
> > lib/efi_loader/efi_acpi.c | 80 +++++++++++++++++++++++++++++++-
> > test/py/tests/test_event_dump.py | 1 +
> > 2 files changed, 79 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_acpi.c b/lib/efi_loader/efi_acpi.c
> > index 67bd7f8ca2..9d38d0060c 100644
> > --- a/lib/efi_loader/efi_acpi.c
> > +++ b/lib/efi_loader/efi_acpi.c
> > @@ -6,15 +6,23 @@
> > */
> >
> > #include <efi_loader.h>
> > -#include <log.h>
> > -#include <mapmem.h>
> > #include <acpi/acpi_table.h>
> > #include <asm/global_data.h>
> > +#include <asm/io.h>
> > +#include <bloblist.h>
> > +#include <linux/sizes.h>
> > +#include <linux/log2.h>
> > +#include <log.h>
> > +#include <malloc.h>
> > +#include <mapmem.h>
> >
> > DECLARE_GLOBAL_DATA_PTR;
> >
> > static const efi_guid_t acpi_guid = EFI_ACPI_TABLE_GUID;
> >
> > +enum {
> > + TABLE_SIZE = SZ_64K,
> > +};
> > /*
> > * Install the ACPI table as a configuration table.
> > *
> > @@ -47,3 +55,71 @@ efi_status_t efi_acpi_register(void)
> > return efi_install_configuration_table(&acpi_guid,
> > (void *)(ulong)addr);
> > }
> > +
> > +/*
> > + * Allocate memory for ACPI tables and write ACPI tables to the
> > + * allocated buffer.
> > + *
> > + * Return: status code
> > + */
> > +static int alloc_write_acpi_tables(void)
> > +{
> > + u64 table_addr, table_end;
> > + u64 new_acpi_addr = 0;
> > + efi_uintn_t pages;
> > + efi_status_t ret;
> > + void *addr;
> > +
> > + if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE))
> > + return 0;
> > +
> > + if (IS_ENABLED(CONFIG_X86) ||
> > + IS_ENABLED(CONFIG_QFW_ACPI) ||
> > + IS_ENABLED(CONFIG_SANDBOX)) {
> > + log_debug("Skipping writing ACPI tables as already done\n");
> > + return 0;
> > + }
> > +
> > + /* Align the table to a 4KB boundary to keep EFI happy */
> > + if (IS_ENABLED(CONFIG_BLOBLIST_TABLES)) {
> > + addr = bloblist_add(BLOBLISTT_ACPI_TABLES, TABLE_SIZE,
> > + ilog2(SZ_4K));
> > +
> > + if (!addr)
> > + return log_msg_ret("mem", -ENOMEM);
> > + } else {
> > + pages = efi_size_in_pages(TABLE_SIZE);
> > +
> > + ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
> > + EFI_ACPI_RECLAIM_MEMORY,
> > + pages, &new_acpi_addr);
> > + if (ret != EFI_SUCCESS)
> > + return log_msg_ret("mem", -ENOMEM);
> > +
> > + addr = (void *)(uintptr_t)new_acpi_addr;
> > + }
> > +
>
> The tables should be written regardless of whether EFI_LOADER is enabled.
*Why*? How do you expect to hand them over to the OS?
>
> Can we drop this else clause? We should always use the bloblist.
Both Heinrich and I said the exact opposite. Unless there's a
perfectly good reason why we should keep them in bloblist memory, I'd
like us to do that
Thanks
/Ilias
>
> We should use efi_add_memory_map() to add to the memory map.
>
> > + table_addr = virt_to_phys(addr);
> > +
> > + gd->arch.table_start_high = table_addr;
> > +
> > + table_end = write_acpi_tables(table_addr);
> > + if (!table_end) {
> > + log_err("Can't create ACPI configuration table\n");
> > + return -EINTR;
> > + }
> > +
> > + log_debug("- wrote 'acpi' to %llx, end %llx\n", table_addr,
> > table_end);
> > + if (table_end - table_addr > TABLE_SIZE) {
> > + log_err("Out of space for configuration tables: need %llx,
> > have %x\n",
> > + table_end - table_addr, TABLE_SIZE);
> > + return log_msg_ret("acpi", -ENOSPC);
> > + }
> > + gd->arch.table_end_high = table_end;
> > +
> > + log_debug("- done writing tables\n");
> > +
> > + return 0;
> > +}
> > +
> > +EVENT_SPY_SIMPLE(EVT_LAST_STAGE_INIT, alloc_write_acpi_tables);
> > diff --git a/test/py/tests/test_event_dump.py
> > b/test/py/tests/test_event_dump.py
> > index e282c67335..459bfa26bb 100644
> > --- a/test/py/tests/test_event_dump.py
> > +++ b/test/py/tests/test_event_dump.py
> > @@ -18,6 +18,7 @@ def test_event_dump(u_boot_console):
> > -------------------- ------------------------------
> > ------------------------------
> > EVT_FT_FIXUP bootmeth_vbe_ft_fixup
> > .*boot/vbe_request.c:.*
> > EVT_FT_FIXUP bootmeth_vbe_simple_ft_fixup
> > .*boot/vbe_simple_os.c:.*
> > +EVT_LAST_STAGE_INIT alloc_write_acpi_tables
> > .*lib/efi_loader/efi_acpi.c:.*
> > EVT_LAST_STAGE_INIT install_smbios_table
> > .*lib/efi_loader/efi_smbios.c:.*
> > EVT_MISC_INIT_F sandbox_early_getopt_check
> > .*arch/sandbox/cpu/start.c:.*
> > EVT_TEST h_adder_simple
> > .*test/common/event.c:'''
> > --
> > 2.46.0
> >
>
> REgards,
> Simon
