[PATCH 00/11] hw/ide: pending IDE fixes

[PATCH 00/11] hw/ide: pending IDE fixes

[Denis V. Lunev]

Pending IDE fixes for various issues reported on GitLab.

Based on:
[PATCH v2 00/17] hw/ide: fix the logical CHS translation a guest selects
https://lore.kernel.org/qemu-devel/20260820100844.411717-1-den@openvz.org/

Signed-off-by: Denis V. Lunev <den@openvz.org>
Cc: John Snow <jsnow@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

Denis V. Lunev (11):
  hw/ide: reject an out-of-range PIO transfer window on load
  tests/qtest/ide-test: cover the migrated PIO transfer window
  hw/ide/ahci: refuse a PIO transfer with no command header
  hw/ide/ahci: clear cur_cmd when the command list is unmapped
  tests/qtest/ahci: regression test for a PIO write vs. engine stop
  hw/ide/ahci: treat a failed PRDT walk as a PIO transfer failure
  hw/ide/ahci: reject a command header with an invalid FIS length
  hw/ide/ahci: drain the ports on teardown
  tests/qtest/ahci: regression test for a request outliving an unplug
  hw/ide: report ATAPI UDMA5 with a matching standard and cable
  tests/qtest/ide-test: cover the UDMA5 identify words

 hw/ide/ahci.c            | 136 ++++++++++++++++++++++++++---------
 hw/ide/core.c            |  23 +++++-
 hw/ide/trace-events      |   3 +
 include/hw/ide/ide-dma.h |   3 +-
 tests/qtest/ahci-test.c  | 147 ++++++++++++++++++++++++++++++++++++++
 tests/qtest/ide-test.c   | 148 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 423 insertions(+), 37 deletions(-)


base-commit: 7f1fc6c9b9d2859577fb8837e05ca6eaa5a8a0ba
-- 
2.53.0

[PATCH 01/11] hw/ide: reject an out-of-range PIO transfer window on load

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

ide_drive_pio_post_load() validates end_transfer_fn_idx but takes
cur_io_buffer_offset and cur_io_buffer_len straight from the migration
stream, so data_ptr and data_end can be placed anywhere within +-2GB of
the 131076-byte io_buffer allocation. Both fields are signed 32-bit.

The subsection loader consumes every subsection present in the stream
without consulting needed(), so a crafted stream can inject
ide_drive/pio_state for a drive that was never in a DRQ state. Once
data_end is out of bounds, ide_data_writew() only compares the guest's
pointer against that same bogus data_end, and the resumed guest turns a
repeated outw to the data port into a controlled 16-bit heap write.
end_transfer_fn_idx picks the direction, so the read side of the same
code path leaks host heap instead.

Validate the window against io_buffer_total_len and fail the load. The
subtraction form avoids overflowing the addition.

Reported-by: XlabAI Team of Tencent Xuanwu Lab <xlabai@tencent.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4179
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3738
Cc: John Snow <jsnow@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 hw/ide/core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 8190549ee8..0dca2b5c52 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2898,6 +2898,12 @@ static int ide_drive_pio_post_load(void *opaque, int version_id)
     if (s->end_transfer_fn_idx >= ARRAY_SIZE(transfer_end_table)) {
         return -EINVAL;
     }
+    if (s->cur_io_buffer_offset < 0 || s->cur_io_buffer_len < 0 ||
+        s->cur_io_buffer_offset > s->io_buffer_total_len ||
+        s->cur_io_buffer_len >
+            s->io_buffer_total_len - s->cur_io_buffer_offset) {
+        return -EINVAL;
+    }
     s->end_transfer_func = transfer_end_table[s->end_transfer_fn_idx];
     s->data_ptr = s->io_buffer + s->cur_io_buffer_offset;
     s->data_end = s->data_ptr + s->cur_io_buffer_len;
-- 
2.53.0

[PATCH 02/11] tests/qtest/ide-test: cover the migrated PIO transfer window

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

/ide/migration/pio_state_rejected leaves a drive in DRQ so the source
streams ide_drive/pio_state, rewrites cur_io_buffer_offset to the end of
the io_buffer, and expects the destination to refuse the load.

It asserts the window the source wrote before overwriting it, so a wrong
guess at the stream layout fails the test rather than passing it for the
wrong reason.

Cc: John Snow <jsnow@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 tests/qtest/ide-test.c | 79 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index f14a0851f0..a3109da908 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -1571,6 +1571,83 @@ static void test_migrate_chs_rejected(void)
     unlink(path);
 }
 
