WIP: hw-isolation rebase: Added guard, deconfiguration, sparecore #1279

gcpin · 2025-05-07T10:59:49Z

1. Add support for guard, deconfiguration and sparecore
2. hw-isolation code relocation to systems_logservices_hwisolation.hpp

Following commits from 1110 are used to create this unified commit

332c310625 - redfish-core: LogServices: Added HardwareIsolation service
36dec57033 - LogServices: HardwareIsolation: LogEntryCollection
3486c6abca - LogServices: HardwareIsolation: Get LogEntry
3d981efb6a - LogServices: HardwareIsolation: Delete LogEntry
11e4cda449 - LogServices: HardwareIsolation: Post ClearLog
723c0fdc70 - redfish-core: Core: Enabled the isolation feature
e4cb16309a - redfish-core: Memory:Enabled the isolation feature
e2158ab941 - Core: Fix, Patch a core into the respective parent processor
4237af36b9 - Hw-Isolation: Deconfiguration type for DIMMs
21582d8c65 - Enabled deconfiguration reason support to the DIMM
558b32f3cc - HW-Isolation: Fix, Don't throw internal error if failed to get error log
2ae01eaf3b - HW-Isolation: Fix, Update State if the Core and DIMM are recovered
84aef9b900 - HW-Isolation: Return an appropriate error if the request is failed.
9ac66c52ac - HW-Isolation: Return ResourceCannotBeDeleted error
f769499d62 - LogService: HW-Isolation: Return an appropriate error
c3839dba53 - redfish-core: Core: Enabled the isolation (aka guard) feature
555fc6ca4c - HW-Isolation: Fix, Use GetAncestors to get the parents id
2cf7c3f830 - HW-Isolation: Fill OriginOfCondition for the TPM and Motherboard
3bd202e472 - LogEntry: HW-Isolation: Removed the Resolved property
10ed0438b8 - HardwareIsolation: Spare core support
5e53b61d8a - HW-Isolation: Fix info PEL AdditionalDataURI
50c9dc3076 - HW-Isolation: Fix Additional Data Uri
7adcfa13f1 - Move getHiddenPropertyValue method to utils
b90a99f053 - Hw-Isolation: Fix eventId HW Deconfiguration page

``` 1. Add support for guard, deconfiguration and sparecore 2. hw-isolation code relocation to systems_logservices_hwisolation.hpp Following commits from 1110 are used to create this unified commit 332c310 - redfish-core: LogServices: Added HardwareIsolation service 36dec57 - LogServices: HardwareIsolation: LogEntryCollection 3486c6a - LogServices: HardwareIsolation: Get LogEntry 3d981ef - LogServices: HardwareIsolation: Delete LogEntry 11e4cda - LogServices: HardwareIsolation: Post ClearLog 723c0fd - redfish-core: Core: Enabled the isolation feature e4cb163 - redfish-core: Memory:Enabled the isolation feature e2158ab - Core: Fix, Patch a core into the respective parent processor 4237af3 - Hw-Isolation: Deconfiguration type for DIMMs 21582d8 - Enabled deconfiguration reason support to the DIMM 558b32f - HW-Isolation: Fix, Don't throw internal error if failed to get error log 2ae01ea - HW-Isolation: Fix, Update State if the Core and DIMM are recovered 84aef9b - HW-Isolation: Return an appropriate error if the request is failed. 9ac66c5 - HW-Isolation: Return ResourceCannotBeDeleted error f769499 - LogService: HW-Isolation: Return an appropriate error c3839db - redfish-core: Core: Enabled the isolation (aka guard) feature 555fc6c - HW-Isolation: Fix, Use GetAncestors to get the parents id 2cf7c3f - HW-Isolation: Fill OriginOfCondition for the TPM and Motherboard 3bd202e - LogEntry: HW-Isolation: Removed the Resolved property 10ed043 - HardwareIsolation: Spare core support 5e53b61 - HW-Isolation: Fix info PEL AdditionalDataURI 50c9dc3 - HW-Isolation: Fix Additional Data Uri 7adcfa1 - Move getHiddenPropertyValue method to utils b90a99f - Hw-Isolation: Fix eventId HW Deconfiguration page ``` Signed-off-by: Gopichand Paturi <[email protected]> Co-authored-by: Ramesh Iyyar <[email protected]> Co-authored-by: deepakala k <[email protected]>

jenkins-openbmc-ibm · 2025-05-07T11:04:02Z

Can one of the admins verify this patch?

gtmills · 2025-05-08T13:59:25Z

