diff mbox series

[RFC,v2,41/48] efi_memory: add an event handler to update memory map

Message ID 20240704073544.670249-42-sughosh.ganu@linaro.org
State Superseded
Headers show
Series Make U-Boot memory reservations coherent | expand

Commit Message

Sughosh Ganu July 4, 2024, 7:35 a.m. UTC
There are events that would be used to notify other interested modules
of any changes in available and occupied memory. This would happen
when a module allocates or reserves memory, or frees up memory. These
changes in memory map should be notified to other interested modules
so that the allocated memory does not get overwritten. Add an event
handler in the EFI memory module to update the EFI memory map
accordingly when such changes happen. As a consequence, any subsequent
memory request would honour the updated memory map and only available
memory would be allocated from.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V1:
* Handle the addition of memory to the LMB memory map.
* Pass the overlap_only_ram parameter to the efi_add_memory_map_pg()
  based on the type of operation.

 lib/efi_loader/Kconfig      |  1 +
 lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

Comments

Simon Glass July 13, 2024, 3:16 p.m. UTC | #1
Hi Sughosh,

On Thu, 4 Jul 2024 at 08:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> There are events that would be used to notify other interested modules
> of any changes in available and occupied memory. This would happen
> when a module allocates or reserves memory, or frees up memory. These
> changes in memory map should be notified to other interested modules
> so that the allocated memory does not get overwritten. Add an event
> handler in the EFI memory module to update the EFI memory map
> accordingly when such changes happen. As a consequence, any subsequent
> memory request would honour the updated memory map and only available
> memory would be allocated from.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> Changes since V1:
> * Handle the addition of memory to the LMB memory map.
> * Pass the overlap_only_ram parameter to the efi_add_memory_map_pg()
>   based on the type of operation.
>
>  lib/efi_loader/Kconfig      |  1 +
>  lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
>

This is getting complicated and I don't believe it is needed.

EFI should not be allocating memory 'in free space' until it starts
up. For the very few (if any) cases where it does, it can do an lmb
allocation.

As to the lmb allocations themselves, EFI can simply call look through
the lmb list and call efi_add_memory_map_pg() for each entry, when it
is ready to boot. There is no need to do it earlier.

> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index bdf5732974..2d90bcef2f 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -16,6 +16,7 @@ config EFI_LOADER
>         select CHARSET
>         # We need to send DM events, dynamically, in the EFI block driver
>         select DM_EVENT
> +       select EVENT
>         select EVENT_DYNAMIC
>         select LIB_UUID
>         select LMB
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 5691b5da03..bd12504f72 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -45,6 +45,10 @@ static LIST_HEAD(efi_mem);
>  void *efi_bounce_buffer;
>  #endif
>
> +#define MAP_OP_RESERVE         (u8)0x1
> +#define MAP_OP_FREE            (u8)0x2
> +#define MAP_OP_ADD             (u8)0x3
> +
>  /**
>   * struct efi_pool_allocation - memory block allocated from pool
>   *
> @@ -928,3 +932,33 @@ int efi_memory_init(void)
>
>         return 0;
>  }
> +
> +#if CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)
> +static int lmb_mem_map_update_sync(void *ctx, struct event *event)
> +{
> +       u8 op;
> +       u64 addr;
> +       u64 pages;
> +       efi_status_t status;
> +       struct event_lmb_map_update *lmb_map = &event->data.lmb_map;
> +
> +       addr = (uintptr_t)map_sysmem(lmb_map->base, 0);
> +       pages = efi_size_in_pages(lmb_map->size + (addr & EFI_PAGE_MASK));
> +       op = lmb_map->op;
> +       addr &= ~EFI_PAGE_MASK;
> +
> +       if (op != MAP_OP_RESERVE && op != MAP_OP_FREE && op != MAP_OP_ADD) {
> +               log_debug("Invalid map update op received (%d)\n", op);
> +               return -1;
> +       }
> +
> +       status = efi_add_memory_map_pg(addr, pages,
> +                                      op == MAP_OP_RESERVE ?
> +                                      EFI_BOOT_SERVICES_DATA :
> +                                      EFI_CONVENTIONAL_MEMORY,
> +                                      op == MAP_OP_RESERVE ? true : false);
> +
> +       return status == EFI_SUCCESS ? 0 : -1;
> +}
> +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync);
> +#endif /* MEM_MAP_UPDATE_NOTIFY */
> --
> 2.34.1
>

Regards,
Simon
Sughosh Ganu July 15, 2024, 9:39 a.m. UTC | #2
hi Simon,

On Sat, 13 Jul 2024 at 20:46, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Thu, 4 Jul 2024 at 08:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > There are events that would be used to notify other interested modules
> > of any changes in available and occupied memory. This would happen
> > when a module allocates or reserves memory, or frees up memory. These
> > changes in memory map should be notified to other interested modules
> > so that the allocated memory does not get overwritten. Add an event
> > handler in the EFI memory module to update the EFI memory map
> > accordingly when such changes happen. As a consequence, any subsequent
> > memory request would honour the updated memory map and only available
> > memory would be allocated from.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> > Changes since V1:
> > * Handle the addition of memory to the LMB memory map.
> > * Pass the overlap_only_ram parameter to the efi_add_memory_map_pg()
> >   based on the type of operation.
> >
> >  lib/efi_loader/Kconfig      |  1 +
> >  lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> >
>
> This is getting complicated and I don't believe it is needed.
>
> EFI should not be allocating memory 'in free space' until it starts
> up. For the very few (if any) cases where it does, it can do an lmb
> allocation.

EFI memory module is not allocating memory at all now. This patch is
adding an event handler for updating the EFI memory map, whenever the
LMB memory map changes. All the EFI allocations are now being routed
through the LMB API's.

>
> As to the lmb allocations themselves, EFI can simply call look through
> the lmb list and call efi_add_memory_map_pg() for each entry, when it
> is ready to boot. There is no need to do it earlier.

So in this case, I believe that rather than adding code in multiple
places where the EFI memory module would have to get the LMB map and
then update it's own, I think it is easier to update the EFI memory
map as and when the LMB map gets updated. Else, we have a scenario
where the EFI memory map would have to be updated as part of the EFI
memory map dump function, as well as before the EFI boot. Any new code
that would be subsequently introduced that might have a similar
requirement would then be needed to keep this point in mind(to get the
memory map from LMB).

This is not an OS file-system kind of an operation where performance
is critical, nor is this event(LMB memory map update) going to happen
very frequently. So I believe that it would be better to keep the EFI
memory map updated along with the LMB one.

-sughosh

>
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index bdf5732974..2d90bcef2f 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -16,6 +16,7 @@ config EFI_LOADER
> >         select CHARSET
> >         # We need to send DM events, dynamically, in the EFI block driver
> >         select DM_EVENT
> > +       select EVENT
> >         select EVENT_DYNAMIC
> >         select LIB_UUID
> >         select LMB
> > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > index 5691b5da03..bd12504f72 100644
> > --- a/lib/efi_loader/efi_memory.c
> > +++ b/lib/efi_loader/efi_memory.c
> > @@ -45,6 +45,10 @@ static LIST_HEAD(efi_mem);
> >  void *efi_bounce_buffer;
> >  #endif
> >
> > +#define MAP_OP_RESERVE         (u8)0x1
> > +#define MAP_OP_FREE            (u8)0x2
> > +#define MAP_OP_ADD             (u8)0x3
> > +
> >  /**
> >   * struct efi_pool_allocation - memory block allocated from pool
> >   *
> > @@ -928,3 +932,33 @@ int efi_memory_init(void)
> >
> >         return 0;
> >  }
> > +
> > +#if CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)
> > +static int lmb_mem_map_update_sync(void *ctx, struct event *event)
> > +{
> > +       u8 op;
> > +       u64 addr;
> > +       u64 pages;
> > +       efi_status_t status;
> > +       struct event_lmb_map_update *lmb_map = &event->data.lmb_map;
> > +
> > +       addr = (uintptr_t)map_sysmem(lmb_map->base, 0);
> > +       pages = efi_size_in_pages(lmb_map->size + (addr & EFI_PAGE_MASK));
> > +       op = lmb_map->op;
> > +       addr &= ~EFI_PAGE_MASK;
> > +
> > +       if (op != MAP_OP_RESERVE && op != MAP_OP_FREE && op != MAP_OP_ADD) {
> > +               log_debug("Invalid map update op received (%d)\n", op);
> > +               return -1;
> > +       }
> > +
> > +       status = efi_add_memory_map_pg(addr, pages,
> > +                                      op == MAP_OP_RESERVE ?
> > +                                      EFI_BOOT_SERVICES_DATA :
> > +                                      EFI_CONVENTIONAL_MEMORY,
> > +                                      op == MAP_OP_RESERVE ? true : false);
> > +
> > +       return status == EFI_SUCCESS ? 0 : -1;
> > +}
> > +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync);
> > +#endif /* MEM_MAP_UPDATE_NOTIFY */
> > --
> > 2.34.1
> >
>
> Regards,
> Simon
Simon Glass July 15, 2024, 11:39 a.m. UTC | #3
Hi Sughosh,

On Mon, 15 Jul 2024 at 10:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Sat, 13 Jul 2024 at 20:46, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Thu, 4 Jul 2024 at 08:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > There are events that would be used to notify other interested modules
> > > of any changes in available and occupied memory. This would happen
> > > when a module allocates or reserves memory, or frees up memory. These
> > > changes in memory map should be notified to other interested modules
> > > so that the allocated memory does not get overwritten. Add an event
> > > handler in the EFI memory module to update the EFI memory map
> > > accordingly when such changes happen. As a consequence, any subsequent
> > > memory request would honour the updated memory map and only available
> > > memory would be allocated from.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > > Changes since V1:
> > > * Handle the addition of memory to the LMB memory map.
> > > * Pass the overlap_only_ram parameter to the efi_add_memory_map_pg()
> > >   based on the type of operation.
> > >
> > >  lib/efi_loader/Kconfig      |  1 +
> > >  lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
> > >  2 files changed, 35 insertions(+)
> > >
> >
> > This is getting complicated and I don't believe it is needed.
> >
> > EFI should not be allocating memory 'in free space' until it starts
> > up. For the very few (if any) cases where it does, it can do an lmb
> > allocation.
>
> EFI memory module is not allocating memory at all now. This patch is
> adding an event handler for updating the EFI memory map, whenever the
> LMB memory map changes. All the EFI allocations are now being routed
> through the LMB API's.

OK

