[PATCH v3 0/8] misc AHCI cleanups

[PATCH v3 0/8] misc AHCI cleanups

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Hello John,

Here comes some misc AHCI cleanups.

Most are related to error handling.

Please review.

Changes since v2:
-Squashed in the test commits that were sent out as a separate series into
 the patch "hw/ide/ahci: PxCI should not get cleared when ERR_STAT is set",
 and reordered some of the patches, such that each and every commit passes
 the ahci test suite as a separate unit. This way it will be possible to
 perform a git bisect without seeing any failures in the ahci test suite.


Kind regards,
Niklas

Niklas Cassel (8):
  hw/ide/ahci: remove stray backslash
  hw/ide/core: set ERR_STAT in unsupported command completion
  hw/ide/ahci: write D2H FIS when processing NCQ command
  hw/ide/ahci: simplify and document PxCI handling
  hw/ide/ahci: PxSACT and PxCI is cleared when PxCMD.ST is cleared
  hw/ide/ahci: PxCI should not get cleared when ERR_STAT is set
  hw/ide/ahci: fix ahci_write_fis_sdb()
  hw/ide/ahci: fix broken SError handling

 hw/ide/ahci.c             | 112 +++++++++++++++++++++++++++-----------
 hw/ide/core.c             |   2 +-
 tests/qtest/libqos/ahci.c | 106 +++++++++++++++++++++++++++---------
 tests/qtest/libqos/ahci.h |   8 +--
 4 files changed, 164 insertions(+), 64 deletions(-)

-- 
2.40.1

[PATCH v3 1/8] hw/ide/ahci: remove stray backslash

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

This backslash obviously does not belong here, so remove it.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 4e76d6b191..48d550f633 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -690,7 +690,7 @@ static void ahci_reset_port(AHCIState *s, int port)
 
     s->dev[port].port_state = STATE_RUN;
     if (ide_state->drive_kind == IDE_CD) {
-        ahci_set_signature(d, SATA_SIGNATURE_CDROM);\
+        ahci_set_signature(d, SATA_SIGNATURE_CDROM);
         ide_state->status = SEEK_STAT | WRERR_STAT | READY_STAT;
     } else {
         ahci_set_signature(d, SATA_SIGNATURE_DISK);
-- 
2.40.1

[PATCH v3 2/8] hw/ide/core: set ERR_STAT in unsupported command completion

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

Currently, the first time sending an unsupported command
(e.g. READ LOG DMA EXT) will not have ERR_STAT set in the completion.
Sending the unsupported command again, will correctly have ERR_STAT set.

When ide_cmd_permitted() returns false, it calls ide_abort_command().
ide_abort_command() first calls ide_transfer_stop(), which will call
ide_transfer_halt() and ide_cmd_done(), after that ide_abort_command()
sets ERR_STAT in status.

ide_cmd_done() for AHCI will call ahci_write_fis_d2h() which writes the
current status in the FIS, and raises an IRQ. (The status here will not
have ERR_STAT set!).

Thus, we cannot call ide_transfer_stop() before setting ERR_STAT, as
ide_transfer_stop() will result in the FIS being written and an IRQ
being raised.

The reason why it works the second time, is that ERR_STAT will still
be set from the previous command, so when writing the FIS, the
completion will correctly have ERR_STAT set.

Set ERR_STAT before writing the FIS (calling cmd_done), so that we will
raise an error IRQ correctly when receiving an unsupported command.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 hw/ide/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index de48ff9f86..07971c0218 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -533,9 +533,9 @@ BlockAIOCB *ide_issue_trim(
 
 void ide_abort_command(IDEState *s)
 {
-    ide_transfer_stop(s);
     s->status = READY_STAT | ERR_STAT;
     s->error = ABRT_ERR;
+    ide_transfer_stop(s);
 }
 
 static void ide_set_retry(IDEState *s)
-- 
2.40.1

Re: [PATCH v3 2/8] hw/ide/core: set ERR_STAT in unsupported command completion

[Philippe Mathieu-Daudé]

On 9/6/23 16:08, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Currently, the first time sending an unsupported command
> (e.g. READ LOG DMA EXT) will not have ERR_STAT set in the completion.
> Sending the unsupported command again, will correctly have ERR_STAT set.
> 
> When ide_cmd_permitted() returns false, it calls ide_abort_command().
> ide_abort_command() first calls ide_transfer_stop(), which will call
> ide_transfer_halt() and ide_cmd_done(), after that ide_abort_command()
> sets ERR_STAT in status.
> 
> ide_cmd_done() for AHCI will call ahci_write_fis_d2h() which writes the
> current status in the FIS, and raises an IRQ. (The status here will not
> have ERR_STAT set!).
> 
> Thus, we cannot call ide_transfer_stop() before setting ERR_STAT, as
> ide_transfer_stop() will result in the FIS being written and an IRQ
> being raised.
> 
> The reason why it works the second time, is that ERR_STAT will still
> be set from the previous command, so when writing the FIS, the
> completion will correctly have ERR_STAT set.
> 
> Set ERR_STAT before writing the FIS (calling cmd_done), so that we will
> raise an error IRQ correctly when receiving an unsupported command.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   hw/ide/core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 3/8] hw/ide/ahci: write D2H FIS when processing NCQ command

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

The way that BUSY + PxCI is cleared for NCQ (FPDMA QUEUED) commands is
described in SATA 3.5a Gold:

11.15 FPDMA QUEUED command protocol
DFPDMAQ2: ClearInterfaceBsy
"Transmit Register Device to Host FIS with the BSY bit cleared to zero
and the DRQ bit cleared to zero and Interrupt bit cleared to zero to
mark interface ready for the next command."

PxCI is currently cleared by handle_cmd(), but we don't write the D2H
FIS to the FIS Receive Area that actually caused PxCI to be cleared.

Similar to how ahci_pio_transfer() calls ahci_write_fis_pio() with an
additional parameter to write a PIO Setup FIS without raising an IRQ,
add a parameter to ahci_write_fis_d2h() so that ahci_write_fis_d2h()
also can write the FIS to the FIS Receive Area without raising an IRQ.

Change process_ncq_command() to call ahci_write_fis_d2h() without
raising an IRQ (similar to ahci_pio_transfer()), such that the FIS
Receive Area is in sync with the PxTFD shadow register.

E.g. Linux reads status and error fields from the FIS Receive Area
directly, so it is wise to keep the FIS Receive Area and the PxTFD
shadow register in sync.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 hw/ide/ahci.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 48d550f633..4b272397fd 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -43,7 +43,7 @@
 static void check_cmd(AHCIState *s, int port);
 static int handle_cmd(AHCIState *s, int port, uint8_t slot);
 static void ahci_reset_port(AHCIState *s, int port);
-static bool ahci_write_fis_d2h(AHCIDevice *ad);
+static bool ahci_write_fis_d2h(AHCIDevice *ad, bool d2h_fis_i);
 static void ahci_init_d2h(AHCIDevice *ad);
 static int ahci_dma_prepare_buf(const IDEDMA *dma, int32_t limit);
 static bool ahci_map_clb_address(AHCIDevice *ad);
@@ -618,7 +618,7 @@ static void ahci_init_d2h(AHCIDevice *ad)
         return;
     }
 
