LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH for v4.17 0/2] Fix two crashes in decomression code
@ 2018-05-10 17:38 Kirill A. Shutemov
  2018-05-10 17:38 ` [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline() Kirill A. Shutemov
  2018-05-10 17:38 ` [PATCH 2/2] x86/boot/compressed/64: Fix moving page table out of trampoline memory Kirill A. Shutemov
  0 siblings, 2 replies; 8+ messages in thread
From: Kirill A. Shutemov @ 2018-05-10 17:38 UTC (permalink / raw)
  To: Ingo Molnar, x86, Thomas Gleixner, H. Peter Anvin
  Cc: Hugh Dickins, linux-kernel, Kirill A. Shutemov

Here's two fixes in early boot code that leads to instant reboot.

Kirill A. Shutemov (2):
  x86/boot/compressed/64: Set up GOT for paging_prepare() and
    cleanup_trampoline()
  x86/boot/compressed/64: Fix moving page table out of trampoline memory

 arch/x86/boot/compressed/head_64.S    | 77 ++++++++++++++++++++++-----
 arch/x86/boot/compressed/pgtable_64.c | 14 ++---
 2 files changed, 67 insertions(+), 24 deletions(-)

-- 
2.17.0

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline()
  2018-05-10 17:38 [PATCH for v4.17 0/2] Fix two crashes in decomression code Kirill A. Shutemov