>
> >
> > As to the lmb allocations themselves, EFI can simply call look through
> > the lmb list and call efi_add_memory_map_pg() for each entry, when it
> > is ready to boot. There is no need to do it earlier.
>
> So in this case, I believe that rather than adding code in multiple
> places where the EFI memory module would have to get the LMB map and
> then update it's own, I think it is easier to update the EFI memory
> map as and when the LMB map gets updated. Else, we have a scenario
> where the EFI memory map would have to be updated as part of the EFI
> memory map dump function, as well as before the EFI boot. Any new code
> that would be subsequently introduced that might have a similar
> requirement would then be needed to keep this point in mind(to get the
> memory map from LMB).

That doesn't hold water in my eyes. I actually like the idea of the
EFI memory map being set up before booting. It should be done in a
single function called from one place, just before booting. Well, I
suppose it could be called from the memory-map-dump function too. But
it should be pretty simple...just add some pre-defined things and then
add the lmb records. You can even write a unit test for it.

>
> This is not an OS file-system kind of an operation where performance
> is critical, nor is this event(LMB memory map update) going to happen
> very frequently. So I believe that it would be better to keep the EFI
> memory map updated along with the LMB one.

I really don't like that idea at all. One table is enough for use by
U-Boot. The EFI one is needed for booting. Keeping them in sync as
U-Boot is running is not necessary, just invites bugs and makes the
whole thing harder to test.

Regards,
Simon
Tom Rini July 15, 2024, 7:05 p.m. UTC | #4
On Mon, Jul 15, 2024 at 12:39:32PM +0100, Simon Glass wrote:
> Hi Sughosh,
> 
> On Mon, 15 Jul 2024 at 10:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > hi Simon,
> >
> > On Sat, 13 Jul 2024 at 20:46, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Thu, 4 Jul 2024 at 08:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > There are events that would be used to notify other interested modules
> > > > of any changes in available and occupied memory. This would happen
> > > > when a module allocates or reserves memory, or frees up memory. These
> > > > changes in memory map should be notified to other interested modules
> > > > so that the allocated memory does not get overwritten. Add an event
> > > > handler in the EFI memory module to update the EFI memory map
> > > > accordingly when such changes happen. As a consequence, any subsequent
> > > > memory request would honour the updated memory map and only available
> > > > memory would be allocated from.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > ---
> > > > Changes since V1:
> > > > * Handle the addition of memory to the LMB memory map.
> > > > * Pass the overlap_only_ram parameter to the efi_add_memory_map_pg()
> > > >   based on the type of operation.
> > > >
> > > >  lib/efi_loader/Kconfig      |  1 +
> > > >  lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
> > > >  2 files changed, 35 insertions(+)
> > > >
> > >
> > > This is getting complicated and I don't believe it is needed.
> > >
> > > EFI should not be allocating memory 'in free space' until it starts
> > > up. For the very few (if any) cases where it does, it can do an lmb
> > > allocation.
> >
> > EFI memory module is not allocating memory at all now. This patch is
> > adding an event handler for updating the EFI memory map, whenever the
> > LMB memory map changes. All the EFI allocations are now being routed
> > through the LMB API's.
> 
> OK
> 
> >
> > >
> > > As to the lmb allocations themselves, EFI can simply call look through
> > > the lmb list and call efi_add_memory_map_pg() for each entry, when it
> > > is ready to boot. There is no need to do it earlier.
> >
> > So in this case, I believe that rather than adding code in multiple
> > places where the EFI memory module would have to get the LMB map and
> > then update it's own, I think it is easier to update the EFI memory
> > map as and when the LMB map gets updated. Else, we have a scenario
> > where the EFI memory map would have to be updated as part of the EFI
> > memory map dump function, as well as before the EFI boot. Any new code
> > that would be subsequently introduced that might have a similar
> > requirement would then be needed to keep this point in mind(to get the
> > memory map from LMB).
> 
> That doesn't hold water in my eyes. I actually like the idea of the
> EFI memory map being set up before booting. It should be done in a
> single function called from one place, just before booting. Well, I
> suppose it could be called from the memory-map-dump function too. But
> it should be pretty simple...just add some pre-defined things and then
> add the lmb records. You can even write a unit test for it.
> 
> >
> > This is not an OS file-system kind of an operation where performance
> > is critical, nor is this event(LMB memory map update) going to happen
> > very frequently. So I believe that it would be better to keep the EFI
> > memory map updated along with the LMB one.
> 
> I really don't like that idea at all. One table is enough for use by
> U-Boot. The EFI one is needed for booting. Keeping them in sync as
> U-Boot is running is not necessary, just invites bugs and makes the
> whole thing harder to test.

Doesn't that ignore the issue of EFI being re-entrant to us? Or no,
because you're suggesting we only update the EFI map before entering the
EFI loader, not strictly "booting the OS"? In which case, maybe that
does end up being both cleaner and smaller? I'm not sure.
Sughosh Ganu July 16, 2024, 6:25 a.m. UTC | #5
On Tue, 16 Jul 2024 at 00:35, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Jul 15, 2024 at 12:39:32PM +0100, Simon Glass wrote:
> > Hi Sughosh,
> >
> > On Mon, 15 Jul 2024 at 10:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Sat, 13 Jul 2024 at 20:46, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Thu, 4 Jul 2024 at 08:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > There are events that would be used to notify other interested modules
> > > > > of any changes in available and occupied memory. This would happen
> > > > > when a module allocates or reserves memory, or frees up memory. These
> > > > > changes in memory map should be notified to other interested modules
> > > > > so that the allocated memory does not get overwritten. Add an event
> > > > > handler in the EFI memory module to update the EFI memory map
> > > > > accordingly when such changes happen. As a consequence, any subsequent
> > > > > memory request would honour the updated memory map and only available
> > > > > memory would be allocated from.
> > > > >
> > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > ---
> > > > > Changes since V1:
> > > > > * Handle the addition of memory to the LMB memory map.
> > > > > * Pass the overlap_only_ram parameter to the efi_add_memory_map_pg()
> > > > >   based on the type of operation.
> > > > >
> > > > >  lib/efi_loader/Kconfig      |  1 +
> > > > >  lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
> > > > >  2 files changed, 35 insertions(+)
> > > > >
> > > >
> > > > This is getting complicated and I don't believe it is needed.
> > > >
> > > > EFI should not be allocating memory 'in free space' until it starts
> > > > up. For the very few (if any) cases where it does, it can do an lmb
> > > > allocation.
> > >
> > > EFI memory module is not allocating memory at all now. This patch is
> > > adding an event handler for updating the EFI memory map, whenever the
> > > LMB memory map changes. All the EFI allocations are now being routed
> > > through the LMB API's.
> >
> > OK
> >
> > >
> > > >
> > > > As to the lmb allocations themselves, EFI can simply call look through
> > > > the lmb list and call efi_add_memory_map_pg() for each entry, when it
> > > > is ready to boot. There is no need to do it earlier.
> > >
> > > So in this case, I believe that rather than adding code in multiple
> > > places where the EFI memory module would have to get the LMB map and
> > > then update it's own, I think it is easier to update the EFI memory
> > > map as and when the LMB map gets updated. Else, we have a scenario
> > > where the EFI memory map would have to be updated as part of the EFI
> > > memory map dump function, as well as before the EFI boot. Any new code
> > > that would be subsequently introduced that might have a similar
> > > requirement would then be needed to keep this point in mind(to get the
> > > memory map from LMB).
> >
> > That doesn't hold water in my eyes. I actually like the idea of the
> > EFI memory map being set up before booting. It should be done in a
> > single function called from one place, just before booting. Well, I
> > suppose it could be called from the memory-map-dump function too. But
> > it should be pretty simple...just add some pre-defined things and then
> > add the lmb records. You can even write a unit test for it.
> >
> > >
> > > This is not an OS file-system kind of an operation where performance
> > > is critical, nor is this event(LMB memory map update) going to happen
> > > very frequently. So I believe that it would be better to keep the EFI
> > > memory map updated along with the LMB one.
> >
> > I really don't like that idea at all. One table is enough for use by
> > U-Boot. The EFI one is needed for booting. Keeping them in sync as
> > U-Boot is running is not necessary, just invites bugs and makes the
> > whole thing harder to test.
>
> Doesn't that ignore the issue of EFI being re-entrant to us? Or no,
> because you're suggesting we only update the EFI map before entering the
> EFI loader, not strictly "booting the OS"? In which case, maybe that
> does end up being both cleaner and smaller? I'm not sure.

And that is my concern here about having to update the EFI map at
multiple call points, instead of keeping it updated -- I feel this is
more prone to being buggy. And to add to this, there might be code
paths added subsequently which might need an updated EFI map where
this gets missed. The only downside to the current design is that it
might be slower since the EFI map gets updated on every change to the
LMB map. But I am not sure if the LMB map changes are going to be that
frequent.

-sughosh

>
> --
> Tom
Simon Glass July 16, 2024, 7:09 a.m. UTC | #6
Hi Sughosh,

On Tue, 16 Jul 2024 at 07:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Tue, 16 Jul 2024 at 00:35, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Jul 15, 2024 at 12:39:32PM +0100, Simon Glass wrote:
> > > Hi Sughosh,
> > >
> > > On Mon, 15 Jul 2024 at 10:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Sat, 13 Jul 2024 at 20:46, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Thu, 4 Jul 2024 at 08:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > There are events that would be used to notify other interested modules
> > > > > > of any changes in available and occupied memory. This would happen
> > > > > > when a module allocates or reserves memory, or frees up memory. These
> > > > > > changes in memory map should be notified to other interested modules
> > > > > > so that the allocated memory does not get overwritten. Add an event
> > > > > > handler in the EFI memory module to update the EFI memory map
> > > > > > accordingly when such changes happen. As a consequence, any subsequent
> > > > > > memory request would honour the updated memory map and only available
> > > > > > memory would be allocated from.
> > > > > >
> > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > ---
> > > > > > Changes since V1:
> > > > > > * Handle the addition of memory to the LMB memory map.
> > > > > > * Pass the overlap_only_ram parameter to the efi_add_memory_map_pg()
> > > > > >   based on the type of operation.
> > > > > >
> > > > > >  lib/efi_loader/Kconfig      |  1 +
> > > > > >  lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
> > > > > >  2 files changed, 35 insertions(+)
> > > > > >
> > > > >
> > > > > This is getting complicated and I don't believe it is needed.
> > > > >
> > > > > EFI should not be allocating memory 'in free space' until it starts
> > > > > up. For the very few (if any) cases where it does, it can do an lmb
> > > > > allocation.
> > > >
> > > > EFI memory module is not allocating memory at all now. This patch is
> > > > adding an event handler for updating the EFI memory map, whenever the
> > > > LMB memory map changes. All the EFI allocations are now being routed
> > > > through the LMB API's.
> > >
> > > OK
> > >
> > > >
> > > > >
> > > > > As to the lmb allocations themselves, EFI can simply call look through
> > > > > the lmb list and call efi_add_memory_map_pg() for each entry, when it
> > > > > is ready to boot. There is no need to do it earlier.
> > > >
> > > > So in this case, I believe that rather than adding code in multiple
> > > > places where the EFI memory module would have to get the LMB map and
> > > > then update it's own, I think it is easier to update the EFI memory
> > > > map as and when the LMB map gets updated. Else, we have a scenario
> > > > where the EFI memory map would have to be updated as part of the EFI
> > > > memory map dump function, as well as before the EFI boot. Any new code
> > > > that would be subsequently introduced that might have a similar
> > > > requirement would then be needed to keep this point in mind(to get the
> > > > memory map from LMB).
> > >
> > > That doesn't hold water in my eyes. I actually like the idea of the
> > > EFI memory map being set up before booting. It should be done in a
> > > single function called from one place, just before booting. Well, I
> > > suppose it could be called from the memory-map-dump function too. But
> > > it should be pretty simple...just add some pre-defined things and then
> > > add the lmb records. You can even write a unit test for it.
> > >
> > > >
> > > > This is not an OS file-system kind of an operation where performance
> > > > is critical, nor is this event(LMB memory map update) going to happen
> > > > very frequently. So I believe that it would be better to keep the EFI
> > > > memory map updated along with the LMB one.
> > >
> > > I really don't like that idea at all. One table is enough for use by
> > > U-Boot. The EFI one is needed for booting. Keeping them in sync as
> > > U-Boot is running is not necessary, just invites bugs and makes the
> > > whole thing harder to test.
> >
> > Doesn't that ignore the issue of EFI being re-entrant to us? Or no,
> > because you're suggesting we only update the EFI map before entering the
> > EFI loader, not strictly "booting the OS"? In which case, maybe that
> > does end up being both cleaner and smaller? I'm not sure.
>
> And that is my concern here about having to update the EFI map at
> multiple call points, instead of keeping it updated -- I feel this is
> more prone to being buggy. And to add to this, there might be code
> paths added subsequently which might need an updated EFI map where
> this gets missed. The only downside to the current design is that it
> might be slower since the EFI map gets updated on every change to the
> LMB map. But I am not sure if the LMB map changes are going to be that
> frequent.

I think it is worth exploring doing the conversion from lmb to EFI
once, in one place. We have this syncing problem, to some extent, with
partitions, which automatically update an EFI table when they are
bound. The design of EFI was somewhat confusing from the start, in
that it created its own tables to avoid relying on driver model. We
are still paying the cost of that design decision. If we are saying
that lmb holds the allocations, then it should be in lmb. The EFI ones
should just be a slave to that, in that it should be possible to throw
away the EFI ones and regenerate them from lmb.

Of course that isn't quite where we are today, but I bet it is close.

Regards,
Simon
Sughosh Ganu July 16, 2024, 8:35 a.m. UTC | #7
hi Simon,

