From 84c017074a6be684cba00b848a3f0432525cd67e Mon Sep 17 00:00:00 2001 From: Uwe Seimet <48174652+uweseimet@users.noreply.github.com> Date: Sun, 6 Nov 2022 19:44:44 +0100 Subject: [PATCH] Added command byte count to SCSI command mapping (#966) * Extended mapping table with command byte count * Terminate early if command is unknown --- cpp/controllers/scsi_controller.cpp | 9 ++- cpp/devices/primary_device.cpp | 2 +- cpp/hal/bus.cpp | 14 ++--- cpp/hal/bus.h | 3 +- cpp/hal/gpiobus.cpp | 29 +++++----- cpp/hal/gpiobus.h | 2 +- cpp/rascsi/rascsi_core.cpp | 4 +- cpp/scsi.h | 86 ++++++++++++++--------------- cpp/test/bus_test.cpp | 68 ++++++++++++++--------- cpp/test/mocks.h | 6 +- 10 files changed, 121 insertions(+), 102 deletions(-) diff --git a/cpp/controllers/scsi_controller.cpp b/cpp/controllers/scsi_controller.cpp index e53fab05..34188a55 100644 --- a/cpp/controllers/scsi_controller.cpp +++ b/cpp/controllers/scsi_controller.cpp @@ -199,7 +199,14 @@ void ScsiController::Command() GetBus().SetCD(true); GetBus().SetIO(false); - const int actual_count = GetBus().CommandHandShake(GetBuffer().data()); + const int actual_count = GetBus().CommandHandShake(GetBuffer()); + if (actual_count == 0) { + LOGTRACE("ID %d LUN %d received unknown command: $%02X", GetTargetId(), GetEffectiveLun(), GetBuffer()[0]) + + Error(sense_key::ILLEGAL_REQUEST, asc::INVALID_COMMAND_OPERATION_CODE); + return; + } + const int command_byte_count = BUS::GetCommandByteCount(GetBuffer()[0]); // If not able to receive all, move to the status phase diff --git a/cpp/devices/primary_device.cpp b/cpp/devices/primary_device.cpp index 94545636..14a21b50 100644 --- a/cpp/devices/primary_device.cpp +++ b/cpp/devices/primary_device.cpp @@ -42,7 +42,7 @@ void PrimaryDevice::AddCommand(scsi_command opcode, const operation& execute) void PrimaryDevice::Dispatch(scsi_command cmd) { if (const auto& it = commands.find(cmd); it != commands.end()) { - LOGDEBUG("Executing %s ($%02X)", command_names.find(cmd)->second, static_cast(cmd)) + LOGDEBUG("Executing %s ($%02X)", command_mapping.find(cmd)->second.second, static_cast(cmd)) it->second(); } diff --git a/cpp/hal/bus.cpp b/cpp/hal/bus.cpp index e791940f..19dfe926 100644 --- a/cpp/hal/bus.cpp +++ b/cpp/hal/bus.cpp @@ -11,24 +11,18 @@ #include "bus.h" using namespace std; +using namespace scsi_defs; //--------------------------------------------------------------------------- // // Get the number of bytes for a command -// TODO Add the byte count to the enum value/command name mapping and remove the comparisons // //--------------------------------------------------------------------------- int BUS::GetCommandByteCount(uint8_t opcode) { - if (opcode == 0x88 || opcode == 0x8A || opcode == 0x8F || opcode == 0x91 || opcode == 0x9E || opcode == 0x9F) { - return 16; - } else if (opcode == 0xA0) { - return 12; - } else if (opcode >= 0x20 && opcode <= 0x7D) { - return 10; - } else { - return 6; - } + const auto& mapping = command_mapping.find(static_cast(opcode)); + + return mapping != command_mapping.end() ? mapping->second.first : 0; } //--------------------------------------------------------------------------- diff --git a/cpp/hal/bus.h b/cpp/hal/bus.h index 641e7cfb..0400efaf 100644 --- a/cpp/hal/bus.h +++ b/cpp/hal/bus.h @@ -12,6 +12,7 @@ #include "scsi.h" #include #include +#include #include #include "hal/board_type.h" @@ -93,7 +94,7 @@ public: virtual bool GetDP() const = 0; // Get parity signal virtual uint32_t Acquire() = 0; - virtual int CommandHandShake(uint8_t *buf) = 0; + virtual int CommandHandShake(vector&) = 0; virtual int ReceiveHandShake(uint8_t *buf, int count) = 0; virtual int SendHandShake(uint8_t *buf, int count, int delay_after_bytes) = 0; diff --git a/cpp/hal/gpiobus.cpp b/cpp/hal/gpiobus.cpp index c5439a4f..971cc0dc 100644 --- a/cpp/hal/gpiobus.cpp +++ b/cpp/hal/gpiobus.cpp @@ -509,7 +509,7 @@ uint32_t GPIOBUS::GetPinRaw(uint32_t raw_data, board_type::pi_physical_pin_e pin // Receive command handshake // //--------------------------------------------------------------------------- -int GPIOBUS::CommandHandShake(uint8_t *buf) +int GPIOBUS::CommandHandShake(vector& buf) { GPIO_FUNCTION_TRACE // Only works in TARGET mode @@ -529,7 +529,7 @@ int GPIOBUS::CommandHandShake(uint8_t *buf) SysTimer::SleepNsec(SCSI_DELAY_BUS_SETTLE_DELAY_NS); // Get data - *buf = GetDAT(); + buf[0] = GetDAT(); // Disable REQ signal SetSignal(board->pin_req, board_type::gpio_high_low_e::GPIO_STATE_LOW); @@ -558,15 +558,15 @@ int GPIOBUS::CommandHandShake(uint8_t *buf) // semantics. I fact, these semantics have become a standard in the Atari world. // RaSCSI becomes ICD compatible by ignoring the prepended $1F byte before processing the CDB. - if (*buf == 0x1F) { - SetSignal(board->pin_req, board_type::gpio_high_low_e::GPIO_STATE_HIGH); + if (buf[0] == 0x1F) { + SetSignal(PIN_REQ, ON); ret = WaitSignal(board->pin_ack, board_type::gpio_high_low_e::GPIO_STATE_HIGH); SysTimer::SleepNsec(SCSI_DELAY_BUS_SETTLE_DELAY_NS); // Get the actual SCSI command - *buf = GetDAT(); + buf[0] = GetDAT(); SetSignal(board->pin_req, board_type::gpio_high_low_e::GPIO_STATE_LOW); @@ -583,14 +583,20 @@ int GPIOBUS::CommandHandShake(uint8_t *buf) } } - const int command_byte_count = GetCommandByteCount(*buf); + const int command_byte_count = GetCommandByteCount(buf[0]); + if (command_byte_count == 0) { + EnableIRQ(); - // Increment buffer pointer - buf++; + return 0; + } + + int offset = 0; int bytes_received; for (bytes_received = 1; bytes_received < command_byte_count; bytes_received++) { - // Assert REQ signal + ++offset; + + // Assert REQ signal SetSignal(board->pin_req, board_type::gpio_high_low_e::GPIO_STATE_HIGH); // Wait for ACK signal @@ -600,7 +606,7 @@ int GPIOBUS::CommandHandShake(uint8_t *buf) SysTimer::SleepNsec(SCSI_DELAY_BUS_SETTLE_DELAY_NS); // Get data - *buf = GetDAT(); + buf[offset] = GetDAT(); // Clear the REQ signal SetSignal(board->pin_req, board_type::gpio_high_low_e::GPIO_STATE_LOW); @@ -617,9 +623,6 @@ int GPIOBUS::CommandHandShake(uint8_t *buf) if (!ret) { break; } - - // Advance the buffer pointer to receive the next byte - buf++; } EnableIRQ(); diff --git a/cpp/hal/gpiobus.h b/cpp/hal/gpiobus.h index a5b1c712..3d01740c 100644 --- a/cpp/hal/gpiobus.h +++ b/cpp/hal/gpiobus.h @@ -341,7 +341,7 @@ class GPIOBUS : public BUS return board; } - int CommandHandShake(uint8_t *buf) override; + int CommandHandShake(vector&) override; // Command receive handshake int ReceiveHandShake(uint8_t *buf, int count) override; // Data receive handshake diff --git a/cpp/rascsi/rascsi_core.cpp b/cpp/rascsi/rascsi_core.cpp index 96a9be90..2a49b5d8 100644 --- a/cpp/rascsi/rascsi_core.cpp +++ b/cpp/rascsi/rascsi_core.cpp @@ -136,9 +136,11 @@ void Rascsi::LogDevices(string_view devices) const } } -void Rascsi::TerminationHandler(int) +void Rascsi::TerminationHandler(int signum) { Cleanup(); + + exit(signum); } bool Rascsi::ProcessId(const string& id_spec, int& id, int& unit) const diff --git a/cpp/scsi.h b/cpp/scsi.h index 37d29f8c..c4309bdb 100644 --- a/cpp/scsi.h +++ b/cpp/scsi.h @@ -120,48 +120,48 @@ namespace scsi_defs { LOAD_OR_EJECT_FAILED = 0x53 }; - static const unordered_map command_names = { - { scsi_command::eCmdTestUnitReady, "TestUnitReady" }, - { scsi_command::eCmdRezero, "Rezero" }, - { scsi_command::eCmdRequestSense, "RequestSense" }, - { scsi_command::eCmdFormatUnit, "FormatUnit" }, - { scsi_command::eCmdReassignBlocks, "ReassignBlocks" }, - { scsi_command::eCmdRead6, "Read6/GetMessage10" }, - { scsi_command::eCmdRetrieveStats, "RetrieveStats" }, - { scsi_command::eCmdWrite6, "Write6/Print/SendMessage10" }, - { scsi_command::eCmdSeek6, "Seek6" }, - { scsi_command::eCmdSetIfaceMode, "SetIfaceMode" }, - { scsi_command::eCmdSetMcastAddr, "SetMcastAddr" }, - { scsi_command::eCmdEnableInterface, "EnableInterface" }, - { scsi_command::eCmdSynchronizeBuffer, "SynchronizeBuffer" }, - { scsi_command::eCmdInquiry, "Inquiry" }, - { scsi_command::eCmdModeSelect6, "ModeSelect6" }, - { scsi_command::eCmdReserve6, "Reserve6" }, - { scsi_command::eCmdRelease6, "Release6" }, - { scsi_command::eCmdModeSense6, "ModeSense6" }, - { scsi_command::eCmdStartStop, "StartStop" }, - { scsi_command::eCmdStopPrint, "StopPrint" }, - { scsi_command::eCmdSendDiagnostic, "SendDiagnostic" }, - { scsi_command::eCmdPreventAllowMediumRemoval, "PreventAllowMediumRemoval" }, - { scsi_command::eCmdReadCapacity10, "ReadCapacity10" }, - { scsi_command::eCmdRead10, "Read10" }, - { scsi_command::eCmdWrite10, "Write10" }, - { scsi_command::eCmdSeek10, "Seek10" }, - { scsi_command::eCmdVerify10, "Verify10" }, - { scsi_command::eCmdSynchronizeCache10, "SynchronizeCache10" }, - { scsi_command::eCmdReadDefectData10, "ReadDefectData10" }, - { scsi_command::eCmdReadLong10, "ReadLong10" }, - { scsi_command::eCmdWriteLong10, "WriteLong10" }, - { scsi_command::eCmdReadToc, "ReadToc" }, - { scsi_command::eCmdGetEventStatusNotification, "GetEventStatusNotification" }, - { scsi_command::eCmdModeSelect10, "ModeSelect10" }, - { scsi_command::eCmdModeSense10, "ModeSense10" }, - { scsi_command::eCmdRead16, "Read16" }, - { scsi_command::eCmdWrite16, "Write16" }, - { scsi_command::eCmdVerify16, "Verify16" }, - { scsi_command::eCmdSynchronizeCache16, "SynchronizeCache16" }, - { scsi_command::eCmdReadCapacity16_ReadLong16, "ReadCapacity16/ReadLong16" }, - { scsi_command::eCmdWriteLong16, "WriteLong16" }, - { scsi_command::eCmdReportLuns, "ReportLuns" } + static const unordered_map> command_mapping = { + { scsi_command::eCmdTestUnitReady, make_pair(6, "TestUnitReady") }, + { scsi_command::eCmdRezero, make_pair(6, "Rezero") }, + { scsi_command::eCmdRequestSense, make_pair(6, "RequestSense") }, + { scsi_command::eCmdFormatUnit, make_pair(6, "FormatUnit") }, + { scsi_command::eCmdReassignBlocks, make_pair(6, "ReassignBlocks") }, + { scsi_command::eCmdRead6, make_pair(6, "Read6/GetMessage10") }, + { scsi_command::eCmdRetrieveStats,make_pair( 6, "RetrieveStats") }, + { scsi_command::eCmdWrite6, make_pair(6, "Write6/Print/SendMessage10") }, + { scsi_command::eCmdSeek6, make_pair(6, "Seek6") }, + { scsi_command::eCmdSetIfaceMode, make_pair(6, "SetIfaceMode") }, + { scsi_command::eCmdSetMcastAddr, make_pair(6, "SetMcastAddr") }, + { scsi_command::eCmdEnableInterface, make_pair(6, "EnableInterface") }, + { scsi_command::eCmdSynchronizeBuffer, make_pair(6, "SynchronizeBuffer") }, + { scsi_command::eCmdInquiry, make_pair(6, "Inquiry") }, + { scsi_command::eCmdModeSelect6, make_pair(6, "ModeSelect6") }, + { scsi_command::eCmdReserve6, make_pair(6, "Reserve6") }, + { scsi_command::eCmdRelease6, make_pair(6, "Release6") }, + { scsi_command::eCmdModeSense6, make_pair(6, "ModeSense6") }, + { scsi_command::eCmdStartStop, make_pair(6, "StartStop") }, + { scsi_command::eCmdStopPrint, make_pair(6, "StopPrint") }, + { scsi_command::eCmdSendDiagnostic, make_pair(6, "SendDiagnostic") }, + { scsi_command::eCmdPreventAllowMediumRemoval, make_pair(6, "PreventAllowMediumRemoval") }, + { scsi_command::eCmdReadCapacity10, make_pair(10, "ReadCapacity10") }, + { scsi_command::eCmdRead10, make_pair(10, "Read10") }, + { scsi_command::eCmdWrite10, make_pair(10, "Write10") }, + { scsi_command::eCmdSeek10, make_pair(10, "Seek10") }, + { scsi_command::eCmdVerify10, make_pair(10, "Verify10") }, + { scsi_command::eCmdSynchronizeCache10, make_pair(10, "SynchronizeCache10") }, + { scsi_command::eCmdReadDefectData10, make_pair(10, "ReadDefectData10") }, + { scsi_command::eCmdReadLong10, make_pair(10, "ReadLong10") }, + { scsi_command::eCmdWriteLong10, make_pair(10, "WriteLong10") }, + { scsi_command::eCmdReadToc, make_pair(10, "ReadToc") }, + { scsi_command::eCmdGetEventStatusNotification, make_pair(10, "GetEventStatusNotification") }, + { scsi_command::eCmdModeSelect10, make_pair(10, "ModeSelect10") }, + { scsi_command::eCmdModeSense10, make_pair(10, "ModeSense10") }, + { scsi_command::eCmdRead16, make_pair(16, "Read16") }, + { scsi_command::eCmdWrite16, make_pair(16, "Write16") }, + { scsi_command::eCmdVerify16, make_pair(16, "Verify16") }, + { scsi_command::eCmdSynchronizeCache16, make_pair(16, "SynchronizeCache16") }, + { scsi_command::eCmdReadCapacity16_ReadLong16, make_pair(16, "ReadCapacity16/ReadLong16") }, + { scsi_command::eCmdWriteLong16, make_pair(16, "WriteLong16") }, + { scsi_command::eCmdReportLuns, make_pair(12, "ReportLuns") } }; }; diff --git a/cpp/test/bus_test.cpp b/cpp/test/bus_test.cpp index c2cf5a18..8b0a0822 100644 --- a/cpp/test/bus_test.cpp +++ b/cpp/test/bus_test.cpp @@ -12,33 +12,49 @@ TEST(BusTest, GetCommandByteCount) { - unordered_set opcodes; - - EXPECT_EQ(16, BUS::GetCommandByteCount(0x88)); - opcodes.insert(0x88); - EXPECT_EQ(16, BUS::GetCommandByteCount(0x8a)); - opcodes.insert(0x8a); - EXPECT_EQ(16, BUS::GetCommandByteCount(0x8f)); - opcodes.insert(0x8f); - EXPECT_EQ(16, BUS::GetCommandByteCount(0x91)); - opcodes.insert(0x91); - EXPECT_EQ(16, BUS::GetCommandByteCount(0x9e)); - opcodes.insert(0x9e); - EXPECT_EQ(16, BUS::GetCommandByteCount(0x9f)); - opcodes.insert(0x9f); + EXPECT_EQ(41, scsi_defs::command_mapping.size()); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x00)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x01)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x03)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x04)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x07)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x08)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x09)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x0a)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x0b)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x0c)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x0d)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x0e)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x10)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x12)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x15)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x16)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x17)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x1a)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x1b)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x1d)); + EXPECT_EQ(6, BUS::GetCommandByteCount(0x1e)); + EXPECT_EQ(10, BUS::GetCommandByteCount(0x25)); + EXPECT_EQ(10, BUS::GetCommandByteCount(0x28)); + EXPECT_EQ(10, BUS::GetCommandByteCount(0x2a)); + EXPECT_EQ(10, BUS::GetCommandByteCount(0x2b)); + EXPECT_EQ(10, BUS::GetCommandByteCount(0x2f)); + EXPECT_EQ(10, BUS::GetCommandByteCount(0x35)); + EXPECT_EQ(10, BUS::GetCommandByteCount(0x37)); + EXPECT_EQ(10, BUS::GetCommandByteCount(0x3e)); + EXPECT_EQ(10, BUS::GetCommandByteCount(0x3f)); + EXPECT_EQ(10, BUS::GetCommandByteCount(0x43)); + EXPECT_EQ(10, BUS::GetCommandByteCount(0x4a)); + EXPECT_EQ(10, BUS::GetCommandByteCount(0x55)); + EXPECT_EQ(10, BUS::GetCommandByteCount(0x5a)); EXPECT_EQ(12, BUS::GetCommandByteCount(0xa0)); - opcodes.insert(0xa0); - - for (int i = 0x20; i <= 0x7d; i++) { - EXPECT_EQ(10, BUS::GetCommandByteCount(i)); - opcodes.insert(i); - } - - for (int i = 0; i < 256; i++) { - if (opcodes.find(i) == opcodes.end()) { - EXPECT_EQ(6, BUS::GetCommandByteCount(i)); - } - } + EXPECT_EQ(16, BUS::GetCommandByteCount(0x88)); + EXPECT_EQ(16, BUS::GetCommandByteCount(0x8a)); + EXPECT_EQ(16, BUS::GetCommandByteCount(0x8f)); + EXPECT_EQ(16, BUS::GetCommandByteCount(0x91)); + EXPECT_EQ(16, BUS::GetCommandByteCount(0x9e)); + EXPECT_EQ(16, BUS::GetCommandByteCount(0x9f)); + EXPECT_EQ(0, BUS::GetCommandByteCount(0x1f)); } TEST(BusTest, GetPhase) diff --git a/cpp/test/mocks.h b/cpp/test/mocks.h index 90d1c159..5ab0396e 100644 --- a/cpp/test/mocks.h +++ b/cpp/test/mocks.h @@ -86,7 +86,7 @@ class MockBus : public BUS // NOSONAR Having many fields/methods cannot be avoid MOCK_METHOD(void, SetDAT, (uint8_t), (override)); MOCK_METHOD(bool, GetDP, (), (const override)); MOCK_METHOD(uint32_t, Acquire, (), (override)); - MOCK_METHOD(int, CommandHandShake, (uint8_t *), (override)); + MOCK_METHOD(int, CommandHandShake, (vector&), (override)); MOCK_METHOD(int, ReceiveHandShake, (uint8_t *, int), (override)); MOCK_METHOD(int, SendHandShake, (uint8_t *, int, int), (override)); MOCK_METHOD(bool, GetSignal, (int), (const override)); @@ -201,10 +201,6 @@ class MockAbstractController : public AbstractController // NOSONAR Having many AllocateBuffer(512); } ~MockAbstractController() override = default; - - // Permit access to all tests without the need for numerous FRIEND_TEST - vector& GetCmd() { return AbstractController::GetCmd(); } //NOSONAR Hides function on purpose - BUS& GetBus() { return AbstractController::GetBus(); } //NOSONAR Hides function on purpose }; class MockScsiController : public ScsiController