@ 2018-05-10 17:38 ` Kirill A. Shutemov
  2018-05-13 18:55   ` Thomas Gleixner
  2018-05-10 17:38 ` [PATCH 2/2] x86/boot/compressed/64: Fix moving page table out of trampoline memory Kirill A. Shutemov
  1 sibling, 1 reply; 8+ messages in thread
From: Kirill A. Shutemov @ 2018-05-10 17:38 UTC (permalink / raw)
  To: Ingo Molnar, x86, Thomas Gleixner, H. Peter Anvin
  Cc: Hugh Dickins, linux-kernel, Kirill A. Shutemov

Eric and Hugh have reported instant reboot due to my recent changes in
decompression code.

The root cause is that I didn't realize that we need to adjust GOT to be
able to run C code that early.

The problem is only visible with old toolchain. Binutils >= 2.24 is able
to eliminate GOT references by replacing them with RIP-relative address
loads[1].

We need to adjust GOT two times:
 - before calling paging_prepare() with address the binary was loaded by bootloader
 - before relocating the kernel to the new place with relocation address

[1] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=80d873266dec

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Fixes: 194a9749c73d ("x86/boot/compressed/64: Handle 5-level paging boot if kernel is above 4G")
Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Reported-by: Hugh Dickins <hughd@google.com>
---
 arch/x86/boot/compressed/head_64.S | 66 ++++++++++++++++++++++++------
 1 file changed, 53 insertions(+), 13 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index fca012baba19..6cbb2d64c91e 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -305,6 +305,21 @@ ENTRY(startup_64)
 	/* Set up the stack */
 	leaq	boot_stack_end(%rbx), %rsp
 
+	/*
+	 * paging_prepare() and cleanup_trampoline() below can have GOT
+	 * references. Adjust the table with address we are running at.
+	 */
+
+	/* The GOP was not adjusted before */
+	xorq	%rax, %rax
+
+	/* Calculate the address the binary is loaded at. */
+	call	1f
+1:	popq	%rdi
+	subq	$1b, %rdi
+
+	call	adjust_gop
+
 	/*
 	 * At this point we are in long mode with 4-level paging enabled,
 	 * but we might want to enable 5-level paging or vice versa.
@@ -381,6 +396,24 @@ trampoline_return:
 	pushq	$0
 	popfq
 
+	/*
+	 * Previously we've adjusted the GOT with address the binary was
+	 * loaded at. Now we need to re-adjust for relocation address.
+	 */
+
+	/*
+	 * Calculate the address the binary is loaded at.
+	 * This address was used to adjust the table before and we need to
+	 * undo the change.
+	 */
+	call	1f
+1:	popq	%rax
+	subq	$1b, %rax
+
+	/* The new adjustment is relocation address */
+	movq	%rbx, %rdi
+	call	adjust_gop
+
 /*
  * Copy the compressed kernel to the end of our buffer
  * where decompression in place becomes safe.
@@ -481,19 +514,6 @@ relocated:
 	shrq	$3, %rcx
 	rep	stosq
 
-/*
- * Adjust our own GOT
- */
-	leaq	_got(%rip), %rdx
-	leaq	_egot(%rip), %rcx
-1:
-	cmpq	%rcx, %rdx
-	jae	2f
-	addq	%rbx, (%rdx)
-	addq	$8, %rdx
-	jmp	1b
-2:
-	
 /*
  * Do the extraction, and jump to the new kernel..
  */
@@ -512,6 +532,26 @@ relocated:
  */
 	jmp	*%rax
 
+/*
+ * Adjust global offest table
+ *
+ * RAX is previous adjustment of the table to undo (0 if it's the first time we touch GOP).
+ * RDI is the new adjustment to apply.
+ */
+adjust_gop:
+	/* Walk through the GOT adding the address to the entries */
+	leaq	_got(%rip), %rdx
+	leaq	_egot(%rip), %rcx
+1:
+	cmpq	%rcx, %rdx
+	jae	2f
+	subq	%rax, (%rdx)	/* Undo previous adjustment */
+	addq	%rdi, (%rdx)	/* Apply the new adjustment */
+	addq	$8, %rdx
+	jmp	1b
+2:
+	ret
+
 	.code32
 /*
  * This is the 32-bit trampoline that will be copied over to low memory.
-- 
2.17.0

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] x86/boot/compressed/64: Fix moving page table out of trampoline memory
  2018-05-10 17:38 [PATCH for v4.17 0/2] Fix two crashes in decomression code Kirill A. Shutemov
  2018-05-10 17:38 ` [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline() Kirill A. Shutemov
@ 2018-05-10 17:38 ` Kirill A. Shutemov
  2018-05-13 18:56   ` Thomas Gleixner
  1 sibling, 1 reply; 8+ messages in thread
From: Kirill A. Shutemov @ 2018-05-10 17:38 UTC (permalink / raw)
  To: Ingo Molnar, x86, Thomas Gleixner, H. Peter Anvin
  Cc: Hugh Dickins, linux-kernel, Kirill A. Shutemov

top_pgtable address has to be calculated relative to where the kernel
image will be relocated for decompression, not relative to position of
kernel is running at the moment. We do the same for the rest of page
table we use the stage. It makes them safe from being overwritten during
decompression.

Calculate the address of top_pgtable in assembly and pass down to
cleanup_trampoline().

Move the page table to .pgtable section where the rest of page tables
are. The section is @nobits so we save 4k in kernel image.

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")
---
 arch/x86/boot/compressed/head_64.S    | 11 +++++++++++
 arch/x86/boot/compressed/pgtable_64.c | 14 +++-----------
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 6cbb2d64c91e..1acf7b5d16cf 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -385,10 +385,14 @@ trampoline_return:
 	/*
 	 * cleanup_trampoline() would restore trampoline memory.
 	 *
+	 * RDI is address of the page table to use instead of page table
+	 * in trampoline memory (if required).
+	 *
 	 * RSI holds real mode data and needs to be preserved across
 	 * this function call.
 	 */
 	pushq	%rsi
+	leaq	top_pgtable(%rbx), %rdi
 	call	cleanup_trampoline
 	popq	%rsi
 
@@ -689,3 +693,10 @@ 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.
+ */
+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..a362fa0b849c 100644
--- a/arch/x86/boot/compressed/pgtable_64.c
+++ b/arch/x86/boot/compressed/pgtable_64.c
@@ -22,14 +22,6 @@ struct paging_config {
 /* Buffer to preserve trampoline memory */
 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);
-
 /*
  * Trampoline address will be printed by extract_kernel() for debugging
  * purposes.
@@ -134,7 +126,7 @@ struct paging_config paging_prepare(void)
 	return paging_config;
 }
 
-void cleanup_trampoline(void)
+void cleanup_trampoline(void *pgtable)
 {
 	void *trampoline_pgtable;
 
@@ -145,8 +137,8 @@ void cleanup_trampoline(void)
 	 * if it's there.
 	 */
 	if ((void *)__native_read_cr3() == trampoline_pgtable) {
-		memcpy(top_pgtable, trampoline_pgtable, PAGE_SIZE);
-		native_write_cr3((unsigned long)top_pgtable);
+		memcpy(pgtable, trampoline_pgtable, PAGE_SIZE);
+		native_write_cr3((unsigned long)pgtable);
 	}
 
 	/* Restore trampoline memory */
-- 
2.17.0

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline()
  2018-05-10 17:38 ` [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline() Kirill A. Shutemov
@ 2018-05-13 18:55   ` Thomas Gleixner
  2018-05-13 20:03     ` Kirill A. Shutemov
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Gleixner @ 2018-05-13 18:55 UTC (permalink / raw)
  To: Kirill A. Shutemov
  Cc: Ingo Molnar, x86, H. Peter Anvin, Hugh Dickins, linux-kernel

On Thu, 10 May 2018, Kirill A. Shutemov wrote:

> +	/*
> +	 * paging_prepare() and cleanup_trampoline() below can have GOT
> +	 * references. Adjust the table with address we are running at.
> +	 */
> +
> +	/* The GOP was not adjusted before */

GOP == EFI speak for Graphics Output Protocol. What the heck? 

> +	xorq	%rax, %rax

And this clearing of RAX is related to this because? Sure you need it for
adjust_got() but adding a comment to that is too much asked for, right?

> +	/* Calculate the address the binary is loaded at. */
> +	call	1f
> +1:	popq	%rdi
> +	subq	$1b, %rdi
> +
> +	call	adjust_gop
> +
>  	/*
>  	 * At this point we are in long mode with 4-level paging enabled,
>  	 * but we might want to enable 5-level paging or vice versa.
> @@ -381,6 +396,24 @@ trampoline_return:
>  	pushq	$0
>  	popfq
>  
> +	/*
> +	 * Previously we've adjusted the GOT with address the binary was
> +	 * loaded at. Now we need to re-adjust for relocation address.
> +	 */

Breaking up those comments makes it more readable, right?

> +	/*
> +	 * Calculate the address the binary is loaded at.
> +	 * This address was used to adjust the table before and we need to
> +	 * undo the change.
> +	 */
> +	call	1f
> +1:	popq	%rax
> +	subq	$1b, %rax
> +
> +	/* The new adjustment is relocation address */

  is the relocation address

> +	movq	%rbx, %rdi
> +	call	adjust_gop
> +
>  /*
>   * Copy the compressed kernel to the end of our buffer
>   * where decompression in place becomes safe.
> @@ -481,19 +514,6 @@ relocated:
>  	shrq	$3, %rcx
>  	rep	stosq
>  
> -/*
> - * Adjust our own GOT
> - */
> -	leaq	_got(%rip), %rdx
> -	leaq	_egot(%rip), %rcx
> -1:
> -	cmpq	%rcx, %rdx
> -	jae	2f
> -	addq	%rbx, (%rdx)
> -	addq	$8, %rdx
> -	jmp	1b
> -2:
> -	
>  /*
>   * Do the extraction, and jump to the new kernel..
>   */
> @@ -512,6 +532,26 @@ relocated:
>   */
>  	jmp	*%rax
>  
> +/*
> + * Adjust global offest table

offest? 

> + *
> + * RAX is previous adjustment of the table to undo (0 if it's the first time we touch GOP).

  is the previous

And there is no reason to make that line overly long.

> + * RDI is the new adjustment to apply.
> + */
> +adjust_gop:
> +	/* Walk through the GOT adding the address to the entries */
> +	leaq	_got(%rip), %rdx
> +	leaq	_egot(%rip), %rcx
> +1:
> +	cmpq	%rcx, %rdx
> +	jae	2f
> +	subq	%rax, (%rdx)	/* Undo previous adjustment */
> +	addq	%rdi, (%rdx)	/* Apply the new adjustment */
> +	addq	$8, %rdx
> +	jmp	1b
> +2:
> +	ret

I'm really tired of your carelessness. The amount of half baken stuff you
submit is way above the tolerance level by now. I asked you several times
to be more careful, but you simply do not care at all. Get your act
together finally.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] x86/boot/compressed/64: Fix moving page table out of trampoline memory
  2018-05-10 17:38 ` [PATCH 2/2] x86/boot/compressed/64: Fix moving page table out of trampoline memory Kirill A. Shutemov
@ 2018-05-13 18:56   ` Thomas Gleixner
  2018-05-13 20:05     ` Kirill A. Shutemov
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Gleixner @ 2018-05-13 18:56 UTC (permalink / raw)
  To: Kirill A. Shutemov
  Cc: Ingo Molnar, x86, H. Peter Anvin, Hugh Dickins, linux-kernel

On Thu, 10 May 2018, Kirill A. Shutemov wrote:

> top_pgtable address has to be calculated relative to where the kernel
> image will be relocated for decompression, not relative to position of
> kernel is running at the moment. We do the same for the rest of page
> table we use the stage. It makes them safe from being overwritten during
> decompression.
> 
> Calculate the address of top_pgtable in assembly and pass down to
> cleanup_trampoline().
> 
> Move the page table to .pgtable section where the rest of page tables
> are. The section is @nobits so we save 4k in kernel image.

So this is supposed to be a fix, but the whole changelog talks about WHAT
the patch does and not WHY. Darn, we need proper description of the failure
which is about to be fixed.

It's not that hard and I'm really tired to tell you that over and over.

>  	/*
>  	 * cleanup_trampoline() would restore trampoline memory.
>  	 *
> +	 * RDI is address of the page table to use instead of page table
> +	 * in trampoline memory (if required).

Do you really believe that you understand that comment 6 month from now?

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline()
  2018-05-13 18:55   ` Thomas Gleixner
@ 2018-05-13 20:03     ` Kirill A. Shutemov
  2018-05-13 21:30       ` Thomas Gleixner
  0 siblings, 1 reply; 8+ messages in thread
From: Kirill A. Shutemov @ 2018-05-13 20:03 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, x86, H. Peter Anvin, Hugh Dickins, linux-kernel

On Sun, May 13, 2018 at 06:55:46PM +0000, Thomas Gleixner wrote:
> On Thu, 10 May 2018, Kirill A. Shutemov wrote:
> 
> > +	/*
> > +	 * paging_prepare() and cleanup_trampoline() below can have GOT
> > +	 * references. Adjust the table with address we are running at.
> > +	 */
> > +
> > +	/* The GOP was not adjusted before */
> 
> GOP == EFI speak for Graphics Output Protocol. What the heck? 

I was not aware about Graphics Output Protocol.
> 
> > +	xorq	%rax, %rax
> 
> And this clearing of RAX is related to this because? Sure you need it for
> adjust_got() but adding a comment to that is too much asked for, right?

Huh? The comment just above the line describes why it's needed.

> > +	/* Calculate the address the binary is loaded at. */
> > +	call	1f
> > +1:	popq	%rdi
> > +	subq	$1b, %rdi
> > +
> > +	call	adjust_gop
> > +
> >  	/*
> >  	 * At this point we are in long mode with 4-level paging enabled,
> >  	 * but we might want to enable 5-level paging or vice versa.
> > @@ -381,6 +396,24 @@ trampoline_return:
> >  	pushq	$0
> >  	popfq
> >  
> > +	/*
> > +	 * Previously we've adjusted the GOT with address the binary was
> > +	 * loaded at. Now we need to re-adjust for relocation address.
> > +	 */
> 
> Breaking up those comments makes it more readable, right?

Yes, I think so.

The first comment is for the whole block of code below. The second is the
comment for the first step.

> > +	/*
> > +	 * Calculate the address the binary is loaded at.
> > +	 * This address was used to adjust the table before and we need to
> > +	 * undo the change.
> > +	 */
> > +	call	1f
> > +1:	popq	%rax
> > +	subq	$1b, %rax
> > +
> > +	/* The new adjustment is relocation address */
> 
>   is the relocation address
> 
> > +	movq	%rbx, %rdi
> > +	call	adjust_gop
> > +
> >  /*
> >   * Copy the compressed kernel to the end of our buffer
> >   * where decompression in place becomes safe.
> > @@ -481,19 +514,6 @@ relocated:
> >  	shrq	$3, %rcx
> >  	rep	stosq
> >  
> > -/*
> > - * Adjust our own GOT
> > - */
> > -	leaq	_got(%rip), %rdx
> > -	leaq	_egot(%rip), %rcx
> > -1:
> > -	cmpq	%rcx, %rdx
> > -	jae	2f
> > -	addq	%rbx, (%rdx)
> > -	addq	$8, %rdx
> > -	jmp	1b
> > -2:
> > -	
> >  /*
> >   * Do the extraction, and jump to the new kernel..
> >   */
> > @@ -512,6 +532,26 @@ relocated:
> >   */
> >  	jmp	*%rax
> >  
> > +/*
> > + * Adjust global offest table
> 
> offest? 
> 
> > + *
> > + * RAX is previous adjustment of the table to undo (0 if it's the first time we touch GOP).
> 
>   is the previous
> 
> And there is no reason to make that line overly long.
> 
> > + * RDI is the new adjustment to apply.
> > + */
> > +adjust_gop:
> > +	/* Walk through the GOT adding the address to the entries */
> > +	leaq	_got(%rip), %rdx
> > +	leaq	_egot(%rip), %rcx
> > +1:
> > +	cmpq	%rcx, %rdx
> > +	jae	2f
> > +	subq	%rax, (%rdx)	/* Undo previous adjustment */
> > +	addq	%rdi, (%rdx)	/* Apply the new adjustment */
> > +	addq	$8, %rdx
> > +	jmp	1b
> > +2:
> > +	ret
> 
> I'm really tired of your carelessness. The amount of half baken stuff you
> submit is way above the tolerance level by now. I asked you several times
> to be more careful, but you simply do not care at all. Get your act
> together finally.

I don't think it's fair.

Yes, I have hard time write correctly, even in my native languages.
I'm blind to mistakes I do. I'm sorry about them.

But I do care about bugs in my code and I do my best addressing them.
It took a lot of effort to find root cause of the bug and your statement
about my carelessness doesn't match my assessment.

Have a nice day.

-- 
 Kirill A. Shutemov

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] x86/boot/compressed/64: Fix moving page table out of trampoline memory
  2018-05-13 18:56   ` Thomas Gleixner
@ 2018-05-13 20:05     ` Kirill A. Shutemov
  0 siblings, 0 replies; 8+ messages in thread
From: Kirill A. Shutemov @ 2018-05-13 20:05 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, x86, H. Peter Anvin, Hugh Dickins, linux-kernel

On Sun, May 13, 2018 at 06:56:14PM +0000, Thomas Gleixner wrote:
> On Thu, 10 May 2018, Kirill A. Shutemov wrote:
> 
> > top_pgtable address has to be calculated relative to where the kernel
> > image will be relocated for decompression, not relative to position of
> > kernel is running at the moment. We do the same for the rest of page
> > table we use the stage. It makes them safe from being overwritten during
> > decompression.
> > 
> > Calculate the address of top_pgtable in assembly and pass down to
> > cleanup_trampoline().
> > 
> > Move the page table to .pgtable section where the rest of page tables
> > are. The section is @nobits so we save 4k in kernel image.
> 
> So this is supposed to be a fix, but the whole changelog talks about WHAT
> the patch does and not WHY. Darn, we need proper description of the failure
> which is about to be fixed.

"It makes them safe from being overwritten during decompression."

> It's not that hard and I'm really tired to tell you that over and over.
> 
> >  	/*
> >  	 * cleanup_trampoline() would restore trampoline memory.
> >  	 *
> > +	 * RDI is address of the page table to use instead of page table
> > +	 * in trampoline memory (if required).
> 
> Do you really believe that you understand that comment 6 month from now?

Yes, I think I will.

-- 
 Kirill A. Shutemov

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline()
  2018-05-13 20:03     ` Kirill A. Shutemov
@ 2018-05-13 21:30       ` Thomas Gleixner
  0 siblings, 0 replies; 8+ messages in thread
From: Thomas Gleixner @ 2018-05-13 21:30 UTC (permalink / raw)
  To: Kirill A. Shutemov
  Cc: Ingo Molnar, x86, H. Peter Anvin, Hugh Dickins, linux-kernel

On Sun, 13 May 2018, Kirill A. Shutemov wrote:
> On Sun, May 13, 2018 at 06:55:46PM +0000, Thomas Gleixner wrote:
> > On Thu, 10 May 2018, Kirill A. Shutemov wrote:
> > 
> > > +	/*
> > > +	 * paging_prepare() and cleanup_trampoline() below can have GOT
> > > +	 * references. Adjust the table with address we are running at.
> > > +	 */
> > > +
> > > +	/* The GOP was not adjusted before */
> > 
> > GOP == EFI speak for Graphics Output Protocol. What the heck? 
> 
> I was not aware about Graphics Output Protocol.

That does not matter. GOP what ever you think it is has nothing to do
here. This is about GOT or am I missing something? 

> > > +	xorq	%rax, %rax
> > 
> > And this clearing of RAX is related to this because? Sure you need it for
> > adjust_got() but adding a comment to that is too much asked for, right?
> 
> Huh? The comment just above the line describes why it's needed.

No it does not. It is a comment which has only value when you first read
the comment above the function which is called 5 lines later. Comments
should make sense on their own. To be honest I did not even make the
connection when I read the function later.

> > > +	/* Calculate the address the binary is loaded at. */
> > > +	call	1f
> > > +1:	popq	%rdi
> > > +	subq	$1b, %rdi
> > > +
> > > +	call	adjust_gop
> > > +
> > >  	/*
> > >  	 * At this point we are in long mode with 4-level paging enabled,
> > >  	 * but we might want to enable 5-level paging or vice versa.
> > > @@ -381,6 +396,24 @@ trampoline_return:
> > >  	pushq	$0
> > >  	popfq
> > >  
> > > +	/*
> > > +	 * Previously we've adjusted the GOT with address the binary was
> > > +	 * loaded at. Now we need to re-adjust for relocation address.
> > > +	 */
> > 
> > Breaking up those comments makes it more readable, right?
> 
> Yes, I think so.
> 
> The first comment is for the whole block of code below. The second is the
> comment for the first step.

Sorry no. It's just confusing as hell and a few weeks down the road it
looks like somebody removed the code between the comments and forgot to
update them. You can put an empty line into one comment block to separate
paragraphs.

> Yes, I have hard time write correctly, even in my native languages.
> I'm blind to mistakes I do. I'm sorry about them.

Sorry, that you have a problem with that, but you could have told me
offlist long ago and we would have found a solution for this.

Not knowing that, it just looks like being careless to the other side.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-05-13 21:30 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-10 17:38 [PATCH for v4.17 0/2] Fix two crashes in decomression code Kirill A. Shutemov
2018-05-10 17:38 ` [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline() Kirill A. Shutemov
2018-05-13 18:55   ` Thomas Gleixner
2018-05-13 20:03     ` Kirill A. Shutemov
2018-05-13 21:30       ` Thomas Gleixner
2018-05-10 17:38 ` [PATCH 2/2] x86/boot/compressed/64: Fix moving page table out of trampoline memory Kirill A. Shutemov
2018-05-13 18:56   ` Thomas Gleixner
2018-05-13 20:05     ` Kirill A. Shutemov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).