[PATCH 0/4] hw/sd: Improve performance of read/write/erase

[PATCH 0/4] hw/sd: Improve performance of read/write/erase

[Christian Speich]

This patch series improves the performance of read/write/erase operations
on sdcards.

This is done by increasing the maximum buffer size that is worked on.
From 1 byte (master) to 512 bytes (first commit) to larger than 512
(adma commit).

Testing on my system with fio I see the following rough performance 
values in MiB/s.

              read write readwrite 
       master:   6     6     3/  3
 first commit:  51    43    23/ 23
second commit: 392   180   144/143

Tested on a 2GiB raw image with:
  fio --filename=/dev/mmcblk0 --direct=1 --runtime=60 --time_based --bs=128k --rw={mode}

The adma values are somewhat unstable but always >100MiB/s, I'm not sure
why but I guess it has something to do with the host side caching.

For erasing the third commit changes the erase operation to write zeros,
as indicated by DATA_STAT_AFTER_ERASE in SCR.

The fourth commit allows erasure in large blocks, to speed it up
significantly. Erasing 2GiB now takes 0.1s instead of 26s.

Signed-off-by: Christian Speich <c.speich@avm.de>
---
Christian Speich (4):
      hw/sd: Switch from byte-wise to buf+len read/writes
      hw/sd/sdhci: Don't use bounce buffer for ADMA
      hw/sd/sdcard: Erase blocks to zero
      hw/sd/sdcard: Erase in large blocks

 hw/sd/core.c       |  16 +---
 hw/sd/sd.c         | 277 ++++++++++++++++++++++++++++++++++++++++-------------
 hw/sd/sdhci.c      | 102 +++++++++++---------
 include/hw/sd/sd.h |  13 +--
 4 files changed, 277 insertions(+), 131 deletions(-)
---
base-commit: e7c1e8043a69c5a8efa39d4f9d111f7c72c076e6
change-id: 20250912-sdcard-performance-b4-d908bbb5a004

Best regards,
-- 
Christian Speich <c.speich@avm.de>

[PATCH 1/4] hw/sd: Switch from byte-wise to buf+len read/writes

[Christian Speich]

Currently, each read/write is eventually split into individual bytes and
then later reconstructed into blocks. Byte-wise operation is always bad
for performance.

This patch switches from the read_byte/write_byte primitives to
read_data/write_data which take a buf + length that should be filled.
Byte-wise operation is still supported via sdbus_{read,write}_byte.

As before, arbitrarily sized and aligned data can be passed. The new
primitives try to pass the largest possible buffer down to the block
layer. This enhances speed and if possible avoids the local bounce
buffer.

Signed-off-by: Christian Speich <c.speich@avm.de>
---
 hw/sd/core.c       |  16 +---
 hw/sd/sd.c         | 257 +++++++++++++++++++++++++++++++++++++++++------------
 include/hw/sd/sd.h |  13 +--
 3 files changed, 209 insertions(+), 77 deletions(-)

diff --git a/hw/sd/core.c b/hw/sd/core.c
index d3c9017445e01c2885a115656ecf23dc336d3b0f..5f05748b9942d5a359d7dee3e0369cf96fc3571b 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -113,22 +113,18 @@ void sdbus_write_byte(SDBus *sdbus, uint8_t value)
     if (card) {
         SDCardClass *sc = SDMMC_COMMON_GET_CLASS(card);
 
-        sc->write_byte(card, value);
+        sc->write_data(card, &value, 1);
     }
 }
 
 void sdbus_write_data(SDBus *sdbus, const void *buf, size_t length)
 {
     SDState *card = get_card(sdbus);
-    const uint8_t *data = buf;
 
     if (card) {
         SDCardClass *sc = SDMMC_COMMON_GET_CLASS(card);
 
-        for (size_t i = 0; i < length; i++) {
-            trace_sdbus_write(sdbus_name(sdbus), data[i]);
-            sc->write_byte(card, data[i]);
-        }
+        sc->write_data(card, buf, length);
     }
 }
 
@@ -140,7 +136,7 @@ uint8_t sdbus_read_byte(SDBus *sdbus)
     if (card) {
         SDCardClass *sc = SDMMC_COMMON_GET_CLASS(card);
 
-        value = sc->read_byte(card);
+        sc->read_data(card, &value, 1);
     }
     trace_sdbus_read(sdbus_name(sdbus), value);
 
@@ -150,15 +146,11 @@ uint8_t sdbus_read_byte(SDBus *sdbus)
 void sdbus_read_data(SDBus *sdbus, void *buf, size_t length)
 {
     SDState *card = get_card(sdbus);
-    uint8_t *data = buf;
 
     if (card) {
         SDCardClass *sc = SDMMC_COMMON_GET_CLASS(card);
 
-        for (size_t i = 0; i < length; i++) {
-            data[i] = sc->read_byte(card);
-            trace_sdbus_read(sdbus_name(sdbus), data[i]);
-        }
+        sc->read_data(card, buf, length);
     }
 }
 
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index d7a496d77c9200d45c267e2b8ee31d026ca34795..23764ed99f36cf39ee7abe02f08e51897c05e718 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1049,24 +1049,36 @@ static const VMStateDescription sd_vmstate = {
     },
 };
 
