ide_sector_read() as well as its callers neglect to call ide_set_retry()
before starting I/O. If the I/O fails, this means that the retry
information is stale. In particular, ide_handle_rw_error() has an
assertion that s->bus->retry_unit == s->unit, which can fail if either
there was no previous request or it came from another device on the bus.
If the assertion weren't there, a wrong request would be retried after
resuming the VM.

Fix this by adding a ide_set_retry() call to ide_sector_read().

This affects only reads because ide_transfer_start() does call
ide_set_retry(). For writes, the data transfer comes first and the I/O
is only started when the data has been read into s->io_buffer, so by
that time, ide_set_retry() has been called. For reads, however, the I/O
comes first and only then the data is transferred to the guest, so the
call in ide_transfer_start() is too late.

Buglink: https://redhat.atlassian.net/browse/RHEL-153537
Reported-by: Tingting Mao <timao@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index b45abf067b2..52a991fc482 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -799,6 +799,7 @@ static void ide_sector_read(IDEState *s)
     s->error = 0; /* not needed by IDE spec, but needed by Windows */
     sector_num = ide_get_sector(s);
     n = s->nsector;
+    ide_set_retry(s);
 
     if (n == 0) {
         ide_transfer_stop(s);
-- 
2.53.0

On 26.03.2026 19:51, Kevin Wolf wrote:
> ide_sector_read() as well as its callers neglect to call ide_set_retry()
> before starting I/O. If the I/O fails, this means that the retry
> information is stale. In particular, ide_handle_rw_error() has an
> assertion that s->bus->retry_unit == s->unit, which can fail if either
> there was no previous request or it came from another device on the bus.
> If the assertion weren't there, a wrong request would be retried after
> resuming the VM.
> 
> Fix this by adding a ide_set_retry() call to ide_sector_read().
> 
> This affects only reads because ide_transfer_start() does call
> ide_set_retry(). For writes, the data transfer comes first and the I/O
> is only started when the data has been read into s->io_buffer, so by
> that time, ide_set_retry() has been called. For reads, however, the I/O
> comes first and only then the data is transferred to the guest, so the
> call in ide_transfer_start() is too late.
> 
> Buglink: https://redhat.atlassian.net/browse/RHEL-153537
> Reported-by: Tingting Mao <timao@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   hw/ide/core.c | 1 +
>   1 file changed, 1 insertion(+)

Shouldn't this one be picked up for stable series too?

I'm picking it, please let me know if I should not.

Thanks,

/mjt

On 26.03.26 17:51, Kevin Wolf wrote:
> ide_sector_read() as well as its callers neglect to call ide_set_retry()
> before starting I/O. If the I/O fails, this means that the retry
> information is stale. In particular, ide_handle_rw_error() has an
> assertion that s->bus->retry_unit == s->unit, which can fail if either
> there was no previous request or it came from another device on the bus.
> If the assertion weren't there, a wrong request would be retried after
> resuming the VM.
>
> Fix this by adding a ide_set_retry() call to ide_sector_read().
>
> This affects only reads because ide_transfer_start() does call
> ide_set_retry(). For writes, the data transfer comes first and the I/O
> is only started when the data has been read into s->io_buffer, so by
> that time, ide_set_retry() has been called. For reads, however, the I/O
> comes first and only then the data is transferred to the guest, so the
> call in ide_transfer_start() is too late.
>
> Buglink: https://redhat.atlassian.net/browse/RHEL-153537
> Reported-by: Tingting Mao <timao@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   hw/ide/core.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>