RE: [PATCH v6 2/5] x86/kexec: do unconditional WBINVD for bare-metal in relocate_kernel()

[Kaplan, David]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Kai Huang <kai.huang@xxxxxxxxx>
> Sent: Monday, September 9, 2024 3:06 AM
> To: dave.hansen@xxxxxxxxx; bp@xxxxxxxxx; tglx@xxxxxxxxxxxxx;
> peterz@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; hpa@xxxxxxxxx;
> kirill.shutemov@xxxxxxxxxxxxxxx
> Cc: x86@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; pbonzini@xxxxxxxxxx;
> seanjc@xxxxxxxxxx; dan.j.williams@xxxxxxxxx; Lendacky, Thomas
> <Thomas.Lendacky@xxxxxxx>; rick.p.edgecombe@xxxxxxxxx;
> isaku.yamahata@xxxxxxxxx; Kalra, Ashish <Ashish.Kalra@xxxxxxx>;
> bhe@xxxxxxxxxx; nik.borisov@xxxxxxxx; sagis@xxxxxxxxxx; Dave Young
> <dyoung@xxxxxxxxxx>; Kaplan, David <David.Kaplan@xxxxxxx>
> Subject: [PATCH v6 2/5] x86/kexec: do unconditional WBINVD for bare-metal
> in relocate_kernel()
>
>
> Note commit 93c1800b3799 ("x86/kexec: Fix bug with call depth tracking")
> moved calling 'cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)' as an
> argument of relocate_kernel() to an earlier place before load_segments() by
> adding a variable 'host_mem_enc_active'.  The reason was the call to
> cc_platform_has() after load_segments() caused a fault and system crash
> when call depth tracking is active because load_segments() resets GS to
> 0 but call depth tracking uses per-CPU variable to operate.
>

>  #define ARCH_HAS_KIMAGE_ARCH
> diff --git a/arch/x86/kernel/machine_kexec_64.c
> b/arch/x86/kernel/machine_kexec_64.c
> index 9c9ac606893e..225242840467 100644
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -322,16 +322,9 @@ void machine_kexec_cleanup(struct kimage *image)
> void machine_kexec(struct kimage *image)  {
>         unsigned long page_list[PAGES_NR];
> -       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);
> -

Functionally the patch looks fine.  I would suggest keeping some form of this comment though, because the limitation about not being able to make function calls after load_segments() is arguably non-obvious and this comment served as a warning for future modifications in this area.

Thanks
--David Kaplan