+/* A PIO transfer window reaching past the io_buffer has to be refused */
+static void test_migrate_pio_state_rejected(void)
+{
+    const char *name = "ide_drive/pio_state";
+    /* IDE_DMA_BUF_SECTORS * 512 + 4, the length of the streamed io_buffer */
+    const gsize io_buffer_len = 256 * 512 + 4;
+    /* cur_io_buffer_offset and cur_io_buffer_len, big endian */
+    const uint8_t in_bounds[8] = { 0, 0, 0, 0, 0, 0, 0x02, 0 };
+    const uint8_t past_the_end[8] = { 0, 0x02, 0, 0x04, 0, 0, 0x10, 0 };
+    QTestState *src, *dst;
+    QPCIDevice *dev;
+    QPCIBar bmdma_bar, ide_bar;
+    g_autofree char *path = NULL;
+    g_autofree char *uri = NULL;
+    g_autofree char *dst_args = NULL;
+    g_autofree char *stream = NULL;
+    char *window;
+    gsize len;
+    int fd;
+
+    fd = g_file_open_tmp("qtest-ide-stream.XXXXXX", &path, NULL);
+    g_assert(fd >= 0);
+    close(fd);
+    uri = g_strdup_printf("file:%s", path);
+
+    src = ide_test_start(
+        "-blockdev driver=file,node-name=hda,filename=%s "
+        "-device ide-hd,drive=hda,bus=ide.0,unit=0 ",
+        tmp_path[0]);
+    dev = get_pci_device(src, &bmdma_bar, &ide_bar);
+
+    /* WRITE SECTOR(S) waits in DRQ for the data, so pio_state is streamed */
+    qpci_io_writeb(dev, ide_bar, reg_nsectors, 1);
+    qpci_io_writeb(dev, ide_bar, reg_lba_low, 0);
+    qpci_io_writeb(dev, ide_bar, reg_lba_middle, 0);
+    qpci_io_writeb(dev, ide_bar, reg_lba_high, 0);
+    qpci_io_writeb(dev, ide_bar, reg_device, LBA);
+    qpci_io_writeb(dev, ide_bar, reg_command, CMD_WRITE);
+    assert_bit_set(qpci_io_readb(dev, ide_bar, reg_status), DRQ);
+
+    qtest_qmp_assert_success(src, "{ 'execute': 'migrate',"
+                             " 'arguments': { 'uri': %s } }", uri);
+    qtest_qmp_eventwait(src, "STOP");
+    ide_migration_wait(src, "completed");
+    free_pci_device(dev);
+    ide_test_quit(src);
+
+    /*
+     * Behind the name come the version and req_nb_sectors as big endian 32
+     * bit, then the io_buffer array, then the transfer window this rewrites.
+     * Asserting the window the source streamed keeps that arithmetic honest.
+     */
+    g_assert(g_file_get_contents(path, &stream, &len, NULL));
+    window = ide_stream_find(stream, len, name);
+    g_assert(window);
+    window += strlen(name) + 8 + io_buffer_len;
+    g_assert_cmpint(window - stream + sizeof(past_the_end), <=, len);
+    g_assert_cmpint(memcmp(window, in_bounds, sizeof(in_bounds)), ==, 0);
+    memcpy(window, past_the_end, sizeof(past_the_end));
+    g_assert(g_file_set_contents(path, stream, len, NULL));
+
+    dst_args = g_strdup_printf(
+        "-machine pc "
+        "-blockdev driver=file,node-name=hda,filename=%s "
+        "-device ide-hd,drive=hda,bus=ide.0,unit=0 -incoming defer",
+        tmp_path[0]);
+    dst = qtest_init(dst_args);
+
+    qtest_qmp_assert_success(dst, "{ 'execute': 'migrate-incoming',"
+                             " 'arguments': { 'uri': %s,"
+                             " 'exit-on-error': false } }", uri);
+    ide_migration_wait(dst, "failed");
+
+    qtest_quit(dst);
+    unlink(path);
+}
+
 /* Words 54 to 58 follow the translation even when the data was cached first */
 static void test_specify_identify(void)
 {
@@ -1764,6 +1841,8 @@ int main(int argc, char **argv)
                    test_migrate_chs_translation);
     qtest_add_func("/ide/migration/chs_snapshot", test_migrate_chs_snapshot);
     qtest_add_func("/ide/migration/chs_rejected", test_migrate_chs_rejected);
+    qtest_add_func("/ide/migration/pio_state_rejected",
+                   test_migrate_pio_state_rejected);
 
     qtest_add_func("/ide/identify", test_identify);
 
-- 
2.53.0

[PATCH 03/11] hw/ide/ahci: refuse a PIO transfer with no command header

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

ahci_map_clb_address() already clears cur_cmd, so every consumer of it
has to cope with there being no current command. ahci_pio_transfer(),
ahci_commit_buf() and ahci_populate_sglist() all dereference it
unconditionally instead.

Give the three of them a NULL check. Declaring the data transferred
anyway is not enough: ide_transfer_start() goes on to call the end
transfer function, and for a multi-sector write that is
ide_sector_write(), which commits an io_buffer the guest never
refilled. Clearing PxCMD.ST during a WRITE SECTOR(S) of two sectors
therefore writes the first sector's contents over the second, at a
sector the guest chose.

Let pio_transfer report that nothing was transferred and halt there, so
no callback acts on a buffer that was never filled. Only the AHCI HBA
implements the callback, so the signature change is local to it.

