Message-ID:
 <LV3PR12MB92651EFF193770C82CDCD36094642@LV3PR12MB9265.namprd12.prod.outlook.com>
Date: Thu, 12 Sep 2024 02:21:21 +0000
From: "Kaplan, David" <David.Kaplan@....com>
To: "Huang, Kai" <kai.huang@...el.com>, "Hansen, Dave"
	<dave.hansen@...el.com>, "bp@...en8.de" <bp@...en8.de>,
	"peterz@...radead.org" <peterz@...radead.org>, "hpa@...or.com"
	<hpa@...or.com>, "mingo@...hat.com" <mingo@...hat.com>, "tglx@...utronix.de"
	<tglx@...utronix.de>, "kirill.shutemov@...ux.intel.com"
	<kirill.shutemov@...ux.intel.com>
CC: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "x86@...nel.org" <x86@...nel.org>, "dyoung@...hat.com"
	<dyoung@...hat.com>, "sagis@...gle.com" <sagis@...gle.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Williams, Dan
 J" <dan.j.williams@...el.com>, "Lendacky, Thomas" <Thomas.Lendacky@....com>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>, "Kalra, Ashish"
	<Ashish.Kalra@....com>, "Yamahata, Isaku" <isaku.yamahata@...el.com>,
	"bhe@...hat.com" <bhe@...hat.com>, "nik.borisov@...e.com"
	<nik.borisov@...e.com>
Subject: RE: [PATCH v6 2/5] x86/kexec: do unconditional WBINVD for bare-metal
 in relocate_kernel()

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Huang, Kai <kai.huang@...el.com>
> Sent: Tuesday, September 10, 2024 4:53 AM
> To: Hansen, Dave <dave.hansen@...el.com>; bp@...en8.de;
> peterz@...radead.org; hpa@...or.com; mingo@...hat.com;
> tglx@...utronix.de; Kaplan, David <David.Kaplan@....com>;
> kirill.shutemov@...ux.intel.com
> Cc: Edgecombe, Rick P <rick.p.edgecombe@...el.com>; seanjc@...gle.com;
> x86@...nel.org; dyoung@...hat.com; sagis@...gle.com; linux-
> kernel@...r.kernel.org; Williams, Dan J <dan.j.williams@...el.com>;
> Lendacky, Thomas <Thomas.Lendacky@....com>; pbonzini@...hat.com;
> Kalra, Ashish <Ashish.Kalra@....com>; Yamahata, Isaku
> <isaku.yamahata@...el.com>; bhe@...hat.com; nik.borisov@...e.com
> Subject: Re: [PATCH v6 2/5] x86/kexec: do unconditional WBINVD for bare-
> metal in relocate_kernel()
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> On Tue, 2024-09-10 at 02:46 +0000, Kaplan, David wrote:
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > > -----Original Message-----
> > > From: Huang, Kai <kai.huang@...el.com>
> > > Sent: Monday, September 9, 2024 9:42 PM
> > > To: Kaplan, David <David.Kaplan@....com>; Hansen, Dave
> > > <dave.hansen@...el.com>; bp@...en8.de; tglx@...utronix.de;
> > > peterz@...radead.org; mingo@...hat.com; hpa@...or.com;
> > > kirill.shutemov@...ux.intel.com
> > > Cc: x86@...nel.org; linux-kernel@...r.kernel.org;
> > > pbonzini@...hat.com; seanjc@...gle.com; Williams, Dan J
> > > <dan.j.williams@...el.com>; Lendacky, Thomas
> > > <Thomas.Lendacky@....com>; Edgecombe, Rick P
> > > <rick.p.edgecombe@...el.com>; Yamahata, Isaku
> > > <isaku.yamahata@...el.com>; Kalra, Ashish <Ashish.Kalra@....com>;
> > > bhe@...hat.com; nik.borisov@...e.com; sagis@...gle.com; Dave
> Young
> > > <dyoung@...hat.com>
> > > Subject: Re: [PATCH v6 2/5] x86/kexec: do unconditional WBINVD for
> > > bare- metal in relocate_kernel()
> > >
> > > Caution: This message originated from an External Source. Use proper
> > > caution when opening attachments, clicking links, or responding.
> > >
> > >
> > > > > --- 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.
> > >
> > > Yeah this makes sense.  Thanks.
> > >
> > > I think we can add some text to the existing comment of
> > > load_segments() to call out this.  Allow me to dig into more about
> > > call depth tracking to understand it better -- relocate_kernel()
> > > after load_segments() seems to be a real function call and I want to
> > > know how does it interact with call depth tracking.
> >
> > That one is explicitly ignored, see skip_addr() in
> > arch/x86/kernel/callthunks.c
> >
>
> That was I thought too.  Thanks for pointing out.
>
> How about below?
>
> --- a/arch/x86/kernel/machine_kexec_64.c
> +++ b/arch/x86/kernel/machine_kexec_64.c
> @@ -351,6 +351,11 @@ void 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.
> +        *
> +        * Note this 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).
>          */

Looks good, thanks!

>
> Btw, it would be very helpful if you can help to verify this patch doesn't break
> call depth tracking in your environment.  Thanks!

Tested it and it seemed fine (kexec worked and disassembly did not show any calls after GS is cleared, as expected).

--David Kaplan

Powered by blists - more mailing lists