[RFC PATCH 0/2] hw/ide/ahci: Delay a bit before completing reset

[RFC PATCH 0/2] hw/ide/ahci: Delay a bit before completing reset

[Philippe Mathieu-Daudé]

Intented to help SeaBIOS development; untested there
(except with QEMU test suite).

Jiaxun, is it helpful to you?

Philippe Mathieu-Daudé (2):
  hw/ide/ahci: Introduce ahci_reset_delayed() and ahci_reset_complete()
  hw/ide/ahci: Delay a bit before completing reset

 include/hw/ide/ahci.h   |  1 +
 hw/ide/ahci.c           | 70 ++++++++++++++++++++++++++++++++++++++---
 tests/qtest/ahci-test.c |  4 +++
 hw/ide/trace-events     |  1 +
 4 files changed, 71 insertions(+), 5 deletions(-)

-- 
2.47.1

[RFC PATCH 1/2] hw/ide/ahci: Introduce ahci_reset_delayed() and ahci_reset_complete()

[Philippe Mathieu-Daudé]

AHCI reset is not instantaneous in physical world, and software
might poll the reset bits of port and host control registers to
detect completion (see chapter 14 of AHCI spec).

In preparation of adding a timed reset, split ahci_reset()
as ahci_reset_delayed() which keeps the reset bits and
ahci_reset_complete() which resets them.

No logical changes so far.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/ahci.c       | 31 ++++++++++++++++++++++++++++---
 hw/ide/trace-events |  1 +
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 1303c21cb70..7e586c7a0a4 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -46,6 +46,7 @@ static bool ahci_map_clb_address(AHCIDevice *ad);
 static bool ahci_map_fis_address(AHCIDevice *ad);
 static void ahci_unmap_clb_address(AHCIDevice *ad);
 static void ahci_unmap_fis_address(AHCIDevice *ad);
+static void ahci_reset_delayed(AHCIState *s, bool immediate);
 
 static const char *AHCIHostReg_lookup[AHCI_HOST_REG__COUNT] = {
     [AHCI_HOST_REG_CAP]        = "CAP",
@@ -1623,7 +1624,22 @@ void ahci_uninit(AHCIState *s)
     g_free(s->dev);
 }
 
