RawSPU: protect NPC from writes/reads in running state

rpcs3/Emu/Cell/Modules/cellSpursSpu.cpp

@@ -1787,7 +1787,6 @@ void spursTasksetDispatch(spu_thread& spu)
 		if (elfAddr & 2)
 		{
 			// TODO: Figure this out
-			spu.status |= SPU_STATUS_STOPPED_BY_STOP;
 			spu_runtime::g_escape(&spu);
 		}
 
@@ -1841,7 +1840,6 @@ void spursTasksetDispatch(spu_thread& spu)
 		if (elfAddr & 2)
 		{
 			// TODO: Figure this out
-			spu.status |= SPU_STATUS_STOPPED_BY_STOP;
 			spu_runtime::g_escape(&spu);
 		}
 

rpcs3/Emu/Cell/RawSPUThread.cpp

@@ -8,14 +8,14 @@
 
 inline void try_start(spu_thread& spu)
 {
-	if (spu.status.fetch_op([](u32& status)
+	if (spu.status_npc.fetch_op([](typename spu_thread::status_npc_sync_var& value)
 	{
-		if (status & SPU_STATUS_RUNNING)
+		if (value.status & SPU_STATUS_RUNNING)
 		{
 			return false;
 		}
 
-		status = SPU_STATUS_RUNNING;
+		value.status = SPU_STATUS_RUNNING;
 		return true;
 	}).second)
 	{
@@ -124,7 +124,7 @@ bool spu_thread::read_reg(const u32 addr, u32& value)
 
 	case SPU_Status_offs:
 	{
-		value = status;
+		value = status_npc.load().status;
 		return true;
 	}
 
@@ -136,8 +136,8 @@ bool spu_thread::read_reg(const u32 addr, u32& value)
 
 	case SPU_NPC_offs:
 	{
-		//npc = pc | ((ch_event_stat & SPU_EVENT_INTR_ENABLED) != 0);
+		const auto current = status_npc.load();
-		value = npc;
+		value = !(current.status & SPU_STATUS_RUNNING) ? current.npc : 0;
 		return true;
 	}
 
@@ -238,13 +238,15 @@ bool spu_thread::write_reg(const u32 addr, const u32 value)
 
 	case SPU_RunCntl_offs:
 	{
+		run_ctrl = value;
+
 		if (value == SPU_RUNCNTL_RUN_REQUEST)
 		{
 			try_start(*this);
 		}
 		else if (value == SPU_RUNCNTL_STOP_REQUEST)
 		{
-			status &= ~SPU_STATUS_RUNNING;
+			// TODO: Wait for the SPU to stop?
 			state += cpu_flag::stop;
 		}
 		else
@@ -252,18 +254,22 @@ bool spu_thread::write_reg(const u32 addr, const u32 value)
 			break;
 		}
 
-		run_ctrl = value;
 		return true;
 	}
 
 	case SPU_NPC_offs:
 	{
-		if ((value & 2) || value >= 0x40000)
+		status_npc.fetch_op([value = value & 0x3fffd](status_npc_sync_var& state)
 		{
-			break;
+			if (!(state.status & SPU_STATUS_RUNNING))
-		}
+			{
+				state.npc = value;
+				return true;
+			}
+
+			return false;
+		});
 
-		npc = value;
 		return true;
 	}
 
@@ -300,5 +306,5 @@ void spu_load_exec(const spu_exec_object& elf)
 		}
 	}
 
-	spu->npc = elf.header.e_entry;
+	spu->status_npc = {SPU_STATUS_RUNNING, elf.header.e_entry};
 }

rpcs3/Emu/Cell/SPUASMJITRecompiler.cpp

@@ -3399,7 +3399,6 @@ void spu_recompiler::HGT(spu_opcode_t op)
 		c->lea(addr->r64(), get_pc(pos));
 		c->and_(*addr, 0x3fffc);
 		c->mov(SPU_OFF_32(pc), *addr);
-		c->lock().bts(SPU_OFF_32(status), 2);
 		c->mov(addr->r64(), reinterpret_cast<u64>(vm::base(0xffdead00)));
 		c->mov(asmjit::x86::dword_ptr(addr->r64()), "HALT"_u32);
 		c->jmp(ret);
@@ -3741,7 +3740,6 @@ void spu_recompiler::HLGT(spu_opcode_t op)
 		c->lea(addr->r64(), get_pc(pos));
 		c->and_(*addr, 0x3fffc);
 		c->mov(SPU_OFF_32(pc), *addr);
-		c->lock().bts(SPU_OFF_32(status), 2);
 		c->mov(addr->r64(), reinterpret_cast<u64>(vm::base(0xffdead00)));
 		c->mov(asmjit::x86::dword_ptr(addr->r64()), "HALT"_u32);
 		c->jmp(ret);
@@ -4071,7 +4069,6 @@ void spu_recompiler::HEQ(spu_opcode_t op)
 		c->lea(addr->r64(), get_pc(pos));
 		c->and_(*addr, 0x3fffc);
 		c->mov(SPU_OFF_32(pc), *addr);
-		c->lock().bts(SPU_OFF_32(status), 2);
 		c->mov(addr->r64(), reinterpret_cast<u64>(vm::base(0xffdead00)));
 		c->mov(asmjit::x86::dword_ptr(addr->r64()), "HALT"_u32);
 		c->jmp(ret);
@@ -4593,7 +4590,6 @@ void spu_recompiler::HGTI(spu_opcode_t op)
 		c->lea(addr->r64(), get_pc(pos));
 		c->and_(*addr, 0x3fffc);
 		c->mov(SPU_OFF_32(pc), *addr);
-		c->lock().bts(SPU_OFF_32(status), 2);
 		c->mov(addr->r64(), reinterpret_cast<u64>(vm::base(0xffdead00)));
 		c->mov(asmjit::x86::dword_ptr(addr->r64()), "HALT"_u32);
 		c->jmp(ret);
@@ -4638,7 +4634,6 @@ void spu_recompiler::HLGTI(spu_opcode_t op)
 		c->lea(addr->r64(), get_pc(pos));
 		c->and_(*addr, 0x3fffc);
 		c->mov(SPU_OFF_32(pc), *addr);
-		c->lock().bts(SPU_OFF_32(status), 2);
 		c->mov(addr->r64(), reinterpret_cast<u64>(vm::base(0xffdead00)));
 		c->mov(asmjit::x86::dword_ptr(addr->r64()), "HALT"_u32);
 		c->jmp(ret);
@@ -4701,7 +4696,6 @@ void spu_recompiler::HEQI(spu_opcode_t op)
 		c->lea(addr->r64(), get_pc(pos));
 		c->and_(*addr, 0x3fffc);
 		c->mov(SPU_OFF_32(pc), *addr);
-		c->lock().bts(SPU_OFF_32(status), 2);
 		c->mov(addr->r64(), reinterpret_cast<u64>(vm::base(0xffdead00)));
 		c->mov(asmjit::x86::dword_ptr(addr->r64()), "HALT"_u32);
 		c->jmp(ret);

rpcs3/Emu/Cell/SPURecompiler.cpp

@@ -7773,9 +7773,6 @@ public:
 			m_ir->CreateStore(&*(m_function->arg_begin() + 2), spu_ptr<u32>(&spu_thread::pc))->setVolatile(true);
 		else
 			update_pc();
-		const auto pstatus = spu_ptr<u32>(&spu_thread::status);
-		const auto chalt = m_ir->getInt32(SPU_STATUS_STOPPED_BY_HALT);
-		m_ir->CreateAtomicRMW(llvm::AtomicRMWInst::Or, pstatus, chalt, llvm::AtomicOrdering::Release)->setVolatile(true);
 		const auto ptr = _ptr<u32>(m_memptr, 0xffdead00);
 		m_ir->CreateStore(m_ir->getInt32("HALT"_u32), ptr)->setVolatile(true);
 		m_ir->CreateBr(next);

rpcs3/Emu/Cell/SPUThread.cpp

@@ -1067,9 +1067,7 @@ void spu_thread::cpu_init()
 	}
 
 	run_ctrl.raw() = 0;
-	status.raw() = 0;
+	status_npc.raw() = {};
-	npc.raw() = 0;
-	skip_npc_set = false;
 
 	int_ctrl[0].clear();
 	int_ctrl[1].clear();
