smbios-mdr: Add assoc-trim-path option and Board match
When a custom object path is being used (not the default object path
that comes from IPMI), allow the Board interface to be a valid
interface for associating the Inventory object with, not just the
System interface.
The purpose is to support large systems of the blade server style,
which have CPU and DIMM attached to a Board that is itself attached to
a System, not attached to the System directly.
Adding the assoc-trim-path option, disabled by default, to drop the
rightmost path component when setting up this association. This
bypasses the Board and associates the CPU and DIMM directly with the
underlying System.
Cleaning up the systemInfoUpdate() function, to correct a logic error
in which the match rule would not be set up for later if an exception
happened the first time. Also sleeping, to work around a race condition
with Object Mapper.
Tested: See this pastebin for run on "platform10" and "platform11"
https://paste.googleplex.com/5818376734113792
Patch Tracking Bug: b/322729266
Upstream info / review: https://gerrit.openbmc.org/c/openbmc/smbios-mdr/+/69054
Upstream-Status: Submitted
Justification: Awaiting upstream review
Google-Bug-Id: 312510295
Change-Id: Ic9ae280babae137ad916ae9cf193e5a340b95380
Signed-off-by: Josh Lehan <krellan@google.com>
diff --git a/recipes-phosphor/smbios/smbios-mdr/0001-Add-assoc-trim-path-option-and-Board-match.patch b/recipes-phosphor/smbios/smbios-mdr/0001-Add-assoc-trim-path-option-and-Board-match.patch
new file mode 100644
index 0000000..304d4e2
--- /dev/null
+++ b/recipes-phosphor/smbios/smbios-mdr/0001-Add-assoc-trim-path-option-and-Board-match.patch
@@ -0,0 +1,220 @@
+From ef027c7d8a8d1546e8af655acd20a9dc74c21b3c Mon Sep 17 00:00:00 2001
+From: Josh Lehan <krellan@google.com>
+Date: Fri, 26 Jan 2024 15:49:50 -0800
+Subject: [PATCH] Add assoc-trim-path option and Board match
+
+When a custom object path is being used (not the default object path
+that comes from IPMI), allow the Board interface to be a valid
+interface for associating the Inventory object with, not just the
+System interface.
+
+The purpose is to support large systems of the blade server style,
+which have CPU and DIMM attached to a Board that is itself attached to
+a System, not attached to the System directly.
+
+Adding the assoc-trim-path option, disabled by default, to drop the
+rightmost path component when setting up this association. This
+bypasses the Board and associates the CPU and DIMM directly with the
+underlying System.
+
+Cleaning up the systemInfoUpdate() function, to correct a logic error
+in which the match rule would not be set up for later if an exception
+happened the first time. Also sleeping, to work around a race condition
+with Object Mapper.
+
+Signed-off-by: Josh Lehan <krellan@google.com>
+---
+ include/mdrv2.hpp | 2 +
+ meson.options | 7 +++
+ src/mdrv2.cpp | 114 +++++++++++++++++++++++++++++++---------------
+ src/meson.build | 4 ++
+ 4 files changed, 90 insertions(+), 37 deletions(-)
+
+diff --git a/include/mdrv2.hpp b/include/mdrv2.hpp
+index 743fbb8..fe184b1 100644
+--- a/include/mdrv2.hpp
++++ b/include/mdrv2.hpp
+@@ -63,6 +63,8 @@ static constexpr const char* defaultInventoryPath =
+ "/xyz/openbmc_project/inventory/system";
+ static constexpr const char* systemInterface =
+ "xyz.openbmc_project.Inventory.Item.System";
++static constexpr const char* boardInterface =
++ "xyz.openbmc_project.Inventory.Item.Board";
+ constexpr const int limitEntryLen = 0xff;
+
+ // Avoid putting multiple interfaces with same name on same object
+diff --git a/meson.options b/meson.options
+index a9f640b..0fc5c2d 100644
+--- a/meson.options
++++ b/meson.options
+@@ -19,6 +19,13 @@ option(
+ description: 'Only use the DIMM number, and not the bank'
+ )
+
++option(
++ 'assoc-trim-path',
++ type: 'feature',
++ value: 'disabled',
++ description: 'Trim one object path component from CPU and DIMM associations'
++)
++
+ option(
+ 'cpuinfo',
+ type: 'feature',
+diff --git a/src/mdrv2.cpp b/src/mdrv2.cpp
+index 52f66fe..0a4323e 100644
+--- a/src/mdrv2.cpp
++++ b/src/mdrv2.cpp
+@@ -415,7 +415,14 @@ void MDRV2::systemInfoUpdate()
+ mapperInterface, "GetSubTreePaths");
+ method.append(mapperAncestorPath);
+ method.append(0);
+- method.append(std::vector<std::string>({systemInterface}));
++
++ // If customized, also accept Board as anchor, not just System
++ std::vector<std::string> desiredInterfaces{systemInterface};
++ if (requireExactMatch)
++ {
++ desiredInterfaces.emplace_back(boardInterface);
++ }
++ method.append(desiredInterfaces);
+
+ try
+ {
+@@ -434,42 +441,6 @@ void MDRV2::systemInfoUpdate()
+ motherboardPath = std::move(paths[i]);
+ break;
+ }
+-
+- if (motherboardPath.empty())
+- {
+- phosphor::logging::log<phosphor::logging::level::ERR>(
+- "Failed to get system motherboard dbus path. Setting up a "
+- "match rule");
+-
+- if (!motherboardConfigMatch)
+- {
+- motherboardConfigMatch =
+- std::make_unique<sdbusplus::bus::match_t>(
+- *bus,
+- sdbusplus::bus::match::rules::interfacesAdded() +
+- sdbusplus::bus::match::rules::argNpath(
+- 0, matchParentPath),
+- [this](sdbusplus::message_t& msg) {
+- sdbusplus::message::object_path objectName;
+- boost::container::flat_map<
+- std::string,
+- boost::container::flat_map<
+- std::string, std::variant<std::string, uint64_t>>>
+- msgData;
+- msg.read(objectName, msgData);
+- if (msgData.contains(systemInterface))
+- {
+- systemInfoUpdate();
+- }
+- });
+- }
+- }
+- else
+- {
+- lg2::info(
+- "Found Inventory anchor object for SMBIOS content {I}: {M}",
+- "I", smbiosInventoryPath, "M", motherboardPath);
+- }
+ }
+ catch (const sdbusplus::exception_t& e)
+ {
+@@ -481,6 +452,75 @@ void MDRV2::systemInfoUpdate()
+ phosphor::logging::entry("ERROR=%s", e.what()));
+ }
+
++ if (motherboardPath.empty())
++ {
++ phosphor::logging::log<phosphor::logging::level::ERR>(
++ "Failed to get system motherboard dbus path. Setting up a "
++ "match rule");
++
++ if (motherboardConfigMatch)
++ {
++ lg2::info("Motherboard match rule already exists");
++ }
++ else
++ {
++ motherboardConfigMatch = std::make_unique<sdbusplus::bus::match_t>(
++ *bus,
++ sdbusplus::bus::match::rules::interfacesAdded() +
++ sdbusplus::bus::match::rules::argNpath(0, matchParentPath),
++ [this, requireExactMatch](sdbusplus::message_t& msg) {
++ sdbusplus::message::object_path objectName;
++ boost::container::flat_map<
++ std::string,
++ boost::container::flat_map<
++ std::string, std::variant<std::string, uint64_t>>>
++ msgData;
++ msg.read(objectName, msgData);
++ bool gotMatch = false;
++
++ if (msgData.contains(systemInterface))
++ {
++ lg2::info("Successful match on system interface");
++ gotMatch = true;
++ }
++
++ // If customized, also accept Board as anchor, not just System
++ if (requireExactMatch && msgData.contains(boardInterface))
++ {
++ lg2::info("Successful match on board interface");
++ gotMatch = true;
++ }
++
++ if (gotMatch)
++ {
++ // There is a race condition here: our desired interface
++ // has just been created, triggering the D-Bus callback,
++ // but Object Mapper has not been told of it yet. The
++ // mapper must also add it. Stall for time, so it can.
++ sleep(2);
++ systemInfoUpdate();
++ }
++ });
++ }
++ }
++ else
++ {
++ lg2::info("Found Inventory anchor object for SMBIOS content {I}: {M}",
++ "I", smbiosInventoryPath, "M", motherboardPath);
++ }
++
++#ifdef ASSOC_TRIM_PATH
++ // When enabled, chop off last component of motherboardPath, to trim one
++ // layer, so that associations are built to the underlying chassis itself,
++ // not the system boards in the chassis. This is for compatibility with
++ // traditional systems which only have one motherboard per chassis.
++ std::filesystem::path foundPath(motherboardPath);
++ motherboardPath = foundPath.parent_path().string();
++#endif
++
++ lg2::info("Using Inventory anchor object for SMBIOS content {I}: {M}", "I",
++ smbiosInventoryPath, "M", motherboardPath);
++
+ std::optional<size_t> num = getTotalCpuSlot();
+ if (!num)
+ {
+diff --git a/src/meson.build b/src/meson.build
+index d849cec..4c7fa48 100644
+--- a/src/meson.build
++++ b/src/meson.build
+@@ -3,6 +3,10 @@ if get_option('dimm-dbus').allowed()
+ cpp_args_smbios += ['-DDIMM_DBUS']
+ endif
+
++if get_option('assoc-trim-path').allowed()
++ cpp_args_smbios += ['-DASSOC_TRIM_PATH']
++endif
++
+ if get_option('dimm-only-locator').allowed()
+ cpp_args_smbios += ['-DDIMM_ONLY_LOCATOR']
+ endif
+--
+2.43.0.429.g432eaa2c6b-goog
+
diff --git a/recipes-phosphor/smbios/smbios-mdr_%.bbappend b/recipes-phosphor/smbios/smbios-mdr_%.bbappend
index 8777a9f..85425c4 100644
--- a/recipes-phosphor/smbios/smbios-mdr_%.bbappend
+++ b/recipes-phosphor/smbios/smbios-mdr_%.bbappend
@@ -11,6 +11,7 @@
file://0010-Corrected-file-format-of-gRPC-cache-file.patch\
file://0011-Completed-implementation-of-gRPC-blobs-handler.patch\
file://0001-Add-support-for-google-oem-cpu-link-structure.patch\
+ file://0001-Add-assoc-trim-path-option-and-Board-match.patch\
"
EXTRA_OEMESON:append:gbmc = " -Ddimm-only-locator=enabled"
@@ -18,4 +19,7 @@
PACKAGECONFIG[smbios-grpc-blob] = "-Dsmbios-grpc-blob=enabled,-Dsmbios-grpc-blob=disabled,fmt grpc grpc-native"
PACKAGECONFIG[google-cpu-link] = "-Dgoogle-cpu-link=enabled,-Dgoogle-cpu-link=disabled"
+# TODO(krellan): Remove this when go/obmccl/69055 merged upstream
+PACKAGECONFIG[assoc-trim-path] = "-Dassoc-trim-path=enabled,-Dassoc-trim-path=disabled"
+
PACKAGECONFIG:remove:gbmc = " cpuinfo"
