Re: [Qemu-devel] [PATCH 01/17] ahci: Fix CD-ROM signature

[John Snow]

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 <address@hidden> 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 <address@hidden>
> > > ---
> > >   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