Messages in this thread Patch in this message | | | Date | Thu, 3 May 2018 11:38:49 +0300 | From | "Kirill A. Shutemov" <> | Subject | Re: [PATCH] x86/boot/compressed: Exclude 'top_pgtable' from relocation |
| |
On Wed, May 02, 2018 at 08:42:30PM -0700, Hugh Dickins wrote: > On Wed, 2 May 2018, Kirill A. Shutemov wrote: > > > startup_64() copies kernel (including .data section) to the new place. > > It's required for safe in-place decompression. > > > > This is a problem if the original place is referenced: by mistake I've > > put 'top_pgtable' into .data section and the address is loaded into CR3. > > If the original place gets overwritten during image decompression the > > kernel will crash and the machine will be rebooted. > > > > Move 'top_pgtable' into .pgtable section where the rest of page tables > > are. This section is not subject for relocation. > > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > > Fixes: e9d0e6330eb8 ("x86/boot/compressed/64: Prepare new top-level page table for trampoline") > > Thanks for the Cc, Kirill, which I presume was because I'd mentioned > to you that I was unable to boot 4.17-rc on laptop or workstation.
Right.
> Which is still so with 4.17-rc3, and I'm sorry to say still so with this > patch: even if I add the fix which I think this patch needs, see below.
Hm.. Could you share your config?
IIRC, you use legacy boot. What bootloader?
> I did bisect on Monday, and the first bad was your commit 194a9749c73d > "x86/boot/compressed/64: Handle 5-level paging boot if kernel is above 4G". > (Cc'ing Dave since his PTI Global work was my other suspect, but that's > off the hook - if I revert just 194a9749c73d then I have no trouble.) > > Am I really the only one getting immediate reboot on x86_64?
There was one more thread:
http://lkml.kernel.org/r/5ecfeb13-84e4-f1ef-bd30-391674b2050a@gmail.com
But no firm conclusion, only blaming GCC without a good reason. BTW, what compiler do you use?
> Perhaps everyone else has machines with 5-level page tables now ?-)
No :)
> I've looked at the changes a little, and tried a few things (hoping to > avoid a long back and forth describing and trying things for you); but > no success yet, and rather out of my depth with these changes - I've > not had to delve into boot/compressed before. > > (I did briefly get excited by the > trampoline_32bit + TRAMPOLINE_32BIT_PGTABLE_OFFSET > in cleanup_trampoline(), which lacks a "/ sizeof(unsigned long)"; > but since ...PGTABLE_OFFSET is 0 anyway, that's nothing but cosmetic.)
It worth fixing anyway. Thanks for pointing it out.
> > --- > > arch/x86/boot/compressed/head_64.S | 8 ++++++++ > > arch/x86/boot/compressed/pgtable_64.c | 4 +--- > > 2 files changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S > > index fca012baba19..c433c21703e6 100644 > > --- a/arch/x86/boot/compressed/head_64.S > > +++ b/arch/x86/boot/compressed/head_64.S > > @@ -649,3 +649,11 @@ boot_stack_end: > > .balign 4096 > > pgtable: > > .fill BOOT_PGT_SIZE, 1, 0 > > + > > +/* > > + * The page table is going to be used instead of page table in the trampoline > > + * memory. > > + */ > > + .global top_pgtable > > +top_pgtable: > > + .fill PAGE_SIZE, 1, 0 > > diff --git a/arch/x86/boot/compressed/pgtable_64.c b/arch/x86/boot/compressed/pgtable_64.c > > index 32af1cbcd903..3a0578f54550 100644 > > --- a/arch/x86/boot/compressed/pgtable_64.c > > +++ b/arch/x86/boot/compressed/pgtable_64.c > > @@ -25,10 +25,8 @@ static char trampoline_save[TRAMPOLINE_32BIT_SIZE]; > > /* > > * The page table is going to be used instead of page table in the trampoline > > * memory. > > - * > > - * It must not be in BSS as BSS is cleared after cleanup_trampoline(). > > */ > > -static char top_pgtable[PAGE_SIZE] __aligned(PAGE_SIZE) __section(.data); > > +extern char *top_pgtable; > > Doesn't that need to be extern char top_pgtable[] ?
Ouch. That's embarrassing.
So in my case the top_pgtable is zero and apparently it's good enough place to put the page table. It boots :P
The patch is bogus and I still don't understand what is going on.
Could you please check if bypassing cleanup_trampoline() altogether fixes the issue for you:
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S index fca012baba19..73821ac626f6 100644 --- a/arch/x86/boot/compressed/head_64.S +++ b/arch/x86/boot/compressed/head_64.S @@ -367,16 +367,6 @@ trampoline_return: /* Restore the stack, the 32-bit trampoline uses its own stack */ leaq boot_stack_end(%rbx), %rsp - /* - * cleanup_trampoline() would restore trampoline memory. - * - * RSI holds real mode data and needs to be preserved across - * this function call. - */ - pushq %rsi - call cleanup_trampoline - popq %rsi - /* Zero EFLAGS */ pushq $0 popfq -- Kirill A. Shutemov
| |