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();