lkml.org 
[lkml]   [2018]   [May]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 1/2] x86/boot/compressed/64: Set up GOT for paging_prepare() and cleanup_trampoline()
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
\
 
 \ /
  Last update: 2018-05-13 22:04    [W:0.073 / U:1.680 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site