* Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:
> +#if 0
> /*
> * Find a suitable spot for the trampoline.
> * This code is based on reserve_bios_regions().
> @@ -49,6 +50,9 @@ struct paging_config paging_prepare(void)
> /* Place the trampoline just below the end of low memory, aligned to 4k */
> paging_config.trampoline_start = bios_start - TRAMPOLINE_32BIT_SIZE;
> paging_config.trampoline_start = round_down(paging_config.trampoline_start, PAGE_SIZE);
> +#else
> + paging_config.trampoline_start = 0x99000;
> +#endif
So if it's suspected to be 'Video BIOS undeclared RAM use' related then wouldn't a
lower address be safer?
Such as:
paging_config.trampoline_start = 0x40000;
or so?
Also, could do a puts() hexdump of the affected memory area _before_ we overwrite
it? Is it empty? Could we add some debug warning that checks that it's all zeroes?
I also kind of regret that this remained a single commit:
3 files changed, 120 insertions(+), 1 deletion(-)
this should be split up further:
- one patch that adds trampoline space to the kernel image
- one patch that calculates the trampoline address and prints the address
- one or two patch that does the functional changes
- (any more split-up you can think of - early boot code is very fragile!)
It will be painful to rebase x86/mm but I think it's unavoidable at this stage.
There's also a few other things I don't like in paging_prepare():
1)
/* Check if LA57 is desired and supported */
if (IS_ENABLED(CONFIG_X86_5LEVEL) && native_cpuid_eax(0) >= 7 &&
(native_cpuid_ecx(7) & (1 << (X86_FEATURE_LA57 & 31))))
paging_config.l5_required = 1;
... it isn't explained why this feature CPU check is so complex.
2)
+ /* Place the trampoline just below the end of low memory, aligned to 4k */
+ paging_config.trampoline_start = bios_start - TRAMPOLINE_32BIT_SIZE;
+ paging_config.trampoline_start = round_down(paging_config.trampoline_start, PAGE_SIZE);
placing trampolines just below or just above BIOS images is fragile. Instead a
better heuristic is to use the "middle" of suspected available RAM and work from
there.
3)
+ /* Clear trampoline memory first */
+ memset(trampoline, 0, TRAMPOLINE_32BIT_SIZE);
Memory bootup state is typically all zeroes (except maybe for kexec), so this
should check that what it's clearing doesn't contain any data.
It should probably also clear this memory _after_ use.
4)
+ /*
+ * Set up a new page table that will be used for switching from 4-
+ * to 5-level paging or vice versa. In other cases trampoline
+ * wouldn't touch CR3.
+ *
+ * For 4- to 5-level paging transition, set up current CR3 as the
+ * first and the only entry in a new top-level page table.
+ *
+ * For 5- to 4-level paging transition, copy page table pointed by
+ * first entry in the current top-level page table as our new
+ * top-level page table. We just cannot point to the page table
+ * from trampoline as it may be above 4G.
+ */
+ if (paging_config.l5_required) {
+ trampoline[TRAMPOLINE_32BIT_PGTABLE_OFFSET] = __native_read_cr3() + _PAGE_TABLE_NOENC;
+ } else if (native_read_cr4() & X86_CR4_LA57) {
+ unsigned long src;
+
+ src = *(unsigned long *)__native_read_cr3() & PAGE_MASK;
+ memcpy(trampoline + TRAMPOLINE_32BIT_PGTABLE_OFFSET / sizeof(unsigned long),
+ (void *)src, PAGE_SIZE);
+ }
Why '+ _PAGE_TABLE_NOENC', while not ' |' ?
Also, it isn't clear what is where at this stage and it would be helpful to add
comments explaining the general purpose.
There's also two main objects here:
- the mode switching code trampoline
- the trampoline pagetable
it's not clear from this code where is which - and the naming isn't overly clear
either: is '*trampoline' the code, or the pagetable, or both?
We need to re-do this as we have now run into _exactly_ the kind of difficult to
debug bug that I was worried about when I insisted on the many iterations of this
patch-set...
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-tip-commits" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
|