-    if (ahci_write_fis_d2h(ad)) {
+    if (ahci_write_fis_d2h(ad, true)) {
         ad->init_d2h_sent = true;
         /* We're emulating receiving the first Reg H2D Fis from the device;
          * Update the SIG register, but otherwise proceed as normal. */
@@ -850,7 +850,7 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len, bool pio_fis_i)
     }
 }
 
-static bool ahci_write_fis_d2h(AHCIDevice *ad)
+static bool ahci_write_fis_d2h(AHCIDevice *ad, bool d2h_fis_i)
 {
     AHCIPortRegs *pr = &ad->port_regs;
     uint8_t *d2h_fis;
@@ -864,7 +864,7 @@ static bool ahci_write_fis_d2h(AHCIDevice *ad)
     d2h_fis = &ad->res_fis[RES_FIS_RFIS];
 
     d2h_fis[0] = SATA_FIS_TYPE_REGISTER_D2H;
-    d2h_fis[1] = (1 << 6); /* interrupt bit */
+    d2h_fis[1] = d2h_fis_i ? (1 << 6) : 0; /* interrupt bit */
     d2h_fis[2] = s->status;
     d2h_fis[3] = s->error;
 
@@ -890,7 +890,10 @@ static bool ahci_write_fis_d2h(AHCIDevice *ad)
         ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
     }
 
-    ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_DHRS);
+    if (d2h_fis_i) {
+        ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_DHRS);
+    }
+
     return true;
 }
 
@@ -1120,6 +1123,8 @@ static void process_ncq_command(AHCIState *s, int port, const uint8_t *cmd_fis,
         return;
     }
 
+    ahci_write_fis_d2h(ad, false);
+
     ncq_tfs->used = 1;
     ncq_tfs->drive = ad;
     ncq_tfs->slot = slot;
@@ -1506,7 +1511,7 @@ static void ahci_cmd_done(const IDEDMA *dma)
     }
 
     /* update d2h status */
-    ahci_write_fis_d2h(ad);
+    ahci_write_fis_d2h(ad, true);
 
     if (ad->port_regs.cmd_issue && !ad->check_bh) {
         ad->check_bh = qemu_bh_new_guarded(ahci_check_cmd_bh, ad,
-- 
2.40.1

[PATCH v3 4/8] hw/ide/ahci: simplify and document PxCI handling

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

The AHCI spec states that:
For NCQ, PxCI is cleared on command queued successfully.

For non-NCQ, PxCI is cleared on command completed successfully.
(A non-NCQ command that completes with error does not clear PxCI.)

The current QEMU implementation either clears PxCI in check_cmd(),
or in ahci_cmd_done().

check_cmd() will clear PxCI for a command if handle_cmd() returns 0.
handle_cmd() will return -1 if BUSY or DRQ is set.

The QEMU implementation for NCQ commands will currently not set BUSY
or DRQ, so they will always have PxCI cleared by handle_cmd().
ahci_cmd_done() will never even get called for NCQ commands.

Non-NCQ commands are executed by ide_bus_exec_cmd().
Non-NCQ commands in QEMU are implemented either in a sync or in an async
way.

For non-NCQ commands implemented in a sync way, the command handler will
return true, and when ide_bus_exec_cmd() sees that a command handler
returns true, it will call ide_cmd_done() (which will call
ahci_cmd_done()). For a command implemented in a sync way,
ahci_cmd_done() will do nothing (since busy_slot is not set). Instead,
after ide_bus_exec_cmd() has finished, check_cmd() will clear PxCI for
these commands.

For non-NCQ commands implemented in an async way (using either aiocb or
pio_aiocb), the command handler will return false, ide_bus_exec_cmd()
will not call ide_cmd_done(), instead it is expected that the async
callback function will call ide_cmd_done() once the async command is
done. handle_cmd() will set busy_slot, if and only if BUSY or DRQ is
set, and this is checked _after_ ide_bus_exec_cmd() has returned.
handle_cmd() will return -1, so check_cmd() will not clear PxCI.
When the async callback calls ide_cmd_done() (which will call
ahci_cmd_done()), it will see that busy_slot is set, and
ahci_cmd_done() will clear PxCI.

This seems racy, since busy_slot is set _after_ ide_bus_exec_cmd() has
returned. The callback might come before busy_slot gets set. And it is
quite confusing that ahci_cmd_done() will be called for all non-NCQ
commands when the command is done, but will only clear PxCI in certain
cases, even though it will always write a D2H FIS and raise an IRQ.

Even worse, in the case where ahci_cmd_done() does not clear PxCI, it
still raises an IRQ. Host software might thus read an old PxCI value,
since PxCI is cleared (by check_cmd()) after the IRQ has been raised.

Try to simplify this by always setting busy_slot for non-NCQ commands,
such that ahci_cmd_done() will always be responsible for clearing PxCI
for non-NCQ commands.

For NCQ commands, clear PxCI when we receive the D2H FIS, but before
raising the IRQ, see AHCI 1.3.1, section 5.3.8, states RegFIS:Entry and
RegFIS:ClearCI.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 hw/ide/ahci.c | 70 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 50 insertions(+), 20 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 4b272397fd..3deaf01add 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -41,9 +41,10 @@
 #include "trace.h"
 
 static void check_cmd(AHCIState *s, int port);
-static int handle_cmd(AHCIState *s, int port, uint8_t slot);
+static void handle_cmd(AHCIState *s, int port, uint8_t slot);
 static void ahci_reset_port(AHCIState *s, int port);
 static bool ahci_write_fis_d2h(AHCIDevice *ad, bool d2h_fis_i);
+static void ahci_clear_cmd_issue(AHCIDevice *ad, uint8_t slot);
 static void ahci_init_d2h(AHCIDevice *ad);
 static int ahci_dma_prepare_buf(const IDEDMA *dma, int32_t limit);
 static bool ahci_map_clb_address(AHCIDevice *ad);
@@ -591,9 +592,8 @@ static void check_cmd(AHCIState *s, int port)
 
     if ((pr->cmd & PORT_CMD_START) && pr->cmd_issue) {
         for (slot = 0; (slot < 32) && pr->cmd_issue; slot++) {
-            if ((pr->cmd_issue & (1U << slot)) &&
-                !handle_cmd(s, port, slot)) {
-                pr->cmd_issue &= ~(1U << slot);
+            if (pr->cmd_issue & (1U << slot)) {
+                handle_cmd(s, port, slot);
             }
         }
     }
@@ -1123,6 +1123,22 @@ static void process_ncq_command(AHCIState *s, int port, const uint8_t *cmd_fis,
         return;
     }
 
+    /*
+     * A NCQ command clears the bit in PxCI after the command has been QUEUED
+     * successfully (ERROR not set, BUSY and DRQ cleared).
+     *
+     * For NCQ commands, PxCI will always be cleared here.
+     *
+     * (Once the NCQ command is COMPLETED, the device will send a SDB FIS with
+     * the interrupt bit set, which will clear PxSACT and raise an interrupt.)
+     */
+    ahci_clear_cmd_issue(ad, slot);
+
+    /*
+     * In reality, for NCQ commands, PxCI is cleared after receiving a D2H FIS
+     * without the interrupt bit set, but since ahci_write_fis_d2h() can raise
+     * an IRQ on error, we need to call them in reverse order.
+     */
     ahci_write_fis_d2h(ad, false);
 
     ncq_tfs->used = 1;
@@ -1197,6 +1213,7 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
 {
     IDEState *ide_state = &s->dev[port].port.ifs[0];
     AHCICmdHdr *cmd = get_cmd_header(s, port, slot);
+    AHCIDevice *ad = &s->dev[port];
     uint16_t opts = le16_to_cpu(cmd->opts);
 
     if (cmd_fis[1] & 0x0F) {
@@ -1273,11 +1290,19 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
     /* Reset transferred byte counter */
     cmd->status = 0;
 
+    /*
+     * A non-NCQ command clears the bit in PxCI after the command has COMPLETED
+     * successfully (ERROR not set, BUSY and DRQ cleared).
+     *
+     * For non-NCQ commands, PxCI will always be cleared by ahci_cmd_done().
+     */
+    ad->busy_slot = slot;
+
     /* We're ready to process the command in FIS byte 2. */
     ide_bus_exec_cmd(&s->dev[port].port, cmd_fis[2]);
 }
 
-static int handle_cmd(AHCIState *s, int port, uint8_t slot)
+static void handle_cmd(AHCIState *s, int port, uint8_t slot)
 {
     IDEState *ide_state;
     uint64_t tbl_addr;
@@ -1288,12 +1313,12 @@ static int handle_cmd(AHCIState *s, int port, uint8_t slot)
     if (s->dev[port].port.ifs[0].status & (BUSY_STAT|DRQ_STAT)) {
         /* Engine currently busy, try again later */
         trace_handle_cmd_busy(s, port);
-        return -1;
+        return;
     }
 
     if (!s->dev[port].lst) {
         trace_handle_cmd_nolist(s, port);
-        return -1;
+        return;
     }
     cmd = get_cmd_header(s, port, slot);
     /* remember current slot handle for later */
@@ -1303,7 +1328,7 @@ static int handle_cmd(AHCIState *s, int port, uint8_t slot)
     ide_state = &s->dev[port].port.ifs[0];
     if (!ide_state->blk) {
         trace_handle_cmd_badport(s, port);
-        return -1;
+        return;
     }
 
     tbl_addr = le64_to_cpu(cmd->tbl_addr);
@@ -1312,7 +1337,7 @@ static int handle_cmd(AHCIState *s, int port, uint8_t slot)
                              DMA_DIRECTION_TO_DEVICE, MEMTXATTRS_UNSPECIFIED);
     if (!cmd_fis) {
         trace_handle_cmd_badfis(s, port);
-        return -1;
+        return;
     } else if (cmd_len != 0x80) {
         ahci_trigger_irq(s, &s->dev[port], AHCI_PORT_IRQ_BIT_HBFS);
         trace_handle_cmd_badmap(s, port, cmd_len);
@@ -1336,15 +1361,6 @@ static int handle_cmd(AHCIState *s, int port, uint8_t slot)
 out:
     dma_memory_unmap(s->as, cmd_fis, cmd_len, DMA_DIRECTION_TO_DEVICE,
                      cmd_len);
-
-    if (s->dev[port].port.ifs[0].status & (BUSY_STAT|DRQ_STAT)) {
-        /* async command, complete later */
-        s->dev[port].busy_slot = slot;
-        return -1;
-    }
-
-    /* done handling the command */
-    return 0;
 }
 
 /* Transfer PIO data between RAM and device */
@@ -1498,6 +1514,16 @@ static int ahci_dma_rw_buf(const IDEDMA *dma, bool is_write)
     return 1;
 }
 
+static void ahci_clear_cmd_issue(AHCIDevice *ad, uint8_t slot)
+{
+    IDEState *ide_state = &ad->port.ifs[0];
+
+    if (!(ide_state->status & (BUSY_STAT | DRQ_STAT))) {
+        ad->port_regs.cmd_issue &= ~(1 << slot);
+    }
+}
+
+/* Non-NCQ command is done - This function is never called for NCQ commands. */
 static void ahci_cmd_done(const IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
@@ -1506,11 +1532,15 @@ static void ahci_cmd_done(const IDEDMA *dma)
 
     /* no longer busy */
     if (ad->busy_slot != -1) {
-        ad->port_regs.cmd_issue &= ~(1 << ad->busy_slot);
+        ahci_clear_cmd_issue(ad, ad->busy_slot);
         ad->busy_slot = -1;
     }
 
-    /* update d2h status */
+    /*
+     * In reality, for non-NCQ commands, PxCI is cleared after receiving a D2H
+     * FIS with the interrupt bit set, but since ahci_write_fis_d2h() will raise
+     * an IRQ, we need to call them in reverse order.
+     */
     ahci_write_fis_d2h(ad, true);
 
     if (ad->port_regs.cmd_issue && !ad->check_bh) {
-- 
2.40.1

[PATCH v3 5/8] hw/ide/ahci: PxSACT and PxCI is cleared when PxCMD.ST is cleared

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

According to AHCI 1.3.1 definition of PxSACT:
This field is cleared when PxCMD.ST is written from a '1' to a '0' by
software. This field is not cleared by a COMRESET or a software reset.

According to AHCI 1.3.1 definition of PxCI:
This field is also cleared when PxCMD.ST is written from a '1' to a '0'
by software.

Clearing PxCMD.ST is part of the error recovery procedure, see
AHCI 1.3.1, section "6.2 Error Recovery".

If we don't clear PxCI on error recovery, the previous command will
incorrectly still be marked as pending after error recovery.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 hw/ide/ahci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 3deaf01add..a31e6fa65e 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -329,6 +329,11 @@ static void ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
         ahci_check_irq(s);
         break;
     case AHCI_PORT_REG_CMD:
+        if ((pr->cmd & PORT_CMD_START) && !(val & PORT_CMD_START)) {
+            pr->scr_act = 0;
+            pr->cmd_issue = 0;
+        }
+
         /* Block any Read-only fields from being set;
          * including LIST_ON and FIS_ON.
          * The spec requires to set ICC bits to zero after the ICC change
-- 
2.40.1

Re: [PATCH v3 5/8] hw/ide/ahci: PxSACT and PxCI is cleared when PxCMD.ST is cleared

[Philippe Mathieu-Daudé]

On 9/6/23 16:08, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> According to AHCI 1.3.1 definition of PxSACT:
> This field is cleared when PxCMD.ST is written from a '1' to a '0' by
> software. This field is not cleared by a COMRESET or a software reset.

Interesting, since its origin in commit f6ad2e32f8 ("ahci: add ahci
emulation") PxSACT is reset unconditionally in ahci_reset_port().

As for this patch:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> According to AHCI 1.3.1 definition of PxCI:
> This field is also cleared when PxCMD.ST is written from a '1' to a '0'
> by software.
> 
> Clearing PxCMD.ST is part of the error recovery procedure, see
> AHCI 1.3.1, section "6.2 Error Recovery".
> 
> If we don't clear PxCI on error recovery, the previous command will
> incorrectly still be marked as pending after error recovery.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   hw/ide/ahci.c | 5 +++++
>   1 file changed, 5 insertions(+)

[PATCH v3 6/8] hw/ide/ahci: PxCI should not get cleared when ERR_STAT is set

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

For NCQ, PxCI is cleared on command queued successfully.
For non-NCQ, PxCI is cleared on command completed successfully.
Successfully means ERR_STAT, BUSY and DRQ are all cleared.

A command that has ERR_STAT set, does not get to clear PxCI.
See AHCI 1.3.1, section 5.3.8, states RegFIS:Entry and RegFIS:ClearCI,
and 5.3.16.5 ERR:FatalTaskfile.

In the case of non-NCQ commands, not clearing PxCI is needed in order
for host software to be able to see which command slot that failed.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 hw/ide/ahci.c             |   7 ++-
 tests/qtest/libqos/ahci.c | 106 ++++++++++++++++++++++++++++----------
 tests/qtest/libqos/ahci.h |   8 ++-
 3 files changed, 88 insertions(+), 33 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index a31e6fa65e..12aaadc554 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1523,7 +1523,8 @@ static void ahci_clear_cmd_issue(AHCIDevice *ad, uint8_t slot)
 {
     IDEState *ide_state = &ad->port.ifs[0];
 
-    if (!(ide_state->status & (BUSY_STAT | DRQ_STAT))) {
+    if (!(ide_state->status & ERR_STAT) &&
+        !(ide_state->status & (BUSY_STAT | DRQ_STAT))) {
         ad->port_regs.cmd_issue &= ~(1 << slot);
     }
 }
@@ -1532,6 +1533,7 @@ static void ahci_clear_cmd_issue(AHCIDevice *ad, uint8_t slot)
 static void ahci_cmd_done(const IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
+    IDEState *ide_state = &ad->port.ifs[0];
 
     trace_ahci_cmd_done(ad->hba, ad->port_no);
 
@@ -1548,7 +1550,8 @@ static void ahci_cmd_done(const IDEDMA *dma)
      */
     ahci_write_fis_d2h(ad, true);
 
-    if (ad->port_regs.cmd_issue && !ad->check_bh) {
+    if (!(ide_state->status & ERR_STAT) &&
+        ad->port_regs.cmd_issue && !ad->check_bh) {
         ad->check_bh = qemu_bh_new_guarded(ahci_check_cmd_bh, ad,
                                            &ad->mem_reentrancy_guard);
         qemu_bh_schedule(ad->check_bh);
diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c
index f53f12aa99..a2c94c6e06 100644
--- a/tests/qtest/libqos/ahci.c
+++ b/tests/qtest/libqos/ahci.c
@@ -404,57 +404,110 @@ void ahci_port_clear(AHCIQState *ahci, uint8_t port)
 /**
  * Check a port for errors.
  */
-void ahci_port_check_error(AHCIQState *ahci, uint8_t port,
-                           uint32_t imask, uint8_t emask)
+void ahci_port_check_error(AHCIQState *ahci, AHCICommand *cmd)
 {
+    uint8_t port = cmd->port;
     uint32_t reg;
 
-    /* The upper 9 bits of the IS register all indicate errors. */
-    reg = ahci_px_rreg(ahci, port, AHCI_PX_IS);
-    reg &= ~imask;
-    reg >>= 23;
-    g_assert_cmphex(reg, ==, 0);
+    /* If expecting TF error, ensure that TFES is set. */
+    if (cmd->errors) {
+        reg = ahci_px_rreg(ahci, port, AHCI_PX_IS);
+        ASSERT_BIT_SET(reg, AHCI_PX_IS_TFES);
+    } else {
+        /* The upper 9 bits of the IS register all indicate errors. */
+        reg = ahci_px_rreg(ahci, port, AHCI_PX_IS);
+        reg &= ~cmd->interrupts;
+        reg >>= 23;
+        g_assert_cmphex(reg, ==, 0);
+    }
 
-    /* The Sata Error Register should be empty. */
+    /* The Sata Error Register should be empty, even when expecting TF error. */
     reg = ahci_px_rreg(ahci, port, AHCI_PX_SERR);
     g_assert_cmphex(reg, ==, 0);
 
+    /* If expecting TF error, and TFES was set, perform error recovery
+     * (see AHCI 1.3 section 6.2.2.1) such that we can send new commands. */
+    if (cmd->errors) {
+        /* This will clear PxCI. */
+        ahci_px_clr(ahci, port, AHCI_PX_CMD, AHCI_PX_CMD_ST);
+
+        /* The port has 500ms to disengage. */
+        usleep(500000);
+        reg = ahci_px_rreg(ahci, port, AHCI_PX_CMD);
+        ASSERT_BIT_CLEAR(reg, AHCI_PX_CMD_CR);
+
+        /* Clear PxIS. */
+        reg = ahci_px_rreg(ahci, port, AHCI_PX_IS);
+        ahci_px_wreg(ahci, port, AHCI_PX_IS, reg);
+
+        /* Check if we need to perform a COMRESET.
+         * Not implemented right now, as there is no reason why our QEMU model
+         * should need a COMRESET when expecting TF error. */
+        reg = ahci_px_rreg(ahci, port, AHCI_PX_TFD);
+        ASSERT_BIT_CLEAR(reg, AHCI_PX_TFD_STS_BSY | AHCI_PX_TFD_STS_DRQ);
+
+        /* Enable issuing new commands. */
+        ahci_px_set(ahci, port, AHCI_PX_CMD, AHCI_PX_CMD_ST);
+    }
+
     /* The TFD also has two error sections. */
     reg = ahci_px_rreg(ahci, port, AHCI_PX_TFD);
-    if (!emask) {
+    if (!cmd->errors) {
         ASSERT_BIT_CLEAR(reg, AHCI_PX_TFD_STS_ERR);
     } else {
         ASSERT_BIT_SET(reg, AHCI_PX_TFD_STS_ERR);
     }
-    ASSERT_BIT_CLEAR(reg, AHCI_PX_TFD_ERR & (~emask << 8));
-    ASSERT_BIT_SET(reg, AHCI_PX_TFD_ERR & (emask << 8));
+    ASSERT_BIT_CLEAR(reg, AHCI_PX_TFD_ERR & (~cmd->errors << 8));
+    ASSERT_BIT_SET(reg, AHCI_PX_TFD_ERR & (cmd->errors << 8));
 }
 
-void ahci_port_check_interrupts(AHCIQState *ahci, uint8_t port,
-                                uint32_t intr_mask)
+void ahci_port_check_interrupts(AHCIQState *ahci, AHCICommand *cmd)
 {
+    uint8_t port = cmd->port;
     uint32_t reg;
 
+    /* If we expect errors, error handling in ahci_port_check_error() will
+     * already have cleared PxIS, so in that case this function cannot verify
+     * and clear expected interrupts. */
+    if (cmd->errors) {
+        return;
+    }
+
     /* Check for expected interrupts */
     reg = ahci_px_rreg(ahci, port, AHCI_PX_IS);
-    ASSERT_BIT_SET(reg, intr_mask);
+    ASSERT_BIT_SET(reg, cmd->interrupts);
 
     /* Clear expected interrupts and assert all interrupts now cleared. */
-    ahci_px_wreg(ahci, port, AHCI_PX_IS, intr_mask);
+    ahci_px_wreg(ahci, port, AHCI_PX_IS, cmd->interrupts);
     g_assert_cmphex(ahci_px_rreg(ahci, port, AHCI_PX_IS), ==, 0);
 }
 
-void ahci_port_check_nonbusy(AHCIQState *ahci, uint8_t port, uint8_t slot)
+void ahci_port_check_nonbusy(AHCIQState *ahci, AHCICommand *cmd)
 {
+    uint8_t slot = cmd->slot;
+    uint8_t port = cmd->port;
     uint32_t reg;
 
-    /* Assert that the command slot is no longer busy (NCQ) */
+    /* For NCQ command with error PxSACT bit should still be set.
+     * For NCQ command without error, PxSACT bit should be cleared.
+     * For non-NCQ command, PxSACT bit should always be cleared. */
     reg = ahci_px_rreg(ahci, port, AHCI_PX_SACT);
-    ASSERT_BIT_CLEAR(reg, (1 << slot));
+    if (cmd->props->ncq && cmd->errors) {
+        ASSERT_BIT_SET(reg, (1 << slot));
+    } else {
+        ASSERT_BIT_CLEAR(reg, (1 << slot));
+    }
 
-    /* Non-NCQ */
+    /* For non-NCQ command with error, PxCI bit should still be set.
+     * For non-NCQ command without error, PxCI bit should be cleared.
+     * For NCQ command without error, PxCI bit should be cleared.
+     * For NCQ command with error, PxCI bit may or may not be cleared. */
     reg = ahci_px_rreg(ahci, port, AHCI_PX_CI);
-    ASSERT_BIT_CLEAR(reg, (1 << slot));
+    if (!cmd->props->ncq && cmd->errors) {
+        ASSERT_BIT_SET(reg, (1 << slot));
+    } else if (!cmd->errors) {
+        ASSERT_BIT_CLEAR(reg, (1 << slot));
+    }
 
     /* And assert that we are generally not busy. */
     reg = ahci_px_rreg(ahci, port, AHCI_PX_TFD);
@@ -1207,9 +1260,10 @@ void ahci_command_wait(AHCIQState *ahci, AHCICommand *cmd)
 
 #define RSET(REG, MASK) (BITSET(ahci_px_rreg(ahci, cmd->port, (REG)), (MASK)))
 
-    while (RSET(AHCI_PX_TFD, AHCI_PX_TFD_STS_BSY) ||
-           RSET(AHCI_PX_CI, 1 << cmd->slot) ||
-           (cmd->props->ncq && RSET(AHCI_PX_SACT, 1 << cmd->slot))) {
+    while (!RSET(AHCI_PX_TFD, AHCI_PX_TFD_STS_ERR) &&
+           (RSET(AHCI_PX_TFD, AHCI_PX_TFD_STS_BSY) ||
+            RSET(AHCI_PX_CI, 1 << cmd->slot) ||
+            (cmd->props->ncq && RSET(AHCI_PX_SACT, 1 << cmd->slot)))) {
         usleep(50);
     }
 
@@ -1226,9 +1280,9 @@ void ahci_command_verify(AHCIQState *ahci, AHCICommand *cmd)
     uint8_t slot = cmd->slot;
     uint8_t port = cmd->port;
 
-    ahci_port_check_error(ahci, port, cmd->interrupts, cmd->errors);
-    ahci_port_check_interrupts(ahci, port, cmd->interrupts);
-    ahci_port_check_nonbusy(ahci, port, slot);
+    ahci_port_check_nonbusy(ahci, cmd);
+    ahci_port_check_error(ahci, cmd);
+    ahci_port_check_interrupts(ahci, cmd);
     ahci_port_check_cmd_sanity(ahci, cmd);
     if (cmd->interrupts & AHCI_PX_IS_DHRS) {
         ahci_port_check_d2h_sanity(ahci, port, slot);
diff --git a/tests/qtest/libqos/ahci.h b/tests/qtest/libqos/ahci.h
index 88835b6228..48017864bf 100644
--- a/tests/qtest/libqos/ahci.h
+++ b/tests/qtest/libqos/ahci.h
@@ -590,11 +590,9 @@ void ahci_set_command_header(AHCIQState *ahci, uint8_t port,
 void ahci_destroy_command(AHCIQState *ahci, uint8_t port, uint8_t slot);
 
 /* AHCI sanity check routines */
-void ahci_port_check_error(AHCIQState *ahci, uint8_t port,
-                           uint32_t imask, uint8_t emask);
-void ahci_port_check_interrupts(AHCIQState *ahci, uint8_t port,
-                                uint32_t intr_mask);
-void ahci_port_check_nonbusy(AHCIQState *ahci, uint8_t port, uint8_t slot);
+void ahci_port_check_error(AHCIQState *ahci, AHCICommand *cmd);
+void ahci_port_check_interrupts(AHCIQState *ahci, AHCICommand *cmd);
+void ahci_port_check_nonbusy(AHCIQState *ahci, AHCICommand *cmd);
 void ahci_port_check_d2h_sanity(AHCIQState *ahci, uint8_t port, uint8_t slot);
 void ahci_port_check_pio_sanity(AHCIQState *ahci, AHCICommand *cmd);
 void ahci_port_check_cmd_sanity(AHCIQState *ahci, AHCICommand *cmd);
-- 
2.40.1

[PATCH v3 7/8] hw/ide/ahci: fix ahci_write_fis_sdb()

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

When there is an error, we need to raise a TFES error irq, see AHCI 1.3.1,
5.3.13.1 SDB:Entry.

If ERR_STAT is set, we jump to state ERR:FatalTaskfile, which will raise
a TFES IRQ unconditionally, regardless if the I bit is set in the FIS or
not.

Thus, we should never raise a normal IRQ after having sent an error IRQ.

It is valid to signal successfully completed commands as finished in the
same SDB FIS that generates the error IRQ. The important thing is that
commands that did not complete successfully (e.g. commands that were
aborted, do not get the finished bit set).

Before this commit, there was never a TFES IRQ raised on NCQ error.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 hw/ide/ahci.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 12aaadc554..ef6c9fc378 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -806,8 +806,14 @@ static void ahci_write_fis_sdb(AHCIState *s, NCQTransferState *ncq_tfs)
     pr->scr_act &= ~ad->finished;
     ad->finished = 0;
 
-    /* Trigger IRQ if interrupt bit is set (which currently, it always is) */
-    if (sdb_fis->flags & 0x40) {
+    /*
+     * TFES IRQ is always raised if ERR_STAT is set, regardless of I bit.
+     * If ERR_STAT is not set, trigger SDBS IRQ if interrupt bit is set
+     * (which currently, it always is).
+     */
+    if (sdb_fis->status & ERR_STAT) {
+        ahci_trigger_irq(s, ad, AHCI_PORT_IRQ_BIT_TFES);
+    } else if (sdb_fis->flags & 0x40) {
         ahci_trigger_irq(s, ad, AHCI_PORT_IRQ_BIT_SDBS);
     }
 }
-- 
2.40.1

Re: [PATCH v3 7/8] hw/ide/ahci: fix ahci_write_fis_sdb()

[Philippe Mathieu-Daudé]

On 9/6/23 16:08, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> When there is an error, we need to raise a TFES error irq, see AHCI 1.3.1,
> 5.3.13.1 SDB:Entry.
> 
> If ERR_STAT is set, we jump to state ERR:FatalTaskfile, which will raise
> a TFES IRQ unconditionally, regardless if the I bit is set in the FIS or
> not.
> 
> Thus, we should never raise a normal IRQ after having sent an error IRQ.
> 
> It is valid to signal successfully completed commands as finished in the
> same SDB FIS that generates the error IRQ. The important thing is that
> commands that did not complete successfully (e.g. commands that were
> aborted, do not get the finished bit set).
> 
> Before this commit, there was never a TFES IRQ raised on NCQ error.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   hw/ide/ahci.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 8/8] hw/ide/ahci: fix broken SError handling

[Niklas Cassel]

From: Niklas Cassel <niklas.cassel@wdc.com>

When encountering an NCQ error, you should not write the NCQ tag to the
SError register. This is completely wrong.

The SError register has a clear definition, where each bit represents a
different error, see PxSERR definition in AHCI 1.3.1.

If we write a random value (like the NCQ tag) in SError, e.g. Linux will
read SError, and will trigger arbitrary error handling depending on the
NCQ tag that happened to be executing.

In case of success, ncq_cb() will call ncq_finish().
In case of error, ncq_cb() will call ncq_err() (which will clear
ncq_tfs->used), and then call ncq_finish(), thus using ncq_tfs->used is
sufficient to tell if finished should get set or not.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 hw/ide/ahci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index ef6c9fc378..d0a774bc17 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1012,7 +1012,6 @@ static void ncq_err(NCQTransferState *ncq_tfs)
 
     ide_state->error = ABRT_ERR;
     ide_state->status = READY_STAT | ERR_STAT;
-    ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag);
     qemu_sglist_destroy(&ncq_tfs->sglist);
     ncq_tfs->used = 0;
 }
@@ -1022,7 +1021,7 @@ static void ncq_finish(NCQTransferState *ncq_tfs)
     /* 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))) {
+    if (ncq_tfs->used) {
         ncq_tfs->drive->finished |= (1 << ncq_tfs->tag);
     }
 
-- 
2.40.1

Re: [PATCH v3 8/8] hw/ide/ahci: fix broken SError handling

[Philippe Mathieu-Daudé]

On 9/6/23 16:08, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> When encountering an NCQ error, you should not write the NCQ tag to the
> SError register. This is completely wrong.
> 
> The SError register has a clear definition, where each bit represents a
> different error, see PxSERR definition in AHCI 1.3.1.
> 
> If we write a random value (like the NCQ tag) in SError, e.g. Linux will
> read SError, and will trigger arbitrary error handling depending on the
> NCQ tag that happened to be executing.
> 
> In case of success, ncq_cb() will call ncq_finish().
> In case of error, ncq_cb() will call ncq_err() (which will clear
> ncq_tfs->used), and then call ncq_finish(), thus using ncq_tfs->used is
> sufficient to tell if finished should get set or not.
> 

Cc: qemu-stable@nongnu.org
Fixes: f6ad2e32f8 ("ahci: add ahci emulation")
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   hw/ide/ahci.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)

Re: [PATCH v3 8/8] hw/ide/ahci: fix broken SError handling

[Michael Tokarev]

19.07.2023 14:59, Philippe Mathieu-Daudé wrote:
> On 9/6/23 16:08, Niklas Cassel wrote:
>> From: Niklas Cassel <niklas.cassel@wdc.com>
>>
>> When encountering an NCQ error, you should not write the NCQ tag to the
>> SError register. This is completely wrong.
>>
>> The SError register has a clear definition, where each bit represents a
>> different error, see PxSERR definition in AHCI 1.3.1.
>>
>> If we write a random value (like the NCQ tag) in SError, e.g. Linux will
>> read SError, and will trigger arbitrary error handling depending on the
>> NCQ tag that happened to be executing.
>>
>> In case of success, ncq_cb() will call ncq_finish().
>> In case of error, ncq_cb() will call ncq_err() (which will clear
>> ncq_tfs->used), and then call ncq_finish(), thus using ncq_tfs->used is
>> sufficient to tell if finished should get set or not.
>>
> 
> Cc: qemu-stable@nongnu.org
> Fixes: f6ad2e32f8 ("ahci: add ahci emulation")

If going this route, it feels like 7/8 "hw/ide/ahci: fix ahci_write_fis_sdb()"
should be there (-stable) too.

/mjt

Re: [PATCH v3 8/8] hw/ide/ahci: fix broken SError handling

[Philippe Mathieu-Daudé]

On 19/7/23 14:06, Michael Tokarev wrote:
> 19.07.2023 14:59, Philippe Mathieu-Daudé wrote:
>> On 9/6/23 16:08, Niklas Cassel wrote:
>>> From: Niklas Cassel <niklas.cassel@wdc.com>
>>>
>>> When encountering an NCQ error, you should not write the NCQ tag to the
>>> SError register. This is completely wrong.
>>>
>>> The SError register has a clear definition, where each bit represents a
>>> different error, see PxSERR definition in AHCI 1.3.1.
>>>
>>> If we write a random value (like the NCQ tag) in SError, e.g. Linux will
>>> read SError, and will trigger arbitrary error handling depending on the
>>> NCQ tag that happened to be executing.
>>>
>>> In case of success, ncq_cb() will call ncq_finish().
>>> In case of error, ncq_cb() will call ncq_err() (which will clear
>>> ncq_tfs->used), and then call ncq_finish(), thus using ncq_tfs->used is
>>> sufficient to tell if finished should get set or not.
>>>
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: f6ad2e32f8 ("ahci: add ahci emulation")
> 
> If going this route, it feels like 7/8 "hw/ide/ahci: fix 
> ahci_write_fis_sdb()"
> should be there (-stable) too.

Certainly; I realized this deserve to be in stable at this final
patch :/

Re: [PATCH v3 0/8] misc AHCI cleanups

[Niklas Cassel]

On Fri, Jun 09, 2023 at 04:08:36PM +0200, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> Hello John,
> 
> Here comes some misc AHCI cleanups.
> 
> Most are related to error handling.
> 
> Please review.
> 
> Changes since v2:
> -Squashed in the test commits that were sent out as a separate series into
>  the patch "hw/ide/ahci: PxCI should not get cleared when ERR_STAT is set",
>  and reordered some of the patches, such that each and every commit passes
>  the ahci test suite as a separate unit. This way it will be possible to
>  perform a git bisect without seeing any failures in the ahci test suite.
> 
> 
> Kind regards,
> Niklas
> 
> Niklas Cassel (8):
>   hw/ide/ahci: remove stray backslash
>   hw/ide/core: set ERR_STAT in unsupported command completion
>   hw/ide/ahci: write D2H FIS when processing NCQ command
>   hw/ide/ahci: simplify and document PxCI handling
>   hw/ide/ahci: PxSACT and PxCI is cleared when PxCMD.ST is cleared
>   hw/ide/ahci: PxCI should not get cleared when ERR_STAT is set
>   hw/ide/ahci: fix ahci_write_fis_sdb()
>   hw/ide/ahci: fix broken SError handling
> 
>  hw/ide/ahci.c             | 112 +++++++++++++++++++++++++++-----------
>  hw/ide/core.c             |   2 +-
>  tests/qtest/libqos/ahci.c | 106 +++++++++++++++++++++++++++---------
>  tests/qtest/libqos/ahci.h |   8 +--
>  4 files changed, 164 insertions(+), 64 deletions(-)
> 
> -- 
> 2.40.1
> 
> 

Hello Philippe,

Considering that you picked up my patch,
"hw/ide/ahci: remove stray backslash" (patch 1/8 in this series),
and since John seems to have gone silent for 40+ days,
could you please consider taking this series through your misc tree?

The series still applies, you just need to skip patch 1/8
(which has already landed in qemu master).


Kind regards,
Niklas

Re: [PATCH v3 0/8] misc AHCI cleanups

[Philippe Mathieu-Daudé]

Hi Niklas, John, Paolo, Kevin,

On 19/7/23 12:47, Niklas Cassel wrote:

>> Niklas Cassel (8):
>>    hw/ide/ahci: remove stray backslash
>>    hw/ide/core: set ERR_STAT in unsupported command completion
>>    hw/ide/ahci: write D2H FIS when processing NCQ command
>>    hw/ide/ahci: simplify and document PxCI handling
>>    hw/ide/ahci: PxSACT and PxCI is cleared when PxCMD.ST is cleared
>>    hw/ide/ahci: PxCI should not get cleared when ERR_STAT is set
>>    hw/ide/ahci: fix ahci_write_fis_sdb()
>>    hw/ide/ahci: fix broken SError handling
>>
>>   hw/ide/ahci.c             | 112 +++++++++++++++++++++++++++-----------
>>   hw/ide/core.c             |   2 +-
>>   tests/qtest/libqos/ahci.c | 106 +++++++++++++++++++++++++++---------
>>   tests/qtest/libqos/ahci.h |   8 +--
>>   4 files changed, 164 insertions(+), 64 deletions(-)
>>
>> -- 
>> 2.40.1
>>
>>
> 
> Hello Philippe,
> 
> Considering that you picked up my patch,
> "hw/ide/ahci: remove stray backslash" (patch 1/8 in this series),
> and since John seems to have gone silent for 40+ days,
> could you please consider taking this series through your misc tree?

(First patch was a cleanup)

Niklas, I don't feel confident enough :/

John, Paolo, Kevin, do you Ack?

Regards,

Phil.

Re: [PATCH v3 0/8] misc AHCI cleanups

[John Snow]

On Tue, Jul 25, 2023 at 9:04 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Hi Niklas, John, Paolo, Kevin,
>
> On 19/7/23 12:47, Niklas Cassel wrote:
>
> >> Niklas Cassel (8):
> >>    hw/ide/ahci: remove stray backslash
> >>    hw/ide/core: set ERR_STAT in unsupported command completion
> >>    hw/ide/ahci: write D2H FIS when processing NCQ command
> >>    hw/ide/ahci: simplify and document PxCI handling
> >>    hw/ide/ahci: PxSACT and PxCI is cleared when PxCMD.ST is cleared
> >>    hw/ide/ahci: PxCI should not get cleared when ERR_STAT is set
> >>    hw/ide/ahci: fix ahci_write_fis_sdb()
> >>    hw/ide/ahci: fix broken SError handling
> >>
> >>   hw/ide/ahci.c             | 112 +++++++++++++++++++++++++++-----------
> >>   hw/ide/core.c             |   2 +-
> >>   tests/qtest/libqos/ahci.c | 106 +++++++++++++++++++++++++++---------
> >>   tests/qtest/libqos/ahci.h |   8 +--
> >>   4 files changed, 164 insertions(+), 64 deletions(-)
> >>
> >> --
> >> 2.40.1
> >>
> >>
> >
> > Hello Philippe,
> >
> > Considering that you picked up my patch,
> > "hw/ide/ahci: remove stray backslash" (patch 1/8 in this series),
> > and since John seems to have gone silent for 40+ days,
> > could you please consider taking this series through your misc tree?
>

40 days, ouch. I kept thinking it had been a week. Don't trust me with time.

> (First patch was a cleanup)
>
> Niklas, I don't feel confident enough :/
>
> John, Paolo, Kevin, do you Ack?
>
> Regards,
>
> Phil.

I'm staging it, but it's for next release. We'll get it in early and
it gives us a chance to fix anything that's amiss before the next RC
window.

Re: [PATCH v3 0/8] misc AHCI cleanups

[Niklas Cassel]

On Tue, Jul 25, 2023 at 03:00:56PM -0400, John Snow wrote:
> On Tue, Jul 25, 2023 at 9:04 AM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
> >
> > Hi Niklas, John, Paolo, Kevin,
> >
> > On 19/7/23 12:47, Niklas Cassel wrote:
> >
> > >> Niklas Cassel (8):
> > >>    hw/ide/ahci: remove stray backslash
> > >>    hw/ide/core: set ERR_STAT in unsupported command completion
> > >>    hw/ide/ahci: write D2H FIS when processing NCQ command
> > >>    hw/ide/ahci: simplify and document PxCI handling
> > >>    hw/ide/ahci: PxSACT and PxCI is cleared when PxCMD.ST is cleared
> > >>    hw/ide/ahci: PxCI should not get cleared when ERR_STAT is set
> > >>    hw/ide/ahci: fix ahci_write_fis_sdb()
> > >>    hw/ide/ahci: fix broken SError handling
> > >>
> > >>   hw/ide/ahci.c             | 112 +++++++++++++++++++++++++++-----------
> > >>   hw/ide/core.c             |   2 +-
> > >>   tests/qtest/libqos/ahci.c | 106 +++++++++++++++++++++++++++---------
> > >>   tests/qtest/libqos/ahci.h |   8 +--
> > >>   4 files changed, 164 insertions(+), 64 deletions(-)
> > >>
> > >> --
> > >> 2.40.1
> > >>
> > >>
> > >
> > > Hello Philippe,
> > >
> > > Considering that you picked up my patch,
> > > "hw/ide/ahci: remove stray backslash" (patch 1/8 in this series),
> > > and since John seems to have gone silent for 40+ days,
> > > could you please consider taking this series through your misc tree?
> >
> 
> 40 days, ouch. I kept thinking it had been a week. Don't trust me with time.

Well, it is summer vacation times, so the days tend to fly by quite
quickly :)


> 
> > (First patch was a cleanup)
> >
> > Niklas, I don't feel confident enough :/
> >
> > John, Paolo, Kevin, do you Ack?
> >
> > Regards,
> >
> > Phil.
> 
> I'm staging it, but it's for next release. We'll get it in early and
> it gives us a chance to fix anything that's amiss before the next RC
> window.
> 

Thank you John!

I don't expect any (further) issues, but I will of course be available
in case something unexpectedly pops up.


Kind regards,
Niklas

Re: [PATCH v3 0/8] misc AHCI cleanups

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 2753 bytes --]

On Mon, Aug 7, 2023, 6:33 AM Niklas Cassel <Niklas.Cassel@wdc.com> wrote:

> On Tue, Jul 25, 2023 at 03:00:56PM -0400, John Snow wrote:
> > On Tue, Jul 25, 2023 at 9:04 AM Philippe Mathieu-Daudé
> > <philmd@linaro.org> wrote:
> > >
> > > Hi Niklas, John, Paolo, Kevin,
> > >
> > > On 19/7/23 12:47, Niklas Cassel wrote:
> > >
> > > >> Niklas Cassel (8):
> > > >>    hw/ide/ahci: remove stray backslash
> > > >>    hw/ide/core: set ERR_STAT in unsupported command completion
> > > >>    hw/ide/ahci: write D2H FIS when processing NCQ command
> > > >>    hw/ide/ahci: simplify and document PxCI handling
> > > >>    hw/ide/ahci: PxSACT and PxCI is cleared when PxCMD.ST is cleared
> > > >>    hw/ide/ahci: PxCI should not get cleared when ERR_STAT is set
> > > >>    hw/ide/ahci: fix ahci_write_fis_sdb()
> > > >>    hw/ide/ahci: fix broken SError handling
> > > >>
> > > >>   hw/ide/ahci.c             | 112
> +++++++++++++++++++++++++++-----------
> > > >>   hw/ide/core.c             |   2 +-
> > > >>   tests/qtest/libqos/ahci.c | 106
> +++++++++++++++++++++++++++---------
> > > >>   tests/qtest/libqos/ahci.h |   8 +--
> > > >>   4 files changed, 164 insertions(+), 64 deletions(-)
> > > >>
> > > >> --
> > > >> 2.40.1
> > > >>
> > > >>
> > > >
> > > > Hello Philippe,
> > > >
> > > > Considering that you picked up my patch,
> > > > "hw/ide/ahci: remove stray backslash" (patch 1/8 in this series),
> > > > and since John seems to have gone silent for 40+ days,
> > > > could you please consider taking this series through your misc tree?
> > >
> >
> > 40 days, ouch. I kept thinking it had been a week. Don't trust me with
> time.
>
> Well, it is summer vacation times, so the days tend to fly by quite
> quickly :)
>
>
> >
> > > (First patch was a cleanup)
> > >
> > > Niklas, I don't feel confident enough :/
> > >
> > > John, Paolo, Kevin, do you Ack?
> > >
> > > Regards,
> > >
> > > Phil.
> >
> > I'm staging it, but it's for next release. We'll get it in early and
> > it gives us a chance to fix anything that's amiss before the next RC
> > window.
> >
>
> Thank you John!
>
> I don't expect any (further) issues, but I will of course be available
> in case something unexpectedly pops up.
>
>
> Kind regards,
> Niklas


Apologies again for the delay. I tested it lightly and it seems fine to me
(and in general I trust your commits as they've got meticulous references
to the spec, so it'll be easy to fix if something goes wrong)

It's my fault we'll miss this release window, but putting it in early next
window gives us a lot of time for people to notice accidental regressions.

Thanks for the patches and I hope to see more from you soon ;)

--js

[-- Attachment #2: Type: text/html, Size: 3953 bytes --]

Re: [PATCH v3 0/8] misc AHCI cleanups

[Philippe Mathieu-Daudé]

Hi John,

On 7/8/23 19:37, John Snow wrote:

> Apologies again for the delay. I tested it lightly and it seems fine to 
> me (and in general I trust your commits as they've got meticulous 
> references to the spec, so it'll be easy to fix if something goes wrong)
> 
> It's my fault we'll miss this release window, but putting it in early 
> next window gives us a lot of time for people to notice accidental 
> regressions.
> 
> Thanks for the patches and I hope to see more from you soon ;)

I've been doing some testing (x86/mips), but I don't think I have the
same coverage as your maintainer test suite. Still, if you are busy,
I can merge this series now that we are at the beginning of the
development cycle.

Regards,

Phil.