On Tue, 16 Jul 2024 at 12:40, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Tue, 16 Jul 2024 at 07:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > On Tue, 16 Jul 2024 at 00:35, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Mon, Jul 15, 2024 at 12:39:32PM +0100, Simon Glass wrote:
> > > > Hi Sughosh,
> > > >
> > > > On Mon, 15 Jul 2024 at 10:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Sat, 13 Jul 2024 at 20:46, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Thu, 4 Jul 2024 at 08:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > > There are events that would be used to notify other interested modules
> > > > > > > of any changes in available and occupied memory. This would happen
> > > > > > > when a module allocates or reserves memory, or frees up memory. These
> > > > > > > changes in memory map should be notified to other interested modules
> > > > > > > so that the allocated memory does not get overwritten. Add an event
> > > > > > > handler in the EFI memory module to update the EFI memory map
> > > > > > > accordingly when such changes happen. As a consequence, any subsequent
> > > > > > > memory request would honour the updated memory map and only available
> > > > > > > memory would be allocated from.
> > > > > > >
> > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > > ---
> > > > > > > Changes since V1:
> > > > > > > * Handle the addition of memory to the LMB memory map.
> > > > > > > * Pass the overlap_only_ram parameter to the efi_add_memory_map_pg()
> > > > > > >   based on the type of operation.
> > > > > > >
> > > > > > >  lib/efi_loader/Kconfig      |  1 +
> > > > > > >  lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
> > > > > > >  2 files changed, 35 insertions(+)
> > > > > > >
> > > > > >
> > > > > > This is getting complicated and I don't believe it is needed.
> > > > > >
> > > > > > EFI should not be allocating memory 'in free space' until it starts
> > > > > > up. For the very few (if any) cases where it does, it can do an lmb
> > > > > > allocation.
> > > > >
> > > > > EFI memory module is not allocating memory at all now. This patch is
> > > > > adding an event handler for updating the EFI memory map, whenever the
> > > > > LMB memory map changes. All the EFI allocations are now being routed
> > > > > through the LMB API's.
> > > >
> > > > OK
> > > >
> > > > >
> > > > > >
> > > > > > As to the lmb allocations themselves, EFI can simply call look through
> > > > > > the lmb list and call efi_add_memory_map_pg() for each entry, when it
> > > > > > is ready to boot. There is no need to do it earlier.
> > > > >
> > > > > So in this case, I believe that rather than adding code in multiple
> > > > > places where the EFI memory module would have to get the LMB map and
> > > > > then update it's own, I think it is easier to update the EFI memory
> > > > > map as and when the LMB map gets updated. Else, we have a scenario
> > > > > where the EFI memory map would have to be updated as part of the EFI
> > > > > memory map dump function, as well as before the EFI boot. Any new code
> > > > > that would be subsequently introduced that might have a similar
> > > > > requirement would then be needed to keep this point in mind(to get the
> > > > > memory map from LMB).
> > > >
> > > > That doesn't hold water in my eyes. I actually like the idea of the
> > > > EFI memory map being set up before booting. It should be done in a
> > > > single function called from one place, just before booting. Well, I
> > > > suppose it could be called from the memory-map-dump function too. But
> > > > it should be pretty simple...just add some pre-defined things and then
> > > > add the lmb records. You can even write a unit test for it.
> > > >
> > > > >
> > > > > This is not an OS file-system kind of an operation where performance
> > > > > is critical, nor is this event(LMB memory map update) going to happen
> > > > > very frequently. So I believe that it would be better to keep the EFI
> > > > > memory map updated along with the LMB one.
> > > >
> > > > I really don't like that idea at all. One table is enough for use by
> > > > U-Boot. The EFI one is needed for booting. Keeping them in sync as
> > > > U-Boot is running is not necessary, just invites bugs and makes the
> > > > whole thing harder to test.
> > >
> > > Doesn't that ignore the issue of EFI being re-entrant to us? Or no,
> > > because you're suggesting we only update the EFI map before entering the
> > > EFI loader, not strictly "booting the OS"? In which case, maybe that
> > > does end up being both cleaner and smaller? I'm not sure.
> >
> > And that is my concern here about having to update the EFI map at
> > multiple call points, instead of keeping it updated -- I feel this is
> > more prone to being buggy. And to add to this, there might be code
> > paths added subsequently which might need an updated EFI map where
> > this gets missed. The only downside to the current design is that it
> > might be slower since the EFI map gets updated on every change to the
> > LMB map. But I am not sure if the LMB map changes are going to be that
> > frequent.
>
> I think it is worth exploring doing the conversion from lmb to EFI
> once, in one place. We have this syncing problem, to some extent, with
> partitions, which automatically update an EFI table when they are
> bound. The design of EFI was somewhat confusing from the start, in
> that it created its own tables to avoid relying on driver model. We
> are still paying the cost of that design decision. If we are saying
> that lmb holds the allocations, then it should be in lmb. The EFI ones
> should just be a slave to that, in that it should be possible to throw
> away the EFI ones and regenerate them from lmb.

So if we talk specifically about maintaining the EFI memory map, what
this patch does is exactly on the lines that you are referring to --
the memory map is being maintained by lmb, and simply notified to the
EFI memory module. The EFI memory map, when it comes to ram memory, is
indeed being made dependent on lmb. I think the only question is, how
does EFI get this information from lmb. This patch is notifying the
EFI memory module as and when the change happens in the lmb memory
map. I think what you are suggesting is that this information should
be obtained on a need basis? And I have listed possible issues that
could crop up with that approach.

-sughosh

>
> Of course that isn't quite where we are today, but I bet it is close.
>
> Regards,
> Simon
Tom Rini July 16, 2024, 5 p.m. UTC | #8
On Tue, Jul 16, 2024 at 11:55:10AM +0530, Sughosh Ganu wrote:
> On Tue, 16 Jul 2024 at 00:35, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Jul 15, 2024 at 12:39:32PM +0100, Simon Glass wrote:
> > > Hi Sughosh,
> > >
> > > On Mon, 15 Jul 2024 at 10:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > >
> > > > hi Simon,
> > > >
> > > > On Sat, 13 Jul 2024 at 20:46, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Sughosh,
> > > > >
> > > > > On Thu, 4 Jul 2024 at 08:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > >
> > > > > > There are events that would be used to notify other interested modules
> > > > > > of any changes in available and occupied memory. This would happen
> > > > > > when a module allocates or reserves memory, or frees up memory. These
> > > > > > changes in memory map should be notified to other interested modules
> > > > > > so that the allocated memory does not get overwritten. Add an event
> > > > > > handler in the EFI memory module to update the EFI memory map
> > > > > > accordingly when such changes happen. As a consequence, any subsequent
> > > > > > memory request would honour the updated memory map and only available
> > > > > > memory would be allocated from.
> > > > > >
> > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > ---
> > > > > > Changes since V1:
> > > > > > * Handle the addition of memory to the LMB memory map.
> > > > > > * Pass the overlap_only_ram parameter to the efi_add_memory_map_pg()
> > > > > >   based on the type of operation.
> > > > > >
> > > > > >  lib/efi_loader/Kconfig      |  1 +
> > > > > >  lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
> > > > > >  2 files changed, 35 insertions(+)
> > > > > >
> > > > >
> > > > > This is getting complicated and I don't believe it is needed.
> > > > >
> > > > > EFI should not be allocating memory 'in free space' until it starts
> > > > > up. For the very few (if any) cases where it does, it can do an lmb
> > > > > allocation.
> > > >
> > > > EFI memory module is not allocating memory at all now. This patch is
> > > > adding an event handler for updating the EFI memory map, whenever the
> > > > LMB memory map changes. All the EFI allocations are now being routed
> > > > through the LMB API's.
> > >
> > > OK
> > >
> > > >
> > > > >
> > > > > As to the lmb allocations themselves, EFI can simply call look through
> > > > > the lmb list and call efi_add_memory_map_pg() for each entry, when it
> > > > > is ready to boot. There is no need to do it earlier.
> > > >
> > > > So in this case, I believe that rather than adding code in multiple
> > > > places where the EFI memory module would have to get the LMB map and
> > > > then update it's own, I think it is easier to update the EFI memory
> > > > map as and when the LMB map gets updated. Else, we have a scenario
> > > > where the EFI memory map would have to be updated as part of the EFI
> > > > memory map dump function, as well as before the EFI boot. Any new code
> > > > that would be subsequently introduced that might have a similar
> > > > requirement would then be needed to keep this point in mind(to get the
> > > > memory map from LMB).
> > >
> > > That doesn't hold water in my eyes. I actually like the idea of the
> > > EFI memory map being set up before booting. It should be done in a
> > > single function called from one place, just before booting. Well, I
> > > suppose it could be called from the memory-map-dump function too. But
> > > it should be pretty simple...just add some pre-defined things and then
> > > add the lmb records. You can even write a unit test for it.
> > >
> > > >
> > > > This is not an OS file-system kind of an operation where performance
> > > > is critical, nor is this event(LMB memory map update) going to happen
> > > > very frequently. So I believe that it would be better to keep the EFI
> > > > memory map updated along with the LMB one.
> > >
> > > I really don't like that idea at all. One table is enough for use by
> > > U-Boot. The EFI one is needed for booting. Keeping them in sync as
> > > U-Boot is running is not necessary, just invites bugs and makes the
> > > whole thing harder to test.
> >
> > Doesn't that ignore the issue of EFI being re-entrant to us? Or no,
> > because you're suggesting we only update the EFI map before entering the
> > EFI loader, not strictly "booting the OS"? In which case, maybe that
> > does end up being both cleaner and smaller? I'm not sure.
> 
> And that is my concern here about having to update the EFI map at
> multiple call points, instead of keeping it updated -- I feel this is
> more prone to being buggy. And to add to this, there might be code
> paths added subsequently which might need an updated EFI map where
> this gets missed. The only downside to the current design is that it
> might be slower since the EFI map gets updated on every change to the
> LMB map. But I am not sure if the LMB map changes are going to be that
> frequent.

To me the question really is, do we have a single entry point to the EFI
loader that must always be used (and is before EFI loader must know for
sure it has up to date memory reservations) or are there two or more
points? If there's two or more points then yes, your current approach is
best as we don't want to introduce problems down the line. If there's
one (which is what I hope), then we can just make that entry point be
where the resync happens.
Sughosh Ganu July 17, 2024, 7:58 a.m. UTC | #9
On Tue, 16 Jul 2024 at 22:30, Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Jul 16, 2024 at 11:55:10AM +0530, Sughosh Ganu wrote:
> > On Tue, 16 Jul 2024 at 00:35, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Mon, Jul 15, 2024 at 12:39:32PM +0100, Simon Glass wrote:
> > > > Hi Sughosh,
> > > >
> > > > On Mon, 15 Jul 2024 at 10:39, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Sat, 13 Jul 2024 at 20:46, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Thu, 4 Jul 2024 at 08:38, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > > > > > >
> > > > > > > There are events that would be used to notify other interested modules
> > > > > > > of any changes in available and occupied memory. This would happen
> > > > > > > when a module allocates or reserves memory, or frees up memory. These
> > > > > > > changes in memory map should be notified to other interested modules
> > > > > > > so that the allocated memory does not get overwritten. Add an event
> > > > > > > handler in the EFI memory module to update the EFI memory map
> > > > > > > accordingly when such changes happen. As a consequence, any subsequent
> > > > > > > memory request would honour the updated memory map and only available
> > > > > > > memory would be allocated from.
> > > > > > >
> > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > > > > > ---
> > > > > > > Changes since V1:
> > > > > > > * Handle the addition of memory to the LMB memory map.
> > > > > > > * Pass the overlap_only_ram parameter to the efi_add_memory_map_pg()
> > > > > > >   based on the type of operation.
> > > > > > >
> > > > > > >  lib/efi_loader/Kconfig      |  1 +
> > > > > > >  lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
> > > > > > >  2 files changed, 35 insertions(+)
> > > > > > >
> > > > > >
> > > > > > This is getting complicated and I don't believe it is needed.
> > > > > >
> > > > > > EFI should not be allocating memory 'in free space' until it starts
> > > > > > up. For the very few (if any) cases where it does, it can do an lmb
> > > > > > allocation.
> > > > >
> > > > > EFI memory module is not allocating memory at all now. This patch is
> > > > > adding an event handler for updating the EFI memory map, whenever the
> > > > > LMB memory map changes. All the EFI allocations are now being routed
> > > > > through the LMB API's.
> > > >
> > > > OK
> > > >
> > > > >
> > > > > >
> > > > > > As to the lmb allocations themselves, EFI can simply call look through
> > > > > > the lmb list and call efi_add_memory_map_pg() for each entry, when it
> > > > > > is ready to boot. There is no need to do it earlier.
> > > > >
> > > > > So in this case, I believe that rather than adding code in multiple
> > > > > places where the EFI memory module would have to get the LMB map and
> > > > > then update it's own, I think it is easier to update the EFI memory
> > > > > map as and when the LMB map gets updated. Else, we have a scenario
> > > > > where the EFI memory map would have to be updated as part of the EFI
> > > > > memory map dump function, as well as before the EFI boot. Any new code
> > > > > that would be subsequently introduced that might have a similar
> > > > > requirement would then be needed to keep this point in mind(to get the
> > > > > memory map from LMB).
> > > >
> > > > That doesn't hold water in my eyes. I actually like the idea of the
> > > > EFI memory map being set up before booting. It should be done in a
> > > > single function called from one place, just before booting. Well, I
> > > > suppose it could be called from the memory-map-dump function too. But
> > > > it should be pretty simple...just add some pre-defined things and then
> > > > add the lmb records. You can even write a unit test for it.
> > > >
> > > > >
> > > > > This is not an OS file-system kind of an operation where performance
> > > > > is critical, nor is this event(LMB memory map update) going to happen
> > > > > very frequently. So I believe that it would be better to keep the EFI
> > > > > memory map updated along with the LMB one.
> > > >
> > > > I really don't like that idea at all. One table is enough for use by
> > > > U-Boot. The EFI one is needed for booting. Keeping them in sync as
> > > > U-Boot is running is not necessary, just invites bugs and makes the
> > > > whole thing harder to test.
> > >
> > > Doesn't that ignore the issue of EFI being re-entrant to us? Or no,
> > > because you're suggesting we only update the EFI map before entering the
> > > EFI loader, not strictly "booting the OS"? In which case, maybe that
> > > does end up being both cleaner and smaller? I'm not sure.
> >
> > And that is my concern here about having to update the EFI map at
> > multiple call points, instead of keeping it updated -- I feel this is
> > more prone to being buggy. And to add to this, there might be code
> > paths added subsequently which might need an updated EFI map where
> > this gets missed. The only downside to the current design is that it
> > might be slower since the EFI map gets updated on every change to the
> > LMB map. But I am not sure if the LMB map changes are going to be that
> > frequent.
>
> To me the question really is, do we have a single entry point to the EFI
> loader that must always be used (and is before EFI loader must know for
> sure it has up to date memory reservations) or are there two or more
> points? If there's two or more points then yes, your current approach is
> best as we don't want to introduce problems down the line. If there's
> one (which is what I hope), then we can just make that entry point be
> where the resync happens.

There are a couple of places where the memory map gets used, one from
the external get_memory_map API, and one from the 'efidebug memmap'
command. But they do call the same internal function to get the memory
map. So that function can be the common point of getting the current
memory map from lmb. This will mean that we expose the lmb structures
into the efi memory module. I will put out an implementation and we
can see about it at that point. This will be part of the second part
of the patches that I will be sending. Will work on getting the lmb
patches out first.

-sughosh
>
> --
> Tom
diff mbox series

Patch

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index bdf5732974..2d90bcef2f 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -16,6 +16,7 @@  config EFI_LOADER
 	select CHARSET
 	# We need to send DM events, dynamically, in the EFI block driver
 	select DM_EVENT
+	select EVENT
 	select EVENT_DYNAMIC
 	select LIB_UUID
 	select LMB
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 5691b5da03..bd12504f72 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -45,6 +45,10 @@  static LIST_HEAD(efi_mem);
 void *efi_bounce_buffer;
 #endif
 
+#define MAP_OP_RESERVE		(u8)0x1
+#define MAP_OP_FREE		(u8)0x2
+#define MAP_OP_ADD		(u8)0x3
+
 /**
  * struct efi_pool_allocation - memory block allocated from pool
  *
@@ -928,3 +932,33 @@  int efi_memory_init(void)
 
 	return 0;
 }
+
+#if CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)
+static int lmb_mem_map_update_sync(void *ctx, struct event *event)
+{
+	u8 op;
+	u64 addr;
+	u64 pages;
+	efi_status_t status;
+	struct event_lmb_map_update *lmb_map = &event->data.lmb_map;
+
+	addr = (uintptr_t)map_sysmem(lmb_map->base, 0);
+	pages = efi_size_in_pages(lmb_map->size + (addr & EFI_PAGE_MASK));
+	op = lmb_map->op;
+	addr &= ~EFI_PAGE_MASK;
+
+	if (op != MAP_OP_RESERVE && op != MAP_OP_FREE && op != MAP_OP_ADD) {
+		log_debug("Invalid map update op received (%d)\n", op);
+		return -1;
+	}
+
+	status = efi_add_memory_map_pg(addr, pages,
+				       op == MAP_OP_RESERVE ?
+				       EFI_BOOT_SERVICES_DATA :
+				       EFI_CONVENTIONAL_MEMORY,
+				       op == MAP_OP_RESERVE ? true : false);
+
+	return status == EFI_SUCCESS ? 0 : -1;
+}
+EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync);
+#endif /* MEM_MAP_UPDATE_NOTIFY */