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;
Date:   Sat, 17 Feb 2018 08:08:18 -0800
From:   Guenter Roeck <linux@roeck-us.net>
To:     Jerry Hoemann <jerry.hoemann@hpe.com>
Cc:     wim@linux-watchdog.org, linux-watchdog@vger.kernel.org,
        linux-kernel@vger.kernel.org, rwright@hpe.com,
        maurice.a.saldivar@hpe.com, mingo@kernel.org,
        marcus.folkesson@gmail.com
Subject: Re: [v3,01/11] watchdog/hpwdt: Remove legacy NMI sourcing.
Message-ID: <20180217160818.GA12622@roeck-us.net>
References: <20180215234400.5022-2-jerry.hoemann@hpe.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180215234400.5022-2-jerry.hoemann@hpe.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Feb 15, 2018 at 04:43:50PM -0700, Jerry Hoemann wrote:
> Gen8 and prior Proliant systems supported the "CRU" interface
> to firmware.  This interfaces allows linux to "call back" into firmware
> to source the cause of an NMI.  This feature isn't fully utilized
> as the actual source of the NMI isn't printed, the driver only
> indicates that the source couldn't be determined when the call
> fails.
> 
> With the advent of Gen9, iCRU replaces the CRU. The call back
> feature is no longer available in firmware.  To be compatible and
> not attempt to call back into firmware on system not supporting CRU,
> the SMBIOS table is consulted to determine if it is safe to
> make the call back or not.
> 
> This results in about half of the driver code being devoted
> to either making CRU calls or determing if it is safe to make
> CRU calls.  As noted, the driver isn't really using the results of
> the CRU calls.
> 
> As the CRU sourcing of the NMI isn't required for handling the
> NMI, remove the legacy (pre Gen9) NMI sourcing and the DMI code to
> determine if the system had the CRU interface.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> Acked-by: Ingo Molnar <mingo@kernel.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/hpwdt.c | 490 +----------------------------------------------
>  1 file changed, 8 insertions(+), 482 deletions(-)
> 
> diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
> index f1f00dfc0e68..113058644fc3 100644
> --- a/drivers/watchdog/hpwdt.c
> +++ b/drivers/watchdog/hpwdt.c
> @@ -48,6 +48,9 @@
>  static unsigned int soft_margin = DEFAULT_MARGIN;	/* in seconds */
>  static unsigned int reload;			/* the computed soft_margin */
>  static bool nowayout = WATCHDOG_NOWAYOUT;
> +#ifdef CONFIG_HPWDT_NMI_DECODING
> +static unsigned int allow_kdump = 1;
> +#endif
>  static char expect_release;
>  static unsigned long hpwdt_is_open;
>  
> @@ -63,373 +66,6 @@ static const struct pci_device_id hpwdt_devices[] = {
>  };
>  MODULE_DEVICE_TABLE(pci, hpwdt_devices);
>  
> -#ifdef CONFIG_HPWDT_NMI_DECODING
> -#define PCI_BIOS32_SD_VALUE		0x5F32335F	/* "_32_" */
> -#define CRU_BIOS_SIGNATURE_VALUE	0x55524324
> -#define PCI_BIOS32_PARAGRAPH_LEN	16
> -#define PCI_ROM_BASE1			0x000F0000
> -#define ROM_SIZE			0x10000
> -
> -struct bios32_service_dir {
> -	u32 signature;
> -	u32 entry_point;
> -	u8 revision;
> -	u8 length;
> -	u8 checksum;
> -	u8 reserved[5];
> -};
> -
> -/* type 212 */
> -struct smbios_cru64_info {
> -	u8 type;
> -	u8 byte_length;
> -	u16 handle;
> -	u32 signature;
> -	u64 physical_address;
> -	u32 double_length;
> -	u32 double_offset;
> -};
> -#define SMBIOS_CRU64_INFORMATION	212
> -
> -/* type 219 */
> -struct smbios_proliant_info {
> -	u8 type;
> -	u8 byte_length;
> -	u16 handle;
> -	u32 power_features;
> -	u32 omega_features;
> -	u32 reserved;
> -	u32 misc_features;
> -};
> -#define SMBIOS_ICRU_INFORMATION		219
> -
> -
> -struct cmn_registers {
> -	union {
> -		struct {
> -			u8 ral;
> -			u8 rah;
> -			u16 rea2;
> -		};
> -		u32 reax;
> -	} u1;
> -	union {
> -		struct {
> -			u8 rbl;
> -			u8 rbh;
> -			u8 reb2l;
> -			u8 reb2h;
> -		};
> -		u32 rebx;
> -	} u2;
> -	union {
> -		struct {
> -			u8 rcl;
> -			u8 rch;
> -			u16 rec2;
> -		};
> -		u32 recx;
> -	} u3;
> -	union {
> -		struct {
> -			u8 rdl;
> -			u8 rdh;
> -			u16 red2;
> -		};
> -		u32 redx;
> -	} u4;
> -
> -	u32 resi;
> -	u32 redi;
> -	u16 rds;
> -	u16 res;
> -	u32 reflags;
> -}  __attribute__((packed));
> -
> -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);
> -
> -#ifdef CONFIG_X86_32
> -/* --32 Bit Bios------------------------------------------------------------ */
> -
> -#define HPWDT_ARCH	32
> -
> -asm(".text                          \n\t"
> -    ".align 4                       \n\t"
> -    ".globl asminline_call	    \n"
> -    "asminline_call:                \n\t"
> -    "pushl       %ebp               \n\t"
> -    "movl        %esp, %ebp         \n\t"
> -    "pusha                          \n\t"
> -    "pushf                          \n\t"
> -    "push        %es                \n\t"
> -    "push        %ds                \n\t"
> -    "pop         %es                \n\t"
> -    "movl        8(%ebp),%eax       \n\t"
> -    "movl        4(%eax),%ebx       \n\t"
> -    "movl        8(%eax),%ecx       \n\t"
> -    "movl        12(%eax),%edx      \n\t"
> -    "movl        16(%eax),%esi      \n\t"
> -    "movl        20(%eax),%edi      \n\t"
> -    "movl        (%eax),%eax        \n\t"
> -    "push        %cs                \n\t"
> -    "call        *12(%ebp)          \n\t"
> -    "pushf                          \n\t"
> -    "pushl       %eax               \n\t"
> -    "movl        8(%ebp),%eax       \n\t"
> -    "movl        %ebx,4(%eax)       \n\t"
> -    "movl        %ecx,8(%eax)       \n\t"
> -    "movl        %edx,12(%eax)      \n\t"
> -    "movl        %esi,16(%eax)      \n\t"
> -    "movl        %edi,20(%eax)      \n\t"
> -    "movw        %ds,24(%eax)       \n\t"
> -    "movw        %es,26(%eax)       \n\t"
> -    "popl        %ebx               \n\t"
> -    "movl        %ebx,(%eax)        \n\t"
> -    "popl        %ebx               \n\t"
> -    "movl        %ebx,28(%eax)      \n\t"
> -    "pop         %es                \n\t"
> -    "popf                           \n\t"
> -    "popa                           \n\t"
> -    "leave                          \n\t"
> -    "ret                            \n\t"
> -    ".previous");
> -
> -
> -/*
> - *	cru_detect
> - *
> - *	Routine Description:
> - *	This function uses the 32-bit BIOS Service Directory record to
> - *	search for a $CRU record.
> - *
> - *	Return Value:
> - *	0        :  SUCCESS
> - *	<0       :  FAILURE
> - */
> -static int cru_detect(unsigned long map_entry,
> -	unsigned long map_offset)
> -{
> -	void *bios32_map;
> -	unsigned long *bios32_entrypoint;
> -	unsigned long cru_physical_address;
> -	unsigned long cru_length;
> -	unsigned long physical_bios_base = 0;
> -	unsigned long physical_bios_offset = 0;
> -	int retval = -ENODEV;
> -
> -	bios32_map = ioremap(map_entry, (2 * PAGE_SIZE));
> -
> -	if (bios32_map == NULL)
> -		return -ENODEV;
> -
> -	bios32_entrypoint = bios32_map + map_offset;
> -
> -	cmn_regs.u1.reax = CRU_BIOS_SIGNATURE_VALUE;
> -
> -	set_memory_x((unsigned long)bios32_map, 2);
> -	asminline_call(&cmn_regs, bios32_entrypoint);
> -
> -	if (cmn_regs.u1.ral != 0) {
> -		pr_warn("Call succeeded but with an error: 0x%x\n",
> -			cmn_regs.u1.ral);
> -	} else {
> -		physical_bios_base = cmn_regs.u2.rebx;
> -		physical_bios_offset = cmn_regs.u4.redx;
> -		cru_length = cmn_regs.u3.recx;
> -		cru_physical_address =
> -			physical_bios_base + physical_bios_offset;
> -
> -		/* If the values look OK, then map it in. */
> -		if ((physical_bios_base + physical_bios_offset)) {
> -			cru_rom_addr =
> -				ioremap(cru_physical_address, cru_length);
> -			if (cru_rom_addr) {
> -				set_memory_x((unsigned long)cru_rom_addr & PAGE_MASK,
> -					(cru_length + PAGE_SIZE - 1) >> PAGE_SHIFT);
> -				retval = 0;
> -			}
> -		}
> -
> -		pr_debug("CRU Base Address:   0x%lx\n", physical_bios_base);
> -		pr_debug("CRU Offset Address: 0x%lx\n", physical_bios_offset);
> -		pr_debug("CRU Length:         0x%lx\n", cru_length);
> -		pr_debug("CRU Mapped Address: %p\n", &cru_rom_addr);
> -	}
> -	iounmap(bios32_map);
> -	return retval;
> -}
> -
> -/*
> - *	bios_checksum
> - */
> -static int bios_checksum(const char __iomem *ptr, int len)
> -{
> -	char sum = 0;
> -	int i;
> -
> -	/*
> -	 * calculate checksum of size bytes. This should add up
> -	 * to zero if we have a valid header.
> -	 */
> -	for (i = 0; i < len; i++)
> -		sum += ptr[i];
> -
> -	return ((sum == 0) && (len > 0));
> -}
> -
> -/*
> - *	bios32_present
> - *
> - *	Routine Description:
> - *	This function finds the 32-bit BIOS Service Directory
> - *
> - *	Return Value:
> - *	0        :  SUCCESS
> - *	<0       :  FAILURE
> - */
> -static int bios32_present(const char __iomem *p)
> -{
> -	struct bios32_service_dir *bios_32_ptr;
> -	int length;
> -	unsigned long map_entry, map_offset;
> -
> -	bios_32_ptr = (struct bios32_service_dir *) p;
> -
> -	/*
> -	 * Search for signature by checking equal to the swizzled value
> -	 * instead of calling another routine to perform a strcmp.
> -	 */
> -	if (bios_32_ptr->signature == PCI_BIOS32_SD_VALUE) {
> -		length = bios_32_ptr->length * PCI_BIOS32_PARAGRAPH_LEN;
> -		if (bios_checksum(p, length)) {
> -			/*
> -			 * According to the spec, we're looking for the
> -			 * first 4KB-aligned address below the entrypoint
> -			 * listed in the header. The Service Directory code
> -			 * is guaranteed to occupy no more than 2 4KB pages.
> -			 */
> -			map_entry = bios_32_ptr->entry_point & ~(PAGE_SIZE - 1);
> -			map_offset = bios_32_ptr->entry_point - map_entry;
> -
> -			return cru_detect(map_entry, map_offset);
> -		}
> -	}
> -	return -ENODEV;
> -}
> -
> -static int detect_cru_service(void)
> -{
> -	char __iomem *p, *q;
> -	int rc = -1;
> -
> -	/*
> -	 * Search from 0x0f0000 through 0x0fffff, inclusive.
> -	 */
> -	p = ioremap(PCI_ROM_BASE1, ROM_SIZE);
> -	if (p == NULL)
> -		return -ENOMEM;
> -
> -	for (q = p; q < p + ROM_SIZE; q += 16) {
> -		rc = bios32_present(q);
> -		if (!rc)
> -			break;
> -	}
> -	iounmap(p);
> -	return rc;
> -}
> -/* ------------------------------------------------------------------------- */
> -#endif /* CONFIG_X86_32 */
> -#ifdef CONFIG_X86_64
> -/* --64 Bit Bios------------------------------------------------------------ */
> -
> -#define HPWDT_ARCH	64
> -
> -asm(".text                      \n\t"
> -    ".align 4                   \n\t"
> -    ".globl asminline_call	\n\t"
> -    ".type asminline_call, @function \n\t"
> -    "asminline_call:            \n\t"
> -    FRAME_BEGIN
> -    "pushq      %rax            \n\t"
> -    "pushq      %rbx            \n\t"
> -    "pushq      %rdx            \n\t"
> -    "pushq      %r12            \n\t"
> -    "pushq      %r9             \n\t"
> -    "movq       %rsi, %r12      \n\t"
> -    "movq       %rdi, %r9       \n\t"
> -    "movl       4(%r9),%ebx     \n\t"
> -    "movl       8(%r9),%ecx     \n\t"
> -    "movl       12(%r9),%edx    \n\t"
> -    "movl       16(%r9),%esi    \n\t"
> -    "movl       20(%r9),%edi    \n\t"
> -    "movl       (%r9),%eax      \n\t"
> -    "call       *%r12           \n\t"
> -    "pushfq                     \n\t"
> -    "popq        %r12           \n\t"
> -    "movl       %eax, (%r9)     \n\t"
> -    "movl       %ebx, 4(%r9)    \n\t"
> -    "movl       %ecx, 8(%r9)    \n\t"
> -    "movl       %edx, 12(%r9)   \n\t"
> -    "movl       %esi, 16(%r9)   \n\t"
> -    "movl       %edi, 20(%r9)   \n\t"
> -    "movq       %r12, %rax      \n\t"
> -    "movl       %eax, 28(%r9)   \n\t"
> -    "popq       %r9             \n\t"
> -    "popq       %r12            \n\t"
> -    "popq       %rdx            \n\t"
> -    "popq       %rbx            \n\t"
> -    "popq       %rax            \n\t"
> -    FRAME_END
> -    "ret                        \n\t"
> -    ".previous");
> -
> -/*
> - *	dmi_find_cru
> - *
> - *	Routine Description:
> - *	This function checks whether or not a SMBIOS/DMI record is
> - *	the 64bit CRU info or not
> - */
> -static void dmi_find_cru(const struct dmi_header *dm, void *dummy)
> -{
> -	struct smbios_cru64_info *smbios_cru64_ptr;
> -	unsigned long cru_physical_address;
> -
> -	if (dm->type == SMBIOS_CRU64_INFORMATION) {
> -		smbios_cru64_ptr = (struct smbios_cru64_info *) dm;
> -		if (smbios_cru64_ptr->signature == CRU_BIOS_SIGNATURE_VALUE) {
> -			cru_physical_address =
> -				smbios_cru64_ptr->physical_address +
> -				smbios_cru64_ptr->double_offset;
> -			cru_rom_addr = ioremap(cru_physical_address,
> -				smbios_cru64_ptr->double_length);
> -			set_memory_x((unsigned long)cru_rom_addr & PAGE_MASK,
> -				smbios_cru64_ptr->double_length >> PAGE_SHIFT);
> -		}
> -	}
> -}
> -
> -static int detect_cru_service(void)
> -{
> -	cru_rom_addr = NULL;
> -
> -	dmi_walk(dmi_find_cru, NULL);
> -
> -	/* if cru_rom_addr has been set then we found a CRU service */
> -	return ((cru_rom_addr != NULL) ? 0 : -ENODEV);
> -}
> -/* ------------------------------------------------------------------------- */
> -#endif /* CONFIG_X86_64 */
> -#endif /* CONFIG_HPWDT_NMI_DECODING */
>  
>  /*
>   *	Watchdog operations
> @@ -486,30 +122,12 @@ 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 "
>  		"resources:\n"
> @@ -675,84 +293,11 @@ static struct miscdevice hpwdt_miscdev = {
>   *	Init & Exit
>   */
>  
> -#ifdef CONFIG_HPWDT_NMI_DECODING
> -#ifdef CONFIG_X86_LOCAL_APIC
> -static void hpwdt_check_nmi_decoding(struct pci_dev *dev)
> -{
> -	/*
> -	 * If nmi_watchdog is turned off then we can turn on
> -	 * our nmi decoding capability.
> -	 */
> -	hpwdt_nmi_decoding = 1;
> -}
> -#else
> -static void hpwdt_check_nmi_decoding(struct pci_dev *dev)
> -{
> -	dev_warn(&dev->dev, "NMI decoding is disabled. "
> -		"Your kernel does not support a NMI Watchdog.\n");
> -}
> -#endif /* CONFIG_X86_LOCAL_APIC */
> -
> -/*
> - *	dmi_find_icru
> - *
> - *	Routine Description:
> - *	This function checks whether or not we are on an iCRU-based server.
> - *	This check is independent of architecture and needs to be made for
> - *	any ProLiant system.
> - */
> -static void dmi_find_icru(const struct dmi_header *dm, void *dummy)
> -{
> -	struct smbios_proliant_info *smbios_proliant_ptr;
> -
> -	if (dm->type == SMBIOS_ICRU_INFORMATION) {
> -		smbios_proliant_ptr = (struct smbios_proliant_info *) dm;
> -		if (smbios_proliant_ptr->misc_features & 0x01)
> -			is_icru = 1;
> -		if (smbios_proliant_ptr->misc_features & 0x1400)
> -			is_uefi = 1;
> -	}
> -}
>  
>  static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
>  {
> +#ifdef CONFIG_HPWDT_NMI_DECODING	/* { */
>  	int retval;
> -
> -	/*
> -	 * On typical CRU-based systems we need to map that service in
> -	 * the BIOS. For 32 bit Operating Systems we need to go through
> -	 * the 32 Bit BIOS Service Directory. For 64 bit Operating
> -	 * Systems we get that service through SMBIOS.
> -	 *
> -	 * On systems that support the new iCRU service all we need to
> -	 * do is call dmi_walk to get the supported flag value and skip
> -	 * the old cru detect code.
> -	 */
> -	dmi_walk(dmi_find_icru, NULL);
> -	if (!is_icru && !is_uefi) {
> -
> -		/*
> -		* We need to map the ROM to get the CRU service.
> -		* For 32 bit Operating Systems we need to go through the 32 Bit
> -		* BIOS Service Directory
> -		* For 64 bit Operating Systems we get that service through SMBIOS.
> -		*/
> -		retval = detect_cru_service();
> -		if (retval < 0) {
> -			dev_warn(&dev->dev,
> -				"Unable to detect the %d Bit CRU Service.\n",
> -				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;
> -	}
> -
>  	/*
>  	 * Only one function can register for NMI_UNKNOWN
>  	 */
> @@ -780,33 +325,19 @@ static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
>  	dev_warn(&dev->dev,
>  		"Unable to register a die notifier (err=%d).\n",
>  		retval);
> -	if (cru_rom_addr)
> -		iounmap(cru_rom_addr);
>  	return retval;
> +#endif					/* } */
> +	return 0;
>  }
>  
>  static void hpwdt_exit_nmi_decoding(void)
>  {
> +#ifdef CONFIG_HPWDT_NMI_DECODING
>  	unregister_nmi_handler(NMI_UNKNOWN, "hpwdt");
>  	unregister_nmi_handler(NMI_SERR, "hpwdt");
>  	unregister_nmi_handler(NMI_IO_CHECK, "hpwdt");
> -	if (cru_rom_addr)
> -		iounmap(cru_rom_addr);
> +#endif
>  }
> -#else /* !CONFIG_HPWDT_NMI_DECODING */
> -static void hpwdt_check_nmi_decoding(struct pci_dev *dev)
> -{
> -}
> -
> -static int hpwdt_init_nmi_decoding(struct pci_dev *dev)
> -{
> -	return 0;
> -}
> -
> -static void hpwdt_exit_nmi_decoding(void)
> -{
> -}
> -#endif /* CONFIG_HPWDT_NMI_DECODING */
>  
>  static int hpwdt_init_one(struct pci_dev *dev,
>  					const struct pci_device_id *ent)
> @@ -814,11 +345,6 @@ static int hpwdt_init_one(struct pci_dev *dev,
>  	int retval;
>  
>  	/*
> -	 * Check if we can do NMI decoding or not
> -	 */
> -	hpwdt_check_nmi_decoding(dev);
> -
> -	/*
>  	 * First let's find out if we are on an iLO2+ server. We will
>  	 * not run on a legacy ASM box.
>  	 * So we only support the G5 ProLiant servers and higher.