Hi Ilias,
On Thu, 19 Sept 2024 at 15:18, Ilias Apalodimas
<ilias.apalodi...@linaro.org> wrote:
>
> Hi Simon,
>
> On Thu, 19 Sept 2024 at 16:01, Simon Glass <s...@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Thu, 19 Sept 2024 at 09:30, Ilias Apalodimas
> > <ilias.apalodi...@linaro.org> wrote:
> > >
> > > On Thu, 19 Sept 2024 at 09:41, Patrick Rudolph
> > > <patrick.rudo...@9elements.com> wrote:
> > > >
> > > > On Thu, Sep 12, 2024 at 8:55 AM Ilias Apalodimas
> > > > <ilias.apalodi...@linaro.org> wrote:
> > > > >
> > > > > Hi Patrick
> > > > >
> > > > > On Wed, 11 Sept 2024 at 09:25, 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);
> > > > >
> > > > > This addr is not in ACPI_RECLAIM_MEMORY now. It might work since
> > > > > efi_acpi_register() adads it on the EFI memory map.
> > > > > In the last version, Simon replied something along the lines of "This
> > > > > is how x86 works", but I don't understand why. U-Boot at this point is
> > > > > the last bootloader you'll run and then jump to your EFI payload. Can
> > > > > someone clearly explain what's going on here?
> > > > >
> > > > > > + } 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;
> > > > >
> > > > > So this would be fine if we initialized the EFI subsystem early, but
> > > > > we don't. On top of that the efi_acpi_register() call is trying to
> > > > > add the allocated memory to the efi memmap, so we don't need to do
> > > > > that twice.
> > > >
> > > > Sounds like always using BLOBLIST would fix this issue. It would
> > > > decouple
> > > > memory allocation and installation.
> > >
> > > bloblist is supposed to be used by early-stage bootloaders to pass
> > > information around and is an implementation of [0]. Why you would
> > > U-Boot need to add the item in a bloblist? There's no loader after
> > > U-Boot that can consume it.
> >
> > That is one use of bloblist.
> >
> > Within U-Boot proper, bloblist is used to hold all the tables
> > generated by U-Boot, so they are all in one place.
>
> What do you mean by 'tables'. That makes little sense to me,
> especially with the caveats it comes along. ACPI is an EFI construct
> and we should limit it there. I can understand this if a next stage
> loader needs the ACPI in a transfer list, but that use case does not
> exist.
Please take a look at the top of bloblist.h - this is how newer x86
platforms work and it has always been intended that way. It is not a
transfer list, but a bloblist, at least in U-Boot parlance.
Regards,
Simon
>
> Thanks
> /Ilias
> >
> > >
> > > >
> > > > >
> > > > > I think there's a cleaner approach overall to this which will allow us
> > > > > to decouple the installation etc from EFI.
> > > > > You could allocate a piece of memory, assign it to
> > > > > arch.table_start/end and then tweak efi_acpi_register(). Instead of
> > > > > marking the table_end - table_start as ACPI_RECLAIM_MEMORY, allocate
> > > > > the pages there, where you know the EFI subsystem is up and copy it.
> > > > > It would then be the callers responsibilty to free that memory.
> > > > >
> > > > We cannot memcpy ACPI tables to a new location. For example the RSDT
> > > > and XSDT
> > > > contain physical addresses to memory within the ACPI reclaim window,
> > > > which must be tweaked
> > > > when you copy tables. On x86 there's also the GNVS which is patched
> > > > into the DSDT, which points
> > > > to memory within the ACPI reclaim memory window.
> > > > There might be more places I'm not aware of as of now.
> > >
> > > Ok, that's fine though, we can just mark the memory as
> > > ACPI_RECLAIM_MEMORY, like we currently do, without allocating and
> > > copying over stuff.
> >
> > Regards,
> > Simon
> >
> >
> > >
> > > Thanks
> > > /Ilias
> > > >
> > > > > Heinrich, do you see a problem with that?
> > > > >
> > > > > Thanks
> > > > > /Ilias
> > > > >
> > > > > > + }
> > > > > > +
> > > > > > + 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
> > > > > >
> > >
> > > [0] https://github.com/FirmwareHandoff/firmware_handoff
