On 10/29/2014 04:11 AM, Hannes Reinecke wrote:
On 10/29/2014 09:07 AM, Markus Armbruster wrote:
Copying John Snow for additional AHCI expertise.
Hannes Reinecke <h...@suse.de> writes:
The CD-ROM signature is 0xeb140101, not 0xeb140000.
Without this change OVMF/Duet runs into a timeout trying
to detect a SATA cdrom.
Signed-off-by: Hannes Reinecke <h...@suse.de>
---
hw/ide/ahci.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index e223258..970eea8 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -161,7 +161,7 @@
#define AHCI_CMD_HDR_CMD_FIS_LEN 0x1f
#define AHCI_CMD_HDR_PRDT_LEN 16
-#define SATA_SIGNATURE_CDROM 0xeb140000
+#define SATA_SIGNATURE_CDROM 0xeb140101
#define SATA_SIGNATURE_DISK 0x00000101
#define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x20
This is actually required by DUET/OVMF.
It just does this check:
(sig & 0xFFFF) == 0x0101
and hence will run into a timeout when a CDROM is attached
to ahci.
It's even in the ATA spec ...
Cheers,
Hannes
This part of the code is a little messy, because the "signature" is kind
of a structure comprised of various other IDE registers.
most significant to least significant:
{
31:24 ("LBA High Register")
23:16 ("LBA Mid Register")
15:08 ("LBA Low Register")
07:00 ("Sector Count Register")
}
You can find this information in AHCI section 3.3.9, "Offset 24h PxSIG -
Port x Signature."
The ATA8 ACS3 Rev. 1b notion of a signature includes more fields, from
which the AHCI spec has cherry-picked a characteristic few.
From Table 184 - Device Signatures for Normal Output (p. 303)
Count 7:0 -- 0x01 (AKA Sector Count)
LBA 27:24 -- Reserved, 0x00 (AKA Low-Nibble of the Device/Select Reg)
LBA 23:16 -- 0xEB (AKA LBA High)
LBA 15:08 -- 0x14 (AKA LBA Mid)
LBA 07:00 -- 0x01 (AKA LBA Low)
Which would make the AHCI-specific version of this signature:
0xEB-14-01-01
So that's good! The sort of messy part is whether or not we update the
actual IDE registers with the signature, or if we should just cheese it
by updating only PxSIG. For instance, the only place where this constant
is used is:
else if (ide_state->drive_kind == IDE_CD) {
pr->sig = SATA_SIGNATURE_CDROM;
ide_state->lcyl = 0x14;
ide_state->hcyl = 0xeb;
ide_state->status = SEEK_STAT | WRERR_STAT | READY_STAT;
}
Where we sort of re-decompose the signature to set the lcyl (LBA Mid)
and hcyl (LBA High) registers, but we don't touch the sector (LBA Low)
or nsector (count) registers.
Maybe we should -- these values are, if nothing changes them, going to
be reported in the Initial D2H Register FIS, where a guest could
conceivably inspect them and, seeing a discrepancy between PxSIG and the
LBA/Count registers, get a little confused.
This patch as-is is likely fine, but if a V2 is warranted for other
reasons, try updating the ide_state->sector and ide_state->nsector
fields to match the #define, by using shifts/masks of the #define, ideally.
--JS
