Re: [RFC PATCH 1/4] purgatory/ipmi: Support BMC watchdog timer start/stop in purgatory

[Corey Minyard]

A general note here.  It does not appear that you implement the
error recovery states in your state machine.  If the system fails
in the middle of doing an IPMI operation, it is likely to fail.

If you do this you will need to detect and abort any running
operation.  Implementing the full state machine is probably the
best approach, it should handle this, though it is rather complex.

-corey

On 01/20/2016 04:37 AM, Hidehiro Kawai wrote:

This patch adds an interface to BMC via KCS I/F and a functionality
to start/stop BMC watchdog timer in purgatory.  Starting the watchdog
timer is useful to automatically reboot the server when we fail to
boot the second kernel.  Stopping the watchdog timer is useful to
prevent the second kernel from being stopped by the watchdog timer
enabled while the first kernel is running.

If you specify --ipmi-wdt-start or --ipmi-wdt-stop option to kexec
command, BMC's watchdog timer will start or stop respectively while
executing purgatory.  You can't specify the both options at the same
time.  The start operation doesn't change the parameters of the
watchdog timer such as initial counter and action, you need to set
those parameters in the first OS.  On the other hand, the stop
operation changes the parameters.  You need to reset them when you
want to reuse the watchdog timer.

Signed-off-by: Hidehiro Kawai 
---
  kexec/ipmi.h  |9 ++
  kexec/kexec.c |   18 +++
  kexec/kexec.h |6 +
  purgatory/Makefile|1
  purgatory/include/purgatory.h |3 +
  purgatory/ipmi.c  |  232 +
  purgatory/purgatory.c |4 +
  7 files changed, 272 insertions(+), 1 deletion(-)
  create mode 100644 kexec/ipmi.h
  create mode 100644 purgatory/ipmi.c

diff --git a/kexec/ipmi.h b/kexec/ipmi.h
new file mode 100644
index 000..395a2c7
--- /dev/null
+++ b/kexec/ipmi.h
@@ -0,0 +1,9 @@
+#ifndef IPMI_H
+#define IPMI_H
+
+/* Options for IPMI code excuted in purgatory */
+#define IPMI_WDT_DO_NOTHING0
+#define IPMI_WDT_START (1 << 0)
+#define IPMI_WDT_STOP  (1 << 1)
+
+#endif /* IPMI_H */
diff --git a/kexec/kexec.c b/kexec/kexec.c
index f0bd527..8a8f268 100644
--- a/kexec/kexec.c
+++ b/kexec/kexec.c
@@ -49,6 +49,7 @@
  #include "kexec-sha256.h"
  #include "kexec-zlib.h"
  #include "kexec-lzma.h"
+#include "ipmi.h"
  #include 
  
  unsigned long long mem_min = 0;

@@ -57,6 +58,7 @@ static unsigned long kexec_flags = 0;
  /* Flags for kexec file (fd) based syscall */
  static unsigned long kexec_file_flags = 0;
  int kexec_debug = 0;
+int opt_ipmi_wdt = IPMI_WDT_DO_NOTHING;
  
  void dbgprint_mem_range(const char *prefix, struct memory_range *mr, int nr_mr)

  {
@@ -643,6 +645,10 @@ static void update_purgatory(struct kexec_info *info)
if (!info->rhdr.e_shdr) {
return;
}
+
+   elf_rel_set_symbol(>rhdr, "ipmi_wdt", _ipmi_wdt,
+  sizeof(opt_ipmi_wdt));
+
arch_update_purgatory(info);
memset(region, 0, sizeof(region));
sha256_starts();
@@ -1345,6 +1351,12 @@ int main(int argc, char *argv[])
case OPT_KEXEC_FILE_SYSCALL:
/* We already parsed it. Nothing to do. */
break;
+   case OPT_IPMI_WDT_START:
+   opt_ipmi_wdt |= IPMI_WDT_START;
+   break;
+   case OPT_IPMI_WDT_STOP:
+   opt_ipmi_wdt |= IPMI_WDT_STOP;
+   break;
default:
break;
}
@@ -1370,6 +1382,12 @@ int main(int argc, char *argv[])
"\"--mem-max\" parameter\n");
}
  
+	if ((opt_ipmi_wdt & IPMI_WDT_START) &&

+   (opt_ipmi_wdt & IPMI_WDT_STOP)) {
+   die("You can't specify both --ipmi-wdt-start and "
+   "--ipmi-wdt-stop\n");
+   }
+
fileind = optind;
/* Reset getopt for the next pass; called in other source modules */
opterr = 1;
diff --git a/kexec/kexec.h b/kexec/kexec.h
index c02ac8f..4638866 100644
--- a/kexec/kexec.h
+++ b/kexec/kexec.h
@@ -224,7 +224,9 @@ extern int file_types;
  #define OPT_LOAD_PRESERVE_CONTEXT 259
  #define OPT_LOAD_JUMP_BACK_HELPER 260
  #define OPT_ENTRY 261
-#define OPT_MAX262
+#define OPT_IPMI_WDT_START 262
+#define OPT_IPMI_WDT_STOP  263
+#define OPT_MAX264
  #define KEXEC_OPTIONS \
{ "help", 0, 0, OPT_HELP }, \
{ "version",  0, 0, OPT_VERSION }, \
@@ -244,6 +246,8 @@ extern int file_types;
{ "reuseinitrd",  0, 0, OPT_REUSE_INITRD }, \
{ "kexec-file-syscall",   0, 0, OPT_KEXEC_FILE_SYSCALL }, \
{ "debug",0, 0, OPT_DEBUG }, \
+   { "ipmi-wdt-start",   0, 0, OPT_IPMI_WDT_START }, \
+   { "ipmi-wdt-stop",

RE: [RFC PATCH 1/4] purgatory/ipmi: Support BMC watchdog timer start/stop in purgatory

[河合英宏 / KAWAI，HIDEHIRO]

> A general note here.  It does not appear that you implement the
> error recovery states in your state machine.  If the system fails
> in the middle of doing an IPMI operation, it is likely to fail.

The reason why I din't implement the error handling is that
I think the error rate is low and it may take many seconds (but I
don't have any statistical data, that's my anticipation).

The most important thing is to start booting the 2nd kernel surely
and as soon as possible.  For example, if a user uses a feature
like fence_kdump and if the execution of fence_kdump gets delayed,
the crashed host will be shot down by other host waiting for the
notification from fence_kdump.

Also, to keep the code simple is important for the reliability.

Anyway, I'll rethink whether I can implement the error handling
in simple logic or not.

> If you do this you will need to detect and abort any running
> operation.  Implementing the full state machine is probably the
> best approach, it should handle this, though it is rather complex.
> 
> -corey

Regards,
--
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec