Hi Heinrich,
On Wed, 12 Jun 2024 at 00:11, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
>
>
>
> Am 12. Juni 2024 04:42:00 MESZ schrieb Simon Glass <s...@chromium.org>:
> >Hi Tom,
> >
> >On Tue, 11 Jun 2024 at 16:54, Tom Rini <tr...@konsulko.com> wrote:
> >>
> >> On Tue, Jun 11, 2024 at 04:22:25PM -0600, Simon Glass wrote:
> >> > Hi Tom,
> >> >
> >> > On Tue, 11 Jun 2024 at 15:01, Tom Rini <tr...@konsulko.com> wrote:
> >> > >
> >> > > On Tue, Jun 11, 2024 at 12:52:19PM -0600, Simon Glass wrote:
> >> > > > Hi,
> >> > > >
> >> > > > On Tue, 11 Jun 2024 at 08:36, Tom Rini <tr...@konsulko.com> wrote:
> >> > > > >
> >> > > > > On Tue, Jun 11, 2024 at 12:17:16PM +0200, Heinrich Schuchardt
> >> > > > > wrote:
> >> > > > > > On 07.06.24 20:52, Sughosh Ganu 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.g...@linaro.org>
> >> > > > > > > ---
> >> > > > > > > lib/efi_loader/efi_memory.c | 70
> >> > > > > > > ++++++++++++++++++++++++++++++-------
> >> > > > > > > 1 file changed, 58 insertions(+), 12 deletions(-)
> >> > > > > > >
> >> > > > > > > diff --git a/lib/efi_loader/efi_memory.c
> >> > > > > > > b/lib/efi_loader/efi_memory.c
> >> > > > > > > index 435e580fb3..93244161b0 100644
> >> > > > > > > --- a/lib/efi_loader/efi_memory.c
> >> > > > > > > +++ b/lib/efi_loader/efi_memory.c
> >> > > > > > > @@ -73,6 +73,10 @@ struct efi_pool_allocation {
> >> > > > > > > #if CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)
> >> > > > > > > extern bool is_addr_in_ram(uintptr_t addr);
> >> > > > > > >
> >> > > > > > > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64
> >> > > > > > > pages,
> >> > > > > > > + int memory_type,
> >> > > > > > > + bool overlap_only_ram);
> >> > > > > > > +
> >> > > > > > > static void efi_map_update_notify(u64 addr, u64 size, u8 op)
> >> > > > > > > {
> >> > > > > > > struct event_efi_mem_map_update efi_map = {0};
> >> > > > > > > @@ -84,6 +88,34 @@ static void efi_map_update_notify(u64 addr,
> >> > > > > > > u64 size, u8 op)
> >> > > > > > > if (is_addr_in_ram((uintptr_t)addr))
> >> > > > > > > event_notify(EVT_EFI_MEM_MAP_UPDATE, &efi_map,
> >> > > > > > > sizeof(efi_map));
> >> > > > > > > }
> >> > > > > > > +
> >> > > > > > > +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) {
> >> > > > > > > + log_debug("Invalid map update op received (%d)\n",
> >> > > > > > > op);
> >> > > > > > > + return -1;
> >> > > > > > > + }
> >> > > > > > > +
> >> > > > > > > + status = __efi_add_memory_map_pg(addr, pages,
> >> > > > > > > + op == MAP_OP_FREE ?
> >> > > > > > > + EFI_CONVENTIONAL_MEMORY :
> >> > > > > >
> >> > > > > > This is dangerous. LMB might turn memory that is marked as
> >> > > > > > EfiReservedMemory which the OS must respect into
> >> > > > > > EfiBootServicesData
> >> > > > > > which the OS may discard.
> >> > > > > >
> >> > > > > > E.g. initr_lmb() is being called after efi_memory_init().
> >> > > > > >
> >> > > > > > Getting all cases of synchronization properly tested seems very
> >> > > > > > hard to
> >> > > > > > me. Everything would be much easier if we had only a single
> >> > > > > > memory
> >> > > > > > management system.
> >> > > > >
> >> > > > > Yes, Sughosh is working on the single memory reservation system for
> >> > > > > everyone to use. This pairs with the single memory allocation
> >> > > > > system
> >> > > > > (malloc) that we have. Parts of the code base that aren't keeping
> >> > > > > these
> >> > > > > systems up to date / obeying their results need to be corrected.
> >> > > >
> >> > > > The EFI allocations don't happen until boot time...so why do we need
> >> > > > to do this now? We can instead have an EFI function to scan LMB and
> >> > > > add to its memory map.
> >> > >
> >> > > We're talking about reservations, not allocations. So yes, when someone
> >> > > is making their reservation, they need to make it. I don't understand
> >> > > your question.
> >> >
> >> > As I understand it, this is used to tell EFI about a memory reservation.
> >>
> >> This patch, or this series? This series isn't about EFI. This patch is,
> >> yes.
> >>
> >> > But the EFI code can scan the LMB reservations just before booting and
> >> > update its tables. I don't see a need to keep them in sync before the
> >> > boot actually happens.
> >>
> >> But that wouldn't work. If something needs to reserve a region it needs
> >> to do it when it starts using it. It's not about the EFI map for the OS,
> >> it's about making sure that U-Boot doesn't scribble over a now-reserved
> >> area.
> >
> >I'm not convinced of that yet. EFI does not do allocations until it
> >starts loading images, and it uses LMB for those (or at least it does
> >with bootstd). I'm just trying to keep this all as simple as possible.
>
> You can load a boot time EFI driver which keeps running while you return to
> U-Boot to load another file.
>
> The background activity of the EFI driver binary can result in any number of
> allocations.
U-Boot isn't multi-threaded so I am really not sure what would happen
in that case. Does it actually work?
Regards,
Simon
