From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail.skyhub.de ([2a01:4f8:120:8448::d00d]) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1a1Vsg-0002WV-0L for kexec@lists.infradead.org; Wed, 25 Nov 2015 08:56:51 +0000 Date: Wed, 25 Nov 2015 09:56:21 +0100 From: Borislav Petkov Subject: Re: [V5 PATCH 2/4] panic/x86: Allow cpus to save registers even if they are looping in NMI context Message-ID: <20151125085620.GA29499@pd.tnic> References: <20151120093641.4285.97253.stgit@softrs> <20151120093646.4285.62259.stgit@softrs> <20151124104853.GC3785@pd.tnic> <04EAB7311EE43145B2D3536183D1A84454A2A425@GSjpTKYDCembx31.service.hitachi.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <04EAB7311EE43145B2D3536183D1A84454A2A425@GSjpTKYDCembx31.service.hitachi.net> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Sender: "kexec" Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org To: =?utf-8?B?5rKz5ZCI6Iux5a6PIC8gS0FXQUnvvIxISURFSElSTw==?= Cc: "x86@kernel.org" , Baoquan He , Jonathan Corbet , Peter Zijlstra , "linux-doc@vger.kernel.org" , "kexec@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Michal Hocko , Ingo Molnar , Thomas Gleixner , "Eric W. Biederman" , "H. Peter Anvin" , =?utf-8?B?5bmz5p2+6ZuF5bezIC8gSElSQU1BVFXvvIxNQVNBTUk=?= , Andrew Morton , Ingo Molnar , Vivek Goyal T24gV2VkLCBOb3YgMjUsIDIwMTUgYXQgMDU6NTE6NTlBTSArMDAwMCwg5rKz5ZCI6Iux5a6PIC8g S0FXQUnvvIxISURFSElSTyB3cm90ZToKPiA+ID4gYEluZmluaXRlIGxvb3AgaW4gTk1JIGNvbnRl eHQnIGNhbiBoYXBwZW46Cj4gPiA+Cj4gPiA+ICAgYS4gd2hlbiBhIGNwdSBwYW5pY3Mgb24gTk1J IHdoaWxlIGFub3RoZXIgY3B1IGlzIHByb2Nlc3NpbmcgcGFuaWMKPiA+ID4gICBiLiB3aGVuIGEg Y3B1IHJlY2VpdmVkIGFuIGV4dGVybmFsIG9yIHVua25vd24gTk1JIHdoaWxlIGFub3RoZXIKPiA+ ID4gICAgICBjcHUgaXMgcHJvY2Vzc2luZyBwYW5pYyBvbiBOTUkKPiA+ID4KPiA+ID4gSW4gdGhl IGNhc2Ugb2YgYSwgaXQgbG9vcHMgaW4gcGFuaWNfc21wX3NlbGZfc3RvcCgpLiAgSW4gdGhlIGNh c2UKPiA+ID4gb2YgYiwgaXQgbG9vcHMgaW4gcmF3X3NwaW5fbG9jaygpIG9mIG5taV9yZWFzb25f bG9jay4KPiA+IAo+ID4gUGxlYXNlIGRlc2NyaWJlIHRob3NlIHR3byBjYXNlcyBtb3JlIHZlcmJv c2VseSAtIGl0IHRha2VzIHNsb3cgcGVvcGxlCj4gPiBsaWtlIG1lIGEgd2hpbGUgdG8gZmlndXJl IG91dCB3aGF0IGV4YWN0bHkgY2FuIGhhcHBlbi4KPiAKPiAgIGEuIHdoZW4gYSBjcHUgcGFuaWNz IG9uIE5NSSB3aGlsZSBhbm90aGVyIGNwdSBpcyBwcm9jZXNzaW5nIHBhbmljCj4gICAgICBFeC4K PiAgICAgIENQVSAwICAgICAgICAgICAgICAgICAgICAgQ1BVIDEKPiAgICAgID09PT09PT09PT09 PT09PT09ICAgICAgICAgPT09PT09PT09PT09PT09PT0KPiAgICAgIHBhbmljKCkKPiAgICAgICAg cGFuaWNfY3B1IDwtLSAwCj4gICAgICAgIGNoZWNrIHBhbmljX2NwdQo+ICAgICAgICBjcmFzaF9r ZXhlYygpCj4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHJlY2VpdmUgYW4gdW5rbm93 biBOTUkKPiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgdW5rbm93bl9ubWlfZXJyb3Io KQo+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIG5taV9wYW5pYygpCj4gICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICBwYW5pYygpCj4gICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgIGNoZWNrIHBhbmljX2NwdQo+ICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICBwYW5pY19zbXBfc2VsZl9zdG9wKCkKPiAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICBpbmZpbml0ZSBsb29wIGluIE5NSSBjb250ZXh0Cj4gCj4g ICBiLiB3aGVuIGEgY3B1IHJlY2VpdmVkIGFuIGV4dGVybmFsIG9yIHVua25vd24gTk1JIHdoaWxl IGFub3RoZXIKPiAgICAgIGNwdSBpcyBwcm9jZXNzaW5nIHBhbmljIG9uIE5NSQo+ICAgICAgRXgu Cj4gICAgICBDUFUgMCAgICAgICAgICAgICAgICAgICAgIENQVSAxCj4gICAgICA9PT09PT09PT09 PT09PT09PT09PT09ICAgID09PT09PT09PT09PT09PT09PQo+ICAgICAgcmVjZWl2ZSBhbiB1bmtu b3duIE5NSQo+ICAgICAgdW5rbm93bl9ubWlfZXJyb3IoKQo+ICAgICAgICBubWlfcGFuaWMoKSAg ICAgICAgICAgICByZWNlaXZlIGFuIHVua25vd24gTk1JCj4gICAgICAgICAgcGFuaWNfY3B1IDwt LSAwICAgICAgIHVua25vd25fbm1pX2Vycm9yKCkKPiAgICAgICAgICBwYW5pYygpICAgICAgICAg ICAgICAgICBubWlfcGFuaWMoKQo+ICAgICAgICAgICAgY2hlY2sgcGFuaWNfY3B1ICAgICAgICAg cGFuaWMKPiAgICAgICAgICAgIGNyYXNoX2tleGVjKCkgICAgICAgICAgICAgY2hlY2sgcGFuaWNf Y3B1Cj4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHBhbmljX3NtcF9zZWxm X3N0b3AoKQo+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGluZmluaXRl IGxvb3AgaW4gTk1JIGNvbnRleHQKCk9rLCB0aGF0J3Mgd2hhdCBJIHNhdyB0b28sIHRoYW5rcyBm b3IgY29uZmlybWluZy4KCkJ1dCBwbGVhc2Ugd3JpdGUgdGhvc2UgZXhhbXBsZXMgd2l0aCB0aGUg Km9sZCogY29kZSBpbiB0aGUgY29tbWl0Cm1lc3NhZ2UsIGkuZS4gd2l0aG91dCBwYW5pY19jcHUg YW5kIG5taV9wYW5pYygpIHdoaWNoIHlvdSdyZSBhZGRpbmcuCgpCYXNpY2FsbHksIHlvdSB3YW50 IHRvIHN0cnVjdHVyZSB5b3VyIGNvbW1pdCBtZXNzYWdlIHRoaXMgd2F5OgoKVGhpcyBpcyB0aGUg cHJvYmxlbSB0aGUgY3VycmVudCBjb2RlIGhhczogLi4uCgpCdXQgd2UgbmVlZCB0byBkbyB0aGlz OiAuLi4KCldlIGZpeCBpdCBieSBkb2luZyB0aGF0OiAuLi4KClRoaXMgd2lsbCBiZSBvZiBncmVh dCBoZWxwIG5vdyB3aGVuIHJlYWRpbmcgdGhlIGNvbW1pdCBtZXNzYWdlIGFuZCBvZgppbnZhbHVh YmxlIGhlbHAgbGF0ZXIsIHdoZW4gd2UgYWxsIGhhdmUgZm9yZ290dGVuIGFib3V0IHRoZSBpc3N1 ZSBhbmQKYXJlIHNjcmF0Y2hpbmcgaGVhZHMgb3ZlciB3aHkgc3R1ZmYgd2FzIGFkZGVkLgoKPiA+ ID4gVG8gc2F2ZSByZWdpc3RlcnMgaW4gdGhlc2UgY2FzZSB0b28sIHRoaXMgcGF0Y2ggZG9lcyBm b2xsb3dpbmcgdGhpbmdzOgo+ID4gPgo+ID4gPiAxLiBNb3ZlIHRoZSB0aW1pbmcgb2YgYGluZmlu aXRlIGxvb3AgaW4gTk1JIGNvbnRleHQnIChhY3R1YWxseQo+ID4gPiAgICBkb25lIGJ5IHBhbmlj X3NtcF9zZWxmX3N0b3AoKSkgb3V0c2lkZSBvZiBwYW5pYygpIHRvIGVuYWJsZSB1cyB0bwo+ID4g PiAgICByZWZlciBwdF9yZWdzCj4gPiAKPiA+IEkgY2FuJ3QgcGFyc2UgdGhhdCBzZW50ZW5jZS4g QW5kIEkgcmVhbGx5IHRyaWVkIDotXAo+ID4gcGFuaWNfc21wX3NlbGZfc3RvcCgpIGlzIHN0aWxs IGluIHBhbmljKCkuCj4gCj4gcGFuaWNfc21wX3NlbGZfc3RvcCgpIGlzIHN0aWxsIHVzZWQgd2hl biBhIENQVSBpbiBub3JtYWwgY29udGV4dAo+IHNob3VsZCBnbyBpbnRvIGluZmluaXRlIGxvb3Au ICBPbmx5IHdoZW4gYSBDUFUgaXMgaW4gTk1JIGNvbnRleHQsCj4gbm1pX3BhbmljX3NlbGZfc3Rv cCgpIGlzIGNhbGxlZCBhbmQgdGhlIENQVSBsb29wcyBpbmZpbml0ZWx5Cj4gd2l0aG91dCBlbnRl cmluZyBwYW5pYygpLgo+IAo+IEknbGwgdHJ5IHRvIHJldmlzZSB0aGlzIHNlbnRlbnNlLgoKRldJ VywgaXQgc291bmRzIGJldHRlciBhbHJlYWR5ISA6KQoKPiA+ID4gKwkgKi8KPiA+ID4gKwl3aGls ZSAoIXJhd19zcGluX3RyeWxvY2soJm5taV9yZWFzb25fbG9jaykpCj4gPiA+ICsJCXBvbGxfY3Jh c2hfaXBpX2FuZF9jYWxsYmFjayhyZWdzKTsKPiA+IAo+ID4gV2FhYWl0IGEgbWludXRlOiBzbyBp ZiB3ZSdyZSBnZXR0aW5nIE5NSXMgYnJvYWRjYXN0ZWQgb24gZXZlcnkgY29yZSBidXQKPiA+IHdl J3JlICpub3QqIGNyYXNoIGR1bXBpbmcsIHdlIHdpbGwgcnVuIGludG8gaGVyZSB0b28uIFRoaXMg Y2FuJ3QgYmUKPiA+IHJpZ2h0LiA6LVwKPiAKPiBBcyBTdGV2ZW4gY29tbWVudGVkLCBwb2xsX2Ny YXNoX2lwaV9hbmRfY2FsbGJhY2soKSBkb2VzIG5vdGhpbmcKPiBpZiB3ZSdyZSBub3QgY3Jhc2gg ZHVtcGluZy4gIEJ1dCB5ZXMsIHRoaXMgaXMgY29uZnVzaW5nLiAgSSdsbCBhZGQKPiBtb3JlIGRl dGFpbGVkIGNvbW1lbnQsIG9yIGNoYW5nZSB0aGUgbG9naWMgYSBiaXQgaWYgSSBjb21lIHVwIHdp dGgKPiBiZXR0ZXIgb25lLgoKVGhhbmtzLCBtdWNoIGFwcHJlY2lhdGVkIQoKPiA+ID4gKy8qCj4g PiA+ICsgKiBXYWl0IGZvciB0aGUgdGltaW5nIG9mIElQSSBmb3IgY3Jhc2ggZHVtcGluZywgYW5k IHRoZW4gY2FsbCBpdHMgY2FsbGJhY2sKPiA+IAo+ID4gIldhaXQgZm9yIHRoZSBjcmFzaCBkdW1w aW5nIElQSSB0byBjb21wbGV0ZS4uLiAiCj4gCj4gU28sIEkgdGhpbmsgIldhaXQgZm9yIHRoZSBj cmFzaCBkdW1waW5nIElQSSB0byBiZSBpc3N1ZWQuLi4iIGlzIGJldHRlci4KPiAiY29tcGxldGUi IHdvdWxkIGJlIGFtYmlndW91cyBpbiB0aGlzIGNvbnRleHQuCgpPay4KCj4gCj4gPiA+ICsgKiBk aXJlY3RseS4gIFRoaXMgZnVuY3Rpb24gaXMgdXNlZCB3aGVuIHdlIGhhdmUgYWxyZWFkeSBiZWVu IGluIE5NSSBoYW5kbGVyLgo+ID4gPiArICovCj4gPiA+ICt2b2lkIHBvbGxfY3Jhc2hfaXBpX2Fu ZF9jYWxsYmFjayhzdHJ1Y3QgcHRfcmVncyAqcmVncykKPiA+IAo+ID4gV2h5ICJwb2xsIj8gV2Ug d29uJ3QgcmV0dXJuIGZyb20gY3Jhc2hfbm1pX2NhbGxiYWNrKCkgaWYgd2UncmUgbm90IHRoZQo+ ID4gY3Jhc2hpbmcgQ1BVLgo+IAo+IFRoaXMgZnVuY3Rpb24gcG9sbHMgdGhhdCBjcmFzaCBJUEkg aGFzIGJlZW4gaXNzdWVkIGJ5IGNoZWNraW5nCj4gY3Jhc2hfaXBpX2RvbmUsIHRoZW4gaW52b2tl cyB0aGUgY2FsbGJhY2suICBUaGlzIGlzIGRpZmZlcmVudAo+IGZyb20gc28tY2FsbGVkICJwb2xs IiBmdW5jdGlvbnMuICBEbyB5b3UgaGF2ZSBzb21lIGdvb2QgbmFtZT8KCk1heWJlIHNvbWV0aGlu ZyBhcyBzaW1wbGUgYXMgInJ1bl9jcmFzaF9jYWxsYmFjayI/CgpPciBzaW5jZSB3ZSdyZSBjYWxs aW5nIGl0IGZyb20gb3RoZXIgcGxhY2VzLCBtYXliZSBhZGQgdGhlICJjcmFzaCIKcHJlZml4OgoK CXdoaWxlICghcmF3X3NwaW5fdHJ5bG9jaygmbm1pX3JlYXNvbl9sb2NrKSkKCQljcmFzaF9ydW5f Y2FsbGJhY2socmVncyk7CgpMb29rcyBldmVuIGJldHRlciB0byBtZSBpbiBjb2RlIGNvbnRleHQu IDopCgpUaGFua3MhCgotLSAKUmVnYXJkcy9HcnVzcywKICAgIEJvcmlzLgoKRUNPIHRpcCAjMTAx OiBUcmltIHlvdXIgbWFpbHMgd2hlbiB5b3UgcmVwbHkuCgpfX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fXwprZXhlYyBtYWlsaW5nIGxpc3QKa2V4ZWNAbGlzdHMu aW5mcmFkZWFkLm9yZwpodHRwOi8vbGlzdHMuaW5mcmFkZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZv L2tleGVjCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755064AbbKYI43 (ORCPT ); Wed, 25 Nov 2015 03:56:29 -0500 Received: from mail.skyhub.de ([78.46.96.112]:51485 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753676AbbKYI40 (ORCPT ); Wed, 25 Nov 2015 03:56:26 -0500 Date: Wed, 25 Nov 2015 09:56:21 +0100 From: Borislav Petkov To: =?utf-8?B?5rKz5ZCI6Iux5a6PIC8gS0FXQUnvvIxISURFSElSTw==?= Cc: Jonathan Corbet , Peter Zijlstra , Ingo Molnar , "Eric W. Biederman" , "H. Peter Anvin" , Andrew Morton , Thomas Gleixner , Vivek Goyal , Baoquan He , "linux-doc@vger.kernel.org" , "x86@kernel.org" , "kexec@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Michal Hocko , Ingo Molnar , =?utf-8?B?5bmz5p2+6ZuF5bezIC8gSElSQU1BVFXvvIxNQVNBTUk=?= Subject: Re: [V5 PATCH 2/4] panic/x86: Allow cpus to save registers even if they are looping in NMI context Message-ID: <20151125085620.GA29499@pd.tnic> References: <20151120093641.4285.97253.stgit@softrs> <20151120093646.4285.62259.stgit@softrs> <20151124104853.GC3785@pd.tnic> <04EAB7311EE43145B2D3536183D1A84454A2A425@GSjpTKYDCembx31.service.hitachi.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <04EAB7311EE43145B2D3536183D1A84454A2A425@GSjpTKYDCembx31.service.hitachi.net> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 25, 2015 at 05:51:59AM +0000, 河合英宏 / KAWAI，HIDEHIRO wrote: > > > `Infinite loop in NMI context' can happen: > > > > > > a. when a cpu panics on NMI while another cpu is processing panic > > > b. when a cpu received an external or unknown NMI while another > > > cpu is processing panic on NMI > > > > > > In the case of a, it loops in panic_smp_self_stop(). In the case > > > of b, it loops in raw_spin_lock() of nmi_reason_lock. > > > > Please describe those two cases more verbosely - it takes slow people > > like me a while to figure out what exactly can happen. > > a. when a cpu panics on NMI while another cpu is processing panic > Ex. > CPU 0 CPU 1 > ================= ================= > panic() > panic_cpu <-- 0 > check panic_cpu > crash_kexec() > receive an unknown NMI > unknown_nmi_error() > nmi_panic() > panic() > check panic_cpu > panic_smp_self_stop() > infinite loop in NMI context > > b. when a cpu received an external or unknown NMI while another > cpu is processing panic on NMI > Ex. > CPU 0 CPU 1 > ====================== ================== > receive an unknown NMI > unknown_nmi_error() > nmi_panic() receive an unknown NMI > panic_cpu <-- 0 unknown_nmi_error() > panic() nmi_panic() > check panic_cpu panic > crash_kexec() check panic_cpu > panic_smp_self_stop() > infinite loop in NMI context Ok, that's what I saw too, thanks for confirming. But please write those examples with the *old* code in the commit message, i.e. without panic_cpu and nmi_panic() which you're adding. Basically, you want to structure your commit message this way: This is the problem the current code has: ... But we need to do this: ... We fix it by doing that: ... This will be of great help now when reading the commit message and of invaluable help later, when we all have forgotten about the issue and are scratching heads over why stuff was added. > > > To save registers in these case too, this patch does following things: > > > > > > 1. Move the timing of `infinite loop in NMI context' (actually > > > done by panic_smp_self_stop()) outside of panic() to enable us to > > > refer pt_regs > > > > I can't parse that sentence. And I really tried :-\ > > panic_smp_self_stop() is still in panic(). > > panic_smp_self_stop() is still used when a CPU in normal context > should go into infinite loop. Only when a CPU is in NMI context, > nmi_panic_self_stop() is called and the CPU loops infinitely > without entering panic(). > > I'll try to revise this sentense. FWIW, it sounds better already! :) > > > + */ > > > + while (!raw_spin_trylock(&nmi_reason_lock)) > > > + poll_crash_ipi_and_callback(regs); > > > > Waaait a minute: so if we're getting NMIs broadcasted on every core but > > we're *not* crash dumping, we will run into here too. This can't be > > right. :-\ > > As Steven commented, poll_crash_ipi_and_callback() does nothing > if we're not crash dumping. But yes, this is confusing. I'll add > more detailed comment, or change the logic a bit if I come up with > better one. Thanks, much appreciated! > > > +/* > > > + * Wait for the timing of IPI for crash dumping, and then call its callback > > > > "Wait for the crash dumping IPI to complete... " > > So, I think "Wait for the crash dumping IPI to be issued..." is better. > "complete" would be ambiguous in this context. Ok. > > > > + * directly. This function is used when we have already been in NMI handler. > > > + */ > > > +void poll_crash_ipi_and_callback(struct pt_regs *regs) > > > > Why "poll"? We won't return from crash_nmi_callback() if we're not the > > crashing CPU. > > This function polls that crash IPI has been issued by checking > crash_ipi_done, then invokes the callback. This is different > from so-called "poll" functions. Do you have some good name? Maybe something as simple as "run_crash_callback"? Or since we're calling it from other places, maybe add the "crash" prefix: while (!raw_spin_trylock(&nmi_reason_lock)) crash_run_callback(regs); Looks even better to me in code context. :) Thanks! -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply.