| Message ID | 20220811101715.3947873-1-ross.lagerwall@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/cpu: Drop _init from *_cpu_cap functions | expand |
On 11/08/2022 11:17, Ross Lagerwall wrote: > These functions may be called by init_amd() after the _init functions > have been purged during CPU hotplug or PV shim boot so drop the _init. > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> Hmm. That's a bug in init_amd() I'd say. These really shouldn't be used after __init. Which path exploded specifically? ~Andrew
> From: Andrew Cooper <Andrew.Cooper3@citrix.com> > Sent: Thursday, August 11, 2022 11:21 AM > To: Ross Lagerwall <ross.lagerwall@citrix.com>; xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org> > Cc: Jan Beulich <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>; Wei Liu <wl@xen.org> > Subject: Re: [PATCH] x86/cpu: Drop _init from *_cpu_cap functions > > On 11/08/2022 11:17, Ross Lagerwall wrote: > > These functions may be called by init_amd() after the _init functions > > have been purged during CPU hotplug or PV shim boot so drop the _init. > > > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> > > Hmm. That's a bug in init_amd() I'd say. These really shouldn't be > used after __init. > > Which path exploded specifically? The stack trace was: setup_force_cpu_cap init_amd identify_cpu start_secondary In setup_force_cpu_cap() here: /* * On pre-CLFLUSHOPT AMD CPUs, CLFLUSH is weakly ordered with * everything, including reads and writes to address, and * LFENCE/SFENCE instructions. */ if (!cpu_has_clflushopt) setup_force_cpu_cap(X86_BUG_CLFLUSH_MFENCE); which was recently introduced by: commit 062868a5a8b428b85db589fa9a6d6e43969ffeb9 Author: Andrew Cooper <andrew.cooper3@citrix.com> Date: Thu Jun 9 14:23:07 2022 +0200 x86/amd: Work around CLFLUSH ordering on older parts Should the fix rather be to guard that call with "if (c == &boot_cpu_data ..." ? Ross
On 11/08/2022 11:30, Ross Lagerwall wrote: >> From: Andrew Cooper <Andrew.Cooper3@citrix.com> >> Sent: Thursday, August 11, 2022 11:21 AM >> To: Ross Lagerwall <ross.lagerwall@citrix.com>; xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org> >> Cc: Jan Beulich <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>; Wei Liu <wl@xen.org> >> Subject: Re: [PATCH] x86/cpu: Drop _init from *_cpu_cap functions >> >> On 11/08/2022 11:17, Ross Lagerwall wrote: >>> These functions may be called by init_amd() after the _init functions >>> have been purged during CPU hotplug or PV shim boot so drop the _init. >>> >>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> >> Hmm. That's a bug in init_amd() I'd say. These really shouldn't be >> used after __init. >> >> Which path exploded specifically? > The stack trace was: > > setup_force_cpu_cap > init_amd > identify_cpu > start_secondary > > In setup_force_cpu_cap() here: > > /* > * On pre-CLFLUSHOPT AMD CPUs, CLFLUSH is weakly ordered with > * everything, including reads and writes to address, and > * LFENCE/SFENCE instructions. > */ > if (!cpu_has_clflushopt) > setup_force_cpu_cap(X86_BUG_CLFLUSH_MFENCE); > > which was recently introduced by: > > commit 062868a5a8b428b85db589fa9a6d6e43969ffeb9 > Author: Andrew Cooper <andrew.cooper3@citrix.com> > Date: Thu Jun 9 14:23:07 2022 +0200 > > x86/amd: Work around CLFLUSH ordering on older parts Bah, and that was also backported in a security fix, to everything back to 4.12 is broken. > Should the fix rather be to guard that call with "if (c == &boot_cpu_data ..." ? Yes please. Sorry. ~Andrew
On 11.08.2022 12:34, Andrew Cooper wrote: > On 11/08/2022 11:30, Ross Lagerwall wrote: >>> From: Andrew Cooper <Andrew.Cooper3@citrix.com> >>> Sent: Thursday, August 11, 2022 11:21 AM >>> To: Ross Lagerwall <ross.lagerwall@citrix.com>; xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org> >>> Cc: Jan Beulich <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>; Wei Liu <wl@xen.org> >>> Subject: Re: [PATCH] x86/cpu: Drop _init from *_cpu_cap functions >>> >>> On 11/08/2022 11:17, Ross Lagerwall wrote: >>>> These functions may be called by init_amd() after the _init functions >>>> have been purged during CPU hotplug or PV shim boot so drop the _init. >>>> >>>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> >>> Hmm. That's a bug in init_amd() I'd say. These really shouldn't be >>> used after __init. >>> >>> Which path exploded specifically? >> The stack trace was: >> >> setup_force_cpu_cap >> init_amd >> identify_cpu >> start_secondary >> >> In setup_force_cpu_cap() here: >> >> /* >> * On pre-CLFLUSHOPT AMD CPUs, CLFLUSH is weakly ordered with >> * everything, including reads and writes to address, and >> * LFENCE/SFENCE instructions. >> */ >> if (!cpu_has_clflushopt) >> setup_force_cpu_cap(X86_BUG_CLFLUSH_MFENCE); >> >> which was recently introduced by: >> >> commit 062868a5a8b428b85db589fa9a6d6e43969ffeb9 >> Author: Andrew Cooper <andrew.cooper3@citrix.com> >> Date: Thu Jun 9 14:23:07 2022 +0200 >> >> x86/amd: Work around CLFLUSH ordering on older parts > > Bah, and that was also backported in a security fix, to everything back > to 4.12 is broken. 4.13, but yes. Oh well. It's actually odd that we use __set_bit() for X86_FEATURE_MFENCE_RDTSC (a few lines up) but the more heavyweight function for X86_BUG_CLFLUSH_MFENCE. Jan
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c index 0412dbc915..20581bf3f8 100644 --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -57,7 +57,7 @@ static unsigned int forced_caps[NCAPINTS]; DEFINE_PER_CPU(bool, full_gdt_loaded); -void __init setup_clear_cpu_cap(unsigned int cap) +void setup_clear_cpu_cap(unsigned int cap) { const uint32_t *dfs; unsigned int i; @@ -86,7 +86,7 @@ void __init setup_clear_cpu_cap(unsigned int cap) } } -void __init setup_force_cpu_cap(unsigned int cap) +void setup_force_cpu_cap(unsigned int cap) { if (__test_and_set_bit(cap, forced_caps)) return; @@ -100,7 +100,7 @@ void __init setup_force_cpu_cap(unsigned int cap) __set_bit(cap, boot_cpu_data.x86_capability); } -bool __init is_forced_cpu_cap(unsigned int cap) +bool is_forced_cpu_cap(unsigned int cap) { return test_bit(cap, forced_caps); }
These functions may be called by init_amd() after the _init functions have been purged during CPU hotplug or PV shim boot so drop the _init. Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> --- xen/arch/x86/cpu/common.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)