On 27 June 2018 at 06:51, Prakhya, Sai Praneeth <sai.praneeth.prakhya@xxxxxxxxx> wrote: >> > + /* Free the memory allocated to the existing memory map */ >> > + efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, >> > + efi.memmap.late); >> > + >> > data.phys_map = addr; >> > data.size = efi.memmap.desc_size * nr_map; >> > data.desc_version = efi.memmap.desc_version; >> > -- >> > 2.7.4 >> > >> >> If only it were so simple :-) >> >> At this point, efi.memmap.phys_map could point to memory that was allocated >> early, allocated late or simply passed to the OS at boot time by the stub (in >> which case it was memblock_reserve()d but not memblock_alloc()d, and it >> should not be freed) >> > > Yes, completely agree that there could be three types of allocations for memmap. > I thought, > efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late); > > should work because the previous type of allocation should have been recorded in efi.memmap.late. > But, now I see this will fail for memblock_reserved() memory because it will be mistaken to > memblock_alloced() (I assumed both are almost similar :(). > >> So only allocations made with efi_memmap_alloc() should be freed here. > > Makes sense and I think that also means efi_memmap_free() should be called from function > that called efi_memmap_alloc() and not efi_memmap_install(). > >> I'm not sure /how/ we should keep track of that: perhaps it is simply a matter of >> replacing the boolean 'late' with an enum that describes where the memory >> came from that phys_map points to. > > I did try changing boolean late to enum and it seems to be working fine. I will do more > testing/clean up and will submit a patch for review. > > Also, could you please clarify if there is any specific reason why memory allocated > using memblock_reserve() shouldn't be freed. I mean, not with memblock_free() but I > think we could make it _available_ using free_bootmem() (or something similar, please > correct me if this is not the right API). On arm64, the memory map is provided to the core kernel by the stub, and after kexec, a pointer to the same memory map will be passed to the next kernel. So the kernel does not 'own' that allocation, and it should not free it or overwrite it. > If we allocate and install a new memory map (as > in case with efi_fake_memmap()), I think we should free the memory used by memory map > originally passed by EFI stub, because, at any point of time there should only be one active > memory map. If we don't free the original memory map passed by EFI stub, we will be having > two and hence will be leaking memory. > > Regards, > Sai -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
