* [PATCH v3 1/6] hw/sd: Switch read/write primitive to buf+len
2026-02-04 13:22 [PATCH v3 0/6] hw/sd: Improve performance of read/write/erase Christian Speich via qemu development
@ 2026-02-04 13:22 ` Christian Speich via qemu development
2026-04-14 13:38 ` Philippe Mathieu-Daudé
2026-02-04 13:22 ` [PATCH v3 2/6] hw/sd/sd: Allow multi-byte read/write for generic paths Christian Speich via qemu development
` (5 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Christian Speich via qemu development @ 2026-02-04 13:22 UTC (permalink / raw)
To: qemu-devel
Cc: Philippe Mathieu-Daudé, Bin Meng, qemu-block,
Christian Speich
Currently, read/writes are broken down into individual bytes which result
in many function calls. This is quite bad for performance and since both
the layer below and above work with larger buffers, it should be
corrected.
This patch is the first that switches the corresponding interface over to
use a buf+len instead of a single byte. However, for most cases the
implementation still only reads one byte and is then called again with
the remaining buffer.
Optimizations taking advantage of this new interface are to follow in the
next commits.
Signed-off-by: Christian Speich <c.speich@avm.de>
---
hw/sd/core.c | 26 ++++++++++++++---------
hw/sd/sd.c | 62 +++++++++++++++++++++++++++++++++++-------------------
include/hw/sd/sd.h | 22 +++++++++++++------
3 files changed, 71 insertions(+), 39 deletions(-)
diff --git a/hw/sd/core.c b/hw/sd/core.c
index 3568a81e809fe107cfd0b5cc33b8b5761b11ce04..594c5e011ba30940a33799f9032c92494ee0ca19 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -113,21 +113,24 @@ 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]);
+ while (length > 0) {
+ size_t written = sc->write_data(card, buf, length);
+
+ g_assert(written >= 1);
+
+ buf += written;
+ length -= written;
}
}
}
@@ -140,7 +143,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,14 +153,17 @@ 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]);
+ while (length > 0) {
+ size_t read = sc->read_data(card, buf, length);
+
+ g_assert(read >= 1);
+
+ buf += read;
+ length -= read;
}
}
}
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 37f6e0702b0bce85915ef727ba1ec05f02f9c32c..135113add29b5ee3cb11d9d535355eba8f6cb3f7 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2638,30 +2638,37 @@ static bool sd_generic_read_byte(SDState *sd, uint8_t *value)
return false;
}
-static void sd_write_byte(SDState *sd, uint8_t value)
+static size_t sd_write_data(SDState *sd, const void *buf, size_t length)
{
unsigned int partition_access;
int i;
+ const uint8_t *value = buf;
if (!sd->blk || !blk_is_inserted(sd->blk)) {
- return;
+ return length;
}
if (sd->state != sd_receivingdata_state) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: not in Receiving-Data state\n", __func__);
- return;
+ return length;
}
if (sd->card_status & (ADDRESS_ERROR | WP_VIOLATION))
- return;
+ return length;
+
+ /*
+ * Only read one byte at a time. We will be called again with the
+ * remaining.
+ */
+ length = 1;
trace_sdcard_write_data(sd->proto->name,
sd->last_cmd_name,
- sd->current_cmd, sd->data_offset, value);
+ sd->current_cmd, sd->data_offset, value[0]);
switch (sd->current_cmd) {
case 24: /* CMD24: WRITE_SINGLE_BLOCK */
- if (sd_generic_write_byte(sd, value)) {
+ if (sd_generic_write_byte(sd, value[0])) {
/* TODO: Check CRC before committing */
sd->state = sd_programming_state;
sd_blk_write(sd, sd->data_start, sd->data_offset);
@@ -2686,7 +2693,7 @@ static void sd_write_byte(SDState *sd, uint8_t value)
}
}
}
- sd->data[sd->data_offset++] = value;
+ sd->data[sd->data_offset++] = value[0];
if (sd->data_offset >= sd->blk_len) {
/* TODO: Check CRC before committing */
sd->state = sd_programming_state;
@@ -2716,7 +2723,7 @@ static void sd_write_byte(SDState *sd, uint8_t value)
break;
case 26: /* CMD26: PROGRAM_CID */
- if (sd_generic_write_byte(sd, value)) {
+ if (sd_generic_write_byte(sd, value[0])) {
/* TODO: Check CRC before committing */
sd->state = sd_programming_state;
for (i = 0; i < sizeof(sd->cid); i ++)
@@ -2734,7 +2741,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_byte(sd, value[0])) {
/* TODO: Check CRC before committing */
sd->state = sd_programming_state;
for (i = 0; i < sizeof(sd->csd); i ++)
@@ -2757,7 +2764,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_byte(sd, value[0])) {
/* TODO: Check CRC before committing */
sd->state = sd_programming_state;
sd_lock_command(sd);
@@ -2767,36 +2774,47 @@ 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_byte(sd, value[0]);
break;
default:
g_assert_not_reached();
}
+
+ return length;
}
-static uint8_t sd_read_byte(SDState *sd)
+static size_t sd_read_data(SDState *sd, void *buf, size_t length)
{
/* TODO: Append CRCs */
const uint8_t dummy_byte = 0x00;
unsigned int partition_access;
- uint8_t ret;
uint32_t io_len;
+ uint8_t *value = buf;
if (!sd->blk || !blk_is_inserted(sd->blk)) {
- return dummy_byte;
+ memset(buf, dummy_byte, length);
+ return length;
}
if (sd->state != sd_sendingdata_state) {
qemu_log_mask(LOG_GUEST_ERROR,
"%s: not in Sending-Data state\n", __func__);
- return dummy_byte;
+ memset(buf, dummy_byte, length);
+ return length;
}
if (sd->card_status & (ADDRESS_ERROR | WP_VIOLATION)) {
- return dummy_byte;
+ memset(buf, dummy_byte, length);
+ return length;
}
+ /*
+ * We will only read one byte at a time. We will be called again with the
+ * remaining buffer.
+ */
+ length = 1;
+
io_len = sd_blk_len(sd);
trace_sdcard_read_data(sd->proto->name,
@@ -2814,7 +2832,7 @@ 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_byte(sd, value);
break;
case 18: /* CMD18: READ_MULTIPLE_BLOCK */
@@ -2831,7 +2849,7 @@ static uint8_t sd_read_byte(SDState *sd)
sd_blk_read(sd, sd->data_start, io_len);
}
}
- ret = sd->data[sd->data_offset ++];
+ *value = sd->data[sd->data_offset++];
if (sd->data_offset >= io_len) {
sd->data_start += io_len;
@@ -2850,10 +2868,10 @@ static uint8_t sd_read_byte(SDState *sd)
default:
qemu_log_mask(LOG_GUEST_ERROR, "%s: DAT read illegal for command %s\n",
__func__, sd->last_cmd_name);
- return dummy_byte;
+ *value = dummy_byte;
}
- return ret;
+ return length;
}
static bool sd_receive_ready(SDState *sd)
@@ -3173,8 +3191,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 d12f24955a5ba3c1ba9ab851d75992e830c00608..2162fb584020110bcdaa7a92c2a05b6cfc041d2f 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -107,22 +107,30 @@ 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
+ * @len: length of data
*
- * Write a byte on the data lines of a SD card.
+ * Write data on the data lines of a SD card. May write not all data, in
+ * which case it should be called again. At least one byte must be consumed.
+ *
+ * Return: number of bytes actually written. >= 1
*/
- void (*write_byte)(SDState *sd, uint8_t value);
+ size_t (*write_data)(SDState *sd, const void *buf, size_t len);
/**
* Read a byte from a SD card.
* @sd: card
+ * @buf: buffer to receive the data
+ * @len: size of data to read
*
- * Read a byte from the data lines of a SD card.
+ * Read data from the data lines of a SD card. This may not read all
+ * requested data, in this case it should be called again with the remaining
+ * buffer. At least one byte must be read.
*
- * Return: byte value read
+ * Return: number of bytes actually read. >= 1
*/
- uint8_t (*read_byte)(SDState *sd);
+ size_t (*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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 1/6] hw/sd: Switch read/write primitive to buf+len
2026-02-04 13:22 ` [PATCH v3 1/6] hw/sd: Switch read/write primitive to buf+len Christian Speich via qemu development
@ 2026-04-14 13:38 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-04-14 13:38 UTC (permalink / raw)
To: Christian Speich, qemu-devel; +Cc: Bin Meng, qemu-block
Hi Christian,
On 4/2/26 14:22, Christian Speich wrote:
> Currently, read/writes are broken down into individual bytes which result
> in many function calls. This is quite bad for performance and since both
> the layer below and above work with larger buffers, it should be
> corrected.
>
> This patch is the first that switches the corresponding interface over to
> use a buf+len instead of a single byte. However, for most cases the
> implementation still only reads one byte and is then called again with
> the remaining buffer.
>
> Optimizations taking advantage of this new interface are to follow in the
> next commits.
>
> Signed-off-by: Christian Speich <c.speich@avm.de>
> ---
> hw/sd/core.c | 26 ++++++++++++++---------
> hw/sd/sd.c | 62 +++++++++++++++++++++++++++++++++++-------------------
> include/hw/sd/sd.h | 22 +++++++++++++------
> 3 files changed, 71 insertions(+), 39 deletions(-)
>
> diff --git a/hw/sd/core.c b/hw/sd/core.c
> index 3568a81e809fe107cfd0b5cc33b8b5761b11ce04..594c5e011ba30940a33799f9032c92494ee0ca19 100644
> --- a/hw/sd/core.c
> +++ b/hw/sd/core.c
> @@ -113,21 +113,24 @@ 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]);
Instead of removing this trace event,
> - sc->write_byte(card, data[i]);
> + while (length > 0) {
> + size_t written = sc->write_data(card, buf, length);
> +
move it here, using qemu_hexdump_line().
> + g_assert(written >= 1);
> +
> + buf += written;
> + length -= written;
> }
> }
> }
> @@ -140,7 +143,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,14 +153,17 @@ 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]);
> + while (length > 0) {
> + size_t read = sc->read_data(card, buf, length);
> +
Ditto with trace_sdbus_read().
> + g_assert(read >= 1);
> +
> + buf += read;
> + length -= read;
> }
> }
> }
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 37f6e0702b0bce85915ef727ba1ec05f02f9c32c..135113add29b5ee3cb11d9d535355eba8f6cb3f7 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2638,30 +2638,37 @@ static bool sd_generic_read_byte(SDState *sd, uint8_t *value)
> return false;
> }
>
> -static void sd_write_byte(SDState *sd, uint8_t value)
> +static size_t sd_write_data(SDState *sd, const void *buf, size_t length)
> {
> unsigned int partition_access;
> int i;
> + const uint8_t *value = buf;
>
> if (!sd->blk || !blk_is_inserted(sd->blk)) {
> - return;
> + return length;
> }
>
> if (sd->state != sd_receivingdata_state) {
> qemu_log_mask(LOG_GUEST_ERROR,
> "%s: not in Receiving-Data state\n", __func__);
> - return;
> + return length;
> }
>
> if (sd->card_status & (ADDRESS_ERROR | WP_VIOLATION))
> - return;
> + return length;
> +
> + /*
> + * Only read one byte at a time. We will be called again with the
> + * remaining.
> + */
> + length = 1;
>
> trace_sdcard_write_data(sd->proto->name,
> sd->last_cmd_name,
> - sd->current_cmd, sd->data_offset, value);
> + sd->current_cmd, sd->data_offset, value[0]);
Another qemu_hexdump_line() candidate.
> switch (sd->current_cmd) {
> case 24: /* CMD24: WRITE_SINGLE_BLOCK */
> - if (sd_generic_write_byte(sd, value)) {
> + if (sd_generic_write_byte(sd, value[0])) {
> /* TODO: Check CRC before committing */
> sd->state = sd_programming_state;
> sd_blk_write(sd, sd->data_start, sd->data_offset);
> @@ -2686,7 +2693,7 @@ static void sd_write_byte(SDState *sd, uint8_t value)
> }
> }
> }
> - sd->data[sd->data_offset++] = value;
> + sd->data[sd->data_offset++] = value[0];
> if (sd->data_offset >= sd->blk_len) {
> /* TODO: Check CRC before committing */
> sd->state = sd_programming_state;
> @@ -2716,7 +2723,7 @@ static void sd_write_byte(SDState *sd, uint8_t value)
> break;
>
> case 26: /* CMD26: PROGRAM_CID */
> - if (sd_generic_write_byte(sd, value)) {
> + if (sd_generic_write_byte(sd, value[0])) {
> /* TODO: Check CRC before committing */
> sd->state = sd_programming_state;
> for (i = 0; i < sizeof(sd->cid); i ++)
> @@ -2734,7 +2741,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_byte(sd, value[0])) {
> /* TODO: Check CRC before committing */
> sd->state = sd_programming_state;
> for (i = 0; i < sizeof(sd->csd); i ++)
> @@ -2757,7 +2764,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_byte(sd, value[0])) {
> /* TODO: Check CRC before committing */
> sd->state = sd_programming_state;
> sd_lock_command(sd);
> @@ -2767,36 +2774,47 @@ 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_byte(sd, value[0]);
> break;
>
> default:
> g_assert_not_reached();
> }
> +
> + return length;
> }
>
> -static uint8_t sd_read_byte(SDState *sd)
> +static size_t sd_read_data(SDState *sd, void *buf, size_t length)
> {
> /* TODO: Append CRCs */
> const uint8_t dummy_byte = 0x00;
> unsigned int partition_access;
> - uint8_t ret;
> uint32_t io_len;
> + uint8_t *value = buf;
>
> if (!sd->blk || !blk_is_inserted(sd->blk)) {
> - return dummy_byte;
> + memset(buf, dummy_byte, length);
> + return length;
> }
>
> if (sd->state != sd_sendingdata_state) {
> qemu_log_mask(LOG_GUEST_ERROR,
> "%s: not in Sending-Data state\n", __func__);
> - return dummy_byte;
> + memset(buf, dummy_byte, length);
> + return length;
> }
>
> if (sd->card_status & (ADDRESS_ERROR | WP_VIOLATION)) {
> - return dummy_byte;
> + memset(buf, dummy_byte, length);
> + return length;
> }
>
> + /*
> + * We will only read one byte at a time. We will be called again with the
> + * remaining buffer.
> + */
> + length = 1;
> +
> io_len = sd_blk_len(sd);
>
> trace_sdcard_read_data(sd->proto->name,
and I suppose we also need to update this trace event to use
qemu_hexdump_line.
The trace events changes can be done as a cleanup patch on top,
so:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 2/6] hw/sd/sd: Allow multi-byte read/write for generic paths
2026-02-04 13:22 [PATCH v3 0/6] hw/sd: Improve performance of read/write/erase Christian Speich via qemu development
2026-02-04 13:22 ` [PATCH v3 1/6] hw/sd: Switch read/write primitive to buf+len Christian Speich via qemu development
@ 2026-02-04 13:22 ` Christian Speich via qemu development
2026-04-14 13:38 ` Philippe Mathieu-Daudé
2026-02-04 13:22 ` [PATCH v3 3/6] hw/sd/sd: Use multi-byte/block writes for block path Christian Speich
` (4 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Christian Speich via qemu development @ 2026-02-04 13:22 UTC (permalink / raw)
To: qemu-devel
Cc: Philippe Mathieu-Daudé, Bin Meng, qemu-block,
Christian Speich
Paths that use sd_generic_write/read_data can now write/read multiple
bytes with one call.
Signed-off-by: Christian Speich <c.speich@avm.de>
---
hw/sd/sd.c | 62 +++++++++++++++++++++++++++++++++++---------------------------
1 file changed, 35 insertions(+), 27 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 135113add29b5ee3cb11d9d535355eba8f6cb3f7..2c81776df316feda75f97e15cc9bbd1538f1a21c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1609,7 +1609,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_data() calls */
static sd_rsp_type_t sd_cmd_to_receivingdata(SDState *sd, SDRequest req,
uint64_t start, size_t size)
{
@@ -1624,7 +1624,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)
@@ -2614,11 +2614,15 @@ 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 *len)
{
- sd->data[sd->data_offset] = value;
+ size_t to_write = MIN(sd->data_size - sd->data_offset, *len);
- if (++sd->data_offset >= sd->data_size) {
+ memcpy(sd->data, buf, to_write);
+ sd->data_offset += to_write;
+ *len = to_write;
+
+ if (sd->data_offset >= sd->data_size) {
sd->state = sd_transfer_state;
return true;
}
@@ -2626,11 +2630,15 @@ static bool sd_generic_write_byte(SDState *sd, uint8_t value)
}
/* 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 *len)
{
- *value = sd->data[sd->data_offset];
+ size_t to_read = MIN(sd->data_size - sd->data_offset, *len);
+
+ memcpy(buf, sd->data, to_read);
+ sd->data_offset += to_read;
+ *len = to_read;
- if (++sd->data_offset >= sd->data_size) {
+ if (sd->data_offset >= sd->data_size) {
sd->state = sd_transfer_state;
return true;
}
@@ -2657,18 +2665,12 @@ static size_t sd_write_data(SDState *sd, const void *buf, size_t length)
if (sd->card_status & (ADDRESS_ERROR | WP_VIOLATION))
return length;
- /*
- * Only read one byte at a time. We will be called again with the
- * remaining.
- */
- length = 1;
-
trace_sdcard_write_data(sd->proto->name,
sd->last_cmd_name,
sd->current_cmd, sd->data_offset, value[0]);
switch (sd->current_cmd) {
case 24: /* CMD24: WRITE_SINGLE_BLOCK */
- if (sd_generic_write_byte(sd, value[0])) {
+ 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);
@@ -2680,6 +2682,12 @@ static size_t sd_write_data(SDState *sd, const void *buf, size_t length)
break;
case 25: /* CMD25: WRITE_MULTIPLE_BLOCK */
+ /*
+ * Only read one byte at a time. We will be called again with the
+ * remaining.
+ */
+ length = 1;
+
if (sd->data_offset == 0) {
/* Start of the block - let's check the address is valid */
if (!address_in_range(sd, "WRITE_MULTIPLE_BLOCK",
@@ -2723,7 +2731,7 @@ static size_t sd_write_data(SDState *sd, const void *buf, size_t length)
break;
case 26: /* CMD26: PROGRAM_CID */
- if (sd_generic_write_byte(sd, value[0])) {
+ 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 ++)
@@ -2741,7 +2749,7 @@ static size_t sd_write_data(SDState *sd, const void *buf, size_t length)
break;
case 27: /* CMD27: PROGRAM_CSD */
- if (sd_generic_write_byte(sd, value[0])) {
+ 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 ++)
@@ -2764,7 +2772,7 @@ static size_t sd_write_data(SDState *sd, const void *buf, size_t length)
break;
case 42: /* CMD42: LOCK_UNLOCK */
- if (sd_generic_write_byte(sd, value[0])) {
+ if (sd_generic_write_data(sd, buf, &length)) {
/* TODO: Check CRC before committing */
sd->state = sd_programming_state;
sd_lock_command(sd);
@@ -2774,7 +2782,7 @@ static size_t sd_write_data(SDState *sd, const void *buf, size_t length)
break;
case 56: /* CMD56: GEN_CMD */
- sd_generic_write_byte(sd, value[0]);
+ sd_generic_write_data(sd, buf, &length);
break;
default:
@@ -2809,12 +2817,6 @@ static size_t sd_read_data(SDState *sd, void *buf, size_t length)
return length;
}
- /*
- * We will only read one byte at a time. We will be called again with the
- * remaining buffer.
- */
- length = 1;
-
io_len = sd_blk_len(sd);
trace_sdcard_read_data(sd->proto->name,
@@ -2832,10 +2834,16 @@ static size_t sd_read_data(SDState *sd, void *buf, size_t length)
case 30: /* CMD30: SEND_WRITE_PROT */
case 51: /* ACMD51: SEND_SCR */
case 56: /* CMD56: GEN_CMD */
- sd_generic_read_byte(sd, value);
+ sd_generic_read_data(sd, buf, &length);
break;
case 18: /* CMD18: READ_MULTIPLE_BLOCK */
+ /*
+ * We will only read one byte at a time. We will be called again with
+ * the remaining buffer.
+ */
+ length = 1;
+
if (sd->data_offset == 0) {
if (!address_in_range(sd, "READ_MULTIPLE_BLOCK",
sd->data_start, io_len)) {
@@ -2868,7 +2876,7 @@ static size_t sd_read_data(SDState *sd, void *buf, size_t length)
default:
qemu_log_mask(LOG_GUEST_ERROR, "%s: DAT read illegal for command %s\n",
__func__, sd->last_cmd_name);
- *value = dummy_byte;
+ memset(buf, dummy_byte, length);
}
return length;
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v3 3/6] hw/sd/sd: Use multi-byte/block writes for block path
2026-02-04 13:22 [PATCH v3 0/6] hw/sd: Improve performance of read/write/erase Christian Speich via qemu development
2026-02-04 13:22 ` [PATCH v3 1/6] hw/sd: Switch read/write primitive to buf+len Christian Speich via qemu development
2026-02-04 13:22 ` [PATCH v3 2/6] hw/sd/sd: Allow multi-byte read/write for generic paths Christian Speich via qemu development
@ 2026-02-04 13:22 ` Christian Speich
2026-04-14 13:45 ` Philippe Mathieu-Daudé
2026-02-04 13:22 ` [PATCH v3 4/6] hw/sd/sdhci: Don't use bounce buffer for ADMA Christian Speich
` (3 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Christian Speich @ 2026-02-04 13:22 UTC (permalink / raw)
To: qemu-devel
Cc: Philippe Mathieu-Daudé, Bin Meng, qemu-block,
Christian Speich
When writing/reading blocks via WRITE/READ_MULTIPLE_BLOCK we try to
directly pass this request down to the block layer. This can only be done
for properly sized and aligned accesses, other access still use a bounce
buffer but still benefit from copying as much data in one memcpy as
possible.
RPMB is limited to the slow path using a bounce buffer.
Signed-off-by: Christian Speich <c.speich@avm.de>
---
hw/sd/sd.c | 220 ++++++++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 168 insertions(+), 52 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 2c81776df316feda75f97e15cc9bbd1538f1a21c..c9cfd3620b563fbe7520cfe361e14b57cd8f7472 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1112,24 +1112,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 bool rpmb_calc_hmac(SDState *sd, const RPMBDataFrame *frame,
unsigned int num_blocks, uint8_t *mac)
{
@@ -2682,44 +2694,84 @@ static size_t sd_write_data(SDState *sd, const void *buf, size_t length)
break;
case 25: /* CMD25: WRITE_MULTIPLE_BLOCK */
- /*
- * Only read one byte at a time. We will be called again with the
- * remaining.
- */
- length = 1;
-
- 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 (!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;
+
+ /* We've read past the end, return a dummy write. */
+ if (length == 0) {
+ return 1;
}
- if (sd->size <= SDSC_MAX_CAPACITY) {
- if (sd_wp_addr(sd, sd->data_start)) {
+ }
+
+ 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[0];
- if (sd->data_offset >= sd->blk_len) {
- /* TODO: Check CRC before committing */
- sd->state = sd_programming_state;
- partition_access = sd->ext_csd[EXT_CSD_PART_CONFIG]
- & EXT_CSD_PART_CONFIG_ACC_MASK;
- if (partition_access == EXT_CSD_PART_CONFIG_ACC_RPMB) {
- emmc_rpmb_blk_write(sd, sd->data_start, sd->data_offset);
- } else {
- sd_blk_write(sd, sd->data_start, sd->data_offset);
+
+ partition_access = sd->ext_csd[EXT_CSD_PART_CONFIG]
+ & EXT_CSD_PART_CONFIG_ACC_MASK;
+
+ /* Partial write or RPMB (single block only for now) */
+ if (sd->data_offset > 0
+ || partition_access == EXT_CSD_PART_CONFIG_ACC_RPMB) {
+ length = MIN(sd->blk_len - sd->data_offset, length);
+
+ memcpy(sd->data + sd->data_offset, buf, length);
+ sd->data_offset += length;
+
+ if (sd->data_offset >= sd->blk_len) {
+ sd->state = sd_programming_state;
+ if (partition_access == EXT_CSD_PART_CONFIG_ACC_RPMB) {
+ emmc_rpmb_blk_write(sd, sd->data_start, sd->data_offset);
+ } else {
+ sd_blk_write(sd, sd->data_start, sd->data_offset);
+ }
+ 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;
}
- sd->blk_written++;
- sd->data_start += sd->blk_len;
- sd->data_offset = 0;
+ }
+ /* Try to write multiple of block sizes */
+ else if (length >= sd->blk_len) {
+ length = QEMU_ALIGN_DOWN(length, sd->blk_len);
+
+ sd->state = sd_programming_state;
+ sd_blk_write_direct(sd, buf, sd->data_start, length);
+ sd->blk_written += length / sd->blk_len;
+ sd->data_start += length;
sd->csd[14] |= 0x40;
- /* Bzzzzzzztt .... Operation complete. */
if (sd->multi_blk_cnt != 0) {
- if (--sd->multi_blk_cnt == 0) {
+ sd->multi_blk_cnt -= length / sd->blk_len;
+
+ if (sd->multi_blk_cnt == 0) {
/* Stop! */
sd->state = sd_transfer_state;
break;
@@ -2728,6 +2780,12 @@ static size_t sd_write_data(SDState *sd, const void *buf, size_t length)
sd->state = sd_receivingdata_state;
}
+ /* Partial write */
+ else if (length > 0) {
+ memcpy(sd->data, buf, length);
+ sd->data_offset = length;
+ }
+
break;
case 26: /* CMD26: PROGRAM_CID */
@@ -2798,7 +2856,6 @@ static size_t sd_read_data(SDState *sd, void *buf, size_t length)
const uint8_t dummy_byte = 0x00;
unsigned int partition_access;
uint32_t io_len;
- uint8_t *value = buf;
if (!sd->blk || !blk_is_inserted(sd->blk)) {
memset(buf, dummy_byte, length);
@@ -2838,36 +2895,95 @@ static size_t sd_read_data(SDState *sd, void *buf, size_t length)
break;
case 18: /* CMD18: READ_MULTIPLE_BLOCK */
+ 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 read past the end, return a dummy read. */
+ if (length == 0) {
+ memset(buf, dummy_byte, 1);
+ return 1;
+ }
+ }
+
+ partition_access = sd->ext_csd[EXT_CSD_PART_CONFIG]
+ & EXT_CSD_PART_CONFIG_ACC_MASK;
+
+ /* We have a partially read block. */
+ if (sd->data_offset > 0) {
+ length = MIN(sd->data_size - sd->data_offset, length);
+
+ memcpy(buf, sd->data + sd->data_offset, length);
+
+ sd->data_offset += length;
+
+ /* 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;
+ }
+ }
+ }
+ }
/*
- * We will only read one byte at a time. We will be called again with
- * the remaining buffer.
+ * Try to read multiples of the block size directly bypassing the local
+ * bounce buffer.
+ * Not for RPMB.
*/
- length = 1;
+ else if (length >= io_len
+ && partition_access != EXT_CSD_PART_CONFIG_ACC_RPMB) {
+ length = QEMU_ALIGN_DOWN(length, io_len);
- if (sd->data_offset == 0) {
- if (!address_in_range(sd, "READ_MULTIPLE_BLOCK",
- sd->data_start, io_len)) {
- return dummy_byte;
+ /* For limited reads, only read the requested block count. */
+ if (sd->multi_blk_cnt != 0) {
+ length = MIN(length, sd->multi_blk_cnt * io_len);
}
- partition_access = sd->ext_csd[EXT_CSD_PART_CONFIG]
- & EXT_CSD_PART_CONFIG_ACC_MASK;
+
+ sd_blk_read_direct(sd, buf, sd->data_start,
+ length);
+
+ sd->data_start += length;
+
+ if (sd->multi_blk_cnt != 0) {
+ sd->multi_blk_cnt -= length / io_len;
+
+ if (sd->multi_blk_cnt == 0) {
+ sd->state = sd_transfer_state;
+ }
+ }
+ }
+ /* Read partial at the end or sinlge-block RPMB */
+ else if (length > 0) {
+ length = MIN(length, io_len);
+
+ /* Fill the buffer */
if (partition_access == EXT_CSD_PART_CONFIG_ACC_RPMB) {
emmc_rpmb_blk_read(sd, sd->data_start, io_len);
} else {
sd_blk_read(sd, sd->data_start, io_len);
}
- }
- *value = sd->data[sd->data_offset++];
- if (sd->data_offset >= io_len) {
- sd->data_start += io_len;
- sd->data_offset = 0;
+ memcpy(buf, sd->data, length);
- if (sd->multi_blk_cnt != 0) {
- if (--sd->multi_blk_cnt == 0) {
- /* Stop! */
- sd->state = sd_transfer_state;
- break;
+ sd->data_size = io_len;
+ sd->data_offset = length;
+
+ if (sd->data_offset >= io_len) {
+ sd->data_start += io_len;
+ sd->data_offset = 0;
+
+ if (sd->multi_blk_cnt != 0) {
+ if (--sd->multi_blk_cnt == 0) {
+ /* Stop! */
+ sd->state = sd_transfer_state;
+ break;
+ }
}
}
}
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 3/6] hw/sd/sd: Use multi-byte/block writes for block path
2026-02-04 13:22 ` [PATCH v3 3/6] hw/sd/sd: Use multi-byte/block writes for block path Christian Speich
@ 2026-04-14 13:45 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-04-14 13:45 UTC (permalink / raw)
To: Christian Speich, qemu-devel; +Cc: Bin Meng, qemu-block
On 4/2/26 14:22, Christian Speich wrote:
> When writing/reading blocks via WRITE/READ_MULTIPLE_BLOCK we try to
> directly pass this request down to the block layer. This can only be done
> for properly sized and aligned accesses, other access still use a bounce
> buffer but still benefit from copying as much data in one memcpy as
> possible.
>
> RPMB is limited to the slow path using a bounce buffer.
>
> Signed-off-by: Christian Speich <c.speich@avm.de>
> ---
> hw/sd/sd.c | 220 ++++++++++++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 168 insertions(+), 52 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 2c81776df316feda75f97e15cc9bbd1538f1a21c..c9cfd3620b563fbe7520cfe361e14b57cd8f7472 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1112,24 +1112,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,
(QEMU coding style: "void *buf")
> + 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");
I'd rename sd_blk_read -> sd_blk_read_bounce_buffer (or similar) and
use sd_blk_read for the one you named sd_blk_read_direct. (Ditto write).
Patch LGTM otherwise!
> }
> }
>
> -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");
> }
> }
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 4/6] hw/sd/sdhci: Don't use bounce buffer for ADMA
2026-02-04 13:22 [PATCH v3 0/6] hw/sd: Improve performance of read/write/erase Christian Speich via qemu development
` (2 preceding siblings ...)
2026-02-04 13:22 ` [PATCH v3 3/6] hw/sd/sd: Use multi-byte/block writes for block path Christian Speich
@ 2026-02-04 13:22 ` Christian Speich
2026-04-14 13:51 ` Philippe Mathieu-Daudé
2026-02-04 13:22 ` [PATCH v3 5/6] hw/sd/sdcard: Add erase-blocks-as-zero option Christian Speich
` (2 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Christian Speich @ 2026-02-04 13:22 UTC (permalink / raw)
To: qemu-devel
Cc: Philippe Mathieu-Daudé, Bin Meng, qemu-block,
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 c86dfa281f4b0218bf6dda7a38d46abfc9638450..6e07711478cb6ca046a7d371a82e2c682ebbda00 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -775,7 +775,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 = {};
@@ -817,66 +817,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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 4/6] hw/sd/sdhci: Don't use bounce buffer for ADMA
2026-02-04 13:22 ` [PATCH v3 4/6] hw/sd/sdhci: Don't use bounce buffer for ADMA Christian Speich
@ 2026-04-14 13:51 ` Philippe Mathieu-Daudé
2026-04-16 7:52 ` Christian Speich
0 siblings, 1 reply; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-04-14 13:51 UTC (permalink / raw)
To: Christian Speich, qemu-devel; +Cc: Bin Meng, qemu-block
On 4/2/26 14:22, Christian Speich wrote:
> 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 c86dfa281f4b0218bf6dda7a38d46abfc9638450..6e07711478cb6ca046a7d371a82e2c682ebbda00 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -775,7 +775,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 = {};
> @@ -817,66 +817,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);
We know the descriptor is a consecutive range. Couldn't we map/unmap
outside of the while() loop? (Ditto write path).
> +
> + 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 {
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 4/6] hw/sd/sdhci: Don't use bounce buffer for ADMA
2026-04-14 13:51 ` Philippe Mathieu-Daudé
@ 2026-04-16 7:52 ` Christian Speich
2026-04-16 8:27 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 20+ messages in thread
From: Christian Speich @ 2026-04-16 7:52 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel, Bin Meng, qemu-block
On Tue, Apr 14, 2026 at 03:51:24PM +0200, Philippe Mathieu-Daudé wrote:
> On 4/2/26 14:22, Christian Speich wrote:
> > 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 c86dfa281f4b0218bf6dda7a38d46abfc9638450..6e07711478cb6ca046a7d371a82e2c682ebbda00 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -775,7 +775,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 = {};
> > @@ -817,66 +817,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);
>
> We know the descriptor is a consecutive range. Couldn't we map/unmap
> outside of the while() loop? (Ditto write path).
The loop is there to handle dma_memory_map mapping less that we've requested. I'm not
really sure if there are any guranteed cases where it always maps the requested length
fully (at least there are non in the dma_memory_map doc string).
Iff this is guranteed to always map the requested length, we could remove the whie()
loop completly.
Kind Regards,
Christian
>
> > +
> > + 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 {
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 4/6] hw/sd/sdhci: Don't use bounce buffer for ADMA
2026-04-16 7:52 ` Christian Speich
@ 2026-04-16 8:27 ` Philippe Mathieu-Daudé
2026-04-16 13:51 ` Stefan Hajnoczi
0 siblings, 1 reply; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-04-16 8:27 UTC (permalink / raw)
To: Christian Speich, Stefan Hajnoczi, Paolo Bonzini
Cc: qemu-devel, Bin Meng, qemu-block
Cc'ing Paolo & Stefan.
On 16/4/26 09:52, Christian Speich wrote:
> On Tue, Apr 14, 2026 at 03:51:24PM +0200, Philippe Mathieu-Daudé wrote:
>> On 4/2/26 14:22, Christian Speich wrote:
>>> 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 c86dfa281f4b0218bf6dda7a38d46abfc9638450..6e07711478cb6ca046a7d371a82e2c682ebbda00 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -775,7 +775,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 = {};
>>> @@ -817,66 +817,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);
>>
>> We know the descriptor is a consecutive range. Couldn't we map/unmap
>> outside of the while() loop? (Ditto write path).
>
> The loop is there to handle dma_memory_map mapping less that we've requested. I'm not
> really sure if there are any guranteed cases where it always maps the requested length
> fully (at least there are non in the dma_memory_map doc string).
>
> Iff this is guranteed to always map the requested length, we could remove the whie()
> loop completly.
>
> Kind Regards,
> Christian
>
>>
>>> +
>>> + 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 {
>>
>>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 4/6] hw/sd/sdhci: Don't use bounce buffer for ADMA
2026-04-16 8:27 ` Philippe Mathieu-Daudé
@ 2026-04-16 13:51 ` Stefan Hajnoczi
0 siblings, 0 replies; 20+ messages in thread
From: Stefan Hajnoczi @ 2026-04-16 13:51 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Christian Speich, Paolo Bonzini, qemu-devel, Bin Meng, qemu-block
[-- Attachment #1: Type: text/plain, Size: 5901 bytes --]
On Thu, Apr 16, 2026 at 10:27:22AM +0200, Philippe Mathieu-Daudé wrote:
> Cc'ing Paolo & Stefan.
>
> On 16/4/26 09:52, Christian Speich wrote:
> > On Tue, Apr 14, 2026 at 03:51:24PM +0200, Philippe Mathieu-Daudé wrote:
> > > On 4/2/26 14:22, Christian Speich wrote:
> > > > 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 c86dfa281f4b0218bf6dda7a38d46abfc9638450..6e07711478cb6ca046a7d371a82e2c682ebbda00 100644
> > > > --- a/hw/sd/sdhci.c
> > > > +++ b/hw/sd/sdhci.c
> > > > @@ -775,7 +775,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 = {};
> > > > @@ -817,66 +817,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);
> > >
> > > We know the descriptor is a consecutive range. Couldn't we map/unmap
> > > outside of the while() loop? (Ditto write path).
> >
> > The loop is there to handle dma_memory_map mapping less that we've requested. I'm not
> > really sure if there are any guranteed cases where it always maps the requested length
> > fully (at least there are non in the dma_memory_map doc string).
> >
> > Iff this is guranteed to always map the requested length, we could remove the whie()
> > loop completly.
I think this loop is required because:
- A consecutive DMA address range can span multiple host address ranges.
This can happen when there is more than 1 RAM block backing the DMA
address space.
- When the DMA address range includes non-RAM addresses (e.g. MMIO
regions), then it is mapped via a bounce buffer that has a size limit
and could require multiple iterations.
> >
> > Kind Regards,
> > Christian
> >
> > >
> > > > +
> > > > + 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 {
> > >
> > >
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 5/6] hw/sd/sdcard: Add erase-blocks-as-zero option.
2026-02-04 13:22 [PATCH v3 0/6] hw/sd: Improve performance of read/write/erase Christian Speich via qemu development
` (3 preceding siblings ...)
2026-02-04 13:22 ` [PATCH v3 4/6] hw/sd/sdhci: Don't use bounce buffer for ADMA Christian Speich
@ 2026-02-04 13:22 ` Christian Speich
2026-04-14 15:02 ` Philippe Mathieu-Daudé
2026-02-04 13:22 ` [PATCH v3 6/6] hw/sd/sdcard: Optimize erase blocks as zero Christian Speich
2026-03-19 8:37 ` [PATCH v3 0/6] hw/sd: Improve performance of read/write/erase Christian Speich
6 siblings, 1 reply; 20+ messages in thread
From: Christian Speich @ 2026-02-04 13:22 UTC (permalink / raw)
To: qemu-devel
Cc: Philippe Mathieu-Daudé, Bin Meng, qemu-block,
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.
This patch does two things.
First it fixes the reporting of DATA_STAT_AFTER_ERASE in SCR to
correctly reflect the content of erased blocks. We also increase the
Product Revision (REV in CID) to indicate to the guest that
DATA_STAT_AFTER_ERASE is now reliable.
Secondly, we introduce a erase-blocks-as-zero option, which allows the
user to choose if erased blocks should contain 0xFF or 0x00. The default
is still 0xFF to remain compatible with current users.
Signed-off-by: Christian Speich <c.speich@avm.de>
---
hw/sd/sd.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index c9cfd3620b563fbe7520cfe361e14b57cd8f7472..5ddaa08d77b6b7606005fb59f42f107d254d6e5c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -162,6 +162,7 @@ struct SDState {
/* Static properties */
uint8_t spec_version;
+ bool erase_blocks_as_zero;
uint64_t boot_part_size;
uint64_t rpmb_part_size;
BlockBackend *blk;
@@ -439,6 +440,9 @@ static void sd_set_scr(SDState *sd)
sd->scr[0] |= 2; /* Spec Version 2.00 or Version 3.0X */
sd->scr[1] = (2 << 4) /* SDSC Card (Security Version 1.01) */
| 0b0101; /* 1-bit or 4-bit width bus modes */
+ if (!sd->erase_blocks_as_zero) {
+ sd->scr[1] |= (1 << 7); /* DATA_STAT_AFTER_ERASE: Erase produces 0xFF */
+ }
sd->scr[2] = 0x00; /* Extended Security is not supported. */
if (sd->spec_version >= SD_PHY_SPECv3_01_VERS) {
sd->scr[2] |= 1 << 7; /* Spec Version 3.0X */
@@ -456,7 +460,7 @@ static void sd_set_scr(SDState *sd)
#define MID 0xaa
#define OID "XY"
#define PNM "QEMU!"
-#define PRV 0x01
+#define PRV 0x02
#define MDT_YR 2006
#define MDT_MON 2
@@ -1363,7 +1367,12 @@ static void sd_erase(SDState *sd)
sd->erase_end = INVALID_ADDRESS;
sd->csd[14] |= 0x40;
- memset(sd->data, 0xff, erase_len);
+ if (sd->erase_blocks_as_zero) {
+ memset(sd->data, 0x0, erase_len);
+ } else {
+ memset(sd->data, 0xFF, erase_len);
+ }
+
for (erase_addr = erase_start; erase_addr <= erase_end;
erase_addr += erase_len) {
if (sdsc) {
@@ -3287,6 +3296,8 @@ static void emmc_realize(DeviceState *dev, Error **errp)
static const Property sdmmc_common_properties[] = {
DEFINE_PROP_DRIVE("drive", SDState, blk),
+ DEFINE_PROP_BOOL("erase-blocks-as-zero", SDState, erase_blocks_as_zero,
+ false),
};
static const Property sd_properties[] = {
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 5/6] hw/sd/sdcard: Add erase-blocks-as-zero option.
2026-02-04 13:22 ` [PATCH v3 5/6] hw/sd/sdcard: Add erase-blocks-as-zero option Christian Speich
@ 2026-04-14 15:02 ` Philippe Mathieu-Daudé
2026-04-16 8:02 ` Christian Speich
0 siblings, 1 reply; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-04-14 15:02 UTC (permalink / raw)
To: Christian Speich, qemu-devel; +Cc: Bin Meng, qemu-block
On 4/2/26 14:22, 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.
>
> This patch does two things.
>
> First it fixes the reporting of DATA_STAT_AFTER_ERASE in SCR to
> correctly reflect the content of erased blocks. We also increase the
> Product Revision (REV in CID) to indicate to the guest that
> DATA_STAT_AFTER_ERASE is now reliable.
>
> Secondly, we introduce a erase-blocks-as-zero option, which allows the
> user to choose if erased blocks should contain 0xFF or 0x00. The default
> is still 0xFF to remain compatible with current users.
>
> Signed-off-by: Christian Speich <c.speich@avm.de>
> ---
> hw/sd/sd.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index c9cfd3620b563fbe7520cfe361e14b57cd8f7472..5ddaa08d77b6b7606005fb59f42f107d254d6e5c 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -162,6 +162,7 @@ struct SDState {
> /* Static properties */
>
> uint8_t spec_version;
> + bool erase_blocks_as_zero;
> uint64_t boot_part_size;
> uint64_t rpmb_part_size;
> BlockBackend *blk;
> @@ -439,6 +440,9 @@ static void sd_set_scr(SDState *sd)
> sd->scr[0] |= 2; /* Spec Version 2.00 or Version 3.0X */
> sd->scr[1] = (2 << 4) /* SDSC Card (Security Version 1.01) */
> | 0b0101; /* 1-bit or 4-bit width bus modes */
> + if (!sd->erase_blocks_as_zero) {
> + sd->scr[1] |= (1 << 7); /* DATA_STAT_AFTER_ERASE: Erase produces 0xFF */
> + }
> sd->scr[2] = 0x00; /* Extended Security is not supported. */
> if (sd->spec_version >= SD_PHY_SPECv3_01_VERS) {
> sd->scr[2] |= 1 << 7; /* Spec Version 3.0X */
> @@ -456,7 +460,7 @@ static void sd_set_scr(SDState *sd)
> #define MID 0xaa
> #define OID "XY"
> #define PNM "QEMU!"
> -#define PRV 0x01
> +#define PRV 0x02
> #define MDT_YR 2006
> #define MDT_MON 2
>
> @@ -1363,7 +1367,12 @@ static void sd_erase(SDState *sd)
> sd->erase_end = INVALID_ADDRESS;
> sd->csd[14] |= 0x40;
>
> - memset(sd->data, 0xff, erase_len);
> + if (sd->erase_blocks_as_zero) {
> + memset(sd->data, 0x0, erase_len);
> + } else {
> + memset(sd->data, 0xFF, erase_len);
> + }
> +
> for (erase_addr = erase_start; erase_addr <= erase_end;
> erase_addr += erase_len) {
> if (sdsc) {
> @@ -3287,6 +3296,8 @@ static void emmc_realize(DeviceState *dev, Error **errp)
>
> static const Property sdmmc_common_properties[] = {
> DEFINE_PROP_DRIVE("drive", SDState, blk),
> + DEFINE_PROP_BOOL("erase-blocks-as-zero", SDState, erase_blocks_as_zero,
> + false),
> };
Keep it enabled for previous versions adding in hw/core/machine.c:
GlobalProperty hw_compat_11_0[] = {
{ "sdmmc-common", "erase-blocks-as-zero", "true" },
};
(Although I wonder about the PRV bit).
Otherwise LGTM.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 5/6] hw/sd/sdcard: Add erase-blocks-as-zero option.
2026-04-14 15:02 ` Philippe Mathieu-Daudé
@ 2026-04-16 8:02 ` Christian Speich
2026-04-16 8:26 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 20+ messages in thread
From: Christian Speich @ 2026-04-16 8:02 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel, Bin Meng, qemu-block
On Tue, Apr 14, 2026 at 05:02:34PM +0200, Philippe Mathieu-Daudé wrote:
> On 4/2/26 14:22, 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.
> >
> > This patch does two things.
> >
> > First it fixes the reporting of DATA_STAT_AFTER_ERASE in SCR to
> > correctly reflect the content of erased blocks. We also increase the
> > Product Revision (REV in CID) to indicate to the guest that
> > DATA_STAT_AFTER_ERASE is now reliable.
> >
> > Secondly, we introduce a erase-blocks-as-zero option, which allows the
> > user to choose if erased blocks should contain 0xFF or 0x00. The default
> > is still 0xFF to remain compatible with current users.
> >
> > Signed-off-by: Christian Speich <c.speich@avm.de>
> > ---
> > hw/sd/sd.c | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index c9cfd3620b563fbe7520cfe361e14b57cd8f7472..5ddaa08d77b6b7606005fb59f42f107d254d6e5c 100644
> > --- a/hw/sd/sd.c
> > +++ b/hw/sd/sd.c
> > @@ -162,6 +162,7 @@ struct SDState {
> > /* Static properties */
> > uint8_t spec_version;
> > + bool erase_blocks_as_zero;
> > uint64_t boot_part_size;
> > uint64_t rpmb_part_size;
> > BlockBackend *blk;
> > @@ -439,6 +440,9 @@ static void sd_set_scr(SDState *sd)
> > sd->scr[0] |= 2; /* Spec Version 2.00 or Version 3.0X */
> > sd->scr[1] = (2 << 4) /* SDSC Card (Security Version 1.01) */
> > | 0b0101; /* 1-bit or 4-bit width bus modes */
> > + if (!sd->erase_blocks_as_zero) {
> > + sd->scr[1] |= (1 << 7); /* DATA_STAT_AFTER_ERASE: Erase produces 0xFF */
> > + }
> > sd->scr[2] = 0x00; /* Extended Security is not supported. */
> > if (sd->spec_version >= SD_PHY_SPECv3_01_VERS) {
> > sd->scr[2] |= 1 << 7; /* Spec Version 3.0X */
> > @@ -456,7 +460,7 @@ static void sd_set_scr(SDState *sd)
> > #define MID 0xaa
> > #define OID "XY"
> > #define PNM "QEMU!"
> > -#define PRV 0x01
> > +#define PRV 0x02
> > #define MDT_YR 2006
> > #define MDT_MON 2
> > @@ -1363,7 +1367,12 @@ static void sd_erase(SDState *sd)
> > sd->erase_end = INVALID_ADDRESS;
> > sd->csd[14] |= 0x40;
> > - memset(sd->data, 0xff, erase_len);
> > + if (sd->erase_blocks_as_zero) {
> > + memset(sd->data, 0x0, erase_len);
> > + } else {
> > + memset(sd->data, 0xFF, erase_len);
> > + }
> > +
> > for (erase_addr = erase_start; erase_addr <= erase_end;
> > erase_addr += erase_len) {
> > if (sdsc) {
> > @@ -3287,6 +3296,8 @@ static void emmc_realize(DeviceState *dev, Error **errp)
> > static const Property sdmmc_common_properties[] = {
> > DEFINE_PROP_DRIVE("drive", SDState, blk),
> > + DEFINE_PROP_BOOL("erase-blocks-as-zero", SDState, erase_blocks_as_zero,
> > + false),
> > };
>
> Keep it enabled for previous versions adding in hw/core/machine.c:
>
> GlobalProperty hw_compat_11_0[] = {
> { "sdmmc-common", "erase-blocks-as-zero", "true" },
> };
>
> (Although I wonder about the PRV bit).
If I understand the hw_compat correctly, this would mean sdcards would switch from
`erase as 0xFF` (old behaviour) to `erase as 0x00` when using the migration path?
I don't think this is a good idea as an guest might depend on the erase behaviour (e.g.
looking if an block is erased by comparing a "well-known" byte in the block for the
eraseure value. Changing this unexpectantly might confuse the guest to think used blocks
are empty and vice versa).
The other way around would be to change the default to true, and use compat to keep it
at false when migrating. This would make more sense, I think?
Kind Regards,
Christian
>
> Otherwise LGTM.
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 5/6] hw/sd/sdcard: Add erase-blocks-as-zero option.
2026-04-16 8:02 ` Christian Speich
@ 2026-04-16 8:26 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-04-16 8:26 UTC (permalink / raw)
To: Christian Speich; +Cc: qemu-devel, Bin Meng, qemu-block
On 16/4/26 10:02, Christian Speich wrote:
> On Tue, Apr 14, 2026 at 05:02:34PM +0200, Philippe Mathieu-Daudé wrote:
>> On 4/2/26 14:22, 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.
>>>
>>> This patch does two things.
>>>
>>> First it fixes the reporting of DATA_STAT_AFTER_ERASE in SCR to
>>> correctly reflect the content of erased blocks. We also increase the
>>> Product Revision (REV in CID) to indicate to the guest that
>>> DATA_STAT_AFTER_ERASE is now reliable.
>>>
>>> Secondly, we introduce a erase-blocks-as-zero option, which allows the
>>> user to choose if erased blocks should contain 0xFF or 0x00. The default
>>> is still 0xFF to remain compatible with current users.
>>>
>>> Signed-off-by: Christian Speich <c.speich@avm.de>
>>> ---
>>> hw/sd/sd.c | 15 +++++++++++++--
>>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index c9cfd3620b563fbe7520cfe361e14b57cd8f7472..5ddaa08d77b6b7606005fb59f42f107d254d6e5c 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -162,6 +162,7 @@ struct SDState {
>>> /* Static properties */
>>> uint8_t spec_version;
>>> + bool erase_blocks_as_zero;
>>> uint64_t boot_part_size;
>>> uint64_t rpmb_part_size;
>>> BlockBackend *blk;
>>> @@ -439,6 +440,9 @@ static void sd_set_scr(SDState *sd)
>>> sd->scr[0] |= 2; /* Spec Version 2.00 or Version 3.0X */
>>> sd->scr[1] = (2 << 4) /* SDSC Card (Security Version 1.01) */
>>> | 0b0101; /* 1-bit or 4-bit width bus modes */
>>> + if (!sd->erase_blocks_as_zero) {
>>> + sd->scr[1] |= (1 << 7); /* DATA_STAT_AFTER_ERASE: Erase produces 0xFF */
>>> + }
>>> sd->scr[2] = 0x00; /* Extended Security is not supported. */
>>> if (sd->spec_version >= SD_PHY_SPECv3_01_VERS) {
>>> sd->scr[2] |= 1 << 7; /* Spec Version 3.0X */
>>> @@ -456,7 +460,7 @@ static void sd_set_scr(SDState *sd)
>>> #define MID 0xaa
>>> #define OID "XY"
>>> #define PNM "QEMU!"
>>> -#define PRV 0x01
>>> +#define PRV 0x02
>>> #define MDT_YR 2006
>>> #define MDT_MON 2
>>> @@ -1363,7 +1367,12 @@ static void sd_erase(SDState *sd)
>>> sd->erase_end = INVALID_ADDRESS;
>>> sd->csd[14] |= 0x40;
>>> - memset(sd->data, 0xff, erase_len);
>>> + if (sd->erase_blocks_as_zero) {
>>> + memset(sd->data, 0x0, erase_len);
>>> + } else {
>>> + memset(sd->data, 0xFF, erase_len);
>>> + }
>>> +
>>> for (erase_addr = erase_start; erase_addr <= erase_end;
>>> erase_addr += erase_len) {
>>> if (sdsc) {
>>> @@ -3287,6 +3296,8 @@ static void emmc_realize(DeviceState *dev, Error **errp)
>>> static const Property sdmmc_common_properties[] = {
>>> DEFINE_PROP_DRIVE("drive", SDState, blk),
>>> + DEFINE_PROP_BOOL("erase-blocks-as-zero", SDState, erase_blocks_as_zero,
>>> + false),
>>> };
>>
>> Keep it enabled for previous versions adding in hw/core/machine.c:
>>
>> GlobalProperty hw_compat_11_0[] = {
>> { "sdmmc-common", "erase-blocks-as-zero", "true" },
>> };
>>
>> (Although I wonder about the PRV bit).
>
> If I understand the hw_compat correctly, this would mean sdcards would switch from
> `erase as 0xFF` (old behaviour) to `erase as 0x00` when using the migration path?
>
> I don't think this is a good idea as an guest might depend on the erase behaviour (e.g.
> looking if an block is erased by comparing a "well-known" byte in the block for the
> eraseure value. Changing this unexpectantly might confuse the guest to think used blocks
> are empty and vice versa).
>
> The other way around would be to change the default to true, and use compat to keep it
> at false when migrating. This would make more sense, I think?
Oops yes this is what I meant, keep "erase as 0xff" old behavior for
up to v11.0, then from now on start using "erase as 0x00" by default
because optimizable, except if the user request otherwise. So:
DEFINE_PROP_BOOL("erase-blocks-as-zero", SDState,
erase_blocks_as_zero, true),
and:
GlobalProperty hw_compat_11_0[] = {
{ "sdmmc-common", "erase-blocks-as-zero", "false" },
};
WDYT?
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 6/6] hw/sd/sdcard: Optimize erase blocks as zero.
2026-02-04 13:22 [PATCH v3 0/6] hw/sd: Improve performance of read/write/erase Christian Speich via qemu development
` (4 preceding siblings ...)
2026-02-04 13:22 ` [PATCH v3 5/6] hw/sd/sdcard: Add erase-blocks-as-zero option Christian Speich
@ 2026-02-04 13:22 ` Christian Speich
2026-04-14 13:58 ` Philippe Mathieu-Daudé
2026-03-19 8:37 ` [PATCH v3 0/6] hw/sd: Improve performance of read/write/erase Christian Speich
6 siblings, 1 reply; 20+ messages in thread
From: Christian Speich @ 2026-02-04 13:22 UTC (permalink / raw)
To: qemu-devel
Cc: Philippe Mathieu-Daudé, Bin Meng, qemu-block,
Christian Speich
When erasing blocks as zero, we can use optimized block functions to
achieve this.
These allow us to request a large rage to be zeroed, possible optimizing
this operation and freeing disk space for sparsesly stored images.
This only is possible when erase-blocks-as-zero=true is used and can
provide a significant performance boost. The case where 0xFF is used
during erase is as slow as before.
Signed-off-by: Christian Speich <c.speich@avm.de>
---
hw/sd/sd.c | 44 ++++++++++++++++++++++++++++++++++----------
1 file changed, 34 insertions(+), 10 deletions(-)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 5ddaa08d77b6b7606005fb59f42f107d254d6e5c..6e3afedbf45d59c2c9b48505ed144a788bc76760 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1331,6 +1331,17 @@ exit:
trace_sdcard_rpmb_write_block(req, lduw_be_p(&sd->rpmb.result.result));
}
+/* Requires sd->buf to be filled with 0xFF */
+static void sd_erase_ff(SDState *sd, uint64_t addr, size_t len)
+{
+ int erase_len = 1 << HWBLOCK_SHIFT;
+ uint64_t erase_addr;
+
+ for (erase_addr = addr; erase_addr < addr + len; erase_addr += erase_len) {
+ sd_blk_write(sd, erase_addr, erase_len);
+ }
+}
+
static void sd_erase(SDState *sd)
{
uint64_t erase_start = sd->erase_start;
@@ -1338,7 +1349,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
@@ -1367,24 +1377,38 @@ static void sd_erase(SDState *sd)
sd->erase_end = INVALID_ADDRESS;
sd->csd[14] |= 0x40;
- if (sd->erase_blocks_as_zero) {
- memset(sd->data, 0x0, erase_len);
- } else {
- memset(sd->data, 0xFF, erase_len);
+ if (!sd->erase_blocks_as_zero) {
+ memset(sd->data, 0xFF, 1 << HWBLOCK_SHIFT);
}
- 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;
}
+
+ if (sd->erase_blocks_as_zero) {
+ blk_pwrite_zeroes(sd->blk, erase_addr + sd_part_offset(sd),
+ to_erase, 0);
+ } else {
+ sd_erase_ff(sd, erase_addr, to_erase);
+ }
+ }
+ } else {
+ if (sd->erase_blocks_as_zero) {
+ blk_pwrite_zeroes(sd->blk, erase_start + sd_part_offset(sd),
+ erase_end - erase_start, 0);
+ } else {
+ sd_erase_ff(sd, erase_start, erase_end - erase_start);
}
- sd_blk_write(sd, erase_addr, erase_len);
}
}
--
2.43.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 6/6] hw/sd/sdcard: Optimize erase blocks as zero.
2026-02-04 13:22 ` [PATCH v3 6/6] hw/sd/sdcard: Optimize erase blocks as zero Christian Speich
@ 2026-04-14 13:58 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 20+ messages in thread
From: Philippe Mathieu-Daudé @ 2026-04-14 13:58 UTC (permalink / raw)
To: Christian Speich, qemu-devel; +Cc: Bin Meng, qemu-block
On 4/2/26 14:22, Christian Speich wrote:
> When erasing blocks as zero, we can use optimized block functions to
> achieve this.
>
> These allow us to request a large rage to be zeroed, possible optimizing
> this operation and freeing disk space for sparsesly stored images.
>
> This only is possible when erase-blocks-as-zero=true is used and can
> provide a significant performance boost. The case where 0xFF is used
> during erase is as slow as before.
>
> Signed-off-by: Christian Speich <c.speich@avm.de>
> ---
> hw/sd/sd.c | 44 ++++++++++++++++++++++++++++++++++----------
> 1 file changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 5ddaa08d77b6b7606005fb59f42f107d254d6e5c..6e3afedbf45d59c2c9b48505ed144a788bc76760 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1331,6 +1331,17 @@ exit:
> trace_sdcard_rpmb_write_block(req, lduw_be_p(&sd->rpmb.result.result));
> }
>
> +/* Requires sd->buf to be filled with 0xFF */
> +static void sd_erase_ff(SDState *sd, uint64_t addr, size_t len)
> +{
> + int erase_len = 1 << HWBLOCK_SHIFT;
> + uint64_t erase_addr;
> +
> + for (erase_addr = addr; erase_addr < addr + len; erase_addr += erase_len) {
> + sd_blk_write(sd, erase_addr, erase_len);
> + }
> +}
> +
> static void sd_erase(SDState *sd)
> {
> uint64_t erase_start = sd->erase_start;
> @@ -1338,7 +1349,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
> @@ -1367,24 +1377,38 @@ static void sd_erase(SDState *sd)
> sd->erase_end = INVALID_ADDRESS;
> sd->csd[14] |= 0x40;
>
> - if (sd->erase_blocks_as_zero) {
> - memset(sd->data, 0x0, erase_len);
> - } else {
> - memset(sd->data, 0xFF, erase_len);
> + if (!sd->erase_blocks_as_zero) {
> + memset(sd->data, 0xFF, 1 << HWBLOCK_SHIFT);
> }
>
> - 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;
s/to_erase/erase_len/
> 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;
> }
> +
> + if (sd->erase_blocks_as_zero) {
> + blk_pwrite_zeroes(sd->blk, erase_addr + sd_part_offset(sd),
> + to_erase, 0);
> + } else {
> + sd_erase_ff(sd, erase_addr, to_erase);
> + }
Could you extract as sd_blk_erase() helper?
> + }
> + } else {
> + if (sd->erase_blocks_as_zero) {
> + blk_pwrite_zeroes(sd->blk, erase_start + sd_part_offset(sd),
> + erase_end - erase_start, 0);
> + } else {
> + sd_erase_ff(sd, erase_start, erase_end - erase_start);
> }
> - sd_blk_write(sd, erase_addr, erase_len);
> }
> }
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 0/6] hw/sd: Improve performance of read/write/erase
2026-02-04 13:22 [PATCH v3 0/6] hw/sd: Improve performance of read/write/erase Christian Speich via qemu development
` (5 preceding siblings ...)
2026-02-04 13:22 ` [PATCH v3 6/6] hw/sd/sdcard: Optimize erase blocks as zero Christian Speich
@ 2026-03-19 8:37 ` Christian Speich
2026-04-14 15:23 ` Philippe Mathieu-Daudé
6 siblings, 1 reply; 20+ messages in thread
From: Christian Speich @ 2026-03-19 8:37 UTC (permalink / raw)
To: qemu-devel, Philippe Mathieu-Daudé, Bin Meng, qemu-block
Ping :) Anything I can do to move this forward?
Kind Regards,
Christian
On Wed, Feb 04, 2026 at 02:22:24PM +0100, Christian Speich via qemu development 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 (commit 1-3) 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.
>
> The fifth commit fixes the DATA_STAT_AFTER_ERASE bit in SCR and
> introduces an option to allow to erase blocks to 0x00.
>
> The sixth commit optimizes block erase when erase-blocks-as-zero=true
> is used, by passing the zeroing request down the to the block device.
> Erasing 2GiB now takes 0.1s instead of 26s.
>
> Signed-off-by: Christian Speich <c.speich@avm.de>
> ---
> Changes in v3:
> - Rebase onto master, updating read/write path for newly added RBMP
> - Split up commit 1 into multiple commits
> - change interface to allow "short" read/writes that are continued
> by the core later by calling again
> - Link to v2: https://lore.kernel.org/qemu-devel/20251202-sdcard-performance-b4-v2-0-d42490b11322@avm.de
>
> Changes in v2:
> - Properly set DATA_STAT_AFTER_ERASE in SCR
> - Add erase-blocks-as-zero option to allow the user to switch between
> 0x00 and 0xFF for erased blocks.
> - Link to v1: https://lore.kernel.org/qemu-devel/20250919-sdcard-performance-b4-v1-0-e1037e481a19@avm.de
>
> ---
> Christian Speich (6):
> hw/sd: Switch read/write primitive to buf+len
> hw/sd/sd: Allow multi-byte read/write for generic paths
> hw/sd/sd: Use multi-byte/block writes for block path
> hw/sd/sdhci: Don't use bounce buffer for ADMA
> hw/sd/sdcard: Add erase-blocks-as-zero option.
> hw/sd/sdcard: Optimize erase blocks as zero.
>
> hw/sd/core.c | 26 +++--
> hw/sd/sd.c | 333 ++++++++++++++++++++++++++++++++++++++++-------------
> hw/sd/sdhci.c | 102 ++++++++--------
> include/hw/sd/sd.h | 22 ++--
> 4 files changed, 341 insertions(+), 142 deletions(-)
> ---
> base-commit: 28a6ca268c2cd3718b5bf24c2d97e3d1e95fc604
> change-id: 20250912-sdcard-performance-b4-d908bbb5a004
>
> Best regards,
> --
> Christian Speich <c.speich@avm.de>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread