From 119eff045d8745146902e5a563c7cdc535e4eecc Mon Sep 17 00:00:00 2001 From: JosJuice Date: Sat, 6 Jul 2024 23:31:04 +0200 Subject: [PATCH] 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 72cf2bdb87f09deff22e1085de3290126aa4ad05 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. 72cf2bdb87f09deff22e1085de3290126aa4ad05 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/Core/Core/BootManager.cpp | 49 +++++---------- .../Core/ConfigLoaders/BaseConfigLoader.cpp | 61 +++++++++++++++++-- .../Core/ConfigLoaders/BaseConfigLoader.h | 15 +++++ 3 files changed, 86 insertions(+), 39 deletions(-) diff --git a/Source/Core/Core/BootManager.cpp b/Source/Core/Core/BootManager.cpp index 52a3ec910f..5dcf2a3f5a 100644 --- a/Source/Core/Core/BootManager.cpp +++ b/Source/Core/Core/BootManager.cpp @@ -32,7 +32,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" @@ -148,6 +147,8 @@ bool BootCore(Core::System& system, std::unique_ptr boot, Core::UpdateWantDeterminism(system, /*initial*/ true); + ConfigLoaders::TransferSYSCONFControlToGuest(); + if (system.IsWii()) { Core::InitializeWiiRoot(Core::WantsDeterminism()); @@ -156,13 +157,16 @@ bool BootCore(Core::System& system, std::unique_ptr 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; + }); } } @@ -183,35 +187,6 @@ bool BootCore(Core::System& system, std::unique_ptr 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 overriden by a -// non-base layer, and restore only the overriden 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(); @@ -219,7 +194,11 @@ void RestoreConfig() if (!Core::WiiRootIsTemporary()) { Core::RestoreWiiSettings(Core::RestoreReason::EmulationEnd); - RestoreSYSCONF(); + ConfigLoaders::TransferSYSCONFControlFromGuest(ConfigLoaders::WriteBackChangedValues::Yes); + } + else + { + ConfigLoaders::TransferSYSCONFControlFromGuest(ConfigLoaders::WriteBackChangedValues::No); } Config::ClearCurrentRunLayer(); diff --git a/Source/Core/Core/ConfigLoaders/BaseConfigLoader.cpp b/Source/Core/Core/ConfigLoaders/BaseConfigLoader.cpp index 88fa7c0696..959428aec2 100644 --- a/Source/Core/Core/ConfigLoaders/BaseConfigLoader.cpp +++ b/Source/Core/Core/ConfigLoaders/BaseConfigLoader.cpp @@ -9,12 +9,14 @@ #include #include #include +#include #include #include #include #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" @@ -23,17 +25,67 @@ #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 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::Yes) + { + // 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 overriden by a + // non-base layer, and restore only the overriden 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); + } + ConfigLoaders::SaveToSYSCONF(Config::LayerType::Base); + } +} + +void SaveToSYSCONF(Config::LayerType layer, SkipIfControlledByGuest skip, + std::function predicate) +{ + std::lock_guard lock(s_sysconf_lock); + if (skip != SkipIfControlledByGuest::No && s_sysconf_controlled_by_guest) return; IOS::HLE::Kernel ios; @@ -183,7 +235,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; diff --git a/Source/Core/Core/ConfigLoaders/BaseConfigLoader.h b/Source/Core/Core/ConfigLoaders/BaseConfigLoader.h index 7c20a5e065..e07fc690e4 100644 --- a/Source/Core/Core/ConfigLoaders/BaseConfigLoader.h +++ b/Source/Core/Core/ConfigLoaders/BaseConfigLoader.h @@ -15,7 +15,22 @@ struct Location; namespace ConfigLoaders { +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 = SkipIfControlledByGuest::Yes, std::function predicate = {}); std::unique_ptr GenerateBaseConfigLoader(); } // namespace ConfigLoaders