Cc: John Snow <jsnow@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 hw/ide/ahci.c            | 47 +++++++++++++++++++++++++++++++---------
 hw/ide/core.c            | 11 +++++++++-
 hw/ide/trace-events      |  2 ++
 include/hw/ide/ide-dma.h |  3 ++-
 4 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 49f3047e6f..995b40efd5 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -906,12 +906,12 @@ static int prdt_tbl_entry_size(const AHCI_SG *tbl)
 static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
                                 AHCICmdHdr *cmd, int64_t limit, uint64_t offset)
 {
-    uint16_t opts = le16_to_cpu(cmd->opts);
-    uint16_t prdtl = le16_to_cpu(cmd->prdtl);
-    uint64_t cfis_addr = le64_to_cpu(cmd->tbl_addr);
-    uint64_t prdt_addr = cfis_addr + 0x80;
-    dma_addr_t prdt_len = (prdtl * sizeof(AHCI_SG));
-    dma_addr_t real_prdt_len = prdt_len;
+    uint16_t opts;
+    uint16_t prdtl;
+    uint64_t cfis_addr;
+    uint64_t prdt_addr;
+    dma_addr_t prdt_len;
+    dma_addr_t real_prdt_len;
     uint8_t *prdt;
     int i;
     int r = 0;
@@ -923,6 +923,18 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
 
     trace_ahci_populate_sglist(ad->hba, ad->port_no);
 
+    if (!cmd) {
+        trace_ahci_populate_sglist_no_cmd(ad->hba, ad->port_no);
+        return -1;
+    }
+
+    opts = le16_to_cpu(cmd->opts);
+    prdtl = le16_to_cpu(cmd->prdtl);
+    cfis_addr = le64_to_cpu(cmd->tbl_addr);
+    prdt_addr = cfis_addr + 0x80;
+    prdt_len = (prdtl * sizeof(AHCI_SG));
+    real_prdt_len = prdt_len;
+
     if (!prdtl) {
         trace_ahci_populate_sglist_no_prdtl(ad->hba, ad->port_no, opts);
         return -1;
@@ -1371,18 +1383,27 @@ out:
 }
 
 /* Transfer PIO data between RAM and device */
-static void ahci_pio_transfer(const IDEDMA *dma)
+static bool ahci_pio_transfer(const IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
     IDEState *s = &ad->port.ifs[0];
     uint32_t size = (uint32_t)(s->data_end - s->data_ptr);
     /* write == ram -> device */
-    uint16_t opts = le16_to_cpu(ad->cur_cmd->opts);
-    int is_write = opts & AHCI_CMD_WRITE;
-    int is_atapi = opts & AHCI_CMD_ATAPI;
+    uint16_t opts;
+    int is_write;
+    int is_atapi;
     int has_sglist = 0;
     bool pio_fis_i;
 
+    if (ad->cur_cmd == NULL) {
+        trace_ahci_pio_transfer_no_cmd(ad->hba, ad->port_no);
+        return false;
+    }
+
+    opts = le16_to_cpu(ad->cur_cmd->opts);
+    is_write = opts & AHCI_CMD_WRITE;
+    is_atapi = opts & AHCI_CMD_ATAPI;
+
     /* The PIO Setup FIS is received prior to transfer, but the interrupt
      * is only triggered after data is received.
      *
@@ -1430,6 +1451,8 @@ out:
     if (pio_fis_i) {
         ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_PSS);
     }
+
+    return true;
 }
 
 static void ahci_start_dma(const IDEDMA *dma, IDEState *s,
@@ -1492,6 +1515,10 @@ static void ahci_commit_buf(const IDEDMA *dma, uint32_t tx_bytes)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
 
+    if (ad->cur_cmd == NULL) {
+        return;
+    }
+
     tx_bytes += le32_to_cpu(ad->cur_cmd->status);
     ad->cur_cmd->status = cpu_to_le32(tx_bytes);
 }
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 0dca2b5c52..06c18dbf09 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -80,6 +80,7 @@ static const char *IDE_DMA_CMD_str(enum ide_dma_cmd enval)
 }
 
 static void ide_dummy_transfer_stop(IDEState *s);
+static void ide_transfer_halt(IDEState *s);
 
 const MemoryRegionPortio ide_portio_list[] = {
     { 0, 8, 1, .read = ide_ioport_read, .write = ide_ioport_write },
@@ -568,7 +569,15 @@ bool ide_transfer_start_norecurse(IDEState *s, uint8_t *buf, int size,
         s->end_transfer_func = end_transfer_func;
         return false;
     }
-    s->bus->dma->ops->pio_transfer(s->bus->dma);
+    if (!s->bus->dma->ops->pio_transfer(s->bus->dma)) {
+        /*
+         * No data reached the buffer, so the caller must not act on it. A
+         * write would otherwise commit whatever the previous phase left
+         * there to the next sector.
+         */
+        ide_transfer_halt(s);
+        return false;
+    }
     return true;
 }
 
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index 57042cafdd..f1472f5852 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -85,6 +85,7 @@ ahci_reset_port(void *s, int port) "ahci(%p)[%d]: reset port"
 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]"
+ahci_populate_sglist_no_cmd(void *s, int port) "ahci(%p)[%d]: no command header"
 ahci_populate_sglist_no_prdtl(void *s, int port, uint16_t opts) "ahci(%p)[%d]: no sg list given by guest: 0x%04x"
 ahci_populate_sglist_no_map(void *s, int port) "ahci(%p)[%d]: DMA mapping failed"
 ahci_populate_sglist_short_map(void *s, int port) "ahci(%p)[%d]: mapped less than expected"
@@ -109,6 +110,7 @@ handle_cmd_badfis(void *s, int port) "ahci(%p)[%d]: guest provided an invalid cm
 handle_cmd_badmap(void *s, int port, uint64_t len) "ahci(%p)[%d]: dma_memory_map failed, 0x%02"PRIx64" != 0x80"
 handle_cmd_unhandled_fis(void *s, int port, uint8_t b0, uint8_t b1, uint8_t b2) "ahci(%p)[%d]: unhandled FIS type. cmd_fis: 0x%02x-%02x-%02x"
 ahci_pio_transfer(void *s, int port, const char *rw, uint32_t size, const char *tgt, const char *sgl) "ahci(%p)[%d]: %sing %d bytes on %s w/%s sglist"
+ahci_pio_transfer_no_cmd(void *s, int port) "ahci(%p)[%d]: PIO transfer without a command header"
 ahci_start_dma(void *s, int port) "ahci(%p)[%d]: start dma"
 ahci_dma_prepare_buf(void *s, int port, int32_t io_buffer_size, int32_t limit) "ahci(%p)[%d]: prepare buf limit=%"PRId32" prepared=%"PRId32
 ahci_dma_prepare_buf_fail(void *s, int port) "ahci(%p)[%d]: sglist population failed"
diff --git a/include/hw/ide/ide-dma.h b/include/hw/ide/ide-dma.h
index 296010a4e0..34154b7cbc 100644
--- a/include/hw/ide/ide-dma.h
+++ b/include/hw/ide/ide-dma.h
@@ -10,6 +10,7 @@ typedef struct IDEDMA IDEDMA;
 
 typedef void DMAStartFunc(const IDEDMA *, IDEState *, BlockCompletionFunc *);
 typedef void DMAVoidFunc(const IDEDMA *);
+typedef bool DMABoolFunc(const IDEDMA *);
 typedef int DMAIntFunc(const IDEDMA *, bool);
 typedef int32_t DMAInt32Func(const IDEDMA *, int32_t len);
 typedef void DMAu32Func(const IDEDMA *, uint32_t);
@@ -17,7 +18,7 @@ typedef void DMAStopFunc(const IDEDMA *, bool);
 
 struct IDEDMAOps {
     DMAStartFunc *start_dma;
-    DMAVoidFunc *pio_transfer;
+    DMABoolFunc *pio_transfer;
     DMAInt32Func *prepare_buf;
     DMAu32Func *commit_buf;
     DMAIntFunc *rw_buf;
-- 
2.53.0

[PATCH 04/11] hw/ide/ahci: clear cur_cmd when the command list is unmapped

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

ahci_unmap_clb_address() drops the CLB mapping but leaves cur_cmd
pointing into it. The cancel added by commit d9f78431d8 covers the
buffered reads, and ide_cancel_dma_sync() drains bus->dma->aiocb, but
neither reaches IDEState::pio_aiocb: a PIO write started before the
guest cleared PxCMD.ST completes afterwards and runs its second DRQ
phase against the stale header.

That is harmless while the CLB is direct RAM, because unmapping it
changes nothing. It is a use-after-free once PxCLB points at an MMIO
region, where address_space_map() hands out a bounce buffer that
dma_memory_unmap() then frees.

Clear cur_cmd after the cancel, so nothing reachable from a later
completion still refers to the freed mapping.

Reported-by: Katherine Leaver <katherine.j.leaver@gmail.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3719
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4043
Cc: John Snow <jsnow@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 hw/ide/ahci.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 995b40efd5..4c138b0c51 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -743,6 +743,12 @@ static void ahci_unmap_clb_address(AHCIDevice *ad)
     /* Cancel in-flight reads that would complete against a cleared cur_cmd. */
     ide_cancel_dma_sync(ide_bus_active_if(&ad->port));
 
+    /*
+     * Whatever survives the cancel must not be left pointing into the
+     * mapping this function is about to drop.
+     */
+    ad->cur_cmd = NULL;
+
     if (ad->lst == NULL) {
         trace_ahci_unmap_clb_address_null(ad->hba, ad->port_no);
         return;
-- 
2.53.0

[PATCH 05/11] tests/qtest/ahci: regression test for a PIO write vs. engine stop

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

Add /ahci/io/pio/engine_stop: hold the backend write of a two-sector
PIO write with a blkdebug breakpoint, clear PxCMD.ST so the command
list is unmapped underneath it, then let the write complete. The
second DRQ phase runs from that completion and reaches
ahci_pio_transfer() with no command header.

Cc: John Snow <jsnow@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 tests/qtest/ahci-test.c | 72 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index 30d7005626..84d4e6b0a5 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -1793,6 +1793,76 @@ static void test_atapi_engine_restart_dma(void)
     test_atapi_engine_restart_in_flight(true);
 }
 
+/*
+ * Regression test: a PIO write outlives the command list it was issued from.
+ * ide_cancel_dma_sync() does not reach s->pio_aiocb, so the second DRQ phase
+ * runs from the write completion after PxCLB has been unmapped and must not
+ * touch the command header any more.
+ */
+static void test_write_engine_stop_in_flight(void)
+{
+    AHCIQState *ahci;
+    AHCICommand *cmd;
+    unsigned char *tx;
+    unsigned char *rx;
+    uint64_t ptr;
+    uint8_t port;
+    size_t bufsize = AHCI_SECTOR_SIZE * 2;
+    size_t i;
+
+    ahci = ahci_boot_and_enable("-drive file=blkdebug::%s,if=none,id=drive0,"
+                                "format=%s,cache=writeback "
+                                "-M q35 "
+                                "-device ide-hd,drive=drive0 ",
+                                tmp_path, imgfmt);
+    port = ahci_port_select(ahci);
+    ahci_port_clear(ahci, port);
+
+    tx = g_malloc(bufsize);
+    generate_pattern(tx, bufsize, AHCI_SECTOR_SIZE);
+    ptr = ahci_alloc(ahci, bufsize);
+    g_assert(ptr);
+    qtest_memwrite(ahci->parent->qts, ptr, tx, bufsize);
+
+    /* Zero the second sector, which the abandoned command must not reach. */
+    rx = g_malloc0(AHCI_SECTOR_SIZE);
+    ahci_io(ahci, port, CMD_WRITE_DMA, rx, AHCI_SECTOR_SIZE, 1);
+
+    /* Suspend the backend write so the first sector stays in flight. */
+    g_free(qtest_hmp(ahci->parent->qts,
+                     "qemu-io drive0 \"break write_aio wr\""));
+
+    cmd = ahci_command_create(CMD_WRITE_PIO);
+    ahci_command_adjust(cmd, 0, ptr, bufsize, 0);
+    ahci_command_commit(ahci, cmd, port);
+    ahci_command_issue_async(ahci, cmd);
+
+    /* Drop the command list while the write is still outstanding. */
+    ahci_px_clr(ahci, port, AHCI_PX_CMD, AHCI_PX_CMD_ST);
+
+    g_free(qtest_hmp(ahci->parent->qts, "qemu-io drive0 \"resume wr\""));
+
+    /* Round-trip through the device to confirm qemu is still alive. */
+    ahci_px_rreg(ahci, port, AHCI_PX_TFD);
+
+    /*
+     * The second DRQ phase never fetched its data, so the sector it would
+     * have carried has to be untouched rather than hold a copy of the first.
+     */
+    ahci_px_set(ahci, port, AHCI_PX_CMD, AHCI_PX_CMD_ST);
+    memset(rx, 0xff, AHCI_SECTOR_SIZE);
+    ahci_io(ahci, port, CMD_READ_DMA, rx, AHCI_SECTOR_SIZE, 1);
+    for (i = 0; i < AHCI_SECTOR_SIZE; i++) {
+        g_assert_cmpint(rx[i], ==, 0);
+    }
+
+    ahci_command_free(cmd);
+    ahci_free(ahci, ptr);
+    g_free(rx);
+    g_free(tx);
+    ahci_shutdown(ahci);
+}
+
 /*
  * Regression test: a multi-sector ATAPI read fetches its later sectors from
  * inside the first read's completion; a concurrent drain (as a guest reset
@@ -2281,6 +2351,8 @@ int main(int argc, char **argv)
                    test_atapi_engine_restart_pio);
     qtest_add_func("/ahci/cdrom/engine_restart/dma",
                    test_atapi_engine_restart_dma);
+    qtest_add_func("/ahci/io/pio/engine_stop",
+                   test_write_engine_stop_in_flight);
     qtest_add_func("/ahci/cdrom/drain/pio", test_atapi_drain_pio);
     qtest_add_func("/ahci/cdrom/drain/dma", test_atapi_drain_dma);
 
-- 
2.53.0

[PATCH 06/11] hw/ide/ahci: treat a failed PRDT walk as a PIO transfer failure

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

ahci_dma_prepare_buf() returns -1 when it cannot build a scatter-gather
list, the PRDTL of zero case among them. ahci_pio_transfer() tests the
result for truth, so a failure sets has_sglist and the transfer goes
ahead against whatever s->sg holds. AHCI 1.3.1 is explicit about the
zero case: "If this field is '0', then no data transfer shall occur
with the command."

Test for a positive byte count instead. A successful walk that yields
nothing to transfer is already handled by the size check below.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4043
Cc: John Snow <jsnow@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 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 4c138b0c51..436a0eaab6 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1428,7 +1428,7 @@ static bool ahci_pio_transfer(const IDEDMA *dma)
         goto out;
     }
 
-    if (ahci_dma_prepare_buf(dma, size)) {
+    if (ahci_dma_prepare_buf(dma, size) > 0) {
         has_sglist = 1;
     }
 
-- 
2.53.0

[PATCH 07/11] hw/ide/ahci: reject a command header with an invalid FIS length

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

AHCI 1.3.1 defines CFL in the command header as the "Length of the
Command FIS", where "A length of '0' or '1' is illegal" and "The
maximum value allowed is 10h, or 16 DW". handle_cmd() never looks at
it, so an all-zero command header is executable: its zero tbl_addr maps
a command table at guest physical address 0, and a guest that has put a
valid Register H2D FIS there gets it run.

That is the reachability a guest gains by pointing PxCLB at an MMIO
region, where the CLB is a zero-filled bounce buffer rather than
anything the guest wrote.

Reject a header whose CFL falls outside the legal range. Nothing else
consults it; the command FIS is always mapped at its full 128 bytes.

The slot is dropped without reporting anything, as the unmappable
command table beside it already is. No PxIS bit describes a malformed
command header: HBFS is for a host bus error, "such as a bad software
pointer", which is why the short mapping below raises it and this does
not.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4043
Cc: John Snow <jsnow@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 hw/ide/ahci.c       | 9 +++++++++
 hw/ide/trace-events | 1 +
 2 files changed, 10 insertions(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 436a0eaab6..2b2ef873e0 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1334,6 +1334,7 @@ static void handle_cmd(AHCIState *s, int port, uint8_t slot)
     AHCICmdHdr *cmd;
     uint8_t *cmd_fis;
     dma_addr_t cmd_len;
+    uint8_t cfl;
 
     if (s->dev[port].port.ifs[0].status & (BUSY_STAT|DRQ_STAT)) {
         /* Engine currently busy, try again later */
@@ -1346,6 +1347,14 @@ static void handle_cmd(AHCIState *s, int port, uint8_t slot)
         return;
     }
     cmd = get_cmd_header(s, port, slot);
+
+    /* AHCI 1.3.1: a CFL below 2 dwords or above 16 is illegal */
+    cfl = le16_to_cpu(cmd->opts) & AHCI_CMD_HDR_CMD_FIS_LEN;
+    if (cfl < 2 || cfl > 16) {
+        trace_handle_cmd_badcfl(s, port, le16_to_cpu(cmd->opts));
+        return;
+    }
+
     /* remember current slot handle for later */
     s->dev[port].cur_cmd = cmd;
 
diff --git a/hw/ide/trace-events b/hw/ide/trace-events
index f1472f5852..3ab5e7bd1d 100644
--- a/hw/ide/trace-events
+++ b/hw/ide/trace-events
@@ -106,6 +106,7 @@ handle_reg_h2d_fis_res(void *s, int port, char b0, char b1, char b2) "ahci(%p)[%
 handle_cmd_busy(void *s, int port) "ahci(%p)[%d]: engine busy"
 handle_cmd_nolist(void *s, int port) "ahci(%p)[%d]: handle_cmd called without s->dev[port].lst"
 handle_cmd_badport(void *s, int port) "ahci(%p)[%d]: guest accessed unused port"
+handle_cmd_badcfl(void *s, int port, uint16_t opts) "ahci(%p)[%d]: guest provided an invalid cmd FIS length: 0x%04x"
 handle_cmd_badfis(void *s, int port) "ahci(%p)[%d]: guest provided an invalid cmd FIS"
 handle_cmd_badmap(void *s, int port, uint64_t len) "ahci(%p)[%d]: dma_memory_map failed, 0x%02"PRIx64" != 0x80"
 handle_cmd_unhandled_fis(void *s, int port, uint8_t b0, uint8_t b1, uint8_t b2) "ahci(%p)[%d]: unhandled FIS type. cmd_fis: 0x%02x-%02x-%02x"
-- 
2.53.0

[PATCH 08/11] hw/ide/ahci: drain the ports on teardown

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

ahci_uninit() frees s->dev without touching the requests still in flight.
The only blk_aio_cancel() for them lives in ahci_reset_port(), which the
unplug path does not run, and the ide-hd child's own drain is deferred
through call_rcu so it happens after the free. A guest that powers the
root port slot off through SLTCTL, or writes the ACPI ejection register,
while a read is outstanding therefore leaves the completion to run
against freed memory.

A plain device_del is not affected: the pciehp attention-button flow
resets the secondary bus first, which cancels through the reset path.
Surprise removal is what skips it.

Cancelling the NCQ requests alone is not enough. IDEDMA and IDEBus are
embedded in AHCIDevice, so a plain DMA read reaches the freed array
through dma_blk_cb() and a PIO read through ide_buffered_readv_cb(),
neither of which the NCQ bookkeeping covers. ide_exit() drains nothing
and frees io_buffer, which an outstanding request may still target.

Move the NCQ cancel loop into a helper, run it from ahci_uninit() too,
and drain each port before ide_exit() so no class of request can outlive
the allocation. Delete check_bh there as well; qemu_bh_new_guarded() in
check_cmd() has no counterpart on this path either.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4069
Cc: John Snow <jsnow@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 hw/ide/ahci.c | 72 +++++++++++++++++++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 23 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 2b2ef873e0..6b04762c4a 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -619,12 +619,37 @@ static void ahci_set_signature(AHCIDevice *ad, uint32_t sig)
                              s->lcyl, s->hcyl, sig);
 }
 
+static void ahci_cancel_ncq_requests(AHCIDevice *ad)
+{
+    int i;
+
+    for (i = 0; i < AHCI_MAX_CMDS; i++) {
+        NCQTransferState *ncq_tfs = &ad->ncq_tfs[i];
+        ncq_tfs->halt = false;
+        if (!ncq_tfs->used) {
+            continue;
+        }
+
+        if (ncq_tfs->aiocb) {
+            blk_aio_cancel(ncq_tfs->aiocb);
+            ncq_tfs->aiocb = NULL;
+        }
+
+        /* Maybe we just finished the request thanks to blk_aio_cancel() */
+        if (!ncq_tfs->used) {
+            continue;
+        }
+
+        qemu_sglist_destroy(&ncq_tfs->sglist);
+        ncq_tfs->used = 0;
+    }
+}
+
 static void ahci_reset_port(AHCIState *s, int port, IDEResetKind kind)
 {
     AHCIDevice *d = &s->dev[port];
     AHCIPortRegs *pr = &d->port_regs;
     IDEState *ide_state = &d->port.ifs[0];
-    int i;
 
     trace_ahci_reset_port(s, port);
 
@@ -645,27 +670,7 @@ static void ahci_reset_port(AHCIState *s, int port, IDEResetKind kind)
         return;
     }
 
-    /* reset ncq queue */
-    for (i = 0; i < AHCI_MAX_CMDS; i++) {
-        NCQTransferState *ncq_tfs = &s->dev[port].ncq_tfs[i];
-        ncq_tfs->halt = false;
-        if (!ncq_tfs->used) {
-            continue;
-        }
-
-        if (ncq_tfs->aiocb) {
-            blk_aio_cancel(ncq_tfs->aiocb);
-            ncq_tfs->aiocb = NULL;
-        }
-
-        /* Maybe we just finished the request thanks to blk_aio_cancel() */
-        if (!ncq_tfs->used) {
-            continue;
-        }
-
-        qemu_sglist_destroy(&ncq_tfs->sglist);
-        ncq_tfs->used = 0;
-    }
+    ahci_cancel_ncq_requests(d);
 
     s->dev[port].port_state = STATE_RUN;
     if (ide_state->drive_kind == IDE_CD) {
@@ -1659,8 +1664,29 @@ void ahci_uninit(AHCIState *s)
     for (i = 0; i < s->ports; i++) {
         AHCIDevice *ad = &s->dev[i];
 
+        /*
+         * Unplug does not go through a reset, so this is the only chance to
+         * detach the requests and the bottom half that would otherwise walk
+         * s->dev after it is freed below.
+         */
+        ahci_cancel_ncq_requests(ad);
+        if (ad->check_bh) {
+            qemu_bh_delete(ad->check_bh);
+            ad->check_bh = NULL;
+        }
+
         for (j = 0; j < 2; j++) {
-            ide_exit(&ad->port.ifs[j]);
+            IDEState *ide_state = &ad->port.ifs[j];
+
+            /*
+             * Everything the port still owns points into the allocation this
+             * function frees, io_buffer included, so nothing may be left in
+             * flight once ide_exit() has run.
+             */
+            if (ide_state->blk) {
+                blk_drain(ide_state->blk);
+            }
+            ide_exit(ide_state);
         }
         object_unparent(OBJECT(&ad->port));
     }
-- 
2.53.0

[PATCH 09/11] tests/qtest/ahci: regression test for a request outliving an unplug

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

Add /ahci/io/{ncq,dma,pio}/unplug: arm a read against a null-co backend
whose latency keeps it in flight, then eject the controller through the
ACPI ejection register. Each of the three reaches the freed AHCIDevice
array by a different route, so covering one command class would leave
the other two untested.

That register is what a guest writes to finish a PCI unplug, and unlike
the pciehp attention button it reaches ahci_uninit() with no secondary
bus reset, so nothing cancels the request on the way. It also dictates
the machine: q35 has no ACPI hotplug on pcie.0, so the eject has no
effect there.

The latency is what holds the request; a blkdebug breakpoint cannot
stand in for it, because cancelling a suspended request waits for it and
the unplug would never return.

Unfixed, all three fail reliably under AddressSanitizer. On a plain build
the use-after-free only faults when the freed page has been returned, so
expect the odd pass there.

Cc: John Snow <jsnow@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 tests/qtest/ahci-test.c | 75 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
index 84d4e6b0a5..b143862ce7 100644
--- a/tests/qtest/ahci-test.c
+++ b/tests/qtest/ahci-test.c
@@ -1793,6 +1793,78 @@ static void test_atapi_engine_restart_dma(void)
     test_atapi_engine_restart_in_flight(true);
 }
 
