PLDM Runtime BMC R/R support: Fix deadlock in runtime PNOR logic (commit 4/n)

This commit fixes two issues in Hostboot runtime code:

1. The constructor of the runtime PNOR component makes several PLDM
   requests. If an error happens during these PLDM requests, an error
   is created, which itself makes an attempt to access PNOR through
   the same RtPnor Singleton instance. This causes the constructor of
   the Singleton class' RtPnor instance to be re-entered. This is
   undefined behavior in C++, and in HBRT it results in a deadlock as
   the thread accessing the Singleton waits on itself to finish
   initializing the Singleton.

   A similar cycle exists for the ErrlManager Singleton instance; when
   creating an error, the ErrlEntry constructor requests a unique ID
   for itself from the ErrlManager singleton, which initializes
   it. The constructor accesses PNOR, which can fail, which in turn
   creates another error, which accesses the ErrlManager again,
   resulting in a deadlock.

   This commit fixes these issues by separating the constructors from
   the initialization of these classes, so that recursive uses don't
   cause problems.

2. Re-entrant constructors in local static variables previously caused
   deadlocks. This commit changes the mutex code to assert when it
   detects that a static variable is already mid-initialization to
   avert an impending deadlock.

Change-Id: I7a61636820776173206865726520626f69696969
RTC: 299918
Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/130043
Tested-by: Jenkins OP Build CI <[email protected]>
Tested-by: Jenkins Server <[email protected]>
Tested-by: FSP CI Jenkins <[email protected]>
Reviewed-by: Matt Derksen <[email protected]>
Reviewed-by: Isaac Salem <[email protected]>
Tested-by: Jenkins Combined Simics CI <[email protected]>
Tested-by: Jenkins OP HW <[email protected]>
Tested-by: Hostboot CI <[email protected]>
Reviewed-by: Daniel M Crowell <[email protected]>

Author: ibmzach

src/include/usr/errl/errlmanager.H

@@ -431,6 +431,13 @@ public:
     static void setLastIplEid(uint32_t i_eid);
 #endif
 
+#ifdef __HOSTBOOT_RUNTIME
+    /* @brief Set up the ErrlManager instance. This must be called before the
+     * ErrlManager is used.
+     */
+    void setup();
+#endif
+
 protected:
     /**
      * @brief   Destructor

src/include/usr/pnor/pnorif.H

@@ -324,6 +324,13 @@ void readAndClearCounter( uint32_t i_threshold,
 const std::array<uint32_t, PNOR::NUM_SECTIONS>& getLidIds();
 #endif
 
+#ifdef __HOSTBOOT_RUNTIME
+/**
+* @brief Whether PNOR is initialized yet.
+*/
+bool isPnorInitialized();
+#endif
+
 } // PNOR
 
 #endif

src/libc++/builtins.C

@@ -5,7 +5,7 @@
 /*                                                                        */
 /* OpenPOWER HostBoot Project                                             */
 /*                                                                        */
-/* Contributors Listed Below - COPYRIGHT 2010,2017                        */
+/* Contributors Listed Below - COPYRIGHT 2010,2022                        */
 /* [+] International Business Machines Corp.                              */
 /*                                                                        */
 /*                                                                        */
@@ -30,6 +30,11 @@
 #include <util/locked/list.H>
 #include <sys/sync.h>
 
+#ifdef __HOSTBOOT_RUNTIME
+#include <assert.h>
+#include <builtins.h>
+#endif
+
 void* operator new(size_t s)
 {
     return malloc(s);
@@ -71,6 +76,19 @@ void operator delete[](void* p, size_t)
 }
 #endif // bl_builtins_C
 
+enum CXA_GUARD_LOCK_VALUE : uint32_t
+{
+    UNINITIALIZED = 0,
+    LOCKED = 1,
+    UNLOCKED_AND_INITIALIZED = 2
+};
+
+enum CXA_GUARD_ACTION : int
+{
+    DO_NOT_RUN_CONSTRUCTOR = 0,
+    RUN_CONSTRUCTOR = 1,
+};
+
 extern "C" int __cxa_guard_acquire(volatile uint64_t* gv)
 {
     // States:
@@ -79,18 +97,30 @@ extern "C" int __cxa_guard_acquire(volatile uint64_t* gv)
     //     2 -> unlocked and initialized
 
     uint32_t v = __sync_val_compare_and_swap((volatile uint32_t*)gv, 0, 1);
-    if (v == 0)
-	return 1;
-    if (v == 2)
-	return 0;
+    if (v == UNINITIALIZED)
+        return RUN_CONSTRUCTOR;
+    if (v == UNLOCKED_AND_INITIALIZED)
+        return DO_NOT_RUN_CONSTRUCTOR;
+
+#ifdef __HOSTBOOT_RUNTIME
+    // Hostboot runtime is single-threaded. If we go to initialize a local
+    // static variable and see that it's already in the "locked" state, it means
+    // that this thread is already initializing the object and we must have
+    // entered this path recursively. Trying to acquire the lock again will
+    // result in a deadlock; so instead, we assert here to fail faster. The C++
+    // standard specifies that if the initialization of a local static variable
+    // is entered recursively, the behavior is undefined, so we are permitted to
+    // do whatever we want here.
+    crit_assert(false);
+#endif
 
     // Wait for peer thread to perform initialization (state 2).
-    while(2 != *(volatile uint32_t*)gv);
+    while(UNLOCKED_AND_INITIALIZED != *(volatile uint32_t*)gv);
 
     // Instruction barrier to ensure value is set before later loads execute.
     isync();
 
-    return 0;
+    return DO_NOT_RUN_CONSTRUCTOR;
 }
 
 extern "C" void __cxa_guard_release(volatile uint64_t* gv)
