From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753614AbcJDMJt (ORCPT ); Tue, 4 Oct 2016 08:09:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50314 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751636AbcJDMJr (ORCPT ); Tue, 4 Oct 2016 08:09:47 -0400 Message-ID: <57F39C09.10001@redhat.com> Date: Tue, 04 Oct 2016 08:09:45 -0400 From: Prarit Bhargava User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Thomas Gleixner CC: linux-kernel@vger.kernel.org, Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Peter Zijlstra , Len Brown , Borislav Petkov , Andi Kleen , Jiri Olsa , Juergen Gross , dyoung@redhat.com, Eric Biederman , kexec@lists.infradead.org Subject: Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs References: <1475514432-27682-1-git-send-email-prarit@redhat.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Tue, 04 Oct 2016 12:09:47 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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: [] uncore_change_context+0xd4/0x180 > ... >> [] ? uncore_cpu_starting+0x130/0x130 >> [] uncore_event_cpu_online+0x6c/0x80 >> [] cpuhp_invoke_callback+0x49/0x100 >> [] cpuhp_thread_fun+0x41/0x100 >> [] smpboot_thread_fn+0x10f/0x160 >> [] ? sort_range+0x30/0x30 >> [] kthread+0xd8/0xf0 >> [] ret_from_fork+0x1f/0x40 >> [] ? 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 > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx1.redhat.com ([209.132.183.28]) by bombadil.infradead.org with esmtps (Exim 4.85_2 #1 (Red Hat Linux)) id 1brOXw-0002d2-BX for kexec@lists.infradead.org; Tue, 04 Oct 2016 12:10:09 +0000 Message-ID: <57F39C09.10001@redhat.com> Date: Tue, 04 Oct 2016 08:09:45 -0400 From: Prarit Bhargava MIME-Version: 1.0 Subject: Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs References: <1475514432-27682-1-git-send-email-prarit@redhat.com> In-Reply-To: List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "kexec" Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org To: Thomas Gleixner Cc: Juergen Gross , Len Brown , Andi Kleen , Peter Zijlstra , dyoung@redhat.com, x86@kernel.org, kexec@lists.infradead.org, linux-kernel@vger.kernel.org, Ingo Molnar , Eric Biederman , "H. Peter Anvin" , Borislav Petkov , Jiri Olsa 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: [] uncore_change_context+0xd4/0x180 > ... >> [] ? uncore_cpu_starting+0x130/0x130 >> [] uncore_event_cpu_online+0x6c/0x80 >> [] cpuhp_invoke_callback+0x49/0x100 >> [] cpuhp_thread_fun+0x41/0x100 >> [] smpboot_thread_fn+0x10f/0x160 >> [] ? sort_range+0x30/0x30 >> [] kthread+0xd8/0xf0 >> [] ret_from_fork+0x1f/0x40 >> [] ? 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 > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec