[PATCH v4 14/35] efi_loader: Allocate and write ACPI tables

[Ilias Apalodimas]

On Thu, 19 Sept 2024 at 18:19, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Simon,
>
> On Thu, 19 Sept 2024 at 18:00, Simon Glass <sjg at 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

Also it's ACPI_RECLAIM memory. The OS doesn't really care if it's
scattered or not. It is up to the OS to reuse it.

/Ilias
>
> 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