nvmed: Adds support for MCTP devices exposed by mctpreactor
Adds a new class, MctpReactorDevice that inherits from MctpDevice and
is specific to the way mctpreactor sets things up.
To get an endpoint ID, MctpReactorDevice queries the "configures"
association in the ObjectMapper, that associates the NVMe device object
with a NVME1000 interface and the MCTP endpoint object.
Additionally, NVMeMi was changed to skip MTU and SMBus frequency setup
for devices that aren't directly accessible over I2C, which is
recognized through the absence of the `bus` parameter in those
devices' JSON configuration files for EntityManager.
Tested: Test were done on a lab machine that has an E1.S SSD
connected to BMC through a USB <-> I2C MCTP bridge.
nvmesensor log:
https://paste.googleplex.com/5240477540548608
Another test on the same machine was ran with mctpreactor
down and then started again after a few minutes:
https://paste.googleplex.com/5326693527060480
Roughly midway down that log periodic retrying can be
observed. A retry happens every 60-90 seconds. Estimate is
based on me looking at the log in realtime rather than
measuring or looking at the code.
Additionally tested on tjbm11 with the new and old
nvmesensor binary to see if there's regressions:
https://paste.googleplex.com/6389420236341248
Finally, tested on tmdja26 to see how does nvmed pick up
regular I2C devices through mctpreactor:
https://paste.googleplex.com/4860794563067904
Google-Bug-Id: 444680700
Change-Id: I3d01d7e6bd7ee6bd9455c0f17e498631cee18cb7
Signed-off-by: Luka Strizic <lstrz@google.com>
diff --git a/src/MctpEndpoint.cpp b/src/MctpEndpoint.cpp
index 3391e35..7ffcc27 100644
--- a/src/MctpEndpoint.cpp
+++ b/src/MctpEndpoint.cpp
@@ -18,8 +18,6 @@
"/au/com/codeconstruct/mctp1/interfaces/";
static constexpr const char* mctpdControlInterface =
"au.com.codeconstruct.MCTP.BusOwner1";
-static constexpr const char* mctpdEndpointControlInterface =
- "au.com.codeconstruct.MCTP.Endpoint1";
MctpdDevice::MctpdDevice(
const std::shared_ptr<sdbusplus::asio::connection>& connection,
@@ -62,7 +60,8 @@
std::bind_front(MctpdDevice::onEndpointInterfacesRemoved,
weak_from_this(), objpath));
endpoint = std::make_shared<MctpdEndpoint>(shared_from_this(), connection,
- objpath, network, eid);
+ objpath, network, eid,
+ /*isI2cAccessible=*/true);
action({}, endpoint);
}
@@ -133,9 +132,10 @@
MctpdEndpoint::MctpdEndpoint(
const std::shared_ptr<MctpDevice>& device,
const std::shared_ptr<sdbusplus::asio::connection>& connection,
- sdbusplus::message::object_path objpath, int network, uint8_t eid) :
+ sdbusplus::message::object_path objpath, const int network,
+ const uint8_t eid, const bool isI2cAccessible) :
device(device), connection(connection), objpath(std::move(objpath)),
- mctp{network, eid}
+ mctp{network, eid, isI2cAccessible}
{}
void MctpdEndpoint::onMctpEndpointChange(sdbusplus::message_t& msg)
@@ -195,6 +195,11 @@
return mctp.eid;
}
+bool MctpdEndpoint::isI2cAccessible() const
+{
+ return mctp.isI2cAccessible;
+}
+
void MctpdEndpoint::subscribe(Event&& degraded, Event&& available,
Event&& removed)
{
diff --git a/src/MctpEndpoint.hpp b/src/MctpEndpoint.hpp
index 770fcb6..ef39f99 100644
--- a/src/MctpEndpoint.hpp
+++ b/src/MctpEndpoint.hpp
@@ -7,6 +7,9 @@
#include <cstdint>
+constexpr const char* mctpdEndpointControlInterface =
+ "au.com.codeconstruct.MCTP.Endpoint1";
+
/**
* @file
* @brief Abstract and concrete classes representing MCTP concepts and
@@ -60,6 +63,11 @@
virtual uint8_t eid() const = 0;
/**
+ * @return Whether this MCTP endpoint is accessible over I2C or not.
+ */
+ virtual bool isI2cAccessible() const = 0;
+
+ /**
* @brief Subscribe to events produced by an endpoint object across its
* lifecycle
*
@@ -163,7 +171,7 @@
/**
* @brief An implementation of MctpEndpoint in terms of the D-Bus interfaces
- * exposed by @c mctpd.
+ * exposed by @c mctpd and @c mctpreactor.
*
* The lifetime of an MctpdEndpoint is proportional to the lifetime of the
* endpoint object exposed by @c mctpd. The lifecycle of @c mctpd endpoint
@@ -180,13 +188,15 @@
MctpdEndpoint(
const std::shared_ptr<MctpDevice>& device,
const std::shared_ptr<sdbusplus::asio::connection>& connection,
- sdbusplus::message::object_path objpath, int network, uint8_t eid);
+ sdbusplus::message::object_path objpath, int network, uint8_t eid,
+ bool isI2cAccessible);
MctpdEndpoint& McptdEndpoint(const MctpdEndpoint& other) = delete;
MctpdEndpoint(MctpdEndpoint&& other) noexcept = default;
~MctpdEndpoint() override = default;
int network() const override;
uint8_t eid() const override;
+ bool isI2cAccessible() const override;
void subscribe(Event&& degraded, Event&& available,
Event&& removed) override;
void setMtu(
@@ -215,6 +225,7 @@
{
int network;
uint8_t eid;
+ bool isI2cAccessible;
} mctp;
Event notifyAvailable;
Event notifyDegraded;
diff --git a/src/MctpReactorDevice.cpp b/src/MctpReactorDevice.cpp
new file mode 100644
index 0000000..7157894
--- /dev/null
+++ b/src/MctpReactorDevice.cpp
@@ -0,0 +1,240 @@
+#include "MctpReactorDevice.hpp"
+
+#include "MctpEndpoint.hpp"
+#include "Utils.hpp"
+
+#include <boost/algorithm/string.hpp>
+#include <boost/system/detail/errc.hpp>
+#include <sdbusplus/asio/connection.hpp>
+#include <sdbusplus/bus/match.hpp>
+#include <sdbusplus/exception.hpp>
+#include <sdbusplus/message.hpp>
+#include <sdbusplus/message/native_types.hpp>
+
+#include <exception>
+#include <limits>
+#include <memory>
+#include <optional>
+#include <string_view>
+#include <system_error>
+#include <variant>
+
+namespace
+{
+constexpr const char* objectMapperServiceName =
+ "xyz.openbmc_project.ObjectMapper";
+constexpr const char* configuresObjectPathSuffix = "/configures";
+constexpr const char* dbusPropertiesInterface =
+ "org.freedesktop.DBus.Properties";
+constexpr const char* associationInterface = "xyz.openbmc_project.Association";
+constexpr const char* endpointPropertyName = "endpoints";
+
+// Returns the token after the last '/' character.
+std::string extractSuffixFromObjectPath(const std::string& path)
+{
+ std::vector<std::string> tokens;
+ boost::split(tokens, path, boost::is_any_of("/"));
+ if (tokens.empty())
+ {
+ return "";
+ }
+ return tokens.back();
+}
+
+// Returns a pair consisting of the network number and endpoint ID parsed from
+// an MCTP endpoint object path such as
+// "/au/com/codeconstruct/mctp1/networks/1/endpoints/17" for example.
+// Assumes that the last number is EID and the first number is the network.
+std::optional<std::pair<int, uint8_t>>
+ extractNetworkAndEid(std::string_view endpointObjectPath)
+{
+ std::vector<std::string> tokens;
+ boost::split(tokens, endpointObjectPath, boost::is_any_of("/"));
+ auto it = tokens.rbegin();
+
+ // Find EID.
+ int eid = -1;
+ for (; it != tokens.rend(); ++it)
+ {
+ try
+ {
+ eid = std::stoi(*it);
+ }
+ catch (...)
+ {
+ continue;
+ }
+ break;
+ }
+ // Make sure the next (preceding) token is "enpdoints".
+ if (it == tokens.rend() || it + 1 == tokens.rend() ||
+ *(it + 1) != "endpoints")
+ {
+ return std::nullopt;
+ }
+ // Number validity check.
+ if (eid < std::numeric_limits<uint8_t>::min() ||
+ eid > std::numeric_limits<uint8_t>::max())
+ {
+ return std::nullopt;
+ }
+
+ // Find network.
+ int network = -1;
+ for (++it; it != tokens.rend(); ++it)
+ {
+ try
+ {
+ network = std::stoi(*it);
+ }
+ catch (...)
+ {
+ continue;
+ }
+ break;
+ }
+ // Make sure the next (preceding) token is "networks".
+ if (it == tokens.rend() || it + 1 == tokens.rend() ||
+ *(it + 1) != "networks")
+ {
+ return std::nullopt;
+ }
+ // Number validity check.
+ if (network < 0)
+ {
+ return std::nullopt;
+ }
+ return std::make_pair(network, eid);
+}
+
+} // namespace
+
+MctpReactorDevice::MctpReactorDevice(
+ const std::shared_ptr<sdbusplus::asio::connection>& connection,
+ const std::string& deviceObjectPath, std::optional<int> busNumber,
+ std::optional<int> address) :
+ connection(connection), deviceObjectPath(deviceObjectPath),
+ prettyObjectName(extractSuffixFromObjectPath(deviceObjectPath)),
+ isI2cAccessible(busNumber.has_value() && address.has_value()),
+ i2cBusNumber(isI2cAccessible ? busNumber.value() : -1),
+ i2cAddress(isI2cAccessible ? address.value() : -1)
+{}
+
+void MctpReactorDevice::setup(
+ std::function<void(const std::error_code& ec,
+ const std::shared_ptr<MctpEndpoint>& ep)>&& action)
+{
+ auto onGetPropertyReturned =
+ [weak{weak_from_this()}, action{std::move(action)}](
+ const boost::system::error_code& ec,
+ const std::variant<std::vector<std::string>>& value) mutable {
+ if (ec)
+ {
+ action(ec, {});
+ return;
+ }
+ const auto* const endpointObjectPaths =
+ std::get_if<std::vector<std::string>>(&value);
+ if (!endpointObjectPaths || endpointObjectPaths->size() != 1)
+ {
+ auto errc = std::errc::invalid_argument;
+ auto ec = std::make_error_code(errc);
+ action(ec, {});
+ return;
+ }
+
+ if (auto self = weak.lock())
+ {
+ self->finaliseEndpoint(endpointObjectPaths->front(),
+ std::move(action));
+ }
+ };
+ try
+ {
+ const std::string configuresObjectPath = deviceObjectPath +
+ configuresObjectPathSuffix;
+ connection->async_method_call(
+ onGetPropertyReturned, objectMapperServiceName,
+ configuresObjectPath, dbusPropertiesInterface, "Get",
+ std::string(associationInterface),
+ std::string(endpointPropertyName));
+ }
+ catch (const sdbusplus::exception::SdBusError& err)
+ {
+ auto errc = std::errc::no_such_device_or_address;
+ auto ec = std::make_error_code(errc);
+ action(ec, {});
+ }
+}
+
+void MctpReactorDevice::remove()
+{
+ if (endpoint)
+ {
+ endpoint->remove();
+ }
+}
+
+std::string MctpReactorDevice::describe() const
+{
+ return "MctpReactorDev: " + prettyObjectName;
+}
+
+void MctpReactorDevice::onEndpointInterfacesRemoved(
+ const std::weak_ptr<MctpReactorDevice>& weak,
+ const std::string& endpointObjectPath, sdbusplus::message_t& msg)
+{
+ auto objectPath = msg.unpack<sdbusplus::message::object_path>();
+ if (objectPath.str != endpointObjectPath)
+ {
+ return;
+ }
+
+ auto removedInterfaces = msg.unpack<std::set<std::string>>();
+ if (!removedInterfaces.contains(mctpdEndpointControlInterface))
+ {
+ return;
+ }
+
+ if (auto self = weak.lock())
+ {
+ self->endpointRemoved();
+ }
+}
+
+void MctpReactorDevice::finaliseEndpoint(
+ const std::string& endpointObjectPath,
+ std::function<void(const std::error_code& ec,
+ const std::shared_ptr<MctpEndpoint>& ep)>&& action)
+{
+ const auto matchSpec =
+ std::string(sdbusplus::bus::match::rules::interfacesRemoved())
+ .append(
+ sdbusplus::bus::match::rules::argNpath(0, endpointObjectPath));
+ removeMatch = std::make_unique<sdbusplus::bus::match_t>(
+ *connection, matchSpec,
+ std::bind_front(MctpReactorDevice::onEndpointInterfacesRemoved,
+ weak_from_this(), endpointObjectPath));
+
+ const auto networkAndEid = extractNetworkAndEid(endpointObjectPath);
+ if (!networkAndEid)
+ {
+ action(std::make_error_code(std::errc::invalid_argument), nullptr);
+ return;
+ }
+ const auto [network, eid] = networkAndEid.value();
+ endpoint = std::make_shared<MctpdEndpoint>(shared_from_this(), connection,
+ endpointObjectPath, network, eid,
+ isI2cAccessible);
+ action({}, endpoint);
+}
+
+void MctpReactorDevice::endpointRemoved()
+{
+ if (endpoint)
+ {
+ removeMatch.reset();
+ endpoint->remove();
+ endpoint.reset();
+ }
+}
diff --git a/src/MctpReactorDevice.hpp b/src/MctpReactorDevice.hpp
new file mode 100644
index 0000000..f506e98
--- /dev/null
+++ b/src/MctpReactorDevice.hpp
@@ -0,0 +1,58 @@
+#pragma once
+
+#include "MctpEndpoint.hpp"
+
+#include <sdbusplus/asio/connection.hpp>
+#include <sdbusplus/bus/match.hpp>
+#include <sdbusplus/message.hpp>
+
+/**
+ * @brief An implementation of MctpDevice in terms of D-Bus interfaces exposed
+ * by @c mctpreactor.
+ *
+ * The construction or destruction of an MctpReactorDevice is not required to be
+ * correlated with signals from @c mctpreactor. For instance, EntityManager may
+ * expose the existence of an MCTP-capable device through its usual
+ * configuration mechanisms.
+ */
+class MctpReactorDevice :
+ public MctpDevice,
+ public std::enable_shared_from_this<MctpReactorDevice>
+{
+ public:
+ MctpReactorDevice() = delete;
+ MctpReactorDevice(
+ const std::shared_ptr<sdbusplus::asio::connection>& connection,
+ const std::string& deviceObjectPath, std::optional<int> busNumber,
+ std::optional<int> address);
+ MctpReactorDevice(const MctpDevice& other) = delete;
+ MctpReactorDevice(MctpDevice&& other) = delete;
+ ~MctpReactorDevice() override = default;
+
+ void setup(std::function<void(const std::error_code& ec,
+ const std::shared_ptr<MctpEndpoint>& ep)>&&
+ action) override;
+ void remove() override;
+ std::string describe() const override;
+
+ private:
+ static void onEndpointInterfacesRemoved(
+ const std::weak_ptr<MctpReactorDevice>& weak,
+ const std::string& endpointObjectPath, sdbusplus::message_t& msg);
+
+ void finaliseEndpoint(
+ const std::string& endpointObjectPath,
+ std::function<void(const std::error_code& ec,
+ const std::shared_ptr<MctpEndpoint>& ep)>&& action);
+
+ void endpointRemoved();
+
+ std::shared_ptr<sdbusplus::asio::connection> connection;
+ const std::string deviceObjectPath;
+ const std::string prettyObjectName; // Suffix of deviceObjectPath.
+ const bool isI2cAccessible;
+ const int i2cBusNumber; // Invalid if isI2cAccessible is false.
+ const int i2cAddress; // Invalid if isI2cAccessible is false.
+ std::shared_ptr<MctpEndpoint> endpoint;
+ std::unique_ptr<sdbusplus::bus::match_t> removeMatch;
+};
diff --git a/src/NVMeMi.cpp b/src/NVMeMi.cpp
index 8b47e77..32162ae 100644
--- a/src/NVMeMi.cpp
+++ b/src/NVMeMi.cpp
@@ -151,6 +151,15 @@
case Status::Terminating:
throw std::logic_error("optimize called from Status::Terminating");
}
+
+ // If the device is behind a bridge (so not directly I2C accessible), we
+ // skip MTU and SMBus frequency configuration.
+ if (!endpoint->isI2cAccessible())
+ {
+ mctpStatus = Status::Connected;
+ return;
+ }
+
optimizeTimer = std::make_shared<boost::asio::steady_timer>(
io, std::chrono::milliseconds(500));
optimizeTimer->async_wait([this](boost::system::error_code ec) {
@@ -492,9 +501,8 @@
<< "] failed reading port info for port_id: "
<< unsigned(portId) << '\n';
}
- else if (portInfo.portt == 0x2)
+ else if (portInfo.portt == 0x2) // SMBus ports = 0x2
{
- // SMBus ports = 0x2
uint16_t supportedMtu = portInfo.mmctptus;
uint8_t supportedFreq = portInfo.smb.mme_freq; // NOLINT
self->io.post([self, portId, supportedMtu, supportedFreq,
diff --git a/src/NVMeSensorMain.cpp b/src/NVMeSensorMain.cpp
index e04dbf4..b3a32b5 100644
--- a/src/NVMeSensorMain.cpp
+++ b/src/NVMeSensorMain.cpp
@@ -15,6 +15,7 @@
*/
#include "MctpEndpoint.hpp"
+#include "MctpReactorDevice.hpp"
#include "NVMeBasic.hpp"
#include "NVMeIntf.hpp"
#include "NVMeMi.hpp"
@@ -50,7 +51,7 @@
// devices on the same bus. Though mctp kernel drive can schedule and
// sequencialize the transactions but assigning individual worker thread to
// each EP makes no sense.
-static std::map<int, std::weak_ptr<NVMeMiWorker>> workerMap{};
+static std::map<int, std::weak_ptr<NVMeMiWorker>> i2cWorkerMap{};
std::unordered_map<std::string, void*> pluginLibMap = {};
@@ -151,6 +152,17 @@
return std::get<std::string>(findProtocol->second);
}
+static std::optional<std::string>
+ extractMctpReactorConfigPath(const SensorBaseConfigMap& properties)
+{
+ auto findMctpReactorConfigPath = properties.find("MctpReactorConfigPath");
+ if (findMctpReactorConfigPath == properties.end())
+ {
+ return std::nullopt;
+ }
+ return std::get<std::string>(findMctpReactorConfigPath->second);
+}
+
static void
setupMctpDevice(const std::shared_ptr<MctpDevice>& dev,
const std::weak_ptr<NVMeMiIntf>& weakIntf,
@@ -247,7 +259,7 @@
* subsystem.
*/
std::map<std::string, NVMeDevice> updatedDevices;
- for (const auto& [interfacePath, configData] : nvmeConfigurations)
+ for (const auto& [nvmeObjectPath, configData] : nvmeConfigurations)
{
// find base configuration
auto sensorBase =
@@ -258,21 +270,41 @@
}
const SensorBaseConfigMap& sensorConfig = sensorBase->second;
- std::optional<int> busNumber = extractBusNumber(interfacePath,
+ std::optional<int> busNumber = extractBusNumber(nvmeObjectPath,
sensorConfig);
- std::optional<int> address = extractAddress(interfacePath,
+ std::optional<int> address = extractAddress(nvmeObjectPath,
sensorConfig);
- std::optional<std::string> sensorName = extractName(interfacePath,
+ std::optional<std::string> sensorName = extractName(nvmeObjectPath,
sensorConfig);
- std::optional<std::string> nvmeProtocol = extractProtocol(interfacePath,
- sensorConfig);
+ std::optional<std::string> nvmeProtocol =
+ extractProtocol(nvmeObjectPath, sensorConfig);
+ std::optional<std::string> mctpReactorConfigPath =
+ extractMctpReactorConfigPath(sensorConfig);
- if (!(busNumber && sensorName))
+ if (!sensorName)
{
continue;
}
- if (bannedBuses.contains(*busNumber))
+ const bool isMctpReactorDevice = mctpReactorConfigPath.has_value();
+
+ // Some NVMe devices are accessible over I2C because they're connected
+ // over SMBus directly to the BMC. In other cases, they might be
+ // connected to the BMC through a bridge (such as
+ // MCTP USB <-> I2C bridge).
+ const bool isI2cAccessible = busNumber.has_value();
+
+ // If a device is not an mctpreactor device, it must be accessible over
+ // I2C directly.
+ if (!isMctpReactorDevice && !isI2cAccessible)
+ {
+ std::cerr << "Found an mctpd device that isn't I2C accessible,"
+ "but it must be: "
+ << nvmeObjectPath.str << '\n';
+ continue;
+ }
+
+ if (busNumber && bannedBuses.contains(*busNumber))
{
std::cerr << "Skip banned i2c bus:" << *busNumber << '\n';
continue;
@@ -284,6 +316,15 @@
nvmeProtocol.emplace("mi_basic");
}
+ // Support for non-I2C-accessible devices doesn't exist for mi_basic
+ // protocol because we don't need it for now.
+ if (*nvmeProtocol == "mi_basic" && !isI2cAccessible)
+ {
+ std::cerr << "Skipping an mi_basic device that's not accessible "
+ "over i2c bus.\n";
+ continue;
+ }
+
if (*nvmeProtocol == "mi_basic")
{
// defualt i2c basic port is 0x6a
@@ -297,12 +338,12 @@
*address);
NVMeDevice dev{{}, nvmeBasic, {}};
- updatedDevices.emplace(interfacePath, std::move(dev));
+ updatedDevices.emplace(nvmeObjectPath, std::move(dev));
}
catch (std::exception& ex)
{
std::cerr << "Failed to add nvme basic interface for "
- << std::string(interfacePath) << ": " << ex.what()
+ << std::string(nvmeObjectPath) << ": " << ex.what()
<< "\n";
continue;
}
@@ -314,11 +355,10 @@
{
address.emplace(0x1d);
}
-
PowerState powerState = getPowerState(sensorConfig);
std::shared_ptr<NVMeMiWorker> worker;
- if (singleWorkerFeature)
+ if (singleWorkerFeature && isI2cAccessible)
{
auto root = deriveRootBus(*busNumber);
@@ -326,12 +366,12 @@
{
throw std::runtime_error("invalid root bus number");
}
- auto res = workerMap.find(*root);
+ auto res = i2cWorkerMap.find(*root);
- if (res == workerMap.end() || res->second.expired())
+ if (res == i2cWorkerMap.end() || res->second.expired())
{
worker = NVMeMiWorker::create(io);
- workerMap[*root] = worker;
+ i2cWorkerMap[*root] = worker;
}
else
{
@@ -345,8 +385,22 @@
try
{
- auto mctpDev = std::make_shared<SmbusMctpdDevice>(
- dbusConnection, *busNumber, *address);
+ // Note that in this codepath, despite using "mi_i2c" protocol,
+ // we can be talking to NVMe devices not accessible through I2C
+ // directly.
+ std::shared_ptr<MctpDevice> mctpDev;
+ if (!isMctpReactorDevice)
+ {
+ mctpDev = std::make_shared<SmbusMctpdDevice>(
+ dbusConnection, *busNumber, *address);
+ }
+ else // isMctpReactorDevice
+ {
+ mctpDev = std::make_shared<MctpReactorDevice>(
+ dbusConnection, *mctpReactorConfigPath, busNumber,
+ address);
+ }
+
NVMeIntf nvmeMi = NVMeIntf::create<NVMeMi>(
io, dbusConnection, mctpDev, worker, powerState);
@@ -354,13 +408,13 @@
nvmeMi.getInferface());
// Create a partial NVMeDevice entry in the temporary
// updatedDevices map
- NVMeDevice dev{mctpDev, nvmeMi, {}};
- updatedDevices.emplace(interfacePath, std::move(dev));
+ NVMeDevice nvmeDev{mctpDev, nvmeMi, {}};
+ updatedDevices.emplace(nvmeObjectPath, std::move(nvmeDev));
}
catch (std::exception& ex)
{
std::cerr << "Failed to add nvme mi interface for "
- << std::string(interfacePath) << ": " << ex.what()
+ << std::string(nvmeObjectPath) << ": " << ex.what()
<< "\n";
continue;
}
diff --git a/src/meson.build b/src/meson.build
index b7046fc..50e1500 100644
--- a/src/meson.build
+++ b/src/meson.build
@@ -194,6 +194,7 @@
if get_option('nvme').enabled()
nvme_srcs = files(
'MctpEndpoint.cpp',
+ 'MctpReactorDevice.cpp',
'NVMeSensorMain.cpp',
'NVMeSensor.cpp',
'NVMeBasic.cpp',
