From 3df534524b257538575306f38d993d2b7caa9b9a Mon Sep 17 00:00:00 2001 From: Daniel Markstedt Date: Sun, 6 Sep 2026 07:58:29 +0200 Subject: [PATCH] fix generic CDB framing and CD-ROM negotiation Infer the length of otherwise unsupported standard SCSI CDBs from their command group, so the target consumes the complete command before returning an invalid-command status. This prevents commands such as READ DISC INFORMATION from leaving trailing CDB bytes on the bus, while preserving the special Atari ICD prefix and rejecting vendor-specific groups whose length is not defined. Handle page-zero MODE SENSE requests as header-and-block-descriptor responses with correct MODE DATA LENGTH fields. Accept descriptor-only CD-ROM MODE SELECT requests, validate the requested logical block size, switch between the supported 512 and 2048 byte formats while preserving capacity, and rebuild CD tracking and cache state. Add unit test coverage for CDB group lengths, MODE SENSE page zero, descriptor-only MODE SELECT, and CD block-size transitions. Thank you to Jan-Benedict Glaw (@jbglaw) for the report --- cpp/controllers/scsi_controller.cpp | 6 ++- cpp/devices/disk.cpp | 16 ++++++-- cpp/devices/scsi_command_util.cpp | 4 +- cpp/devices/scsicd.cpp | 63 +++++++++++++++++++++++++++++ cpp/devices/scsicd.h | 1 + cpp/hal/bus.cpp | 27 ++++++++++++- cpp/test/bus_test.cpp | 7 +++- cpp/test/disk_test.cpp | 29 +++++++++++++ cpp/test/mocks.h | 2 + cpp/test/scsi_command_util_test.cpp | 11 +++++ cpp/test/scsicd_test.cpp | 29 +++++++++++++ 11 files changed, 185 insertions(+), 10 deletions(-) diff --git a/cpp/controllers/scsi_controller.cpp b/cpp/controllers/scsi_controller.cpp index c0b45173a0..579add54ac 100644 --- a/cpp/controllers/scsi_controller.cpp +++ b/cpp/controllers/scsi_controller.cpp @@ -184,8 +184,10 @@ void ScsiController::Execute() { if (spdlog::get_level() == spdlog::level::trace) { stringstream s; - s << "Controller is executing " << command_mapping.find(GetOpcode())->second.second << ", CDB $" - << setfill('0') << hex; + const auto command = command_mapping.find(GetOpcode()); + s << "Controller is executing " + << (command != command_mapping.end() ? command->second.second : "UnknownCommand") + << ", CDB $" << setfill('0') << hex; for (int i = 0; i < BUS::GetCommandByteCount(static_cast(GetOpcode())); i++) { s << setw(2) << GetCmdByte(i); } diff --git a/cpp/devices/disk.cpp b/cpp/devices/disk.cpp index 3d23e89741..3974a21e50 100644 --- a/cpp/devices/disk.cpp +++ b/cpp/devices/disk.cpp @@ -301,9 +301,13 @@ int Disk::ModeSense6(cdb_t cdb, vector& buf) const size = 12; } - size = AddModePages(cdb, buf, size, length, 255); + // Page code 0 requests only the mode parameter header and block descriptor. + if (cdb[2] & 0x3f) { + size = AddModePages(cdb, buf, size, length, 255); + } - buf[0] = (uint8_t)size; + // MODE DATA LENGTH does not include this byte. + buf[0] = static_cast(size - 1); return size; } @@ -355,9 +359,13 @@ int Disk::ModeSense10(cdb_t cdb, vector& buf) const } } - size = AddModePages(cdb, buf, size, length, 65535); + // Page code 0 requests only the mode parameter header and block descriptor. + if (cdb[2] & 0x3f) { + size = AddModePages(cdb, buf, size, length, 65535); + } - SetInt16(buf, 0, size); + // MODE DATA LENGTH does not include the two-byte length field itself. + SetInt16(buf, 0, size - 2); return size; } diff --git a/cpp/devices/scsi_command_util.cpp b/cpp/devices/scsi_command_util.cpp index 667e493a1c..8de7e3a8e2 100644 --- a/cpp/devices/scsi_command_util.cpp +++ b/cpp/devices/scsi_command_util.cpp @@ -92,7 +92,9 @@ string scsi_command_util::ModeSelect(scsi_command cmd, cdb_t cdb, span #include +#include using namespace scsi_defs; using namespace scsi_command_util; @@ -165,6 +166,68 @@ vector SCSICD::InquiryInternal() const return HandleInquiry(device_type::cd_rom, scsi_level, true); } +void SCSICD::ModeSelect(scsi_command cmd, cdb_t cdb, span buf, int length) +{ + if (const string result = scsi_command_util::ModeSelect(cmd, cdb, buf, length, GetSectorSizeInBytes()); + !result.empty()) { + LogWarn(result); + } + + const int header_length = cmd == scsi_command::eCmdModeSelect10 ? 8 : 4; + const int descriptor_length = cmd == scsi_command::eCmdModeSelect10 ? GetInt16(buf, 6) : buf[3]; + if (!descriptor_length) { + return; + } + + // SCSI block descriptors are eight bytes. PiSCSI has one logical block size, so all supplied + // descriptors must request the same supported size. + if (descriptor_length % 8) { + throw scsi_exception(sense_key::illegal_request, asc::invalid_field_in_parameter_list); + } + + const uint32_t sector_size = GetInt24(buf, header_length + 5); + for (int offset = header_length + 8; offset < header_length + descriptor_length; offset += 8) { + if (GetInt24(buf, offset + 5) != static_cast(sector_size)) { + throw scsi_exception(sense_key::illegal_request, asc::invalid_field_in_parameter_list); + } + } + + if (!sector_size || sector_size == GetSectorSizeInBytes()) { + return; + } + if (!GetSupportedSectorSizes().contains(sector_size) || (rawfile && sector_size != 2048)) { + throw scsi_exception(sense_key::illegal_request, asc::invalid_field_in_parameter_list); + } + + const uint32_t old_sector_size = GetSectorSizeInBytes(); + uint64_t block_count = GetBlockCount(); + if (sector_size > old_sector_size) { + const uint32_t ratio = sector_size / old_sector_size; + if (block_count % ratio) { + throw scsi_exception(sense_key::illegal_request, asc::invalid_field_in_parameter_list); + } + block_count /= ratio; + } + else { + const uint32_t ratio = old_sector_size / sector_size; + if (block_count > numeric_limits::max() / ratio) { + throw scsi_exception(sense_key::illegal_request, asc::invalid_field_in_parameter_list); + } + block_count *= ratio; + } + + if (IsReady()) { + FlushCache(); + } + SetSectorSizeInBytes(sector_size); + SetBlockCount(block_count); + if (IsReady()) { + ClearTrack(); + CreateDataTrack(); + ResizeCache(GetFilename(), rawfile); + } +} + void SCSICD::SetUpModePages(map>& pages, int page, bool changeable) const { Disk::SetUpModePages(pages, page, changeable); diff --git a/cpp/devices/scsicd.h b/cpp/devices/scsicd.h index c2aaf4e3d1..cb1158dd2d 100644 --- a/cpp/devices/scsicd.h +++ b/cpp/devices/scsicd.h @@ -34,6 +34,7 @@ class SCSICD : public Disk, private ScsiMmcCommands vector InquiryInternal() const override; int Read(span, uint64_t) override; + void ModeSelect(scsi_defs::scsi_command, cdb_t, span, int) override; protected: diff --git a/cpp/hal/bus.cpp b/cpp/hal/bus.cpp index b135342a9f..5fd367d7dc 100644 --- a/cpp/hal/bus.cpp +++ b/cpp/hal/bus.cpp @@ -21,9 +21,32 @@ using namespace scsi_defs; //--------------------------------------------------------------------------- int BUS::GetCommandByteCount(uint8_t opcode) { - const auto& mapping = command_mapping.find(static_cast(opcode)); + if (const auto& mapping = command_mapping.find(static_cast(opcode)); + mapping != command_mapping.end()) { + return mapping->second.first; + } - return mapping != command_mapping.end() ? mapping->second.first : 0; + // The CDB length is defined by the SCSI command group, not by whether this target implements + // the command. Receive a complete standard CDB so that an unsupported command can be rejected + // cleanly instead of leaving its remaining bytes on the bus. + // + // $1f is an exception: GPIOBUS recognizes it as the Atari ICD prefix before this method is + // called for the actual SCSI command. + switch (opcode >> 5) { //NOSONAR: opcode is a numeric SCSI command field, not raw byte storage + case 0: + return opcode == 0x1f ? 0 : 6; + case 1: + case 2: + case 3: + return 10; + case 4: + return 16; + case 5: + return 12; + default: + // Groups 6 and 7 are vendor-specific, for which no common CDB length exists. + return 0; + } } //--------------------------------------------------------------------------- diff --git a/cpp/test/bus_test.cpp b/cpp/test/bus_test.cpp index 0bf4577888..68997f8a04 100644 --- a/cpp/test/bus_test.cpp +++ b/cpp/test/bus_test.cpp @@ -47,6 +47,7 @@ TEST(BusTest, GetCommandByteCount) EXPECT_EQ(10, BUS::GetCommandByteCount(0x3f)); EXPECT_EQ(10, BUS::GetCommandByteCount(0x43)); EXPECT_EQ(10, BUS::GetCommandByteCount(0x4a)); + EXPECT_EQ(10, BUS::GetCommandByteCount(0x51)); EXPECT_EQ(10, BUS::GetCommandByteCount(0x55)); EXPECT_EQ(10, BUS::GetCommandByteCount(0x5a)); EXPECT_EQ(12, BUS::GetCommandByteCount(0xa0)); @@ -57,7 +58,11 @@ TEST(BusTest, GetCommandByteCount) EXPECT_EQ(16, BUS::GetCommandByteCount(0x9e)); EXPECT_EQ(16, BUS::GetCommandByteCount(0x9f)); EXPECT_EQ(6, BUS::GetCommandByteCount(0xc2)); - EXPECT_EQ(0, BUS::GetCommandByteCount(0x1f)); + EXPECT_EQ(10, BUS::GetCommandByteCount(0x20)); + EXPECT_EQ(16, BUS::GetCommandByteCount(0x80)); + EXPECT_EQ(12, BUS::GetCommandByteCount(0xa1)); + EXPECT_EQ(0, BUS::GetCommandByteCount(0x1f)); + EXPECT_EQ(0, BUS::GetCommandByteCount(0xe0)); } TEST(BusTest, GetPhase) diff --git a/cpp/test/disk_test.cpp b/cpp/test/disk_test.cpp index f7ad298fed..6ca8d14e5a 100644 --- a/cpp/test/disk_test.cpp +++ b/cpp/test/disk_test.cpp @@ -746,6 +746,35 @@ TEST(DiskTest, ModeSense10) DiskTest_ValidateCachePage(*controller, 16); } +TEST(DiskTest, ModeSensePageZero) +{ + auto [controller, disk] = CreateDisk(); + disk->SetReady(true); + disk->SetBlockCount(2); + disk->SetSectorSizeInBytes(512); + + // MODE SENSE(6), page 0, with room for the header and one short block descriptor. + controller->SetCmdByte(1, 0); + controller->SetCmdByte(2, 0); + controller->SetCmdByte(4, 12); + disk->Dispatch(scsi_command::eCmdModeSense6); + const auto& sense6 = controller->GetBuffer(); + EXPECT_EQ(12, controller->GetLength()); + EXPECT_EQ(11, sense6[0]); + EXPECT_EQ(8, sense6[3]); + EXPECT_EQ(512, GetInt16(sense6, 10)); + + // MODE SENSE(10) uses the same page request and excludes its two-byte length field. + controller->SetCmdByte(7, 0); + controller->SetCmdByte(8, 16); + disk->Dispatch(scsi_command::eCmdModeSense10); + const auto& sense10 = controller->GetBuffer(); + EXPECT_EQ(16, controller->GetLength()); + EXPECT_EQ(14, GetInt16(sense10, 0)); + EXPECT_EQ(8, sense10[7]); + EXPECT_EQ(512, GetInt16(sense10, 14)); +} + TEST(DiskTest, SynchronizeCache) { auto [controller, disk] = CreateDisk(); diff --git a/cpp/test/mocks.h b/cpp/test/mocks.h index a7cd2dc6b0..c0e38832b4 100644 --- a/cpp/test/mocks.h +++ b/cpp/test/mocks.h @@ -145,6 +145,7 @@ class MockAbstractController : public AbstractController //NOSONAR Having many f FRIEND_TEST(DiskTest, StartStopUnit); FRIEND_TEST(DiskTest, ModeSense6); FRIEND_TEST(DiskTest, ModeSense10); + FRIEND_TEST(DiskTest, ModeSensePageZero); FRIEND_TEST(ScsiDaynaportTest, Read); FRIEND_TEST(ScsiDaynaportTest, Write); FRIEND_TEST(ScsiDaynaportTest, Read6); @@ -350,6 +351,7 @@ class MockDisk : public Disk FRIEND_TEST(DiskTest, Eject); FRIEND_TEST(DiskTest, ModeSense6); FRIEND_TEST(DiskTest, ModeSense10); + FRIEND_TEST(DiskTest, ModeSensePageZero); FRIEND_TEST(DiskTest, SynchronizeCache); FRIEND_TEST(DiskTest, ReadDefectData); FRIEND_TEST(DiskTest, SectorSize); diff --git a/cpp/test/scsi_command_util_test.cpp b/cpp/test/scsi_command_util_test.cpp index 945f98cb11..976efc5b86 100644 --- a/cpp/test/scsi_command_util_test.cpp +++ b/cpp/test/scsi_command_util_test.cpp @@ -137,6 +137,17 @@ TEST(ScsiCommandUtilTest, ModeSelectRejectsTruncatedParameterList) << "Truncated mode page header was accepted"; } +TEST(ScsiCommandUtilTest, ModeSelectAcceptsHeaderAndBlockDescriptorWithoutPages) +{ + vector cdb(6); + cdb[1] = 0x10; + vector buf(12); + buf[3] = 8; + buf[10] = 2; + + EXPECT_TRUE(ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, static_cast(buf.size()), 512).empty()); +} + TEST(ScsiCommandUtilTest, ModeSelectRejectsFormatDevicePageWithInvalidLength) { vector cdb(6); diff --git a/cpp/test/scsicd_test.cpp b/cpp/test/scsicd_test.cpp index d0d316fe7a..1b7d164758 100644 --- a/cpp/test/scsicd_test.cpp +++ b/cpp/test/scsicd_test.cpp @@ -119,6 +119,35 @@ TEST(ScsiCdTest, Open) remove(filename); } +TEST(ScsiCdTest, ModeSelect) +{ + MockSCSICD cd(0); + ASSERT_TRUE(cd.SetConfiguredSectorSize(512)); + + const path filename = CreateTempFile(2 * 2048); + cd.SetFilename(string(filename)); + cd.Open(); + EXPECT_EQ(512, cd.GetSectorSizeInBytes()); + EXPECT_EQ(8, cd.GetBlockCount()); + + vector cdb(6); + cdb[0] = static_cast(scsi_command::eCmdModeSelect6); + cdb[1] = 0x10; // PF + vector buf(12); + buf[3] = 8; + buf[10] = 8; + cd.ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, static_cast(buf.size())); + EXPECT_EQ(2048, cd.GetSectorSizeInBytes()); + EXPECT_EQ(2, cd.GetBlockCount()); + + buf[10] = 2; + cd.ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, static_cast(buf.size())); + EXPECT_EQ(512, cd.GetSectorSizeInBytes()); + EXPECT_EQ(8, cd.GetBlockCount()); + + remove(filename); +} + TEST(ScsiCdTest, ReadToc) { auto controller = make_shared();