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

["Alex Bennée" <alex.bennee@linaro.org>]

Wadim Mueller <wafgo01@gmail.com> writes:

> sdhci_do_adma() returns to the main loop after every descriptor and relies
> on a timer re-entry to pick up the next one. For a descriptor chain whose
> entries do not request an interrupt this makes the transfer rate depend
> from the virtual clock rather than on the guest's programming, which
> significantly slows down large transfers and makes guest-visible timing
> depend on host timer behaviour.
>
> Keep processing the chain in the same invocation while no descriptor asks
> for an interrupt and the transfer has not finished, and only fall back to
> the deferred path when the guest actually requested a notification.
>
> A qtest reproducer is added later in this series.
>
> Signed-off-by: Wadim Mueller <wafgo01@gmail.com>
> ---
> This patch can be dropped once Bin Meng's SDHCI series
> https://patchwork.ozlabs.org/project/qemu-devel/list/?series=515264
> (which needs series=513930 applied first) is merged - it covers the
> same AM64x failure, see
> https://lore.kernel.org/qemu-devel/20260810124519.34501-1-wafgo01@gmail.com/
> It is included here only so that the series works on actual master.
>
>  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 4aeed120bf..e6ce12617e 100644
> --- a/hw/sd/sdhci-internal.h
> +++ b/hw/sd/sdhci-internal.h
> @@ -277,6 +277,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 e58a610397..9a5dd1d93f 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -780,7 +780,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 */
> @@ -788,7 +787,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.

Hmm SDHC_ADMA_MAX_DESCRIPTORS is (1 << 20) which seems an awful lot to
handle in dma session especially if this is a guest controlled thing.

> +     */
> +    for (unsigned int adma_descs = 0; ; adma_descs++) {
> +        if (adma_descs >= SDHC_ADMA_MAX_DESCRIPTORS) {
> +            break;
> +        }

Seems an odd formulation when you have the for loop construct already.
Although I see sdhci_do_adma already has multiple exit paths that skip
the final timer reset so it is hard to follow the control flow anyway.

I wonder if there is a better way to structure this logic so its easier
to see where irq's are set, transfers ended and timers scheduled for
continuation?

>          s->admaerr &= ~SDHC_ADMAERR_LENGTH_MISMATCH;
>  
>          get_adma_description(s, &dscr);

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro