[Skiboot] [PATCH v4 01/18] libstb/secureboot: use platform.terminate instead of hard abort

[Vasant Hegde]

On 5/21/20 12:37 AM, Eric Richter wrote:
> On 5/11/20 10:49 PM, Oliver O'Halloran wrote:
>> On Mon, 2020-05-11 at 16:31 -0500, Eric Richter wrote:
>>> Halting the boot via an abort() call will cause the BMC to keep
>>> restarting the machine indefinitely.
>>
>> The BMC is supposed to detect the host bootlooping and stop powering it
>> on after 5 or so.
>>
>>> Ending via platform.terminate()
>>> should be cleaner and prevent needless bootloops.
>>
>> abort() is a #defined as assert(0) which results in a call to
>> assert_fail():
>>
>> void __noreturn assert_fail(const char *msg, const char *file,
>>                                  unsigned int line, const char *function)
>> {
>>          static bool in_abort = false;
>>
>>          (void)function;
>>          if (in_abort)
>>                  for (;;) ;
>>          in_abort = true;
>>
>>          /* snip comment */
>>          prlog(PR_EMERG, "assert failed at %s:%u: %s\n", file, line, msg);
>>          backtrace();
>>
>>          /* Save crashing CPU details */
>>          opal_mpipl_save_crashing_pir();
>>
>>          if (platform.terminate)
>>                  platform.terminate(msg);
>>
>>          for (;;) ;
>> }
>>
>> So I'm not really seeing how this is an improvement. The terminate
>> callback is usually ipmi_terminate() which will also trigger an MPIPL
>> on the systems which support it. That might be the cause of your boot
>> loop?
>>
> 
> Admittedly, I took the off-list suggestion directly, and didn't look deep into the terminate() behavior. Is there a better method to guaranteed halt the machine that could be used instead? There is no reason to attempt a reboot/MPIPL if a secureboot trap is hit.

I have posted patch to fix MPIPL behavior in boot path [1]. With that assert() 
should work fine.

[1] https://lists.ozlabs.org/pipermail/skiboot/2020-June/017075.html

Let me know if you still hit any issue with assert().

-Vasant