Re: [PATCH 09/26] hw/sd: sdhci: Use version 4 system address for SDMA

Hi Bin,

On 23/7/26 17:18, Bin Meng wrote:
> SDHCI version 4 moves the SDMA system address from the legacy 32-bit
> register at offset 0x00 to the address pair at offsets 0x58 and 0x5c.
> At present QEMU always uses the legacy register, so guest SDHCI driver
> with version 4 mode enabled uses an incorrect DMA address.
> 
> Select the address register from Host Version 4 Enable and honor 64-bit
> Addressing when reading and advancing it. Keep the existing offset 0x00
> behavior when version 4 mode is disabled.
> 
> Signed-off-by: Bin Meng <[email protected]>
> ---
> 
>   hw/sd/sdhci-internal.h |  1 +
>   hw/sd/sdhci.c          | 63 ++++++++++++++++++++++++++++++++++--------
>   2 files changed, 53 insertions(+), 11 deletions(-)

I'm splitting this patch in 3 to ease reviewing:

-- >8 --
commit 39ca75d42c0926d1b48e377bc3d32d439689936c
Author: Bin Meng <[email protected]>
Date:   Sat Aug 15 16:35:46 2026 +0200

     hw/sd: sdhci: Factor sdhci_advance_sdma_address() helper out

     Signed-off-by: Bin Meng <[email protected]>
     Message-ID: <[email protected]>
     [PMD: Split of bigger patch]
     Signed-off-by: Philippe Mathieu-Daudé <[email protected]>

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 586e4680bee..ad370407c6e 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -600,2 +600,7 @@ static void sdhci_write_dataport(SDHCIState *s, 
uint32_t value, unsigned size)

+static void sdhci_advance_sdma_address(SDHCIState *s, uint32_t bytes)
+{
+        s->sdmasysad += bytes;
+}
+
  /* Multi block SDMA transfer */
@@ -643,3 +648,3 @@ static void 
sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
                               s->data_count - begin, 
MEMTXATTRS_UNSPECIFIED);
-            s->sdmasysad += s->data_count - begin;
+            sdhci_advance_sdma_address(s, s->data_count - begin);
              if (s->data_count == block_size) {
@@ -664,3 +669,3 @@ static void 
sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
                              s->data_count - begin, 
MEMTXATTRS_UNSPECIFIED);
-            s->sdmasysad += s->data_count - begin;
+            sdhci_advance_sdma_address(s, s->data_count - begin);
              if (s->data_count == block_size) {
---

-- >8 --
commit a9ba0290e8c299c6e32c623755219fcd95f3d237
Author: Bin Meng <[email protected]>
Date:   Sat Aug 15 16:36:00 2026 +0200

     hw/sd: sdhci: Factor sdhci_sdma_address() helper out

     Signed-off-by: Bin Meng <[email protected]>
     Message-ID: <[email protected]>
     [PMD: Split of bigger patch]
     Signed-off-by: Philippe Mathieu-Daudé <[email protected]>

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index ad370407c6e..93e1687dbe5 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -600,2 +600,7 @@ static void sdhci_write_dataport(SDHCIState *s, 
uint32_t value, unsigned size)

+static uint64_t sdhci_sdma_address(SDHCIState *s)
+{
+        return s->sdmasysad;
+}
+
  static void sdhci_advance_sdma_address(SDHCIState *s, uint32_t bytes)
@@ -612,3 +617,4 @@ static void 
sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
      uint32_t boundary_chk = 1 << (((s->blksize & ~BLOCK_SIZE_MASK) >> 
12) + 12);
-    uint32_t boundary_count = boundary_chk - (s->sdmasysad % boundary_chk);
+    uint64_t sdma_address = sdhci_sdma_address(s);
+    uint32_t boundary_count = boundary_chk - (sdma_address % boundary_chk);

@@ -624,3 +630,3 @@ static void 
sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
       */
-    if ((s->sdmasysad % boundary_chk) == 0) {
+    if ((sdma_address % boundary_chk) == 0) {
          page_aligned = true;
@@ -646,3 +652,4 @@ static void 
sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
              }
-            dma_memory_write(s->dma_as, s->sdmasysad, 
&s->fifo_buffer[begin],
+            dma_memory_write(s->dma_as, sdhci_sdma_address(s),
+                             &s->fifo_buffer[begin],
                               s->data_count - begin, 
MEMTXATTRS_UNSPECIFIED);
@@ -667,3 +674,4 @@ static void 
sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
              }
-            dma_memory_read(s->dma_as, s->sdmasysad, 
&s->fifo_buffer[begin],
+            dma_memory_read(s->dma_as, sdhci_sdma_address(s),
+                            &s->fifo_buffer[begin],
                              s->data_count - begin, 
MEMTXATTRS_UNSPECIFIED);
@@ -702,6 +710,8 @@ static void 
sdhci_sdma_transfer_single_block(SDHCIState *s)
          sdbus_read_data(&s->sdbus, s->fifo_buffer, datacnt);
-        dma_memory_write(s->dma_as, s->sdmasysad, s->fifo_buffer, datacnt,
+        dma_memory_write(s->dma_as, sdhci_sdma_address(s),
+                         s->fifo_buffer, datacnt,
                           MEMTXATTRS_UNSPECIFIED);
      } else {
-        dma_memory_read(s->dma_as, s->sdmasysad, s->fifo_buffer, datacnt,
+        dma_memory_read(s->dma_as, sdhci_sdma_address(s),
+                        s->fifo_buffer, datacnt,
                          MEMTXATTRS_UNSPECIFIED);
---

This patch becoming simpler:

-- >8 --
diff --git a/hw/sd/sdhci-internal.h b/hw/sd/sdhci-internal.h
index 4aeed120bf1..2116995dcca 100644
--- a/hw/sd/sdhci-internal.h
+++ b/hw/sd/sdhci-internal.h
@@ -203,2 +203,3 @@ FIELD(SDHC_HOSTCTL2, CMD23_ENA,       11, 1); /* 
since v4 */
  FIELD(SDHC_HOSTCTL2, VERSION4,        12, 1); /* since v4 */
+FIELD(SDHC_HOSTCTL2, ADDRESSING_64,   13, 1); /* since v4 */
  FIELD(SDHC_HOSTCTL2, ASYNC_INT,       14, 1);
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 93e1687dbe5..3fba3970d4f 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -600,5 +600,20 @@ static void sdhci_write_dataport(SDHCIState *s, 
uint32_t value, unsigned size)

+static bool sdhci_version4_enabled(SDHCIState *s)
+{
+    return FIELD_EX32(s->hostctl2, SDHC_HOSTCTL2, VERSION4);
+}
+
+static bool sdhci_64bit_addressing_enabled(SDHCIState *s)
+{
+    return FIELD_EX32(s->hostctl2, SDHC_HOSTCTL2, ADDRESSING_64);
+}
+
  static uint64_t sdhci_sdma_address(SDHCIState *s)
  {
+    if (!sdhci_version4_enabled(s)) {
          return s->sdmasysad;
+    }
+
+    return sdhci_64bit_addressing_enabled(s) ?
+           s->admasysaddr : (uint32_t)s->admasysaddr;
  }
@@ -607,3 +622,13 @@ static void sdhci_advance_sdma_address(SDHCIState 
*s, uint32_t bytes)
  {
+    if (!sdhci_version4_enabled(s)) {
          s->sdmasysad += bytes;
+    } else if (sdhci_64bit_addressing_enabled(s)) {
+        s->admasysaddr += bytes;
+    } else {
+        uint32_t address = s->admasysaddr;
+
+        address += bytes;
+        s->admasysaddr = (s->admasysaddr & 0xffffffff00000000ULL) |
+                         address;
+    }
  }
@@ -1382,6 +1407,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t 
val, unsigned size)
              /*
-             * VERSION4 is writable even without UHS-I. Preserve all other
-             * Host Control 2 bits when UHS-I is not supported.
+             * Version 4 fields are writable even without UHS-I. 
Preserve all
+             * other Host Control 2 bits when UHS-I is not supported.
               */
-            uint16_t independent = R_SDHC_HOSTCTL2_VERSION4_MASK;
+            uint16_t independent = R_SDHC_HOSTCTL2_VERSION4_MASK |
+                                   R_SDHC_HOSTCTL2_ADDRESSING_64_MASK;

---

Reviewed-by: Philippe Mathieu-Daudé <[email protected]>