From 1a68c27d0a49abb05c27367537ed229591c4d242 Mon Sep 17 00:00:00 2001 From: Tim Jarzombek Date: Mon, 9 Feb 2026 20:48:36 -0500 Subject: [PATCH] SCSI: Implement MODE SELECT(6) parameter parsing and unit tests - Add shared `scsiparseutils` module to parse SCSI-2 MODE SELECT(6) parameter lists (headers, block descriptors, and variable-length mode pages). - Integrate parser into `ScsiHardDisk` and `ScsiCdrom` to replace TODO stubs. Incoming MODE SELECT commands are now validated and detailed structure is logged for debugging. - Add `test_modeselect.cpp` test suite with 44 unit tests covering parsing edge cases (truncation, overflow, multi-page). - Add `DPPC_BUILD_SCSI_TESTS` CMake option (default OFF) and `testscsi` target. - Add documentation in `ScsiCdrom` regarding the `BlockStorageDevice` refactoring needed to support 512 <-> 2048 byte block size switching. --- CMakeLists.txt | 11 + devices/common/scsi/scsicdrom.cpp | 13 +- devices/common/scsi/scsihd.cpp | 5 +- devices/common/scsi/scsiparseutils.cpp | 98 ++++++ devices/common/scsi/scsiparseutils.h | 60 ++++ devices/common/scsi/test/test_modeselect.cpp | 296 +++++++++++++++++++ 6 files changed, 480 insertions(+), 3 deletions(-) create mode 100644 devices/common/scsi/scsiparseutils.cpp create mode 100644 devices/common/scsi/scsiparseutils.h create mode 100644 devices/common/scsi/test/test_modeselect.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 30ab93df94..1db2ec2aca 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -50,6 +50,7 @@ if (EMSCRIPTEN) endif() option(DPPC_BUILD_PPC_TESTS "Build PowerPC tests" OFF) +option(DPPC_BUILD_SCSI_TESTS "Build SCSI tests" OFF) option(DPPC_BUILD_BENCHMARKS "Build benchmarking programs" OFF) option(DPPC_68K_DEBUGGER "Enable 68k debugging" OFF) @@ -231,4 +232,14 @@ if (DPPC_BUILD_PPC_TESTS) $) endif() +if (DPPC_BUILD_SCSI_TESTS) + file(GLOB SCSI_TEST_SOURCES "${PROJECT_SOURCE_DIR}/devices/common/scsi/test/*.cpp") + add_executable(testscsi ${SCSI_TEST_SOURCES} + "${PROJECT_SOURCE_DIR}/devices/common/scsi/scsiparseutils.cpp") + target_include_directories(testscsi PRIVATE + "${PROJECT_SOURCE_DIR}" + "${PROJECT_SOURCE_DIR}/thirdparty/loguru/") + target_link_libraries(testscsi PRIVATE loguru ${CMAKE_DL_LIBS} ${CMAKE_THREAD_LIBS_INIT}) +endif() + install (TARGETS dingusppc DESTINATION ${PROJECT_SOURCE_DIR}/build) diff --git a/devices/common/scsi/scsicdrom.cpp b/devices/common/scsi/scsicdrom.cpp index 6f88d49ba9..56941936bc 100644 --- a/devices/common/scsi/scsicdrom.cpp +++ b/devices/common/scsi/scsicdrom.cpp @@ -23,6 +23,7 @@ along with this program. If not, see . #include #include +#include #include #include #include @@ -233,8 +234,16 @@ void ScsiCdrom::mode_select_6(uint8_t param_len) std::memset(&this->data_buf[0], 0, 512); this->post_xfer_action = [this]() { - // TODO: parse the received mode parameter list here - LOG_F(INFO, "Mode Select: received mode parameter list"); + ModeSelectData parsed; + parse_mode_select_6(this->data_buf, this->incoming_size, + this->name.c_str(), parsed); + + // TODO: if the block descriptor requests 512-byte blocks (parsed.bd_block_len == 512), + // we should switch block_size so subsequent READs use 512-byte LBA addressing. + // This requires BlockStorageDevice::set_block_size() to also update block_size + // (currently it only sets raw_blk_size), and size_blocks must be recalculated. + // Classic Mac OS drivers may not need this (they subdivide 2048-byte sectors + // internally), but some drivers may depend on it. }; this->switch_phase(ScsiPhase::DATA_OUT); diff --git a/devices/common/scsi/scsihd.cpp b/devices/common/scsi/scsihd.cpp index d78561bfc1..ef7e9e4dda 100644 --- a/devices/common/scsi/scsihd.cpp +++ b/devices/common/scsi/scsihd.cpp @@ -24,6 +24,7 @@ along with this program. If not, see . #include #include #include +#include #include #include #include @@ -183,7 +184,9 @@ void ScsiHardDisk::mode_select_6(uint8_t param_len) { std::memset(&this->data_buf[0], 0xDD, this->sector_size); this->post_xfer_action = [this]() { - // TODO: parse the received mode parameter list here + ModeSelectData parsed; + parse_mode_select_6(this->data_buf, this->incoming_size, + this->name.c_str(), parsed); }; this->switch_phase(ScsiPhase::DATA_OUT); diff --git a/devices/common/scsi/scsiparseutils.cpp b/devices/common/scsi/scsiparseutils.cpp new file mode 100644 index 0000000000..ea8754f309 --- /dev/null +++ b/devices/common/scsi/scsiparseutils.cpp @@ -0,0 +1,98 @@ +/* +DingusPPC - The Experimental PowerPC Macintosh emulator +Copyright (C) 2018-26 The DingusPPC Development Team + (See CREDITS.MD for more details) + +This program is free software: you can redistribute it and/or modify +it under the terms of the GNU General Public License as published by +the Free Software Foundation, either version 3 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU General Public License for more details. + +You should have received a copy of the GNU General Public License +along with this program. If not, see . +*/ + +/** @file SCSI Mode Select parameter list parsing utilities. */ + +#include +#include + +#include + +int parse_mode_select_6(const uint8_t* data, int data_len, const char* dev_name, + ModeSelectData& result) +{ + std::memset(&result, 0, sizeof(result)); + + // Need at least the 4-byte mode parameter header + if (data_len < 4) { + if (dev_name) + LOG_F(WARNING, "%s: MODE SELECT parameter list too short (%d bytes)", + dev_name, data_len); + return -1; + } + + // Parse the mode parameter header (4 bytes for MODE SELECT(6)) + // Byte 0: mode data length (reserved for MODE SELECT, should be 0) + result.medium_type = data[1]; + result.device_param = data[2]; + result.block_desc_len = data[3]; + + if (dev_name) + LOG_F(INFO, "%s: MODE SELECT medium_type=%d, dev_param=0x%02X, bdl=%d", + dev_name, result.medium_type, result.device_param, result.block_desc_len); + + // Validate block descriptor length fits in the data + if (4 + result.block_desc_len > data_len) { + if (dev_name) + LOG_F(WARNING, "%s: MODE SELECT block descriptor overflows data", + dev_name); + return -1; + } + + // Parse block descriptor if present + if (result.block_desc_len >= 8) { + result.bd_num_blocks = (data[5] << 16) | (data[6] << 8) | data[7]; + result.bd_block_len = (data[9] << 16) | (data[10] << 8) | data[11]; + if (dev_name && result.bd_num_blocks) + LOG_F(INFO, "%s: MODE SELECT block descriptor: num_blocks=%d, block_len=%d", + dev_name, result.bd_num_blocks, result.bd_block_len); + } + + // Parse mode pages + int offset = 4 + result.block_desc_len; + result.num_pages = 0; + + while (offset + 1 < data_len) { + uint8_t page_code = data[offset] & 0x3F; + uint8_t page_len = data[offset + 1]; + + if (offset + 2 + page_len > data_len) { + if (dev_name) + LOG_F(WARNING, "%s: MODE SELECT page 0x%02X truncated " + "(need %d bytes at offset %d, only %d available)", + dev_name, page_code, page_len, offset + 2, data_len - offset - 2); + return -1; + } + + if (result.num_pages < ModeSelectData::MAX_PAGES) { + result.pages[result.num_pages].page_code = page_code; + result.pages[result.num_pages].page_len = page_len; + result.pages[result.num_pages].data_offset = offset + 2; + result.num_pages++; + } + + if (dev_name) + LOG_F(INFO, "%s: MODE SELECT page 0x%02X, length %d", + dev_name, page_code, page_len); + + offset += 2 + page_len; + } + + return result.num_pages; +} diff --git a/devices/common/scsi/scsiparseutils.h b/devices/common/scsi/scsiparseutils.h new file mode 100644 index 0000000000..986253aee1 --- /dev/null +++ b/devices/common/scsi/scsiparseutils.h @@ -0,0 +1,60 @@ +/* +DingusPPC - The Experimental PowerPC Macintosh emulator +Copyright (C) 2018-26 The DingusPPC Development Team + (See CREDITS.MD for more details) + +This program is free software: you can redistribute it and/or modify +it under the terms of the GNU General Public License as published by +the Free Software Foundation, either version 3 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU General Public License for more details. + +You should have received a copy of the GNU General Public License +along with this program. If not, see . +*/ + +/** @file SCSI Mode Select parameter list parsing utilities. */ + +#ifndef SCSI_PARSE_UTILS_H +#define SCSI_PARSE_UTILS_H + +#include + +/** Result of parsing a mode page entry. */ +struct ModePageParsed { + uint8_t page_code; + uint8_t page_len; + int data_offset; // byte offset to page data (past page_code + page_len) +}; + +/** Parsed MODE SELECT(6) parameter list. */ +struct ModeSelectData { + uint8_t medium_type; + uint8_t device_param; + uint8_t block_desc_len; + + // Block descriptor fields (valid when block_desc_len >= 8) + uint32_t bd_num_blocks; + uint32_t bd_block_len; + + int num_pages; + static const int MAX_PAGES = 16; + ModePageParsed pages[MAX_PAGES]; +}; + +/** Parse a MODE SELECT(6) parameter list. + * + * @param data Pointer to the parameter list data. + * @param data_len Length of the parameter list in bytes. + * @param dev_name Device name for log messages (may be nullptr to suppress logging). + * @param result Parsed result output. + * @return Number of pages parsed, or -1 on structural error. + */ +int parse_mode_select_6(const uint8_t* data, int data_len, const char* dev_name, + ModeSelectData& result); + +#endif // SCSI_PARSE_UTILS_H diff --git a/devices/common/scsi/test/test_modeselect.cpp b/devices/common/scsi/test/test_modeselect.cpp new file mode 100644 index 0000000000..f068fd2cfc --- /dev/null +++ b/devices/common/scsi/test/test_modeselect.cpp @@ -0,0 +1,296 @@ +/* +DingusPPC - The Experimental PowerPC Macintosh emulator +Copyright (C) 2018-26 The DingusPPC Development Team + (See CREDITS.MD for more details) + +This program is free software: you can redistribute it and/or modify +it under the terms of the GNU General Public License as published by +the Free Software Foundation, either version 3 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU General Public License for more details. + +You should have received a copy of the GNU General Public License +along with this program. If not, see . +*/ + +/** @file Unit tests for SCSI MODE SELECT(6) parameter list parsing. */ + +#include + +#include +#include +#include + +using namespace std; + +static int ntested = 0; +static int nfailed = 0; + +#define CHECK(cond, msg) \ + do { \ + ntested++; \ + if (!(cond)) { \ + nfailed++; \ + cout << "FAIL: " << (msg) \ + << " (" #cond ")" << endl; \ + } \ + } while (0) + +// ---- Test: header-only, no block descriptor, no pages ---- + +static void test_header_only() { + // 4-byte header: mode_data_len=0, medium_type=0, dev_param=0, bdl=0 + uint8_t data[] = { 0x00, 0x00, 0x00, 0x00 }; + + ModeSelectData result; + int rc = parse_mode_select_6(data, sizeof(data), nullptr, result); + + CHECK(rc == 0, "header-only: should return 0 pages"); + CHECK(result.medium_type == 0, "header-only: medium_type"); + CHECK(result.device_param == 0, "header-only: device_param"); + CHECK(result.block_desc_len == 0, "header-only: block_desc_len"); + CHECK(result.num_pages == 0, "header-only: num_pages"); +} + +// ---- Test: header + block descriptor, no pages ---- + +static void test_block_descriptor() { + uint8_t data[] = { + 0x00, 0x00, 0x00, 0x08, // header: bdl=8 + 0x00, // density code + 0x00, 0x01, 0x00, // num_blocks = 256 + 0x00, // reserved + 0x00, 0x02, 0x00, // block_len = 512 + }; + + ModeSelectData result; + int rc = parse_mode_select_6(data, sizeof(data), nullptr, result); + + CHECK(rc == 0, "block desc: should return 0 pages"); + CHECK(result.block_desc_len == 8, "block desc: bdl"); + CHECK(result.bd_num_blocks == 256, "block desc: num_blocks"); + CHECK(result.bd_block_len == 512, "block desc: block_len"); +} + +// ---- Test: header + one mode page ---- + +static void test_single_page() { + uint8_t data[] = { + 0x00, 0x00, 0x00, 0x00, // header: no block descriptor + 0x01, 0x06, // page 0x01, length 6 + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 // page data + }; + + ModeSelectData result; + int rc = parse_mode_select_6(data, sizeof(data), nullptr, result); + + CHECK(rc == 1, "single page: should return 1 page"); + CHECK(result.num_pages == 1, "single page: num_pages"); + CHECK(result.pages[0].page_code == 0x01, "single page: page_code"); + CHECK(result.pages[0].page_len == 6, "single page: page_len"); + CHECK(result.pages[0].data_offset == 6, "single page: data_offset"); +} + +// ---- Test: header + block descriptor + multiple pages ---- + +static void test_multi_page() { + uint8_t data[] = { + 0x00, 0x00, 0x00, 0x08, // header: bdl=8 + 0x00, 0x00, 0x00, 0x00, // block desc (4 bytes) + 0x00, 0x00, 0x02, 0x00, // block desc (4 bytes), block_len=512 + // page 0x01, length 6 + 0x01, 0x06, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + // page 0x03, length 2 + 0x03, 0x02, + 0xAA, 0xBB, + // page 0x30, length 4 + 0x30, 0x04, + 0x41, 0x50, 0x50, 0x4C, // "APPL" + }; + + ModeSelectData result; + int rc = parse_mode_select_6(data, sizeof(data), nullptr, result); + + CHECK(rc == 3, "multi page: should return 3 pages"); + CHECK(result.pages[0].page_code == 0x01, "multi page: page[0] code"); + CHECK(result.pages[0].page_len == 6, "multi page: page[0] len"); + CHECK(result.pages[1].page_code == 0x03, "multi page: page[1] code"); + CHECK(result.pages[1].page_len == 2, "multi page: page[1] len"); + CHECK(result.pages[2].page_code == 0x30, "multi page: page[2] code"); + CHECK(result.pages[2].page_len == 4, "multi page: page[2] len"); +} + +// ---- Test: PS (parameter saveable) bit in page code is masked ---- + +static void test_ps_bit_masked() { + uint8_t data[] = { + 0x00, 0x00, 0x00, 0x00, // header + 0x81, 0x02, // page_code byte = 0x81 → PS=1, code=0x01 + 0x00, 0x00, + }; + + ModeSelectData result; + int rc = parse_mode_select_6(data, sizeof(data), nullptr, result); + + CHECK(rc == 1, "ps bit: should return 1 page"); + CHECK(result.pages[0].page_code == 0x01, "ps bit: page_code masks to 0x01"); +} + +// ---- Test: too-short data (< 4 bytes) returns error ---- + +static void test_too_short() { + uint8_t data[] = { 0x00, 0x00 }; + + ModeSelectData result; + int rc = parse_mode_select_6(data, sizeof(data), nullptr, result); + + CHECK(rc == -1, "too short: should return -1"); +} + +// ---- Test: block descriptor overflows data ---- + +static void test_bd_overflow() { + // header says bdl=8, but only 6 bytes total + uint8_t data[] = { 0x00, 0x00, 0x00, 0x08, 0x00, 0x00 }; + + ModeSelectData result; + int rc = parse_mode_select_6(data, sizeof(data), nullptr, result); + + CHECK(rc == -1, "bd overflow: should return -1"); +} + +// ---- Test: truncated page data returns error ---- + +static void test_truncated_page() { + uint8_t data[] = { + 0x00, 0x00, 0x00, 0x00, // header + 0x01, 0x06, // page 0x01, claims length 6 + 0x00, 0x00, // but only 2 bytes of data follow + }; + + ModeSelectData result; + int rc = parse_mode_select_6(data, sizeof(data), nullptr, result); + + CHECK(rc == -1, "truncated page: should return -1"); +} + +// ---- Test: medium type and device-specific parameter are captured ---- + +static void test_header_fields() { + uint8_t data[] = { + 0x00, 0x05, 0x80, 0x00, // medium_type=5, dev_param=0x80 (write protected) + }; + + ModeSelectData result; + int rc = parse_mode_select_6(data, sizeof(data), nullptr, result); + + CHECK(rc == 0, "header fields: should return 0 pages"); + CHECK(result.medium_type == 0x05, "header fields: medium_type"); + CHECK(result.device_param == 0x80, "header fields: device_param (write protect)"); +} + +// ---- Test: data_offset points correctly into buffer ---- + +static void test_data_offset() { + uint8_t data[] = { + 0x00, 0x00, 0x00, 0x08, // header: bdl=8 + 0x00, 0x00, 0x00, 0x00, // block desc + 0x00, 0x00, 0x00, 0x00, // block desc + 0x04, 0x03, // page 0x04, length 3 + 0xDE, 0xAD, 0xBE, // page data + }; + + ModeSelectData result; + int rc = parse_mode_select_6(data, sizeof(data), nullptr, result); + + CHECK(rc == 1, "data offset: should return 1 page"); + CHECK(result.pages[0].data_offset == 14, "data offset: points past header+bd+page_hdr"); + CHECK(data[result.pages[0].data_offset] == 0xDE, "data offset: first byte is 0xDE"); +} + +// ---- Test: page data is accessible for memcpy via data_offset ---- + +static void test_page_data_copy() { + uint8_t data[] = { + 0x00, 0x00, 0x00, 0x00, // header + 0x01, 0x06, // page 0x01, length 6 + 0x04, 0x03, 0x00, 0x00, 0x00, 0x00 // error recovery params + }; + + ModeSelectData result; + int rc = parse_mode_select_6(data, sizeof(data), nullptr, result); + CHECK(rc == 1, "page data copy: parse ok"); + + // Simulate what the device does: copy page data into storage + uint8_t storage[6] = {}; + int offset = result.pages[0].data_offset; + int len = result.pages[0].page_len; + CHECK(len == 6, "page data copy: page_len is 6"); + std::memcpy(storage, &data[offset], len < 6 ? len : 6); + CHECK(storage[0] == 0x04, "page data copy: first byte copied"); + CHECK(storage[1] == 0x03, "page data copy: second byte copied"); +} + +// ---- Test: multiple pages with page data accessible ---- + +static void test_multi_page_data_access() { + uint8_t data[] = { + 0x00, 0x00, 0x00, 0x00, // header + // page 0x01, length 2 + 0x01, 0x02, 0xAA, 0xBB, + // page 0x03, length 2 + 0x03, 0x02, 0xCC, 0xDD, + }; + + ModeSelectData result; + int rc = parse_mode_select_6(data, sizeof(data), nullptr, result); + CHECK(rc == 2, "multi data access: 2 pages"); + + CHECK(data[result.pages[0].data_offset] == 0xAA, "multi data access: page 0 data[0]"); + CHECK(data[result.pages[0].data_offset + 1] == 0xBB, "multi data access: page 0 data[1]"); + CHECK(data[result.pages[1].data_offset] == 0xCC, "multi data access: page 1 data[0]"); + CHECK(data[result.pages[1].data_offset + 1] == 0xDD, "multi data access: page 1 data[1]"); +} + +// ---- Test: zero-length page is valid ---- + +static void test_zero_length_page() { + uint8_t data[] = { + 0x00, 0x00, 0x00, 0x00, // header + 0x01, 0x00, // page 0x01, length 0 (empty page) + }; + + ModeSelectData result; + int rc = parse_mode_select_6(data, sizeof(data), nullptr, result); + CHECK(rc == 1, "zero len page: parse ok"); + CHECK(result.pages[0].page_code == 0x01, "zero len page: page_code"); + CHECK(result.pages[0].page_len == 0, "zero len page: page_len is 0"); +} + +int main() { + cout << "=== SCSI MODE SELECT(6) Parser Tests ===" << endl; + + test_header_only(); + test_block_descriptor(); + test_single_page(); + test_multi_page(); + test_ps_bit_masked(); + test_too_short(); + test_bd_overflow(); + test_truncated_page(); + test_header_fields(); + test_data_offset(); + test_page_data_copy(); + test_multi_page_data_access(); + test_zero_length_page(); + + cout << endl; + cout << ntested << " tests, " << nfailed << " failed." << endl; + + return nfailed ? 1 : 0; +}