+/*
+ * Regression test: an unplug runs no device reset, so it is the last chance to
+ * detach an outstanding request. Its completion would otherwise walk the
+ * AHCIDevice array that ahci_uninit() has freed, which each of the NCQ, DMA
+ * and PIO completions reaches by a different route.
+ *
+ * The ACPI ejection register is what a guest writes to finish a PCI unplug.
+ * -M pc is what puts it in reach: q35 has no ACPI hotplug on pcie.0, so the
+ * unplug never happens there. Unlike the pciehp attention button this reaches
+ * the unplug with no secondary bus reset, which is the ordering that leaves a
+ * request outstanding.
+ */
+static void test_unplug_in_flight(uint8_t ide_cmd)
+{
+    AHCIQState *ahci;
+    AHCICommand *cmd;
+    uint64_t ptr;
+    uint8_t port;
+    QTestState *qts;
+
+    /*
+     * The latency keeps the backend read in flight across the unplug. A
+     * blkdebug breakpoint cannot stand in for it: cancelling a suspended
+     * request waits for it, so the unplug would never return.
+     */
+    ahci = ahci_boot_and_enable(
+        "-M pc "
+        "-blockdev driver=null-co,node-name=drive0,read-zeroes=on,"
+        "latency-ns=100000000 "
+        "-device ich9-ahci,addr=1f.2,id=ahci0 "
+        "-device ide-hd,drive=drive0,bus=ahci0.0 ");
+    qts = ahci->parent->qts;
+    port = ahci_port_select(ahci);
+    ahci_port_clear(ahci, port);
+
+    ptr = ahci_alloc(ahci, AHCI_SECTOR_SIZE);
+    g_assert(ptr);
+
+    cmd = ahci_command_create(ide_cmd);
+    ahci_command_adjust(cmd, 0, ptr, AHCI_SECTOR_SIZE, 0);
+    ahci_command_commit(ahci, cmd, port);
+    ahci_command_issue_async(ahci, cmd);
+
+    /* Eject slot 0x1f of the root bus, which frees the AHCIDevice array. */
+    qtest_outl(qts, 0xae10, 0);
+    qtest_outl(qts, 0xae08, 1u << 0x1f);
+    qtest_qmp_eventwait(qts, "DEVICE_DELETED");
+
+    /* Four times the backend latency, so the completion has surely run. */
+    g_usleep(400 * 1000);
+    qtest_qmp_assert_success(qts, "{ 'execute': 'query-status' }");
+
+    ahci_command_free(cmd);
+    ahci_free(ahci, ptr);
+    ahci_shutdown(ahci);
+}
+
+static void test_unplug_ncq(void)
+{
+    test_unplug_in_flight(READ_FPDMA_QUEUED);
+}
+
+static void test_unplug_dma(void)
+{
+    test_unplug_in_flight(CMD_READ_DMA);
+}
+
+static void test_unplug_pio(void)
+{
+    test_unplug_in_flight(CMD_READ_PIO);
+}
+
 /*
  * Regression test: a PIO write outlives the command list it was issued from.
  * ide_cancel_dma_sync() does not reach s->pio_aiocb, so the second DRQ phase
@@ -2351,6 +2423,9 @@ int main(int argc, char **argv)
                    test_atapi_engine_restart_pio);
     qtest_add_func("/ahci/cdrom/engine_restart/dma",
                    test_atapi_engine_restart_dma);
+    qtest_add_func("/ahci/io/ncq/unplug", test_unplug_ncq);
+    qtest_add_func("/ahci/io/dma/unplug", test_unplug_dma);
+    qtest_add_func("/ahci/io/pio/unplug", test_unplug_pio);
     qtest_add_func("/ahci/io/pio/engine_stop",
                    test_write_engine_stop_in_flight);
     qtest_add_func("/ahci/cdrom/drain/pio", test_atapi_drain_pio);
-- 
2.53.0

[PATCH 10/11] hw/ide: report ATAPI UDMA5 with a matching standard and cable

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

IDENTIFY PACKET DEVICE claims UDMA mode 5 in word 88 while word 80
reports support only up to ATA/ATAPI-4. UDMA5 first appears in
ATA/ATAPI-6; ATA/ATAPI-5 stops at mode 4. Bits 3:1 of word 80 are
obsolete in IDENTIFY PACKET DEVICE data as well, so the old 001eh
claimed three standards that mean nothing for a packet device. Report
0070h, ATA/ATAPI-4 through ATA/ATAPI-6.

Word 93 was left unset, so nothing reported the 80-conductor cable that
UDMA5 needs. Fill it in, but only for a parallel attachment: ACS-3
7.13.6.41 gives word 93 of IDENTIFY PACKET DEVICE data the meaning of
word 93 of IDENTIFY DEVICE data, where "For SATA devices, word 93 shall
be set to the value 0000h". A cleared ncq_queues is how both identify
paths already tell a parallel attachment from an AHCI one.

The device 0 reset result is 0fh rather than the 01h ide_identify()
reports: bit 3 says diagnostics passed, which they did, and bits 2:1
say the device number came from some other method, the only one of the
four encodings that is not a jumper, CSEL or reserved.

Raising word 80 has a second effect. Linux decides a device is SATA in
ata_id_is_sata(), which wants word 93 clear and word 80 at ATA/ATAPI-5
or later. An AHCI CD-ROM satisfied neither condition before and was
taken for a parallel device; now it satisfies both.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4038
Cc: John Snow <jsnow@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 hw/ide/core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 06c18dbf09..ef573798d9 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -292,7 +292,7 @@ static void ide_atapi_identify(IDEState *s)
         put_le16(p + 76, (1 << 8));
     }
 
-    put_le16(p + 80, 0x1e); /* support up to ATA/ATAPI-4 */
+    put_le16(p + 80, 0x70); /* support up to ATA/ATAPI-6 */
     if (s->wwn) {
         put_le16(p + 84, (1 << 8)); /* supports WWN for words 108-111 */
         put_le16(p + 87, (1 << 8)); /* WWN enabled */
@@ -300,6 +300,10 @@ static void ide_atapi_identify(IDEState *s)
 
 #ifdef USE_DMA_CDROM
     put_le16(p + 88, 0x3f | (1 << 13)); /* udma5 set and supported */
+    if (!s->ncq_queues) {
+        /* word 93 is parallel ATA only, a SATA device reports zero */
+        put_le16(p + 93, 0x600f);
+    }
 #endif
 
     if (s->wwn) {
-- 
2.53.0

[PATCH 11/11] tests/qtest/ide-test: cover the UDMA5 identify words

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

/ide/identify/udma and /ide/identify/udma_atapi check that a device
advertising UDMA mode 5 claims a standard that defines it and reports the
hardware reset result, on the disk and on the CD-ROM. The ATAPI case also
checks that the words obsolete in IDENTIFY PACKET DEVICE data stay
clear, and that the reset result reports a passed diagnostic, which
only the packet path does so far.

Cc: John Snow <jsnow@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 tests/qtest/ide-test.c | 69 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/tests/qtest/ide-test.c b/tests/qtest/ide-test.c
index a3109da908..92e3d9b343 100644
--- a/tests/qtest/ide-test.c
+++ b/tests/qtest/ide-test.c
@@ -104,6 +104,7 @@ enum {
     CMD_FLUSH_CACHE = 0xe7,
     CMD_IDENTIFY    = 0xec,
     CMD_PACKET      = 0xa0,
+    CMD_IDENTIFY_PACKET = 0xa1,
     CMD_READ_NATIVE = 0xf8,  /* READ NATIVE MAX ADDRESS */
 
     CMDF_ABORT      = 0x100,
@@ -1571,6 +1572,72 @@ static void test_migrate_chs_rejected(void)
     unlink(path);
 }
 
+/*
+ * A device advertising UDMA5 has to claim a standard that defines it, and a
+ * parallel attachment has to report the cable word (ACS-3 7.12.7.47).
+ */
+static void test_identify_udma(bool packet)
+{
+    QTestState *qts;
+    QPCIDevice *dev;
+    QPCIBar bmdma_bar, ide_bar;
+    uint16_t buf[256];
+    int i;
+
+    if (packet) {
+        qts = ide_test_start("-device ide-cd,bus=ide.0");
+    } else {
+        qts = ide_test_start(
+            "-blockdev driver=file,node-name=hda,filename=%s "
+            "-device ide-hd,drive=hda,bus=ide.0,unit=0 ",
+            tmp_path[0]);
+    }
+    dev = get_pci_device(qts, &bmdma_bar, &ide_bar);
+
+    qpci_io_writeb(dev, ide_bar, reg_device, 0);
+    qpci_io_writeb(dev, ide_bar, reg_command,
+                   packet ? CMD_IDENTIFY_PACKET : CMD_IDENTIFY);
+    for (i = 0; i < 256; i++) {
+        buf[i] = qpci_io_readw(dev, ide_bar, reg_data);
+    }
+
+    /* UDMA5 supported and selected */
+    assert_bit_set(buf[88], 1 << 5);
+    assert_bit_set(buf[88], 1 << 13);
+
+    /* UDMA5 arrived in ATA/ATAPI-6, so word 80 has to reach bit 6 */
+    assert_bit_set(buf[80], 1 << 6);
+    if (packet) {
+        /* Bits 3:1 are obsolete in IDENTIFY PACKET DEVICE data */
+        assert_bit_clear(buf[80], 0x0e);
+    }
+
+    /* Word 93: reserved bit clear, fixed bit set, 80-conductor cable */
+    assert_bit_clear(buf[93], 1 << 15);
+    assert_bit_set(buf[93], 1 << 14);
+    assert_bit_set(buf[93], 1 << 13);
+    assert_bit_set(buf[93], 1 << 0);
+    /* Device 0 clears the device 1 result */
+    assert_bit_clear(buf[93], 0x1f00);
+    if (packet) {
+        /* the disk path has yet to gain this */
+        assert_bit_set(buf[93], 1 << 3);
+    }
+
+    free_pci_device(dev);
+    ide_test_quit(qts);
+}
+
+static void test_identify_udma_ata(void)
+{
+    test_identify_udma(false);
+}
+
+static void test_identify_udma_atapi(void)
+{
+    test_identify_udma(true);
+}
+
 /* A PIO transfer window reaching past the io_buffer has to be refused */
 static void test_migrate_pio_state_rejected(void)
 {
@@ -1843,6 +1910,8 @@ int main(int argc, char **argv)
     qtest_add_func("/ide/migration/chs_rejected", test_migrate_chs_rejected);
     qtest_add_func("/ide/migration/pio_state_rejected",
                    test_migrate_pio_state_rejected);
+    qtest_add_func("/ide/identify/udma", test_identify_udma_ata);
+    qtest_add_func("/ide/identify/udma_atapi", test_identify_udma_atapi);
 
     qtest_add_func("/ide/identify", test_identify);
 
-- 
2.53.0