| Message ID | 1435018875-22527-17-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 22, 2015 at 08:21:15PM -0400, John Snow wrote: > @@ -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 */ ... > - ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS); > + if (sdb_fis->flags & 0x40) { > + ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS); > + } This if statement is always true.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On 06/26/2015 12:11 PM, Stefan Hajnoczi wrote: > On Mon, Jun 22, 2015 at 08:21:15PM -0400, John Snow wrote: >> @@ -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 */ > ... >> - ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS); + if >> (sdb_fis->flags & 0x40) { + ahci_trigger_irq(s, ad, >> PORT_IRQ_SDB_FIS); + } > > This if statement is always true. > Yes. I did that on purpose. Maybe that was too weird. The idea was to emphasize that the IRQ is definitely only triggered *IF* the interrupt bit is set. It just so happens that we currently always set it. The generation and trigger mechanics are supposed to be separate, but we currently have no use case for them being separate (because we are an omniscient HBA-HDD amalgamation), so they're mushed together here. This is kind of like a documentation "NB." I can replace it with: /* Trigger IRQ if interrupt bit is set, which currently it always is: */ ahci_trigger_irq(...); or just add that comment to the existing structure, etc. (Or if you really prefer, just the naked IRQ trigger, but I feel that's kind of misleading.) -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJVjY2XAAoJEH3vgQaq/DkOl60P/j7dpKYPHobr+R8YP2ujWsc0 SinxemsCsgdvUXfTK5ma1yYYccHc0y5f7Ug617U4+/Tr7hK/gMhpQS0sLh+aTh1k DZTo30kqvXsDXIkw6dUjqBJRAPg0bm3RSB+YjqJxOH8o6nyntOmSzaJrxU6Rd6Oj AJFNtXPmlk3RLiU6uRD/EK7K+HaoRmvcvga6aGfNX4YJgM3+NUUUqiK6Urjok0sR qRvoYEkqvsJqujMAyHxlZuzW50PVcvEcWMyBXYaO2bvQWG/e/1SueQ9fLO8emdiF HQEyeCts4fN5VsZqZBdp+68PUqcRm6BtDodh5pPdPOe8OUc3l9Hu4tkTFqbe+iXM Em6+G9umMo0jPIepztYr56uXwYdLK0VGc2JQ91CqE0OXEmL+qh0+8xTg0e4Ix4Ff z7M9g90Lvuq9yLRAGtwjBKO+NDdgGXszc40W64yEtChqgt2GNOfLUIUbx45tVU4O vmddjNVyKE432AcMDC1+Je4uIKHrXwQGHO6ZdncrCvCeAKaDhZF77gpuy7fvga9v /I51zZVuxCNXEGPSuLYCE7ynA0vNBzQ1zgcEcxTM/2/odFBmgACY2nMSlgFy2b5x qpTQsTyIis8/mi6sdaX/l3FZ1i/pXbuoUzmrTxOfOmcjFJjS7n5Qb7Yj6aX64sHn /vESog1pKwxvzwyPmejk =OYom -----END PGP SIGNATURE-----
On Fri, Jun 26, 2015 at 01:36:23PM -0400, John Snow wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA256 > > > > On 06/26/2015 12:11 PM, Stefan Hajnoczi wrote: > > On Mon, Jun 22, 2015 at 08:21:15PM -0400, John Snow wrote: > >> @@ -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 */ > > ... > >> - ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS); + if > >> (sdb_fis->flags & 0x40) { + ahci_trigger_irq(s, ad, > >> PORT_IRQ_SDB_FIS); + } > > > > This if statement is always true. > > > > Yes. I did that on purpose. Maybe that was too weird. > > The idea was to emphasize that the IRQ is definitely only triggered > *IF* the interrupt bit is set. It just so happens that we currently > always set it. > > The generation and trigger mechanics are supposed to be separate, but > we currently have no use case for them being separate (because we are > an omniscient HBA-HDD amalgamation), so they're mushed together here. > > This is kind of like a documentation "NB." > > I can replace it with: > > /* Trigger IRQ if interrupt bit is set, which currently it always is: */ > ahci_trigger_irq(...); Good idea, that makes it clear.
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);
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 <jsnow@redhat.com> --- hw/ide/ahci.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)