diff mbox

[v3,29/32] ahci: properly shadow the TFD register

Message ID 1407966975-3723-6-git-send-email-jsnow@redhat.com
State New
Headers show

Commit Message

John Snow Aug. 13, 2014, 9:56 p.m. UTC 
In a real AHCI device, several S/ATA registers are mirrored or shadowed
within the AHCI register set. These registers are not updated
synchronously for each read access, but rather after a Device-to-Host
Register FIS packet is received, which contains the values from these
registers on the device.

In QEMU, by reaching directly into the device to grab these bits before
they are "sent," we may introduce race conditions where unexpected
values are present "before they are sent" which could cause issues for
some guests, particularly if an attempt is made to read the PxTFD
register prior to enabling the port, where incorrect values will be read.

This patch also addresses the boot-time values for the PxTFD and PxSIG
registers to bring them in line with the AHCI 1.3 specification.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

Comments

Stefan Hajnoczi Aug. 14, 2014, 4:09 p.m. UTC | #1 
On Wed, Aug 13, 2014 at 05:56:12PM -0400, John Snow wrote:
> @@ -497,6 +495,8 @@ static void ahci_reset_port(AHCIState *s, int port)
>      pr->scr_stat = 0;
>      pr->scr_err = 0;
>      pr->scr_act = 0;
> +    pr->tfdata = 0x7F;

Is it possible to avoid the magic number?
John Snow Aug. 14, 2014, 4:13 p.m. UTC | #2 
On 08/14/2014 12:09 PM, Stefan Hajnoczi wrote:
> On Wed, Aug 13, 2014 at 05:56:12PM -0400, John Snow wrote:
>> @@ -497,6 +495,8 @@ static void ahci_reset_port(AHCIState *s, int port)
>>       pr->scr_stat = 0;
>>       pr->scr_err = 0;
>>       pr->scr_act = 0;
>> +    pr->tfdata = 0x7F;
>
> Is it possible to avoid the magic number?
>

I can name it as AHCI_TFD_BOOT_VALUE or AHCI_TFD_INIT_VALUE or so. Same 
for the PxSIG boot value too. I don't think I can give any greater 
meaning to the value because it's just a boot signature that means "We 
haven't received an FIS yet."
Stefan Hajnoczi Aug. 14, 2014, 5:09 p.m. UTC | #3 
On Thu, Aug 14, 2014 at 12:13:51PM -0400, John Snow wrote:
> 
> 
> On 08/14/2014 12:09 PM, Stefan Hajnoczi wrote:
> >On Wed, Aug 13, 2014 at 05:56:12PM -0400, John Snow wrote:
> >>@@ -497,6 +495,8 @@ static void ahci_reset_port(AHCIState *s, int port)
> >>      pr->scr_stat = 0;
> >>      pr->scr_err = 0;
> >>      pr->scr_act = 0;
> >>+    pr->tfdata = 0x7F;
> >
> >Is it possible to avoid the magic number?
> >
> 
> I can name it as AHCI_TFD_BOOT_VALUE or AHCI_TFD_INIT_VALUE or so. Same for
> the PxSIG boot value too. I don't think I can give any greater meaning to
> the value because it's just a boot signature that means "We haven't received
> an FIS yet."

In that case, don't bother.  I thought this was error and status fields
shifted.

Stefan
diff mbox

Patch

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 4cda0d0..00d8a7a 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -78,8 +78,7 @@  static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)
         val = pr->cmd;
         break;
     case PORT_TFDATA:
-        val = ((uint16_t)s->dev[port].port.ifs[0].error << 8) |
-              s->dev[port].port.ifs[0].status;
+        val = pr->tfdata;
         break;
     case PORT_SIG:
         val = pr->sig;
@@ -251,14 +250,13 @@  static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
             check_cmd(s, port);
             break;
         case PORT_TFDATA:
-            s->dev[port].port.ifs[0].error = (val >> 8) & 0xff;
-            s->dev[port].port.ifs[0].status = val & 0xff;
+            /* Read Only. */
             break;
         case PORT_SIG:
-            pr->sig = val;
+            /* Read Only */
             break;
         case PORT_SCR_STAT:
-            pr->scr_stat = val;
+            /* Read Only */
             break;
         case PORT_SCR_CTL:
             if (((pr->scr_ctl & AHCI_SCR_SCTL_DET) == 1) &&
@@ -497,6 +495,8 @@  static void ahci_reset_port(AHCIState *s, int port)
     pr->scr_stat = 0;
     pr->scr_err = 0;
     pr->scr_act = 0;
+    pr->tfdata = 0x7F;
+    pr->sig = 0xFFFFFFFF;
     d->busy_slot = -1;
     d->init_d2h_sent = false;
 
@@ -528,16 +528,16 @@  static void ahci_reset_port(AHCIState *s, int port)
 
     s->dev[port].port_state = STATE_RUN;
     if (!ide_state->bs) {
-        s->dev[port].port_regs.sig = 0;
+        pr->sig = 0;
         ide_state->status = SEEK_STAT | WRERR_STAT;
     } else if (ide_state->drive_kind == IDE_CD) {
-        s->dev[port].port_regs.sig = SATA_SIGNATURE_CDROM;
+        pr->sig = SATA_SIGNATURE_CDROM;
         ide_state->lcyl = 0x14;
         ide_state->hcyl = 0xeb;
         DPRINTF(port, "set lcyl = %d\n", ide_state->lcyl);
         ide_state->status = SEEK_STAT | WRERR_STAT | READY_STAT;
     } else {
-        s->dev[port].port_regs.sig = SATA_SIGNATURE_DISK;
+        pr->sig = SATA_SIGNATURE_DISK;
         ide_state->status = SEEK_STAT | WRERR_STAT;
     }
 
@@ -563,7 +563,8 @@  static void debug_print_fis(uint8_t *fis, int cmd_len)
 
 static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished)
 {
-    AHCIPortRegs *pr = &s->dev[port].port_regs;
+    AHCIDevice *ad = &s->dev[port];
+    AHCIPortRegs *pr = &ad->port_regs;
     IDEState *ide_state;
     uint8_t *sdb_fis;
 
@@ -572,8 +573,8 @@  static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished)
         return;
     }
 
-    sdb_fis = &s->dev[port].res_fis[RES_FIS_SDBFIS];
-    ide_state = &s->dev[port].port.ifs[0];
+    sdb_fis = &ad->res_fis[RES_FIS_SDBFIS];
+    ide_state = &ad->port.ifs[0];
 
     /* clear memory */
     *(uint32_t*)sdb_fis = 0;
@@ -582,9 +583,14 @@  static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished)
     sdb_fis[0] = ide_state->error;
     sdb_fis[2] = ide_state->status & 0x77;
     s->dev[port].finished |= finished;
-    *(uint32_t*)(sdb_fis + 4) = cpu_to_le32(s->dev[port].finished);
+    *(uint32_t*)(sdb_fis + 4) = cpu_to_le32(ad->finished);
 
-    ahci_trigger_irq(s, &s->dev[port], PORT_IRQ_SDB_FIS);
+    /* 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);
+
+    ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS);
 }
 
 static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
@@ -642,6 +648,10 @@  static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
     pio_fis[18] = 0;
     pio_fis[19] = 0;
 
+    /* Update shadow registers: */
+    pr->tfdata = (ad->port.ifs[0].error << 8) |
+        ad->port.ifs[0].status;
+
     if (pio_fis[2] & ERR_STAT) {
         ahci_trigger_irq(ad->hba, ad, PORT_IRQ_TF_ERR);
     }
@@ -693,6 +703,10 @@  static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
         d2h_fis[i] = 0;
     }
 
+    /* Update shadow registers: */
+    pr->tfdata = (ad->port.ifs[0].error << 8) |
+        ad->port.ifs[0].status;
+
     if (d2h_fis[2] & ERR_STAT) {
         ahci_trigger_irq(ad->hba, ad, PORT_IRQ_TF_ERR);
     }