* Ard Biesheuvel <[email protected]> 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
