ahci: fix completion race condition

These functions work on the AHCI device, not the individual

AHCI devices, so trim the AHCIDevice argument.

Signed-off-by John Snow <[email protected]>

---
  hw/ide/ahci.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

Quoted 1 line · John Snow

These functions work on the AHCI device, not the individual

s/device/state/ ?

Quoted 7 lines

AHCI devices, so trim the AHCIDevice argument.

Signed-off-by John Snow <[email protected]>

---
  hw/ide/ahci.c | 8 ++++----

  1 file changed, 4 insertions(+), 4 deletions(-)

These functions work on the AHCI device, not the individual AHCI devices, so trim the AHCIDevice argument.

Signed-off-by John Snow <[email protected]>

---
 hw/ide/ahci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

hw/ide/ahci.c +4 -4

index e22d7be05f..b7a6f68790 100644
@@ -131,7 +131,7 @@ static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)
     return val;
 }
 
-static void ahci_irq_raise(AHCIState *s, AHCIDevice *dev)
+static void ahci_irq_raise(AHCIState *s)
 {
     DeviceState *dev_state = s->container;
     PCIDevice *pci_dev = (PCIDevice *) object_dynamic_cast(OBJECT(dev_state),

@@ -146,7 +146,7 @@ static void ahci_irq_raise(AHCIState *s, AHCIDevice *dev)
     }
 }
 
-static void ahci_irq_lower(AHCIState *s, AHCIDevice *dev)
+static void ahci_irq_lower(AHCIState *s)
 {
     DeviceState *dev_state = s->container;
     PCIDevice *pci_dev = (PCIDevice *) object_dynamic_cast(OBJECT(dev_state),

@@ -174,9 +174,9 @@ static void ahci_check_irq(AHCIState *s)
     trace_ahci_check_irq(s, old_irq, s->control_regs.irqstatus);
     if (s->control_regs.irqstatus &&
         (s->control_regs.ghc & HOST_CTL_IRQ_EN)) {
-            ahci_irq_raise(s, NULL);
+            ahci_irq_raise(s);
     } else {
-        ahci_irq_lower(s, NULL);
+        ahci_irq_lower(s);
     }
 }
 

Signature · 1 line

2.14.3

From comments on the cover letter, I am adding:

AHCI presently signals completion prior to the PxCI register being cleared to indicate completion. If a guest driver attempts to issue a new command in its IRQ handler, it might be surprised to learn there is still a command pending.

In the case of Windows 10's boot driver, it will actually poll the IRQ register hoping to find out when the command is done running -- which will never happen, as there isn't a command running.

Fix this: clear PxCI in ahci_cmd_done and not in the asynchronous BH. Because it now runs synchronously, we don't need to check if the command is actually done by spying on the ATA registers. We know it's done.

Signed-off-by John Snow <[email protected]>

---
 hw/ide/ahci.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

hw/ide/ahci.c +6 -7

index b7a6f68790..a9558e45e7 100644
@@ -532,13 +532,6 @@ static void ahci_check_cmd_bh(void *opaque)
     qemu_bh_delete(ad->check_bh);
     ad->check_bh = NULL;
 
-    if ((ad->busy_slot != -1) &&
-        !(ad->port.ifs[0].status & (BUSY_STAT|DRQ_STAT))) {
-        /* no longer busy */
-        ad->port_regs.cmd_issue &= ~(1 << ad->busy_slot);
-        ad->busy_slot = -1;
-    }
-
     check_cmd(ad->hba, ad->port_no);
 }
 

@@ -1425,6 +1418,12 @@ static void ahci_cmd_done(IDEDMA *dma)
 
     trace_ahci_cmd_done(ad->hba, ad->port_no);
 
+    /* no longer busy */
+    if (ad->busy_slot != -1) {
+        ad->port_regs.cmd_issue &= ~(1 << ad->busy_slot);
+        ad->busy_slot = -1;
+    }
+
     /* update d2h status */
     ahci_write_fis_d2h(ad);
 

AHCI presently signals completion prior to the PxCI register being cleared to indicate completion. If a guest driver attempts to issue a new command in its IRQ handler, it might be surprised to learn there is still a command pending.

In the case of Windows 10's boot driver, it will actually poll the IRQ register hoping to find out when the command is done running -- which will never happen, as there isn't a command running.

Fix this: clear PxCI in ahci_cmd_done and not in the asynchronous BH. Because it now runs synchronously, we don't need to check if the command is actually done by spying on the ATA registers. We know it's done.

Signed-off-by John Snow <[email protected]>

---
 hw/ide/ahci.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

hw/ide/ahci.c +6 -7

index b7a6f68790..a9558e45e7 100644
@@ -532,13 +532,6 @@ static void ahci_check_cmd_bh(void *opaque)
     qemu_bh_delete(ad->check_bh);
     ad->check_bh = NULL;
 
-    if ((ad->busy_slot != -1) &&
-        !(ad->port.ifs[0].status & (BUSY_STAT|DRQ_STAT))) {
-        /* no longer busy */
-        ad->port_regs.cmd_issue &= ~(1 << ad->busy_slot);
-        ad->busy_slot = -1;
-    }
-
     check_cmd(ad->hba, ad->port_no);
 }
 

@@ -1425,6 +1418,12 @@ static void ahci_cmd_done(IDEDMA *dma)
 
     trace_ahci_cmd_done(ad->hba, ad->port_no);
 
+    /* no longer busy */
+    if (ad->busy_slot != -1) {
+        ad->port_regs.cmd_issue &= ~(1 << ad->busy_slot);
+        ad->busy_slot = -1;
+    }
+
     /* update d2h status */
     ahci_write_fis_d2h(ad);
 

Signature · 1 line

2.14.3

The comment gives us a hint. *Maybe* we still have something to process. Well, why not check?

Signed-off-by John Snow <[email protected]>

---
 hw/ide/ahci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

hw/ide/ahci.c +1 -2

index a9558e45e7..380366b038 100644
@@ -1427,8 +1427,7 @@ static void ahci_cmd_done(IDEDMA *dma)
     /* update d2h status */
     ahci_write_fis_d2h(ad);
 
