Create empty CD-ROM drives for SCSI-based machines

[mihaip]

Follow-up to #222, to support inserting CD-ROMs on more machines.

We only created SCSI CD-ROM drives when an image was supplied at startup, so insertion requests had no drive to go to. The Beige G3 already worked because its ATAPI drive is always created. Enable a default drive on the appropriate bus for SCSI-based machines.

Creating an empty drive exposed another issue: the 7300 ROM tries to read from it during boot. We would try to transfer data from an unallocated buffer and crash. The first commit fixes this by returning NOT READY / MEDIUM NOT PRESENT for reads, and other requests. This matches the behavior described in ATAPI 2.6, section 8.5 and the per-command error tables.

[maximumspatium]

Related to c3737bb
I think it would be better to check for device readiness at the beginning of a command group (CD-ROM, block, other) instead of putting checks into the particular command.
We could also add a flag indicating that that check already has been performed by the parent group so it doesn't need to be done several times while traveling down the command group tree.

[maximumspatium]

Regarding 4d3322b
Do we really need this workaround?
Why can't we just always register CD-ROM drives?

[joevt]

We want to be able to not add a CD-ROM drive. Therefore, my suggestion would be to enable the cdr_config and hdd_config options to specify a device on a SCSI bus (or "" to represent no device to remove the default CD-ROM or HDD-ROM).

We want to be able to add multiple empty CD-ROM drives, so allow cdr_img to contain empty "" image paths.

These features already exist in my fork.

More notes:

For Create empty CD-ROM drives for SCSI-based machines , could we add a cdr_config to each machine that points to the default SCSI CD-ROM location? It's what we do for IDE devices and I expanded it to work in my fork for SCSI devices as well.

cdr_config is currently handled by AtapiCdrom::device_postinit but it can handle only one AtapiCdrom. I understand you might not want to mess with this IDE / ATAPI code since it works sufficiently already. I think it's safe to leave it as is because it only happens if the machine has a AtapiCdrom device added but some tweaks may be needed to make it usable for SCSI bus as well.

parse_device_path is used to extract the bus device name and the bus number. The caller is responsible for checking that the bus device is of the appropriate type.
AtapiCdrom::device_postinit does not check the result of get_comp_by_name - it could be null or it could be an incorrect bus type.

A machine sub-device list includes a device AtaHardDisk or AtapiCdrom if the default hdd_config or cdr_config machine property is not empty. I think those are redundant and should be removed from the machine sub-device list. Instead, a AtaHardDisk or AtapiCdrom can be added programatically if hdd_config or cdr_config are non-empty and point to a bus, and hdd_img / cdr_img can add additional devices with a colon delimited image path list, where empty paths represent empty CD-ROM drive.

If a default hard disk is specified but an hdd_img path is not, then the hard disk should be removed because I don't think we support the concept of a hard disk device with removable media (e.g. Zip disk?)

In my fork, I have a method MachineFactory::apply_configs which handles cdr_config and hdr_config for both SCSI and ATA buses.
The set_property method of a ATA or SCSI harddrive or CD-ROM device is used to set the hdd_img or cdr_img for that device.
The set_property method of a ATA or SCSI bus is used to add a new device to that bus.

For the main fork, we can change the ATI bus and devices to work like the SCSI devices.

[mihaip]

Thanks for the feedback @maximumspatium and @joevt. I've reworked this a bit:

• I made the SCSI bus also support the cdr_config property, which describes where a default CD-ROM drive should be attached, similar to how it works for IDE
• I added a helper similar to joevt@baa012b to encapsulate some of the repeated empty checks. We can't do it one place (before any commands) because some commands can be issued even with no media inserted (INQUIRY, REQUEST SENSE, etc.)

[maximumspatium]

We can't do it one place (before any commands) because some commands can be issued even with no media inserted

AFAIK, the only commands that can be issued without any media are INQUIRY and REQUEST_SENSE. ready_for_command() can check for them and allow control passing down to the ScsiCommonCmds.
It should be sufficient to invoke ready_for_command() at the beginning of the particular command group.
I don't want all those ScsiXXXCmds classes to become polluted with unneccessary code/checks. So please use as few checks as possible and generalize wherever possible.

[maximumspatium]

Keep in mind that READ CD is about to be moved to ScsiCdromCmds because it is NOT a typical ATAPI feature but a common SCSI CD-ROM command...
The way it's currently being handled is a hack because the underlying CD-ROM image that can be a full binary dump including low-level headers isn't considered at all.
Handling of the low-level CD-ROM format pecularities should be in cdromdrive.cpp and nowhere else.
A refactoring commit is on the way...

[mihaip]

@maximumspatium I did another pass on this, ready_for_command is now invoked at the start of the common command blocks, and has an internal exemption for INQUIRY and REQ_SENSE.

[maximumspatium]

If I read your changes correctly, test_unit_ready() will be called only when a medium is present in the virtual drive. In this case, it will just perform the same check again ready_for_command() already did.
If there is no medium in the drive, test_unit_ready() will be never called.
Is that correct?

[mihaip]

@maximumspatium yes, the pre-command check for media being present means that the TEST_UNIT_READY handler never runs directly when there is no media, but the refactored logic ready_for_command means that will have the same affect.

The current behavior/implementation is because I was trying to follow your feedback to centralize the media present checks. If you'd like, I can go back to the original implementation (c3737bb) or a variant of it, where we more explicitly check for media insertion on CD-ROM reads only.

[maximumspatium]

If you'd like, I can go back to the original implementation (c3737bb) or a variant of it, where we more explicitly check for media insertion on CD-ROM reads only.

Well, my intention was to minimize code. Theoretically, no SCSI CD-ROM or block device command will work without media so we'd need to check for media presence in every command.

I suggest you check for TEST_UNIT_READY and MODE_SENSE in your ready_for_command() method as well allowing them to be passed to ScsiCommonCmds just like REQ_SENSE and INQUIRY.

This way, TEST_UNIT_READY will be handled in the dedicated handler that delegates to ready_for_command() if necessary.

I also suggest to move ready_for_command() check to ScsiCdromCmds. Otherwise, each new CD-ROM emulation would need to perform it which is wasteful.

ScsiBlockCmds also needs to perform this check because hard drives can be accessed directly without calling TEST_UNIT_READY.

Because ScsiCdromCmds passes control to ScsiBlockCmds, ready_for_command() will be called twice for block commands in CD-ROM devices. That isn't bad because the check is lightweight.

[mihaip]

Thanks for the feedback, I think it's cleaner now (except for the special-casing in AtapiCdrom::perform_packet_command, but I think that's the best we can do since it doesn't use ScsiCdromCmds).

[maximumspatium]

LGTM