| Message ID | 20240607185240.1892031-16-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Make U-Boot memory reservations coherent | expand |
On Fri, 7 Jun 2024 at 21:54, 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. So the question here, is why do we need a notifier chain overall? Can't we change the EFI subsystem and allocate memory with lmb now? And any special functions we have in EFI (e.g allocate aligned memory) can migrate to lmb() Cheers /Ilias > > Signed-off-by: Sughosh Ganu <sughosh.ganu@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 : > + EFI_BOOT_SERVICES_DATA, > + true); > + > + return status == EFI_SUCCESS ? 0 : -1; > +} > +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync); > #endif /* MEM_MAP_UPDATE_NOTIFY */ > > /** > @@ -275,18 +307,9 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, > return EFI_CARVE_LOOP_AGAIN; > } > > -/** > - * efi_add_memory_map_pg() - add pages to the memory map > - * > - * @start: start address, must be a multiple of EFI_PAGE_SIZE > - * @pages: number of pages to add > - * @memory_type: type of memory added > - * @overlap_only_ram: region may only overlap RAM > - * Return: status code > - */ > -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > - int memory_type, > - bool overlap_only_ram) > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages, > + int memory_type, > + bool overlap_only_ram) > { > struct list_head *lhandle; > struct efi_mem_list *newlist; > @@ -395,6 +418,29 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > } > } > > + return EFI_SUCCESS; > +} > + > +/** > + * efi_add_memory_map_pg() - add pages to the memory map > + * > + * @start: start address, must be a multiple of EFI_PAGE_SIZE > + * @pages: number of pages to add > + * @memory_type: type of memory added > + * @overlap_only_ram: region may only overlap RAM > + * Return: status code > + */ > +static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > + int memory_type, > + bool overlap_only_ram) > +{ > + efi_status_t status; > + > + status = __efi_add_memory_map_pg(start, pages, memory_type, > + overlap_only_ram); > + if (status != EFI_SUCCESS) > + return status; > + > if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)) > efi_map_update_notify(start, pages << EFI_PAGE_SHIFT, > memory_type == EFI_CONVENTIONAL_MEMORY ? > -- > 2.34.1 >
On Mon, 10 Jun 2024 at 17:40, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Fri, 7 Jun 2024 at 21:54, 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. > > So the question here, is why do we need a notifier chain overall? > Can't we change the EFI subsystem and allocate memory with lmb now? > And any special functions we have in EFI (e.g allocate aligned memory) > can migrate to lmb() Like we discussed offline, that was my initial attempt -- to use the LMB allocation API's from inside the EFI allocate pages module. But I was facing a lot of corner case issues, primarily in keeping the two memory maps the same. Which is why I moved to the current implementation of notifying other modules, and that too only for the addresses in the RAM region. -sughosh > > Cheers > /Ilias > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@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 : > > + EFI_BOOT_SERVICES_DATA, > > + true); > > + > > + return status == EFI_SUCCESS ? 0 : -1; > > +} > > +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync); > > #endif /* MEM_MAP_UPDATE_NOTIFY */ > > > > /** > > @@ -275,18 +307,9 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, > > return EFI_CARVE_LOOP_AGAIN; > > } > > > > -/** > > - * efi_add_memory_map_pg() - add pages to the memory map > > - * > > - * @start: start address, must be a multiple of EFI_PAGE_SIZE > > - * @pages: number of pages to add > > - * @memory_type: type of memory added > > - * @overlap_only_ram: region may only overlap RAM > > - * Return: status code > > - */ > > -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > - int memory_type, > > - bool overlap_only_ram) > > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages, > > + int memory_type, > > + bool overlap_only_ram) > > { > > struct list_head *lhandle; > > struct efi_mem_list *newlist; > > @@ -395,6 +418,29 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > } > > } > > > > + return EFI_SUCCESS; > > +} > > + > > +/** > > + * efi_add_memory_map_pg() - add pages to the memory map > > + * > > + * @start: start address, must be a multiple of EFI_PAGE_SIZE > > + * @pages: number of pages to add > > + * @memory_type: type of memory added > > + * @overlap_only_ram: region may only overlap RAM > > + * Return: status code > > + */ > > +static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > + int memory_type, > > + bool overlap_only_ram) > > +{ > > + efi_status_t status; > > + > > + status = __efi_add_memory_map_pg(start, pages, memory_type, > > + overlap_only_ram); > > + if (status != EFI_SUCCESS) > > + return status; > > + > > if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)) > > efi_map_update_notify(start, pages << EFI_PAGE_SHIFT, > > memory_type == EFI_CONVENTIONAL_MEMORY ? > > -- > > 2.34.1 > >
Hi Sughosh On Mon, 10 Jun 2024 at 15:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Mon, 10 Jun 2024 at 17:40, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > On Fri, 7 Jun 2024 at 21:54, 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. > > > > So the question here, is why do we need a notifier chain overall? > > Can't we change the EFI subsystem and allocate memory with lmb now? > > And any special functions we have in EFI (e.g allocate aligned memory) > > can migrate to lmb() > > Like we discussed offline, that was my initial attempt -- to use the > LMB allocation API's from inside the EFI allocate pages module. But I > was facing a lot of corner case issues, primarily in keeping the two > memory maps the same. I think it would worth discussing this a bit more. I like the idea of having a single allocator more than having events to update memory reservations > Which is why I moved to the current > implementation of notifying other modules, and that too only for the > addresses in the RAM region. The notification to 'other modules' i still done by updating the static memory map though no? So what corner cases we couldn't solve by having a single allocator? Thanks /Ilias > > -sughosh > > > > > Cheers > > /Ilias > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@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 : > > > + EFI_BOOT_SERVICES_DATA, > > > + true); > > > + > > > + return status == EFI_SUCCESS ? 0 : -1; > > > +} > > > +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync); > > > #endif /* MEM_MAP_UPDATE_NOTIFY */ > > > > > > /** > > > @@ -275,18 +307,9 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, > > > return EFI_CARVE_LOOP_AGAIN; > > > } > > > > > > -/** > > > - * efi_add_memory_map_pg() - add pages to the memory map > > > - * > > > - * @start: start address, must be a multiple of EFI_PAGE_SIZE > > > - * @pages: number of pages to add > > > - * @memory_type: type of memory added > > > - * @overlap_only_ram: region may only overlap RAM > > > - * Return: status code > > > - */ > > > -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > - int memory_type, > > > - bool overlap_only_ram) > > > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages, > > > + int memory_type, > > > + bool overlap_only_ram) > > > { > > > struct list_head *lhandle; > > > struct efi_mem_list *newlist; > > > @@ -395,6 +418,29 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > } > > > } > > > > > > + return EFI_SUCCESS; > > > +} > > > + > > > +/** > > > + * efi_add_memory_map_pg() - add pages to the memory map > > > + * > > > + * @start: start address, must be a multiple of EFI_PAGE_SIZE > > > + * @pages: number of pages to add > > > + * @memory_type: type of memory added > > > + * @overlap_only_ram: region may only overlap RAM > > > + * Return: status code > > > + */ > > > +static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > + int memory_type, > > > + bool overlap_only_ram) > > > +{ > > > + efi_status_t status; > > > + > > > + status = __efi_add_memory_map_pg(start, pages, memory_type, > > > + overlap_only_ram); > > > + if (status != EFI_SUCCESS) > > > + return status; > > > + > > > if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)) > > > efi_map_update_notify(start, pages << EFI_PAGE_SHIFT, > > > memory_type == EFI_CONVENTIONAL_MEMORY ? > > > -- > > > 2.34.1 > > >
hi Ilias, On Mon, 10 Jun 2024 at 19:48, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Sughosh > > On Mon, 10 Jun 2024 at 15:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Mon, 10 Jun 2024 at 17:40, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > On Fri, 7 Jun 2024 at 21:54, 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. > > > > > > So the question here, is why do we need a notifier chain overall? > > > Can't we change the EFI subsystem and allocate memory with lmb now? > > > And any special functions we have in EFI (e.g allocate aligned memory) > > > can migrate to lmb() > > > > Like we discussed offline, that was my initial attempt -- to use the > > LMB allocation API's from inside the EFI allocate pages module. But I > > was facing a lot of corner case issues, primarily in keeping the two > > memory maps the same. > > I think it would worth discussing this a bit more. I like the idea of > having a single allocator more than having events to update memory > reservations > > > Which is why I moved to the current > > implementation of notifying other modules, and that too only for the > > addresses in the RAM region. > > The notification to 'other modules' i still done by updating the > static memory map though no? > So what corner cases we couldn't solve by having a single allocator? I can re-check what were the issues that I faced when trying with a single allocator. But please note that we are not making any significant gains by having a single allocator. Even with a common allocator(say LMB), it would still be required to have the same notification mechanism to update the EFI memory map. Else the EFI memory map would show regions of memory as conventional memory, while they are being used by the other subsystem. So, all in all, I think that the notification mechanism is not that inefficient. Thanks. -sughosh > > Thanks > /Ilias > > > > -sughosh > > > > > > > > Cheers > > > /Ilias > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@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 : > > > > + EFI_BOOT_SERVICES_DATA, > > > > + true); > > > > + > > > > + return status == EFI_SUCCESS ? 0 : -1; > > > > +} > > > > +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync); > > > > #endif /* MEM_MAP_UPDATE_NOTIFY */ > > > > > > > > /** > > > > @@ -275,18 +307,9 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, > > > > return EFI_CARVE_LOOP_AGAIN; > > > > } > > > > > > > > -/** > > > > - * efi_add_memory_map_pg() - add pages to the memory map > > > > - * > > > > - * @start: start address, must be a multiple of EFI_PAGE_SIZE > > > > - * @pages: number of pages to add > > > > - * @memory_type: type of memory added > > > > - * @overlap_only_ram: region may only overlap RAM > > > > - * Return: status code > > > > - */ > > > > -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > > - int memory_type, > > > > - bool overlap_only_ram) > > > > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages, > > > > + int memory_type, > > > > + bool overlap_only_ram) > > > > { > > > > struct list_head *lhandle; > > > > struct efi_mem_list *newlist; > > > > @@ -395,6 +418,29 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > > } > > > > } > > > > > > > > + return EFI_SUCCESS; > > > > +} > > > > + > > > > +/** > > > > + * efi_add_memory_map_pg() - add pages to the memory map > > > > + * > > > > + * @start: start address, must be a multiple of EFI_PAGE_SIZE > > > > + * @pages: number of pages to add > > > > + * @memory_type: type of memory added > > > > + * @overlap_only_ram: region may only overlap RAM > > > > + * Return: status code > > > > + */ > > > > +static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > > + int memory_type, > > > > + bool overlap_only_ram) > > > > +{ > > > > + efi_status_t status; > > > > + > > > > + status = __efi_add_memory_map_pg(start, pages, memory_type, > > > > + overlap_only_ram); > > > > + if (status != EFI_SUCCESS) > > > > + return status; > > > > + > > > > if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)) > > > > efi_map_update_notify(start, pages << EFI_PAGE_SHIFT, > > > > memory_type == EFI_CONVENTIONAL_MEMORY ? > > > > -- > > > > 2.34.1 > > > >
On Mon, 10 Jun 2024 at 20:22, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > hi Ilias, > > On Mon, 10 Jun 2024 at 19:48, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Sughosh > > > > On Mon, 10 Jun 2024 at 15:25, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Mon, 10 Jun 2024 at 17:40, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > On Fri, 7 Jun 2024 at 21:54, 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. > > > > > > > > So the question here, is why do we need a notifier chain overall? > > > > Can't we change the EFI subsystem and allocate memory with lmb now? > > > > And any special functions we have in EFI (e.g allocate aligned memory) > > > > can migrate to lmb() > > > > > > Like we discussed offline, that was my initial attempt -- to use the > > > LMB allocation API's from inside the EFI allocate pages module. But I > > > was facing a lot of corner case issues, primarily in keeping the two > > > memory maps the same. > > > > I think it would worth discussing this a bit more. I like the idea of > > having a single allocator more than having events to update memory > > reservations > > > > > Which is why I moved to the current > > > implementation of notifying other modules, and that too only for the > > > addresses in the RAM region. > > > > The notification to 'other modules' i still done by updating the > > static memory map though no? > > So what corner cases we couldn't solve by having a single allocator? > > I can re-check what were the issues that I faced when trying with a > single allocator. But please note that we are not making any > significant gains by having a single allocator. Even with a common > allocator(say LMB), it would still be required to have the same > notification mechanism to update the EFI memory map. Else the EFI > memory map would show regions of memory as conventional memory, while > they are being used by the other subsystem. So, all in all, I think > that the notification mechanism is not that inefficient. Thanks. Or rather, "the notification mechanism is not inefficient". -sughosh > > -sughosh > > > > > Thanks > > /Ilias > > > > > > -sughosh > > > > > > > > > > > Cheers > > > > /Ilias > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@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 : > > > > > + EFI_BOOT_SERVICES_DATA, > > > > > + true); > > > > > + > > > > > + return status == EFI_SUCCESS ? 0 : -1; > > > > > +} > > > > > +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync); > > > > > #endif /* MEM_MAP_UPDATE_NOTIFY */ > > > > > > > > > > /** > > > > > @@ -275,18 +307,9 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, > > > > > return EFI_CARVE_LOOP_AGAIN; > > > > > } > > > > > > > > > > -/** > > > > > - * efi_add_memory_map_pg() - add pages to the memory map > > > > > - * > > > > > - * @start: start address, must be a multiple of EFI_PAGE_SIZE > > > > > - * @pages: number of pages to add > > > > > - * @memory_type: type of memory added > > > > > - * @overlap_only_ram: region may only overlap RAM > > > > > - * Return: status code > > > > > - */ > > > > > -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > > > - int memory_type, > > > > > - bool overlap_only_ram) > > > > > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages, > > > > > + int memory_type, > > > > > + bool overlap_only_ram) > > > > > { > > > > > struct list_head *lhandle; > > > > > struct efi_mem_list *newlist; > > > > > @@ -395,6 +418,29 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > > > } > > > > > } > > > > > > > > > > + return EFI_SUCCESS; > > > > > +} > > > > > + > > > > > +/** > > > > > + * efi_add_memory_map_pg() - add pages to the memory map > > > > > + * > > > > > + * @start: start address, must be a multiple of EFI_PAGE_SIZE > > > > > + * @pages: number of pages to add > > > > > + * @memory_type: type of memory added > > > > > + * @overlap_only_ram: region may only overlap RAM > > > > > + * Return: status code > > > > > + */ > > > > > +static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > > > + int memory_type, > > > > > + bool overlap_only_ram) > > > > > +{ > > > > > + efi_status_t status; > > > > > + > > > > > + status = __efi_add_memory_map_pg(start, pages, memory_type, > > > > > + overlap_only_ram); > > > > > + if (status != EFI_SUCCESS) > > > > > + return status; > > > > > + > > > > > if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)) > > > > > efi_map_update_notify(start, pages << EFI_PAGE_SHIFT, > > > > > memory_type == EFI_CONVENTIONAL_MEMORY ? > > > > > -- > > > > > 2.34.1 > > > > >
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 I am worried about the "frees up memory" case. When LMB frees memory we cannot add it back to EFI conventional memory as there might still be a file image lingering around that EFI should not overwrite. It has to stay marked as EfiLoaderCode or EfiLoaderData. How do you ensure that if a region reserved by LMB notification as EfiLoaderData is coalesced with some other allocation LMB is not requested to mark the coalesced region as reserved? @Tom Clinging to the existing logic that you can do anything when loading files is obviously leading us into coding hell. If somebody wants to load two images into the same location, he should be forced to unload the first image. This will allow us to have a single memory management system. Best regards Heinrich > 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> > --- > 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 : > + EFI_BOOT_SERVICES_DATA, > + true); > + > + return status == EFI_SUCCESS ? 0 : -1; > +} > +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync); > #endif /* MEM_MAP_UPDATE_NOTIFY */ > > /** > @@ -275,18 +307,9 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, > return EFI_CARVE_LOOP_AGAIN; > } > > -/** > - * efi_add_memory_map_pg() - add pages to the memory map > - * > - * @start: start address, must be a multiple of EFI_PAGE_SIZE > - * @pages: number of pages to add > - * @memory_type: type of memory added > - * @overlap_only_ram: region may only overlap RAM > - * Return: status code > - */ > -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > - int memory_type, > - bool overlap_only_ram) > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages, > + int memory_type, > + bool overlap_only_ram) > { > struct list_head *lhandle; > struct efi_mem_list *newlist; > @@ -395,6 +418,29 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > } > } > > + return EFI_SUCCESS; > +} > + > +/** > + * efi_add_memory_map_pg() - add pages to the memory map > + * > + * @start: start address, must be a multiple of EFI_PAGE_SIZE > + * @pages: number of pages to add > + * @memory_type: type of memory added > + * @overlap_only_ram: region may only overlap RAM > + * Return: status code > + */ > +static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > + int memory_type, > + bool overlap_only_ram) > +{ > + efi_status_t status; > + > + status = __efi_add_memory_map_pg(start, pages, memory_type, > + overlap_only_ram); > + if (status != EFI_SUCCESS) > + return status; > + > if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)) > efi_map_update_notify(start, pages << EFI_PAGE_SHIFT, > memory_type == EFI_CONVENTIONAL_MEMORY ?
On Mon, 10 Jun 2024 at 20:47, Heinrich Schuchardt <xypron.glpk@gmx.de> 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 > > I am worried about the "frees up memory" case. > > When LMB frees memory we cannot add it back to EFI conventional memory > as there might still be a file image lingering around that EFI should > not overwrite. It has to stay marked as EfiLoaderCode or EfiLoaderData. So here is my basic doubt. Why would LMB free up memory if it still has a valid image. If that is the case, the lmb_free API should not be called? -sughosh > > How do you ensure that if a region reserved by LMB notification as > EfiLoaderData is coalesced with some other allocation LMB is not > requested to mark the coalesced region as reserved? > > @Tom > > Clinging to the existing logic that you can do anything when loading > files is obviously leading us into coding hell. > > If somebody wants to load two images into the same location, he should > be forced to unload the first image. This will allow us to have a single > memory management system. > > Best regards > > Heinrich > > > 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> > > --- > > 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 : > > + EFI_BOOT_SERVICES_DATA, > > + true); > > + > > + return status == EFI_SUCCESS ? 0 : -1; > > +} > > +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync); > > #endif /* MEM_MAP_UPDATE_NOTIFY */ > > > > /** > > @@ -275,18 +307,9 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, > > return EFI_CARVE_LOOP_AGAIN; > > } > > > > -/** > > - * efi_add_memory_map_pg() - add pages to the memory map > > - * > > - * @start: start address, must be a multiple of EFI_PAGE_SIZE > > - * @pages: number of pages to add > > - * @memory_type: type of memory added > > - * @overlap_only_ram: region may only overlap RAM > > - * Return: status code > > - */ > > -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > - int memory_type, > > - bool overlap_only_ram) > > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages, > > + int memory_type, > > + bool overlap_only_ram) > > { > > struct list_head *lhandle; > > struct efi_mem_list *newlist; > > @@ -395,6 +418,29 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > } > > } > > > > + return EFI_SUCCESS; > > +} > > + > > +/** > > + * efi_add_memory_map_pg() - add pages to the memory map > > + * > > + * @start: start address, must be a multiple of EFI_PAGE_SIZE > > + * @pages: number of pages to add > > + * @memory_type: type of memory added > > + * @overlap_only_ram: region may only overlap RAM > > + * Return: status code > > + */ > > +static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > + int memory_type, > > + bool overlap_only_ram) > > +{ > > + efi_status_t status; > > + > > + status = __efi_add_memory_map_pg(start, pages, memory_type, > > + overlap_only_ram); > > + if (status != EFI_SUCCESS) > > + return status; > > + > > if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)) > > efi_map_update_notify(start, pages << EFI_PAGE_SHIFT, > > memory_type == EFI_CONVENTIONAL_MEMORY ? >
Hi, On Mon, 10 Jun 2024 at 09:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > On Mon, 10 Jun 2024 at 20:47, Heinrich Schuchardt <xypron.glpk@gmx.de> 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 > > > > I am worried about the "frees up memory" case. > > > > When LMB frees memory we cannot add it back to EFI conventional memory > > as there might still be a file image lingering around that EFI should > > not overwrite. It has to stay marked as EfiLoaderCode or EfiLoaderData. > > So here is my basic doubt. Why would LMB free up memory if it still > has a valid image. If that is the case, the lmb_free API should not be > called? > > -sughosh > > > > > > How do you ensure that if a region reserved by LMB notification as > > EfiLoaderData is coalesced with some other allocation LMB is not > > requested to mark the coalesced region as reserved? > > > > @Tom > > > > Clinging to the existing logic that you can do anything when loading > > files is obviously leading us into coding hell. > > > > If somebody wants to load two images into the same location, he should > > be forced to unload the first image. This will allow us to have a single > > memory management system. It seems we really shouldn't use the words 'allocate' and 'free' when talking about LMB. They are simply reservations. I believe we have got into this situation due to an assumption that these two things are the same, but in U-Boot they certainly are not. LMB is a very lighweight and temporary reservation system to be used for a single boot process. Regards, Simon > > > > Best regards > > > > Heinrich > > > > > 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> > > > --- > > > 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 : > > > + EFI_BOOT_SERVICES_DATA, > > > + true); > > > + > > > + return status == EFI_SUCCESS ? 0 : -1; > > > +} > > > +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync); > > > #endif /* MEM_MAP_UPDATE_NOTIFY */ > > > > > > /** > > > @@ -275,18 +307,9 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, > > > return EFI_CARVE_LOOP_AGAIN; > > > } > > > > > > -/** > > > - * efi_add_memory_map_pg() - add pages to the memory map > > > - * > > > - * @start: start address, must be a multiple of EFI_PAGE_SIZE > > > - * @pages: number of pages to add > > > - * @memory_type: type of memory added > > > - * @overlap_only_ram: region may only overlap RAM > > > - * Return: status code > > > - */ > > > -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > - int memory_type, > > > - bool overlap_only_ram) > > > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages, > > > + int memory_type, > > > + bool overlap_only_ram) > > > { > > > struct list_head *lhandle; > > > struct efi_mem_list *newlist; > > > @@ -395,6 +418,29 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > } > > > } > > > > > > + return EFI_SUCCESS; > > > +} > > > + > > > +/** > > > + * efi_add_memory_map_pg() - add pages to the memory map > > > + * > > > + * @start: start address, must be a multiple of EFI_PAGE_SIZE > > > + * @pages: number of pages to add > > > + * @memory_type: type of memory added > > > + * @overlap_only_ram: region may only overlap RAM > > > + * Return: status code > > > + */ > > > +static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > + int memory_type, > > > + bool overlap_only_ram) > > > +{ > > > + efi_status_t status; > > > + > > > + status = __efi_add_memory_map_pg(start, pages, memory_type, > > > + overlap_only_ram); > > > + if (status != EFI_SUCCESS) > > > + return status; > > > + > > > if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)) > > > efi_map_update_notify(start, pages << EFI_PAGE_SHIFT, > > > memory_type == EFI_CONVENTIONAL_MEMORY ? > >
On Mon, Jun 10, 2024, 5:52 PM Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > hi Ilias, > > On Mon, 10 Jun 2024 at 19:48, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Sughosh > > > > On Mon, 10 Jun 2024 at 15:25, Sughosh Ganu <sughosh.ganu@linaro.org> > wrote: > > > > > > On Mon, 10 Jun 2024 at 17:40, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > On Fri, 7 Jun 2024 at 21:54, 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. > > > > > > > > So the question here, is why do we need a notifier chain overall? > > > > Can't we change the EFI subsystem and allocate memory with lmb now? > > > > And any special functions we have in EFI (e.g allocate aligned > memory) > > > > can migrate to lmb() > > > > > > Like we discussed offline, that was my initial attempt -- to use the > > > LMB allocation API's from inside the EFI allocate pages module. But I > > > was facing a lot of corner case issues, primarily in keeping the two > > > memory maps the same. > > > > I think it would worth discussing this a bit more. I like the idea of > > having a single allocator more than having events to update memory > > reservations > > > > > Which is why I moved to the current > > > implementation of notifying other modules, and that too only for the > > > addresses in the RAM region. > > > > The notification to 'other modules' i still done by updating the > > static memory map though no? > > So what corner cases we couldn't solve by having a single allocator? > > I can re-check what were the issues that I faced when trying with a > single allocator. But please note that we are not making any > significant gains by having a single allocator. Even with a common > allocator(say LMB), it would still be required to have the same > notification mechanism to update the EFI memory map. I am not sure about this. With the proposal I did on the other thread, you would have the allocations updated at runtime and an ID of the subsystem that allocated the memory. The only difference with having a central allocator is that we will need a 'flags' field to store the EFI-specific options. Else the EFI > memory map would show regions of memory as conventional memory, while > they are being used by the other subsystem. So, all in all, I think > that the notification mechanism is not that inefficient. Thanks. > > Regards /Ilias > -sughosh > > > > > Thanks > > /Ilias > > > > > > -sughosh > > > > > > > > > > > Cheers > > > > /Ilias > > > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu@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 : > > > > > + EFI_BOOT_SERVICES_DATA, > > > > > + true); > > > > > + > > > > > + return status == EFI_SUCCESS ? 0 : -1; > > > > > +} > > > > > +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync); > > > > > #endif /* MEM_MAP_UPDATE_NOTIFY */ > > > > > > > > > > /** > > > > > @@ -275,18 +307,9 @@ static s64 efi_mem_carve_out(struct > efi_mem_list *map, > > > > > return EFI_CARVE_LOOP_AGAIN; > > > > > } > > > > > > > > > > -/** > > > > > - * efi_add_memory_map_pg() - add pages to the memory map > > > > > - * > > > > > - * @start: start address, must be a multiple of > EFI_PAGE_SIZE > > > > > - * @pages: number of pages to add > > > > > - * @memory_type: type of memory added > > > > > - * @overlap_only_ram: region may only overlap RAM > > > > > - * Return: status code > > > > > - */ > > > > > -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > > > - int memory_type, > > > > > - bool overlap_only_ram) > > > > > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages, > > > > > + int memory_type, > > > > > + bool overlap_only_ram) > > > > > { > > > > > struct list_head *lhandle; > > > > > struct efi_mem_list *newlist; > > > > > @@ -395,6 +418,29 @@ static efi_status_t efi_add_memory_map_pg(u64 > start, u64 pages, > > > > > } > > > > > } > > > > > > > > > > + return EFI_SUCCESS; > > > > > +} > > > > > + > > > > > +/** > > > > > + * efi_add_memory_map_pg() - add pages to the memory map > > > > > + * > > > > > + * @start: start address, must be a multiple of > EFI_PAGE_SIZE > > > > > + * @pages: number of pages to add > > > > > + * @memory_type: type of memory added > > > > > + * @overlap_only_ram: region may only overlap RAM > > > > > + * Return: status code > > > > > + */ > > > > > +static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > > > + int memory_type, > > > > > + bool overlap_only_ram) > > > > > +{ > > > > > + efi_status_t status; > > > > > + > > > > > + status = __efi_add_memory_map_pg(start, pages, memory_type, > > > > > + overlap_only_ram); > > > > > + if (status != EFI_SUCCESS) > > > > > + return status; > > > > > + > > > > > if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)) > > > > > efi_map_update_notify(start, pages << > EFI_PAGE_SHIFT, > > > > > memory_type == > EFI_CONVENTIONAL_MEMORY ? > > > > > -- > > > > > 2.34.1 > > > > > >
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.ganu@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. Best regards Heinrich > + EFI_BOOT_SERVICES_DATA, > + true); > + > + return status == EFI_SUCCESS ? 0 : -1; > +} > +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync); > #endif /* MEM_MAP_UPDATE_NOTIFY */ > > /** > @@ -275,18 +307,9 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, > return EFI_CARVE_LOOP_AGAIN; > } > > -/** > - * efi_add_memory_map_pg() - add pages to the memory map > - * > - * @start: start address, must be a multiple of EFI_PAGE_SIZE > - * @pages: number of pages to add > - * @memory_type: type of memory added > - * @overlap_only_ram: region may only overlap RAM > - * Return: status code > - */ > -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > - int memory_type, > - bool overlap_only_ram) > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages, > + int memory_type, > + bool overlap_only_ram) > { > struct list_head *lhandle; > struct efi_mem_list *newlist; > @@ -395,6 +418,29 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > } > } > > + return EFI_SUCCESS; > +} > + > +/** > + * efi_add_memory_map_pg() - add pages to the memory map > + * > + * @start: start address, must be a multiple of EFI_PAGE_SIZE > + * @pages: number of pages to add > + * @memory_type: type of memory added > + * @overlap_only_ram: region may only overlap RAM > + * Return: status code > + */ > +static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > + int memory_type, > + bool overlap_only_ram) > +{ > + efi_status_t status; > + > + status = __efi_add_memory_map_pg(start, pages, memory_type, > + overlap_only_ram); > + if (status != EFI_SUCCESS) > + return status; > + > if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)) > efi_map_update_notify(start, pages << EFI_PAGE_SHIFT, > memory_type == EFI_CONVENTIONAL_MEMORY ?
On Tue, 11 Jun 2024 at 15:47, Heinrich Schuchardt <xypron.glpk@gmx.de> 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.ganu@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. If memory is being marked as EfiReservedMemory, that will also update the LMB memory map to have it marked as reserved. So a correct implementation should then not be freeing memory that is not allocated by that module. -sughosh > > 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. > > Best regards > > Heinrich > > > + EFI_BOOT_SERVICES_DATA, > > + true); > > + > > + return status == EFI_SUCCESS ? 0 : -1; > > +} > > +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync); > > #endif /* MEM_MAP_UPDATE_NOTIFY */ > > > > /** > > @@ -275,18 +307,9 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, > > return EFI_CARVE_LOOP_AGAIN; > > } > > > > -/** > > - * efi_add_memory_map_pg() - add pages to the memory map > > - * > > - * @start: start address, must be a multiple of EFI_PAGE_SIZE > > - * @pages: number of pages to add > > - * @memory_type: type of memory added > > - * @overlap_only_ram: region may only overlap RAM > > - * Return: status code > > - */ > > -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > - int memory_type, > > - bool overlap_only_ram) > > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages, > > + int memory_type, > > + bool overlap_only_ram) > > { > > struct list_head *lhandle; > > struct efi_mem_list *newlist; > > @@ -395,6 +418,29 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > } > > } > > > > + return EFI_SUCCESS; > > +} > > + > > +/** > > + * efi_add_memory_map_pg() - add pages to the memory map > > + * > > + * @start: start address, must be a multiple of EFI_PAGE_SIZE > > + * @pages: number of pages to add > > + * @memory_type: type of memory added > > + * @overlap_only_ram: region may only overlap RAM > > + * Return: status code > > + */ > > +static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > + int memory_type, > > + bool overlap_only_ram) > > +{ > > + efi_status_t status; > > + > > + status = __efi_add_memory_map_pg(start, pages, memory_type, > > + overlap_only_ram); > > + if (status != EFI_SUCCESS) > > + return status; > > + > > if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)) > > efi_map_update_notify(start, pages << EFI_PAGE_SHIFT, > > memory_type == EFI_CONVENTIONAL_MEMORY ? >
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.ganu@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.
Hi, On Tue, 11 Jun 2024 at 08:36, Tom Rini <trini@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.ganu@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. Regards, Simon
On Tue, Jun 11, 2024 at 12:52:19PM -0600, Simon Glass wrote: > Hi, > > On Tue, 11 Jun 2024 at 08:36, Tom Rini <trini@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.ganu@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.
Hi Tom, On Tue, 11 Jun 2024 at 15:01, Tom Rini <trini@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 <trini@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.ganu@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. 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. Regards, Simon
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 <trini@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 <trini@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.ganu@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.
Hi Tom, On Tue, 11 Jun 2024 at 16:54, Tom Rini <trini@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 <trini@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 <trini@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.ganu@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. Regards, Simon
[...] > > > > > > > > + 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, It does in some cases. E.g The efi variables allocate some pages when the subsystem starts, the TCG protocol allocates the EventLog once it's installed and I am pretty sure we have more. > 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. Heinrich already pointed out a potential danger in the current design. If an EFI allocation happens *before* LMB comes up, we might end up updating the efi memory map with the wrong attributes. That would lead to the OS discarding memory areas that should be preserved and we won't figure that out until the OS boots and blows up. Cheers /Ilias > > Regards, > Simon
Am 12. Juni 2024 04:42:00 MESZ schrieb Simon Glass <sjg@chromium.org>: >Hi Tom, > >On Tue, 11 Jun 2024 at 16:54, Tom Rini <trini@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 <trini@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 <trini@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.ganu@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. Best regards Heinrich > >Regards, >Simon
hi Ilias, On Wed, 12 Jun 2024 at 11:19, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > [...] > > > > > > > > > > + 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, > > It does in some cases. E.g The efi variables allocate some pages when > the subsystem starts, the TCG protocol allocates the EventLog once > it's installed and I am pretty sure we have more. > > > 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. > > Heinrich already pointed out a potential danger in the current design. > If an EFI allocation happens *before* LMB comes up, we might end up > updating the efi memory map with the wrong attributes. That would lead > to the OS discarding memory areas that should be preserved and we > won't figure that out until the OS boots and blows up. Like I mentioned in one of the earlier replies [1], this is actually not an issue, as long as modules are freeing memory that was actually allocated/reserved by them. In case of the EFI subsystem marking some memory region as EfiReservedMemory, this will send a notification to the LMB module, and the LMB module will mark this region as reserved. So unless some LMB user decides on freeing up memory that was not reserved by it, this should be just fine. Making use of the LMB API's as the common backend for allocating memory is a separate thing, and I will look into that. But the above highlighted issue is not one, with correctly working code. -sughosh [1] - https://lists.denx.de/pipermail/u-boot/2024-June/555883.html
On Mon, 10 Jun 2024 at 18:54, Simon Glass <sjg@chromium.org> wrote: > > Hi, > > On Mon, 10 Jun 2024 at 09:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > On Mon, 10 Jun 2024 at 20:47, Heinrich Schuchardt <xypron.glpk@gmx.de> 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 > > > > > > I am worried about the "frees up memory" case. > > > > > > When LMB frees memory we cannot add it back to EFI conventional memory > > > as there might still be a file image lingering around that EFI should > > > not overwrite. It has to stay marked as EfiLoaderCode or EfiLoaderData. > > > > So here is my basic doubt. Why would LMB free up memory if it still > > has a valid image. If that is the case, the lmb_free API should not be > > called? > > > > -sughosh > > > > > > > > > > How do you ensure that if a region reserved by LMB notification as > > > EfiLoaderData is coalesced with some other allocation LMB is not > > > requested to mark the coalesced region as reserved? > > > > > > @Tom > > > > > > Clinging to the existing logic that you can do anything when loading > > > files is obviously leading us into coding hell. > > > > > > If somebody wants to load two images into the same location, he should > > > be forced to unload the first image. This will allow us to have a single > > > memory management system. > > It seems we really shouldn't use the words 'allocate' and 'free' when > talking about LMB. They are simply reservations. Correct and while at it can we please make the code less confusing to read. What we today mark as reserved isnt even trully reserved as it can be overwritten. struct lmb_region memory -> available memory we added on LMB. That's fine struct lmb_region reserved -> can we rename this to 'used' and rename LMB_NOOVERWRITE to LMB_RESERVED? Thanks /Ilias > I believe we have got > into this situation due to an assumption that these two things are the > same, but in U-Boot they certainly are not. LMB is a very lighweight > and temporary reservation system to be used for a single boot process. > > Regards, > Simon > > > > > > > > Best regards > > > > > > Heinrich > > > > > > > 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> > > > > --- > > > > 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 : > > > > + EFI_BOOT_SERVICES_DATA, > > > > + true); > > > > + > > > > + return status == EFI_SUCCESS ? 0 : -1; > > > > +} > > > > +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync); > > > > #endif /* MEM_MAP_UPDATE_NOTIFY */ > > > > > > > > /** > > > > @@ -275,18 +307,9 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, > > > > return EFI_CARVE_LOOP_AGAIN; > > > > } > > > > > > > > -/** > > > > - * efi_add_memory_map_pg() - add pages to the memory map > > > > - * > > > > - * @start: start address, must be a multiple of EFI_PAGE_SIZE > > > > - * @pages: number of pages to add > > > > - * @memory_type: type of memory added > > > > - * @overlap_only_ram: region may only overlap RAM > > > > - * Return: status code > > > > - */ > > > > -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > > - int memory_type, > > > > - bool overlap_only_ram) > > > > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages, > > > > + int memory_type, > > > > + bool overlap_only_ram) > > > > { > > > > struct list_head *lhandle; > > > > struct efi_mem_list *newlist; > > > > @@ -395,6 +418,29 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > > } > > > > } > > > > > > > > + return EFI_SUCCESS; > > > > +} > > > > + > > > > +/** > > > > + * efi_add_memory_map_pg() - add pages to the memory map > > > > + * > > > > + * @start: start address, must be a multiple of EFI_PAGE_SIZE > > > > + * @pages: number of pages to add > > > > + * @memory_type: type of memory added > > > > + * @overlap_only_ram: region may only overlap RAM > > > > + * Return: status code > > > > + */ > > > > +static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > > + int memory_type, > > > > + bool overlap_only_ram) > > > > +{ > > > > + efi_status_t status; > > > > + > > > > + status = __efi_add_memory_map_pg(start, pages, memory_type, > > > > + overlap_only_ram); > > > > + if (status != EFI_SUCCESS) > > > > + return status; > > > > + > > > > if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)) > > > > efi_map_update_notify(start, pages << EFI_PAGE_SHIFT, > > > > memory_type == EFI_CONVENTIONAL_MEMORY ? > > >
On Wed, 12 Jun 2024 at 12:16, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Mon, 10 Jun 2024 at 18:54, Simon Glass <sjg@chromium.org> wrote: > > > > Hi, > > > > On Mon, 10 Jun 2024 at 09:42, Sughosh Ganu <sughosh.ganu@linaro.org> wrote: > > > > > > On Mon, 10 Jun 2024 at 20:47, Heinrich Schuchardt <xypron.glpk@gmx.de> 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 > > > > > > > > I am worried about the "frees up memory" case. > > > > > > > > When LMB frees memory we cannot add it back to EFI conventional memory > > > > as there might still be a file image lingering around that EFI should > > > > not overwrite. It has to stay marked as EfiLoaderCode or EfiLoaderData. > > > > > > So here is my basic doubt. Why would LMB free up memory if it still > > > has a valid image. If that is the case, the lmb_free API should not be > > > called? > > > > > > -sughosh > > > > > > > > > > > > > > How do you ensure that if a region reserved by LMB notification as > > > > EfiLoaderData is coalesced with some other allocation LMB is not > > > > requested to mark the coalesced region as reserved? > > > > > > > > @Tom > > > > > > > > Clinging to the existing logic that you can do anything when loading > > > > files is obviously leading us into coding hell. > > > > > > > > If somebody wants to load two images into the same location, he should > > > > be forced to unload the first image. This will allow us to have a single > > > > memory management system. > > > > It seems we really shouldn't use the words 'allocate' and 'free' when > > talking about LMB. They are simply reservations. > > Correct and while at it can we please make the code less confusing to > read. What we today mark as reserved isnt even trully reserved as it > can be overwritten. > struct lmb_region memory -> available memory we added on LMB. That's fine > struct lmb_region reserved -> can we rename this to 'used' and rename > LMB_NOOVERWRITE to LMB_RESERVED? Okay. Will incorporate this change in the next version. Thanks. -sughosh > > Thanks > /Ilias > > > I believe we have got > > into this situation due to an assumption that these two things are the > > same, but in U-Boot they certainly are not. LMB is a very lighweight > > and temporary reservation system to be used for a single boot process. > > > > Regards, > > Simon > > > > > > > > > > > > Best regards > > > > > > > > Heinrich > > > > > > > > > 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> > > > > > --- > > > > > 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 : > > > > > + EFI_BOOT_SERVICES_DATA, > > > > > + true); > > > > > + > > > > > + return status == EFI_SUCCESS ? 0 : -1; > > > > > +} > > > > > +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync); > > > > > #endif /* MEM_MAP_UPDATE_NOTIFY */ > > > > > > > > > > /** > > > > > @@ -275,18 +307,9 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, > > > > > return EFI_CARVE_LOOP_AGAIN; > > > > > } > > > > > > > > > > -/** > > > > > - * efi_add_memory_map_pg() - add pages to the memory map > > > > > - * > > > > > - * @start: start address, must be a multiple of EFI_PAGE_SIZE > > > > > - * @pages: number of pages to add > > > > > - * @memory_type: type of memory added > > > > > - * @overlap_only_ram: region may only overlap RAM > > > > > - * Return: status code > > > > > - */ > > > > > -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > > > - int memory_type, > > > > > - bool overlap_only_ram) > > > > > +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages, > > > > > + int memory_type, > > > > > + bool overlap_only_ram) > > > > > { > > > > > struct list_head *lhandle; > > > > > struct efi_mem_list *newlist; > > > > > @@ -395,6 +418,29 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > > > } > > > > > } > > > > > > > > > > + return EFI_SUCCESS; > > > > > +} > > > > > + > > > > > +/** > > > > > + * efi_add_memory_map_pg() - add pages to the memory map > > > > > + * > > > > > + * @start: start address, must be a multiple of EFI_PAGE_SIZE > > > > > + * @pages: number of pages to add > > > > > + * @memory_type: type of memory added > > > > > + * @overlap_only_ram: region may only overlap RAM > > > > > + * Return: status code > > > > > + */ > > > > > +static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, > > > > > + int memory_type, > > > > > + bool overlap_only_ram) > > > > > +{ > > > > > + efi_status_t status; > > > > > + > > > > > + status = __efi_add_memory_map_pg(start, pages, memory_type, > > > > > + overlap_only_ram); > > > > > + if (status != EFI_SUCCESS) > > > > > + return status; > > > > > + > > > > > if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)) > > > > > efi_map_update_notify(start, pages << EFI_PAGE_SHIFT, > > > > > memory_type == EFI_CONVENTIONAL_MEMORY ? > > > >
Hi Heinrich, On Wed, 12 Jun 2024 at 00:11, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > Am 12. Juni 2024 04:42:00 MESZ schrieb Simon Glass <sjg@chromium.org>: > >Hi Tom, > > > >On Tue, 11 Jun 2024 at 16:54, Tom Rini <trini@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 <trini@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 <trini@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.ganu@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
Hi Ilias, On Tue, 11 Jun 2024 at 23:49, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > [...] > > > > > > > > > > + 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, > > It does in some cases. E.g The efi variables allocate some pages when > the subsystem starts, the TCG protocol allocates the EventLog once > it's installed and I am pretty sure we have more. Both of those seem wrong to me, though. - EFI variables should use malloc() like other small amounts of data - TCG should probably use a bloblist, but in any case I suspect it is needed beyond EFI > > > 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. > > Heinrich already pointed out a potential danger in the current design. > If an EFI allocation happens *before* LMB comes up, we might end up > updating the efi memory map with the wrong attributes. That would lead > to the OS discarding memory areas that should be preserved and we > won't figure that out until the OS boots and blows up. Yes, I believe the EFI memory sort-out should happen when booting and not before. It is a bit like creating a plate of spaghetti if we do all this stuff randomly while trying to boot different things, retrying, etc.... Regards, Simon
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 : + EFI_BOOT_SERVICES_DATA, + true); + + return status == EFI_SUCCESS ? 0 : -1; +} +EVENT_SPY_FULL(EVT_LMB_MAP_UPDATE, lmb_mem_map_update_sync); #endif /* MEM_MAP_UPDATE_NOTIFY */ /** @@ -275,18 +307,9 @@ static s64 efi_mem_carve_out(struct efi_mem_list *map, return EFI_CARVE_LOOP_AGAIN; } -/** - * efi_add_memory_map_pg() - add pages to the memory map - * - * @start: start address, must be a multiple of EFI_PAGE_SIZE - * @pages: number of pages to add - * @memory_type: type of memory added - * @overlap_only_ram: region may only overlap RAM - * Return: status code - */ -static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, - int memory_type, - bool overlap_only_ram) +static efi_status_t __efi_add_memory_map_pg(u64 start, u64 pages, + int memory_type, + bool overlap_only_ram) { struct list_head *lhandle; struct efi_mem_list *newlist; @@ -395,6 +418,29 @@ static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, } } + return EFI_SUCCESS; +} + +/** + * efi_add_memory_map_pg() - add pages to the memory map + * + * @start: start address, must be a multiple of EFI_PAGE_SIZE + * @pages: number of pages to add + * @memory_type: type of memory added + * @overlap_only_ram: region may only overlap RAM + * Return: status code + */ +static efi_status_t efi_add_memory_map_pg(u64 start, u64 pages, + int memory_type, + bool overlap_only_ram) +{ + efi_status_t status; + + status = __efi_add_memory_map_pg(start, pages, memory_type, + overlap_only_ram); + if (status != EFI_SUCCESS) + return status; + if (CONFIG_IS_ENABLED(MEM_MAP_UPDATE_NOTIFY)) efi_map_update_notify(start, pages << EFI_PAGE_SHIFT, memory_type == EFI_CONVENTIONAL_MEMORY ?
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> --- lib/efi_loader/efi_memory.c | 70 ++++++++++++++++++++++++++++++------- 1 file changed, 58 insertions(+), 12 deletions(-)