| Message ID | 1414569232-21357-2-git-send-email-hare@suse.de |
|---|---|
| State | New |
| Headers | show |
Copying John Snow for additional AHCI expertise. Hannes Reinecke <hare@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 <hare@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
On 10/29/2014 09:07 AM, Markus Armbruster wrote: > Copying John Snow for additional AHCI expertise. > > Hannes Reinecke <hare@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 <hare@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
On Wed, Oct 29, 2014 at 08:53:36AM +0100, Hannes Reinecke wrote: > 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 <hare@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 Looks good, this change is justified by ATA/ATAPI-4 "9.1 Signature and persistence". Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
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 <hare@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 <hare@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
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
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 <hare@suse.de> --- hw/ide/ahci.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)