* Sai Praneeth Prakhya <sai.praneeth.prakhya@xxxxxxxxx> wrote: > Presently, in EFI subsystem of kernel, every time kernel allocates memory for a > new EFI memory map, it forgets to free the memory occupied by old EFI memory map. > It does clear the mappings though (using efi_memmap_unmap()), but forgets to > free up the memory. Also, there is another minor issue, where in the newly > allocated memory isn't freed, should remap fail. > > The first issue is addressed by adding efi_memmap_free() to efi_memmap_install() > and the second issue is addressed by pushing the remap code into > efi_memmap_alloc() and there by handling the failure condition. > > Memory allocated to EFI memory map is leaked in below functions and hence they > are modified to fix the issue. Functions that modify EFI memmap are: > 1. efi_clean_memmap(), > 2. efi_fake_memmap(), > 3. efi_arch_mem_reserve(), > 4. efi_free_boot_services(), > 5. and __efi_enter_virtual_mode() > > More detailed explanation: > -------------------------- > A typical boot flow on EFI supported x86_64 machines might look something like > below > 1. EFI memory map is passed by firmware to kernel. > 2. Kernel does a memblock_reserve() on this memory > (see efi_memblock_x86_reserve_range()). > 3. This memory map is checked for invalid entries in efi_clean_memmap(). If any > invalid entries are found, they are omitted from EFI memory map but the > memory occupied by these invalid EFI memory descriptors isn't freed. > 3. To further process this memory map (see efi_fake_memmap(), efi_bgrt_init() > and efi_esrt_init()), kernel allocates memory using efi_memmap_alloc() and > copies the processed memory map to newly allocated memory but it forgets to > free memory occupied by old EFI memory map. > 4. Further, in efi_map_regions() the EFI memory map is processed again to > include only EFI memory descriptors of type Runtime Code/Data and Boot > Code/Data. Again, memory is allocated for this new memory map through > realloc_pages() and the old EFI memory map is not freed. > 5. After SetVirtualAddressMap() is done, the EFI memory map is processed again > to have only EFI memory descriptors of type Runtime Code/Data. Again, memory > is allocated for this new memory map through efi_memmap_alloc() and the old > EFI memory map is not freed. So it was only halfway through the changelog that I realized that by 'free the memory occupied by old EFI memory map' you didn't mean the EFI memory map itself (the system RAM described), but the EFI memory map *table*, this relatively small array of descriptors that we allocate and reallocate every now and then according to the limitations of the (largely non-existent yet) early boot memory management system... Could we please use much more precise terminology when referring to these entities, in changelogs and code alike? I.e.: - 'EFI memory map' is the whole map and the contents - 'EFI memory map table' is the table structure itself, without regard of its contents - 'EFI memory map table entry/descriptor' is an entry in the table. This kind of clarity is what we are using on the e820 memory map side as well: we have struct e820_table and struct e820_entry. Ard, would you object to standardizing the EFI code in a similar fashion as well: efi_mem_table and efi_mem_entry? Parameters and local variables should be named unambiguously following these concepts as well. There should be no opaque 'new' variables *especially* where the primary role of the efi_memmap_insert() API call is to add a new *entry* - it should be 'new_efi_map_table' (or 'new_table' where it's unambiguous) instead, and new entries added should be 'new_entry' - etc. Once this is reorganized and clarified it should be a lot more easy to review 'table freeing' patches. I can help out with patches if there's no conceptual objections. Thanks, Ingo
