Merge pull request #3794 from EmptyChaos/frame-advance-race

Core: Add synchronization to state changes (Fix Frame Step and FIFO Player -  Issue 8718)

Source/Android/jni/MainAndroid.cpp

@@ -7,6 +7,8 @@
 #include <cstdlib>
 #include <jni.h>
 #include <memory>
+#include <mutex>
+#include <thread>
 #include <android/log.h>
 #include <android/native_window_jni.h>
 #include <EGL/egl.h>
@@ -29,7 +31,6 @@
 #include "Core/HW/Wiimote.h"
 #include "Core/HW/WiimoteReal/WiimoteReal.h"
 #include "Core/PowerPC/JitInterface.h"
-#include "Core/PowerPC/PowerPC.h"
 #include "Core/PowerPC/Profiler.h"
 
 #include "DiscIO/Volume.h"
@@ -65,9 +66,24 @@ jint JNI_OnLoad(JavaVM* vm, void* reserved)
 void Host_NotifyMapLoaded() {}
 void Host_RefreshDSPDebuggerWindow() {}
 
+// The Core only supports using a single Host thread.
+// If multiple threads want to call host functions then they need to queue
+// sequentially for access.
+static std::mutex s_host_identity_lock;
 Common::Event updateMainFrameEvent;
+static bool s_have_wm_user_stop = false;
 void Host_Message(int Id)
 {
+	if (Id == WM_USER_JOB_DISPATCH)
+	{
+		updateMainFrameEvent.Set();
+	}
+	else if (Id == WM_USER_STOP)
+	{
+		s_have_wm_user_stop = true;
+		if (Core::IsRunning())
+			Core::QueueHostJob(&Core::Stop);
+	}
 }
 
 void* Host_GetRenderHandle()
@@ -388,15 +404,18 @@ JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_SurfaceDestr
 
 JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_UnPauseEmulation(JNIEnv *env, jobject obj)
 {
+	std::lock_guard<std::mutex> guard(s_host_identity_lock);
 	Core::SetState(Core::CORE_RUN);
 }
 JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_PauseEmulation(JNIEnv *env, jobject obj)
 {
+	std::lock_guard<std::mutex> guard(s_host_identity_lock);
 	Core::SetState(Core::CORE_PAUSE);
 }
 
 JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_StopEmulation(JNIEnv *env, jobject obj)
 {
+	std::lock_guard<std::mutex> guard(s_host_identity_lock);
 	Core::SaveScreenShot("thumb");
 	Renderer::s_screenshotCompleted.WaitFor(std::chrono::seconds(2));
 	Core::Stop();
@@ -485,6 +504,7 @@ JNIEXPORT jboolean JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_Supports
 
 JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_SaveScreenShot(JNIEnv *env, jobject obj)
 {
+	std::lock_guard<std::mutex> guard(s_host_identity_lock);
 	Core::SaveScreenShot();
 }
 
@@ -529,11 +549,13 @@ JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_SetFilename(
 
 JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_SaveState(JNIEnv *env, jobject obj, jint slot)
 {
+	std::lock_guard<std::mutex> guard(s_host_identity_lock);
 	State::Save(slot);
 }
 
 JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_LoadState(JNIEnv *env, jobject obj, jint slot)
 {
+	std::lock_guard<std::mutex> guard(s_host_identity_lock);
 	State::Load(slot);
 }
 
@@ -560,6 +582,7 @@ JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_CreateUserFo
 
 JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_SetUserDirectory(JNIEnv *env, jobject obj, jstring jDirectory)
 {
+	std::lock_guard<std::mutex> guard(s_host_identity_lock);
 	std::string directory = GetJString(env, jDirectory);
 	g_set_userpath = directory;
 	UICommon::SetUserDirectory(directory);
@@ -572,6 +595,7 @@ JNIEXPORT jstring JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_GetUserDi
 
 JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_SetProfiling(JNIEnv *env, jobject obj, jboolean enable)
 {
+	std::lock_guard<std::mutex> guard(s_host_identity_lock);
 	Core::SetState(Core::CORE_PAUSE);
 	JitInterface::ClearCache();
 	Profiler::g_ProfileBlocks = enable;
@@ -580,6 +604,7 @@ JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_SetProfiling
 
 JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_WriteProfileResults(JNIEnv *env, jobject obj)
 {
+	std::lock_guard<std::mutex> guard(s_host_identity_lock);
 	std::string filename = File::GetUserPath(D_DUMP_IDX) + "Debug/profiler.txt";
 	File::CreateFullPath(filename);
 	JitInterface::WriteProfileResults(filename);
@@ -638,6 +663,7 @@ JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_SurfaceDestr
 }
 JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_RefreshWiimotes(JNIEnv *env, jobject obj)
 {
+	std::lock_guard<std::mutex> guard(s_host_identity_lock);
 	WiimoteReal::Refresh();
 }
 
@@ -651,21 +677,37 @@ JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_NativeLibrary_Run(JNIEnv *
 
 	RegisterMsgAlertHandler(&MsgAlert);
 
+	std::unique_lock<std::mutex> guard(s_host_identity_lock);
 	UICommon::SetUserDirectory(g_set_userpath);
 	UICommon::Init();
 
 	WiimoteReal::InitAdapterClass();
 
 	// No use running the loop when booting fails
+	s_have_wm_user_stop = false;
 	if ( BootManager::BootCore( g_filename.c_str() ) )
 	{
-		PowerPC::Start();
-		while (PowerPC::GetState() != PowerPC::CPU_POWERDOWN)
+		static constexpr int TIMEOUT   = 10000;
+		static constexpr int WAIT_STEP = 25;
+		int time_waited = 0;
+		// A Core::CORE_ERROR state would be helpful here.
+		while (!Core::IsRunning() && time_waited < TIMEOUT && !s_have_wm_user_stop)
+		{
+			std::this_thread::sleep_for(std::chrono::milliseconds(WAIT_STEP));
+			time_waited += WAIT_STEP;
+		}
+		while (Core::IsRunning())
+		{
+			guard.unlock();
 			updateMainFrameEvent.Wait();
+			guard.lock();
+			Core::HostDispatchJobs();
+		}
 	}
 
 	Core::Shutdown();
 	UICommon::Shutdown();
+	guard.unlock();
 
 	if (surf)
 	{

Source/Core/AudioCommon/Mixer.cpp

@@ -12,9 +12,6 @@
 #include "Common/Logging/Log.h"
 #include "Core/ConfigManager.h"
 
-// UGLINESS
-#include "Core/PowerPC/PowerPC.h"
-
 #if _M_SSE >= 0x301 && !(defined __GNUC__ && !defined __SSSE3__)
 #include <tmmintrin.h>
 #endif
@@ -121,12 +118,6 @@ unsigned int CMixer::Mix(short* samples, unsigned int num_samples, bool consider
 
 	memset(samples, 0, num_samples * 2 * sizeof(short));
 
-	if (PowerPC::GetState() != PowerPC::CPU_RUNNING)
-	{
-		// Silence
-		return num_samples;
-	}
-
 	m_dma_mixer.Mix(samples, num_samples, consider_framelimit);
 	m_streaming_mixer.Mix(samples, num_samples, consider_framelimit);
 	m_wiimote_speaker_mixer.Mix(samples, num_samples, consider_framelimit);

Source/Core/Common/Common.h

@@ -83,6 +83,7 @@ enum HOST_COMM
 	WM_USER_STOP = 10,
 	WM_USER_CREATE,
 	WM_USER_SETCURSOR,
+	WM_USER_JOB_DISPATCH,
 };
 
 // Used for notification on emulation state

Source/Core/Common/Event.h

@@ -42,8 +42,7 @@ public:
 			return;
 
 		std::unique_lock<std::mutex> lk(m_mutex);
-		m_condvar.wait(lk, [&]{ return m_flag.IsSet(); });
-		m_flag.Clear();
+		m_condvar.wait(lk, [&]{ return m_flag.TestAndClear(); });
 	}
 
 	template<class Rep, class Period>
@@ -54,8 +53,7 @@ public:
 
 		std::unique_lock<std::mutex> lk(m_mutex);
 		bool signaled = m_condvar.wait_for(lk, rel_time,
-			[&]{ return m_flag.IsSet(); });
-		m_flag.Clear();
+			[&]{ return m_flag.TestAndClear(); });
 
 		return signaled;
 	}

Source/Core/Core/Core.cpp

@@ -5,6 +5,9 @@
 #include <atomic>
 #include <cctype>
 #include <cstring>
+#include <mutex>
+#include <queue>
+#include <utility>
 
 #ifdef _WIN32
 #include <windows.h>
@@ -102,6 +105,7 @@ void EmuThread();
 static bool s_is_stopping = false;
 static bool s_hardware_initialized = false;
 static bool s_is_started = false;
+static std::atomic<bool> s_is_booting{ false };
 static void* s_window_handle = nullptr;
 static std::string s_state_filename;
 static std::thread s_emu_thread;
@@ -112,6 +116,14 @@ static bool s_request_refresh_info = false;
 static int s_pause_and_lock_depth = 0;
 static bool s_is_throttler_temp_disabled = false;
 
+struct HostJob
+{
+	std::function<void()> job;
+	bool run_after_stop;
+};
+static std::mutex          s_host_jobs_lock;
+static std::queue<HostJob> s_host_jobs_queue;
+
 #ifdef ThreadLocalStorage
 static ThreadLocalStorage bool tls_is_cpu_thread = false;
 #else
@@ -225,6 +237,9 @@ bool Init()
 		s_emu_thread.join();
 	}
 
+	// Drain any left over jobs
+	HostDispatchJobs();
+
 	Core::UpdateWantDeterminism(/*initial*/ true);
 
 	INFO_LOG(OSREPORT, "Starting core = %s mode",
@@ -260,16 +275,16 @@ void Stop()  // - Hammertime!
 
 	s_is_stopping = true;
 
+	// Dump left over jobs
+	HostDispatchJobs();
+
 	Fifo::EmulatorState(false);
 
 	INFO_LOG(CONSOLE, "Stop [Main Thread]\t\t---- Shutting down ----");
 
 	// Stop the CPU
 	INFO_LOG(CONSOLE, "%s", StopMessage(true, "Stop CPU").c_str());
-	PowerPC::Stop();
-
-	// Kick it if it's waiting (code stepping wait loop)
-	CPU::StepOpcode();
+	CPU::Stop();
 
 	if (_CoreParameter.bCPUThread)
 	{
@@ -313,6 +328,16 @@ void UndeclareAsCPUThread()
 #endif
 }
 
+// For the CPU Thread only.
+static void CPUSetInitialExecutionState()
+{
+	QueueHostJob([]
+	{
+		SetState(SConfig::GetInstance().bBootToPause ? CORE_PAUSE : CORE_RUN);
+		Host_UpdateMainFrame();
+	});
+}
+
 // Create the CPU thread, which is a CPU + Video thread in Single Core mode.
 static void CpuThread()
 {
@@ -334,10 +359,20 @@ static void CpuThread()
 		EMM::InstallExceptionHandler(); // Let's run under memory watch
 
 	if (!s_state_filename.empty())
-		State::LoadAs(s_state_filename);
+	{
+		// Needs to PauseAndLock the Core
+		// NOTE: EmuThread should have left us in CPU_STEPPING so nothing will happen
+		//   until after the job is serviced.
+		QueueHostJob([]
+		{
+			// Recheck in case Movie cleared it since.
+			if (!s_state_filename.empty())
+				State::LoadAs(s_state_filename);
+		});
+	}
 
 	s_is_started = true;
-
+	CPUSetInitialExecutionState();
 
 	#ifdef USE_GDBSTUB
 	#ifndef _WIN32
@@ -376,6 +411,7 @@ static void CpuThread()
 
 static void FifoPlayerThread()
 {
+	DeclareAsCPUThread();
 	const SConfig& _CoreParameter = SConfig::GetInstance();
 
 	if (_CoreParameter.bCPUThread)
@@ -388,18 +424,37 @@ static void FifoPlayerThread()
 		Common::SetCurrentThreadName("FIFO-GPU thread");
 	}
 
-	s_is_started = true;
-	DeclareAsCPUThread();
-
 	// Enter CPU run loop. When we leave it - we are done.
 	if (FifoPlayer::GetInstance().Open(_CoreParameter.m_strFilename))
 	{
-		FifoPlayer::GetInstance().Play();
+		if (auto cpu_core = FifoPlayer::GetInstance().GetCPUCore())
+		{
+			PowerPC::InjectExternalCPUCore(cpu_core.get());
+			s_is_started = true;
+
+			CPUSetInitialExecutionState();
+			CPU::Run();
+
+			s_is_started = false;
+			PowerPC::InjectExternalCPUCore(nullptr);
+		}
 		FifoPlayer::GetInstance().Close();
 	}
 
-	UndeclareAsCPUThread();
-	s_is_started = false;
+	// If we did not enter the CPU Run Loop above then run a fake one instead.
+	// We need to be IsRunningAndStarted() for DolphinWX to stop us.
+	if (CPU::GetState() != CPU::CPU_POWERDOWN)
+	{
+		s_is_started = true;
+		Host_Message(WM_USER_STOP);
+		while (CPU::GetState() != CPU::CPU_POWERDOWN)
+		{
+			if (!_CoreParameter.bCPUThread)
+				g_video_backend->PeekMessages();
+			std::this_thread::sleep_for(std::chrono::milliseconds(20));
+		}
+		s_is_started = false;
+	}
 
 	if (!_CoreParameter.bCPUThread)
 		g_video_backend->Video_Cleanup();
@@ -413,6 +468,7 @@ static void FifoPlayerThread()
 void EmuThread()
 {
 	const SConfig& core_parameter = SConfig::GetInstance();
+	s_is_booting.store(true);
 
 	Common::SetCurrentThreadName("Emuthread - Starting");
 
@@ -431,6 +487,7 @@ void EmuThread()
 
 	if (!g_video_backend->Initialize(s_window_handle))
 	{
+		s_is_booting.store(false);
 		PanicAlert("Failed to initialize video backend!");
 		Host_Message(WM_USER_STOP);
 		return;
@@ -445,6 +502,7 @@ void EmuThread()
 
 	if (!DSP::GetDSPEmulator()->Initialize(core_parameter.bWii, core_parameter.bDSPThread))
 	{
+		s_is_booting.store(false);
 		HW::Shutdown();
 		g_video_backend->Shutdown();
 		PanicAlert("Failed to initialize DSP emulation!");
@@ -485,9 +543,10 @@ void EmuThread()
 
 	// The hardware is initialized.
 	s_hardware_initialized = true;
+	s_is_booting.store(false);
 
-	// Boot to pause or not
-	Core::SetState(core_parameter.bBootToPause ? Core::CORE_PAUSE : Core::CORE_RUN);
+	// Set execution state to known values (CPU/FIFO/Audio Paused)
+	CPU::Break();
 
 	// Load GCM/DOL/ELF whatever ... we boot with the interpreter core
 	PowerPC::SetMode(PowerPC::MODE_INTERPRETER);
@@ -552,7 +611,7 @@ void EmuThread()
 		// Spawn the CPU+GPU thread
 		s_cpu_thread = std::thread(cpuThreadFunc);
 
-		while (PowerPC::GetState() != PowerPC::CPU_POWERDOWN)
+		while (CPU::GetState() != CPU::CPU_POWERDOWN)
 		{
 			g_video_backend->PeekMessages();
 			Common::SleepCurrentThread(20);
@@ -621,11 +680,17 @@ void EmuThread()
 
 // Set or get the running state
 
-void SetState(EState _State)
+void SetState(EState state)
 {
-	switch (_State)
+	// State cannot be controlled until the CPU Thread is operational
+	if (!IsRunningAndStarted())
+		return;
+
+	switch (state)
 	{
 	case CORE_PAUSE:
+		// NOTE: GetState() will return CORE_PAUSE immediately, even before anything has
+		//   stopped (including the CPU).
 		CPU::EnableStepping(true);  // Break
 		Wiimote::Pause();
 #if defined(__LIBUSB__) || defined(_WIN32)
@@ -719,30 +784,50 @@ void RequestRefreshInfo()
 	s_request_refresh_info = true;
 }
 
-bool PauseAndLock(bool doLock, bool unpauseOnUnlock)
+bool PauseAndLock(bool do_lock, bool unpause_on_unlock)
 {
+	// WARNING: PauseAndLock is not fully threadsafe so is only valid on the Host Thread
 	if (!IsRunning())
 		return true;
 
 	// let's support recursive locking to simplify things on the caller's side,
 	// and let's do it at this outer level in case the individual systems don't support it.
-	if (doLock ? s_pause_and_lock_depth++ : --s_pause_and_lock_depth)
+	if (do_lock ? s_pause_and_lock_depth++ : --s_pause_and_lock_depth)
 		return true;
 
-	// first pause or unpause the CPU
-	bool wasUnpaused = CPU::PauseAndLock(doLock, unpauseOnUnlock);
-	ExpansionInterface::PauseAndLock(doLock, unpauseOnUnlock);
+	bool was_unpaused = true;
+	if (do_lock)
+	{
+		// first pause the CPU
+		// This acquires a wrapper mutex and converts the current thread into
+		// a temporary replacement CPU Thread.
+		was_unpaused = CPU::PauseAndLock(true);
+	}
+
+	ExpansionInterface::PauseAndLock(do_lock, false);
 
 	// audio has to come after CPU, because CPU thread can wait for audio thread (m_throttle).
-	DSP::GetDSPEmulator()->PauseAndLock(doLock, unpauseOnUnlock);
+	DSP::GetDSPEmulator()->PauseAndLock(do_lock, false);
 
 	// video has to come after CPU, because CPU thread can wait for video thread (s_efbAccessRequested).
-	Fifo::PauseAndLock(doLock, unpauseOnUnlock);
+	Fifo::PauseAndLock(do_lock, false);
 
 #if defined(__LIBUSB__) || defined(_WIN32)
 	GCAdapter::ResetRumble();
 #endif
-	return wasUnpaused;
+
+	// CPU is unlocked last because CPU::PauseAndLock contains the synchronization
+	// mechanism that prevents CPU::Break from racing.
+	if (!do_lock)
+	{
+		// The CPU is responsible for managing the Audio and FIFO state so we use its
+		// mechanism to unpause them. If we unpaused the systems above when releasing
+		// the locks then they could call CPU::Break which would require detecting it
+		// and re-pausing with CPU::EnableStepping.
+		was_unpaused = CPU::PauseAndLock(false, unpause_on_unlock, true);
+	}
+
+	return was_unpaused;
 }
 
 // Display FPS info
@@ -803,7 +888,7 @@ void UpdateTitle()
 	float Speed = (float)(s_drawn_video.load() * (100 * 1000.0) / (VideoInterface::GetTargetRefreshRate() * ElapseTime));
 
 	// Settings are shown the same for both extended and summary info
-	std::string SSettings = StringFromFormat("%s %s | %s | %s", cpu_core_base->GetName(), _CoreParameter.bCPUThread ? "DC" : "SC",
+	std::string SSettings = StringFromFormat("%s %s | %s | %s", PowerPC::GetCPUName(), _CoreParameter.bCPUThread ? "DC" : "SC",
 		g_video_backend->GetDisplayName().c_str(), _CoreParameter.bDSPHLE ? "HLE" : "LLE");
 
 	std::string SFPS;
@@ -865,6 +950,9 @@ void Shutdown()
 	// on MSDN.
 	if (s_emu_thread.joinable())
 		s_emu_thread.join();
+
+	// Make sure there's nothing left over in case we're about to exit.
+	HostDispatchJobs();
 }
 
 void SetOnStoppedCallback(StoppedCallbackFunc callback)
@@ -898,4 +986,45 @@ void UpdateWantDeterminism(bool initial)
 	}
 }
 
+void QueueHostJob(std::function<void()> job, bool run_during_stop)
+{
+	if (!job)
+		return;
+
+	bool send_message = false;
+	{
+		std::lock_guard<std::mutex> guard(s_host_jobs_lock);
+		send_message = s_host_jobs_queue.empty();
+		s_host_jobs_queue.emplace(HostJob{ std::move(job), run_during_stop });
+	}
+	// If the the queue was empty then kick the Host to come and get this job.
+	if (send_message)
+		Host_Message(WM_USER_JOB_DISPATCH);
+}
+
+void HostDispatchJobs()
+{
+	// WARNING: This should only run on the Host Thread.
+	// NOTE: This function is potentially re-entrant. If a job calls
+	//   Core::Stop for instance then we'll enter this a second time.
+	std::unique_lock<std::mutex> guard(s_host_jobs_lock);
+	while (!s_host_jobs_queue.empty())
+	{
+		HostJob job = std::move(s_host_jobs_queue.front());
+		s_host_jobs_queue.pop();
+
+		// NOTE: Memory ordering is important. The booting flag needs to be
+		//   checked first because the state transition is:
+		//   CORE_UNINITIALIZED: s_is_booting -> s_hardware_initialized
+		//   We need to check variables in the same order as the state
+		//   transition, otherwise we race and get transient failures.
+		if (!job.run_after_stop && !s_is_booting.load() && !IsRunning())
+			continue;
+
+		guard.unlock();
+		job.job();
+		guard.lock();
+	}
+}
+
 } // Core

Source/Core/Core/Core.h

@@ -11,6 +11,7 @@
 
 #pragma once
 
+#include <functional>
 #include <string>
 #include <vector>
 
@@ -52,7 +53,8 @@ bool IsRunningInCurrentThread(); // this tells us whether we are running in the
 bool IsCPUThread(); // this tells us whether we are the CPU thread.
 bool IsGPUThread();
 
-void SetState(EState _State);
+// [NOT THREADSAFE] For use by Host only
+void SetState(EState state);
 EState GetState();
 
 void SaveScreenShot();
@@ -80,13 +82,29 @@ void UpdateTitle();
 // or, if doLock is false, releases a lock on that state and optionally unpauses.
 // calls must be balanced (once with doLock true, then once with doLock false) but may be recursive.
 // the return value of the first call should be passed in as the second argument of the second call.
-bool PauseAndLock(bool doLock, bool unpauseOnUnlock=true);
+// [NOT THREADSAFE] Host only
+bool PauseAndLock(bool doLock, bool unpauseOnUnlock = true);
 
 // for calling back into UI code without introducing a dependency on it in core
 typedef void(*StoppedCallbackFunc)(void);
 void SetOnStoppedCallback(StoppedCallbackFunc callback);
 
-// Run on the GUI thread when the factors change.
+// Run on the Host thread when the factors change. [NOT THREADSAFE]
 void UpdateWantDeterminism(bool initial = false);
 
+// Queue an arbitrary function to asynchronously run once on the Host thread later.
+// Threadsafe. Can be called by any thread, including the Host itself.
+// Jobs will be executed in RELATIVE order. If you queue 2 jobs from the same thread
+// then they will be executed in the order they were queued; however, there is no
+// global order guarantee across threads - jobs from other threads may execute in
+// between.
+// NOTE: Make sure the jobs check the global state instead of assuming everything is
+//   still the same as when the job was queued.
+// NOTE: Jobs that are not set to run during stop will be discarded instead.
+void QueueHostJob(std::function<void()> job, bool run_during_stop = false);
+
+// Should be called periodically by the Host to run pending jobs.
+// WM_USER_JOB_DISPATCH will be sent when something is added to the queue.
+void HostDispatchJobs();
+
 }  // namespace

Source/Core/Core/Debugger/PPCDebugInterface.cpp

@@ -10,7 +10,6 @@
 #include "Core/Host.h"
 #include "Core/Debugger/Debugger_SymbolMap.h"
 #include "Core/Debugger/PPCDebugInterface.h"
-#include "Core/HW/CPU.h"
 #include "Core/HW/DSP.h"
 #include "Core/HW/Memmap.h"
 #include "Core/PowerPC/PowerPC.h"
@@ -20,7 +19,7 @@
 std::string PPCDebugInterface::Disassemble(unsigned int address)
 {
 	// PowerPC::HostRead_U32 seemed to crash on shutdown
-	if (PowerPC::GetState() == PowerPC::CPU_POWERDOWN)
+	if (!IsAlive())
 		return "";
 
 	if (Core::GetState() == Core::CORE_PAUSE)
@@ -51,7 +50,7 @@ std::string PPCDebugInterface::Disassemble(unsigned int address)
 
 void PPCDebugInterface::GetRawMemoryString(int memory, unsigned int address, char *dest, int max_size)
 {
-	if (Core::GetState() != Core::CORE_UNINITIALIZED)
+	if (IsAlive())
 	{
 		if (memory || PowerPC::HostIsRAMAddress(address))
 		{
@@ -96,7 +95,7 @@ unsigned int PPCDebugInterface::ReadInstruction(unsigned int address)
 
 bool PPCDebugInterface::IsAlive()
 {
-	return Core::GetState() != Core::CORE_UNINITIALIZED;
+	return Core::IsRunning();
 }
 
 bool PPCDebugInterface::IsBreakpoint(unsigned int address)

Source/Core/Core/FifoPlayer/FifoPlayer.cpp

@@ -7,12 +7,14 @@
 
 #include "Common/Assert.h"
 #include "Common/CommonTypes.h"
+#include "Common/MsgHandler.h"
 #include "Core/ConfigManager.h"
 #include "Core/Core.h"
 #include "Core/CoreTiming.h"
 #include "Core/Host.h"
 #include "Core/FifoPlayer/FifoDataFile.h"
 #include "Core/FifoPlayer/FifoPlayer.h"
+#include "Core/HW/CPU.h"
 #include "Core/HW/GPFifo.h"
 #include "Core/HW/Memmap.h"
 #include "Core/HW/ProcessorInterface.h"
@@ -58,55 +60,103 @@ void FifoPlayer::Close()
 	m_FrameRangeEnd = 0;
 }
 
-bool FifoPlayer::Play()
+class FifoPlayer::CPUCore final : public CPUCoreBase
 {
-	if (!m_File)
-		return false;
-
-	if (m_File->GetFrameCount() == 0)
-		return false;
-
-	IsPlayingBackFifologWithBrokenEFBCopies = m_File->HasBrokenEFBCopies();
-
-	m_CurrentFrame = m_FrameRangeStart;
-
-	LoadMemory();
-
-	// This loop replaces the CPU loop that occurs when a game is run
-	while (PowerPC::GetState() != PowerPC::CPU_POWERDOWN)
+public:
+	explicit CPUCore(FifoPlayer* parent)
+		: m_parent(parent)
 	{
-		if (PowerPC::GetState() == PowerPC::CPU_RUNNING)
+	}
+	CPUCore(const CPUCore&) = delete;
+	~CPUCore()
+	{
+	}
+	CPUCore& operator=(const CPUCore&) = delete;
+
+	void Init() override
+	{
+		IsPlayingBackFifologWithBrokenEFBCopies = m_parent->m_File->HasBrokenEFBCopies();
+
+		m_parent->m_CurrentFrame = m_parent->m_FrameRangeStart;
+		m_parent->LoadMemory();
+	}
+
+	void Shutdown() override
+	{
+		IsPlayingBackFifologWithBrokenEFBCopies = false;
+	}
+
+	void ClearCache() override
+	{
+		// Nothing to clear.
+	}
+
+	void SingleStep() override
+	{
+		// NOTE: AdvanceFrame() will get stuck forever in Dual Core because the FIFO
+		//   is disabled by CPU::EnableStepping(true) so the frame never gets displayed.
+		PanicAlertT("Cannot SingleStep the FIFO. Use Frame Advance instead.");
+	}
+
+	const char* GetName() override
+	{
+		return "FifoPlayer";
+	}
+
+	void Run() override
+	{
+		while (CPU::GetState() == CPU::CPU_RUNNING)
 		{
-			if (m_CurrentFrame >= m_FrameRangeEnd)
+			switch (m_parent->AdvanceFrame())
 			{
-				if (m_Loop)
-				{
-					m_CurrentFrame = m_FrameRangeStart;
-				}
-				else
-				{
-					PowerPC::Stop();
-					Host_Message(WM_USER_STOP);
-				}
-			}
-			else
-			{
-				if (m_FrameWrittenCb)
-					m_FrameWrittenCb();
+			case CPU::CPU_POWERDOWN:
+				CPU::Break();
+				Host_Message(WM_USER_STOP);
+				break;
 
-				if (m_EarlyMemoryUpdates && m_CurrentFrame == m_FrameRangeStart)
-					WriteAllMemoryUpdates();
-
-				WriteFrame(m_File->GetFrame(m_CurrentFrame), m_FrameInfo[m_CurrentFrame]);
-
-				++m_CurrentFrame;
+			case CPU::CPU_STEPPING:
+				CPU::Break();
+				Host_UpdateMainFrame();
+				break;
 			}
 		}
 	}
 
-	IsPlayingBackFifologWithBrokenEFBCopies = false;
+private:
+	FifoPlayer* m_parent;
+};
 
-	return true;
+int FifoPlayer::AdvanceFrame()
+{
+	if (m_CurrentFrame >= m_FrameRangeEnd)
+	{
+		if (!m_Loop)
+			return CPU::CPU_POWERDOWN;
+		// If there are zero frames in the range then sleep instead of busy spinning
+		if (m_FrameRangeStart >= m_FrameRangeEnd)
+			return CPU::CPU_STEPPING;
+
+		m_CurrentFrame = m_FrameRangeStart;
+	}
+
+	if (m_FrameWrittenCb)
+		m_FrameWrittenCb();
+
+	if (m_EarlyMemoryUpdates && m_CurrentFrame == m_FrameRangeStart)
+		WriteAllMemoryUpdates();
+
+	WriteFrame(m_File->GetFrame(m_CurrentFrame), m_FrameInfo[m_CurrentFrame]);
+
+	++m_CurrentFrame;
+	return CPU::CPU_RUNNING;
+}
+
+std::unique_ptr<CPUCoreBase> FifoPlayer::GetCPUCore()
+{
+	if (!m_File || m_File->GetFrameCount() == 0)
+		return nullptr;
+
+	return std::make_unique<CPUCore>(this);
 }
 
 u32 FifoPlayer::GetFrameObjectCount()

Source/Core/Core/FifoPlayer/FifoPlayer.h

@@ -4,10 +4,12 @@
 
 #pragma once
 
+#include <memory>
 #include <string>
 #include <vector>
 
 #include "Core/FifoPlayer/FifoPlaybackAnalyzer.h"
+#include "Core/PowerPC/CPUCoreBase.h"
 
 class FifoDataFile;
 struct MemoryUpdate;
@@ -50,8 +52,12 @@ public:
 	bool Open(const std::string& filename);
 	void Close();
 
-	// Play is controlled by the state of PowerPC
-	bool Play();
+	// Returns a CPUCoreBase instance that can be injected into PowerPC as a
+	// pseudo-CPU. The instance is only valid while the FifoPlayer is Open().
+	// Returns nullptr if the FifoPlayer is not initialized correctly.
+	// Play/Pause/Stop of the FifoLog can be controlled normally via the
+	// PowerPC state.
+	std::unique_ptr<CPUCoreBase> GetCPUCore();
 
 	FifoDataFile *GetFile() { return m_File; }
 
@@ -85,8 +91,12 @@ public:
 	static FifoPlayer &GetInstance();
 
 private:
+	class CPUCore;
+
 	FifoPlayer();
 
+	int AdvanceFrame();
+
 	void WriteFrame(const FifoFrameInfo& frame, const AnalyzedFrameInfo &info);
 	void WriteFramePart(u32 dataStart, u32 dataEnd, u32 &nextMemUpdate, const FifoFrameInfo& frame, const AnalyzedFrameInfo& info);
 

Source/Core/Core/HLE/HLE_Misc.cpp

@@ -9,6 +9,7 @@
 #include "Core/ConfigManager.h"
 #include "Core/Host.h"
 #include "Core/HLE/HLE_Misc.h"
+#include "Core/HW/CPU.h"
 #include "Core/PowerPC/PowerPC.h"
 #include "Core/PowerPC/PPCCache.h"
 
@@ -35,7 +36,7 @@ void HLEPanicAlert()
 void HBReload()
 {
 	// There isn't much we can do. Just stop cleanly.
-	PowerPC::Pause();
+	CPU::Break();
 	Host_Message(WM_USER_STOP);
 }
 

Source/Core/Core/HW/CPU.cpp

@@ -2,11 +2,13 @@
 // Licensed under GPLv2+
 // Refer to the license.txt file included.
 
+#include <condition_variable>
 #include <mutex>
 
 #include "AudioCommon/AudioCommon.h"
 #include "Common/CommonTypes.h"
 #include "Common/Event.h"
+#include "Common/Logging/Log.h"
 #include "Core/Core.h"
 #include "Core/Host.h"
 #include "Core/HW/CPU.h"
@@ -14,82 +16,177 @@
 #include "Core/PowerPC/PowerPC.h"
 #include "VideoCommon/Fifo.h"
 
-namespace
-{
-	static Common::Event m_StepEvent;
-	static Common::Event *m_SyncEvent = nullptr;
-	static std::mutex m_csCpuOccupied;
-}
-
 namespace CPU
 {
 
+// CPU Thread execution state.
+// Requires s_state_change_lock to modify the value.
+// Read access is unsynchronized.
+static State s_state = CPU_POWERDOWN;
+
+// Synchronizes EnableStepping and PauseAndLock so only one instance can be
+// active at a time. Simplifies code by eliminating several edge cases where
+// the EnableStepping(true)/PauseAndLock(true) case must release the state lock
+// and wait for the CPU Thread which would otherwise require additional flags.
+// NOTE: When using the stepping lock, it must always be acquired first. If
+//   the lock is acquired after the state lock then that is guaranteed to
+//   deadlock because of the order inversion. (A -> X,Y; B -> Y,X; A waits for
+//   B, B waits for A)
+static std::mutex s_stepping_lock;
+
+// Primary lock. Protects changing s_state, requesting instruction stepping and
+// pause-and-locking.
+static std::mutex              s_state_change_lock;
+// When s_state_cpu_thread_active changes to false
+static std::condition_variable s_state_cpu_idle_cvar;
+// When s_state changes / s_state_paused_and_locked becomes false (for CPU Thread only)
+static std::condition_variable s_state_cpu_cvar;
+static bool                    s_state_cpu_thread_active         = false;
+static bool                    s_state_paused_and_locked         = false;
+static bool                    s_state_system_request_stepping   = false;
+static bool                    s_state_cpu_step_instruction      = false;
+static Common::Event*          s_state_cpu_step_instruction_sync = nullptr;
+
 void Init(int cpu_core)
 {
 	PowerPC::Init(cpu_core);
-	m_SyncEvent = nullptr;
+	s_state = CPU_STEPPING;
 }
 
 void Shutdown()
 {
+	Stop();
 	PowerPC::Shutdown();
-	m_SyncEvent = nullptr;
+}
+
+// Requires holding s_state_change_lock
+static void FlushStepSyncEventLocked()
+{
+	if (s_state_cpu_step_instruction_sync)
+	{
+		s_state_cpu_step_instruction_sync->Set();
+		s_state_cpu_step_instruction_sync = nullptr;
+	}
+	s_state_cpu_step_instruction = false;
 }
 
 void Run()
 {
-	std::lock_guard<std::mutex> lk(m_csCpuOccupied);
-	Host_UpdateDisasmDialog();
-
-	while (true)
+	std::unique_lock<std::mutex> state_lock(s_state_change_lock);
+	while (s_state != CPU_POWERDOWN)
 	{
-		switch (PowerPC::GetState())
+		s_state_cpu_cvar.wait(state_lock, [] { return !s_state_paused_and_locked; });
+
+		switch (s_state)
 		{
-		case PowerPC::CPU_RUNNING:
-			//1: enter a fast runloop
+		case CPU_RUNNING:
+			s_state_cpu_thread_active = true;
+			state_lock.unlock();
+
+			// Adjust PC for JIT when debugging
+			// SingleStep so that the "continue", "step over" and "step out" debugger functions
+			// work when the PC is at a breakpoint at the beginning of the block
+			// If watchpoints are enabled, any instruction could be a breakpoint.
+			if (PowerPC::GetMode() != PowerPC::MODE_INTERPRETER)
+			{
+#ifndef ENABLE_MEM_CHECK
+				if (PowerPC::breakpoints.IsAddressBreakPoint(PC))
+#endif
+				{
+					PowerPC::CoreMode old_mode = PowerPC::GetMode();
+					PowerPC::SetMode(PowerPC::MODE_INTERPRETER);
+					PowerPC::SingleStep();
+					PowerPC::SetMode(old_mode);
+				}
+			}
+
+			// Enter a fast runloop
 			PowerPC::RunLoop();
+
+			state_lock.lock();
+			s_state_cpu_thread_active = false;
+			s_state_cpu_idle_cvar.notify_all();
 			break;
 
-		case PowerPC::CPU_STEPPING:
-			m_csCpuOccupied.unlock();
-
-			//1: wait for step command..
-			m_StepEvent.Wait();
-
-			m_csCpuOccupied.lock();
-			if (PowerPC::GetState() == PowerPC::CPU_POWERDOWN)
-				return;
-			if (PowerPC::GetState() != PowerPC::CPU_STEPPING)
+		case CPU_STEPPING:
+			// Wait for step command.
+			s_state_cpu_cvar.wait(state_lock, []
+			{
+				return s_state_cpu_step_instruction ||
+				       s_state != CPU_STEPPING;
+			});
+			if (s_state != CPU_STEPPING)
+			{
+				// Signal event if the mode changes.
+				FlushStepSyncEventLocked();
+				continue;
+			}
+			if (s_state_paused_and_locked)
 				continue;
 
-			//3: do a step
+			// Do step
+			s_state_cpu_thread_active = true;
+			state_lock.unlock();
+
 			PowerPC::SingleStep();
 
-			//4: update disasm dialog
-			if (m_SyncEvent)
-			{
-				m_SyncEvent->Set();
-				m_SyncEvent = nullptr;
-			}
+			state_lock.lock();
+			s_state_cpu_thread_active = false;
+			s_state_cpu_idle_cvar.notify_all();
+
+			// Update disasm dialog
+			FlushStepSyncEventLocked();
 			Host_UpdateDisasmDialog();
 			break;
 
-		case PowerPC::CPU_POWERDOWN:
-			//1: Exit loop!!
-			return;
+		case CPU_POWERDOWN:
+			break;
 		}
 	}
+	state_lock.unlock();
+	Host_UpdateDisasmDialog();
+}
+
+// Requires holding s_state_change_lock
+static void RunAdjacentSystems(bool running)
+{
+	// NOTE: We're assuming these will not try to call Break or EnableStepping.
+	Fifo::EmulatorState(running);
+	AudioCommon::ClearAudioBuffer(!running);
 }
 
 void Stop()
 {
-	PowerPC::Stop();
-	m_StepEvent.Set();
+	// Change state and wait for it to be acknowledged.
+	// We don't need the stepping lock because CPU_POWERDOWN is a priority state which
+	// will stick permanently.
+	std::unique_lock<std::mutex> state_lock(s_state_change_lock);
+	s_state = CPU_POWERDOWN;
+	s_state_cpu_cvar.notify_one();
+	// FIXME: MsgHandler can cause this to deadlock the GUI Thread. Remove the timeout.
+	bool success = s_state_cpu_idle_cvar.wait_for(state_lock, std::chrono::seconds(5), []
+	{
+		return !s_state_cpu_thread_active;
+	});
+	if (!success)
+		ERROR_LOG(POWERPC, "CPU Thread failed to acknowledge CPU_POWERDOWN. It may be deadlocked.");
+	RunAdjacentSystems(false);
+	FlushStepSyncEventLocked();
 }
 
 bool IsStepping()
 {
-	return PowerPC::GetState() == PowerPC::CPU_STEPPING;
+	return s_state == CPU_STEPPING;
+}
+
+State GetState()
+{
+	return s_state;
+}
+
+const volatile State* GetStatePtr()
+{
+	return &s_state;
 }
 
 void Reset()
@@ -98,87 +195,142 @@ void Reset()
 
 void StepOpcode(Common::Event* event)
 {
-	m_StepEvent.Set();
-	if (PowerPC::GetState() == PowerPC::CPU_STEPPING)
+	std::lock_guard<std::mutex> state_lock(s_state_change_lock);
+	// If we're not stepping then this is pointless
+	if (!IsStepping())
 	{
-		m_SyncEvent = event;
+		if (event)
+			event->Set();
+		return;
 	}
+
+	// Potential race where the previous step has not been serviced yet.
+	if (s_state_cpu_step_instruction_sync && s_state_cpu_step_instruction_sync != event)
+		s_state_cpu_step_instruction_sync->Set();
+
+	s_state_cpu_step_instruction = true;
+	s_state_cpu_step_instruction_sync = event;
+	s_state_cpu_cvar.notify_one();
 }
 
-void EnableStepping(const bool stepping)
+// Requires s_state_change_lock
+static bool SetStateLocked(State s)
 {
+	if (s_state == CPU_POWERDOWN)
+		return false;
+	s_state = s;
+	return true;
+}
+
+void EnableStepping(bool stepping)
+{
+	std::lock_guard<std::mutex> stepping_lock(s_stepping_lock);
+	std::unique_lock<std::mutex> state_lock(s_state_change_lock);
+
 	if (stepping)
 	{
-		PowerPC::Pause();
-		m_StepEvent.Reset();
-		Fifo::EmulatorState(false);
-		AudioCommon::ClearAudioBuffer(true);
-	}
-	else
-	{
-		// SingleStep so that the "continue", "step over" and "step out" debugger functions
-		// work when the PC is at a breakpoint at the beginning of the block
-		// If watchpoints are enabled, any instruction could be a breakpoint.
-		bool could_be_bp;
-#ifdef ENABLE_MEM_CHECK
-		could_be_bp = true;
-#else
-		could_be_bp = PowerPC::breakpoints.IsAddressBreakPoint(PC);
-#endif
-		if (could_be_bp && PowerPC::GetMode() != PowerPC::MODE_INTERPRETER)
+		SetStateLocked(CPU_STEPPING);
+
+		// Wait for the CPU Thread to leave the run loop
+		// FIXME: MsgHandler can cause this to deadlock the GUI Thread. Remove the timeout.
+		bool success = s_state_cpu_idle_cvar.wait_for(state_lock, std::chrono::seconds(5), []
 		{
-			PowerPC::CoreMode oldMode = PowerPC::GetMode();
-			PowerPC::SetMode(PowerPC::MODE_INTERPRETER);
-			PowerPC::SingleStep();
-			PowerPC::SetMode(oldMode);
-		}
-		PowerPC::Start();
-		m_StepEvent.Set();
-		Fifo::EmulatorState(true);
-		AudioCommon::ClearAudioBuffer(false);
+			return !s_state_cpu_thread_active;
+		});
+		if (!success)
+			ERROR_LOG(POWERPC, "Abandoned waiting for CPU Thread! The Core may be deadlocked.");
+
+		RunAdjacentSystems(false);
+	}
+	else if (SetStateLocked(CPU_RUNNING))
+	{
+		s_state_cpu_cvar.notify_one();
+		RunAdjacentSystems(true);
 	}
 }
 
 void Break()
 {
-	EnableStepping(true);
+	std::lock_guard<std::mutex> state_lock(s_state_change_lock);
+
+	// If another thread is trying to PauseAndLock then we need to remember this
+	// for later to ignore the unpause_on_unlock.
+	if (s_state_paused_and_locked)
+	{
+		s_state_system_request_stepping = true;
+		return;
+	}
+
+	// We'll deadlock if we synchronize, the CPU may block waiting for our caller to
+	// finish resulting in the CPU loop never terminating.
+	SetStateLocked(CPU_STEPPING);
+	RunAdjacentSystems(false);
 }
 
-bool PauseAndLock(bool do_lock, bool unpause_on_unlock)
+bool PauseAndLock(bool do_lock, bool unpause_on_unlock, bool control_adjacent)
 {
-	static bool s_have_fake_cpu_thread;
-	bool wasUnpaused = !IsStepping();
+	// NOTE: This is protected by s_stepping_lock.
+	static bool s_have_fake_cpu_thread = false;
+	bool was_unpaused = false;
+
 	if (do_lock)
 	{
-		// we can't use EnableStepping, that would causes deadlocks with both audio and video
-		PowerPC::Pause();
+		s_stepping_lock.lock();
+
+		std::unique_lock<std::mutex> state_lock(s_state_change_lock);
+		s_state_paused_and_locked = true;
+
+		was_unpaused = s_state == CPU_RUNNING;
+		SetStateLocked(CPU_STEPPING);
+
+		// FIXME: MsgHandler can cause this to deadlock the GUI Thread. Remove the timeout.
+		bool success = s_state_cpu_idle_cvar.wait_for(state_lock, std::chrono::seconds(10), []
+		{
+			return !s_state_cpu_thread_active;
+		});
+		if (!success)
+			NOTICE_LOG(POWERPC, "Abandoned CPU Thread synchronization in CPU::PauseAndLock! We'll probably crash now.");
+
+		if (control_adjacent)
+			RunAdjacentSystems(false);
+		state_lock.unlock();
+
+		// NOTE: It would make more sense for Core::DeclareAsCPUThread() to keep a
+		//   depth counter instead of being a boolean.
 		if (!Core::IsCPUThread())
 		{
-			m_csCpuOccupied.lock();
 			s_have_fake_cpu_thread = true;
 			Core::DeclareAsCPUThread();
 		}
-		else
-		{
-			s_have_fake_cpu_thread = false;
-		}
 	}
 	else
 	{
-		if (unpause_on_unlock)
-		{
-			PowerPC::Start();
-			m_StepEvent.Set();
-		}
-
+		// Only need the stepping lock for this
 		if (s_have_fake_cpu_thread)
 		{
-			Core::UndeclareAsCPUThread();
-			m_csCpuOccupied.unlock();
 			s_have_fake_cpu_thread = false;
+			Core::UndeclareAsCPUThread();
 		}
+
+		{
+			std::lock_guard<std::mutex> state_lock(s_state_change_lock);
+			if (s_state_system_request_stepping)
+			{
+				s_state_system_request_stepping = false;
+			}
+			else if (unpause_on_unlock && SetStateLocked(CPU_RUNNING))
+			{
+				was_unpaused = true;
+			}
+			s_state_paused_and_locked = false;
+			s_state_cpu_cvar.notify_one();
+
+			if (control_adjacent)
+				RunAdjacentSystems(s_state == CPU_RUNNING);
+		}
+		s_stepping_lock.unlock();
 	}
-	return wasUnpaused;
+	return was_unpaused;
 }
 
 }

Source/Core/Core/HW/CPU.h

@@ -11,6 +11,13 @@ namespace Common {
 namespace CPU
 {
 
+enum State
+{
+	CPU_RUNNING   = 0,
+	CPU_STEPPING  = 2,
+	CPU_POWERDOWN = 3
+};
+
 // Init
 void Init(int cpu_core);
 
@@ -18,32 +25,49 @@ void Init(int cpu_core);
 void Shutdown();
 
 // Starts the CPU
+// To be called by the CPU Thread.
 void Run();
 
 // Causes shutdown
+// Once started, CPU_POWERDOWN cannot be stopped.
+// Synchronizes with the CPU Thread (waits for CPU::Run to exit).
 void Stop();
 
-// Reset
+// Reset [NOT IMPLEMENTED]
 void Reset();
 
 // StepOpcode (Steps one Opcode)
 void StepOpcode(Common::Event* event = nullptr);
 
-// Enable or Disable Stepping
+// Enable or Disable Stepping. [Will deadlock if called from a system thread]
 void EnableStepping(bool stepping);
 
-// Break, same as EnableStepping(true).
+// Breakpoint activation for system threads. Similar to EnableStepping(true).
+// NOTE: Unlike EnableStepping, this does NOT synchronize with the CPU Thread
+//   which enables it to avoid deadlocks but also makes it less safe so it
+//   should not be used by the Host.
 void Break();
 
-// Is stepping ?
+// Shorthand for GetState() == CPU_STEPPING.
+// WARNING: CPU_POWERDOWN will return false, not just CPU_RUNNING.
 bool IsStepping();
 
-// Waits until is stepping and is ready for a command (paused and fully idle), and acquires a lock on that state.
-// or, if doLock is false, releases a lock on that state and optionally re-disables stepping.
-// calls must be balanced and non-recursive (once with doLock true, then once with doLock false).
-// intended (but not required) to be called from another thread,
-// e.g. when the GUI thread wants to make sure everything is paused so that it can create a savestate.
-// the return value is whether the CPU was unpaused before the call.
-bool PauseAndLock(bool do_lock, bool unpause_on_unlock = true);
+// Get current CPU Thread State
+State GetState();
+
+// Direct State Access (Raw pointer for embedding into JIT Blocks)
+// Strictly read-only. A lock is required to change the value.
+const volatile State* GetStatePtr();
+
+// Locks the CPU Thread (waiting for it to become idle).
+// While this lock is held, the CPU Thread will not perform any action so it is safe to access
+// PowerPC::ppcState, CoreTiming, etc. without racing the CPU Thread.
+// Cannot be used recursively. Must be paired as PauseAndLock(true)/PauseAndLock(false).
+// Return value for do_lock == true is whether the state was CPU_RUNNING or not.
+// Return value for do_lock == false is whether the state was changed *to* CPU_RUNNING or not.
+// Cannot be used by System threads as it will deadlock. It is threadsafe otherwise.
+// "control_adjacent" causes PauseAndLock to behave like EnableStepping by modifying the
+//   state of the Audio and FIFO subsystems as well.
+bool PauseAndLock(bool do_lock, bool unpause_on_unlock = true, bool control_adjacent = false);
 
 }

Source/Core/Core/HW/DVDInterface.cpp

@@ -478,8 +478,8 @@ static void InsertDiscCallback(u64 userdata, s64 cyclesLate)
 
 void ChangeDisc(const std::string& newFileName)
 {
-	bool is_cpu = Core::IsCPUThread();
-	bool was_unpaused = is_cpu ? false : Core::PauseAndLock(true);
+	// WARNING: Can only run on Host Thread
+	bool was_unpaused = Core::PauseAndLock(true);
 	std::string* _FileName = new std::string(newFileName);
 	CoreTiming::ScheduleEvent(0, s_eject_disc);
 	CoreTiming::ScheduleEvent(500000000, s_insert_disc, (u64)_FileName);
@@ -495,8 +495,7 @@ void ChangeDisc(const std::string& newFileName)
 		}
 		Movie::g_discChange = fileName.substr(sizeofpath);
 	}
-	if (!is_cpu)
-		Core::PauseAndLock(false, was_unpaused);
+	Core::PauseAndLock(false, was_unpaused);
 }
 
 void SetLidOpen(bool open)

Source/Core/Core/HW/DVDInterface.h

@@ -102,7 +102,7 @@ bool VolumeIsValid();
 // Disc detection and swapping
 void SetDiscInside(bool _DiscInside);
 bool IsDiscInside();
-void ChangeDisc(const std::string& fileName);
+void ChangeDisc(const std::string& fileName);  // [NOT THREADSAFE] Host only
 
 // DVD Access Functions
 bool ChangePartition(u64 offset);

Source/Core/Core/PowerPC/CachedInterpreter.cpp

@@ -7,6 +7,7 @@
 #include "Core/ConfigManager.h"
 #include "Core/CoreTiming.h"
 #include "Core/HLE/HLE.h"
+#include "Core/HW/CPU.h"
 #include "Core/PowerPC/CachedInterpreter.h"
 #include "Core/PowerPC/Gekko.h"
 #include "Core/PowerPC/PowerPC.h"
@@ -32,13 +33,10 @@ void CachedInterpreter::Shutdown()
 
 void CachedInterpreter::Run()
 {
-	while (!PowerPC::GetState())
+	while (!CPU::GetState())
 	{
 		SingleStep();
 	}
-
-	// Let the waiting thread know we are done leaving
-	PowerPC::FinishStateMove();
 }
 
 void CachedInterpreter::SingleStep()

Source/Core/Core/PowerPC/Interpreter/Interpreter.cpp

@@ -200,7 +200,7 @@ int ShowSteps = 300;
 // FastRun - inspired by GCemu (to imitate the JIT so that they can be compared).
 void Interpreter::Run()
 {
-	while (!PowerPC::GetState())
+	while (!CPU::GetState())
 	{
 		//we have to check exceptions at branches apparently (or maybe just rfi?)
 		if (SConfig::GetInstance().bEnableDebugging)
@@ -279,9 +279,6 @@ void Interpreter::Run()
 
 		CoreTiming::Advance();
 	}
-
-	// Let the waiting thread know we are done leaving
-	PowerPC::FinishStateMove();
 }
 
 void Interpreter::unknown_instruction(UGeckoInstruction _inst)

Source/Core/Core/PowerPC/Jit64/Jit.cpp

@@ -19,6 +19,7 @@
 #include "Core/CoreTiming.h"
 #include "Core/PatchEngine.h"
 #include "Core/HLE/HLE.h"
+#include "Core/HW/CPU.h"
 #include "Core/HW/GPFifo.h"
 #include "Core/HW/ProcessorInterface.h"
 #include "Core/PowerPC/JitInterface.h"
@@ -561,7 +562,7 @@ void Jit64::Jit(u32 em_address)
 		// Comment out the following to disable breakpoints (speed-up)
 		if (!Profiler::g_ProfileBlocks)
 		{
-			if (GetState() == CPU_STEPPING)
+			if (CPU::GetState() == CPU::CPU_STEPPING)
 			{
 				blockSize = 1;
 
@@ -800,7 +801,9 @@ const u8* Jit64::DoJit(u32 em_address, PPCAnalyst::CodeBuffer *code_buf, JitBloc
 				js.firstFPInstructionFound = true;
 			}
 
-			if (SConfig::GetInstance().bEnableDebugging && breakpoints.IsAddressBreakPoint(ops[i].address) && GetState() != CPU_STEPPING)
+			if (SConfig::GetInstance().bEnableDebugging &&
+			    breakpoints.IsAddressBreakPoint(ops[i].address) &&
+			    CPU::GetState() != CPU::CPU_STEPPING)
 			{
 				// Turn off block linking if there are breakpoints so that the Step Over command does not link this block.
 				jo.enableBlocklink = false;
@@ -812,7 +815,7 @@ const u8* Jit64::DoJit(u32 em_address, PPCAnalyst::CodeBuffer *code_buf, JitBloc
 				ABI_PushRegistersAndAdjustStack({}, 0);
 				ABI_CallFunction(reinterpret_cast<void *>(&PowerPC::CheckBreakPoints));
 				ABI_PopRegistersAndAdjustStack({}, 0);
-				TEST(32, M(PowerPC::GetStatePtr()), Imm32(0xFFFFFFFF));
+				TEST(32, M(CPU::GetStatePtr()), Imm32(0xFFFFFFFF));
 				FixupBranch noBreakpoint = J_CC(CC_Z);
 
 				WriteExit(ops[i].address);

Source/Core/Core/PowerPC/Jit64/JitAsm.cpp

@@ -8,6 +8,7 @@
 #include "Common/x64Emitter.h"
 #include "Core/ConfigManager.h"
 #include "Core/CoreTiming.h"
+#include "Core/HW/CPU.h"
 #include "Core/HW/Memmap.h"
 #include "Core/PowerPC/PowerPC.h"
 #include "Core/PowerPC/Jit64/Jit.h"
@@ -75,12 +76,12 @@ void Jit64AsmRoutineManager::Generate()
 
 			if (SConfig::GetInstance().bEnableDebugging)
 			{
-				TEST(32, M(PowerPC::GetStatePtr()), Imm32(PowerPC::CPU_STEPPING));
+				TEST(32, M(CPU::GetStatePtr()), Imm32(CPU::CPU_STEPPING));
 				FixupBranch notStepping = J_CC(CC_Z);
 				ABI_PushRegistersAndAdjustStack({}, 0);
 				ABI_CallFunction(reinterpret_cast<void *>(&PowerPC::CheckBreakPoints));
 				ABI_PopRegistersAndAdjustStack({}, 0);
-				TEST(32, M(PowerPC::GetStatePtr()), Imm32(0xFFFFFFFF));
+				TEST(32, M(CPU::GetStatePtr()), Imm32(0xFFFFFFFF));
 				dbg_exit = J_CC(CC_NZ, true);
 				SetJumpTarget(notStepping);
 			}
@@ -208,7 +209,7 @@ void Jit64AsmRoutineManager::Generate()
 
 		// Check the state pointer to see if we are exiting
 		// Gets checked on at the end of every slice
-		TEST(32, M(PowerPC::GetStatePtr()), Imm32(0xFFFFFFFF));
+		TEST(32, M(CPU::GetStatePtr()), Imm32(0xFFFFFFFF));
 		J_CC(CC_Z, outerLoop);
 
 	//Landing pad for drec space
@@ -221,11 +222,6 @@ void Jit64AsmRoutineManager::Generate()
 		POP(RSP);
 	}
 
-	// Let the waiting thread know we are done leaving
-	ABI_PushRegistersAndAdjustStack({}, 0);
-	ABI_CallFunction(reinterpret_cast<void *>(&PowerPC::FinishStateMove));
-	ABI_PopRegistersAndAdjustStack({}, 0);
-
 	ABI_PopRegistersAndAdjustStack(ABI_ALL_CALLEE_SAVED, 8, 16);
 	RET();
 

Source/Core/Core/PowerPC/Jit64/Jit_LoadStore.cpp

@@ -12,6 +12,7 @@
 #include "Common/x64Emitter.h"
 #include "Core/ConfigManager.h"
 #include "Core/CoreTiming.h"
+#include "Core/HW/CPU.h"
 #include "Core/HW/DSP.h"
 #include "Core/HW/Memmap.h"
 #include "Core/PowerPC/JitInterface.h"
@@ -120,7 +121,7 @@ void Jit64::lXXx(UGeckoInstruction inst)
 	// TODO: We shouldn't use a debug read here.  It should be possible to get
 	// the following instructions out of the JIT state.
 	if (SConfig::GetInstance().bSkipIdle &&
-	    PowerPC::GetState() != PowerPC::CPU_STEPPING &&
+	    CPU::GetState() != CPU::CPU_STEPPING &&
 	    inst.OPCD == 32 &&
 	    MergeAllowedNextInstructions(2) &&
 	    (inst.hex & 0xFFFF0000) == 0x800D0000 &&