[Qemu-devel] [PATCH 16/16] ahci: fix sdb fis semantics

[John Snow]

There are two things to fix here:

The first one is subtle: the PxSACT register in the AHCI HBA has different
semantics from the field it is shadowing, the ACT field in the
Set Device Bits FIS.

In the HBA register, PxSACT acts as a bitfield indicating outstanding
NCQ commands where a set bit indicates a pending NCQ operation. The FIS
field however operates as an RWC register update to PxSACT, where a set
bit indicates a *successfully* completed command.

Correct the FIS semantics. At the same time, move the "clear finished"
action to the SDB FIS generation instead of the register read to mimick
how the other shadow registers work, which always just report the last
reported value from a FIS, and not the most current values which may
not have been reported by a FIS yet.

Lastly and more simply, SATA 3.2 section 13.6.4.2 (and later sections)
all specify that the Interrupt bit for the SDB FIS should always be set
to one for NCQ commands. That's currently the only time we generate this
FIS, so set it on all the time.

Signed-off-by: John Snow <address@hidden>
---
 hw/ide/ahci.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index d344701..db62a53 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -106,8 +106,6 @@ static uint32_t  ahci_port_read(AHCIState *s, int port, int 
offset)
         val = pr->scr_err;
         break;
     case PORT_SCR_ACT:
-        pr->scr_act &= ~s->dev[port].finished;
-        s->dev[port].finished = 0;
         val = pr->scr_act;
         break;
     case PORT_CMD_ISSUE:
@@ -665,14 +663,14 @@ static void ahci_unmap_clb_address(AHCIDevice *ad)
     ad->lst = NULL;
 }
 
-static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished)
+static void ahci_write_fis_sdb(AHCIState *s, NCQTransferState *ncq_tfs)
 {
-    AHCIDevice *ad = &s->dev[port];
+    AHCIDevice *ad = ncq_tfs->drive;
     AHCIPortRegs *pr = &ad->port_regs;
     IDEState *ide_state;
     SDBFIS *sdb_fis;
 
-    if (!s->dev[port].res_fis ||
+    if (!ad->res_fis ||
         !(pr->cmd & PORT_CMD_FIS_RX)) {
         return;
     }
@@ -682,19 +680,22 @@ static void ahci_write_fis_sdb(AHCIState *s, int port, 
uint32_t finished)
 
     sdb_fis->type = SATA_FIS_TYPE_SDB;
     /* Interrupt pending & Notification bit */
-    sdb_fis->flags = (ad->hba->control_regs.irqstatus ? (1 << 6) : 0);
+    sdb_fis->flags = 0x40; /* Interrupt bit, always 1 for NCQ */
     sdb_fis->status = ide_state->status & 0x77;
     sdb_fis->error = ide_state->error;
     /* update SAct field in SDB_FIS */
-    s->dev[port].finished |= finished;
     sdb_fis->payload = cpu_to_le32(ad->finished);
 
     /* Update shadow registers (except BSY 0x80 and DRQ 0x08) */
     pr->tfdata = (ad->port.ifs[0].error << 8) |
         (ad->port.ifs[0].status & 0x77) |
         (pr->tfdata & 0x88);
+    pr->scr_act &= ~ad->finished;
+    ad->finished = 0;
 
-    ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS);
+    if (sdb_fis->flags & 0x40) {
+        ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS);
+    }
 }
 
 static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
@@ -894,11 +895,14 @@ static void ncq_err(NCQTransferState *ncq_tfs)
 
 static void ncq_finish(NCQTransferState *ncq_tfs)
 {
-    /* Clear bit for this tag in SActive */
-    ncq_tfs->drive->port_regs.scr_act &= ~(1 << ncq_tfs->tag);
+    /* If we didn't error out, set our finished bit. Errored commands
+     * do not get a bit set for the SDB FIS ACT register, nor do they
+     * clear the outstanding bit in scr_act (PxSACT). */
+    if (!(ncq_tfs->drive->port_regs.scr_err & (1 << ncq_tfs->tag))) {
+        ncq_tfs->drive->finished |= (1 << ncq_tfs->tag);
+    }
 
-    ahci_write_fis_sdb(ncq_tfs->drive->hba, ncq_tfs->drive->port_no,
-                       (1 << ncq_tfs->tag));
+    ahci_write_fis_sdb(ncq_tfs->drive->hba, ncq_tfs);
 
     DPRINTF(ncq_tfs->drive->port_no, "NCQ transfer tag %d finished\n",
             ncq_tfs->tag);
-- 
2.1.0