On 20/7/26 09:58, Denis V. Lunev wrote:
On 6/19/26 13:21, Denis V. Lunev wrote:
An in-flight ATAPI buffered CD read (ide_buffered_readv(), used by both
the PIO and DMA reply paths in hw/ide/atapi.c) can collide with a
concurrent port operation, and it breaks in two distinct ways. Both are
guest-triggerable and reproduce on current master.
Neither was found by inspection: both are real issues reported against
the qemu-kvm 10.0 downstream -- the crash from production, and the
deadlock from the downstream live-migration validation suite -- on
CD-ROMs backed by high-latency network storage, where a backend read
outlives the guest's SATA command timeout.
1. Command engine restart -> NULL-deref crash (patches 1-2)
A guest can stop and restart a port's command engine
(PxCMD.ST 1 -> 0 -> 1) while a read is in flight. The restart re-maps
the command list and clears AHCIDevice.cur_cmd, but nothing tears
down the outstanding read; its completion then dereferences the
cleared cur_cmd in ahci_pio_transfer() / ahci_dma_rw_buf(). This is
not the COMRESET path -- an engine restart does not run ide_reset(),
so the drive's transfer state is preserved and the read still runs.
Patch 1 cancels the outstanding I/O when the command list is
unmapped, reusing ide_cancel_dma_sync() exactly as ATAPI DEVICE
RESET already does from the same MMIO context. Patch 2 is a qtest
covering the PIO and DMA reply paths.
2. Concurrent drain -> main-loop deadlock (patch 8, repro in patch 9)
A multi-sector PIO read whose byte-count limit spans more than one
sector must fetch the later sectors of a DRQ burst from inside the
completion of the first, asynchronous read. cd_read_sector_sync()
did that with a synchronous blk_pread(), which queues behind any
drain in progress. When a guest reset reaches
virtio_blk_stop_ioeventfd() -> bdrv_drain_all_begin() while such a
read is in flight, the nested read is queued while the outer
completion still holds blk->in_flight, so the drain never finishes:
the main loop wedges in the drain with the BQL held and every
QMP/monitor operation blocks behind it.
Patch 8 reads the whole elementary transfer in a single asynchronous
request, so no read is ever issued mid-burst; cd_read_sector_sync()
is removed and the DMA path is unchanged. Patch 9 reproduces the
hang via x-blockdev-set-iothread, which takes the same
bdrv_drain_all_begin() path a guest reset does.
Patches 3-7 are the test groundwork the deadlock fix needs: they fold
the IDE CD-ROM read tests into one parametrized helper and add the
previously missing coverage -- multi-sector DMA, and raw 2352-byte
READ CD on both IDE and AHCI -- so the rewritten ATAPI read path
(including its in-place 2352 unpack) is exercised on both consumers.
The test patches pass with or without patch 8, keeping the series
bisectable.
Patches 1-2 stand alone as the engine-restart crash fix; they are
included here because the deadlock work shares and extends the same
ATAPI read path and its test scaffolding.
checkpatch is clean on all nine patches; the new and existing IDE and
AHCI qtests pass.
Denis V. Lunev (9):
hw/ide/ahci: cancel in-flight buffered reads on command engine restart
tests/qtest/ahci: test ATAPI read completing after engine restart
tests/qtest/ide-test: parametrize the ATAPI CD-ROM read test
tests/qtest/ide-test: add a multi-sector ATAPI DMA read test
tests/qtest/ide-test: cover raw (2352-byte) ATAPI CD reads
tests/qtest/libqos/ahci: support raw (2352-byte) READ CD
tests/qtest/ahci: cover raw (2352-byte) ATAPI CD reads
hw/ide/atapi: read the whole elementary transfer asynchronously
tests/qtest/ahci: regression test for ATAPI read vs. drain
hw/ide/ahci.c | 3 +
hw/ide/atapi.c | 180 +++++++++++++++-----------------
tests/qtest/ahci-test.c | 201 ++++++++++++++++++++++++++++++++++++
tests/qtest/ide-test.c | 211 ++++++++++++++++++++++++--------------
tests/qtest/libqos/ahci.c | 8 ++
tests/qtest/libqos/ahci.h | 2 +
6 files changed, 433 insertions(+), 172 deletions(-)
Hi!
if nobody is able to take a look, will it be fine if I'll
send pull request from my name for this?
qemu-block@ is Cc'ed so no objection on my side but first send a
MAINTAINERS patch stepping up for IDE, so you can include as first
patch of the pullreq?