-static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
+static void sd_blk_read_direct(SDState *sd, void* buf, uint64_t addr,
+                               uint32_t len)
 {
     trace_sdcard_read_block(addr, len);
     addr += sd_part_offset(sd);
-    if (!sd->blk || blk_pread(sd->blk, addr, len, sd->data, 0) < 0) {
+    if (!sd->blk || blk_pread(sd->blk, addr, len, buf, 0) < 0) {
         fprintf(stderr, "sd_blk_read: read error on host side\n");
     }
 }
 
-static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
+static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
+{
+    sd_blk_read_direct(sd, sd->data, addr, len);
+}
+
+static void sd_blk_write_direct(SDState *sd, const void *buf, uint64_t addr,
+                                uint32_t len)
 {
     trace_sdcard_write_block(addr, len);
     addr += sd_part_offset(sd);
-    if (!sd->blk || blk_pwrite(sd->blk, addr, len, sd->data, 0) < 0) {
+    if (!sd->blk || blk_pwrite(sd->blk, addr, len, buf, 0) < 0) {
         fprintf(stderr, "sd_blk_write: write error on host side\n");
     }
 }
 
+static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
+{
+    sd_blk_write_direct(sd, sd->data, addr, len);
+}
+
 static void sd_erase(SDState *sd)
 {
     uint64_t erase_start = sd->erase_start;
@@ -1348,7 +1360,7 @@ static sd_rsp_type_t sd_cmd_optional(SDState *sd, SDRequest req)
     return sd_illegal;
 }
 
-/* Configure fields for following sd_generic_write_byte() calls */
+/* Configure fields for following sd_generic_write_date() calls */
 static sd_rsp_type_t sd_cmd_to_receivingdata(SDState *sd, SDRequest req,
                                              uint64_t start, size_t size)
 {
@@ -1363,7 +1375,7 @@ static sd_rsp_type_t sd_cmd_to_receivingdata(SDState *sd, SDRequest req,
     return sd_r1;
 }
 
-/* Configure fields for following sd_generic_read_byte() calls */
+/* Configure fields for following sd_generic_read_data() calls */
 static sd_rsp_type_t sd_cmd_to_sendingdata(SDState *sd, SDRequest req,
                                            uint64_t start,
                                            const void *data, size_t size)
@@ -2352,23 +2364,35 @@ send_response:
 }
 
 /* Return true if buffer is consumed. Configured by sd_cmd_to_receivingdata() */
-static bool sd_generic_write_byte(SDState *sd, uint8_t value)
+static bool sd_generic_write_data(SDState *sd, const void* buf, size_t length)
 {
-    sd->data[sd->data_offset] = value;
+    size_t to_write = MIN(sd->data_size - sd->data_offset, length);
 
-    if (++sd->data_offset >= sd->data_size) {
+    memcpy(sd->data, buf, to_write);
+    sd->data_offset += to_write;
+
+    if (sd->data_offset >= sd->data_size) {
         sd->state = sd_transfer_state;
         return true;
     }
+
     return false;
 }
 
 /* Return true when buffer is consumed. Configured by sd_cmd_to_sendingdata() */
-static bool sd_generic_read_byte(SDState *sd, uint8_t *value)
+static bool sd_generic_read_data(SDState *sd, void *buf, size_t length)
 {
-    *value = sd->data[sd->data_offset];
+    size_t to_read = MIN(sd->data_size - sd->data_offset, length);
+
+    memcpy(buf, sd->data, to_read);
+    sd->data_offset += to_read;
+
+    /* Fill remaining with zero, if requested to read more than requested. */
+    if (to_read < length) {
+        memset(buf + to_read, 0, length - to_read);
+    }
 
-    if (++sd->data_offset >= sd->data_size) {
+    if (sd->data_offset >= sd->data_size) {
         sd->state = sd_transfer_state;
         return true;
     }
@@ -2376,7 +2400,7 @@ static bool sd_generic_read_byte(SDState *sd, uint8_t *value)
     return false;
 }
 
-static void sd_write_byte(SDState *sd, uint8_t value)
+static void sd_write_data(SDState *sd, const void *buf, size_t length)
 {
     int i;
 
@@ -2395,10 +2419,10 @@ static void sd_write_byte(SDState *sd, uint8_t value)
 
     trace_sdcard_write_data(sd->proto->name,
                             sd->last_cmd_name,
-                            sd->current_cmd, sd->data_offset, value);
+                            sd->current_cmd, sd->data_offset, 0);
     switch (sd->current_cmd) {
     case 24:  /* CMD24:  WRITE_SINGLE_BLOCK */
-        if (sd_generic_write_byte(sd, value)) {
+        if (sd_generic_write_data(sd, buf, length)) {
             /* TODO: Check CRC before committing */
             sd->state = sd_programming_state;
             sd_blk_write(sd, sd->data_start, sd->data_offset);
@@ -2410,32 +2434,76 @@ static void sd_write_byte(SDState *sd, uint8_t value)
         break;
 
     case 25:  /* CMD25:  WRITE_MULTIPLE_BLOCK */
-        if (sd->data_offset == 0) {
-            /* Start of the block - let's check the address is valid */
-            if (!address_in_range(sd, "WRITE_MULTIPLE_BLOCK",
-                                  sd->data_start, sd->blk_len)) {
-                break;
-            }
-            if (sd->size <= SDSC_MAX_CAPACITY) {
-                if (sd_wp_addr(sd, sd->data_start)) {
+        if (!address_in_range(sd, "WRITE_MULTIPLE_BLOCK",
+                              sd->data_start + sd->data_offset, length)) {
+            /* Limit writing data to our device size */
+            length = sd->size - sd->data_start - sd->data_offset;
+        }
+
+        if (sd->size <= SDSC_MAX_CAPACITY) {
+            uint64_t start = sd->data_start + sd->data_offset;
+
+            /*
+             * Check if any covered address violates WP. If so, limit our write
+             * up to the allowed address.
+             */
+            for (uint64_t addr = start; addr < start + length;
+                 addr = ROUND_UP(addr + 1, WPGROUP_SIZE)) {
+                if (sd_wp_addr(sd, addr)) {
                     sd->card_status |= WP_VIOLATION;
+
+                    length = addr - start - 1;
                     break;
                 }
             }
         }
-        sd->data[sd->data_offset++] = value;
-        if (sd->data_offset >= sd->blk_len) {
-            /* TODO: Check CRC before committing */
+
+        /* Partial write */
+        if (sd->data_offset > 0) {
+            size_t to_write = MIN(sd->blk_len - sd->data_offset, length);
+
+            memcpy(sd->data + sd->data_offset, buf, to_write);
+            sd->data_offset += to_write;
+            buf += to_write;
+            length -= to_write;
+
+            if (sd->data_offset >= sd->blk_len) {
+                sd->state = sd_programming_state;
+                sd_blk_write(sd, sd->data_start, sd->blk_len);
+                sd->blk_written++;
+                sd->data_start += sd->blk_len;
+                sd->data_offset = 0;
+                sd->csd[14] |= 0x40;
+
+                /* Bzzzzzzztt .... Operation complete.  */
+                if (sd->multi_blk_cnt != 0) {
+                    if (--sd->multi_blk_cnt == 0) {
+                        /* Stop! */
+                        sd->state = sd_transfer_state;
+                        break;
+                    }
+                }
+
+                sd->state = sd_receivingdata_state;
+            }
+        }
+
+        /* Try to write multiple of block sizes */
+        if (length >= sd->blk_len) {
+            size_t to_write = QEMU_ALIGN_DOWN(length, sd->blk_len);
+
             sd->state = sd_programming_state;
-            sd_blk_write(sd, sd->data_start, sd->data_offset);
-            sd->blk_written++;
-            sd->data_start += sd->blk_len;
-            sd->data_offset = 0;
+            sd_blk_write_direct(sd, buf, sd->data_start, to_write);
+            sd->blk_written += to_write / sd->blk_len;
+            sd->data_start += to_write;
             sd->csd[14] |= 0x40;
+            buf += to_write;
+            length -= to_write;
 
-            /* Bzzzzzzztt .... Operation complete.  */
             if (sd->multi_blk_cnt != 0) {
-                if (--sd->multi_blk_cnt == 0) {
+                sd->multi_blk_cnt -= to_write / sd->blk_len;
+
+                if (sd->multi_blk_cnt == 0) {
                     /* Stop! */
                     sd->state = sd_transfer_state;
                     break;
@@ -2444,10 +2512,16 @@ static void sd_write_byte(SDState *sd, uint8_t value)
 
             sd->state = sd_receivingdata_state;
         }
+
+        /* Partial write */
+        if (length > 0) {
+            memcpy(sd->data, buf, length);
+            sd->data_offset = length;
+        }
         break;
 
     case 26:  /* CMD26:  PROGRAM_CID */
-        if (sd_generic_write_byte(sd, value)) {
+        if (sd_generic_write_data(sd, buf, length)) {
             /* TODO: Check CRC before committing */
             sd->state = sd_programming_state;
             for (i = 0; i < sizeof(sd->cid); i ++)
@@ -2465,7 +2539,7 @@ static void sd_write_byte(SDState *sd, uint8_t value)
         break;
 
     case 27:  /* CMD27:  PROGRAM_CSD */
-        if (sd_generic_write_byte(sd, value)) {
+        if (sd_generic_write_data(sd, buf, length)) {
             /* TODO: Check CRC before committing */
             sd->state = sd_programming_state;
             for (i = 0; i < sizeof(sd->csd); i ++)
@@ -2488,7 +2562,7 @@ static void sd_write_byte(SDState *sd, uint8_t value)
         break;
 
     case 42:  /* CMD42:  LOCK_UNLOCK */
-        if (sd_generic_write_byte(sd, value)) {
+        if (sd_generic_write_data(sd, buf, length)) {
             /* TODO: Check CRC before committing */
             sd->state = sd_programming_state;
             sd_lock_command(sd);
@@ -2498,7 +2572,7 @@ static void sd_write_byte(SDState *sd, uint8_t value)
         break;
 
     case 56:  /* CMD56:  GEN_CMD */
-        sd_generic_write_byte(sd, value);
+        sd_generic_write_data(sd, buf, length);
         break;
 
     default:
@@ -2506,25 +2580,28 @@ static void sd_write_byte(SDState *sd, uint8_t value)
     }
 }
 
-static uint8_t sd_read_byte(SDState *sd)
+static void sd_read_data(SDState *sd, void *data, size_t length)
 {
     /* TODO: Append CRCs */
     const uint8_t dummy_byte = 0x00;
-    uint8_t ret;
     uint32_t io_len;
+    void *fill_end = data + length;
 
     if (!sd->blk || !blk_is_inserted(sd->blk)) {
-        return dummy_byte;
+        memset(data, dummy_byte, length);
+        return;
     }
 
     if (sd->state != sd_sendingdata_state) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: not in Sending-Data state\n", __func__);
-        return dummy_byte;
+        memset(data, dummy_byte, length);
+        return;
     }
 
     if (sd->card_status & (ADDRESS_ERROR | WP_VIOLATION)) {
-        return dummy_byte;
+        memset(data, dummy_byte, length);
+        return;
     }
 
     io_len = sd_blk_len(sd);
@@ -2544,40 +2621,102 @@ static uint8_t sd_read_byte(SDState *sd)
     case 30: /* CMD30:  SEND_WRITE_PROT */
     case 51: /* ACMD51: SEND_SCR */
     case 56: /* CMD56:  GEN_CMD */
-        sd_generic_read_byte(sd, &ret);
+        sd_generic_read_data(sd, data, length);
         break;
 
     case 18:  /* CMD18:  READ_MULTIPLE_BLOCK */
-        if (sd->data_offset == 0) {
-            if (!address_in_range(sd, "READ_MULTIPLE_BLOCK",
-                                  sd->data_start, io_len)) {
-                return dummy_byte;
+        if (!address_in_range(sd, "READ_MULTIPLE_BLOCK",
+                              sd->data_start + sd->data_offset, length)) {
+            /* Limit reading data to our device size */
+            length = sd->size - sd->data_start - sd->data_offset;
+        }
+
+        /* We have a partially read block. */
+        if (sd->data_offset > 0) {
+            size_t to_read = MIN(sd->data_size - sd->data_offset, length);
+
+            memcpy(data, sd->data + sd->data_offset, to_read);
+
+            sd->data_offset += to_read;
+            data += to_read;
+            length -= to_read;
+
+            /* Partial read is complete, clear state. */
+            if (sd->data_offset >= sd->data_size) {
+                sd->data_start += io_len;
+                sd->data_size = 0;
+                sd->data_offset = 0;
+
+                if (sd->multi_blk_cnt != 0) {
+                    if (--sd->multi_blk_cnt == 0) {
+                        sd->state = sd_transfer_state;
+                    }
+                }
             }
-            sd_blk_read(sd, sd->data_start, io_len);
         }
-        ret = sd->data[sd->data_offset ++];
 
-        if (sd->data_offset >= io_len) {
-            sd->data_start += io_len;
-            sd->data_offset = 0;
+        /*
+         * Try to read multiples of the block size directly bypassing the local
+         * bounce buffer.
+         */
+        if (sd->state == sd_sendingdata_state && length >= io_len) {
+            size_t to_read = QEMU_ALIGN_DOWN(length, io_len);
 
+            /* For limited reads, only read the requested block count. */
             if (sd->multi_blk_cnt != 0) {
-                if (--sd->multi_blk_cnt == 0) {
-                    /* Stop! */
+                to_read = MIN(to_read, sd->multi_blk_cnt * io_len);
+            }
+
+            sd_blk_read_direct(sd, data, sd->data_start,
+                               to_read);
+
+            sd->data_start += to_read;
+            data += to_read;
+            length -= to_read;
+
+            if (sd->multi_blk_cnt != 0) {
+                sd->multi_blk_cnt -= to_read / io_len;
+
+                if (sd->multi_blk_cnt == 0) {
                     sd->state = sd_transfer_state;
-                    break;
                 }
             }
         }
+
+        /* Read partial at the end */
+        if (sd->state == sd_sendingdata_state && length > 0) {
+            /* Fill the buffer */
+            sd_blk_read(sd, sd->data_start, io_len);
+
+            memcpy(data, sd->data, length);
+
+            sd->data_size = io_len;
+            sd->data_offset = length;
+            data += length;
+            length = 0;
+
+            /*
+             * No need to check multi_blk_cnt, as to_read will always be
+             * < io_len and we will never finish a block here.
+             */
+        }
+
+        /*
+         * We always need to fill the supplied buffer fully, recalulate
+         * remaining length based on the actual buffer end and not a possible
+         * early end due to a read past the device size.
+         */
+        length = fill_end - data;
+        if (length > 0) {
+            memset(data, 0, length);
+        }
         break;
 
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "%s: DAT read illegal for command %s\n",
                                        __func__, sd->last_cmd_name);
-        return dummy_byte;
+        memset(data, dummy_byte, length);
     }
-
-    return ret;
 }
 
 static bool sd_receive_ready(SDState *sd)
@@ -2859,8 +2998,8 @@ static void sdmmc_common_class_init(ObjectClass *klass, const void *data)
     sc->get_dat_lines = sd_get_dat_lines;
     sc->get_cmd_line = sd_get_cmd_line;
     sc->do_command = sd_do_command;
-    sc->write_byte = sd_write_byte;
-    sc->read_byte = sd_read_byte;
+    sc->write_data = sd_write_data;
+    sc->read_data = sd_read_data;
     sc->receive_ready = sd_receive_ready;
     sc->data_ready = sd_data_ready;
     sc->get_inserted = sd_get_inserted;
diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 91b5c40a5f893ee41cff21ebbe78d7dfa753d94e..56cd30391bfefd867f4ce97a58de5cc0acd84e97 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -107,22 +107,23 @@ struct SDCardClass {
     size_t (*do_command)(SDState *sd, SDRequest *req,
                          uint8_t *resp, size_t respsz);
     /**
-     * Write a byte to a SD card.
+     * Write data to a SD card.
      * @sd: card
-     * @value: byte to write
+     * @value: data to write
      *
      * Write a byte on the data lines of a SD card.
      */
-    void (*write_byte)(SDState *sd, uint8_t value);
+    void (*write_data)(SDState *sd, const void* buf, size_t len);
     /**
-     * Read a byte from a SD card.
+     * Read data from a SD card.
      * @sd: card
      *
-     * Read a byte from the data lines of a SD card.
+     * Read data from the data lines of a SD card. The requestes length is
+     * always filled even if an error occours.
      *
      * Return: byte value read
      */
-    uint8_t (*read_byte)(SDState *sd);
+    void (*read_data)(SDState *sd, void* buf, size_t len);
     bool (*receive_ready)(SDState *sd);
     bool (*data_ready)(SDState *sd);
     void (*set_voltage)(SDState *sd, uint16_t millivolts);

-- 
2.43.0

[PATCH 2/4] hw/sd/sdhci: Don't use bounce buffer for ADMA

[Christian Speich]

Currently, ADMA will temporarily store data into a local bounce buffer
when transferring it. This will produce unneeded copies of the data and
limit us to the bounce buffer size for each step.

This patch now maps the requested DMA address and passes this buffer
directly to sdbus_{read,write}_data. This allows to pass much larger
buffers down to increase the performance. sdbus_{read,write}_data is
already able to handle arbitrary length and alignments, so we do not
need to ensure this.

Signed-off-by: Christian Speich <c.speich@avm.de>
---
 hw/sd/sdhci.c | 102 +++++++++++++++++++++++++++++++---------------------------
 1 file changed, 55 insertions(+), 47 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 3c897e54b721075a3ebd215e027fb73a65ff39b2..94ba23a8da990e69fd59c039e4fdd25b98929dfd 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -774,7 +774,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
 
 static void sdhci_do_adma(SDHCIState *s)
 {
-    unsigned int begin, length;
+    unsigned int length;
     const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
     const MemTxAttrs attrs = { .memory = true };
     ADMADescr dscr = {};
@@ -816,66 +816,74 @@ static void sdhci_do_adma(SDHCIState *s)
             if (s->trnmod & SDHC_TRNS_READ) {
                 s->prnsts |= SDHC_DOING_READ;
                 while (length) {
-                    if (s->data_count == 0) {
-                        sdbus_read_data(&s->sdbus, s->fifo_buffer, block_size);
-                    }
-                    begin = s->data_count;
-                    if ((length + begin) < block_size) {
-                        s->data_count = length + begin;
-                        length = 0;
-                     } else {
-                        s->data_count = block_size;
-                        length -= block_size - begin;
-                    }
-                    res = dma_memory_write(s->dma_as, dscr.addr,
-                                           &s->fifo_buffer[begin],
-                                           s->data_count - begin,
-                                           attrs);
-                    if (res != MEMTX_OK) {
+                    dma_addr_t dma_len = length;
+
+                    void *buf = dma_memory_map(s->dma_as, dscr.addr, &dma_len,
+                                               DMA_DIRECTION_FROM_DEVICE,
+                                               attrs);
+
+                    if (buf == NULL) {
+                        res = MEMTX_ERROR;
                         break;
+                    } else {
+                        res = MEMTX_OK;
                     }
-                    dscr.addr += s->data_count - begin;
-                    if (s->data_count == block_size) {
-                        s->data_count = 0;
-                        if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
-                            s->blkcnt--;
-                            if (s->blkcnt == 0) {
-                                break;
-                            }
+
+                    sdbus_read_data(&s->sdbus, buf, dma_len);
+                    length -= dma_len;
+                    dscr.addr += dma_len;
+
+                    dma_memory_unmap(s->dma_as, buf, dma_len,
+                                     DMA_DIRECTION_FROM_DEVICE, dma_len);
+
+                    if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
+                        size_t transfered = s->data_count + dma_len;
+
+                        s->blkcnt -= transfered / block_size;
+                        s->data_count = transfered % block_size;
+
+                        if (s->blkcnt == 0) {
+                            s->data_count = 0;
+                            break;
                         }
                     }
                 }
             } else {
                 s->prnsts |= SDHC_DOING_WRITE;
                 while (length) {
-                    begin = s->data_count;
-                    if ((length + begin) < block_size) {
-                        s->data_count = length + begin;
-                        length = 0;
-                     } else {
-                        s->data_count = block_size;
-                        length -= block_size - begin;
-                    }
-                    res = dma_memory_read(s->dma_as, dscr.addr,
-                                          &s->fifo_buffer[begin],
-                                          s->data_count - begin,
-                                          attrs);
-                    if (res != MEMTX_OK) {
+                    dma_addr_t dma_len = length;
+
+                    void *buf = dma_memory_map(s->dma_as, dscr.addr, &dma_len,
+                                               DMA_DIRECTION_TO_DEVICE, attrs);
+
+                    if (buf == NULL) {
+                        res = MEMTX_ERROR;
                         break;
+                    } else {
+                        res = MEMTX_OK;
                     }
-                    dscr.addr += s->data_count - begin;
-                    if (s->data_count == block_size) {
-                        sdbus_write_data(&s->sdbus, s->fifo_buffer, block_size);
-                        s->data_count = 0;
-                        if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
-                            s->blkcnt--;
-                            if (s->blkcnt == 0) {
-                                break;
-                            }
+
+                    sdbus_write_data(&s->sdbus, buf, dma_len);
+                    length -= dma_len;
+                    dscr.addr += dma_len;
+
+                    dma_memory_unmap(s->dma_as, buf, dma_len,
+                                     DMA_DIRECTION_TO_DEVICE, dma_len);
+
+                    if (s->trnmod & SDHC_TRNS_BLK_CNT_EN) {
+                        size_t transfered = s->data_count + dma_len;
+
+                        s->blkcnt -= transfered / block_size;
+                        s->data_count = transfered % block_size;
+
+                        if (s->blkcnt == 0) {
+                            s->data_count = 0;
+                            break;
                         }
                     }
                 }
             }
+
             if (res != MEMTX_OK) {
                 s->data_count = 0;
                 if (s->errintstsen & SDHC_EISEN_ADMAERR) {

-- 
2.43.0

[PATCH 3/4] hw/sd/sdcard: Erase blocks to zero

[Christian Speich]

Currently, erased blocks are filled with 0xFF. However SCR Bit 55
(DATA_STAT_AFTER_ERASE) indicates that an erase produces zeros. One of
them is wrong.

As erasing to zero is more performant and allows block devices to
use optimizations, we the erase to produce 0x00.

Signed-off-by: Christian Speich <c.speich@avm.de>
---
 hw/sd/sd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 23764ed99f36cf39ee7abe02f08e51897c05e718..94ef3cc62582717ee044c4b114b7f22bd1b4a256 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1115,7 +1115,6 @@ static void sd_erase(SDState *sd)
     sd->erase_end = INVALID_ADDRESS;
     sd->csd[14] |= 0x40;
 
-    memset(sd->data, 0xff, erase_len);
     for (erase_addr = erase_start; erase_addr <= erase_end;
          erase_addr += erase_len) {
         if (sdsc) {
@@ -1127,7 +1126,8 @@ static void sd_erase(SDState *sd)
                 continue;
             }
         }
-        sd_blk_write(sd, erase_addr, erase_len);
+        blk_pwrite_zeroes(sd->blk, erase_addr + sd_part_offset(sd),
+                          erase_len, 0);
     }
 }
 

-- 
2.43.0

[PATCH 4/4] hw/sd/sdcard: Erase in large blocks

[Christian Speich]

Erasing each block individually is slow, so this patch reworks the logic
to erase as much as possible in one go.

Signed-off-by: Christian Speich <c.speich@avm.de>
---
 hw/sd/sd.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 94ef3cc62582717ee044c4b114b7f22bd1b4a256..42870fa19414be61e43d2e07619ed193cc514319 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1086,7 +1086,6 @@ static void sd_erase(SDState *sd)
     bool sdsc = true;
     uint64_t wpnum;
     uint64_t erase_addr;
-    int erase_len = 1 << HWBLOCK_SHIFT;
 
     trace_sdcard_erase(sd->erase_start, sd->erase_end);
     if (sd->erase_start == INVALID_ADDRESS
@@ -1115,19 +1114,26 @@ static void sd_erase(SDState *sd)
     sd->erase_end = INVALID_ADDRESS;
     sd->csd[14] |= 0x40;
 
-    for (erase_addr = erase_start; erase_addr <= erase_end;
-         erase_addr += erase_len) {
-        if (sdsc) {
-            /* Only SDSC cards support write protect groups */
+    /* Only SDSC cards support write protect groups */
+    if (sdsc) {
+        for (erase_addr = erase_start; erase_addr <= erase_end;
+             erase_addr = ROUND_UP(erase_addr + 1, WPGROUP_SIZE)) {
+            uint64_t wp_group_end = ROUND_UP(erase_addr + 1, WPGROUP_SIZE) - 1;
+            size_t to_erase = MIN(erase_end, wp_group_end) - erase_addr;
+
             wpnum = sd_addr_to_wpnum(erase_addr);
             assert(wpnum < sd->wp_group_bits);
             if (test_bit(wpnum, sd->wp_group_bmap)) {
                 sd->card_status |= WP_ERASE_SKIP;
                 continue;
             }
+
+            blk_pwrite_zeroes(sd->blk, erase_addr + sd_part_offset(sd),
+                              to_erase, 0);
         }
-        blk_pwrite_zeroes(sd->blk, erase_addr + sd_part_offset(sd),
-                          erase_len, 0);
+    } else {
+        blk_pwrite_zeroes(sd->blk, erase_start + sd_part_offset(sd),
+                          erase_end - erase_start, 0);
     }
 }
 

-- 
2.43.0

Re: [PATCH 0/4] hw/sd: Improve performance of read/write/erase

[Christian Speich]

ping, I've not received any reaction on this series[1]. Is there anything I can do to
move this forward?

Greetings,
Christian

[1]: https://patchew.org/QEMU/20250919-sdcard-performance-b4-v1-0-e1037e481a19@avm.de/

On Fri, Sep 19, 2025 at 02:34:39PM +0200, Christian Speich wrote:
> This patch series improves the performance of read/write/erase operations
> on sdcards.
> 
> This is done by increasing the maximum buffer size that is worked on.
> >From 1 byte (master) to 512 bytes (first commit) to larger than 512
> (adma commit).
> 
> Testing on my system with fio I see the following rough performance 
> values in MiB/s.
> 
>               read write readwrite 
>        master:   6     6     3/  3
>  first commit:  51    43    23/ 23
> second commit: 392   180   144/143
> 
> Tested on a 2GiB raw image with:
>   fio --filename=/dev/mmcblk0 --direct=1 --runtime=60 --time_based --bs=128k --rw={mode}
> 
> The adma values are somewhat unstable but always >100MiB/s, I'm not sure
> why but I guess it has something to do with the host side caching.
> 
> For erasing the third commit changes the erase operation to write zeros,
> as indicated by DATA_STAT_AFTER_ERASE in SCR.
> 
> The fourth commit allows erasure in large blocks, to speed it up
> significantly. Erasing 2GiB now takes 0.1s instead of 26s.
> 
> Signed-off-by: Christian Speich <c.speich@avm.de>
> ---
> Christian Speich (4):
>       hw/sd: Switch from byte-wise to buf+len read/writes
>       hw/sd/sdhci: Don't use bounce buffer for ADMA
>       hw/sd/sdcard: Erase blocks to zero
>       hw/sd/sdcard: Erase in large blocks
> 
>  hw/sd/core.c       |  16 +---
>  hw/sd/sd.c         | 277 ++++++++++++++++++++++++++++++++++++++++-------------
>  hw/sd/sdhci.c      | 102 +++++++++++---------
>  include/hw/sd/sd.h |  13 +--
>  4 files changed, 277 insertions(+), 131 deletions(-)
> ---
> base-commit: e7c1e8043a69c5a8efa39d4f9d111f7c72c076e6
> change-id: 20250912-sdcard-performance-b4-d908bbb5a004
> 
> Best regards,
> -- 
> Christian Speich <c.speich@avm.de>
> 
> 

Re: [PATCH 0/4] hw/sd: Improve performance of read/write/erase

[Philippe Mathieu-Daudé]

Hi Christian,

On 7/11/25 10:08, Christian Speich wrote:
> ping, I've not received any reaction on this series[1]. Is there anything I can do to
> move this forward?

My apologies for missing this... I was in PTO when you posted and then
it felt into my INBOX cracks :/

> On Fri, Sep 19, 2025 at 02:34:39PM +0200, Christian Speich wrote:
>> This patch series improves the performance of read/write/erase operations
>> on sdcards.
>>
>> This is done by increasing the maximum buffer size that is worked on.
>> >From 1 byte (master) to 512 bytes (first commit) to larger than 512
>> (adma commit).
>>
>> Testing on my system with fio I see the following rough performance
>> values in MiB/s.
>>
>>                read write readwrite
>>         master:   6     6     3/  3
>>   first commit:  51    43    23/ 23
>> second commit: 392   180   144/143
>>
>> Tested on a 2GiB raw image with:
>>    fio --filename=/dev/mmcblk0 --direct=1 --runtime=60 --time_based --bs=128k --rw={mode}
>>
>> The adma values are somewhat unstable but always >100MiB/s, I'm not sure
>> why but I guess it has something to do with the host side caching.
>>
>> For erasing the third commit changes the erase operation to write zeros,
>> as indicated by DATA_STAT_AFTER_ERASE in SCR.
>>
>> The fourth commit allows erasure in large blocks, to speed it up
>> significantly. Erasing 2GiB now takes 0.1s instead of 26s.
>>
>> Signed-off-by: Christian Speich <c.speich@avm.de>
>> ---
>> Christian Speich (4):
>>        hw/sd: Switch from byte-wise to buf+len read/writes
>>        hw/sd/sdhci: Don't use bounce buffer for ADMA
>>        hw/sd/sdcard: Erase blocks to zero
>>        hw/sd/sdcard: Erase in large blocks
>>
>>   hw/sd/core.c       |  16 +---
>>   hw/sd/sd.c         | 277 ++++++++++++++++++++++++++++++++++++++++-------------
>>   hw/sd/sdhci.c      | 102 +++++++++++---------
>>   include/hw/sd/sd.h |  13 +--
>>   4 files changed, 277 insertions(+), 131 deletions(-)
>> ---
>> base-commit: e7c1e8043a69c5a8efa39d4f9d111f7c72c076e6
>> change-id: 20250912-sdcard-performance-b4-d908bbb5a004
>>
>> Best regards,
>> -- 
>> Christian Speich <c.speich@avm.de>
>>
>>

Re: [PATCH 3/4] hw/sd/sdcard: Erase blocks to zero

[Philippe Mathieu-Daudé]

On 19/9/25 14:34, Christian Speich wrote:
> Currently, erased blocks are filled with 0xFF. However SCR Bit 55
> (DATA_STAT_AFTER_ERASE) indicates that an erase produces zeros. One of
> them is wrong.

You are right, we don't set DATA_STAT_AFTER_ERASE correctly.

> As erasing to zero is more performant and allows block devices to
> use optimizations, we the erase to produce 0x00.
> 
> Signed-off-by: Christian Speich <c.speich@avm.de>
> ---
>   hw/sd/sd.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 23764ed99f36cf39ee7abe02f08e51897c05e718..94ef3cc62582717ee044c4b114b7f22bd1b4a256 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1115,7 +1115,6 @@ static void sd_erase(SDState *sd)
>       sd->erase_end = INVALID_ADDRESS;
>       sd->csd[14] |= 0x40;
>   
> -    memset(sd->data, 0xff, erase_len);
>       for (erase_addr = erase_start; erase_addr <= erase_end;
>            erase_addr += erase_len) {
>           if (sdsc) {
> @@ -1127,7 +1126,8 @@ static void sd_erase(SDState *sd)
>                   continue;
>               }
>           }
> -        sd_blk_write(sd, erase_addr, erase_len);
> +        blk_pwrite_zeroes(sd->blk, erase_addr + sd_part_offset(sd),
> +                          erase_len, 0);
>       }
>   }

I'm OK with this change, but I'd rather having a device boolean property
so we can keep the old behavior for backward compatibility. Maybe
'erase-block-as-zero'? Do you mind updating this patch?

Regards,

Phil.

Re: [PATCH 3/4] hw/sd/sdcard: Erase blocks to zero

[Christian Speich]

On Mon, Nov 24, 2025 at 05:09:03AM +0100, Philippe Mathieu-Daudé wrote:
> On 19/9/25 14:34, Christian Speich wrote:
> > Currently, erased blocks are filled with 0xFF. However SCR Bit 55
> > (DATA_STAT_AFTER_ERASE) indicates that an erase produces zeros. One of
> > them is wrong.
> 
> You are right, we don't set DATA_STAT_AFTER_ERASE correctly.
> 
> > As erasing to zero is more performant and allows block devices to
> > use optimizations, we the erase to produce 0x00.
> > 
> > Signed-off-by: Christian Speich <c.speich@avm.de>
> > ---
> >   hw/sd/sd.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index 23764ed99f36cf39ee7abe02f08e51897c05e718..94ef3cc62582717ee044c4b114b7f22bd1b4a256 100644
> > --- a/hw/sd/sd.c
> > +++ b/hw/sd/sd.c
> > @@ -1115,7 +1115,6 @@ static void sd_erase(SDState *sd)
> >       sd->erase_end = INVALID_ADDRESS;
> >       sd->csd[14] |= 0x40;
> > -    memset(sd->data, 0xff, erase_len);
> >       for (erase_addr = erase_start; erase_addr <= erase_end;
> >            erase_addr += erase_len) {
> >           if (sdsc) {
> > @@ -1127,7 +1126,8 @@ static void sd_erase(SDState *sd)
> >                   continue;
> >               }
> >           }
> > -        sd_blk_write(sd, erase_addr, erase_len);
> > +        blk_pwrite_zeroes(sd->blk, erase_addr + sd_part_offset(sd),
> > +                          erase_len, 0);
> >       }
> >   }
> 
> I'm OK with this change, but I'd rather having a device boolean property
> so we can keep the old behavior for backward compatibility. Maybe
> 'erase-block-as-zero'? Do you mind updating this patch?

I've refrained from making this a user-configruable property to keep the
code simple and as erasing to zero can be better optimized (performance
and disk usage, e.g. the following commit). Additionally, it may be less
confusing to users as new disks created by qemu-img will be filled with
zeros.

I see that this is a breaking change though, so I'll look into adding
a property.

I assume you want erase-block-as-zero to default to false? (I'd prefer
true, but thats just my use-case).

Greetings,
Christian


> 
> Regards,
> 
> Phil.
>