Re: [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline()

["Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>]

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