| Message ID | 20241011212126.747741-5-sjg@chromium.org |
|---|---|
| State | New |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Adjust initial EFI memory-allocation to be in the U-Boot region | expand |
Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass <sjg@chromium.org>: >The 'point of cooperation' is where U-Boot starts allowing EFI to use >memory outside of the U-Boot region. Until that point, it is desirable >to keep more below U-Boot free for loading images. > >Reserve a small region for this purpose. Your commit message provides no clue why this should be needed. If we ensure thst LMB is up before EFI initialization, the EFI subsystem will be able to allocate memory from there. Best regards Heinrich > >Signed-off-by: Simon Glass <sjg@chromium.org> >--- > >(no changes since v1) > > include/asm-generic/global_data.h | 6 ++++++ > lib/efi_loader/efi_memory.c | 21 +++++++++++++++++++++ > test/py/tests/test_event_dump.py | 1 + > 3 files changed, 28 insertions(+) > >diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h >index d6c15e2c406..20705be14dd 100644 >--- a/include/asm-generic/global_data.h >+++ b/include/asm-generic/global_data.h >@@ -443,6 +443,12 @@ struct global_data { > */ > struct upl *upl; > #endif >+#if CONFIG_IS_ENABLED(EFI_LOADER) >+ /** >+ * @efi_region: Start of EFI's early-memory region >+ */ >+ ulong efi_region; >+#endif > }; > #ifndef DO_DEPS_ONLY > static_assert(sizeof(struct global_data) == GD_SIZE); >diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c >index 50cb2f3898b..9cc33397371 100644 >--- a/lib/efi_loader/efi_memory.c >+++ b/lib/efi_loader/efi_memory.c >@@ -24,6 +24,12 @@ DECLARE_GLOBAL_DATA_PTR; > /* Magic number identifying memory allocated from pool */ > #define EFI_ALLOC_POOL_MAGIC 0x1fe67ddf6491caa2 > >+/* >+ * Amount of memory to reserve for EFI before relocation. This must be a >+ * multiple of EFI_PAGE_SIZE >+ */ >+#define EFI_EARLY_REGION_SIZE SZ_256K >+ > efi_uintn_t efi_memory_map_key; > > struct efi_mem_list { >@@ -944,3 +950,18 @@ int efi_memory_init(void) > > return 0; > } >+ >+static int reserve_efi_region(void) >+{ >+ /* >+ * Reserve some memory for EFI. Since pool allocations consume 4KB each >+ * and there are three allocations, allow 16KB of memory, enough for >+ * four. This can be increased as needed. >+ */ >+ gd->efi_region = ALIGN_DOWN(gd->start_addr_sp - EFI_EARLY_REGION_SIZE, >+ SZ_4K); >+ gd->start_addr_sp = gd->efi_region; >+ >+ return 0; >+} >+EVENT_SPY_SIMPLE(EVT_RESERVE, reserve_efi_region); >diff --git a/test/py/tests/test_event_dump.py b/test/py/tests/test_event_dump.py >index e282c67335c..e36fc6a1586 100644 >--- a/test/py/tests/test_event_dump.py >+++ b/test/py/tests/test_event_dump.py >@@ -20,5 +20,6 @@ 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 install_smbios_table .*lib/efi_loader/efi_smbios.c:.* > EVT_MISC_INIT_F sandbox_early_getopt_check .*arch/sandbox/cpu/start.c:.* >+EVT_RESERVE reserve_efi_region .*lib/efi_loader/efi_memory.c:.* > EVT_TEST h_adder_simple .*test/common/event.c:''' > assert re.match(expect, out, re.MULTILINE) is not None
On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote: > > > Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass <sjg@chromium.org>: > >The 'point of cooperation' is where U-Boot starts allowing EFI to use > >memory outside of the U-Boot region. Until that point, it is desirable > >to keep more below U-Boot free for loading images. > > > >Reserve a small region for this purpose. > > Your commit message provides no clue why this should be needed. The alternative here would be a patch to doc/develop/memory.rst saying something to the effect of: EFI pool allocations start at ... U-Boot stack pointer + CONFIG_STACK_SIZE and grow downwards from there. These are done by ... a brief description of how the pool allocations work ... Nominally, this will extend for only a few pages worth of memory but depending on requests made by EFI applications can grow larger. All of this is covered by the LMB that covers U-Boot itself and is considered part of the U-Boot memory map. It may be broken down in to further detail to the memory map provided to the OS, in some cases.
Hi Tom On Sat, 12 Oct 2024 at 04:27, Tom Rini <trini@konsulko.com> wrote: > > On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote: > > > > > > Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass <sjg@chromium.org>: > > >The 'point of cooperation' is where U-Boot starts allowing EFI to use > > >memory outside of the U-Boot region. Until that point, it is desirable > > >to keep more below U-Boot free for loading images. > > > > > >Reserve a small region for this purpose. > > > > Your commit message provides no clue why this should be needed. > > The alternative here would be a patch to doc/develop/memory.rst saying > something to the effect of: > > EFI pool allocations start at ... U-Boot stack pointer + > CONFIG_STACK_SIZE and grow downwards from there. These are done by ... a > brief description of how the pool allocations work ... Nominally, this > will extend for only a few pages worth of memory but depending on > requests made by EFI applications can grow larger. All of this is > covered by the LMB that covers U-Boot itself and is considered part of > the U-Boot memory map. It may be broken down in to further detail to the > memory map provided to the OS, in some cases. I'll send a patch to update that next week. Perhaps even fold it in the LMB series Thanks /Ilias > > -- > Tom
Hi, On Fri, 11 Oct 2024 at 23:47, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Tom > > On Sat, 12 Oct 2024 at 04:27, Tom Rini <trini@konsulko.com> wrote: > > > > On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote: > > > > > > > > > Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass <sjg@chromium.org>: > > > >The 'point of cooperation' is where U-Boot starts allowing EFI to use > > > >memory outside of the U-Boot region. Until that point, it is desirable > > > >to keep more below U-Boot free for loading images. > > > > > > > >Reserve a small region for this purpose. > > > > > > Your commit message provides no clue why this should be needed. Yes, I hadn't realised that people didn't understand what I was getting at. Tom asked the same question on irc. It allows us to separate the lmb allocations from the EFI allocations, so we don't need to have them both in sync. We use lmb for loading images, then EFI takes over and does what it likes, respecting the existing lmb reservations. > > > > The alternative here would be a patch to doc/develop/memory.rst saying > > something to the effect of: > > > > EFI pool allocations start at ... U-Boot stack pointer + > > CONFIG_STACK_SIZE and grow downwards from there. These are done by ... a > > brief description of how the pool allocations work ... Nominally, this > > will extend for only a few pages worth of memory but depending on > > requests made by EFI applications can grow larger. All of this is > > covered by the LMB that covers U-Boot itself and is considered part of > > the U-Boot memory map. It may be broken down in to further detail to the > > memory map provided to the OS, in some cases. > > I'll send a patch to update that next week. Perhaps even fold it in > the LMB series Before we know we are booting an EFI app? No, thank you. EFI should not be growing downwards like a stack while U-Boot is running... Regards, SImon
On Mon, Oct 14, 2024 at 02:19:56PM -0600, Simon Glass wrote: > Hi, > > On Fri, 11 Oct 2024 at 23:47, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Tom > > > > On Sat, 12 Oct 2024 at 04:27, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote: > > > > > > > > > > > > Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass <sjg@chromium.org>: > > > > >The 'point of cooperation' is where U-Boot starts allowing EFI to use > > > > >memory outside of the U-Boot region. Until that point, it is desirable > > > > >to keep more below U-Boot free for loading images. > > > > > > > > > >Reserve a small region for this purpose. > > > > > > > > Your commit message provides no clue why this should be needed. > > Yes, I hadn't realised that people didn't understand what I was > getting at. Tom asked the same question on irc. > > It allows us to separate the lmb allocations from the EFI allocations, > so we don't need to have them both in sync. We use lmb for loading > images, then EFI takes over and does what it likes, respecting the > existing lmb reservations. But, again, LMB is not for loading of images. It's for dealing with memory reservations of various types.
Hi Tom, On Mon, 14 Oct 2024 at 15:42, Tom Rini <trini@konsulko.com> wrote: > > On Mon, Oct 14, 2024 at 02:19:56PM -0600, Simon Glass wrote: > > Hi, > > > > On Fri, 11 Oct 2024 at 23:47, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Tom > > > > > > On Sat, 12 Oct 2024 at 04:27, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote: > > > > > > > > > > > > > > > Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass <sjg@chromium.org>: > > > > > >The 'point of cooperation' is where U-Boot starts allowing EFI to use > > > > > >memory outside of the U-Boot region. Until that point, it is desirable > > > > > >to keep more below U-Boot free for loading images. > > > > > > > > > > > >Reserve a small region for this purpose. > > > > > > > > > > Your commit message provides no clue why this should be needed. > > > > Yes, I hadn't realised that people didn't understand what I was > > getting at. Tom asked the same question on irc. > > > > It allows us to separate the lmb allocations from the EFI allocations, > > so we don't need to have them both in sync. We use lmb for loading > > images, then EFI takes over and does what it likes, respecting the > > existing lmb reservations. > > But, again, LMB is not for loading of images. It's for dealing with > memory reservations of various types. Up until recently, I believe, my statement was true, but in any case, this isn't gemaine to the issue here. The point is, this is a useful distinction, allowing us to avoid the complexity of keeping them in sync, and avoid putting this pain on people who are not using EFI. We are either in U-Boot code, in which case lmb rules, or we are going into an app, in which case EFI rules (but must read in the lmb info). We are going to end up with people turning off EFI_LOADER because it behaves so badly. Regards, Simon
On Mon, Oct 14, 2024 at 03:50:51PM -0600, Simon Glass wrote: > Hi Tom, > > On Mon, 14 Oct 2024 at 15:42, Tom Rini <trini@konsulko.com> wrote: > > > > On Mon, Oct 14, 2024 at 02:19:56PM -0600, Simon Glass wrote: > > > Hi, > > > > > > On Fri, 11 Oct 2024 at 23:47, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Hi Tom > > > > > > > > On Sat, 12 Oct 2024 at 04:27, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > > > On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote: > > > > > > > > > > > > > > > > > > Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass <sjg@chromium.org>: > > > > > > >The 'point of cooperation' is where U-Boot starts allowing EFI to use > > > > > > >memory outside of the U-Boot region. Until that point, it is desirable > > > > > > >to keep more below U-Boot free for loading images. > > > > > > > > > > > > > >Reserve a small region for this purpose. > > > > > > > > > > > > Your commit message provides no clue why this should be needed. > > > > > > Yes, I hadn't realised that people didn't understand what I was > > > getting at. Tom asked the same question on irc. > > > > > > It allows us to separate the lmb allocations from the EFI allocations, > > > so we don't need to have them both in sync. We use lmb for loading > > > images, then EFI takes over and does what it likes, respecting the > > > existing lmb reservations. > > > > But, again, LMB is not for loading of images. It's for dealing with > > memory reservations of various types. > > Up until recently, I believe, my statement was true, but in any case, > this isn't gemaine to the issue here. > > The point is, this is a useful distinction, allowing us to avoid the > complexity of keeping them in sync, and avoid putting this pain on > people who are not using EFI. We are either in U-Boot code, in which > case lmb rules, or we are going into an app, in which case EFI rules > (but must read in the lmb info). > > We are going to end up with people turning off EFI_LOADER because it > behaves so badly. Frankly, in hind sight I should not have agreed to split the LMB rework between "everything else" and "EFI" in hopes that we would then be able to get the "EFI" part of this agreed upon. It's "behaving badly" right now because we merged half of the changes in the hopes that we could get your agreement on the rest of them fairly quickly. This is not happening, however.
Hi Tom, On Mon, 14 Oct 2024 at 15:55, Tom Rini <trini@konsulko.com> wrote: > > On Mon, Oct 14, 2024 at 03:50:51PM -0600, Simon Glass wrote: > > Hi Tom, > > > > On Mon, 14 Oct 2024 at 15:42, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Mon, Oct 14, 2024 at 02:19:56PM -0600, Simon Glass wrote: > > > > Hi, > > > > > > > > On Fri, 11 Oct 2024 at 23:47, Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > Hi Tom > > > > > > > > > > On Sat, 12 Oct 2024 at 04:27, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > > > > > On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote: > > > > > > > > > > > > > > > > > > > > > Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass <sjg@chromium.org>: > > > > > > > >The 'point of cooperation' is where U-Boot starts allowing EFI to use > > > > > > > >memory outside of the U-Boot region. Until that point, it is desirable > > > > > > > >to keep more below U-Boot free for loading images. > > > > > > > > > > > > > > > >Reserve a small region for this purpose. > > > > > > > > > > > > > > Your commit message provides no clue why this should be needed. > > > > > > > > Yes, I hadn't realised that people didn't understand what I was > > > > getting at. Tom asked the same question on irc. > > > > > > > > It allows us to separate the lmb allocations from the EFI allocations, > > > > so we don't need to have them both in sync. We use lmb for loading > > > > images, then EFI takes over and does what it likes, respecting the > > > > existing lmb reservations. > > > > > > But, again, LMB is not for loading of images. It's for dealing with > > > memory reservations of various types. > > > > Up until recently, I believe, my statement was true, but in any case, > > this isn't gemaine to the issue here. > > > > The point is, this is a useful distinction, allowing us to avoid the > > complexity of keeping them in sync, and avoid putting this pain on > > people who are not using EFI. We are either in U-Boot code, in which > > case lmb rules, or we are going into an app, in which case EFI rules > > (but must read in the lmb info). > > > > We are going to end up with people turning off EFI_LOADER because it > > behaves so badly. > > Frankly, in hind sight I should not have agreed to split the LMB rework > between "everything else" and "EFI" in hopes that we would then be able > to get the "EFI" part of this agreed upon. It's "behaving badly" right > now because we merged half of the changes in the hopes that we could get > your agreement on the rest of them fairly quickly. This is not > happening, however. Well, another option would be to revert the problem patch (or two) and do this work with careful consideration of the impacts, taking account of my architectural objections. My main concern is using U-Boot's memory like a stack. Suddenly this is 'OK' and we apparently want to update U-Boot's docs to say so. It is just not a good idea! A good part of the reason that U-Boot solves the problems it does is that it mostly has a good design. The EFI_LOADER can be improved and it does not need to implement things in a particular way...it just needs to follow the spec. We are being far too rigid in saying that every memory allocation at every stage has to be handled the same way, etc. That said, I did not expect the first series to cause problems. If my EFI-test series had landed then we could perhaps have added a test for this case of the app requesting a chunk of memory. My hope is for that test to grow and cover some of these areas, I hope this lmb/efi thing doesn't cause further problems and I will be looking to ensure that this code does not affect non-EFI booting negatively. My series to solve this same problem (EFI's use of memory) have never been more than a few small patches. Please take a look at the current series[1], particularly patch 5. Regards, Simon [1] https://patchwork.ozlabs.org/project/uboot/list/?series=427729
On Tue, Oct 15, 2024 at 07:12:21AM -0600, Simon Glass wrote: > Hi Tom, > > On Mon, 14 Oct 2024 at 15:55, Tom Rini <trini@konsulko.com> wrote: > > > > On Mon, Oct 14, 2024 at 03:50:51PM -0600, Simon Glass wrote: > > > Hi Tom, > > > > > > On Mon, 14 Oct 2024 at 15:42, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > On Mon, Oct 14, 2024 at 02:19:56PM -0600, Simon Glass wrote: > > > > > Hi, > > > > > > > > > > On Fri, 11 Oct 2024 at 23:47, Ilias Apalodimas > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > Hi Tom > > > > > > > > > > > > On Sat, 12 Oct 2024 at 04:27, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > > > > > > > On Sat, Oct 12, 2024 at 12:01:54AM +0200, Heinrich Schuchardt wrote: > > > > > > > > > > > > > > > > > > > > > > > > Am 11. Oktober 2024 23:21:25 MESZ schrieb Simon Glass <sjg@chromium.org>: > > > > > > > > >The 'point of cooperation' is where U-Boot starts allowing EFI to use > > > > > > > > >memory outside of the U-Boot region. Until that point, it is desirable > > > > > > > > >to keep more below U-Boot free for loading images. > > > > > > > > > > > > > > > > > >Reserve a small region for this purpose. > > > > > > > > > > > > > > > > Your commit message provides no clue why this should be needed. > > > > > > > > > > Yes, I hadn't realised that people didn't understand what I was > > > > > getting at. Tom asked the same question on irc. > > > > > > > > > > It allows us to separate the lmb allocations from the EFI allocations, > > > > > so we don't need to have them both in sync. We use lmb for loading > > > > > images, then EFI takes over and does what it likes, respecting the > > > > > existing lmb reservations. > > > > > > > > But, again, LMB is not for loading of images. It's for dealing with > > > > memory reservations of various types. > > > > > > Up until recently, I believe, my statement was true, but in any case, > > > this isn't gemaine to the issue here. > > > > > > The point is, this is a useful distinction, allowing us to avoid the > > > complexity of keeping them in sync, and avoid putting this pain on > > > people who are not using EFI. We are either in U-Boot code, in which > > > case lmb rules, or we are going into an app, in which case EFI rules > > > (but must read in the lmb info). > > > > > > We are going to end up with people turning off EFI_LOADER because it > > > behaves so badly. > > > > Frankly, in hind sight I should not have agreed to split the LMB rework > > between "everything else" and "EFI" in hopes that we would then be able > > to get the "EFI" part of this agreed upon. It's "behaving badly" right > > now because we merged half of the changes in the hopes that we could get > > your agreement on the rest of them fairly quickly. This is not > > happening, however. > > Well, another option would be to revert the problem patch (or two) and > do this work with careful consideration of the impacts, taking account > of my architectural objections. Well, no. You have objections that no one else has agreed with and most of the active developers in the areas in question have objections to your designs. So no, I repeat myself. My mistake here was taking half of the work in, rather than all of the work in.
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index d6c15e2c406..20705be14dd 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -443,6 +443,12 @@ struct global_data { */ struct upl *upl; #endif +#if CONFIG_IS_ENABLED(EFI_LOADER) + /** + * @efi_region: Start of EFI's early-memory region + */ + ulong efi_region; +#endif }; #ifndef DO_DEPS_ONLY static_assert(sizeof(struct global_data) == GD_SIZE); diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 50cb2f3898b..9cc33397371 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -24,6 +24,12 @@ DECLARE_GLOBAL_DATA_PTR; /* Magic number identifying memory allocated from pool */ #define EFI_ALLOC_POOL_MAGIC 0x1fe67ddf6491caa2 +/* + * Amount of memory to reserve for EFI before relocation. This must be a + * multiple of EFI_PAGE_SIZE + */ +#define EFI_EARLY_REGION_SIZE SZ_256K + efi_uintn_t efi_memory_map_key; struct efi_mem_list { @@ -944,3 +950,18 @@ int efi_memory_init(void) return 0; } + +static int reserve_efi_region(void) +{ + /* + * Reserve some memory for EFI. Since pool allocations consume 4KB each + * and there are three allocations, allow 16KB of memory, enough for + * four. This can be increased as needed. + */ + gd->efi_region = ALIGN_DOWN(gd->start_addr_sp - EFI_EARLY_REGION_SIZE, + SZ_4K); + gd->start_addr_sp = gd->efi_region; + + return 0; +} +EVENT_SPY_SIMPLE(EVT_RESERVE, reserve_efi_region); diff --git a/test/py/tests/test_event_dump.py b/test/py/tests/test_event_dump.py index e282c67335c..e36fc6a1586 100644 --- a/test/py/tests/test_event_dump.py +++ b/test/py/tests/test_event_dump.py @@ -20,5 +20,6 @@ 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 install_smbios_table .*lib/efi_loader/efi_smbios.c:.* EVT_MISC_INIT_F sandbox_early_getopt_check .*arch/sandbox/cpu/start.c:.* +EVT_RESERVE reserve_efi_region .*lib/efi_loader/efi_memory.c:.* EVT_TEST h_adder_simple .*test/common/event.c:''' assert re.match(expect, out, re.MULTILINE) is not None
The 'point of cooperation' is where U-Boot starts allowing EFI to use memory outside of the U-Boot region. Until that point, it is desirable to keep more below U-Boot free for loading images. Reserve a small region for this purpose. Signed-off-by: Simon Glass <sjg@chromium.org> --- (no changes since v1) include/asm-generic/global_data.h | 6 ++++++ lib/efi_loader/efi_memory.c | 21 +++++++++++++++++++++ test/py/tests/test_event_dump.py | 1 + 3 files changed, 28 insertions(+)