@@ -1082,15 +1080,19 @@ void spu_thread::cpu_stop()
 {
 	if (!group && offset >= RAW_SPU_BASE_ADDR)
 	{
-		// Save next PC and current SPU Interrupt Status
+		status_npc.fetch_op([this](status_npc_sync_var& state)
-		if (skip_npc_set)
 		{
-			skip_npc_set = false;
+			if (state.status & SPU_STATUS_RUNNING)
-		}
+			{
-		else
+				// Save next PC and current SPU Interrupt Status
-		{
+				// Used only by RunCtrl stop requests
-			npc = pc | interrupts_enabled;
+				state.status &= ~SPU_STATUS_RUNNING;
-		}
+				state.npc =	pc | +interrupts_enabled;
+				return true;
+			}
+
+			return false;
+		});
 	}
 	else if (group && is_stopped())
 	{
@@ -1128,9 +1130,7 @@ extern thread_local std::string(*g_tls_log_prefix)();
 void spu_thread::cpu_task()
 {
 	// Get next PC and SPU Interrupt status
-	pc = npc.exchange(0);
+	pc = status_npc.load().npc;
-
-	skip_npc_set = false;
 
 	// Note: works both on RawSPU and threaded SPU!
 	set_interrupt_status((pc & 1) != 0);
@@ -2786,14 +2786,13 @@ bool spu_thread::stop_and_signal(u32 code)
 	if (offset >= RAW_SPU_BASE_ADDR)
 	{
 		// Save next PC and current SPU Interrupt Status
-		npc = (pc + 4) | (interrupts_enabled);
-		skip_npc_set = true;
 		state += cpu_flag::stop + cpu_flag::wait;
-		status.atomic_op([code](u32& status)
+		status_npc.atomic_op([&](status_npc_sync_var& state)
 		{
-			status = (status & 0xffff) | (code << 16);
+			state.status = (state.status & 0xffff) | (code << 16);
-			status |= SPU_STATUS_STOPPED_BY_STOP;
+			state.status |= SPU_STATUS_STOPPED_BY_STOP;
-			status &= ~SPU_STATUS_RUNNING;
+			state.status &= ~SPU_STATUS_RUNNING;
+			state.npc = (pc + 4) | +interrupts_enabled;
 		});
 
 		int_ctrl[2].set(SPU_INT2_STAT_SPU_STOP_AND_SIGNAL_INT);
@@ -3118,7 +3117,7 @@ bool spu_thread::stop_and_signal(u32 code)
 		}
 
 		spu_log.trace("sys_spu_thread_exit(status=0x%x)", ch_out_mbox.get_value());
-		status |= SPU_STATUS_STOPPED_BY_STOP;
+		status_npc = {SPU_STATUS_STOPPED_BY_STOP, 0};
 		state += cpu_flag::stop;
 		check_state();
 		return true;
@@ -3143,10 +3142,11 @@ void spu_thread::halt()
 	{
 		state += cpu_flag::stop + cpu_flag::wait;
 
-		status.atomic_op([](u32& status)
+		status_npc.atomic_op([this](status_npc_sync_var& state)
 		{
-			status |= SPU_STATUS_STOPPED_BY_HALT;
+			state.status |= SPU_STATUS_STOPPED_BY_HALT;
-			status &= ~SPU_STATUS_RUNNING;
+			state.status &= ~SPU_STATUS_RUNNING;
+			state.npc = pc | +interrupts_enabled;
 		});
 
 		int_ctrl[2].set(SPU_INT2_STAT_SPU_HALT_OR_STEP_INT);
@@ -3154,7 +3154,6 @@ void spu_thread::halt()
 		spu_runtime::g_escape(this);
 	}
 
-	status |= SPU_STATUS_STOPPED_BY_HALT;
 	fmt::throw_exception("Halt" HERE);
 }
 

rpcs3/Emu/Cell/SPUThread.h

@@ -577,14 +577,18 @@ public:
 	atomic_t<u32> ch_event_stat;
 	atomic_t<bool> interrupts_enabled;
 
-	bool skip_npc_set = false;
-
 	u64 ch_dec_start_timestamp; // timestamp of writing decrementer value
 	u32 ch_dec_value; // written decrementer value
 
 	atomic_t<u32> run_ctrl; // SPU Run Control register (only provided to get latest data written)
-	atomic_t<u32> status; // SPU Status register
+
-	atomic_t<u32> npc; // SPU Next Program Counter register
+	struct alignas(8) status_npc_sync_var
+	{
+		u32 status; // SPU Status register
+		u32 npc; // SPU Next Program Counter register
+	};
+
+	atomic_t<status_npc_sync_var> status_npc; 
 
 	std::array<spu_int_ctrl_t, 3> int_ctrl; // SPU Class 0, 1, 2 Interrupt Management
 

rpcs3/Emu/Cell/lv2/sys_spu.cpp

@@ -433,7 +433,7 @@ error_code sys_spu_thread_get_exit_status(ppu_thread& ppu, u32 id, vm::ptr<u32>
 		return CELL_ESRCH;
 	}
 
-	if (thread->status & SPU_STATUS_STOPPED_BY_STOP)
+	if (thread->status_npc.load().status & SPU_STATUS_STOPPED_BY_STOP)
 	{
 		*status = thread->ch_out_mbox.get_value();
 		return CELL_OK;
@@ -668,13 +668,12 @@ error_code sys_spu_thread_group_start(ppu_thread& ppu, u32 id)
 			sys_spu_image::deploy(thread->offset, img.second.data(), img.first.nsegs);
 
 			thread->cpu_init();
-			thread->npc = img.first.entry_point;
 			thread->gpr[3] = v128::from64(0, args[0]);
 			thread->gpr[4] = v128::from64(0, args[1]);
 			thread->gpr[5] = v128::from64(0, args[2]);
 			thread->gpr[6] = v128::from64(0, args[3]);
 
-			thread->status.exchange(SPU_STATUS_RUNNING);
+			thread->status_npc = {SPU_STATUS_RUNNING, img.first.entry_point};
 		}
 	}