[PATCH] hw/sd/sdhci: complete non-interrupt ADMA descriptor chains in one pass

[PATCH] hw/sd/sdhci: complete non-interrupt ADMA descriptor chains in one pass

[Wadim Mueller]

sdhci_do_adma() processes at most SDHC_ADMA_DESCS_PER_DELAY descriptors
per invocation and then reschedules itself SDHC_TRANSFER_DELAY ns later
on QEMU_CLOCK_VIRTUAL.  For a large bulk transfer this spreads the DMA
across hundreds of virtual-clock round-trips, advancing the guest's
virtual time between descriptor batches.

A guest that bounds the transfer with its own data/status timeout
(counted in guest time) can then see that timeout expire mid-transfer.
Concretely, U-Boot on a TI AM64x reading a ~28 MiB image with a single
CMD18 multi-block ADMA2 read intermittently aborts with "Timeout for
status update" and falls back to (failing) distro boot.  The transfer
itself is correct; only the artificial per-batch pacing triggers the
guest timeout.

Run a descriptor chain that carries no SDHC_ADMA_ATTR_INT attribute to
completion within a single call.  Chains that do request a DMA-boundary
interrupt still deliver it and reschedule exactly as before, so a guest
relying on that pacing is unaffected.

Now that the per-call bound is gone, cap the number of descriptors
processed per call so that a malformed or circular descriptor list (a
self-referencing link, or a persistently faulting non-END/non-INT
descriptor) cannot spin the calling thread forever; on overflow, break
to the existing reschedule path so the main loop stays responsive.

Signed-off-by: Wadim Mueller <wafgo01@gmail.com>
---
 hw/sd/sdhci-internal.h |  9 +++++++++
 hw/sd/sdhci.c          | 23 +++++++++++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 9f768c418e..bb586a9666 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -278,6 +278,15 @@ FIELD(SDHC_MAXCURR, V18_VDD2,         32, 8); /* since v4.20 */
 #define SDHC_INSERTION_DELAY            (NANOSECONDS_PER_SECOND)
 #define SDHC_TRANSFER_DELAY             100
 #define SDHC_ADMA_DESCS_PER_DELAY       5
+/*
+ * Upper bound on ADMA2 descriptors handled in a single sdhci_do_adma()
+ * call, as a safety valve against a malformed or circular descriptor
+ * list. A well-formed transfer terminates far below this via END or
+ * blkcnt == 0 (even a 4 GiB transfer built from 64 KiB TRAN descriptors
+ * is only ~64K descriptors); the bound merely guarantees the loop makes
+ * a decision instead of spinning forever.
+ */
+#define SDHC_ADMA_MAX_DESCRIPTORS       (1 << 20)
 #define SDHC_CMD_RESPONSE               (3 << 0)
 
 enum {
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 89b595ce4a..e8e8928b4f 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -779,7 +779,6 @@ static void sdhci_do_adma(SDHCIState *s)
     const MemTxAttrs attrs = { .memory = true };
     ADMADescr dscr = {};
     MemTxResult res = MEMTX_ERROR;
-    int i;
 
     if (s->trnmod & SDHC_TRNS_BLK_CNT_EN && !s->blkcnt) {
         /* Stop Multiple Transfer */
@@ -787,7 +786,27 @@ static void sdhci_do_adma(SDHCIState *s)
         return;
     }
 
-    for (i = 0; i < SDHC_ADMA_DESCS_PER_DELAY; ++i) {
+    /*
+     * Process the descriptor chain to completion (END or blkcnt == 0),
+     * yielding to the guest only for a descriptor carrying the INT
+     * attribute (a DMA-boundary interrupt, handled at the end of the loop).
+     *
+     * Historically at most SDHC_ADMA_DESCS_PER_DELAY descriptors were
+     * handled per call before rescheduling SDHC_TRANSFER_DELAY ns later on
+     * QEMU_CLOCK_VIRTUAL. That pacing is only needed so a guest can observe
+     * the intermediate DMA-interrupt state; a bulk transfer that requests
+     * no interrupt does not need slicing, and throttling it across many
+     * virtual-clock round-trips can make it race a guest-side transfer
+     * timeout. Run such chains to completion in one call instead.
+     *
+     * SDHC_ADMA_MAX_DESCRIPTORS bounds the loop so a malformed or circular
+     * chain cannot spin here forever; on overflow, break to the reschedule
+     * path so the main loop stays responsive.
+     */
+    for (unsigned int adma_descs = 0; ; adma_descs++) {
+        if (adma_descs >= SDHC_ADMA_MAX_DESCRIPTORS) {
+            break;
+        }
         s->admaerr &= ~SDHC_ADMAERR_LENGTH_MISMATCH;
 
         get_adma_description(s, &dscr);
-- 
2.43.0

Re: [PATCH] hw/sd/sdhci: complete non-interrupt ADMA descriptor chains in one pass

[Bin Meng]

Hi Wadim,

On Tue, Aug 4, 2026 at 4:15 PM Wadim Mueller <wafgo01@gmail.com> wrote:
>
> sdhci_do_adma() processes at most SDHC_ADMA_DESCS_PER_DELAY descriptors
> per invocation and then reschedules itself SDHC_TRANSFER_DELAY ns later
> on QEMU_CLOCK_VIRTUAL.  For a large bulk transfer this spreads the DMA
> across hundreds of virtual-clock round-trips, advancing the guest's
> virtual time between descriptor batches.
>
> A guest that bounds the transfer with its own data/status timeout
> (counted in guest time) can then see that timeout expire mid-transfer.
> Concretely, U-Boot on a TI AM64x reading a ~28 MiB image with a single
> CMD18 multi-block ADMA2 read intermittently aborts with "Timeout for
> status update" and falls back to (failing) distro boot.  The transfer
> itself is correct; only the artificial per-batch pacing triggers the
> guest timeout.
>
> Run a descriptor chain that carries no SDHC_ADMA_ATTR_INT attribute to
> completion within a single call.  Chains that do request a DMA-boundary
> interrupt still deliver it and reschedule exactly as before, so a guest
> relying on that pacing is unaffected.
>
> Now that the per-call bound is gone, cap the number of descriptors
> processed per call so that a malformed or circular descriptor list (a
> self-referencing link, or a persistently faulting non-END/non-INT
> descriptor) cannot spin the calling thread forever; on overflow, break
> to the existing reschedule path so the main loop stays responsive.
>
> Signed-off-by: Wadim Mueller <wafgo01@gmail.com>
> ---
>  hw/sd/sdhci-internal.h |  9 +++++++++
>  hw/sd/sdhci.c          | 23 +++++++++++++++++++++--
>  2 files changed, 30 insertions(+), 2 deletions(-)
>

Would you please try this series to see if this fixes the issue you
saw on TI AM64x?
https://patchwork.ozlabs.org/project/qemu-devel/list/?series=515264

Regards,
Bin

Re: [PATCH] hw/sd/sdhci: complete non-interrupt ADMA descriptor chains in one pass

[Wadim Mueller]

Hi Bin,

Thanks a lot for the pointer -- I gave it a go. Let me share what I saw,
with a couple of caveats up front, since I'm not sure I tested it fairly.

Caveat 1: series 515264 doesn't apply on the tree I'm on -- it seems to
build on earlier SD/SDHCI rework I don't have yet (e.g.
SDHCIState.sdma_boundary_paused, sdhci_sdma_transfer_active, the Host
Control 2 migration), so patches 07-11 and 13 didn't build against my
base. So I couldn't test the series as-is.

Caveat 2: to still get a data point I hand-applied only what I understood
to be the core idea of patch 12 -- driving ADMA purely from the transfer
timer and no longer resuming it from MMIO reads/writes -- on top of my
AM64x tree. That's my approximation of your change, so I may well be
misrepresenting it; please correct me if so.

With that approximation, over 15 hands-off boots each of the same
unmodified image (a ~28 MiB CMD18 ADMA2-64 read), counting the U-Boot
"Timeout for status update" failures:

  baseline (sliced sdhci_do_adma, ADMA resumed from MMIO):    7/15 time out
  patch-12 approximation (ADMA timer-only, MMIO-decoupled):  10/15 time out
  my patch (complete non-INT chains in one pass):             0/15

I don't want to read too much into 7 vs 10 (small sample, overlapping),
but what I take from it is that the timer-only approximation still timed
out in most boots here, i.e. it doesn't seem to cover this particular
failure, whereas completing the chain in one pass avoided it in every run.

My guess as to why -- and this is only a guess -- is that the two things
might be different facets of the same symptom:

 - Your patch, as I understand it, targets a status *read* itself
   executing a pending ADMA batch and thus returning late.

 - What bites us on AM64x looks more like the *batched* transfer racing a
   guest-side timeout: sdhci_do_adma still does only
   SDHC_ADMA_DESCS_PER_DELAY descriptors per call and reschedules
   SDHC_TRANSFER_DELAY ns later on QEMU_CLOCK_VIRTUAL, so a big transfer
   is spread over many virtual-clock round-trips and U-Boot's timeout (in
   guest time) can expire mid-transfer. If that's right, keeping the
   per-batch reschedule would leave that race in place, which might be why
   the approximation didn't help here.

So my hunch is the two changes may be complementary rather than
alternatives -- but I could easily be wrong about your patch given I only
approximated it. Would it help if I rebased onto a base with the
prerequisite SD rework and tested the actual series, or if I shared the
AM64x reproducer? Whatever's most useful to you.

Thanks again,
Wadim

Re: [PATCH] hw/sd/sdhci: complete non-interrupt ADMA descriptor chains in one pass

[Bin Meng]

Hi Wadim,

On Wed, Aug 5, 2026 at 4:05 PM Wadim Mueller <wafgo01@gmail.com> wrote:
>
> Hi Bin,
>
> Thanks a lot for the pointer -- I gave it a go. Let me share what I saw,
> with a couple of caveats up front, since I'm not sure I tested it fairly.
>
> Caveat 1: series 515264 doesn't apply on the tree I'm on -- it seems to
> build on earlier SD/SDHCI rework I don't have yet (e.g.
> SDHCIState.sdma_boundary_paused, sdhci_sdma_transfer_active, the Host
> Control 2 migration), so patches 07-11 and 13 didn't build against my
> base. So I couldn't test the series as-is.

Sorry about that, I should have mentioned that the following SDMA fix
should be applied first:
https://patchwork.ozlabs.org/project/qemu-devel/list/?series=513930

> Caveat 2: to still get a data point I hand-applied only what I understood
> to be the core idea of patch 12 -- driving ADMA purely from the transfer
> timer and no longer resuming it from MMIO reads/writes -- on top of my
> AM64x tree. That's my approximation of your change, so I may well be
> misrepresenting it; please correct me if so.
>
> With that approximation, over 15 hands-off boots each of the same
> unmodified image (a ~28 MiB CMD18 ADMA2-64 read), counting the U-Boot
> "Timeout for status update" failures:
>
>   baseline (sliced sdhci_do_adma, ADMA resumed from MMIO):    7/15 time out
>   patch-12 approximation (ADMA timer-only, MMIO-decoupled):  10/15 time out
>   my patch (complete non-INT chains in one pass):             0/15
>
> I don't want to read too much into 7 vs 10 (small sample, overlapping),
> but what I take from it is that the timer-only approximation still timed
> out in most boots here, i.e. it doesn't seem to cover this particular
> failure, whereas completing the chain in one pass avoided it in every run.
>
> My guess as to why -- and this is only a guess -- is that the two things
> might be different facets of the same symptom:
>
>  - Your patch, as I understand it, targets a status *read* itself
>    executing a pending ADMA batch and thus returning late.
>
>  - What bites us on AM64x looks more like the *batched* transfer racing a
>    guest-side timeout: sdhci_do_adma still does only
>    SDHC_ADMA_DESCS_PER_DELAY descriptors per call and reschedules
>    SDHC_TRANSFER_DELAY ns later on QEMU_CLOCK_VIRTUAL, so a big transfer
>    is spread over many virtual-clock round-trips and U-Boot's timeout (in
>    guest time) can expire mid-transfer. If that's right, keeping the
>    per-batch reschedule would leave that race in place, which might be why
>    the approximation didn't help here.
>
> So my hunch is the two changes may be complementary rather than
> alternatives -- but I could easily be wrong about your patch given I only
> approximated it. Would it help if I rebased onto a base with the
> prerequisite SD rework and tested the actual series, or if I shared the
> AM64x reproducer? Whatever's most useful to you.

Yeah, please share the AM64x reproducer. I could investigate a bit more.

Regards,
Bin

Re: [PATCH] hw/sd/sdhci: complete non-interrupt ADMA descriptor chains in one pass

[Wadim Mueller]

Hi Bin,

Thanks for the 513930 pointer -- that was exactly the missing piece, and
it changes my earlier result, so let me correct the record first.

With "hw/sd: sdhci: Fix SDMA boundary bug" (513930) applied first, series
515264 builds on my tree (I applied 513930 patch 1 + 515264 patches
07-12; I dropped 13/26 "keep high-capacity memory blocks at 512" -- it
didn't apply on my older hw/sd/sd.c and is orthogonal to the ADMA path).
Re-running the same AM64x reproducer, 15 hands-off boots each:

  baseline (before either change):              7/15 time out
  your series (513930 + 515264 07-12):          0/15
  my patch (complete non-INT chains in a pass): 0/15

So your series *does* fix the AM64x case -- my earlier "10/15" number was
from my crude hand-approximation of patch 12 alone on the old base, which
(as I feared) simply wasn't representative. Apologies for the noise; the
real series resolves it here, same as my patch.

As promised, the reproducer -- I kept it free of any proprietary bits:

 - The machine is public in my QEMU fork:
     https://github.com/wafgo/qemu  branch cmblu/corenode
   (an "am64-virt" TI AM64x board). Build qemu-system-aarch64 from it;
   to observe the original failure, revert my fix commit cad8b499
   ("fix(sdhci): complete non-int adma chains in one pass").

 - A small image-free qtest on that branch,
   tests/qtest/am64-adma-pacing-test.c, drives an ADMA2 read on am64-virt
   and counts how many SDHC_TRANSFER_DELAY virtual-clock steps the chain
   takes to complete: 0 with either fix in place, >0 (the sliced,
   timer-paced transfer) without. It reproduces the *mechanism*
   deterministically, no guest image needed.

Since your series fixes this, my standalone patch is effectively
superseded -- please feel free to drop it in favour of 515264, I don't
think a separate fix is needed.

The one thing possibly worth keeping is the pacing regression test. As
written it targets my fork's am64-virt, so it isn't directly
mainline-able; if a test that catches this ADMA virtual-clock pacing
would be useful upstream, I'd be glad to port it to an in-tree SDHCI
machine -- e.g. the Icicle Kit where you hit the same "Timeout for
status update", or the xilinx-zynq SDHCI you just added a test for in
513930. Would that be worthwhile, and which board would you prefer?

Thanks again for the help,
Wadim