add to approvelist

baemyung · 2025-05-08T14:45:51Z

redfish-core/include/utils/error_log_utils.hpp

+            asyncResp->res.jsonValue[errorLogPropPath]["@odata.id"] =
+                boost::urls::format(
+                    "/redfish/v1/Systems/system/LogServices/{}/Entries/{}{}",
+                    logPath, entryID, linkAttachment);


I think this is better to be

asyncResp->res.jsonValue[errorLogPropPath]["@odata.id"] = boost::urls::format( "/redfish/v1/Systems/{}/LogServices/{}/Entries/{}{}", BMCWEB_REDFISH_SYSTEM_URI_NAME, logPath, entryID, linkAttachment);

baemyung · 2025-05-08T14:47:11Z

redfish-core/include/utils/hw_isolation.hpp

@@ -0,0 +1,690 @@
+#pragma once


License statement

// SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright OpenBMC Authors #pragma once

baemyung · 2025-05-08T14:48:42Z

redfish-core/include/utils/hw_isolation.hpp

+#include <sdbusplus/message.hpp>
+#include <sdbusplus/message/native_types.hpp>
+#include <utils/error_log_utils.hpp>
+#include <utils/time_utils.hpp>


I think the above 2 include files would need to be (moved to the section L2-L7)

#include "utils/error_log_utils.hpp" #include "utils/time_utils.hpp"

baemyung · 2025-05-08T14:52:04Z

redfish-core/include/utils/hw_isolation.hpp

+        },
+        "xyz.openbmc_project.ObjectMapper",
+        "/xyz/openbmc_project/object_mapper",
+        "xyz.openbmc_project.ObjectMapper", "GetSubTreePaths",


This is better to be done with

dbus::utility::getSubTreePaths(...)

baemyung · 2025-05-08T14:55:42Z

redfish-core/include/utils/hw_isolation.hpp

+                            // the respective hardware is not functional so
+                            // set the state as "Disabled".
+                            asyncResp->res.jsonValue["Status"]["State"] =
+                                "Disabled";


asyncResp->res.jsonValue["Status"]["State"] = resource::State::Disabled;

baemyung · 2025-05-08T16:03:01Z

redfish-core/lib/memory.hpp

+inline void getObjectEnable(std::shared_ptr<bmcweb::AsyncResp> asyncResp,
+                            const std::string& service, const std::string& path)
+{
+    sdbusplus::asio::getProperty<bool>(


Use

dbus::utility::getProperty<bool>()

baemyung · 2025-05-08T16:03:18Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+        }
+        return;
+    }
+    sdbusplus::asio::getProperty<std::string>(


Use

dbus::utility::getProperty<bool>()

baemyung · 2025-05-08T16:03:40Z

redfish-core/include/utils/error_log_utils.hpp

+    const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
+    const std::string& entryId, std::function<void(bool hidden)>&& callback)
+{
+    sdbusplus::asio::getProperty<bool>(


Use

dbus::utility::getProperty<bool>()

baemyung · 2025-05-08T16:04:29Z

redfish-core/include/utils/hw_isolation.hpp

+                    }
+
+                    // Get event properties and fill into status conditions
+                    sdbusplus::asio::getAllProperties(


Use

dbus::utility::getAllProperties

baemyung · 2025-05-08T16:11:58Z

redfish-core/lib/log_services.hpp

+                                   "LogServices/HardwareIsolation"}});
+                asyncResp->res.jsonValue["[email protected]"] =
+                    logServiceArrayLocal.size();
+            }


This would be better like to in the older #1256

if constexpr (BMCWEB_HW_ISOLATION) { nlohmann::json& logServiceArrayLocal = asyncResp->res.jsonValue["Members"]; nlohmann::json::object_t member; member["@odata.id"] = boost::urls::format( "redfish/v1/Systems/{}/LogServices/HardwareIsolation", BMCWEB_REDFISH_SYSTEM_URI_NAME); logServiceArrayLocal.emplace_back(std::move(member)); asyncResp->res.jsonValue["[email protected]"] = logServiceArrayLocal.size(); }

jeaaustx · 2025-05-08T18:42:07Z

redfish-core/include/utils/hw_isolation.hpp

+    crow::connections::systemBus->async_method_call(
+        [asyncResp, resourceObjPath](
+            const boost::system::error_code& ec,
+            const dbus::utility::MapperGetAncestorsResponse& ancestors) {


I realize this code is being carried forward from prior releases. But in its current format it goes against the OpenBMC coding standards. Specifically using long lambdas. Suggest cleaning this up to move into a named function instead.

(Same comment applies throughout this file and much of this review.)

jeaaustx · 2025-05-08T18:43:38Z

redfish-core/include/utils/hw_isolation.hpp

+                // for parent chassis object id alone, so we should get one
+                // element.
+                BMCWEB_LOG_ERROR(
+                    "The given resource object [{}] is contains more than one Chassis as parent so failed return ChassisPowerStateOffRequiredError in the response",


extraneous word: "is"

jeaaustx · 2025-05-08T20:18:42Z

redfish-core/include/utils/hw_isolation.hpp

+    if (severityVal ==
+        "xyz.openbmc_project.Logging.Event.SeverityLevel.Critical")
+    {
+        asyncResp->res.jsonValue[severityPropPath] = "Critical";


Should these strings map to generated Redfish enums?

jeaaustx · 2025-05-08T20:26:49Z

redfish-core/include/utils/hw_isolation.hpp

+        // MessageRegistries
+        std::vector<std::string> messageArgs{*msgPropVal};
+
+        // Fill the "msgPropVal" as reason


Can this use the utility function fillMessageArgs() instead?

jeaaustx · 2025-05-08T20:47:32Z

redfish-core/lib/memory.hpp

+ * @brief API used to process the Memory "Enabled" member which is
+ *        patched to do appropriate action.
+ *
+ * @param[in] resp - The redfish response to return.


nit: comment doesn't match name of parameter

jeaaustx · 2025-05-08T21:21:47Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+    crow::connections::systemBus->async_method_call(
+        [asyncResp, dbusObjPath,
+         entryJsonIdx](const boost::system::error_code& ec,
+                       const dbus::utility::MapperGetObject& objType) {


This is a really long lambda. It would be better to put it into a named function.

jeaaustx · 2025-05-08T21:23:19Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+            }
+
+            // Fill the isolated hardware object id along with the Redfish URI
+            std::string redfishUri =


Better to use std::format() here.

jeaaustx · 2025-05-08T21:30:46Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+                        severityString ==
+                            "xyz.openbmc_project.HardwareIsolation.Entry.Type.Spare")
+                    {
+                        entryJson["Severity"] = "OK";


Could this use a generated Redifsh enum?

jeaaustx · 2025-05-08T21:35:11Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+    crow::connections::systemBus->async_method_call(
+        [asyncResp, entryId,
+         entryObjPath](const boost::system::error_code& ec,
+                       const dbus::utility::MapperGetObject& objType) {


This file also has lots of long lambdas which would benefit the readability to move into named functions per the coding guidelines.

jeaaustx · 2025-05-08T21:37:31Z

redfish-core/lib/systems_logservices_hwisolation.hpp

+        .methods(boost::beast::http::verb::get)(
+            getSystemHardwareIsolationLogEntryCollection);
+
+    BMCWEB_ROUTE(app, "/redfish/v1/Systems/system/LogServices/"


It would be better for searching the code if the Redfish URI path string was all in a single string. Same comment below.

gtmills requested review from gtmills, baemyung and jeaaustx May 8, 2025 13:58

baemyung reviewed May 8, 2025

View reviewed changes

jeaaustx reviewed May 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: hw-isolation rebase: Added guard, deconfiguration, sparecore #1279

WIP: hw-isolation rebase: Added guard, deconfiguration, sparecore #1279

gcpin commented May 7, 2025

jenkins-openbmc-ibm commented May 7, 2025

gtmills commented May 8, 2025

baemyung May 8, 2025

baemyung May 8, 2025

baemyung May 8, 2025

baemyung May 8, 2025

baemyung May 8, 2025

baemyung May 8, 2025

baemyung May 8, 2025

baemyung May 8, 2025

baemyung May 8, 2025

baemyung May 8, 2025

jeaaustx May 8, 2025

jeaaustx May 8, 2025

jeaaustx May 8, 2025

jeaaustx May 8, 2025

jeaaustx May 8, 2025

jeaaustx May 8, 2025

jeaaustx May 8, 2025

jeaaustx May 8, 2025

jeaaustx May 8, 2025

jeaaustx May 8, 2025

WIP: hw-isolation rebase: Added guard, deconfiguration, sparecore #1279

Are you sure you want to change the base?

WIP: hw-isolation rebase: Added guard, deconfiguration, sparecore #1279

Conversation

gcpin commented May 7, 2025

jenkins-openbmc-ibm commented May 7, 2025

gtmills commented May 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment