From: Niklas Cassel <niklas.cas...@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.cas...@wdc.com>
Message-id: 20230609140844.202795-5-...@flawful.org
Signed-off-by: John Snow <js...@redhat.com>
(cherry picked from commit e2a5d9b3d9c3d311618160603cc9bc04fbd98796)
Signed-off-by: Michael Tokarev <m...@tls.msk.ru>
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 1b937e5e23..bdafa91bf0 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.39.2
