[PATCH 0/2] Misc comments fixup in relocate_kernel_64.S

[PATCH 0/2] Misc comments fixup in relocate_kernel_64.S

[Kai Huang]

The assembly code of relcocate_kernel() takes some time to follow for
newbies like me.  This series adds some comments to try to improve the
readability hoping they could be helpful to others too.  Also fix an
error (IIUC) in one comment.

Kai Huang (2):
  x86/kexec: Fix a comment of swap_pages() assembly
  x86/kexec: Add comments around swap_pages() assembly to improve
    readability

 arch/x86/kernel/relocate_kernel_64.S | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)


base-commit: b8c7cbc324dc17b9e42379b42603613580bec2d8
-- 
2.45.2

[PATCH 1/2] x86/kexec: Fix a comment of swap_pages() assembly

[Kai Huang]

When relocate_kernel() gets called, %rdi holds 'indirection_page' and
%rsi holds 'page_list'.  And %rdi always holds 'indirection_page' when
swap_pages() is called.

Therefore the comment of the first line code of swap_pages()

	movq    %rdi, %rcx      /* Put the page_list in %rcx */

.. isn't correct because it actually moves the 'indirection_page' to
the %rcx.  Fix it.

Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/kernel/relocate_kernel_64.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
index 042c9a0334e9..f7a3ca3dee53 100644
--- a/arch/x86/kernel/relocate_kernel_64.S
+++ b/arch/x86/kernel/relocate_kernel_64.S
@@ -258,7 +258,7 @@ SYM_CODE_END(virtual_mapped)
 	/* Do the copies */
 SYM_CODE_START_LOCAL_NOALIGN(swap_pages)
 	UNWIND_HINT_END_OF_STACK
-	movq	%rdi, %rcx	/* Put the page_list in %rcx */
+	movq	%rdi, %rcx	/* Put the indirection_page in %rcx */
 	xorl	%edi, %edi
 	xorl	%esi, %esi
 	jmp	1f
-- 
2.45.2

Re: [PATCH 1/2] x86/kexec: Fix a comment of swap_pages() assembly

[Kirill A. Shutemov]

On Tue, Aug 20, 2024 at 12:21:11AM +1200, Kai Huang wrote:
> When relocate_kernel() gets called, %rdi holds 'indirection_page' and
> %rsi holds 'page_list'.  And %rdi always holds 'indirection_page' when
> swap_pages() is called.
> 
> Therefore the comment of the first line code of swap_pages()
> 
> 	movq    %rdi, %rcx      /* Put the page_list in %rcx */
> 
> .. isn't correct because it actually moves the 'indirection_page' to
> the %rcx.  Fix it.
> 
> Signed-off-by: Kai Huang <kai.huang@intel.com>

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

Looks like it got broken by 4bfaaef01a1b ("[PATCH] Avoid overwriting the
current pgd (V4, x86_64)")

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH 1/2] x86/kexec: Fix a comment of swap_pages() assembly

[Huang, Kai]

On Tue, 2024-08-20 at 11:40 +0300, Kirill A. Shutemov wrote:
> On Tue, Aug 20, 2024 at 12:21:11AM +1200, Kai Huang wrote:
> > When relocate_kernel() gets called, %rdi holds 'indirection_page' and
> > %rsi holds 'page_list'.  And %rdi always holds 'indirection_page' when
> > swap_pages() is called.
> > 
> > Therefore the comment of the first line code of swap_pages()
> > 
> > 	movq    %rdi, %rcx      /* Put the page_list in %rcx */
> > 
> > .. isn't correct because it actually moves the 'indirection_page' to
> > the %rcx.  Fix it.
> > 
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> 
> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> 
> Looks like it got broken by 4bfaaef01a1b ("[PATCH] Avoid overwriting the
> current pgd (V4, x86_64)")
> 

Thanks for finding this.  I spent some time yesterday trying to do so but it
wasn't obvious to me.  :-)

Yes that line was firstly introduced by commit 5234f5eb04abb ("[PATCH] kexec:
x86_64 kexec implementation") but by that time it was correct: %rdi indeed
holds 'page_list'.  The commit you mentioned above adds a new (first) argument
to relocate_kernel() and %rdi was changed to hold 'indirection_page', but the
comment was leftover.

But the two commits were introduced at 2004 and 2006, so I don't think it
worth any Fixes tag?

Re: [PATCH 1/2] x86/kexec: Fix a comment of swap_pages() assembly

[kirill.shutemov]

On Tue, Aug 20, 2024 at 10:32:39AM +0000, Huang, Kai wrote:
> But the two commits were introduced at 2004 and 2006, so I don't think it
> worth any Fixes tag?

Nah. It is a comment after all.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

[PATCH 2/2] x86/kexec: Add comments around swap_pages() assembly to improve readability

[Kai Huang]

The current assembly around swap_pages() in the relocate_kernel() takes
some time to follow because the use of registers can be easily lost when
the line of assembly goes long.  Add a couple of comments to clarify the
code around swap_pages() to improve readability.

Signed-off-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/kernel/relocate_kernel_64.S | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
index f7a3ca3dee53..e9e88c342f75 100644
--- a/arch/x86/kernel/relocate_kernel_64.S
+++ b/arch/x86/kernel/relocate_kernel_64.S
@@ -170,6 +170,7 @@ SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
 	wbinvd
 .Lsme_off:
 
+	/* Save the preserve_context to %r11 as swap_pages clobbers %rcx. */
 	movq	%rcx, %r11
 	call	swap_pages
 
@@ -289,18 +290,21 @@ SYM_CODE_START_LOCAL_NOALIGN(swap_pages)
 	movq	%rcx,   %rsi  /* For ever source page do a copy */
 	andq	$0xfffffffffffff000, %rsi
 
-	movq	%rdi, %rdx
-	movq	%rsi, %rax
+	movq	%rdi, %rdx    /* Save destination page to %rdx */
+	movq	%rsi, %rax    /* Save source page to %rax */
 
+	/* copy source page to swap page */
 	movq	%r10, %rdi
 	movl	$512, %ecx
 	rep ; movsq
 
+	/* copy destination page to source page */
 	movq	%rax, %rdi
 	movq	%rdx, %rsi
 	movl	$512, %ecx
 	rep ; movsq
 
+	/* copy swap page to destination page */
 	movq	%rdx, %rdi
 	movq	%r10, %rsi
 	movl	$512, %ecx
-- 
2.45.2

Re: [PATCH 2/2] x86/kexec: Add comments around swap_pages() assembly to improve readability

[Kirill A. Shutemov]

On Tue, Aug 20, 2024 at 12:21:12AM +1200, Kai Huang wrote:
> The current assembly around swap_pages() in the relocate_kernel() takes
> some time to follow because the use of registers can be easily lost when
> the line of assembly goes long.  Add a couple of comments to clarify the
> code around swap_pages() to improve readability.
> 
> Signed-off-by: Kai Huang <kai.huang@intel.com>

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
  Kiryl Shutsemau / Kirill A. Shutemov