| Message ID | 1575057677-13839-1-git-send-email-igor.druzhinin@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-4.13] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs | expand |
On Fri, Nov 29, 2019 at 08:01:17PM +0000, Igor Druzhinin wrote: > If the feature is not present Xen will try to force X86_BUG_FPU_PTRS > feature at CPU identification time. This is especially noticeable in > PV-shim that usually hotplugs its vCPUs. We either need to restrict this > action for boot CPU only or allow secondary CPUs to modify > forced CPU capabilities at runtime. Choose the latter accounting > for potential microcode asymmetry between the boot and secondary CPUs. > > Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com> LGTM, both setup_{force/clear}_cpu_cap are called from non-init functions, albeit I'm not sure how well Xen and guests will deal with a system that has such asymmetry in CPU features if APs don't have RstrFpErrPtrs and the BSP does. Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Since I assume this fixes a page-fault, could you please paste part of the crash trace to the commit message? Thanks, Roger.
On 29.11.2019 21:01, Igor Druzhinin wrote: > --- a/xen/arch/x86/cpu/common.c > +++ b/xen/arch/x86/cpu/common.c > @@ -54,7 +54,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; > @@ -83,7 +83,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; The two functions are deliberately __init, as any call to them post-init is not going to take system-wide effect. These functions should really be __init_presmp, if we had something like this. No use of them on an AP boot path is going to affect the BSP, and hence will leave the system in an inconsistent state. Jan
On 03/12/2019 10:08, Jan Beulich wrote: > On 29.11.2019 21:01, Igor Druzhinin wrote: >> --- a/xen/arch/x86/cpu/common.c >> +++ b/xen/arch/x86/cpu/common.c >> @@ -54,7 +54,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; >> @@ -83,7 +83,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; > > The two functions are deliberately __init, as any call to them > post-init is not going to take system-wide effect. These functions > should really be __init_presmp, if we had something like this. No > use of them on an AP boot path is going to affect the BSP, and > hence will leave the system in an inconsistent state. > I agree with you and have a version where I just gate the corresponding calls with (c == &boot_cpu_data). Removing __init was the approach suggested by Andrew following the concern of potentially asymmetric microcode in a system which I don't think would work anyway due to the reasons you mentioned. I will send the original approach as v2. Igor
On 03/12/2019 10:08, Jan Beulich wrote: > On 29.11.2019 21:01, Igor Druzhinin wrote: >> --- a/xen/arch/x86/cpu/common.c >> +++ b/xen/arch/x86/cpu/common.c >> @@ -54,7 +54,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; >> @@ -83,7 +83,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; > The two functions are deliberately __init, as any call to them > post-init is not going to take system-wide effect. Current example demonstrates the contrary. Setting X86_BUG_FPU_PTRS at any point through the runtime of Xen will cause the safe action to start happening. Dropping this call on the non-boot CPUs leads to an insecure configuration which we're perfectly capable of working around, and therefore isn't an acceptable solution. ~Andrew
On 03/12/2019 10:08, Jan Beulich wrote: > On 29.11.2019 21:01, Igor Druzhinin wrote: >> --- a/xen/arch/x86/cpu/common.c >> +++ b/xen/arch/x86/cpu/common.c >> @@ -54,7 +54,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; >> @@ -83,7 +83,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; > > The two functions are deliberately __init, as any call to them > post-init is not going to take system-wide effect. These functions > should really be __init_presmp, if we had something like this. No > use of them on an AP boot path is going to affect the BSP, and > hence will leave the system in an inconsistent state. On second thought, looking at how many places actually call setup_{force,clear}_cpu_cap() on AP init path it still makes sense to keep the v1 approach as otherwise we will have to manually workaround every single place where it happens. Igor
On 03.12.2019 15:11, Andrew Cooper wrote: > On 03/12/2019 10:08, Jan Beulich wrote: >> On 29.11.2019 21:01, Igor Druzhinin wrote: >>> --- a/xen/arch/x86/cpu/common.c >>> +++ b/xen/arch/x86/cpu/common.c >>> @@ -54,7 +54,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; >>> @@ -83,7 +83,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; >> The two functions are deliberately __init, as any call to them >> post-init is not going to take system-wide effect. > > Current example demonstrates the contrary. Setting X86_BUG_FPU_PTRS at > any point through the runtime of Xen will cause the safe action to start > happening. This is because of an implementation detail _and_ specific to this one flag. In general what I've said applies; making these functions non-_init will give the false impression that their use it going to have an effect in general. I.e. doing as you suggest would lay the groundwork for future bugs. As an aside, recall my objection to use the x86_capabilities[] machinery for this erratum? You wanting __init dropped here is a result of that (as I would call it) abuse. > Dropping this call on the non-boot CPUs leads to an insecure > configuration which we're perfectly capable of working around, and > therefore isn't an acceptable solution. A prereq to retaining the calls on APs would be to make non-BSP use of the functions generally safe. Otherwise, if you want to support such asymmetric configurations, cpu_bug_fpu_ptrs wants to be switched to a bool variable. Jan
On 03.12.2019 15:21, Igor Druzhinin wrote: > On 03/12/2019 10:08, Jan Beulich wrote: >> On 29.11.2019 21:01, Igor Druzhinin wrote: >>> --- a/xen/arch/x86/cpu/common.c >>> +++ b/xen/arch/x86/cpu/common.c >>> @@ -54,7 +54,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; >>> @@ -83,7 +83,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; >> >> The two functions are deliberately __init, as any call to them >> post-init is not going to take system-wide effect. These functions >> should really be __init_presmp, if we had something like this. No >> use of them on an AP boot path is going to affect the BSP, and >> hence will leave the system in an inconsistent state. > > On second thought, looking at how many places actually call > setup_{force,clear}_cpu_cap() on AP init path it still makes sense > to keep the v1 approach as otherwise we will have to manually workaround > every single place where it happens. While not all of the other uses of the functions happen from __init functions, all of them are unreachable on APs afaict - I've just gone through all instances. Jan
On 03/12/2019 14:28, Jan Beulich wrote: > On 03.12.2019 15:21, Igor Druzhinin wrote: >> On 03/12/2019 10:08, Jan Beulich wrote: >>> On 29.11.2019 21:01, Igor Druzhinin wrote: >>>> --- a/xen/arch/x86/cpu/common.c >>>> +++ b/xen/arch/x86/cpu/common.c >>>> @@ -54,7 +54,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; >>>> @@ -83,7 +83,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; >>> >>> The two functions are deliberately __init, as any call to them >>> post-init is not going to take system-wide effect. These functions >>> should really be __init_presmp, if we had something like this. No >>> use of them on an AP boot path is going to affect the BSP, and >>> hence will leave the system in an inconsistent state. >> >> On second thought, looking at how many places actually call >> setup_{force,clear}_cpu_cap() on AP init path it still makes sense >> to keep the v1 approach as otherwise we will have to manually workaround >> every single place where it happens. > > While not all of the other uses of the functions happen from __init > functions, all of them are unreachable on APs afaict - I've just > gone through all instances. I see 2 places where it looks suspicious: psr_cpu_init(), mwait_idle_cpu_init() Igor
On 03.12.2019 15:41, Igor Druzhinin wrote: > On 03/12/2019 14:28, Jan Beulich wrote: >> On 03.12.2019 15:21, Igor Druzhinin wrote: >>> On 03/12/2019 10:08, Jan Beulich wrote: >>>> On 29.11.2019 21:01, Igor Druzhinin wrote: >>>>> --- a/xen/arch/x86/cpu/common.c >>>>> +++ b/xen/arch/x86/cpu/common.c >>>>> @@ -54,7 +54,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; >>>>> @@ -83,7 +83,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; >>>> >>>> The two functions are deliberately __init, as any call to them >>>> post-init is not going to take system-wide effect. These functions >>>> should really be __init_presmp, if we had something like this. No >>>> use of them on an AP boot path is going to affect the BSP, and >>>> hence will leave the system in an inconsistent state. >>> >>> On second thought, looking at how many places actually call >>> setup_{force,clear}_cpu_cap() on AP init path it still makes sense >>> to keep the v1 approach as otherwise we will have to manually workaround >>> every single place where it happens. >> >> While not all of the other uses of the functions happen from __init >> functions, all of them are unreachable on APs afaict - I've just >> gone through all instances. > > I see 2 places where it looks suspicious: > psr_cpu_init(), mwait_idle_cpu_init() if ( !psr_alloc_feat_enabled() || !boot_cpu_has(X86_FEATURE_PQE) ) goto assoc_init; if ( boot_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT ) { setup_clear_cpu_cap(X86_FEATURE_PQE); goto assoc_init; } The boot_cpu_has(X86_FEATURE_PQE) will prevent the 2nd if() from being reached by an AP, if the BSP force-cleared the feature. if (state > 2 && !boot_cpu_has(X86_FEATURE_NONSTOP_TSC) && !pm_idle_save) setup_clear_cpu_cap(X86_FEATURE_TSC_RELIABLE); The !pm_idle_save check is the guard here. Jan
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c index e5ad17d..4293075 100644 --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -54,7 +54,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; @@ -83,7 +83,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;
If the feature is not present Xen will try to force X86_BUG_FPU_PTRS feature at CPU identification time. This is especially noticeable in PV-shim that usually hotplugs its vCPUs. We either need to restrict this action for boot CPU only or allow secondary CPUs to modify forced CPU capabilities at runtime. Choose the latter accounting for potential microcode asymmetry between the boot and secondary CPUs. Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com> --- Justification for 4.13 - PV-shim is effectively broken on such a system. --- xen/arch/x86/cpu/common.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)