Conversation
Merged all functionally relevant changes, omitted:
1. Left out almost all of the "dbg_log()" changes, they're like 90% of the original PR and make the essential changes in the diffs a bit hard to read, these can be added back in at a later point in time.
2. Also left out the "wants_cdrom" option and implemented its desired effect with "cdrom: { ejected: true }" as was suggested.
3. The test "tests/api/cdrom-insert-eject.js" is also left out, can be added later back in.
This patch should behave like the last changeset from JoeOsborn in 2023.
Implement a single PCI IDE device in Compatibility Mode for up to 4 drives. Changes to ide.js: - added new exported class IDEPCIAdapter, the root class for up to 4 drives - introduced new config scheme for IDEPCIAdapter (see comment about adapter_config) - moved PCI-device related members from IDEDevice to IDEPCIAdapter - rewrote constructor of IDEDevice (no longer exported) - fixed (?) write-access to ata_port registers 1-6 (see comment below) - added missing declarations for BAR2 and BAR3 in pci_bars[] - made definition of the PCI configuration space a bit more more verbose - renamed "master_port" to "bus_master_port" to avoid confusion - added a few comments here and there Regarding write-access to ata_port registers 1-6: It is not clear why these 6 functions simultaneously modify master and slave attributes instead of just modifying this.current_interface (as is the case everywhere else in the code). This patch changes that to use this.current_interface, experimental until there is some explanation. Also changed cpu.js to now use IDEPCIAdapter instead of IDEDevice. TODO: 1. clear up the matter around write-access to ata_port registers 1-6 2. in ide.js, a single-device channel needs to be better supported, currently the code reports a Hard-Disk device of size 0 for an unconnected slave drive, and there must be a better way to signal this case. 3. in cpu.js, this.devices.hda and this.devices.cdrom point to a IDEDevice, they should point to an IDEInterface for this.devices.hdb to make sense. This needs to be better understood and fixed for slave drives to work. 4. getting some very early output under SeaBIOS, but the screen clears so quickly that I cannot read it and it's also not printed to the serial console
…ased access to IDEDevice and IDEInterface objects
- immplemented Command and Status register requirements from the standard, ignored all others - rearranged IDE register definitions, added a descriptive comment to each (standard lingo) - fixed the EXECUTE DEVICE DIAGNOSTIC (90h) ATA command
In v86, the name "device" is already used in the context of PCI, and additionally, in the ATA/ATAPI standard the term "device" is used for disk drives. The name used for this entity (IDEDevice) in the ATA/ATAPI standard is "channel", and since "channel" is not otherwise used in ide.js it seems right to rename IDEDevice to IDEChannel.
Merged and improved on the work from JoeOsborn in respect to formatting in dbg_log() output. Used register names from the standard for better readability of the output, and added a few missing logs.
Renamed members that carry register values to better align with the actual wording in the ATA/ATAPI-6 standard. Other changes in this commit include: - added a bunch of named constants for ATA registers and their bits - improved PCI configuration space setup - renamed class IDEPCIAdapter to IDEController
In the original code, status register bit DSC (Drive Seek Complete, 0x10) is always set when completing without error, but ATA/ATAPI-6 specifies this bit to be command dependent. This PR now sets the status register exactly like it was before, just using named constants instead of magic numbers. Also replaced most of the remaining magic number assignments to the ATA status register.
In the context of ATAPI there's only a single linked document in ide.js: https://www.t10.org/ftp/x3t9.2/document.87/87-106r0.txt. This link points to "PROPOSAL FOR CD-ROM IN SCSI-2" from 1987 (!) and its content matches the current code in v86, which the SCSI-3 documents did not at all (only the command codes matched, but otherwise SCSI-3 is very different from SCSI-2).
The SCSI Status field ("Status Key" and "Additional Status Code" aka. ASC) was incorrectly passed from the SCSI layer to the ATA register layer, and ATAPI command "TEST UNIT READY" was not implemented correctly.
This led to Linux not properly detecting when no CD was inserted into the CD-ROM device, which led to error output in "hwinfo" like (it attempted to read a disk that's no inserted):
> root@debian-iscsi:~# hwinfo --cdrom
> > block.5.1: /dev/sr0 cache[ 78.371349] I/O error, dev sr0, sector 64 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 2
> [ 78.412349] I/O error, dev sr0, sector 64 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 2
> [ 78.412349] Buffer I/O error on dev sr0, logical block 16, async page read
> [ 78.448365] I/O error, dev sr0, sector 68 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 2
> [ 78.448369] Buffer I/O error on dev sr0, logical block 17, async page read 08: SCSI 100.0: 10602 CD-ROM
This commit fixes that problem by now properly returning SCSI Sense and Additional Sense responses to the ATAPI host.
Debug log messages in ide.js are currently at maximum granularity and emitted at a very high volume. Under rare conditions this output can be very useful, else it clutters the view and makes comprehension of and reasoning about the runtime behaviour too challenging. Changes: - Erros and warnings are always logged, as well as important IDE state-related messages. - Debug log messages are now split into 5 categories, 4 of them (the highly specific and also high-frequency ones) are optional and by default disabled. - The 4 optional categories are REG_IO (log messages related to register I/O), IRQ, RW (read/write data) and RW_DMA (read/write data with DMA). - ATA/ATAPI commands are always logged unless they fall into one of the 4 optional categories. - Commands are logged with their name and including the full register state before and after command execution (input and output arguments). Normal debug log output (with all 4 optional categories disabled, the default) is now mostly reduced to the actual ATA/ATAPI command flow with input/output command arguments, enough so that it's much easier to understand what happens. The 4 optional debug categories can only be enabled programmatically in ide.js, they are meant to be used in advanced debugging sessions.
…ects - changed cdrom and hda to point to the drive's IDEInterface instead of its parent IDEChannel - added boolean method IDEInterface.has_disk() to query whether a Compact Disk is inserted or not - the IDEController object and the CD-ROM device are now created unconditionally
The minor change from yesterday changed Win95's behaviour such that it now issues an ATAPI command ("PAUSE") that wasn't implemented yet which led to Win95 crashing in v86 Debug mode. Added that command and now it works again.
A few ATAPI commands did not return an error (CD Not Ready condition) with ejected medium as specified in MMC-3 (https://www.t10.org/ftp/t10/document.97/97-108r0.pdf). Adding these to our subset of ATAPI commands improved the FreeDOS 1.4 situation around eject/insert a bit (the problem really is eject, insert always works given the medium is ejected). Waiting around 20-30 sec after eject seems to work reliably.
Adds support to eject a CD from within Windows 95 through the context menu of the CD icon. Before this commit eject was only supported externally through the v86 API.
There was an overlap in the PCI space register declarations of the Floppy and the IDE controller. Since the conflicting registers were used exclusively by the two controllers this was fine, but 9front prints out this boot warning message:
ioalloc: 3f0 - 3f5 floppy: clashes with: 3f4 - 3f5 PCI.0.31.0
Note that "PCI.0.31.0" is our IDE controller (with pci_id 0x1f).
Problem: IDE actually uses only register address 0x3f6, but declared BAR1 for address range 0x3f4-0x3f7 which overlaps with the Floppy controller (the same with BAR3 and its address range of 0x374-0x377, but there was no overlap with other devices here).
Fix:
- changed BAR1 base address from 0x3f4 to 0x3f6 and its size from 4 to 1
- changed BAR3 base address from 0x374 to 0x376 and its size from 4 to 1
This commit fixes that problem and the 9front boot message from above.
Closed
Contributor
Much agree to everything in the second commit (cleanup). But I don't fully understand the first. This condition in cpu.js in your commit can never be true, see further down in cpu.js where we create the IDEController. In line 1152, we statically assign the cdrom device to EDIT: I missed your change even further down below in |
Owner
Author
Indeed, this code is to handle the I will write some more testcases in the future, but my manual testing and the other tests we have so far have all been successful. Merged, thanks a lot! |
Contributor
Awesome, and you're welcome! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the same as #1325, with three extra changes:
@chschnell Could you do a quick review of this? An alternative would have been to layout the ide controller the same as previously (CD-ROM primary if there is no hard disk), but I slightly prefer having the CD-ROM always as the secondary.