-    if (!ad->check_bh) {
-        /* maybe we still have something to process, check later */
+    if (ad->port_regs.cmd_issue && !ad->check_bh) {
         ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
         qemu_bh_schedule(ad->check_bh);
     }

Signature · 1 line

2.14.3

Commit d759c951f changed the main thread lock release/reacquisition, and in so doing apparently jostled loose a race condition in the AHCI code.

Patch 2 should be sufficient to fix this, and patches 1 and 3 are just little trivial fixes.

This might be sufficient to fix the bug as reported at
https://bugs.launchpad.net/qemu/+bug/1769189
but the nature of the timing changes make it difficult to confirm, so I am posting this patchset for the reporters to help test.

John Snow (3):
ahci: trim signatures on raise/lower
ahci: fix PxCI register race
ahci: don't schedule unnecessary BH

 hw/ide/ahci.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

Quoted 1 line

Quoted 1 line · John Snow

Commit d759c951f changed the main thread lock release/reacquisition, and in so doing apparently jostled loose a race condition in the AHCI code.

Patch 2 should be sufficient to fix this, and patches 1 and 3 are just little trivial fixes.

This might be sufficient to fix the bug as reported at
https://bugs.launchpad.net/qemu/+bug/1769189
but the nature of the timing changes make it difficult to confirm, so I am posting this patchset for the reporters to help test.

John Snow (3):
ahci: trim signatures on raise/lower
ahci: fix PxCI register race
ahci: don't schedule unnecessary BH

hw/ide/ahci.c | 24 +++++++++++‑‑‑‑‑‑‑‑‑‑‑‑‑

 1 file changed, 11 insertions(+), 13 deletions(‑)

‑‑
2.14.3

Quoted 21 lines

Quoted 1 line

Quoted 1 line · John Snow

Commit d759c951f changed the main thread lock release/reacquisition, and in so doing apparently jostled loose a race condition in the AHCI code.

Patch 2 should be sufficient to fix this, and patches 1 and 3 are just little trivial fixes.

This might be sufficient to fix the bug as reported at
https://bugs.launchpad.net/qemu/+bug/1769189
but the nature of the timing changes make it difficult to confirm, so I am posting this patchset for the reporters to help test.

John Snow (3):
ahci: trim signatures on raise/lower
ahci: fix PxCI register race
ahci: don't schedule unnecessary BH

hw/ide/ahci.c | 24 +++++++++++‑‑‑‑‑‑‑‑‑‑‑‑‑

 1 file changed, 11 insertions(+), 13 deletions(‑)

Quoted 2 lines

‑‑
2.14.3

In my case, I applied these 3 patches on top of v2.12 qemu under which I can fairly reliably reproduce Windows10 disk delays. With these patches, after quite a number of attempts, I no longer can reproduce the failure case, so from my perspective it solves the issue.

Quoted 1 line

Quoted 1 line · Philippe Mathieu-Daudé

Hi Bruce,

Quoted 24 lines · Bruce Rogers

Quoted 23 lines

Quoted 1 line

Quoted 1 line · John Snow

Commit d759c951f changed the main thread lock release/reacquisition, and in so doing apparently jostled loose a race condition in the AHCI code.

Patch 2 should be sufficient to fix this, and patches 1 and 3 are just little trivial fixes.

This might be sufficient to fix the bug as reported at
https://bugs.launchpad.net/qemu/+bug/1769189
but the nature of the timing changes make it difficult to confirm, so I am posting this patchset for the reporters to help test.

John Snow (3):
ahci: trim signatures on raise/lower
ahci: fix PxCI register race
ahci: don't schedule unnecessary BH

hw/ide/ahci.c | 24 +++++++++++‑‑‑‑‑‑‑‑‑‑‑‑‑

 1 file changed, 11 insertions(+), 13 deletions(‑)

‑‑
2.14.3

Quoted 4 lines

In my case, I applied these 3 patches on top of v2.12 qemu under which I can fairly reliably reproduce Windows10 disk delays. With these patches, after quite a number of attempts, I no longer can reproduce the failure case, so from my perspective it solves the issue.

Does that implicitly mean John can use your "Tested-by: Bruce Rogers <[email protected]>" tag?

Commit d759c951f changed the main thread lock release/reacquisition, and in so doing apparently jostled loose a race condition in the AHCI code.

Patch 2 should be sufficient to fix this, and patches 1 and 3 are just little trivial fixes.

This might be sufficient to fix the bug as reported at
https://bugs.launchpad.net/qemu/+bug/1769189
but the nature of the timing changes make it difficult to confirm, so I am posting this patchset for the reporters to help test.

John Snow (3):
ahci: trim signatures on raise/lower
ahci: fix PxCI register race
ahci: don't schedule unnecessary BH

 hw/ide/ahci.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

Quoted 12 lines · John Snow

Commit d759c951f changed the main thread lock release/reacquisition, and in so doing apparently jostled loose a race condition in the AHCI code.

Patch 2 should be sufficient to fix this, and patches 1 and 3 are just little trivial fixes.

This might be sufficient to fix the bug as reported at
https://bugs.launchpad.net/qemu/+bug/1769189
but the nature of the timing changes make it difficult to confirm, so I am posting this patchset for the reporters to help test.

Quoted 8 lines

John Snow (3):
ahci: trim signatures on raise/lower
ahci: fix PxCI register race
ahci: don't schedule unnecessary BH

 hw/ide/ahci.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

Thanks for the testing and reviews, everyone!

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git