* Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote: > 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 So if you don't actively hate the idea (which you don't seem to :) I can cook up a couple of patches and send the series out as an RFC - I think such reorganization is much better seen through the lens of an end result. I can then iterate/rebase the reorganization with low overhead, without committing them permanently yet. > 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 Yeah - that's the analogue of e820_entry->type, right? > 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. Makes sense. Just to make clear I got it right: AFAICS we have 'struct efi_boot_memmap' that describes all of these areas, and boot_memmap->map is a linear array of efi_memory_desc_t entries, where each entry describes a contiguous range of memory of a given type, correct? On that data structure level the notion of an 'area' doesn't really come up, which would be all the (discontiguous) entries of a given type. What we have is the 'map', the table, and the entries of the table. ( Regarding being pedantic: reading efi_insert_memmap() hit my pain+confusion threshold, and I think it's better to follow a hysteresis curve: i.e. instead of minimally moving it just below the pain threshold we might as well bite the bullet and be really pedantic and systematic about all of this in a single quick pass. That batches it all up and is also generally a lot more fun and productive to execute, as the end result will be a visible improvement (hopefully). We did that with the FPU code and the e820 code recently and it was rather successful. ) Also, would it make sense to actually turn the table into a real C array, while taking the "->desc_size != sizeof(efi_memory_desc_t)" future expansion compatibility constraint into account: - A single, quick bootstrap step allocates a proper array with known entry sizes. - mem_table->entries[i] would thus actully exist on the C level, with a mem_table->nr_entries limit - very similar to the e820 table code. - This would simplify the code at the expense of a single bootstrap function that does the allocation and copying from EFI-format table to Linux-format table. - By all means efi_early_memdesc_ptr() and for_each_efi_memory_desc*() are wrappers around a linear array with entry sizes unknown at build time - that complication would largely go away. Does the EFI runtime have any notion of the OS *modifying* the memory map and then the runtime relying on those modifications, or is it in practice treated as a read-only table? If we can make it a real array on the C level I was also hoping to sneak in the concept of an 'entry' somewhere, so that readers of the simple e820 table/entry logic would feel at home in the EFI code as well. Could we perhaps name 'descriptor' as 'entry', instead of 'region'? I.e. on the data structure level we'd have a clear hierarchy of table/entries/entry: efi_mem_table: ->entries[i] ->nr_entries where 'entry->type' distinguishes the different areas. It's all pretty straightforward and would allow some unification of utility functions with the e820 code perhaps. In fact if we do that then we could probably rename: struct e820_table => struct x86_mem_table struct e820_entry => struct x86_mem_entry ... and extend x86_mem_entry with an 'attr' field? That way all the information stored in the EFI entry could be stored in an x86_mem_entry - and the EFI code (and the e820 code) could use this more generic table/entry data structure directly and deal with the raw underlying data types as little as possible. The e820 code already has a decoupled 'struct boot_e820_entry' data type for the boot protocol ABI, so I think 'e820_entry' could be extended and made generic - at least in principle. There's a number of high level advantages from doing that: - Right now we put the EFI memory table entries into an e820 table and then feed that to the memblock allocator. This is a mild conceptual layering violation: EFI per definition has nothing to do with E820 BIOS calls. The e820_table/e820_entry is the de facto 'generic' entry already, and the naming should now reflect that I believe. - There's 23 uses of for_each_efi_memory_desc*(), so the simplification would be measurable even in low level EFI code. - I'd also use the opportunity and collect these various pieces of memory map functionality and decouple e820, EFI and generic memory map handling methods a bit more - and maybe start sharing/unifying some of it, such as user-defined memory maps which I believe are now duplicated to a certain extent. Is this is a rough direction you'd agree to, or is there anything I'm missing here? Thanks, Ingo
