fix generic CDB framing and CD-ROM negotiation

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<uint8_t>(GetOpcode())); i++) {
             s << setw(2) << GetCmdByte(i);
         }

cpp/devices/disk.cpp

@@ -301,9 +301,13 @@ int Disk::ModeSense6(cdb_t cdb, vector<uint8_t>& 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<uint8_t>(size - 1);
 
 	return size;
 }
@@ -355,9 +359,13 @@ int Disk::ModeSense10(cdb_t cdb, vector<uint8_t>& 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;
 }

cpp/devices/scsi_command_util.cpp

@@ -92,7 +92,9 @@ string scsi_command_util::ModeSelect(scsi_command cmd, cdb_t cdb, span<const uin
 		offset += size;
 	}
 
-	if (!has_valid_page_code) {
+	// A MODE SELECT parameter list may legally contain only its header and block descriptors.
+	// Unknown mode pages remain invalid, but an empty page list is not one.
+	if (!has_valid_page_code && !result.empty()) {
 		throw scsi_exception(sense_key::illegal_request, asc::invalid_field_in_parameter_list);
 	}
 

cpp/devices/scsicd.cpp

@@ -17,6 +17,7 @@
 #include "scsicd.h"
 #include <array>
 #include <fstream>
+#include <limits>
 
 using namespace scsi_defs;
 using namespace scsi_command_util;
@@ -165,6 +166,68 @@ vector<uint8_t> SCSICD::InquiryInternal() const
 	return HandleInquiry(device_type::cd_rom, scsi_level, true);
 }
 
+void SCSICD::ModeSelect(scsi_command cmd, cdb_t cdb, span<const uint8_t> 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<int>(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<uint64_t>::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<int, vector<byte>>& pages, int page, bool changeable) const
 {
 	Disk::SetUpModePages(pages, page, changeable);

cpp/devices/scsicd.h

@@ -34,6 +34,7 @@ class SCSICD : public Disk, private ScsiMmcCommands
 
 	vector<uint8_t> InquiryInternal() const override;
 	int Read(span<uint8_t>, uint64_t) override;
+	void ModeSelect(scsi_defs::scsi_command, cdb_t, span<const uint8_t>, int) override;
 
 protected:
 

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<scsi_command>(opcode));
+	if (const auto& mapping = command_mapping.find(static_cast<scsi_command>(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;
+	}
 }
 
 //---------------------------------------------------------------------------

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)

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

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

cpp/test/scsi_command_util_test.cpp

@@ -137,6 +137,17 @@ TEST(ScsiCommandUtilTest, ModeSelectRejectsTruncatedParameterList)
 		<< "Truncated mode page header was accepted";
 }
 
+TEST(ScsiCommandUtilTest, ModeSelectAcceptsHeaderAndBlockDescriptorWithoutPages)
+{
+	vector<int> cdb(6);
+	cdb[1] = 0x10;
+	vector<uint8_t> buf(12);
+	buf[3] = 8;
+	buf[10] = 2;
+
+	EXPECT_TRUE(ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, static_cast<int>(buf.size()), 512).empty());
+}
+
 TEST(ScsiCommandUtilTest, ModeSelectRejectsFormatDevicePageWithInvalidLength)
 {
 	vector<int> cdb(6);

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<int> cdb(6);
+	cdb[0] = static_cast<int>(scsi_command::eCmdModeSelect6);
+	cdb[1] = 0x10; // PF
+	vector<uint8_t> buf(12);
+	buf[3] = 8;
+	buf[10] = 8;
+	cd.ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, static_cast<int>(buf.size()));
+	EXPECT_EQ(2048, cd.GetSectorSizeInBytes());
+	EXPECT_EQ(2, cd.GetBlockCount());
+
+	buf[10] = 2;
+	cd.ModeSelect(scsi_command::eCmdModeSelect6, cdb, buf, static_cast<int>(buf.size()));
+	EXPECT_EQ(512, cd.GetSectorSizeInBytes());
+	EXPECT_EQ(8, cd.GetBlockCount());
+
+	remove(filename);
+}
+
 TEST(ScsiCdTest, ReadToc)
 {
 	auto controller = make_shared<MockAbstractController>();