Explicitly transfer control of SYSCONF to emulated system

The functions SaveToSYSCONF and LoadFromSYSCONF contain checks for
whether emulation is running. The intent of this is that when we're
emulating a Wii, the emulated system may write to SYSCONF whenever it
likes and does not expect anything else to write to SYSCONF, so the
host code shouldn't access SYSCONF while emulation is ongoing. However,
Core::IsRunning is an imperfect proxy for whether we've handed over
control of SYSCONF to the emulated system yet, as the actual handover
happens at a slightly different point in time than when the emulation
state is changed. This usually isn't a problem, but in theory it could
be a determinism problem if a setting is changed right as emulation is
starting, or it could cause the emulated software to briefly misbehave
if a setting is changed right as emulation is stopping.

Things got worse in 72cf2bdb87 when I
replaced the Core::IsRunning calls with !Core::IsUninitialized. With
IsRunning, there was be a period of time where SYSCONF should have been
protected but wasn't. With !IsUninitialized, there was a period of time
where SYSCONF shouldn't have been protected but was, and crucially, this
period of time included the moments where we do setup and teardown of
the emulated NAND, which broke transferring SYSCONF settings between the
host and the guest. 72cf2bdb87 was
reverted because of this.

This commit adds a flag that we explicitly flip when control is handed
over to or from the emulated system. This protects the SYSCONF file
for exactly as long as is needed.

Source/Android/jni/Config/NativeConfig.cpp

@@ -11,7 +11,6 @@
 #include "Core/ConfigLoaders/GameConfigLoader.h"
 #include "Core/ConfigLoaders/IsSettingSaveable.h"
 #include "jni/AndroidCommon/AndroidCommon.h"
-#include "jni/Host.h"
 
 constexpr jint LAYER_BASE_OR_CURRENT = 0;
 constexpr jint LAYER_BASE = 1;
@@ -143,10 +142,6 @@ Java_org_dolphinemu_dolphinemu_features_settings_model_NativeConfig_unloadGameIn
 JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_features_settings_model_NativeConfig_save(
     JNIEnv*, jclass, jint layer)
 {
-  // HostThreadLock is used to ensure we don't try to save to SYSCONF at the same time as
-  // emulation shutdown does
-  HostThreadLock guard;
-
   return GetLayer(layer, {})->Save();
 }
 

Source/Core/Core/BootManager.cpp

@@ -26,7 +26,6 @@
 #include "Core/AchievementManager.h"
 #include "Core/Boot/Boot.h"
 #include "Core/Config/MainSettings.h"
-#include "Core/Config/SYSCONFSettings.h"
 #include "Core/ConfigLoaders/BaseConfigLoader.h"
 #include "Core/ConfigLoaders/NetPlayConfigLoader.h"
 #include "Core/ConfigManager.h"
@@ -138,6 +137,8 @@ bool BootCore(Core::System& system, std::unique_ptr<BootParameters> boot,
 
   Core::UpdateWantDeterminism(system, /*initial*/ true);
 
+  ConfigLoaders::TransferSYSCONFControlToGuest();
+
   if (system.IsWii())
   {
     Core::InitializeWiiRoot(Core::WantsDeterminism());
@@ -146,13 +147,16 @@ bool BootCore(Core::System& system, std::unique_ptr<BootParameters> boot,
     if (!Core::WantsDeterminism())
     {
       Core::BackupWiiSettings();
-      ConfigLoaders::SaveToSYSCONF(Config::LayerType::Meta);
+      ConfigLoaders::SaveToSYSCONF(Config::LayerType::Meta,
+                                   ConfigLoaders::SkipIfControlledByGuest::No);
     }
     else
     {
-      ConfigLoaders::SaveToSYSCONF(Config::LayerType::Meta, [](const Config::Location& location) {
-        return Config::GetActiveLayerForConfig(location) >= Config::LayerType::Movie;
-      });
+      ConfigLoaders::SaveToSYSCONF(
+          Config::LayerType::Meta, ConfigLoaders::SkipIfControlledByGuest::No,
+          [](const Config::Location& location) {
+            return Config::GetActiveLayerForConfig(location) >= Config::LayerType::Movie;
+          });
     }
   }
 
@@ -173,43 +177,18 @@ bool BootCore(Core::System& system, std::unique_ptr<BootParameters> boot,
   return Core::Init(system, std::move(boot), wsi);
 }
 
-// SYSCONF can be modified during emulation by the user and internally, which makes it
-// a bad idea to just always overwrite it with the settings from the base layer.
-//
-// Conversely, we also shouldn't just accept any changes to SYSCONF, as it may cause
-// temporary settings (from Movie, Netplay, game INIs, etc.) to stick around.
-//
-// To avoid inconveniences in most cases, we accept changes that aren't being overridden by a
-// non-base layer, and restore only the overridden settings.
-static void RestoreSYSCONF()
-{
-  // This layer contains the new SYSCONF settings (including any temporary settings).
-  Config::Layer temp_layer(Config::LayerType::Base);
-  // Use a separate loader so the temp layer doesn't automatically save
-  ConfigLoaders::GenerateBaseConfigLoader()->Load(&temp_layer);
-
-  for (const auto& setting : Config::SYSCONF_SETTINGS)
-  {
-    std::visit(
-        [&](auto* info) {
-          // If this setting was overridden, then we copy the base layer value back to the SYSCONF.
-          // Otherwise we leave the new value in the SYSCONF.
-          if (Config::GetActiveLayerForConfig(*info) == Config::LayerType::Base)
-            Config::SetBase(*info, temp_layer.Get(*info));
-        },
-        setting.config_info);
-  }
-  ConfigLoaders::SaveToSYSCONF(Config::LayerType::Base);
-}
-
 void RestoreConfig()
 {
   Core::ShutdownWiiRoot();
 
-  if (!Core::WiiRootIsTemporary())
+  if (Core::WiiRootIsTemporary())
+  {
+    ConfigLoaders::TransferSYSCONFControlFromGuest(ConfigLoaders::WriteBackChangedValues::No);
+  }
+  else
   {
     Core::RestoreWiiSettings(Core::RestoreReason::EmulationEnd);
-    RestoreSYSCONF();
+    ConfigLoaders::TransferSYSCONFControlFromGuest(ConfigLoaders::WriteBackChangedValues::Yes);
   }
 
   Config::ClearCurrentRunLayer();

Source/Core/Core/ConfigLoaders/BaseConfigLoader.cpp

@@ -8,11 +8,13 @@
 #include <functional>
 #include <map>
 #include <memory>
+#include <mutex>
 #include <string>
 #include <variant>
 
 #include "Common/CommonTypes.h"
 #include "Common/Config/Config.h"
+#include "Common/Config/Layer.h"
 #include "Common/FileUtil.h"
 #include "Common/IniFile.h"
 #include "Common/Logging/Log.h"
@@ -21,17 +23,68 @@
 #include "Core/Config/SYSCONFSettings.h"
 #include "Core/ConfigLoaders/IsSettingSaveable.h"
 #include "Core/ConfigManager.h"
-#include "Core/Core.h"
 #include "Core/IOS/IOS.h"
 #include "Core/IOS/USB/Bluetooth/BTBase.h"
 #include "Core/SysConf.h"
 #include "Core/System.h"
 
+static bool s_sysconf_controlled_by_guest = false;
+static std::recursive_mutex s_sysconf_lock;
+
 namespace ConfigLoaders
 {
-void SaveToSYSCONF(Config::LayerType layer, std::function<bool(const Config::Location&)> predicate)
+void TransferSYSCONFControlToGuest()
 {
-  if (Core::IsRunning(Core::System::GetInstance()))
+  std::lock_guard lock(s_sysconf_lock);
+
+  ASSERT(!s_sysconf_controlled_by_guest);
+  s_sysconf_controlled_by_guest = true;
+}
+
+void TransferSYSCONFControlFromGuest(WriteBackChangedValues write_back_changed_values)
+{
+  std::lock_guard lock(s_sysconf_lock);
+
+  ASSERT(s_sysconf_controlled_by_guest);
+  s_sysconf_controlled_by_guest = false;
+
+  if (write_back_changed_values == WriteBackChangedValues::No)
+    return;
+
+  // SYSCONF can be modified during emulation by the user and internally, which makes it
+  // a bad idea to just always overwrite it with the settings from the base layer.
+  //
+  // Conversely, we also shouldn't just accept any changes to SYSCONF, as it may cause
+  // temporary settings (from Movie, Netplay, game INIs, etc.) to stick around.
+  //
+  // To avoid inconveniences in most cases, we accept changes that aren't being overridden by a
+  // non-base layer, and restore only the overridden settings.
+
+  // This layer contains the new SYSCONF settings (including any temporary settings).
+  Config::Layer temp_layer(Config::LayerType::Base);
+  // Use a separate loader so the temp layer doesn't automatically save
+  GenerateBaseConfigLoader()->Load(&temp_layer);
+
+  for (const auto& setting : Config::SYSCONF_SETTINGS)
+  {
+    std::visit(
+        [&](auto* info) {
+          // If this setting was overridden, then we copy the base layer value back to the
+          // SYSCONF. Otherwise we leave the new value in the SYSCONF.
+          if (Config::GetActiveLayerForConfig(*info) == Config::LayerType::Base)
+            Config::SetBase(*info, temp_layer.Get(*info));
+        },
+        setting.config_info);
+  }
+
+  SaveToSYSCONF(Config::LayerType::Base, SkipIfControlledByGuest::No);
+}
+
+void SaveToSYSCONF(Config::LayerType layer, SkipIfControlledByGuest skip,
+                   std::function<bool(const Config::Location&)> predicate)
+{
+  std::lock_guard lock(s_sysconf_lock);
+  if (skip == SkipIfControlledByGuest::Yes && s_sysconf_controlled_by_guest)
     return;
 
   IOS::HLE::Kernel ios;
@@ -125,7 +178,7 @@ public:
 
   void Save(Config::Layer* layer) override
   {
-    SaveToSYSCONF(layer->GetLayer());
+    SaveToSYSCONF(layer->GetLayer(), SkipIfControlledByGuest::Yes);
 
     std::map<Config::System, Common::IniFile> inis;
 
@@ -180,7 +233,8 @@ public:
 private:
   void LoadFromSYSCONF(Config::Layer* layer)
   {
-    if (Core::IsRunning(Core::System::GetInstance()))
+    std::lock_guard lock(s_sysconf_lock);
+    if (s_sysconf_controlled_by_guest)
       return;
 
     IOS::HLE::Kernel ios;

Source/Core/Core/ConfigLoaders/BaseConfigLoader.h

@@ -15,7 +15,21 @@ struct Location;
 
 namespace ConfigLoaders
 {
-void SaveToSYSCONF(Config::LayerType layer,
+enum class WriteBackChangedValues
+{
+  No,
+  Yes,
+};
+
+enum class SkipIfControlledByGuest
+{
+  No,
+  Yes,
+};
+
+void TransferSYSCONFControlToGuest();
+void TransferSYSCONFControlFromGuest(WriteBackChangedValues write_back_changed_values);
+void SaveToSYSCONF(Config::LayerType layer, SkipIfControlledByGuest skip,
                    std::function<bool(const Config::Location&)> predicate = {});
 std::unique_ptr<Config::ConfigLayerLoader> GenerateBaseConfigLoader();
 }  // namespace ConfigLoaders