@@ -176,5 +206,3 @@ extern "C" int __cxa_atexit(void (*i_dtor)(void*),
     return 0;
 }
 #endif // bl_builtins_C
-
-

src/libc++/makefile

@@ -5,7 +5,7 @@
 #
 # OpenPOWER HostBoot Project
 #
-# Contributors Listed Below - COPYRIGHT 2010,2019
+# Contributors Listed Below - COPYRIGHT 2010,2022
 # [+] International Business Machines Corp.
 #
 #
@@ -26,6 +26,9 @@ ROOTPATH = ../..
 
 HOSTBOOT_PROFILE_ARTIFACT=
 
+EXTRAINCDIR = ${ROOTPATH}/src/include/usr
+
 OBJS += builtins.o
+OBJS += rt_builtins.o
 
 include ${ROOTPATH}/config.mk

src/libc++/rt_builtins.C

@@ -0,0 +1,28 @@
+/* IBM_PROLOG_BEGIN_TAG                                                   */
+/* This is an automatically generated prolog.                             */
+/*                                                                        */
+/* $Source: src/libc++/rt_builtins.C $                                    */
+/*                                                                        */
+/* OpenPOWER HostBoot Project                                             */
+/*                                                                        */
+/* Contributors Listed Below - COPYRIGHT 2022                             */
+/* [+] International Business Machines Corp.                              */
+/*                                                                        */
+/*                                                                        */
+/* Licensed under the Apache License, Version 2.0 (the "License");        */
+/* you may not use this file except in compliance with the License.       */
+/* You may obtain a copy of the License at                                */
+/*                                                                        */
+/*     http://www.apache.org/licenses/LICENSE-2.0                         */
+/*                                                                        */
+/* Unless required by applicable law or agreed to in writing, software    */
+/* distributed under the License is distributed on an "AS IS" BASIS,      */
+/* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or        */
+/* implied. See the License for the specific language governing           */
+/* permissions and limitations under the License.                         */
+/*                                                                        */
+/* IBM_PROLOG_END_TAG                                                     */
+
+#define __HOSTBOOT_RUNTIME
+
+#include "builtins.C"

src/usr/errl/errlmanager_common.C

@@ -57,12 +57,59 @@ const uint32_t PNOR_ERROR_LENGTH = 4096;
 const uint32_t EMPTY_ERRLOG_IN_PNOR = 0xFFFFFFFF;
 const uint32_t FIRST_BYTE_ERRLOG = 0xF0000000;
 
-///////////////////////////////////////////////////////////////////////////////
-///////////////////////////////////////////////////////////////////////////////
-// Global function (not a method on an object) to commit the error log.
-void errlCommit(errlHndl_t& io_err, compId_t i_committerComp )
+/* @brief Structure to record the arguments to errlCommit, so that it can be
+ * re-invoked later if called too early.
+ *
+ */
+struct errlCommitRecord
+{
+    errlHndl_t errl;
+    compId_t committerComp;
+};
+
+void errlCommit(errlHndl_t& io_err, const compId_t i_committerComp)
 {
-    ERRORLOG::theErrlManager::instance().commitErrLog(io_err, i_committerComp );
+    do
+    {
+
+#if defined(__HOSTBOOT_RUNTIME) && defined(CONFIG_FILE_XFER_VIA_PLDM)
+    // Committing an error at runtime requires PNOR access. If an error log is
+    // committed before PNOR is available, queue it up to be committed later.
+
+    static std::vector<errlCommitRecord> early_error_logs;
+
+    if (PNOR::isPnorInitialized())
+    { // If PNOR is initialized and we've saved error logs to commit later,
+      // commit them now.
+        if (!early_error_logs.empty())
+        {
+            // Move the storage to a local variable so that the static vector is
+            // empty and we don't enter this path multiple times if we commit an
+            // error in the error commit code. This will also cause the storage
+            // to be deallocated when we're done here.
+            const std::vector<errlCommitRecord> recs = move(early_error_logs);
+
+            auto& theErrlManager = ERRORLOG::theErrlManager::instance();
+
+            for (auto record : recs)
+            {
+                theErrlManager.commitErrLog(record.errl, record.committerComp);
+            }
+        }
+    }
+    else
+    { // If PNOR isn't initialized, we can't commit errors yet. Save the error
+      // for committing later.
+        early_error_logs.push_back({ io_err, i_committerComp });
+        io_err = nullptr;
+        break;
+    }
+#endif
+
+    ERRORLOG::theErrlManager::instance().commitErrLog(io_err, i_committerComp);
+
+    } while (false);
+
     return;
 }
 

src/usr/errl/runtime/rt_errlmanager.C

@@ -73,7 +73,7 @@ bool rt_processCallout(errlHndl_t &io_errl,
 ErrlManager::ErrlManager() :
         iv_currLogId(0),
         iv_baseNodeId(ERRLOG_PLID_BASE_MASK),
-        iv_pnorReadyForErrorLogs(true),
+        iv_pnorReadyForErrorLogs(false),
         iv_pStorage(NULL),
         iv_hwasProcessCalloutFn(NULL),
         iv_pnorAddr(NULL),
@@ -92,7 +92,6 @@ ErrlManager::ErrlManager() :
 #endif
 {
     TRACFCOMP( g_trac_errl, ENTER_MRK "ErrlManager::ErrlManager constructor." );
-    //Note - There is no PNOR access inside HBRT
 
     iv_hwasProcessCalloutFn = rt_processCallout;
 
@@ -108,7 +107,8 @@ ErrlManager::ErrlManager() :
                                 "TARGETING is ready..." );
 
     }
-    if(sys)
+
+    if (sys)
     {
         iv_currLogId = sys->getAttr<TARGETING::ATTR_HOSTSVC_PLID>();
         TRACFCOMP( g_trac_errl,"Initial Error Log ID = %.8X", iv_currLogId );
@@ -124,11 +124,6 @@ ErrlManager::ErrlManager() :
         // specifically disabled via the attribute ATTR_DISABLE_PLD_WAIT,
         // then PLD waits are enabled.
         iv_pldWaitEnable = !iv_isFSP && !sys->getAttr<TARGETING::ATTR_DISABLE_PLD_WAIT>();
-
-        if(!iv_isFSP)
-        {
-            setupPnorInfo();
-        }
     }
     else
     {
@@ -144,6 +139,18 @@ ErrlManager::ErrlManager() :
     TRACFCOMP( g_trac_errl, EXIT_MRK "ErrlManager::ErrlManager constructor." );
 }
 
+void ErrlManager::setup()
+{
+    TRACFCOMP( g_trac_errl, ENTER_MRK"ErrlManager::setup" );
+
+    if (TARGETING::targetService().isInitialized() && !iv_isFSP)
+    {
+        setupPnorInfo();
+    }
+
+    TRACFCOMP( g_trac_errl, EXIT_MRK"ErrlManager::setup" );
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 ///////////////////////////////////////////////////////////////////////////////
 ErrlManager::~ErrlManager()
@@ -773,7 +780,7 @@ void initErrlManager(void)
 {
     // Note: rtPnor needs to be setup before this is called
     // call errlManager ctor so that we're ready and waiting for errors.
-    ERRORLOG::theErrlManager::instance();
+    ERRORLOG::theErrlManager::instance().setup();
 
     // Note: TARGETING needs to be ready before this is called
     // Set the FW Release Version

src/usr/pnor/pnor_common.C

@@ -5,7 +5,7 @@
 /*                                                                        */
 /* OpenPOWER HostBoot Project                                             */
 /*                                                                        */
-/* Contributors Listed Below - COPYRIGHT 2014,2021                        */
+/* Contributors Listed Below - COPYRIGHT 2014,2022                        */
 /* [+] Google Inc.                                                        */
 /* [+] International Business Machines Corp.                              */
 /*                                                                        */
@@ -55,7 +55,13 @@
 
 // Trace definition
 trace_desc_t* g_trac_pnor = NULL;
+
+#ifndef __HOSTBOOT_RUNTIME
+// If this is IPL time code, initialize the trace buffer as normal. If this is
+// runtime code, we need to ensure that this is initialized before the RtPnor
+// instance, so we explicitly sequence the initialization ourselves in rt_pnor.C
 TRAC_INIT(&g_trac_pnor, PNOR_COMP_NAME, 4*KILOBYTE, TRACE::BUFFER_SLOW); //4K
+#endif
 
 // Easy macro replace for unit testing
 //#define TRACUCOMP(args...)  TRACFCOMP(args)