[Xen-devel] [PATCH for-4.13] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

Igor Druzhinin posted 1 patch 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/1575057677-13839-1-git-send-email-igor.druzhinin@citrix.com
xen/arch/x86/cpu/common.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

[Xen-devel] [PATCH for-4.13] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

Posted by Igor Druzhinin 1 week ago
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(-)

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;
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.13] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

Posted by Jan Beulich 5 days ago
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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.13] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

Posted by Igor Druzhinin 5 days ago
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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.13] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

Posted by Jan Beulich 5 days ago
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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.13] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

Posted by Igor Druzhinin 5 days ago
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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.13] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

Posted by Jan Beulich 5 days ago
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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.13] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

Posted by Andrew Cooper 5 days ago
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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.13] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

Posted by Jan Beulich 5 days ago
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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.13] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

Posted by Igor Druzhinin 5 days ago
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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.13] x86/AMD: unbreak CPU hotplug on AMD systems without RstrFpErrPtrs

Posted by Roger Pau Monné 1 week ago
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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel