hw/sd: Switch read/write primitive to buf+len

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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-ID: <20260417-sdcard-performance-b4-v4-1-119e66be10c2@avm.de>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

diff --git a/hw/sd/core.c b/hw/sd/core.c
index 3568a81e809fe107cfd0b5cc33b8b5761b11ce04..594c5e011ba30940a33799f9032c92494ee0ca19 100644 (file)
--- 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 ebe6e35b15ea700d2e503a7cdeb99cbc5ac7d4b5..c0b89735f696f226d7e6199537d0afa14d46c35a 100644 (file)
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2639,30 +2639,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);
@@ -2687,7 +2694,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;
@@ -2717,7 +2724,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 ++)
@@ -2735,7 +2742,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 ++)
@@ -2758,7 +2765,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);
@@ -2768,36 +2775,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,
@@ -2815,7 +2833,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 */
@@ -2832,7 +2850,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;
@@ -2851,10 +2869,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)
@@ -3196,8 +3214,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 (file)
--- 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);