Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs

[Prarit Bhargava]

On 10/04/2016 06:58 AM, Thomas Gleixner wrote:
> On Mon, 3 Oct 2016, Prarit Bhargava wrote:
>> BUG: unable to handle kernel paging request at 0000000000841f1f
>> IP: [<ffffffff81014ec4>] uncore_change_context+0xd4/0x180
> ...
>>  [<ffffffff81015a60>] ? uncore_cpu_starting+0x130/0x130
>>  [<ffffffff81015acc>] uncore_event_cpu_online+0x6c/0x80
>>  [<ffffffff8108e819>] cpuhp_invoke_callback+0x49/0x100
>>  [<ffffffff8108ead1>] cpuhp_thread_fun+0x41/0x100
>>  [<ffffffff810b054f>] smpboot_thread_fn+0x10f/0x160
>>  [<ffffffff810b0440>] ? sort_range+0x30/0x30
>>  [<ffffffff810accd8>] kthread+0xd8/0xf0
>>  [<ffffffff816ff4bf>] ret_from_fork+0x1f/0x40
>>  [<ffffffff810acc00>] ? kthread_park+0x60/0x60
> 
>> arch/x86/events/intel/uncore.c:
>> 1137 static void uncore_change_type_ctx(struct intel_uncore_type *type, int old_     cpu,
>> 1138                                    int new_cpu)
>> 1139 {
>> 1140         struct intel_uncore_pmu *pmu = type->pmus;
>> 1141         struct intel_uncore_box *box;
>> 1142         int i, pkg;
>> 1143
>> 1144         pkg = topology_logical_package_id(old_cpu < 0 ? new_cpu : old_cpu);
>> 1145         for (i = 0; i < type->num_boxes; i++, pmu++) {
>> 1146                 box = pmu->boxes[pkg];
>>
>> pmu->boxes[pkg] is garbage because pkg was returned as 0xffff.
> 
> And that's what needs to be fixed in the first place.
> 
>> This patch adds the missing generic_processor_info() to
>> prefill_possible_map() to ensure the initialization of the boot cpu is
>> correct. 
> 
>> This results in smp_init_package_map() having correct data and
>> properly setting the package map for the hotplugged boot cpu, which in
>> turn resolves the kdump kernel panic on physically hotplugged cpus.
> 
> While it is the right thing to initialize the package map in that case, it
> still papers over a robustness issue in the uncore code, which needs to be
> fixed first.

I will include a separate patch with an error check for pkg == 0xffff in the
uncore code.

> 
>> [2] prefill_possible_map() is called before smp_store_boot_cpu_info().
>> The comment beside the call to smp_store_boot_cpu_info() states that the
>> completed call results in "Final full version of the data".
> 
> I'm not sure what that [2] here means and I cannot figure out the meaning
> of this sentence either.

My understanding is that after the call to smp_store_boot_cpu_info(), that for
the rest of the initial bringup the cpu_data structs, etc., are complete.

> 
> This changelog is incomprehensible in general and more a "oh look how I
> decoded this problem" report than something which clearly describes the
> problem at hand, the root cause and the fix. 

That wasn't my intention.

I had to figure it out and it took quite a while to get through it.  I had
doubts that all of the others on the cc list would understand this and I
expected questions of "Can you prove that is the case?".

My changelog addresses those expected questions and I have no problem with a
shortened changelog.

The latter wants a
> understandable explanation why prefill_possible_map() is the right place to
> do this.

> 
>> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
>> index 4296beb8fdd3..d1272febc13b 100644
>> --- a/arch/x86/kernel/smpboot.c
>> +++ b/arch/x86/kernel/smpboot.c
>> @@ -1406,9 +1406,18 @@ __init void prefill_possible_map(void)
>>  {
>>  	int i, possible;
>>  
>> -	/* no processor from mptable or madt */
>> -	if (!num_processors)
>> -		num_processors = 1;
>> +	/* No boot processor was found in mptable or ACPI MADT */
>> +	if (!num_processors) {
>> +		/* Make sure boot cpu is enumerated */
>> +		if (apic->cpu_present_to_apicid(0) == BAD_APICID &&
>> +		    apic->apic_id_valid(boot_cpu_physical_apicid))
>> +			generic_processor_info(boot_cpu_physical_apicid,
>> +					apic_version[boot_cpu_physical_apicid]);
>> +		if (!num_processors) {
>> +			pr_warn("CPU 0 not enumerated in mptable or ACPI MADT\n");
>> +			num_processors = 1;
> 
> And in this case we end up with the same problem, right?

It occurs to me that I over thought this: I was thinking that there might exist
a pre-ACPI (or at least a system without an MADT) x86 system that wold boot such
that num_processors = 0.  But in that case, the cpu should be listed in the
mptables so the above should not happen.  I'll change that to a BUG().

P.

> 
> Thanks,
> 
> 	tglx
>