Received: by 10.223.185.116 with SMTP id b49csp466290wrg; Wed, 14 Feb 2018 01:45:36 -0800 (PST) X-Google-Smtp-Source: AH8x224Xh4j90YrqXT4AelluJNKyMFfDSThutGIUih0yO2CoZeVvotr28T3V6UexIJhuQalShYH4 X-Received: by 2002:a17:902:5914:: with SMTP id o20-v6mr3897434pli.60.1518601536604; Wed, 14 Feb 2018 01:45:36 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1518601536; cv=none; d=google.com; s=arc-20160816; b=GknxKz40jWVlR/aPjYkUZUfuyyHL0kum5RQVbtO0irJKiCh0lruhg0TWopTF76IlOV qHbZbfSyoUZqXkZ6e6FOpBBjEMDBVCvkuYqfnGx2X/a7rqCgxlPvL6ADXcSMUcQKJ5CJ lJVK0CXhE69CMIprOoLio5sh4bEJonuG/IukKyRplf99jPp42t4mJAPi/uO5ftMbzF4S G6dtZG0HYfD6DpXU80yUPHCweDo70g8oGOCShsD6lP2MC0GcmBh18Nc7rSYaiR4ZIuxJ kavlRBtvhkupguoxt41eV4XG6hZpfBxMUSSNPYuF2o35djBE/q1ICDs/h09Q1K74CRR6 rz8w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:arc-authentication-results; bh=MByIcy2TaH2ocsDmiIUQUVkvlRJxJBmGwTFx4l5tJNc=; b=LvusDxYawhps9/gjsLgtithxINxG0z9Ct0WCtfF1YibFfTTl2A3Ms4wq2ZxZBVQdCf iRC9ArltJUMFnKiEkCPs3UBSa9nte+33f5O5KAB9Kkt078MUgntqBLcgK9QUSKTQUf39 Ics/CjXr6TXzywBghZFOxPAldBWToESHoE/RcEZyvmj6URSoej75U1C4lQKvbG5qt+W5 oSOhj1m75eLtOBx0v/MLb7DhFmPeIB5sNmkqqXwm6wPO6hIrY+3PIIbhgZQcjRHnD+mq xAxPxd/ksY4SsP16/H1YIu2PFr379GNgPg9mvqfIEDn8VIETbdJTsPbDn1fzwmPPBIb3 APYw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 1-v6si978185plz.756.2018.02.14.01.45.22; Wed, 14 Feb 2018 01:45:36 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966980AbeBNJok (ORCPT + 99 others); Wed, 14 Feb 2018 04:44:40 -0500 Received: from mail.skyhub.de ([5.9.137.197]:49722 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966503AbeBNJog (ORCPT ); Wed, 14 Feb 2018 04:44:36 -0500 X-Virus-Scanned: Nedap ESD1 at mail.skyhub.de Received: from mail.skyhub.de ([127.0.0.1]) by localhost (blast.alien8.de [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id km8TB24PY9CK; Wed, 14 Feb 2018 10:44:34 +0100 (CET) Received: from pd.tnic (p200300EC2BC9550024931BD807293D49.dip0.t-ipconnect.de [IPv6:2003:ec:2bc9:5500:2493:1bd8:729:3d49]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.skyhub.de (SuperMail on ZX Spectrum 128k) with ESMTPSA id 258C81EC05F5; Wed, 14 Feb 2018 10:44:34 +0100 (CET) Date: Wed, 14 Feb 2018 10:44:05 +0100 From: Borislav Petkov To: Ingo Molnar Cc: Tim Chen , Peter Zijlstra , Dave Hansen , hpa@zytor.com, tglx@linutronix.de, torvalds@linux-foundation.org, linux-kernel@vger.kernel.org, dwmw@amazon.co.uk, linux-tip-commits@vger.kernel.org, Arjan van de Ven , Jerry Hoemann Subject: Re: [PATCH] watchdog: hpwdt: Remove spinlock acquire and BIOS calls from NMI context Message-ID: <20180214094404.GA18349@pd.tnic> References: <1518362359-1005-1-git-send-email-dwmw@amazon.co.uk> <20180212102211.cdrrqqd4hdw7xu5y@gmail.com> <20180212165835.GO25181@hirez.programming.kicks-ass.net> <20180213075540.3lkikkpgjoe6ocjk@gmail.com> <5c3ba123-abbe-f153-7b75-a89d31d25c72@linux.intel.com> <20180214093159.mdzfupne547bi5qx@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20180214093159.mdzfupne547bi5qx@gmail.com> User-Agent: Mutt/1.9.3 (2018-01-21) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org + Jerry On Wed, Feb 14, 2018 at 10:31:59AM +0100, Ingo Molnar wrote: > > * Tim Chen wrote: > > > Dave Hansen and I had some discussions on how to handle the nested NMI and > > firmware calls. We thought of using a per cpu counter to record the nesting > > call depth and toggle IBRS appropriately for the depth 0->1 and 1->0 transition. > > Will this change be sufficient? > > Yeah, so I think the first question should be: does the firmware call from NMI > context make sense to begin with? > > Because in this particular case it does not appear to be so: the reason for the > BIOS/firmware call appears to be to determine how we nmi_panic() after receiving > an NMI that no other NMI handler handled: with a passive-aggressive "I don't know" > panic message or with a slightly more informative panic message. > > That's not a real value-add to users - so we can avoid all these complications by > applying the patch below: > > drivers/watchdog/hpwdt.c | 30 ++++-------------------------- > 1 file changed, 4 insertions(+), 26 deletions(-) > > As a bonus the spinlock use can be removed as well. > > Thanks, > > Ingo > > ====================> > From b038428a739a3fcf0b9678305c131f60af7422ca Mon Sep 17 00:00:00 2001 > From: Ingo Molnar > Date: Wed, 14 Feb 2018 10:24:41 +0100 > Subject: [PATCH] watchdog: hpwdt: Remove spinlock acquire and BIOS calls from > NMI context > > Taking a spinlock and calling into the firmware are problematic things to > do from NMI callbacks. > > It also seems completely pointless in this particular case: > > - cmn_regs is updated by the firmware call in the hpwdt_pretimeout() NMI > callback, but there's almost no use for it: we use it only to determine > whether to panic with an 'unknown NMI' or a slightly more descriptive > message. > > - cmn_regs and rom_lock is not used anywhere else, other than early detection > of the firmware capability. > > So remove rom_lock, make the cmn_regs call from the detection routine only, > and thus make the NMI callback a lot more robust. > > Signed-off-by: Ingo Molnar > --- > drivers/watchdog/hpwdt.c | 30 ++++-------------------------- > 1 file changed, 4 insertions(+), 26 deletions(-) > > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c > index f1f00dfc0e68..2544fe482fe3 100644 > --- a/drivers/watchdog/hpwdt.c > +++ b/drivers/watchdog/hpwdt.c > @@ -150,9 +150,7 @@ static unsigned int hpwdt_nmi_decoding; > static unsigned int allow_kdump = 1; > static unsigned int is_icru; > static unsigned int is_uefi; > -static DEFINE_SPINLOCK(rom_lock); > static void *cru_rom_addr; > -static struct cmn_registers cmn_regs; > > extern asmlinkage void asminline_call(struct cmn_registers *pi86Regs, > unsigned long *pRomEntry); > @@ -225,6 +223,7 @@ static int cru_detect(unsigned long map_entry, > unsigned long physical_bios_base = 0; > unsigned long physical_bios_offset = 0; > int retval = -ENODEV; > + struct cmn_registers cmn_regs = { }; > > bios32_map = ioremap(map_entry, (2 * PAGE_SIZE)); > > @@ -486,32 +485,18 @@ static int hpwdt_my_nmi(void) > */ > static int hpwdt_pretimeout(unsigned int ulReason, struct pt_regs *regs) > { > - unsigned long rom_pl; > - static int die_nmi_called; > - > if (!hpwdt_nmi_decoding) > return NMI_DONE; > > if ((ulReason == NMI_UNKNOWN) && !hpwdt_my_nmi()) > return NMI_DONE; > > - spin_lock_irqsave(&rom_lock, rom_pl); > - if (!die_nmi_called && !is_icru && !is_uefi) > - asminline_call(&cmn_regs, cru_rom_addr); > - die_nmi_called = 1; > - spin_unlock_irqrestore(&rom_lock, rom_pl); > - > if (allow_kdump) > hpwdt_stop(); > > - if (!is_icru && !is_uefi) { > - if (cmn_regs.u1.ral == 0) { > - nmi_panic(regs, "An NMI occurred, but unable to determine source.\n"); > - return NMI_HANDLED; > - } > - } > - nmi_panic(regs, "An NMI occurred. Depending on your system the reason " > - "for the NMI is logged in any one of the following " > + nmi_panic(regs, > + "An NMI occurred. Depending on your system the reason " > + "for the NMI might be logged in any one of the following " > "resources:\n" > "1. Integrated Management Log (IML)\n" > "2. OA Syslog\n" > @@ -744,13 +729,6 @@ static int hpwdt_init_nmi_decoding(struct pci_dev *dev) > HPWDT_ARCH); > return retval; > } > - > - /* > - * We know this is the only CRU call we need to make so lets keep as > - * few instructions as possible once the NMI comes in. > - */ > - cmn_regs.u1.rah = 0x0D; > - cmn_regs.u1.ral = 0x02; > } > > /* -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.