-void ahci_reset(AHCIState *s)
+static void ahci_reset_complete(void *opaque)
+{
+    AHCIState *s = opaque;
+
+    trace_ahci_reset_done(s);
+
+    for (unsigned i = 0; i < s->ports; i++) {
+        AHCIPortRegs *pr;
+
+        pr = &s->dev[i].port_regs;
+        pr->cmd &= ~PORT_CMD_LIST_ON;
+    }
+    s->control_regs.ghc &= ~HOST_CTL_RESET;
+}
+
+static void ahci_reset_delayed(AHCIState *s, bool immediate)
 {
     AHCIPortRegs *pr;
     int i;
@@ -1639,16 +1655,25 @@ void ahci_reset(AHCIState *s)
      *
      * We set HOST_CAP_AHCI so we must enable AHCI at reset.
      */
-    s->control_regs.ghc = HOST_CTL_AHCI_EN;
+    s->control_regs.ghc = HOST_CTL_RESET | HOST_CTL_AHCI_EN;
 
     for (i = 0; i < s->ports; i++) {
         pr = &s->dev[i].port_regs;
         pr->irq_stat = 0;
         pr->irq_mask = 0;
         pr->scr_ctl = 0;
-        pr->cmd = PORT_CMD_SPIN_UP | PORT_CMD_POWER_ON;
+        pr->cmd = PORT_CMD_SPIN_UP | PORT_CMD_POWER_ON | PORT_CMD_LIST_ON;
         ahci_reset_port(s, i);
     }
+
+    if (immediate) {
+        ahci_reset_complete(s);
+    }
+}
+
+void ahci_reset(AHCIState *s)
+{
+    ahci_reset_delayed(s, false);
 }
 
 static const VMStateDescription vmstate_ncq_tfs = {
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index 57042cafddc..979145c58b8 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -82,6 +82,7 @@ ahci_mem_write_host(void *s, unsigned size, const char *reg, uint64_t addr, uint
 ahci_mem_write_unimpl(void *s, unsigned size, uint64_t addr, uint64_t val) "ahci(%p): write%u to unknown register 0x%"PRIx64": 0x%016"PRIx64
 ahci_set_signature(void *s, int port, uint8_t nsector, uint8_t sector, uint8_t lcyl, uint8_t hcyl, uint32_t sig) "ahci(%p)[%d]: set signature sector:0x%02x nsector:0x%02x lcyl:0x%02x hcyl:0x%02x (cumulatively: 0x%08x)"
 ahci_reset_port(void *s, int port) "ahci(%p)[%d]: reset port"
+ahci_reset_done(void *s) "ahci(%p): reset done"
 ahci_unmap_fis_address_null(void *s, int port) "ahci(%p)[%d]: Attempt to unmap NULL FIS address"
 ahci_unmap_clb_address_null(void *s, int port) "ahci(%p)[%d]: Attempt to unmap NULL CLB address"
 ahci_populate_sglist(void *s, int port) "ahci(%p)[%d]"
-- 
2.47.1

[RFC PATCH 2/2] hw/ide/ahci: Delay a bit before completing reset

[Philippe Mathieu-Daudé]

Give few milliseconds to (emulated) hardware to complete
its reset sequence.

The intent is to have this model better match hardware,
reducing firmware bugs "works in QEMU but not in real
world" such https://github.com/FlyGoat/csmwrap/issues/14.

Reported-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/ide/ahci.h   |  1 +
 hw/ide/ahci.c           | 39 +++++++++++++++++++++++++++++++++++++--
 tests/qtest/ahci-test.c |  4 ++++
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
index cd07b87811b..82617835195 100644
--- a/include/hw/ide/ahci.h
+++ b/include/hw/ide/ahci.h
@@ -46,6 +46,7 @@ typedef struct AHCIState {
     uint32_t ports;
     qemu_irq irq;
     AddressSpace *as;
+    QEMUTimer *reset_timer;
 } AHCIState;
 
 
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 7e586c7a0a4..cf1c5d3f3db 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -28,6 +28,7 @@
 #include "qemu/error-report.h"
 #include "qemu/log.h"
 #include "qemu/main-loop.h"
+#include "qemu/timer.h"
 #include "system/block-backend.h"
 #include "system/dma.h"
 #include "ahci-internal.h"
@@ -47,6 +48,7 @@ static bool ahci_map_fis_address(AHCIDevice *ad);
 static void ahci_unmap_clb_address(AHCIDevice *ad);
 static void ahci_unmap_fis_address(AHCIDevice *ad);
 static void ahci_reset_delayed(AHCIState *s, bool immediate);
+static void ahci_reset_complete(void *opaque);
 
 static const char *AHCIHostReg_lookup[AHCI_HOST_REG__COUNT] = {
     [AHCI_HOST_REG_CAP]        = "CAP",
@@ -459,7 +461,7 @@ static void ahci_mem_write(void *opaque, hwaddr addr,
             break;
         case AHCI_HOST_REG_CTL: /* R/W */
             if (val & HOST_CTL_RESET) {
-                ahci_reset(s);
+                ahci_reset_delayed(s, true);
             } else {
                 s->control_regs.ghc = (val & 0x3) | HOST_CTL_AHCI_EN;
                 ahci_check_irq(s);
@@ -1591,6 +1593,7 @@ void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as)
     s->as = as;
     assert(s->ports > 0);
     s->dev = g_new0(AHCIDevice, s->ports);
+    s->reset_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, ahci_reset_complete, s);
     ahci_reg_init(s);
     irqs = qemu_allocate_irqs(ahci_irq_set, s, s->ports);
     for (i = 0; i < s->ports; i++) {
@@ -1622,8 +1625,12 @@ void ahci_uninit(AHCIState *s)
     }
 
     g_free(s->dev);
+
+    timer_free(s->reset_timer);
 }
 
+#define AHCI_RESET_DELAY_MS     69  /* Arbitrary value less than 500 ms */
+
 static void ahci_reset_complete(void *opaque)
 {
     AHCIState *s = opaque;
@@ -1667,7 +1674,11 @@ static void ahci_reset_delayed(AHCIState *s, bool immediate)
     }
 
     if (immediate) {
+        timer_del(s->reset_timer);
         ahci_reset_complete(s);
+    } else if (!timer_pending(s->reset_timer)) {
+        timer_mod(s->reset_timer,
+                  qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + AHCI_RESET_DELAY_MS);
     }
 }
 
@@ -1676,6 +1687,24 @@ void ahci_reset(AHCIState *s)
     ahci_reset_delayed(s, false);
 }
 
+static bool ahci_timer_needed(void *opaque)
+{
+    AHCIState *s = opaque;
+
+    return timer_pending(s->reset_timer);
+}
+
+static const VMStateDescription vmstate_ahci_timer = {
+    .name = "ahci/reset_timer",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = ahci_timer_needed,
+    .fields = (const VMStateField[]) {
+        VMSTATE_TIMER_PTR(reset_timer, AHCIState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_ncq_tfs = {
     .name = "ncq state",
     .version_id = 1,
@@ -1734,7 +1763,9 @@ static int ahci_state_post_load(void *opaque, int version_id)
         ad = &s->dev[i];
         pr = &ad->port_regs;
 
-        if (!(pr->cmd & PORT_CMD_START) && (pr->cmd & PORT_CMD_LIST_ON)) {
+        if (timer_pending(s->reset_timer)) {
+            /* Reset in progress */
+        } else if (!(pr->cmd & PORT_CMD_START) && (pr->cmd & PORT_CMD_LIST_ON)) {
             error_report("AHCI: DMA engine should be off, but status bit "
                          "indicates it is still running.");
             return -1;
@@ -1823,6 +1854,10 @@ const VMStateDescription vmstate_ahci = {
         VMSTATE_UINT32_EQUAL(ports, AHCIState, NULL),
         VMSTATE_END_OF_LIST()
     },
+    .subsections = (const VMStateDescription * const []) {
+        &vmstate_ahci_timer,
+        NULL
+    }
 };
 
 void ahci_ide_create_devs(AHCIState *ahci, DriveInfo **hd)
diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index e8aabfc13f5..4389aaa7f70 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -40,6 +40,8 @@
 #define TEST_IMAGE_SIZE_MB_LARGE (200 * 1024)
 #define TEST_IMAGE_SIZE_MB_SMALL 64
 
+#define AHCI_RESET_TIMEOUT_NS   (500 * 1000 * 1000)
+
 /*** Globals ***/
 static char *tmp_path;
 static char *debug_path;
@@ -483,6 +485,8 @@ static void ahci_test_hba_spec(AHCIQState *ahci)
 
     g_assert(ahci != NULL);
 
+    qtest_clock_step(ahci->parent->qts, AHCI_RESET_TIMEOUT_NS);
+
     /*
      * Note that the AHCI spec does expect the BIOS to set up a few things:
      * CAP.SSS    - Support for staggered spin-up            (t/f)
-- 
2.47.1

Re: [RFC PATCH 0/2] hw/ide/ahci: Delay a bit before completing reset

[Jiaxun Yang]

在2025年5月26日周一 下午7:05，Philippe Mathieu-Daudé写道：
> Intented to help SeaBIOS development; untested there
> (except with QEMU test suite).
>
> Jiaxun, is it helpful to you?


Hi Philippe,

Thanks for the proposal!

The spec says:

```
HBA Reset (HR): When set by SW, this bit causes an internal reset of the HBA. All
state machines that relate to data transfers and queuing shall return to an idle
condition, and all ports shall be re-initialized via COMRESET (if staggered spin-up is
not supported). If staggered spin-up is supported, then it is the responsibility of
software to spin-up each port after the reset has completed.

When the HBA has performed the reset action, it shall reset this bit to ‘0’. A software
write of ‘0’ shall have no effect. For a description on which bits are reset when this bit is
set, see section 10.4.3.
```

I do believe QEMU's current implementation is also in conformance to the spec,
as the reset process itself is done instantly in QEMU.

I don't know if it's worth it to introduce extra complexity in QEMU to model
a very specific hardware behaviour. Even some hardware is working in QEMU's way.

[...]

Thanks
-- 
- Jiaxun

Re: [RFC PATCH 0/2] hw/ide/ahci: Delay a bit before completing reset

[Gerd Hoffmann]

  Hi,

> I do believe QEMU's current implementation is also in conformance to the spec,
> as the reset process itself is done instantly in QEMU.

Yes, that is fine spec-wise.  The problem is the seabios driver which
doesn't wait until the hardware signals completion.

> I don't know if it's worth it to introduce extra complexity in QEMU to model
> a very specific hardware behaviour. Even some hardware is working in QEMU's way.

We have that kind of differences between virtual and physical hardware
in other places too.  Timing is notoriously difficult to emulate.  Often
qemu completes hardware actions faster than physical hardware, or it at
least looks that way to the guest because it does not get CPU time until
qemu is done.

One way to change that would be to have all mmio writes return instantly
and only kick a timer or BH which runs the actual action.  I'm not
convinced this is worth the effort.  If you think it is I'd suggest to
move the complete driver to that mode of operation instead of adding a
reset tweak only.

take care,
  Gerd

Re: [RFC PATCH 0/2] hw/ide/ahci: Delay a bit before completing reset

[Philippe Mathieu-Daudé]

On 27/5/25 12:45, Gerd Hoffmann wrote:
>    Hi,
> 
>> I do believe QEMU's current implementation is also in conformance to the spec,
>> as the reset process itself is done instantly in QEMU.
> 
> Yes, that is fine spec-wise.  The problem is the seabios driver which
> doesn't wait until the hardware signals completion.
> 
>> I don't know if it's worth it to introduce extra complexity in QEMU to model
>> a very specific hardware behaviour. Even some hardware is working in QEMU's way.
> 
> We have that kind of differences between virtual and physical hardware
> in other places too.  Timing is notoriously difficult to emulate.  Often
> qemu completes hardware actions faster than physical hardware, or it at
> least looks that way to the guest because it does not get CPU time until
> qemu is done.
> 
> One way to change that would be to have all mmio writes return instantly
> and only kick a timer or BH which runs the actual action.  I'm not
> convinced this is worth the effort.  If you think it is I'd suggest to
> move the complete driver to that mode of operation instead of adding a
> reset tweak only.

Nah I'm fine ;)