FeatureStore: Support Log Page 12h for feature discovery and filtering - Add support for reading Log Page 12h in NVMeMi. - Use Log Page 12h to filter supported features in FeatureStore. - Hardcode NSID to NONE for Log Page 12h queries. Design-Doc: go/bmc-redfish-nvme-get-set-features Google-Bug-Id: 513600176 Change-Id: I8fb1d414cd5aaac3bbcf5fe9e389f19a2598bc57 Signed-off-by: Guangzong Chen <guangzong@google.com>
diff --git a/src/NVMeFeatureStore.cpp b/src/NVMeFeatureStore.cpp index 0a2a0ed..1de3b85 100644 --- a/src/NVMeFeatureStore.cpp +++ b/src/NVMeFeatureStore.cpp
@@ -8,6 +8,7 @@ #include <boost/asio/post.hpp> #include <phosphor-logging/lg2.hpp> +#include <algorithm> #include <cstring> #include <iomanip> #include <map> @@ -143,6 +144,33 @@ yield, nvmeIntf, ctrl, cns, nsid, cntid); } +static std::tuple<std::error_code, std::vector<uint8_t>> asyncAdminGetLogPage( + boost::asio::io_context& io, const std::shared_ptr<NVMeMiIntf>& nvmeIntf, + nvme_mi_ctrl_t ctrl, nvme_cmd_get_log_lid lid, uint32_t nsid, uint8_t lsp, + uint16_t lsi, boost::asio::yield_context yield) +{ + return boost::asio::async_initiate<boost::asio::yield_context, + void(std::error_code, + std::vector<uint8_t>)>( + [&io](auto&& callback, const std::shared_ptr<NVMeMiIntf>& nvmeIntf, + nvme_mi_ctrl_t ctrl, nvme_cmd_get_log_lid lid, uint32_t nsid, + uint8_t lsp, uint16_t lsi) { + auto sharedCb = std::make_shared<std::decay_t<decltype(callback)>>( + std::forward<decltype(callback)>(callback)); + nvmeIntf->adminGetLogPage( + ctrl, lid, nsid, lsp, lsi, + [sharedCb, &io](const std::error_code& ec, + std::span<uint8_t> data) mutable { + std::vector<uint8_t> dataCopy(data.begin(), data.end()); + boost::asio::post(io, [sharedCb = std::move(sharedCb), ec, + dataCopy = std::move(dataCopy)]() mutable { + (*sharedCb)(ec, dataCopy); + }); + }); + }, + yield, nvmeIntf, ctrl, lid, nsid, lsp, lsi); +} + FeatureStore::FeatureStore( boost::asio::io_context& io, sdbusplus::asio::object_server& objServer, const std::shared_ptr<sdbusplus::asio::connection>& conn, @@ -163,19 +191,19 @@ sdbusplus::asio::PropertyPermission::readOnly); dbusIntf->register_method( "GetFeature", - [weakSelf = weak_from_this()](boost::asio::yield_context yield, + [weakSelf = weak_from_this()](const boost::asio::yield_context& yield, const std::string& featureName) { auto self = weakSelf.lock(); if (!self) { throw sdbusplus::xyz::openbmc_project::Common::Error::Unavailable(); } - return self->getFeature(std::move(yield), featureName); + return self->getFeature(yield, featureName); }); dbusIntf->register_method( "SetFeature", - [weakSelf = weak_from_this()](boost::asio::yield_context yield, + [weakSelf = weak_from_this()](const boost::asio::yield_context& yield, const std::string& featureName, const std::vector<uint8_t>& data) { auto self = weakSelf.lock(); @@ -183,7 +211,7 @@ { throw sdbusplus::xyz::openbmc_project::Common::Error::Unavailable(); } - self->setFeature(std::move(yield), featureName, data); + self->setFeature(yield, featureName, data); }); dbusIntf->initialize(); @@ -252,8 +280,57 @@ } } + // Fetch Log Page 12h (Feature Identifiers Supported and Effects) + std::vector<uint8_t> fidSupportedEffects; + auto [ec, logData] = asyncAdminGetLogPage( + io, nvmeIntf, ctrl, NVME_LOG_LID_FID_SUPPORTED_EFFECTS, NVME_NSID_NONE, + 0, 0, yield); + if (!ec) + { + if (logData.size() == sizeof(struct nvme_fid_supported_effects_log)) + { + fidSupportedEffects = std::move(logData); + } + else + { + lg2::error( + "Log Page 12h size mismatch: expected {EXPECTED}, got {GOT}", + "EXPECTED", sizeof(struct nvme_fid_supported_effects_log), + "GOT", logData.size()); + } + } + else + { + lg2::error("Failed to fetch Log Page 12h: {ERROR}", "ERROR", + ec.message()); + } + for (const auto& [name, meta] : featuresToCheck) { + if (!fidSupportedEffects.empty()) + { + size_t offset = static_cast<size_t>(meta.fid) * 4; + if (offset + sizeof(uint32_t) <= fidSupportedEffects.size()) + { + uint32_t entry = 0; + std::memcpy(&entry, fidSupportedEffects.data() + offset, + sizeof(entry)); + entry = le32toh(entry); + if ((entry & 0x1) == 0) // FSUPP bit + { + lg2::info( + "Feature {NAME} (FID {FID}) not supported per Log Page 12h", + "NAME", name, "FID", meta.fid); + continue; + } + } + else + { + lg2::error("Log Page 12h data too short for FID {FID}", "FID", + meta.fid); + continue; + } + } std::stringstream ss; ss << "0x" << std::setfill('0') << std::setw(2) << std::hex << static_cast<int>(meta.fid); @@ -305,9 +382,22 @@ } } -std::vector<uint8_t> FeatureStore::getFeature(boost::asio::yield_context yield, - const std::string& featureName) +bool FeatureStore::isFeatureSupported(const std::string& featureName) const { + return std::any_of(featureCollection.begin(), featureCollection.end(), + [&featureName](const auto& item) { + return std::get<0>(item) == featureName; + }); +} + +std::vector<uint8_t> + FeatureStore::getFeature(const boost::asio::yield_context& yield, + const std::string& featureName) +{ + if (!isFeatureSupported(featureName)) + { + throw sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument(); + } size_t lastUnderscore = featureName.find_last_of('_'); if (lastUnderscore == std::string::npos) { @@ -350,7 +440,7 @@ args.data_len = 0; auto [ex, cdw0, data] = asyncAdminGetFeatures(io, nvmeIntf, ctrl, args, - std::move(yield)); + yield); if (ex) { @@ -514,10 +604,14 @@ return protoData; } -void FeatureStore::setFeature(boost::asio::yield_context yield, +void FeatureStore::setFeature(const boost::asio::yield_context& yield, const std::string& featureName, const std::vector<uint8_t>& data) { + if (!isFeatureSupported(featureName)) + { + throw sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument(); + } size_t lastUnderscore = featureName.find_last_of('_'); if (lastUnderscore == std::string::npos) { @@ -712,8 +806,7 @@ args.nsid = (scope == FeatureScope::Namespace) ? nsid : NVME_NSID_NONE; args.data = {}; - auto [ex, resp] = asyncAdminSetFeatures(io, nvmeIntf, ctrl, args, - std::move(yield)); + auto [ex, resp] = asyncAdminSetFeatures(io, nvmeIntf, ctrl, args, yield); if (ex) {
diff --git a/src/NVMeFeatureStore.hpp b/src/NVMeFeatureStore.hpp index a513a73..3068696 100644 --- a/src/NVMeFeatureStore.hpp +++ b/src/NVMeFeatureStore.hpp
@@ -66,10 +66,11 @@ void fetchIdentifyData(const boost::asio::yield_context& yield); void registerDbusInterface(); + bool isFeatureSupported(const std::string& featureName) const; - std::vector<uint8_t> getFeature(boost::asio::yield_context yield, + std::vector<uint8_t> getFeature(const boost::asio::yield_context& yield, const std::string& featureName); - void setFeature(boost::asio::yield_context yield, + void setFeature(const boost::asio::yield_context& yield, const std::string& featureName, const std::vector<uint8_t>& data); };
diff --git a/src/NVMeMi.cpp b/src/NVMeMi.cpp index 195ef1c..41d8c86 100644 --- a/src/NVMeMi.cpp +++ b/src/NVMeMi.cpp
@@ -1333,6 +1333,26 @@ } } break; + case NVME_LOG_LID_FID_SUPPORTED_EFFECTS: + { + data.resize(sizeof(struct nvme_fid_supported_effects_log)); + struct nvme_get_log_args args = {}; + args.args_size = sizeof(args); + args.lid = NVME_LOG_LID_FID_SUPPORTED_EFFECTS; + args.nsid = NVME_NSID_NONE; + args.lsp = lsp; + args.len = data.size(); + args.log = data.data(); + + rc = nvme_mi_admin_get_log(ctrl, &args); + if (rc != 0) + { + lg2::error("fail to get FID supported and effects log", + "ENDPOINT", ep->describe()); + break; + } + } + break; case NVME_LOG_LID_DEVICE_SELF_TEST: { data.resize(sizeof(nvme_self_test_log));
diff --git a/src/NVMeMiFake.hpp b/src/NVMeMiFake.hpp index 6b29101..2723cc4 100644 --- a/src/NVMeMiFake.hpp +++ b/src/NVMeMiFake.hpp
@@ -1,3 +1,4 @@ +#include "NVMeError.hpp" #include "NVMeIntf.hpp" #include <boost/asio.hpp> @@ -590,7 +591,7 @@ { lg2::info("do post"); workerIsNotified = true; - workerIO.post(std::move(func)); + boost::asio::post(workerIO, std::move(func)); workerCv.notify_all(); return; }
diff --git a/tests/test_nvme_feature_store.cpp b/tests/test_nvme_feature_store.cpp index 7c101bf..3e1bd5c 100644 --- a/tests/test_nvme_feature_store.cpp +++ b/tests/test_nvme_feature_store.cpp
@@ -126,18 +126,18 @@ public: static std::vector<uint8_t> callGetFeature(const std::shared_ptr<FeatureStore>& fs, - boost::asio::yield_context yield, + const boost::asio::yield_context& yield, const std::string& featureName) { - return fs->getFeature(std::move(yield), featureName); + return fs->getFeature(yield, featureName); } static void callSetFeature(const std::shared_ptr<FeatureStore>& fs, - boost::asio::yield_context yield, + const boost::asio::yield_context& yield, const std::string& featureName, const std::vector<uint8_t>& data) { - fs->setFeature(std::move(yield), featureName, data); + fs->setFeature(yield, featureName, data); } static std::map<uint8_t, uint32_t>& @@ -145,6 +145,14 @@ { return fs->featureCaps; } + + static void addSupportedFeature(const std::shared_ptr<FeatureStore>& fs, + const std::string& name, + const std::string& fid, + const std::string& protoClass) + { + fs->featureCollection.emplace_back(name, fid, protoClass); + } }; TEST_F(FeatureStoreTest, GetFeatureArbitration) @@ -161,6 +169,10 @@ io, objServer, conn, objectPath, mockNvme, ctrl, 0, FeatureScope::Controller); + FeatureStoreTest::addSupportedFeature(featureStore, "Arbitration_Current", + "0x01", + "google.gbmc.nvme.base.Arbitration"); + EXPECT_CALL(*mockNvme, adminGetFeatures(::testing::_, ::testing::_, ::testing::_)) .WillOnce( @@ -173,9 +185,9 @@ std::vector<uint8_t> result; bool done = false; - boost::asio::spawn(io, [&](boost::asio::yield_context yield) { - result = FeatureStoreTest::callGetFeature( - featureStore, std::move(yield), "Arbitration_Current"); + boost::asio::spawn(io, [&](const boost::asio::yield_context& yield) { + result = FeatureStoreTest::callGetFeature(featureStore, yield, + "Arbitration_Current"); done = true; }); while (!done) @@ -207,6 +219,10 @@ io, objServer, conn, objectPath, mockNvme, ctrl, 0, FeatureScope::Controller); + FeatureStoreTest::addSupportedFeature(featureStore, "Arbitration_Current", + "0x01", + "google.gbmc.nvme.base.Arbitration"); + google::gbmc::nvme::base::Arbitration msg; msg.set_arbitration_burst(0x08); msg.set_low_priority_weight(0x07); @@ -227,8 +243,8 @@ }); bool done = false; - boost::asio::spawn(io, [&](boost::asio::yield_context yield) { - FeatureStoreTest::callSetFeature(featureStore, std::move(yield), + boost::asio::spawn(io, [&](const boost::asio::yield_context& yield) { + FeatureStoreTest::callSetFeature(featureStore, yield, "Arbitration_Current", data); done = true; }); @@ -290,6 +306,27 @@ } }); + // Mock Get Log Page for Log Page 12h + EXPECT_CALL(*mockNvme, adminGetLogPage(::testing::_, + NVME_LOG_LID_FID_SUPPORTED_EFFECTS, + ::testing::_, ::testing::_, + ::testing::_, ::testing::_)) + .WillOnce( + [](nvme_mi_ctrl_t, nvme_cmd_get_log_lid, uint32_t, uint8_t, + uint16_t, + std::function<void(const std::error_code&, std::span<uint8_t>)>&& + cb) { + std::vector<uint8_t> data(sizeof(struct nvme_fid_supported_effects_log), + 0); + std::vector<uint8_t> fids = {0x01, 0x02, 0x04, 0x05, 0x06, + 0x07, 0x08, 0x09, 0x0A, 0x0B}; + for (uint8_t fid : fids) + { + data[static_cast<size_t>(fid) * 4] = 0x1; + } + cb(std::error_code(), data); + }); + bool done = false; featureStore->init([&done](const std::error_code& ec) { EXPECT_FALSE(ec); @@ -328,10 +365,10 @@ bool threw = false; bool done = false; - boost::asio::spawn(io, [&](boost::asio::yield_context yield) { + boost::asio::spawn(io, [&](const boost::asio::yield_context& yield) { try { - FeatureStoreTest::callGetFeature(featureStore, std::move(yield), + FeatureStoreTest::callGetFeature(featureStore, yield, "InvalidFeature_Current"); } catch (const sdbusplus::xyz::openbmc_project::Common::Error:: @@ -362,6 +399,10 @@ io, objServer, conn, objectPath, mockNvme, ctrl, 0, FeatureScope::Controller); + FeatureStoreTest::addSupportedFeature(featureStore, "Arbitration_Current", + "0x01", + "google.gbmc.nvme.base.Arbitration"); + EXPECT_CALL(*mockNvme, adminGetFeatures(::testing::_, ::testing::_, ::testing::_)) .WillOnce( @@ -375,10 +416,10 @@ bool threw = false; bool done = false; - boost::asio::spawn(io, [&](boost::asio::yield_context yield) { + boost::asio::spawn(io, [&](const boost::asio::yield_context& yield) { try { - FeatureStoreTest::callGetFeature(featureStore, std::move(yield), + FeatureStoreTest::callGetFeature(featureStore, yield, "Arbitration_Current"); } catch ( @@ -411,10 +452,10 @@ bool threw = false; bool done = false; - boost::asio::spawn(io, [&](boost::asio::yield_context yield) { + boost::asio::spawn(io, [&](const boost::asio::yield_context& yield) { try { - FeatureStoreTest::callSetFeature(featureStore, std::move(yield), + FeatureStoreTest::callSetFeature(featureStore, yield, "Arbitration_InvalidSelect", {}); } catch (const sdbusplus::xyz::openbmc_project::Common::Error:: @@ -445,16 +486,19 @@ io, objServer, conn, objectPath, mockNvme, ctrl, 0, FeatureScope::Controller); + FeatureStoreTest::addSupportedFeature(featureStore, "Arbitration_Current", + "0x01", + "google.gbmc.nvme.base.Arbitration"); + std::vector<uint8_t> malformedData = {0xFF, 0xFF, 0xFF}; bool threw = false; bool done = false; - boost::asio::spawn(io, [&](boost::asio::yield_context yield) { + boost::asio::spawn(io, [&](const boost::asio::yield_context& yield) { try { - FeatureStoreTest::callSetFeature(featureStore, std::move(yield), - "Arbitration_Current", - malformedData); + FeatureStoreTest::callSetFeature( + featureStore, yield, "Arbitration_Current", malformedData); } catch (const sdbusplus::xyz::openbmc_project::Common::Error:: InvalidArgument&) @@ -484,6 +528,10 @@ io, objServer, conn, objectPath, mockNvme, ctrl, 0, FeatureScope::Controller); + FeatureStoreTest::addSupportedFeature(featureStore, "Arbitration_Current", + "0x01", + "google.gbmc.nvme.base.Arbitration"); + google::gbmc::nvme::base::Arbitration msg; msg.set_arbitration_burst(256); // Exceeds 8-bit bound (0-255) @@ -493,10 +541,10 @@ bool threw = false; bool done = false; - boost::asio::spawn(io, [&](boost::asio::yield_context yield) { + boost::asio::spawn(io, [&](const boost::asio::yield_context& yield) { try { - FeatureStoreTest::callSetFeature(featureStore, std::move(yield), + FeatureStoreTest::callSetFeature(featureStore, yield, "Arbitration_Current", data); } catch (const sdbusplus::xyz::openbmc_project::Common::Error:: @@ -527,6 +575,13 @@ io, objServer, conn, objectPath, mockNvme, ctrl, 0, FeatureScope::Controller); + FeatureStoreTest::addSupportedFeature(featureStore, "Arbitration_Current", + "0x01", + "google.gbmc.nvme.base.Arbitration"); + FeatureStoreTest::addSupportedFeature(featureStore, "Arbitration_Saved", + "0x01", + "google.gbmc.nvme.base.Arbitration"); + // Seed featureCaps for Arbitration (FID 0x01) with 0 (neither changeable // nor saveable) FeatureStoreTest::getFeatureCaps(featureStore)[0x01] = 0; @@ -540,10 +595,10 @@ // 1. Try to set Current (changeable=false) -> should throw NotAllowed bool threwNotAllowedCurrent = false; bool doneCurrent = false; - boost::asio::spawn(io, [&](boost::asio::yield_context yield) { + boost::asio::spawn(io, [&](const boost::asio::yield_context& yield) { try { - FeatureStoreTest::callSetFeature(featureStore, std::move(yield), + FeatureStoreTest::callSetFeature(featureStore, yield, "Arbitration_Current", data); } catch ( @@ -563,10 +618,10 @@ io.restart(); bool threwNotAllowedSaved = false; bool doneSaved = false; - boost::asio::spawn(io, [&](boost::asio::yield_context yield) { + boost::asio::spawn(io, [&](const boost::asio::yield_context& yield) { try { - FeatureStoreTest::callSetFeature(featureStore, std::move(yield), + FeatureStoreTest::callSetFeature(featureStore, yield, "Arbitration_Saved", data); } catch (
diff --git a/tests/test_nvme_mi.cpp b/tests/test_nvme_mi.cpp index 6025d4b..363116d 100644 --- a/tests/test_nvme_mi.cpp +++ b/tests/test_nvme_mi.cpp
@@ -223,14 +223,6 @@ systemBus->request_name("xyz.openbmc_project.NVMeTest"); subsys->unavailableMaxCount = 1; subsys->pollingInterval = subsysPollTime; - - ON_CALL(mock, adminGetFeatures) - .WillByDefault([](nvme_mi_ctrl_t, - const NVMeMiIntf::GetFeaturesRequest&, - std::function<void(nvme_ex_ptr, uint32_t, - std::span<uint8_t>)>&& cb) { - cb(nullptr, 0, {}); - }); } static void SetUpTestSuite() @@ -759,6 +751,7 @@ }); io.run(); } + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv);