Re: [RFC PATCH 1/7] x86/kexec: Clean up and document register use in relocate_kernel_64.S

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 2466 bytes --]

On Tue, 2024-11-05 at 10:00 +0000, Huang, Kai wrote:
> On Sun, 2024-11-03 at 05:35 +0000, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > Add more comments explaining what each register contains, and save the
> > preserve_context flag to a non-clobbered register sooner, to keep things
> > simpler.
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> >  arch/x86/kernel/relocate_kernel_64.S | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
> > index e9e88c342f75..c065806884f8 100644
> > --- a/arch/x86/kernel/relocate_kernel_64.S
> > +++ b/arch/x86/kernel/relocate_kernel_64.S
> > @@ -100,6 +100,9 @@ SYM_CODE_START_NOALIGN(relocate_kernel)
> >         movq    %r10, CP_PA_SWAP_PAGE(%r11)
> >         movq    %rdi, CP_PA_BACKUP_PAGES_MAP(%r11)
> >  
> > +       /* Save the preserve_context to %r11 as swap_pages clobbers %rcx. */
> > +       movq    %rcx, %r11
> > +
> 
> Seems moving this here won't break anything.  From functionality's perspective
> there's no need move this here, but fine to me since the move is needed for the
> sake of adding the comment (below) to identity_mapped.

I believe I did actually use %rcx in the debug code I added later,
didn't I?

> >         /* Switch to the identity mapped page tables */
> >         movq    %r9, %cr3
> >  
> > @@ -116,6 +119,13 @@ SYM_CODE_END(relocate_kernel)
> >  
> >  SYM_CODE_START_LOCAL_NOALIGN(identity_mapped)
> >         UNWIND_HINT_END_OF_STACK
> > +       /*
> > +        * %rdi indirection page
> > +        * %rdx start address
> > +        * %r11 preserve_context
> > +        * %r12 host_mem_enc_active
> > +        */
> > +
> 
> I think adding this comment is the main purpose of this patch.  Since we have
> listed 4 regs in the comment, how about also add an entry for %r13?
> 
> Something like:
> 
>         %r13 original CR4 when relocate_kernel() is invoked
> 
> Yeah this is kinda mentioned in later code but it seems it's more complete if we
> also mention %r13 here.

Ack, I'll add that too. Thanks.

> Anyway, I suppose adding this comment is kinda helpful, so:
> 
> Acked-by: Kai Huang <kai.huang@intel.com>
> 


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]