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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel