Re: [RFC PATCH 2/5] x86/kexec: Do unconditional WBINVD for bare-metal in relocate_kernel()

[Huang, Kai]

> > 
> > > > --- a/arch/x86/kernel/machine_kexec_64.c
> > > > +++ b/arch/x86/kernel/machine_kexec_64.c
> > > > @@ -346,16 +346,9 @@ void __nocfi machine_kexec(struct kimage *image)
> > > >  {
> > > >  	unsigned long reloc_start = (unsigned long)__relocate_kernel_start;
> > > >  	relocate_kernel_fn *relocate_kernel_ptr;
> > > > -	unsigned int host_mem_enc_active;
> > > >  	int save_ftrace_enabled;
> > > >  	void *control_page;
> > > >  
> > > > -	/*
> > > > -	 * This must be done before load_segments() since if call depth tracking
> > > > -	 * is used then GS must be valid to make any function calls.
> > > > -	 */
> > > > -	host_mem_enc_active = cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT);
> > > > -
> > > >  #ifdef CONFIG_KEXEC_JUMP
> > > >  	if (image->preserve_context)
> > > >  		save_processor_state();
> > > > @@ -398,6 +391,11 @@ void __nocfi machine_kexec(struct kimage *image)
> > > >  	 *
> > > >  	 * I take advantage of this here by force loading the
> > > >  	 * segments, before I zap the gdt with an invalid value.
> > > > +	 *
> > > > +	 * load_segments() resets GS to 0.  Don't make any function call
> > > > +	 * after here since call depth tracking uses per-CPU variables to
> > > > +	 * operate (relocate_kernel() is explicitly ignored by call depth
> > > > +	 * tracking).
> > > 
> > > I think I suggested you should call out the opportunistic change here in the
> > > log. Did you disagree?
> > 
> > I replied this was suggested by David Kaplan, but I guess I forgot to reply the
> > "opportunistic" part.
> > 
> > I don't think this is opportunistic change.  It's a valid comment after the 
> > 'host_mem_enc_active' variable and the comment around it were removed.
> 
> It's valid before too. So it's a separate change. 
> 

I tried to understand what you mean here, but I am not sure I am following.  My
thinking:

Before this code change, in the existing code there's a comment right before the
'host_mem_enc_active' variable to explain why this variable is needed (which is
because of depth tracking).

After we remove 'host_mem_enc_active' and the comment before it, there's no
comment to mention anything about depth tracking here.  So comparing to the
existing code, we lost information which is actually helpful.

To still keep the helpful information about the depth tracking, a new comment is
added before load_segments().

Could you explain why this is a separate/extra change?

Nevertheless, are you looking for something like below in the changelog?

  With the 'host_mem_enc_active' and the comment around it removed,
  the information about depth tracking no longer exists.  Expand the 
  comment around load_segments() to mention that due to depth tracking
  no function call can be made after load_segments().