On Wed, 5 Dec 2018 at 08:41, Ingo Molnar <mi...@kernel.org> wrote: > > > * Sai Praneeth Prakhya <sai.praneeth.prak...@intel.com> 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? >
I share your concern, but 'EFI memory map' already has an established meaning beyond Linux, as the set of memory map entries describing the address space to the OS. (for instance, the EFI boot service to retrieve it is called GetMemoryMap()) I am not sure what 'the whole EFI memory map' would mean'. It describes all of DRAM (used, unused or reserved) and on top of that, some peripheral mappings that need to be virtually remapped by the OS so that the runtime services can use them (e.g., RTC or flash). So from UEFI spec pov, your notion of 'EFI memory map' is just the entire memory map. So if we are going to rename these things wholesale (which is fine with me), I'd prefer it if we can drop the 'map' entirely. EFI memory table EFI memory table entry EFI memory table descriptor For things described by the EFI memory table, we need to be more specific anyway, since it typically describes everything, so EFI boot services area EFI runtime services area EFI reserved area EFI conventional memory area EFI MMIO area etc etc Since we're being pedantic, it also makes sense to decide now whether 'area' refers to all [discontiguous] regions or just one of them. I'd say the former, and use 'region' for the latter, i.e., an area may be made up of several regions, but only one exists of each type. > 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. I'm happy to devote some time to this as well.
