Hi Simon, On Thu, 19 Sept 2024 at 18:00, Simon Glass <s...@chromium.org> wrote: > > Hi Ilias, >
[...] > > > > + > > > > + 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? > > Why - because boards which need ACPI tables to boot should generate > them; Noone argued that. > also this happens when U-Boot starts up, in last_stage_init() > How - it isn't possible, but eventually I suppose it will be, once we > have a use case for booting with ACPI but without EFI Are you aware of such an OS? If not, we can accept the patches when we have a reason. I remember patches being hard NAK'ed on using DTs to pass the ACPI table address in the past. > > > > > > > > > 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 > > The main reason is that we should not be putting things in memory > between the start of RAM and the bottom of the U-Boot area. That > region of memory is supposed to be for loading images. Most boards > hard-code the image-load addresses in environment variables. But even > if they didn't, we should not be using that memory for U-Boot data. EFI allows you to request a specific address to use. We can choose one that fits the requirements > > Another reason is that the bloblist is designed for this, for keeping > tables in a small, contiguous area of memory, so that when passed to > Linux they are not spread all over the place. Linux does not and will not read tables from a bloblist though Thanks /Ilias > > Regards, > Simon > > > > > > 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