commit b4c3bcd71888ef263c1561fd487fefa11d4a41cd
author Jonathan Doman <jonathan.doman@intel.com> Thu Mar 09 11:41:41 2023 -0800
committer Zhikui Ren <zhikui.ren@intel.com> Mon Mar 20 23:58:43 2023 +0000
tree eeded394fa57ceb44a35f617d30982595c802467
parent dd1811c9f7fc40f88b91d944eee4770d7c2f57cf

sst: Prefer cached values over default values

In anticipation of support for future CPUs, change some assumptions:
* When host is off, return cached property values instead of "default"
  values. CPUs may not guarantee support for level 0, so there is no
  universal default.
* Must always check the backend interface is ready before using it.
* Rename numLevels() to maxLevel() since there may be discontinuities in
  the supported levels.
* Also add some more debug prints.

Tested:
* On CPU that supports level 0 - verified that output of sst-info.sh
  script was the same before and after these changes.
* On a CPU that only supports level 4 - verified that when host was
  powered off, the AppliedConfig showed config4 (valid) instead of
  config0 (invalid).

Change-Id: Idffcb9a6534ba32760f6d6b2ac244f47427995ea
Signed-off-by: Jonathan Doman <jonathan.doman@intel.com>

src/speed_select.cpp

diff --git a/src/speed_select.cpp b/src/speed_select.cpp
index 24fb11d..56c0ec8 100644
--- a/src/speed_select.cpp
+++ b/src/speed_select.cpp
@@ -83,7 +83,7 @@
 std::unique_ptr<SSTInterface> getInstance(uint8_t address, CPUModel model)
 {
     DEBUG_PRINT << "Searching for provider for " << address << ", model "
-                << std::hex << model << '\n';
+                << std::hex << model << std::dec << '\n';
     for (const auto& provider : getProviders())
     {
         try
@@ -176,13 +176,11 @@
     sdbusplus::message::object_path appliedConfig() const override
     {
         DEBUG_PRINT << "Reading AppliedConfig\n";
-        // If CPU is powered off, return power-up default value of Level 0.
-        unsigned int level = 0;
         if (hostState != HostState::off)
         {
             // Otherwise, try to read current state
             auto sst = getInstance(peciAddress, cpuModel);
-            if (!sst)
+            if (!sst || !sst->ready())
             {
                 std::cerr << __func__
                           << ": Failed to get SST provider instance\n";
@@ -199,19 +197,17 @@
                               << "\n";
                 }
             }
-            level = currentLevel;
         }
-        return generateConfigPath(level);
+        return generateConfigPath(currentLevel);
     }
 
     bool baseSpeedPriorityEnabled() const override
     {
         DEBUG_PRINT << "Reading BaseSpeedPriorityEnabled\n";
-        bool enabled = false;
         if (hostState != HostState::off)
         {
             auto sst = getInstance(peciAddress, cpuModel);
-            if (!sst)
+            if (!sst || !sst->ready())
             {
                 std::cerr << __func__
                           << ": Failed to get SST provider instance\n";
@@ -228,9 +224,8 @@
                               << "\n";
                 }
             }
-            enabled = bfEnabled;
         }
-        return enabled;
+        return bfEnabled;
     }
 
     sdbusplus::message::object_path
@@ -332,14 +327,19 @@
                             OperatingConfig& config)
 {
     config.powerLimit(sst.tdp(level));
+    DEBUG_PRINT << " TDP = " << config.powerLimit() << '\n';
 
     config.availableCoreCount(sst.coreCount(level));
+    DEBUG_PRINT << " coreCount = " << config.availableCoreCount() << '\n';
 
     config.baseSpeed(sst.p1Freq(level));
+    DEBUG_PRINT << " baseSpeed = " << config.baseSpeed() << '\n';
 
     config.maxSpeed(sst.p0Freq(level));
+    DEBUG_PRINT << " maxSpeed = " << config.maxSpeed() << '\n';
 
     config.maxJunctionTemperature(sst.prochotTemp(level));
+    DEBUG_PRINT << " procHot = " << config.maxJunctionTemperature() << '\n';
 
     // Construct BaseSpeedPrioritySettings
     std::vector<std::tuple<uint32_t, std::vector<uint32_t>>> baseSpeeds;
@@ -451,16 +451,20 @@
 
         bool foundCurrentLevel = false;
 
-        for (unsigned int level = 0; level <= sst->numLevels(); ++level)
+        for (unsigned int level = 0; level <= sst->maxLevel(); ++level)
         {
+            DEBUG_PRINT << "checking level " << level << ": ";
             // levels 1 and 2 were legacy/deprecated, originally used for AVX
             // license pre-granting. They may be reused for more levels in
             // future generations. So we need to check for discontinuities.
             if (!sst->levelSupported(level))
             {
+                DEBUG_PRINT << "not supported\n";
                 continue;
             }
 
+            DEBUG_PRINT << "supported\n";
+
             getSingleConfig(*sst, level, cpu.newConfig(level));
 
             if (level == sst->currentLevel())
@@ -469,6 +473,8 @@
             }
         }
 
+        DEBUG_PRINT << "current level is " << sst->currentLevel() << '\n';
+
         if (!foundCurrentLevel)
         {
             // In case we didn't encounter a PECI error, but also didn't find

src/sst_mailbox.cpp

diff --git a/src/sst_mailbox.cpp b/src/sst_mailbox.cpp
index bd929d4..6024d6d 100644
--- a/src/sst_mailbox.cpp
+++ b/src/sst_mailbox.cpp
@@ -418,7 +418,7 @@
     {
         return GetLevelsInfo(pm).currentConfigTdpLevel();
     }
-    unsigned int numLevels() override
+    unsigned int maxLevel() override
     {
         return GetLevelsInfo(pm).configTdpLevels();
     }