From 574a37814c07883284d44409aecc9dba0a29e0a7 Mon Sep 17 00:00:00 2001
From: Thomas Harte <thomas.harte@gmail.com>
Date: Tue, 8 Dec 2020 18:46:30 -0500
Subject: [PATCH] Attempts to fix exception selection and timing.

---
 .../Implementation/65816Implementation.hpp    | 122 +++++++++---------
 .../65816/Implementation/65816Storage.cpp     |  51 ++++----
 .../65816/Implementation/65816Storage.hpp     |  13 +-
 3 files changed, 103 insertions(+), 83 deletions(-)

diff --git a/Processors/65816/Implementation/65816Implementation.hpp b/Processors/65816/Implementation/65816Implementation.hpp
index 0a910f158..d53a5c652 100644
--- a/Processors/65816/Implementation/65816Implementation.hpp
+++ b/Processors/65816/Implementation/65816Implementation.hpp
@@ -48,8 +48,19 @@ template <typename BusHandler, bool uses_ready_line> void Processor<BusHandler,
 					// The exception program will determine the appropriate way to respond
 					// based on the pending exception if one exists; otherwise just do a
 					// standard fetch-decode-execute.
-					const size_t slot = size_t(selected_exceptions_ ? OperationSlot::Exception : OperationSlot::FetchDecodeExecute);
-					active_instruction_ = &instructions[slot];
+					if(selected_exceptions_) {
+						exception_is_interrupt_ = true;
+
+						// Do enough quick early decoding to spot a reset.
+						if(selected_exceptions_ & (Reset | PowerOn)) {
+							active_instruction_ = &instructions[size_t(OperationSlot::Reset)];
+						} else {
+							active_instruction_ = &instructions[size_t(OperationSlot::Exception)];
+						}
+					} else {
+						exception_is_interrupt_ = false;
+						active_instruction_ = &instructions[size_t(OperationSlot::FetchDecodeExecute)];
+					}
 
 					next_op_ = &micro_ops_[active_instruction_->program_offsets[0]];
 					instruction_buffer_.clear();
@@ -199,7 +210,7 @@ template <typename BusHandler, bool uses_ready_line> void Processor<BusHandler,
 				//
 
 				case CycleRepeatingNone:
-					if(pending_exceptions_ & required_exceptions_) {
+					if(selected_exceptions_ & required_exceptions_) {
 						continue;
 					} else {
 						--next_op_;
@@ -243,6 +254,10 @@ template <typename BusHandler, bool uses_ready_line> void Processor<BusHandler,
 					registers_.pc = uint16_t(data_buffer_.value);
 				continue;
 
+				case OperationClearDataBuffer:
+					data_buffer_.clear();
+				continue;
+
 				//
 				// Address construction.
 				//
@@ -390,62 +405,10 @@ template <typename BusHandler, bool uses_ready_line> void Processor<BusHandler,
 					data_buffer_.size = 2;
 				continue;
 
-				case OperationPrepareException: {
-					// Put the proper exception vector into the data address, put the flags and PC
-					// into the data buffer (possibly also PBR), and skip an instruction if in
-					// emulation mode.
-					//
-					// I've assumed here that interrupts, BRKs and COPs can be usurped similarly
-					// to a 6502 but may not have the exact details correct. E.g. if IRQ has
-					// become inactive since the decision was made to start an interrupt, should
-					// that turn into a BRK?
-					//
-					// Also: priority here is a guess.
-
-					bool is_brk = false;
-
-					if(pending_exceptions_ & (Reset | PowerOn)) {
-						pending_exceptions_ &= ~(Reset | PowerOn);
-						data_address_ = 0xfffc;
-						set_reset_state();
-
-						// Also switch tracks to the reset program, and don't load up the
-						// data buffer. set_reset_state() will already have fixed the
-						// interrupt and decimal flags.
-						active_instruction_ = &instructions[size_t(OperationSlot::ResetTail)];
-						next_op_ = &micro_ops_[active_instruction_->program_offsets[0]];
-						continue;
-					} else if(pending_exceptions_ & NMI) {
-						pending_exceptions_ &= ~NMI;
-						data_address_ = registers_.emulation_flag ? 0xfffa : 0xffea;
-					} else if(pending_exceptions_ & IRQ & registers_.flags.inverse_interrupt) {
-						// TODO: this isn't a correct way to handle usurption, I think;
-						// if an IRQ was selected for servicing I think it'll now be servied
-						// even if the IRQ line has gone low in the interim.
-						pending_exceptions_ &= ~IRQ;
-						data_address_ = registers_.emulation_flag ? 0xfffe : 0xffee;
-					} else if(pending_exceptions_ & Abort) {
-						// Special case: restore registers from start of instruction.
-						registers_ = abort_registers_copy_;
-
-						pending_exceptions_ &= ~Abort;
-						data_address_ = registers_.emulation_flag ? 0xfff8 : 0xffe8;;
-					} else {
-						// Test that this really is BRK or COP.
-						assert((active_instruction_ == instructions) || (active_instruction_ == &instructions[0x02]));
-
-						is_brk = active_instruction_ == instructions;	// Given that BRK has opcode 00.
-						if(is_brk) {
-							data_address_ = registers_.emulation_flag ? 0xfffe : 0xffe6;
-						} else {
-							// Implicitly: COP.
-							data_address_ = registers_.emulation_flag ? 0xfff4 : 0xffe4;
-						}
-					}
-
+				case OperationPrepareException:
 					data_buffer_.value = uint32_t((registers_.pc << 8) | get_flags());
 					if(registers_.emulation_flag) {
-						if(is_brk) data_buffer_.value |= Flag::Break;
+						if(!exception_is_interrupt_) data_buffer_.value |= Flag::Break;
 						data_buffer_.size = 3;
 						++next_op_;
 					} else {
@@ -456,7 +419,50 @@ template <typename BusHandler, bool uses_ready_line> void Processor<BusHandler,
 
 					registers_.flags.inverse_interrupt = 0;
 					registers_.flags.decimal = 0;
-				} continue;
+				continue;
+
+				case OperationPickExceptionVector:
+					// Priority for abort and reset here is a guess.
+
+					if(pending_exceptions_ & (Reset | PowerOn)) {
+						pending_exceptions_ &= ~(Reset | PowerOn);
+						data_address_ = 0xfffc;
+						set_reset_state();
+						continue;
+					}
+
+					if(pending_exceptions_ & Abort) {
+						// Special case: restore registers from start of instruction.
+						registers_ = abort_registers_copy_;
+
+						pending_exceptions_ &= ~Abort;
+						data_address_ = registers_.emulation_flag ? 0xfff8 : 0xffe8;
+						continue;
+					}
+
+					if(pending_exceptions_ & NMI) {
+						pending_exceptions_ &= ~NMI;
+						data_address_ = registers_.emulation_flag ? 0xfffa : 0xffea;
+						continue;
+					}
+
+					// Last chance saloon for the interrupt process.
+					if(exception_is_interrupt_) {
+						data_address_ = registers_.emulation_flag ? 0xfffe : 0xffee;
+						continue;
+					}
+
+					// ... then this must be a BRK or COP that is being treated as such.
+					assert((active_instruction_ == instructions) || (active_instruction_ == &instructions[0x02]));
+
+					// Test for BRK, given that it has opcode 00.
+					if(active_instruction_ == instructions) {
+						data_address_ = registers_.emulation_flag ? 0xfffe : 0xffe6;
+					} else {
+						// Implicitly: COP.
+						data_address_ = registers_.emulation_flag ? 0xfff4 : 0xffe4;
+					}
+				continue;
 
 				//
 				// Performance.
diff --git a/Processors/65816/Implementation/65816Storage.cpp b/Processors/65816/Implementation/65816Storage.cpp
index 6860e240e..2978cb2b7 100644
--- a/Processors/65816/Implementation/65816Storage.cpp
+++ b/Processors/65816/Implementation/65816Storage.cpp
@@ -89,16 +89,16 @@ struct CPU::WDC65816::ProcessorStorageConstructor {
 		++opcode;
 	}
 
-	void set_exception_generator(Generator generator, Generator reset_tail_generator) {
+	void set_exception_generator(Generator generator, Generator reset_generator) {
 		const auto map_entry = install(generator);
 		storage_.instructions[size_t(ProcessorStorage::OperationSlot::Exception)].program_offsets[0] =
 		storage_.instructions[size_t(ProcessorStorage::OperationSlot::Exception)].program_offsets[1] = uint16_t(map_entry->second.first);
 		storage_.instructions[size_t(ProcessorStorage::OperationSlot::Exception)].operation = JMPind;
 
-		const auto reset_tail_entry = install(reset_tail_generator);
-		storage_.instructions[size_t(ProcessorStorage::OperationSlot::ResetTail)].program_offsets[0] =
-		storage_.instructions[size_t(ProcessorStorage::OperationSlot::ResetTail)].program_offsets[1] = uint16_t(reset_tail_entry->second.first);
-		storage_.instructions[size_t(ProcessorStorage::OperationSlot::ResetTail)].operation = JMPind;
+		const auto reset_tail_entry = install(reset_generator);
+		storage_.instructions[size_t(ProcessorStorage::OperationSlot::Reset)].program_offsets[0] =
+		storage_.instructions[size_t(ProcessorStorage::OperationSlot::Reset)].program_offsets[1] = uint16_t(reset_tail_entry->second.first);
+		storage_.instructions[size_t(ProcessorStorage::OperationSlot::Reset)].operation = JMPind;
 	}
 
 	void install_fetch_decode_execute() {
@@ -617,7 +617,9 @@ struct CPU::WDC65816::ProcessorStorageConstructor {
 	}
 
 	// 22a. Stack; s, abort/irq/nmi/res.
-	static void stack_exception(AccessType, bool, const std::function<void(MicroOp)> &target) {
+	//
+	// Combined here with reset, which is the same sequence but with a different stack access.
+	static void stack_exception_impl(AccessType, bool, const std::function<void(MicroOp)> &target, MicroOp stack_op) {
 		target(CycleFetchPCThrowaway);		// IO.
 		target(CycleFetchPCThrowaway);		// IO.
 
@@ -625,10 +627,15 @@ struct CPU::WDC65816::ProcessorStorageConstructor {
 											// reset then switches to the reset tail program.
 											// Otherwise skips a micro-op if in emulation mode.
 
-		target(CyclePush);					// PBR	[skipped in emulation mode].
-		target(CyclePush);					// PCH.
-		target(CyclePush);					// PCL.
-		target(CyclePush);					// P.
+		target(stack_op);					// PBR	[skipped in emulation mode].
+		target(stack_op);					// PCH.
+		target(stack_op);					// PCL.
+
+		target(OperationPickExceptionVector);	// Select vector address.
+
+		target(stack_op);					// P.
+
+		target(OperationClearDataBuffer);	// Prepare to fetch the vector.
 
 		target(CycleFetchIncrementVector);	// AAVL.
 		target(CycleFetchVector);			// AAVH.
@@ -636,18 +643,13 @@ struct CPU::WDC65816::ProcessorStorageConstructor {
 		target(OperationPerform);			// Jumps to the vector address.
 	}
 
-	static void reset_tail(AccessType, bool, const std::function<void(MicroOp)> &target) {
-		// The reset program still walks through three stack accesses as if it were doing
-		// the usual exception stack activity, but forces them to reads that don't modify
-		// the stack pointer. Here they are:
-		target(CycleAccessStack);			// PCH.
-		target(CycleAccessStack);			// PCL.
-		target(CycleAccessStack);			// P.
+	static void stack_exception(AccessType type, bool is8bit, const std::function<void(MicroOp)> &target) {
+		stack_exception_impl(type, is8bit, target, CyclePush);
+	}
 
-		target(CycleFetchIncrementVector);	// AAVL.
-		target(CycleFetchVector);			// AAVH.
-
-		target(OperationPerform);			// Jumps to the vector address.
+	static void reset(AccessType type, bool is8bit, const std::function<void(MicroOp)> &target) {
+		// The reset program just disables the usual pushes.
+		stack_exception_impl(type, is8bit, target, CycleAccessStack);
 	}
 
 	// 22b. Stack; s, PLx.
@@ -754,6 +756,11 @@ struct CPU::WDC65816::ProcessorStorageConstructor {
 		target(CyclePush);					// PBR	[skipped in emulation mode].
 		target(CyclePush);					// PCH.
 		target(CyclePush);					// PCL.
+
+		target(OperationPickExceptionVector);	// Selects the exception's target; I've assumed the same
+												// logic as a 6502 here — i.e. that some interrupts can
+												// usurp the vectors of others.
+
 		target(CyclePush);					// P.
 
 		target(CycleFetchIncrementVector);	// AAVL.
@@ -1069,7 +1076,7 @@ ProcessorStorage::ProcessorStorage() {
 	/* 0xff SBC al, x */		op(absolute_long_x, SBC);
 #undef op
 
-	constructor.set_exception_generator(&ProcessorStorageConstructor::stack_exception, &ProcessorStorageConstructor::reset_tail);
+	constructor.set_exception_generator(&ProcessorStorageConstructor::stack_exception, &ProcessorStorageConstructor::reset);
 	constructor.install_fetch_decode_execute();
 
 	// Find any OperationMoveToNextProgram.
diff --git a/Processors/65816/Implementation/65816Storage.hpp b/Processors/65816/Implementation/65816Storage.hpp
index ccbb67f06..7ea71ffac 100644
--- a/Processors/65816/Implementation/65816Storage.hpp
+++ b/Processors/65816/Implementation/65816Storage.hpp
@@ -148,11 +148,17 @@ enum MicroOp: uint8_t {
 	/// Copies the data buffer to A.
 	OperationCopyDataToA,
 
+	/// Clears the data buffer.
+	OperationClearDataBuffer,
+
 	/// Fills the data buffer with three or four bytes, depending on emulation mode, containing the program
-	/// counter, flags and possibly the program bank. Also puts the appropriate vector address into the
-	/// address register.
+	/// counter, flags and possibly the program bank. Skips the next operation if only three are filled.
 	OperationPrepareException,
 
+	/// Picks the appropriate vector address to service the current exception and places it into
+	/// the data address register.
+	OperationPickExceptionVector,
+
 	/// Sets the memory lock output for the rest of this instruction.
 	OperationSetMemoryLock,
 
@@ -250,7 +256,7 @@ struct ProcessorStorage {
 
 	enum class OperationSlot {
 		Exception = 256,
-		ResetTail,
+		Reset,
 		FetchDecodeExecute,
 	};
 
@@ -302,6 +308,7 @@ struct ProcessorStorage {
 	static constexpr int default_exceptions = PowerOn;
 	int pending_exceptions_ = default_exceptions;
 	int selected_exceptions_ = default_exceptions;
+	bool exception_is_interrupt_ = false;
 
 	bool ready_line_ = false;
 	bool memory_lock_ = false;