[Qemu-devel] [PATCH v2 00/14] sdcard: Proper implementation of CRC7

[Qemu-devel] [PATCH v2 00/14] sdcard: Proper implementation of CRC7

[Philippe Mathieu-Daudé]

Hi,

This series emerged after last Coverity scan and Peter suggestion in:
http://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg05046.html

    (3) "proper" implementation of CRC, so that an sd controller
    can either (a) mark the SDRequest as "no CRC" and have
    sd_req_crc_validate() always pass, or (b) mark the SDRequest
    as having a CRC and have sd_req_crc_validate() actually
    do the check which it currently stubs out with "return 0"

- Coverity issues fixed
- new functions documented
- qtests added

This series would be much smaller without qtests (less refactor and code
movement), but I feel more confident having them passing :)

v2:
- simplified (do not touch CRC16: less code added)
- added R-b

v1: http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg01130.html

Regards,

Phil.

Philippe Mathieu-Daudé (14):
  sdcard: Use the ldst API
  sdcard: Constify sd_crc*()'s message argument
  sdcard: Fix sd_crc*() style
  sdcard: Extract sd_frame48/136_calc_checksum()
  sdcard: Move sd_crc7() and calc_checksum() out for qtesting
  sdcard: Add test_sd_verify_cksum_frame136()
  sdcard: Invert the sd_req_crc_is_valid() logic
  sdcard: Extract sd_frame48_verify_checksum() out for qtesting
  sdcard: Add sd_frame136_verify_checksum()
  sdcard: Remove the SDRequest argument from internal functions
  sdcard: Add sd_frame48_init(), replace SDRequest by a raw buffer
  sdcard: Add tests to validate the 7-bit CRC checksum of 48-bit SD frame
  sdcard: Add a "validate-crc" property
  hw/sd/ssi-sd: Enable CRC validation

 include/hw/sd/sd.h        |  64 +++++++++++++---
 hw/sd/bcm2835_sdhost.c    |  21 +++---
 hw/sd/core.c              |   7 +-
 hw/sd/milkymist-memcard.c |  14 +---
 hw/sd/omap_mmc.c          |  14 ++--
 hw/sd/pl181.c             |  21 +++---
 hw/sd/pxa2xx_mmci.c       |   8 +-
 hw/sd/sd.c                | 153 ++++++++++++++++++--------------------
 hw/sd/sdhci.c             |  35 ++++-----
 hw/sd/sdmmc-internal.c    |  59 +++++++++++++++
 hw/sd/ssi-sd.c            |  19 ++---
 tests/sdcard-test.c       | 121 ++++++++++++++++++++++++++++++
 tests/Makefile.include    |   4 +
 13 files changed, 369 insertions(+), 171 deletions(-)
 create mode 100644 tests/sdcard-test.c

-- 
2.17.0

[Qemu-devel] [PATCH v2 01/14] sdcard: Use the ldst API

[Philippe Mathieu-Daudé]

The load/store API will ease further code movement.

Per the Physical Layer Simplified Spec. "3.6 Bus Protocol":

  "In the CMD line the Most Significant Bit (MSB) is transmitted
   first, the Least Significant Bit (LSB) is the last."

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/bcm2835_sdhost.c    | 13 +++++--------
 hw/sd/milkymist-memcard.c |  3 +--
 hw/sd/omap_mmc.c          |  6 ++----
 hw/sd/pl181.c             | 11 ++++-------
 hw/sd/sdhci.c             | 15 +++++----------
 hw/sd/ssi-sd.c            |  6 ++----
 6 files changed, 19 insertions(+), 35 deletions(-)

diff --git a/hw/sd/bcm2835_sdhost.c b/hw/sd/bcm2835_sdhost.c
index ebf3b926c2..4df4de7d67 100644
--- a/hw/sd/bcm2835_sdhost.c
+++ b/hw/sd/bcm2835_sdhost.c
@@ -118,8 +118,6 @@ static void bcm2835_sdhost_send_command(BCM2835SDHostState *s)
         goto error;
     }
     if (!(s->cmd & SDCMD_NO_RESPONSE)) {
-#define RWORD(n) (((uint32_t)rsp[n] << 24) | (rsp[n + 1] << 16) \
-                  | (rsp[n + 2] << 8) | rsp[n + 3])
         if (rlen == 0 || (rlen == 4 && (s->cmd & SDCMD_LONG_RESPONSE))) {
             goto error;
         }
@@ -127,15 +125,14 @@ static void bcm2835_sdhost_send_command(BCM2835SDHostState *s)
             goto error;
         }
         if (rlen == 4) {
-            s->rsp[0] = RWORD(0);
+            s->rsp[0] = ldl_be_p(&rsp[0]);
             s->rsp[1] = s->rsp[2] = s->rsp[3] = 0;
         } else {
-            s->rsp[0] = RWORD(12);
-            s->rsp[1] = RWORD(8);
-            s->rsp[2] = RWORD(4);
-            s->rsp[3] = RWORD(0);
+            s->rsp[0] = ldl_be_p(&rsp[12]);
+            s->rsp[1] = ldl_be_p(&rsp[8]);
+            s->rsp[2] = ldl_be_p(&rsp[4]);
+            s->rsp[3] = ldl_be_p(&rsp[0]);
         }
-#undef RWORD
     }
     /* We never really delay commands, so if this was a 'busywait' command
      * then we've completed it now and can raise the interrupt.
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index 5570c1e9a0..ff2b92dc64 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -100,8 +100,7 @@ static void memcard_sd_command(MilkymistMemcardState *s)
     SDRequest req;
 
     req.cmd = s->command[0] & 0x3f;
-    req.arg = (s->command[1] << 24) | (s->command[2] << 16)
-              | (s->command[3] << 8) | s->command[4];
+    req.arg = ldl_be_p(s->command + 1);
     req.crc = s->command[5];
 
     s->response[0] = req.cmd;
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index 5b47cadf11..51c6c124b2 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -162,8 +162,7 @@ static void omap_mmc_command(struct omap_mmc_s *host, int cmd, int dir,
                 CID_CSD_OVERWRITE;
         if (host->sdio & (1 << 13))
             mask |= AKE_SEQ_ERROR;
-        rspstatus = (response[0] << 24) | (response[1] << 16) |
-                (response[2] << 8) | (response[3] << 0);
+        rspstatus = ldl_be_p(response);
         break;
 
     case sd_r2:
@@ -181,8 +180,7 @@ static void omap_mmc_command(struct omap_mmc_s *host, int cmd, int dir,
         }
         rsplen = 4;
 
-        rspstatus = (response[0] << 24) | (response[1] << 16) |
-                (response[2] << 8) | (response[3] << 0);
+        rspstatus = ldl_be_p(response);
         if (rspstatus & 0x80000000)
             host->status &= 0xe000;
         else
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index 3ba1f7dd23..c9b1a6cb23 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -183,23 +183,20 @@ static void pl181_send_command(PL181State *s)
     if (rlen < 0)
         goto error;
     if (s->cmd & PL181_CMD_RESPONSE) {
-#define RWORD(n) (((uint32_t)response[n] << 24) | (response[n + 1] << 16) \
-                  | (response[n + 2] << 8) | response[n + 3])
         if (rlen == 0 || (rlen == 4 && (s->cmd & PL181_CMD_LONGRESP)))
             goto error;
         if (rlen != 4 && rlen != 16)
             goto error;
-        s->response[0] = RWORD(0);
+        s->response[0] = ldl_be_p(&response[0]);
         if (rlen == 4) {
             s->response[1] = s->response[2] = s->response[3] = 0;
         } else {
-            s->response[1] = RWORD(4);
-            s->response[2] = RWORD(8);
-            s->response[3] = RWORD(12) & ~1;
+            s->response[1] = ldl_be_p(&response[4]);
+            s->response[2] = ldl_be_p(&response[8]);
+            s->response[3] = ldl_be_p(&response[12]) & ~1;
         }
         DPRINTF("Response received\n");
         s->status |= PL181_STATUS_CMDRESPEND;
-#undef RWORD
     } else {
         DPRINTF("Command sent\n");
         s->status |= PL181_STATUS_CMDSENT;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 63c44a4ee8..f6fe93f033 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -344,17 +344,13 @@ static void sdhci_send_command(SDHCIState *s)
 
     if (s->cmdreg & SDHC_CMD_RESPONSE) {
         if (rlen == 4) {
-            s->rspreg[0] = (response[0] << 24) | (response[1] << 16) |
-                           (response[2] << 8)  |  response[3];
+            s->rspreg[0] = ldl_be_p(response);
             s->rspreg[1] = s->rspreg[2] = s->rspreg[3] = 0;
             trace_sdhci_response4(s->rspreg[0]);
         } else if (rlen == 16) {
-            s->rspreg[0] = (response[11] << 24) | (response[12] << 16) |
-                           (response[13] << 8) |  response[14];
-            s->rspreg[1] = (response[7] << 24) | (response[8] << 16) |
-                           (response[9] << 8)  |  response[10];
-            s->rspreg[2] = (response[3] << 24) | (response[4] << 16) |
-                           (response[5] << 8)  |  response[6];
+            s->rspreg[0] = ldl_be_p(&response[11]);
+            s->rspreg[1] = ldl_be_p(&response[7]);
+            s->rspreg[2] = ldl_be_p(&response[3]);
             s->rspreg[3] = (response[0] << 16) | (response[1] << 8) |
                             response[2];
             trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
@@ -398,8 +394,7 @@ static void sdhci_end_transfer(SDHCIState *s)
         trace_sdhci_end_transfer(request.cmd, request.arg);
         sdbus_do_command(&s->sdbus, &request, response);
         /* Auto CMD12 response goes to the upper Response register */
-        s->rspreg[3] = (response[0] << 24) | (response[1] << 16) |
-                (response[2] << 8) | response[3];
+        s->rspreg[3] = ldl_be_p(response);
     }
 
     s->prnsts &= ~(SDHC_DOING_READ | SDHC_DOING_WRITE |
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index ae04b6641b..dbcff4013d 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -97,8 +97,7 @@ static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
             uint8_t longresp[16];
             /* FIXME: Check CRC.  */
             request.cmd = s->cmd;
-            request.arg = (s->cmdarg[0] << 24) | (s->cmdarg[1] << 16)
-                           | (s->cmdarg[2] << 8) | s->cmdarg[3];
+            request.arg = ldl_be_p(s->cmdarg);
             DPRINTF("CMD%d arg 0x%08x\n", s->cmd, request.arg);
             s->arglen = sdbus_do_command(&s->sdbus, &request, longresp);
             if (s->arglen <= 0) {
@@ -123,8 +122,7 @@ static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
                 /* CMD13 returns a 2-byte statuse work. Other commands
                    only return the first byte.  */
                 s->arglen = (s->cmd == 13) ? 2 : 1;
-                cardstatus = (longresp[0] << 24) | (longresp[1] << 16)
-                             | (longresp[2] << 8) | longresp[3];
+                cardstatus = ldl_be_p(longresp);
                 status = 0;
                 if (((cardstatus >> 9) & 0xf) < 4)
                     status |= SSI_SDR_IDLE;
-- 
2.17.0

[Qemu-devel] [PATCH v2 02/14] sdcard: Constify sd_crc*()'s message argument

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 235e0518d6..eb6dd5482e 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -236,11 +236,11 @@ static const int sd_cmd_class[SDMMC_CMD_MAX] = {
     7,  7, 10,  7,  9,  9,  9,  8,  8, 10,  8,  8,  8,  8,  8,  8,
 };
 
-static uint8_t sd_crc7(void *message, size_t width)
+static uint8_t sd_crc7(const void *message, size_t width)
 {
     int i, bit;
     uint8_t shift_reg = 0x00;
-    uint8_t *msg = (uint8_t *) message;
+    const uint8_t *msg = (const uint8_t *)message;
 
     for (i = 0; i < width; i ++, msg ++)
         for (bit = 7; bit >= 0; bit --) {
@@ -252,11 +252,11 @@ static uint8_t sd_crc7(void *message, size_t width)
     return shift_reg;
 }
 
-static uint16_t sd_crc16(void *message, size_t width)
+static uint16_t sd_crc16(const void *message, size_t width)
 {
     int i, bit;
     uint16_t shift_reg = 0x0000;
-    uint16_t *msg = (uint16_t *) message;
+    const uint16_t *msg = (const uint16_t *)message;
     width <<= 1;
 
     for (i = 0; i < width; i ++, msg ++)
-- 
2.17.0

[Qemu-devel] [PATCH v2 03/14] sdcard: Fix sd_crc*() style

[Philippe Mathieu-Daudé]

Fix style to keep patchew/checkpatch happy when moving this code
in the next patch.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/sd/sd.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index eb6dd5482e..27a70896cd 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -242,12 +242,14 @@ static uint8_t sd_crc7(const void *message, size_t width)
     uint8_t shift_reg = 0x00;
     const uint8_t *msg = (const uint8_t *)message;
 
-    for (i = 0; i < width; i ++, msg ++)
-        for (bit = 7; bit >= 0; bit --) {
+    for (i = 0; i < width; i++, msg++) {
+        for (bit = 7; bit >= 0; bit--) {
             shift_reg <<= 1;
-            if ((shift_reg >> 7) ^ ((*msg >> bit) & 1))
+            if ((shift_reg >> 7) ^ ((*msg >> bit) & 1)) {
                 shift_reg ^= 0x89;
+            }
         }
+    }
 
     return shift_reg;
 }
@@ -259,12 +261,14 @@ static uint16_t sd_crc16(const void *message, size_t width)
     const uint16_t *msg = (const uint16_t *)message;
     width <<= 1;
 
-    for (i = 0; i < width; i ++, msg ++)
-        for (bit = 15; bit >= 0; bit --) {
+    for (i = 0; i < width; i++, msg++) {
+        for (bit = 15; bit >= 0; bit--) {
             shift_reg <<= 1;
-            if ((shift_reg >> 15) ^ ((*msg >> bit) & 1))
+            if ((shift_reg >> 15) ^ ((*msg >> bit) & 1)) {
                 shift_reg ^= 0x1011;
+            }
         }
+    }
 
     return shift_reg;
 }
-- 
2.17.0

[Qemu-devel] [PATCH v2 04/14] sdcard: Extract sd_frame48/136_calc_checksum()

[Philippe Mathieu-Daudé]

It will help when moving this around for qtesting in the next commit.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 27a70896cd..06607115a7 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -273,6 +273,21 @@ static uint16_t sd_crc16(const void *message, size_t width)
     return shift_reg;
 }
 
+enum {
+    F48_CONTENT_LENGTH  = 1 /* command */ + 4 /* argument */,
+    F136_CONTENT_LENGTH = 15,
+};
+
+static uint8_t sd_frame48_calc_checksum(const void *content)
+{
+    return sd_crc7(content, F48_CONTENT_LENGTH);
+}
+
+static uint8_t sd_frame136_calc_checksum(const void *content)
+{
+    return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
+}
+
 #define OCR_POWER_DELAY_NS      500000 /* 0.5ms */
 
 FIELD(OCR, VDD_VOLTAGE_WINDOW,          0, 24)
@@ -352,7 +367,7 @@ static void sd_set_cid(SDState *sd)
     sd->cid[13] = 0x00 |	/* Manufacture date (MDT) */
         ((MDT_YR - 2000) / 10);
     sd->cid[14] = ((MDT_YR % 10) << 4) | MDT_MON;
-    sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
+    sd->cid[15] = sd_frame136_calc_checksum(sd->cid);
 }
 
 #define HWBLOCK_SHIFT	9			/* 512 bytes */
@@ -416,7 +431,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
         sd->csd[13] = 0x40;
         sd->csd[14] = 0x00;
     }
-    sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
+    sd->csd[15] = sd_frame136_calc_checksum(sd->csd);
 }
 
 static void sd_set_rca(SDState *sd)
@@ -491,7 +506,7 @@ static int sd_req_crc_validate(SDRequest *req)
     buffer[0] = 0x40 | req->cmd;
     stl_be_p(&buffer[1], req->arg);
     return 0;
-    return sd_crc7(buffer, 5) != req->crc;	/* TODO */
+    return sd_frame48_calc_checksum(buffer) != req->crc; /* TODO */
 }
 
 static void sd_response_r1_make(SDState *sd, uint8_t *response)
-- 
2.17.0

[Qemu-devel] [PATCH v2 05/14] sdcard: Move sd_crc7() and calc_checksum() out for qtesting

[Philippe Mathieu-Daudé]

Move the calc_checksum() functions out so we will able to write
qtests for them.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/sd/sd.h     | 20 ++++++++++++++++++++
 hw/sd/sd.c             | 33 ---------------------------------
 hw/sd/sdmmc-internal.c | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 33 deletions(-)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 9bdb3c9285..c854ed6a14 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -130,6 +130,26 @@ typedef struct {
     void (*set_readonly)(DeviceState *dev, bool readonly);
 } SDBusClass;
 
+/**
+ * sd_frame48_calc_checksum:
+ * @content: pointer to the frame content
+ *
+ * Calculate the 7-bit CRC of a 48-bit SD frame.
+ *
+ * Returns: The frame CRC.
+ */
+uint8_t sd_frame48_calc_checksum(const void *content);
+
+/**
+ * sd_frame136_calc_checksum:
+ * @content: pointer to the frame content
+ *
+ * Calculate the 7-bit CRC of a 136-bit SD frame.
+ *
+ * Returns: The frame CRC.
+ */
+uint8_t sd_frame136_calc_checksum(const void *content);
+
 /* Legacy functions to be used only by non-qdevified callers */
 SDState *sd_init(BlockBackend *bs, bool is_spi);
 int sd_do_command(SDState *sd, SDRequest *req,
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 06607115a7..d8dad94fc4 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -236,24 +236,6 @@ static const int sd_cmd_class[SDMMC_CMD_MAX] = {
     7,  7, 10,  7,  9,  9,  9,  8,  8, 10,  8,  8,  8,  8,  8,  8,
 };
 
-static uint8_t sd_crc7(const void *message, size_t width)
-{
-    int i, bit;
-    uint8_t shift_reg = 0x00;
-    const uint8_t *msg = (const uint8_t *)message;
-
-    for (i = 0; i < width; i++, msg++) {
-        for (bit = 7; bit >= 0; bit--) {
-            shift_reg <<= 1;
-            if ((shift_reg >> 7) ^ ((*msg >> bit) & 1)) {
-                shift_reg ^= 0x89;
-            }
-        }
-    }
-
-    return shift_reg;
-}
-
 static uint16_t sd_crc16(const void *message, size_t width)
 {
     int i, bit;
@@ -273,21 +255,6 @@ static uint16_t sd_crc16(const void *message, size_t width)
     return shift_reg;
 }
 
-enum {
-    F48_CONTENT_LENGTH  = 1 /* command */ + 4 /* argument */,
-    F136_CONTENT_LENGTH = 15,
-};
-
-static uint8_t sd_frame48_calc_checksum(const void *content)
-{
-    return sd_crc7(content, F48_CONTENT_LENGTH);
-}
-
-static uint8_t sd_frame136_calc_checksum(const void *content)
-{
-    return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
-}
-
 #define OCR_POWER_DELAY_NS      500000 /* 0.5ms */
 
 FIELD(OCR, VDD_VOLTAGE_WINDOW,          0, 24)
diff --git a/hw/sd/sdmmc-internal.c b/hw/sd/sdmmc-internal.c
index 2053def3f1..a94d65b756 100644
--- a/hw/sd/sdmmc-internal.c
+++ b/hw/sd/sdmmc-internal.c
@@ -9,6 +9,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/sd/sd.h"
 #include "sdmmc-internal.h"
 
 const char *sd_cmd_name(uint8_t cmd)
@@ -70,3 +71,37 @@ const char *sd_acmd_name(uint8_t cmd)
 
     return acmd_abbrev[cmd] ? acmd_abbrev[cmd] : "UNKNOWN_ACMD";
 }
+
+/* 7 bit CRC with polynomial x^7 + x^3 + 1 */
+static uint8_t sd_crc7(const void *message, size_t width)
+{
+    int i, bit;
+    uint8_t shift_reg = 0x00;
+    const uint8_t *msg = (const uint8_t *)message;
+
+    for (i = 0; i < width; i++, msg++) {
+        for (bit = 7; bit >= 0; bit--) {
+            shift_reg <<= 1;
+            if ((shift_reg >> 7) ^ ((*msg >> bit) & 1)) {
+                shift_reg ^= 0x89;
+            }
+        }
+    }
+
+    return shift_reg;
+}
+
+enum {
+    F48_CONTENT_LENGTH  = 1 /* command */ + 4 /* argument */,
+    F136_CONTENT_LENGTH = 15,
+};
+
+uint8_t sd_frame48_calc_checksum(const void *content)
+{
+    return sd_crc7(content, F48_CONTENT_LENGTH);
+}
+
+uint8_t sd_frame136_calc_checksum(const void *content)
+{
+    return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
+}
-- 
2.17.0

[Qemu-devel] [PATCH v2 06/14] sdcard: Add test_sd_verify_cksum_frame136()

[Philippe Mathieu-Daudé]

Per the Physical Layer Simplified Spec. "3.6 Bus Protocol":

  There are two types of Data packet format for the SD card.

  (1) Usual data (8-bit width): The usual data (8-bit width) are sent in
      LSB (Least Significant Byte) first, MSB (Most Significant Byte) last
      sequence. But in the individual byte, it is MSB (Most Significant Bit)
      first, LSB (Least Significant Bit) last.

  (2) Wide width data (SD Memory Register): The wide width data is shifted
      from the MSB bit.

Since the SD frames use different byte order, we use the lm32 target
for big-endian qtesting, and aarch64 for the little-endian qtesting.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/sdcard-test.c    | 35 +++++++++++++++++++++++++++++++++++
 tests/Makefile.include |  4 ++++
 2 files changed, 39 insertions(+)
 create mode 100644 tests/sdcard-test.c

diff --git a/tests/sdcard-test.c b/tests/sdcard-test.c
new file mode 100644
index 0000000000..c59af28382
--- /dev/null
+++ b/tests/sdcard-test.c
@@ -0,0 +1,35 @@
+/*
+ * QTest testcase for SD protocol and cards
+ *
+ * Examples taken from:
+ *
+ * - Physical Layer Simplified Specification (chap. 4.5: Cyclic Redundancy Code)
+ * - http://wiki.seabright.co.nz/wiki/SdCardProtocol.html
+ *
+ * Tests written by Philippe Mathieu-Daudé <f4bug@amsat.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "hw/sd/sd.h"
+
+static void test_sd_response_frame136_crc7(void)
+{
+    uint8_t buf[16];
+
+    /* response to CMD2 ALL_SEND_CID */
+    memcpy(&buf, "\x1d\x41\x44\x53\x44\x20\x20\x20\x10\xa0\x40\x0b\xc1\x00\x88",
+           sizeof(buf));
+    buf[15] = sd_frame136_calc_checksum(buf);
+    g_assert_cmphex(buf[15], ==, 0xad);
+}
+
+int main(int argc, char *argv[])
+{
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_add_func("sd/prepare_resp136_crc7", test_sd_response_frame136_crc7);
+
+    return g_test_run();
+}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3b9a5e31a2..34230b150e 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -384,6 +384,9 @@ check-qtest-arm-y += tests/sdhci-test$(EXESUF)
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/sdhci-test$(EXESUF)
 check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
+check-qtest-aarch64-y += tests/sdcard-test$(EXESUF)
+
+check-qtest-lm32-y += tests/sdcard-test$(EXESUF)
 
 check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
 
@@ -835,6 +838,7 @@ tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
 tests/numa-test$(EXESUF): tests/numa-test.o
 tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
 tests/sdhci-test$(EXESUF): tests/sdhci-test.o $(libqos-pc-obj-y)
+tests/sdcard-test$(EXESUF): tests/sdcard-test.o hw/sd/sdmmc-internal.o
 
 tests/migration/stress$(EXESUF): tests/migration/stress.o
 	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
-- 
2.17.0

[Qemu-devel] [PATCH v2 07/14] sdcard: Invert the sd_req_crc_is_valid() logic

[Philippe Mathieu-Daudé]

Let's return TRUE when the CRC is valid.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index d8dad94fc4..6fc8daa5b8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -467,13 +467,13 @@ static void sd_set_sdstatus(SDState *sd)
     memset(sd->sd_status, 0, 64);
 }
 
-static int sd_req_crc_validate(SDRequest *req)
+static bool sd_req_crc_is_valid(SDRequest *req)
 {
     uint8_t buffer[5];
     buffer[0] = 0x40 | req->cmd;
     stl_be_p(&buffer[1], req->arg);
-    return 0;
-    return sd_frame48_calc_checksum(buffer) != req->crc; /* TODO */
+    return true;
+    return sd_frame48_calc_checksum(buffer) == req->crc; /* TODO */
 }
 
 static void sd_response_r1_make(SDState *sd, uint8_t *response)
@@ -1631,7 +1631,7 @@ int sd_do_command(SDState *sd, SDRequest *req,
         return 0;
     }
 
-    if (sd_req_crc_validate(req)) {
+    if (!sd_req_crc_is_valid(req)) {
         sd->card_status |= COM_CRC_ERROR;
         rtype = sd_illegal;
         goto send_response;
-- 
2.17.0

[Qemu-devel] [PATCH v2 08/14] sdcard: Extract sd_frame48_verify_checksum() out for qtesting

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/sd/sd.h     | 10 ++++++++++
 hw/sd/sd.c             |  2 +-
 hw/sd/sdmmc-internal.c |  6 ++++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index c854ed6a14..752d8edd6c 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -150,6 +150,16 @@ uint8_t sd_frame48_calc_checksum(const void *content);
  */
 uint8_t sd_frame136_calc_checksum(const void *content);
 
+/**
+ * sd_frame48_verify_checksum:
+ * @content: pointer to the frame content
+ *
+ * Verify the 7-bit CRC checksum of a 48-bit SD frame.
+ *
+ * Returns: A boolean indicating whether the frame 7-bit CRC is correct.
+ */
+bool sd_frame48_verify_checksum(const void *content);
+
 /* Legacy functions to be used only by non-qdevified callers */
 SDState *sd_init(BlockBackend *bs, bool is_spi);
 int sd_do_command(SDState *sd, SDRequest *req,
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 6fc8daa5b8..125707a65c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -473,7 +473,7 @@ static bool sd_req_crc_is_valid(SDRequest *req)
     buffer[0] = 0x40 | req->cmd;
     stl_be_p(&buffer[1], req->arg);
     return true;
-    return sd_frame48_calc_checksum(buffer) == req->crc; /* TODO */
+    return sd_frame48_verify_checksum(buffer); /* TODO */
 }
 
 static void sd_response_r1_make(SDState *sd, uint8_t *response)
diff --git a/hw/sd/sdmmc-internal.c b/hw/sd/sdmmc-internal.c
index a94d65b756..a9d19ce3eb 100644
--- a/hw/sd/sdmmc-internal.c
+++ b/hw/sd/sdmmc-internal.c
@@ -105,3 +105,9 @@ uint8_t sd_frame136_calc_checksum(const void *content)
 {
     return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
 }
+
+bool sd_frame48_verify_checksum(const void *content)
+{
+    return sd_frame48_calc_checksum(content)
+           == ((const uint8_t *)content)[F48_CONTENT_LENGTH];
+}
-- 
2.17.0

[Qemu-devel] [PATCH v2 09/14] sdcard: Add sd_frame136_verify_checksum()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/sd/sd.h     | 10 ++++++++++
 hw/sd/sdmmc-internal.c |  6 ++++++
 tests/sdcard-test.c    | 12 ++++++++++++
 3 files changed, 28 insertions(+)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 752d8edd6c..83399cd42d 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -160,6 +160,16 @@ uint8_t sd_frame136_calc_checksum(const void *content);
  */
 bool sd_frame48_verify_checksum(const void *content);
 
+/**
+ * sd_frame136_verify_checksum:
+ * @content: pointer to the frame content
+ *
+ * Verify the 7-bit CRC checksum of a 136-bit SD frame.
+ *
+ * Returns: A boolean indicating whether the frame 7-bit CRC is correct.
+ */
+bool sd_frame136_verify_checksum(const void *content);
+
 /* Legacy functions to be used only by non-qdevified callers */
 SDState *sd_init(BlockBackend *bs, bool is_spi);
 int sd_do_command(SDState *sd, SDRequest *req,
diff --git a/hw/sd/sdmmc-internal.c b/hw/sd/sdmmc-internal.c
index a9d19ce3eb..f709211401 100644
--- a/hw/sd/sdmmc-internal.c
+++ b/hw/sd/sdmmc-internal.c
@@ -111,3 +111,9 @@ bool sd_frame48_verify_checksum(const void *content)
     return sd_frame48_calc_checksum(content)
            == ((const uint8_t *)content)[F48_CONTENT_LENGTH];
 }
+
+bool sd_frame136_verify_checksum(const void *content)
+{
+    return sd_frame136_calc_checksum(content)
+           == ((const uint8_t *)content)[F136_CONTENT_LENGTH];
+}
diff --git a/tests/sdcard-test.c b/tests/sdcard-test.c
index c59af28382..81789d1f88 100644
--- a/tests/sdcard-test.c
+++ b/tests/sdcard-test.c
@@ -23,6 +23,17 @@ static void test_sd_response_frame136_crc7(void)
            sizeof(buf));
     buf[15] = sd_frame136_calc_checksum(buf);
     g_assert_cmphex(buf[15], ==, 0xad);
+
+    g_assert_true(sd_frame136_verify_checksum(buf));
+}
+
+static void test_sd_verify_cksum_frame136(void)
+{
+    uint8_t buf[16];
+
+    memset(buf, 69, sizeof(buf));
+    buf[15] = sd_frame136_calc_checksum(buf);
+    g_assert_true(sd_frame136_verify_checksum(buf));
 }
 
 int main(int argc, char *argv[])
@@ -30,6 +41,7 @@ int main(int argc, char *argv[])
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_func("sd/prepare_resp136_crc7", test_sd_response_frame136_crc7);
+    qtest_add_func("sd/verify_cksum_frame136", test_sd_verify_cksum_frame136);
 
     return g_test_run();
 }
-- 
2.17.0

[Qemu-devel] [PATCH v2 10/14] sdcard: Remove the SDRequest argument from internal functions

[Philippe Mathieu-Daudé]

Replace the SDRequest argument using directly {uint8_t cmd, uint32_t arg},
it will be easier to remove the SDRequest struct in the next commit.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 104 +++++++++++++++++++++++++++--------------------------
 1 file changed, 53 insertions(+), 51 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 125707a65c..0dfcaf480c 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -853,35 +853,34 @@ static void sd_lock_command(SDState *sd)
         sd->card_status &= ~CARD_IS_LOCKED;
 }
 
-static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
+static sd_rsp_type_t sd_normal_command(SDState *sd, uint8_t cmd, uint32_t arg)
 {
     uint32_t rca = 0x0000;
-    uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) req.arg << 9 : req.arg;
+    uint64_t addr = (sd->ocr & (1 << 30)) ? (uint64_t) arg << 9 : arg;
 
     /* CMD55 precedes an ACMD, so we are not interested in tracing it.
      * However there is no ACMD55, so we want to trace this particular case.
      */
-    if (req.cmd != 55 || sd->expecting_acmd) {
+    if (cmd != 55 || sd->expecting_acmd) {
         trace_sdcard_normal_command(sd->proto_name,
-                                    sd_cmd_name(req.cmd), req.cmd,
-                                    req.arg, sd_state_name(sd->state));
+                                    sd_cmd_name(cmd), cmd,
+                                    arg, sd_state_name(sd->state));
     }
 
     /* Not interpreting this as an app command */
     sd->card_status &= ~APP_CMD;
 
-    if (sd_cmd_type[req.cmd] == sd_ac
-        || sd_cmd_type[req.cmd] == sd_adtc) {
-        rca = req.arg >> 16;
+    if (sd_cmd_type[cmd] == sd_ac || sd_cmd_type[cmd] == sd_adtc) {
+        rca = arg >> 16;
     }
 
     /* CMD23 (set block count) must be immediately followed by CMD18 or CMD25
      * if not, its effects are cancelled */
-    if (sd->multi_blk_cnt != 0 && !(req.cmd == 18 || req.cmd == 25)) {
+    if (sd->multi_blk_cnt != 0 && !(cmd == 18 || cmd == 25)) {
         sd->multi_blk_cnt = 0;
     }
 
-    switch (req.cmd) {
+    switch (cmd) {
     /* Basic commands (Class 0 and Class 1) */
     case 0:	/* CMD0:   GO_IDLE_STATE */
         switch (sd->state) {
@@ -950,7 +949,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             goto bad_cmd;
         switch (sd->mode) {
         case sd_data_transfer_mode:
-            sd_function_switch(sd, req.arg);
+            sd_function_switch(sd, arg);
             sd->state = sd_sendingdata_state;
             sd->data_start = 0;
             sd->data_offset = 0;
@@ -1007,12 +1006,12 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         sd->vhs = 0;
 
         /* No response if not exactly one VHS bit is set.  */
-        if (!(req.arg >> 8) || (req.arg >> (ctz32(req.arg & ~0xff) + 1))) {
+        if (!(arg >> 8) || (arg >> (ctz32(arg & ~0xff) + 1))) {
             return sd->spi ? sd_r7 : sd_r0;
         }
 
         /* Accept.  */
-        sd->vhs = req.arg;
+        sd->vhs = arg;
         return sd_r7;
 
     case 9:	/* CMD9:   SEND_CSD */
@@ -1109,11 +1108,11 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     case 16:	/* CMD16:  SET_BLOCKLEN */
         switch (sd->state) {
         case sd_transfer_state:
-            if (req.arg > (1 << HWBLOCK_SHIFT)) {
+            if (arg > (1 << HWBLOCK_SHIFT)) {
                 sd->card_status |= BLOCK_LEN_ERROR;
             } else {
-                trace_sdcard_set_blocklen(req.arg);
-                sd->blk_len = req.arg;
+                trace_sdcard_set_blocklen(arg);
+                sd->blk_len = arg;
             }
 
             return sd_r1;
@@ -1166,7 +1165,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     case 23:    /* CMD23: SET_BLOCK_COUNT */
         switch (sd->state) {
         case sd_transfer_state:
-            sd->multi_blk_cnt = req.arg;
+            sd->multi_blk_cnt = arg;
             return sd_r1;
 
         default:
@@ -1303,7 +1302,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         switch (sd->state) {
         case sd_transfer_state:
             sd->state = sd_sendingdata_state;
-            *(uint32_t *) sd->data = sd_wpbits(sd, req.arg);
+            *(uint32_t *) sd->data = sd_wpbits(sd, arg);
             sd->data_start = addr;
             sd->data_offset = 0;
             return sd_r1b;
@@ -1317,7 +1316,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     case 32:	/* CMD32:  ERASE_WR_BLK_START */
         switch (sd->state) {
         case sd_transfer_state:
-            sd->erase_start = req.arg;
+            sd->erase_start = arg;
             return sd_r1;
 
         default:
@@ -1328,7 +1327,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     case 33:	/* CMD33:  ERASE_WR_BLK_END */
         switch (sd->state) {
         case sd_transfer_state:
-            sd->erase_end = req.arg;
+            sd->erase_end = arg;
             return sd_r1;
 
         default:
@@ -1391,7 +1390,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         case sd_idle_state:
             if (rca) {
                 qemu_log_mask(LOG_GUEST_ERROR,
-                              "SD: illegal RCA 0x%04x for APP_CMD\n", req.cmd);
+                              "SD: illegal RCA 0x%04x for APP_CMD\n", cmd);
             }
         default:
             break;
@@ -1409,10 +1408,11 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         switch (sd->state) {
         case sd_transfer_state:
             sd->data_offset = 0;
-            if (req.arg & 1)
+            if (arg & 1) {
                 sd->state = sd_sendingdata_state;
-            else
+            } else {
                 sd->state = sd_receivingdata_state;
+            }
             return sd_r1;
 
         default:
@@ -1434,27 +1434,26 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
     default:
     bad_cmd:
-        qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", req.cmd);
+        qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", cmd);
         return sd_illegal;
 
     unimplemented_spi_cmd:
         /* Commands that are recognised but not yet implemented in SPI mode.  */
         qemu_log_mask(LOG_UNIMP, "SD: CMD%i not implemented in SPI mode\n",
-                      req.cmd);
+                      cmd);
         return sd_illegal;
     }
 
-    qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state\n", req.cmd);
+    qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state\n", cmd);
     return sd_illegal;
 }
 
-static sd_rsp_type_t sd_app_command(SDState *sd,
-                                    SDRequest req)
+static sd_rsp_type_t sd_app_command(SDState *sd, uint8_t cmd, uint32_t arg)
 {
-    trace_sdcard_app_command(sd->proto_name, sd_acmd_name(req.cmd),
-                             req.cmd, req.arg, sd_state_name(sd->state));
+    trace_sdcard_app_command(sd->proto_name, sd_acmd_name(cmd),
+                             cmd, arg, sd_state_name(sd->state));
     sd->card_status |= APP_CMD;
-    switch (req.cmd) {
+    switch (cmd) {
     case 6:	/* ACMD6:  SET_BUS_WIDTH */
         if (sd->spi) {
             goto unimplemented_spi_cmd;
@@ -1462,7 +1461,7 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
         switch (sd->state) {
         case sd_transfer_state:
             sd->sd_status[0] &= 0x3f;
-            sd->sd_status[0] |= (req.arg & 0x03) << 6;
+            sd->sd_status[0] |= (arg & 0x03) << 6;
             return sd_r1;
 
         default:
@@ -1526,7 +1525,7 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
          * assumes that the card is in ready state as soon as it
          * sees the power up bit set. */
         if (!FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP)) {
-            if ((req.arg & ACMD41_ENQUIRY_MASK) != 0) {
+            if ((arg & ACMD41_ENQUIRY_MASK) != 0) {
                 timer_del(sd->ocr_power_timer);
                 sd_ocr_powerup(sd);
             } else {
@@ -1539,7 +1538,7 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
             }
         }
 
-        if (FIELD_EX32(sd->ocr & req.arg, OCR, VDD_VOLTAGE_WINDOW)) {
+        if (FIELD_EX32(sd->ocr & arg, OCR, VDD_VOLTAGE_WINDOW)) {
             /* We accept any voltage.  10000 V is nothing.
              *
              * Once we're powered up, we advance straight to ready state
@@ -1583,25 +1582,25 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
          * information about the SD Security Features.
          */
         qemu_log_mask(LOG_UNIMP, "SD: CMD%i Security not implemented\n",
-                      req.cmd);
+                      cmd);
         return sd_illegal;
 
     default:
         /* Fall back to standard commands.  */
-        return sd_normal_command(sd, req);
+        return sd_normal_command(sd, cmd, arg);
 
     unimplemented_spi_cmd:
         /* Commands that are recognised but not yet implemented in SPI mode.  */
         qemu_log_mask(LOG_UNIMP, "SD: CMD%i not implemented in SPI mode\n",
-                      req.cmd);
+                      cmd);
         return sd_illegal;
     }
 
-    qemu_log_mask(LOG_GUEST_ERROR, "SD: ACMD%i in a wrong state\n", req.cmd);
+    qemu_log_mask(LOG_GUEST_ERROR, "SD: ACMD%i in a wrong state\n", cmd);
     return sd_illegal;
 }
 
-static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
+static int cmd_valid_while_locked(SDState *sd, uint8_t cmd)
 {
     /* Valid commands in locked state:
      * basic class (0)
@@ -1612,13 +1611,12 @@ static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
      * Anything else provokes an "illegal command" response.
      */
     if (sd->expecting_acmd) {
-        return req->cmd == 41 || req->cmd == 42;
+        return cmd == 41 || cmd == 42;
     }
-    if (req->cmd == 16 || req->cmd == 55) {
+    if (cmd == 16 || cmd == 55) {
         return 1;
     }
-    return sd_cmd_class[req->cmd] == 0
-            || sd_cmd_class[req->cmd] == 7;
+    return sd_cmd_class[cmd] == 0 || sd_cmd_class[cmd] == 7;
 }
 
 int sd_do_command(SDState *sd, SDRequest *req,
@@ -1626,25 +1624,29 @@ int sd_do_command(SDState *sd, SDRequest *req,
     int last_state;
     sd_rsp_type_t rtype;
     int rsplen;
+    uint8_t cmd;
+    uint32_t arg;
 
     if (!sd->blk || !blk_is_inserted(sd->blk) || !sd->enable) {
         return 0;
     }
 
+    cmd = req->cmd;
+    arg = req->arg;
+
     if (!sd_req_crc_is_valid(req)) {
         sd->card_status |= COM_CRC_ERROR;
         rtype = sd_illegal;
         goto send_response;
     }
 
-    if (req->cmd >= SDMMC_CMD_MAX) {
-        qemu_log_mask(LOG_GUEST_ERROR, "SD: incorrect command 0x%02x\n",
-                      req->cmd);
-        req->cmd &= 0x3f;
+    if (cmd >= SDMMC_CMD_MAX) {
+        qemu_log_mask(LOG_GUEST_ERROR, "SD: incorrect command 0x%02x\n", cmd);
+        cmd &= 0x3f;
     }
 
     if (sd->card_status & CARD_IS_LOCKED) {
-        if (!cmd_valid_while_locked(sd, req)) {
+        if (!cmd_valid_while_locked(sd, cmd)) {
             sd->card_status |= ILLEGAL_COMMAND;
             sd->expecting_acmd = false;
             qemu_log_mask(LOG_GUEST_ERROR, "SD: Card is locked\n");
@@ -1658,9 +1660,9 @@ int sd_do_command(SDState *sd, SDRequest *req,
 
     if (sd->expecting_acmd) {
         sd->expecting_acmd = false;
-        rtype = sd_app_command(sd, *req);
+        rtype = sd_app_command(sd, cmd, arg);
     } else {
-        rtype = sd_normal_command(sd, *req);
+        rtype = sd_normal_command(sd, cmd, arg);
     }
 
     if (rtype == sd_illegal) {
@@ -1669,7 +1671,7 @@ int sd_do_command(SDState *sd, SDRequest *req,
         /* Valid command, we can update the 'state before command' bits.
          * (Do this now so they appear in r1 responses.)
          */
-        sd->current_cmd = req->cmd;
+        sd->current_cmd = cmd;
         sd->card_status &= ~CURRENT_STATE;
         sd->card_status |= (last_state << 9);
     }
-- 
2.17.0

[Qemu-devel] [PATCH v2 11/14] sdcard: Add sd_frame48_init(), replace SDRequest by a raw buffer

[Philippe Mathieu-Daudé]

Using sd_frame48_init() we silent the Coverity warning:

  "Use of an uninitialized variable (CWE-457)"

and fixes the following issues (all "Uninitialized scalar variable"):

- CID1386072 (hw/sd/sdhci.c::sdhci_end_transfer)
- CID1386074 (hw/sd/bcm2835_sdhost.c::bcm2835_sdhost_send_command)
- CID1386076 (hw/sd/sdhci.c::sdhci_send_command)
- CID1390571 (hw/sd/ssi-sd.c::ssi_sd_transfer)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/sd/sd.h        | 24 +++++++++++++++---------
 hw/sd/bcm2835_sdhost.c    |  8 ++++----
 hw/sd/core.c              |  7 ++++---
 hw/sd/milkymist-memcard.c | 12 +++---------
 hw/sd/omap_mmc.c          |  8 +++-----
 hw/sd/pl181.c             | 10 +++++-----
 hw/sd/pxa2xx_mmci.c       |  8 +++-----
 hw/sd/sd.c                | 13 +++++--------
 hw/sd/sdhci.c             | 20 ++++++++++----------
 hw/sd/sdmmc-internal.c    | 12 ++++++++++++
 hw/sd/ssi-sd.c            | 12 +++++++-----
 11 files changed, 71 insertions(+), 63 deletions(-)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 83399cd42d..85b66a27a3 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -76,12 +76,6 @@ typedef enum {
     sd_adtc,	/* addressed with data transfer */
 } sd_cmd_type_t;
 
-typedef struct {
-    uint8_t cmd;
-    uint32_t arg;
-    uint8_t crc;
-} SDRequest;
-
 typedef struct SDState SDState;
 typedef struct SDBus SDBus;
 
@@ -97,7 +91,7 @@ typedef struct {
     DeviceClass parent_class;
     /*< public >*/
 
-    int (*do_command)(SDState *sd, SDRequest *req, uint8_t *response);
+    int (*do_command)(SDState *sd, const uint8_t *request, uint8_t *response);
     void (*write_data)(SDState *sd, uint8_t value);
     uint8_t (*read_data)(SDState *sd);
     bool (*data_ready)(SDState *sd);
@@ -130,6 +124,18 @@ typedef struct {
     void (*set_readonly)(DeviceState *dev, bool readonly);
 } SDBusClass;
 
+/**
+ * sd_frame48_init: Initialize a 48-bit SD frame
+ *
+ * @buf: the buffer to be filled
+ * @bufsize: the size of the @buffer
+ * @cmd: the SD command
+ * @arg: the SD command argument
+ * @is_response: whether the frame is a command request or response
+ */
+void sd_frame48_init(uint8_t *buf, size_t bufsize, uint8_t cmd, uint32_t arg,
+                     bool is_response);
+
 /**
  * sd_frame48_calc_checksum:
  * @content: pointer to the frame content
@@ -172,7 +178,7 @@ bool sd_frame136_verify_checksum(const void *content);
 
 /* Legacy functions to be used only by non-qdevified callers */
 SDState *sd_init(BlockBackend *bs, bool is_spi);
-int sd_do_command(SDState *sd, SDRequest *req,
+int sd_do_command(SDState *sd, const uint8_t *request,
                   uint8_t *response);
 void sd_write_data(SDState *sd, uint8_t value);
 uint8_t sd_read_data(SDState *sd);
@@ -193,7 +199,7 @@ void sd_enable(SDState *sd, bool enable);
 void sdbus_set_voltage(SDBus *sdbus, uint16_t millivolts);
 uint8_t sdbus_get_dat_lines(SDBus *sdbus);
 bool sdbus_get_cmd_line(SDBus *sdbus);
-int sdbus_do_command(SDBus *sd, SDRequest *req, uint8_t *response);
+int sdbus_do_command(SDBus *sd, const uint8_t *request, uint8_t *response);
 void sdbus_write_data(SDBus *sd, uint8_t value);
 uint8_t sdbus_read_data(SDBus *sd);
 bool sdbus_data_ready(SDBus *sd);
diff --git a/hw/sd/bcm2835_sdhost.c b/hw/sd/bcm2835_sdhost.c
index 4df4de7d67..b637a392b6 100644
--- a/hw/sd/bcm2835_sdhost.c
+++ b/hw/sd/bcm2835_sdhost.c
@@ -106,14 +106,14 @@ static void bcm2835_sdhost_update_irq(BCM2835SDHostState *s)
 
 static void bcm2835_sdhost_send_command(BCM2835SDHostState *s)
 {
-    SDRequest request;
+    uint8_t req[6];
     uint8_t rsp[16];
     int rlen;
 
-    request.cmd = s->cmd & SDCMD_CMD_MASK;
-    request.arg = s->cmdarg;
+    sd_frame48_init(req, sizeof(req), s->cmd & SDCMD_CMD_MASK,
+                    s->cmdarg, false);
 
-    rlen = sdbus_do_command(&s->sdbus, &request, rsp);
+    rlen = sdbus_do_command(&s->sdbus, req, rsp);
     if (rlen < 0) {
         goto error;
     }
diff --git a/hw/sd/core.c b/hw/sd/core.c
index 820345f704..15cae5053c 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -87,15 +87,16 @@ void sdbus_set_voltage(SDBus *sdbus, uint16_t millivolts)
     }
 }
 
-int sdbus_do_command(SDBus *sdbus, SDRequest *req, uint8_t *response)
+int sdbus_do_command(SDBus *sdbus, const uint8_t *request, uint8_t *response)
 {
     SDState *card = get_card(sdbus);
 
-    trace_sdbus_command(sdbus_name(sdbus), req->cmd, req->arg, req->crc);
+    trace_sdbus_command(sdbus_name(sdbus),
+                        request[0] & 0x3f, ldl_be_p(&request[1]), request[5]);
     if (card) {
         SDCardClass *sc = SD_CARD_GET_CLASS(card);
 
-        return sc->do_command(card, req, response);
+        return sc->do_command(card, request, response);
     }
 
     return 0;
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index ff2b92dc64..94bb1ffc6f 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -97,14 +97,8 @@ static void update_pending_bits(MilkymistMemcardState *s)
 
 static void memcard_sd_command(MilkymistMemcardState *s)
 {
-    SDRequest req;
-
-    req.cmd = s->command[0] & 0x3f;
-    req.arg = ldl_be_p(s->command + 1);
-    req.crc = s->command[5];
-
-    s->response[0] = req.cmd;
-    s->response_len = sdbus_do_command(&s->sdbus, &req, s->response + 1);
+    s->response[0] = s->command[0];
+    s->response_len = sdbus_do_command(&s->sdbus, s->command, s->response + 1);
     s->response_read_ptr = 0;
 
     if (s->response_len == 16) {
@@ -117,7 +111,7 @@ static void memcard_sd_command(MilkymistMemcardState *s)
         s->response_len += 2;
     }
 
-    if (req.cmd == 0) {
+    if ((s->command[0] & 0x3f) == 0) {
         /* next write is a dummy byte to clock the initialization of the sd
          * card */
         s->ignore_next_cmd = 1;
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index 51c6c124b2..ca6a2ab2aa 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -113,7 +113,7 @@ static void omap_mmc_command(struct omap_mmc_s *host, int cmd, int dir,
 {
     uint32_t rspstatus, mask;
     int rsplen, timeout;
-    SDRequest request;
+    uint8_t request[6];
     uint8_t response[16];
 
     if (init && cmd == 0) {
@@ -135,11 +135,9 @@ static void omap_mmc_command(struct omap_mmc_s *host, int cmd, int dir,
     mask = 0;
     rspstatus = 0;
 
-    request.cmd = cmd;
-    request.arg = host->arg;
-    request.crc = 0; /* FIXME */
+    sd_frame48_init(request, sizeof(request), cmd, host->arg, false);
 
-    rsplen = sd_do_command(host->card, &request, response);
+    rsplen = sd_do_command(host->card, request, response);
 
     /* TODO: validate CRCs */
     switch (resptype) {
diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
index c9b1a6cb23..d8f6df8726 100644
--- a/hw/sd/pl181.c
+++ b/hw/sd/pl181.c
@@ -172,14 +172,14 @@ static uint32_t pl181_fifo_pop(PL181State *s)
 
 static void pl181_send_command(PL181State *s)
 {
-    SDRequest request;
+    uint8_t request[6];
     uint8_t response[16];
     int rlen;
 
-    request.cmd = s->cmd & PL181_CMD_INDEX;
-    request.arg = s->cmdarg;
-    DPRINTF("Command %d %08x\n", request.cmd, request.arg);
-    rlen = sd_do_command(s->card, &request, response);
+    sd_frame48_init(request, sizeof(request), s->cmd & PL181_CMD_INDEX,
+                    s->cmdarg, false);
+
+    rlen = sd_do_command(s->card, request, response);
     if (rlen < 0)
         goto error;
     if (s->cmd & PL181_CMD_RESPONSE) {
diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 82f8ec0d50..055d140f83 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -216,7 +216,7 @@ static void pxa2xx_mmci_fifo_update(PXA2xxMMCIState *s)
 static void pxa2xx_mmci_wakequeues(PXA2xxMMCIState *s)
 {
     int rsplen, i;
-    SDRequest request;
+    uint8_t request[6];
     uint8_t response[16];
 
     s->active = 1;
@@ -224,11 +224,9 @@ static void pxa2xx_mmci_wakequeues(PXA2xxMMCIState *s)
     s->tx_len = 0;
     s->cmdreq = 0;
 
-    request.cmd = s->cmd;
-    request.arg = s->arg;
-    request.crc = 0;	/* FIXME */
+    sd_frame48_init(request, sizeof(request), s->cmd, s->arg, false);
 
-    rsplen = sdbus_do_command(&s->sdbus, &request, response);
+    rsplen = sdbus_do_command(&s->sdbus, request, response);
     s->intreq |= INT_END_CMD;
 
     memset(s->resp_fifo, 0, sizeof(s->resp_fifo));
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 0dfcaf480c..aaf3a6806a 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -467,11 +467,8 @@ static void sd_set_sdstatus(SDState *sd)
     memset(sd->sd_status, 0, 64);
 }
 
-static bool sd_req_crc_is_valid(SDRequest *req)
+static bool sd_req_crc_is_valid(const void *buffer)
 {
-    uint8_t buffer[5];
-    buffer[0] = 0x40 | req->cmd;
-    stl_be_p(&buffer[1], req->arg);
     return true;
     return sd_frame48_verify_checksum(buffer); /* TODO */
 }
@@ -1619,7 +1616,7 @@ static int cmd_valid_while_locked(SDState *sd, uint8_t cmd)
     return sd_cmd_class[cmd] == 0 || sd_cmd_class[cmd] == 7;
 }
 
-int sd_do_command(SDState *sd, SDRequest *req,
+int sd_do_command(SDState *sd, const uint8_t *request,
                   uint8_t *response) {
     int last_state;
     sd_rsp_type_t rtype;
@@ -1631,10 +1628,10 @@ int sd_do_command(SDState *sd, SDRequest *req,
         return 0;
     }
 
-    cmd = req->cmd;
-    arg = req->arg;
+    cmd = request[0];
+    arg = ldl_be_p(&request[1]);
 
-    if (!sd_req_crc_is_valid(req)) {
+    if (!sd_req_crc_is_valid(request)) {
         sd->card_status |= COM_CRC_ERROR;
         rtype = sd_illegal;
         goto send_response;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index f6fe93f033..554bb059ec 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -330,17 +330,18 @@ static void sdhci_data_transfer(void *opaque);
 
 static void sdhci_send_command(SDHCIState *s)
 {
-    SDRequest request;
+    uint8_t request[6];
     uint8_t response[16];
     int rlen;
 
     s->errintsts = 0;
     s->acmd12errsts = 0;
-    request.cmd = s->cmdreg >> 8;
-    request.arg = s->argument;
 
-    trace_sdhci_send_command(request.cmd, request.arg);
-    rlen = sdbus_do_command(&s->sdbus, &request, response);
+    trace_sdhci_send_command(s->cmdreg >> 8, s->argument);
+    sd_frame48_init(request, sizeof(request), s->cmdreg >> 8,
+                    s->argument, false);
+
+    rlen = sdbus_do_command(&s->sdbus, request, response);
 
     if (s->cmdreg & SDHC_CMD_RESPONSE) {
         if (rlen == 4) {
@@ -386,13 +387,12 @@ static void sdhci_end_transfer(SDHCIState *s)
 {
     /* Automatically send CMD12 to stop transfer if AutoCMD12 enabled */
     if ((s->trnmod & SDHC_TRNS_ACMD12) != 0) {
-        SDRequest request;
+        uint8_t request[6];
         uint8_t response[16];
 
-        request.cmd = 0x0C;
-        request.arg = 0;
-        trace_sdhci_end_transfer(request.cmd, request.arg);
-        sdbus_do_command(&s->sdbus, &request, response);
+        trace_sdhci_end_transfer(12, 0);
+        sd_frame48_init(request, sizeof(request), 12, 0, false);
+        sdbus_do_command(&s->sdbus, request, response);
         /* Auto CMD12 response goes to the upper Response register */
         s->rspreg[3] = ldl_be_p(response);
     }
diff --git a/hw/sd/sdmmc-internal.c b/hw/sd/sdmmc-internal.c
index f709211401..c8475a6e8e 100644
--- a/hw/sd/sdmmc-internal.c
+++ b/hw/sd/sdmmc-internal.c
@@ -92,7 +92,9 @@ static uint8_t sd_crc7(const void *message, size_t width)
 }
 
 enum {
+    CRC7_LENGTH         = 1,
     F48_CONTENT_LENGTH  = 1 /* command */ + 4 /* argument */,
+    F48_SIZE_MAX        = F48_CONTENT_LENGTH + CRC7_LENGTH,
     F136_CONTENT_LENGTH = 15,
 };
 
@@ -117,3 +119,13 @@ bool sd_frame136_verify_checksum(const void *content)
     return sd_frame136_calc_checksum(content)
            == ((const uint8_t *)content)[F136_CONTENT_LENGTH];
 }
+
+void sd_frame48_init(uint8_t *buf, size_t bufsize, uint8_t cmd, uint32_t arg,
+                     bool is_response)
+{
+    assert(bufsize >= F48_SIZE_MAX);
+    buf[0] = (!is_response << 6) | cmd;
+    stl_be_p(&buf[1], arg);
+    /* Zero-initialize the CRC byte to avoid leaking host memory to the guest */
+    buf[F48_CONTENT_LENGTH] = 0x00;
+}
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index dbcff4013d..77e446bb94 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -93,13 +93,15 @@ static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
         return 0xff;
     case SSI_SD_CMDARG:
         if (s->arglen == 4) {
-            SDRequest request;
+            uint8_t request[6];
             uint8_t longresp[16];
             /* FIXME: Check CRC.  */
-            request.cmd = s->cmd;
-            request.arg = ldl_be_p(s->cmdarg);
-            DPRINTF("CMD%d arg 0x%08x\n", s->cmd, request.arg);
-            s->arglen = sdbus_do_command(&s->sdbus, &request, longresp);
+
+            DPRINTF("CMD%d arg 0x%08x\n", s->cmd, ldl_be_p(s->cmdarg));
+            sd_frame48_init(request, sizeof(request), s->cmd,
+                            ldl_be_p(s->cmdarg), false);
+
+            s->arglen = sdbus_do_command(&s->sdbus, request, longresp);
             if (s->arglen <= 0) {
                 s->arglen = 1;
                 s->response[0] = 4;
-- 
2.17.0

[Qemu-devel] [PATCH v2 12/14] sdcard: Add tests to validate the 7-bit CRC checksum of 48-bit SD frame

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/sdcard-test.c | 74 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/tests/sdcard-test.c b/tests/sdcard-test.c
index 81789d1f88..2288a05cdf 100644
--- a/tests/sdcard-test.c
+++ b/tests/sdcard-test.c
@@ -14,6 +14,66 @@
 #include "libqtest.h"
 #include "hw/sd/sd.h"
 
+static void sd_prepare_request48(uint8_t *buf, size_t bufsize,
+                                 uint8_t cmd, uint32_t arg)
+{
+    sd_frame48_init(buf, bufsize, cmd, arg, /* is_resp */ false);
+    buf[5] = sd_frame48_calc_checksum(buf);
+}
+
+static void sd_prepare_response48(uint8_t *buf, size_t bufsize,
+                                  uint8_t cmd, uint32_t arg)
+{
+    sd_frame48_init(buf, bufsize, cmd, arg, /* is_resp */ true);
+    buf[5] = sd_frame48_calc_checksum(buf);
+}
+
+static void test_sd_request_frame_crc7(void)
+{
+    uint8_t req[6];
+
+    /* CMD0 */
+    sd_prepare_request48(req, sizeof(req), 0, 0);
+    g_assert_cmphex(req[5], ==, 0b1001010);
+
+    /* CMD17 */
+    sd_prepare_request48(req, sizeof(req), 17, 0);
+    g_assert_cmphex(req[5], ==, 0b0101010);
+
+    /* APP_CMD */
+    sd_prepare_request48(req, sizeof(req), 55, 0);
+    g_assert_cmphex(req[5], ==, 0x32);
+
+    /* ACMD41 SEND_OP_COND */
+    sd_prepare_request48(req, sizeof(req), 41, 0x00100000);
+    g_assert_cmphex(req[5], ==, 0x5f >> 1);
+
+    /* CMD2 ALL_SEND_CID */
+    sd_prepare_request48(req, sizeof(req), 2, 0);
+    g_assert_cmphex(req[5], ==, 0x4d >> 1);
+
+    g_assert_true(sd_frame48_verify_checksum(req));
+}
+
+static void test_sd_response_frame48_crc7(void)
+{
+    uint8_t resp[6];
+
+    /* response to CMD17 */
+    sd_prepare_response48(resp, sizeof(resp), 17, 0x00000900);
+    g_assert_cmphex(resp[5], ==, 0b0110011);
+
+    /* response to the APP_CMD */
+    sd_prepare_response48(resp, sizeof(resp), 55, 0x00000120);
+    g_assert_cmphex(resp[5], ==, 0x41);
+
+    /* response to CMD3 SEND_RELATIVE_ADDR (Relative Card Address is 0xb368) */
+    sd_prepare_response48(resp, sizeof(resp), 3, 0xb3680500);
+    g_assert_cmphex(resp[5], ==, 0x0c);
+
+    g_assert_true(sd_frame48_verify_checksum(resp));
+}
+
 static void test_sd_response_frame136_crc7(void)
 {
     uint8_t buf[16];
@@ -27,6 +87,17 @@ static void test_sd_response_frame136_crc7(void)
     g_assert_true(sd_frame136_verify_checksum(buf));
 }
 
+static void test_sd_verify_cksum_frame48(void)
+{
+    uint8_t buf[6];
+
+    sd_prepare_request48(buf, sizeof(buf), 42, 0x12345678);
+    g_assert_true(sd_frame48_verify_checksum(buf));
+
+    sd_prepare_response48(buf, sizeof(buf), 69, 0x98765432);
+    g_assert_true(sd_frame48_verify_checksum(buf));
+}
+
 static void test_sd_verify_cksum_frame136(void)
 {
     uint8_t buf[16];
@@ -40,7 +111,10 @@ int main(int argc, char *argv[])
 {
     g_test_init(&argc, &argv, NULL);
 
+    qtest_add_func("sd/prepare_req_crc7", test_sd_request_frame_crc7);
+    qtest_add_func("sd/prepare_resp48_crc7", test_sd_response_frame48_crc7);
     qtest_add_func("sd/prepare_resp136_crc7", test_sd_response_frame136_crc7);
+    qtest_add_func("sd/verify_cksum_frame48", test_sd_verify_cksum_frame48);
     qtest_add_func("sd/verify_cksum_frame136", test_sd_verify_cksum_frame136);
 
     return g_test_run();
-- 
2.17.0

[Qemu-devel] [PATCH v2 13/14] sdcard: Add a "validate-crc" property

[Philippe Mathieu-Daudé]

Since not all modelled controllers use the CRC verification (which is
somehow expensive), let the controller have a configurable property
to enable verification.

So far only the Milkymist controller uses it.

This silent the Coverity warning:

  "Code block is unreachable because of the syntactic structure of the code (CWE-561)"

and fixes the following issue:

- CID1005332 (hw/sd/sd.c::sd_req_crc_validate) Structurally dead code

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/milkymist-memcard.c |  1 +
 hw/sd/sd.c                | 10 +++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index 94bb1ffc6f..f7b6d3b140 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -273,6 +273,7 @@ static void milkymist_memcard_realize(DeviceState *dev, Error **errp)
     blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL;
     carddev = qdev_create(&s->sdbus.qbus, TYPE_SD_CARD);
     qdev_prop_set_drive(carddev, "drive", blk, &err);
+    object_property_set_bool(OBJECT(carddev), true, "validate-crc", &err);
     object_property_set_bool(OBJECT(carddev), true, "realized", &err);
     if (err) {
         error_setg(errp, "failed to init SD card: %s", error_get_pretty(err));
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index aaf3a6806a..0170eb832b 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -93,6 +93,7 @@ struct SDState {
     /* Configurable properties */
     BlockBackend *blk;
     bool spi;
+    bool validate_crc;
 
     uint32_t mode;    /* current card mode, one of SDCardModes */
     int32_t state;    /* current card state, one of SDCardStates */
@@ -467,10 +468,12 @@ static void sd_set_sdstatus(SDState *sd)
     memset(sd->sd_status, 0, 64);
 }
 
-static bool sd_req_crc_is_valid(const void *buffer)
+static bool sd_req_crc_is_valid(SDState *sd, const void *request)
 {
+    if (sd->validate_crc) {
+        return sd_frame48_verify_checksum(request);
+    }
     return true;
-    return sd_frame48_verify_checksum(buffer); /* TODO */
 }
 
 static void sd_response_r1_make(SDState *sd, uint8_t *response)
@@ -1631,7 +1634,7 @@ int sd_do_command(SDState *sd, const uint8_t *request,
     cmd = request[0];
     arg = ldl_be_p(&request[1]);
 
-    if (!sd_req_crc_is_valid(request)) {
+    if (!sd_req_crc_is_valid(sd, request)) {
         sd->card_status |= COM_CRC_ERROR;
         rtype = sd_illegal;
         goto send_response;
@@ -2079,6 +2082,7 @@ static Property sd_properties[] = {
      * board to ensure that ssi transfers only occur when the chip select
      * is asserted.  */
     DEFINE_PROP_BOOL("spi", SDState, spi, false),
+    DEFINE_PROP_BOOL("validate-crc", SDState, validate_crc, false),
     DEFINE_PROP_END_OF_LIST()
 };
 
-- 
2.17.0

[Qemu-devel] [RFC PATCH v2 14/14] hw/sd/ssi-sd: Enable CRC validation

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/ssi-sd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 77e446bb94..0375f0b959 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -95,11 +95,11 @@ static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
         if (s->arglen == 4) {
             uint8_t request[6];
             uint8_t longresp[16];
-            /* FIXME: Check CRC.  */
 
             DPRINTF("CMD%d arg 0x%08x\n", s->cmd, ldl_be_p(s->cmdarg));
             sd_frame48_init(request, sizeof(request), s->cmd,
                             ldl_be_p(s->cmdarg), false);
+            request[5] = sd_frame48_calc_checksum(request);
 
             s->arglen = sdbus_do_command(&s->sdbus, request, longresp);
             if (s->arglen <= 0) {
@@ -257,6 +257,7 @@ static void ssi_sd_realize(SSISlave *d, Error **errp)
         qdev_prop_set_drive(carddev, "drive", blk_by_legacy_dinfo(dinfo), &err);
     }
     object_property_set_bool(OBJECT(carddev), true, "spi", &err);
+    object_property_set_bool(OBJECT(carddev), true, "validate-crc", &err);
     object_property_set_bool(OBJECT(carddev), true, "realized", &err);
     if (err) {
         error_setg(errp, "failed to init SD card: %s", error_get_pretty(err));
-- 
2.17.0

Re: [Qemu-devel] [PATCH v2 02/14] sdcard: Constify sd_crc*()'s message argument

[Alistair Francis]

On Tue, May 8, 2018 at 8:46 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/sd/sd.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 235e0518d6..eb6dd5482e 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -236,11 +236,11 @@ static const int sd_cmd_class[SDMMC_CMD_MAX] = {
>      7,  7, 10,  7,  9,  9,  9,  8,  8, 10,  8,  8,  8,  8,  8,  8,
>  };
>
> -static uint8_t sd_crc7(void *message, size_t width)
> +static uint8_t sd_crc7(const void *message, size_t width)
>  {
>      int i, bit;
>      uint8_t shift_reg = 0x00;
> -    uint8_t *msg = (uint8_t *) message;
> +    const uint8_t *msg = (const uint8_t *)message;
>
>      for (i = 0; i < width; i ++, msg ++)
>          for (bit = 7; bit >= 0; bit --) {
> @@ -252,11 +252,11 @@ static uint8_t sd_crc7(void *message, size_t width)
>      return shift_reg;
>  }
>
> -static uint16_t sd_crc16(void *message, size_t width)
> +static uint16_t sd_crc16(const void *message, size_t width)
>  {
>      int i, bit;
>      uint16_t shift_reg = 0x0000;
> -    uint16_t *msg = (uint16_t *) message;
> +    const uint16_t *msg = (const uint16_t *)message;
>      width <<= 1;
>
>      for (i = 0; i < width; i ++, msg ++)
> --
> 2.17.0
>
>

Re: [Qemu-devel] [PATCH v2 04/14] sdcard: Extract sd_frame48/136_calc_checksum()

[Alistair Francis]

On Tue, May 8, 2018 at 8:46 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> It will help when moving this around for qtesting in the next commit.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 27a70896cd..06607115a7 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -273,6 +273,21 @@ static uint16_t sd_crc16(const void *message, size_t width)
>      return shift_reg;
>  }
>
> +enum {
> +    F48_CONTENT_LENGTH  = 1 /* command */ + 4 /* argument */,
> +    F136_CONTENT_LENGTH = 15,
> +};
> +
> +static uint8_t sd_frame48_calc_checksum(const void *content)
> +{
> +    return sd_crc7(content, F48_CONTENT_LENGTH);
> +}
> +
> +static uint8_t sd_frame136_calc_checksum(const void *content)
> +{
> +    return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
> +}
> +
>  #define OCR_POWER_DELAY_NS      500000 /* 0.5ms */
>
>  FIELD(OCR, VDD_VOLTAGE_WINDOW,          0, 24)
> @@ -352,7 +367,7 @@ static void sd_set_cid(SDState *sd)
>      sd->cid[13] = 0x00 |       /* Manufacture date (MDT) */
>          ((MDT_YR - 2000) / 10);
>      sd->cid[14] = ((MDT_YR % 10) << 4) | MDT_MON;
> -    sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
> +    sd->cid[15] = sd_frame136_calc_checksum(sd->cid);
>  }
>
>  #define HWBLOCK_SHIFT  9                       /* 512 bytes */
> @@ -416,7 +431,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>          sd->csd[13] = 0x40;
>          sd->csd[14] = 0x00;
>      }
> -    sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
> +    sd->csd[15] = sd_frame136_calc_checksum(sd->csd);
>  }
>
>  static void sd_set_rca(SDState *sd)
> @@ -491,7 +506,7 @@ static int sd_req_crc_validate(SDRequest *req)
>      buffer[0] = 0x40 | req->cmd;
>      stl_be_p(&buffer[1], req->arg);
>      return 0;
> -    return sd_crc7(buffer, 5) != req->crc;     /* TODO */
> +    return sd_frame48_calc_checksum(buffer) != req->crc; /* TODO */

This 5 has changed to a 15. Is that on purpose? It should be mentioned
in the commit message if it is.

Alistair

>  }
>
>  static void sd_response_r1_make(SDState *sd, uint8_t *response)
> --
> 2.17.0
>
>

Re: [Qemu-devel] [PATCH v2 07/14] sdcard: Invert the sd_req_crc_is_valid() logic

[Alistair Francis]

On Tue, May 8, 2018 at 8:46 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Let's return TRUE when the CRC is valid.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/sd/sd.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index d8dad94fc4..6fc8daa5b8 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -467,13 +467,13 @@ static void sd_set_sdstatus(SDState *sd)
>      memset(sd->sd_status, 0, 64);
>  }
>
> -static int sd_req_crc_validate(SDRequest *req)
> +static bool sd_req_crc_is_valid(SDRequest *req)
>  {
>      uint8_t buffer[5];
>      buffer[0] = 0x40 | req->cmd;
>      stl_be_p(&buffer[1], req->arg);
> -    return 0;
> -    return sd_frame48_calc_checksum(buffer) != req->crc; /* TODO */
> +    return true;
> +    return sd_frame48_calc_checksum(buffer) == req->crc; /* TODO */
>  }
>
>  static void sd_response_r1_make(SDState *sd, uint8_t *response)
> @@ -1631,7 +1631,7 @@ int sd_do_command(SDState *sd, SDRequest *req,
>          return 0;
>      }
>
> -    if (sd_req_crc_validate(req)) {
> +    if (!sd_req_crc_is_valid(req)) {
>          sd->card_status |= COM_CRC_ERROR;
>          rtype = sd_illegal;
>          goto send_response;
> --
> 2.17.0
>
>

Re: [Qemu-devel] [PATCH v2 04/14] sdcard: Extract sd_frame48/136_calc_checksum()

[Philippe Mathieu-Daudé]

Hi Alistair,

On 05/09/2018 03:00 PM, Alistair Francis wrote:
> On Tue, May 8, 2018 at 8:46 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> It will help when moving this around for qtesting in the next commit.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sd.c | 21 ++++++++++++++++++---
>>  1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 27a70896cd..06607115a7 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -273,6 +273,21 @@ static uint16_t sd_crc16(const void *message, size_t width)
>>      return shift_reg;
>>  }
>>
>> +enum {
>> +    F48_CONTENT_LENGTH  = 1 /* command */ + 4 /* argument */,
>> +    F136_CONTENT_LENGTH = 15,
>> +};
>> +
>> +static uint8_t sd_frame48_calc_checksum(const void *content)
>> +{
>> +    return sd_crc7(content, F48_CONTENT_LENGTH);
>> +}
>> +
>> +static uint8_t sd_frame136_calc_checksum(const void *content)
>> +{
>> +    return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
>> +}
>> +
>>  #define OCR_POWER_DELAY_NS      500000 /* 0.5ms */
>>
>>  FIELD(OCR, VDD_VOLTAGE_WINDOW,          0, 24)
>> @@ -352,7 +367,7 @@ static void sd_set_cid(SDState *sd)
>>      sd->cid[13] = 0x00 |       /* Manufacture date (MDT) */
>>          ((MDT_YR - 2000) / 10);
>>      sd->cid[14] = ((MDT_YR % 10) << 4) | MDT_MON;
>> -    sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
>> +    sd->cid[15] = sd_frame136_calc_checksum(sd->cid);
>>  }
>>
>>  #define HWBLOCK_SHIFT  9                       /* 512 bytes */
>> @@ -416,7 +431,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>>          sd->csd[13] = 0x40;
>>          sd->csd[14] = 0x00;
>>      }
>> -    sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
>> +    sd->csd[15] = sd_frame136_calc_checksum(sd->csd);
>>  }
>>
>>  static void sd_set_rca(SDState *sd)
>> @@ -491,7 +506,7 @@ static int sd_req_crc_validate(SDRequest *req)
>>      buffer[0] = 0x40 | req->cmd;
>>      stl_be_p(&buffer[1], req->arg);
>>      return 0;
>> -    return sd_crc7(buffer, 5) != req->crc;     /* TODO */
>> +    return sd_frame48_calc_checksum(buffer) != req->crc; /* TODO */
> 
> This 5 has changed to a 15. Is that on purpose? It should be mentioned
> in the commit message if it is.

I just extracted this function:

  static uint8_t sd_frame48_calc_checksum(const void *content)
  {
      return sd_crc7(content, F48_CONTENT_LENGTH);
  }

Having:

  enum {
      F48_CONTENT_LENGTH  = 1 /* command */ + 4 /* argument */,

So F48_CONTENT_LENGTH = 5 as previous.

This function is later verified with tests from patch 12 of this series.

> 
> Alistair
> 
>>  }
>>
>>  static void sd_response_r1_make(SDState *sd, uint8_t *response)
>> --
>> 2.17.0
>>
>>

Re: [Qemu-devel] [PATCH v2 04/14] sdcard: Extract sd_frame48/136_calc_checksum()

[Alistair Francis]

On Wed, May 9, 2018 at 12:15 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Alistair,
>
> On 05/09/2018 03:00 PM, Alistair Francis wrote:
>> On Tue, May 8, 2018 at 8:46 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> It will help when moving this around for qtesting in the next commit.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/sd/sd.c | 21 ++++++++++++++++++---
>>>  1 file changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index 27a70896cd..06607115a7 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -273,6 +273,21 @@ static uint16_t sd_crc16(const void *message, size_t width)
>>>      return shift_reg;
>>>  }
>>>
>>> +enum {
>>> +    F48_CONTENT_LENGTH  = 1 /* command */ + 4 /* argument */,
>>> +    F136_CONTENT_LENGTH = 15,
>>> +};
>>> +
>>> +static uint8_t sd_frame48_calc_checksum(const void *content)
>>> +{
>>> +    return sd_crc7(content, F48_CONTENT_LENGTH);
>>> +}
>>> +
>>> +static uint8_t sd_frame136_calc_checksum(const void *content)
>>> +{
>>> +    return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
>>> +}
>>> +
>>>  #define OCR_POWER_DELAY_NS      500000 /* 0.5ms */
>>>
>>>  FIELD(OCR, VDD_VOLTAGE_WINDOW,          0, 24)
>>> @@ -352,7 +367,7 @@ static void sd_set_cid(SDState *sd)
>>>      sd->cid[13] = 0x00 |       /* Manufacture date (MDT) */
>>>          ((MDT_YR - 2000) / 10);
>>>      sd->cid[14] = ((MDT_YR % 10) << 4) | MDT_MON;
>>> -    sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
>>> +    sd->cid[15] = sd_frame136_calc_checksum(sd->cid);
>>>  }
>>>
>>>  #define HWBLOCK_SHIFT  9                       /* 512 bytes */
>>> @@ -416,7 +431,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>>>          sd->csd[13] = 0x40;
>>>          sd->csd[14] = 0x00;
>>>      }
>>> -    sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
>>> +    sd->csd[15] = sd_frame136_calc_checksum(sd->csd);
>>>  }
>>>
>>>  static void sd_set_rca(SDState *sd)
>>> @@ -491,7 +506,7 @@ static int sd_req_crc_validate(SDRequest *req)
>>>      buffer[0] = 0x40 | req->cmd;
>>>      stl_be_p(&buffer[1], req->arg);
>>>      return 0;
>>> -    return sd_crc7(buffer, 5) != req->crc;     /* TODO */
>>> +    return sd_frame48_calc_checksum(buffer) != req->crc; /* TODO */
>>
>> This 5 has changed to a 15. Is that on purpose? It should be mentioned
>> in the commit message if it is.
>
> I just extracted this function:
>
>   static uint8_t sd_frame48_calc_checksum(const void *content)
>   {
>       return sd_crc7(content, F48_CONTENT_LENGTH);
>   }
>
> Having:
>
>   enum {
>       F48_CONTENT_LENGTH  = 1 /* command */ + 4 /* argument */,
>
> So F48_CONTENT_LENGTH = 5 as previous.

Ah, I missed the '+ 4 '. I just stopped reading at the comment.

Looks good then:

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

>
> This function is later verified with tests from patch 12 of this series.
>
>>
>> Alistair
>>
>>>  }
>>>
>>>  static void sd_response_r1_make(SDState *sd, uint8_t *response)
>>> --
>>> 2.17.0
>>>
>>>

Re: [Qemu-devel] [RFC PATCH v2 14/14] hw/sd/ssi-sd: Enable CRC validation

[Alistair Francis]

On Tue, May 8, 2018 at 8:46 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/sd/ssi-sd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index 77e446bb94..0375f0b959 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -95,11 +95,11 @@ static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
>          if (s->arglen == 4) {
>              uint8_t request[6];
>              uint8_t longresp[16];
> -            /* FIXME: Check CRC.  */
>
>              DPRINTF("CMD%d arg 0x%08x\n", s->cmd, ldl_be_p(s->cmdarg));
>              sd_frame48_init(request, sizeof(request), s->cmd,
>                              ldl_be_p(s->cmdarg), false);
> +            request[5] = sd_frame48_calc_checksum(request);
>
>              s->arglen = sdbus_do_command(&s->sdbus, request, longresp);
>              if (s->arglen <= 0) {
> @@ -257,6 +257,7 @@ static void ssi_sd_realize(SSISlave *d, Error **errp)
>          qdev_prop_set_drive(carddev, "drive", blk_by_legacy_dinfo(dinfo), &err);
>      }
>      object_property_set_bool(OBJECT(carddev), true, "spi", &err);
> +    object_property_set_bool(OBJECT(carddev), true, "validate-crc", &err);
>      object_property_set_bool(OBJECT(carddev), true, "realized", &err);
>      if (err) {
>          error_setg(errp, "failed to init SD card: %s", error_get_pretty(err));
> --
> 2.17.0
>
>

Re: [Qemu-devel] [PATCH v2 08/14] sdcard: Extract sd_frame48_verify_checksum() out for qtesting

[Alistair Francis]

On Tue, May 8, 2018 at 8:46 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/sd/sd.h     | 10 ++++++++++
>  hw/sd/sd.c             |  2 +-
>  hw/sd/sdmmc-internal.c |  6 ++++++
>  3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
> index c854ed6a14..752d8edd6c 100644
> --- a/include/hw/sd/sd.h
> +++ b/include/hw/sd/sd.h
> @@ -150,6 +150,16 @@ uint8_t sd_frame48_calc_checksum(const void *content);
>   */
>  uint8_t sd_frame136_calc_checksum(const void *content);
>
> +/**
> + * sd_frame48_verify_checksum:
> + * @content: pointer to the frame content
> + *
> + * Verify the 7-bit CRC checksum of a 48-bit SD frame.
> + *
> + * Returns: A boolean indicating whether the frame 7-bit CRC is correct.
> + */
> +bool sd_frame48_verify_checksum(const void *content);
> +
>  /* Legacy functions to be used only by non-qdevified callers */
>  SDState *sd_init(BlockBackend *bs, bool is_spi);
>  int sd_do_command(SDState *sd, SDRequest *req,
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 6fc8daa5b8..125707a65c 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -473,7 +473,7 @@ static bool sd_req_crc_is_valid(SDRequest *req)
>      buffer[0] = 0x40 | req->cmd;
>      stl_be_p(&buffer[1], req->arg);
>      return true;
> -    return sd_frame48_calc_checksum(buffer) == req->crc; /* TODO */
> +    return sd_frame48_verify_checksum(buffer); /* TODO */
>  }
>
>  static void sd_response_r1_make(SDState *sd, uint8_t *response)
> diff --git a/hw/sd/sdmmc-internal.c b/hw/sd/sdmmc-internal.c
> index a94d65b756..a9d19ce3eb 100644
> --- a/hw/sd/sdmmc-internal.c
> +++ b/hw/sd/sdmmc-internal.c
> @@ -105,3 +105,9 @@ uint8_t sd_frame136_calc_checksum(const void *content)
>  {
>      return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
>  }
> +
> +bool sd_frame48_verify_checksum(const void *content)
> +{
> +    return sd_frame48_calc_checksum(content)
> +           == ((const uint8_t *)content)[F48_CONTENT_LENGTH];
> +}
> --
> 2.17.0
>
>

Re: [Qemu-devel] [PATCH v2 04/14] sdcard: Extract sd_frame48/136_calc_checksum()

[Philippe Mathieu-Daudé]

On 05/09/2018 08:04 PM, Alistair Francis wrote:
> On Wed, May 9, 2018 at 12:15 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Hi Alistair,
>>
>> On 05/09/2018 03:00 PM, Alistair Francis wrote:
>>> On Tue, May 8, 2018 at 8:46 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>> It will help when moving this around for qtesting in the next commit.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>>  hw/sd/sd.c | 21 ++++++++++++++++++---
>>>>  1 file changed, 18 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>> index 27a70896cd..06607115a7 100644
>>>> --- a/hw/sd/sd.c
>>>> +++ b/hw/sd/sd.c
>>>> @@ -273,6 +273,21 @@ static uint16_t sd_crc16(const void *message, size_t width)
>>>>      return shift_reg;
>>>>  }
>>>>
>>>> +enum {
>>>> +    F48_CONTENT_LENGTH  = 1 /* command */ + 4 /* argument */,
>>>> +    F136_CONTENT_LENGTH = 15,
>>>> +};
>>>> +
>>>> +static uint8_t sd_frame48_calc_checksum(const void *content)
>>>> +{
>>>> +    return sd_crc7(content, F48_CONTENT_LENGTH);
>>>> +}
>>>> +
>>>> +static uint8_t sd_frame136_calc_checksum(const void *content)
>>>> +{
>>>> +    return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
>>>> +}
>>>> +
>>>>  #define OCR_POWER_DELAY_NS      500000 /* 0.5ms */
>>>>
>>>>  FIELD(OCR, VDD_VOLTAGE_WINDOW,          0, 24)
>>>> @@ -352,7 +367,7 @@ static void sd_set_cid(SDState *sd)
>>>>      sd->cid[13] = 0x00 |       /* Manufacture date (MDT) */
>>>>          ((MDT_YR - 2000) / 10);
>>>>      sd->cid[14] = ((MDT_YR % 10) << 4) | MDT_MON;
>>>> -    sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
>>>> +    sd->cid[15] = sd_frame136_calc_checksum(sd->cid);
>>>>  }
>>>>
>>>>  #define HWBLOCK_SHIFT  9                       /* 512 bytes */
>>>> @@ -416,7 +431,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>>>>          sd->csd[13] = 0x40;
>>>>          sd->csd[14] = 0x00;
>>>>      }
>>>> -    sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
>>>> +    sd->csd[15] = sd_frame136_calc_checksum(sd->csd);
>>>>  }
>>>>
>>>>  static void sd_set_rca(SDState *sd)
>>>> @@ -491,7 +506,7 @@ static int sd_req_crc_validate(SDRequest *req)
>>>>      buffer[0] = 0x40 | req->cmd;
>>>>      stl_be_p(&buffer[1], req->arg);
>>>>      return 0;
>>>> -    return sd_crc7(buffer, 5) != req->crc;     /* TODO */
>>>> +    return sd_frame48_calc_checksum(buffer) != req->crc; /* TODO */
>>>
>>> This 5 has changed to a 15. Is that on purpose? It should be mentioned
>>> in the commit message if it is.
>>
>> I just extracted this function:
>>
>>   static uint8_t sd_frame48_calc_checksum(const void *content)
>>   {
>>       return sd_crc7(content, F48_CONTENT_LENGTH);
>>   }
>>
>> Having:
>>
>>   enum {
>>       F48_CONTENT_LENGTH  = 1 /* command */ + 4 /* argument */,
>>
>> So F48_CONTENT_LENGTH = 5 as previous.
> 
> Ah, I missed the '+ 4 '. I just stopped reading at the comment.

This way looked clearer to me, but it might not be...
Would this be clearer?

   F48_CONTENT_LENGTH  = 1 + 4 /* command + argument */,

> 
> Looks good then:
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Thanks for your review time :)

> 
> Alistair
> 
>>
>> This function is later verified with tests from patch 12 of this series.
>>
>>>
>>> Alistair
>>>
>>>>  }
>>>>
>>>>  static void sd_response_r1_make(SDState *sd, uint8_t *response)
>>>> --
>>>> 2.17.0
>>>>
>>>>

Re: [Qemu-devel] [PATCH v2 00/14] sdcard: Proper implementation of CRC7

[Peter Maydell]

On 9 May 2018 at 04:46, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi,
>
> This series emerged after last Coverity scan and Peter suggestion in:
> http://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg05046.html
>
>     (3) "proper" implementation of CRC, so that an sd controller
>     can either (a) mark the SDRequest as "no CRC" and have
>     sd_req_crc_validate() always pass, or (b) mark the SDRequest
>     as having a CRC and have sd_req_crc_validate() actually
>     do the check which it currently stubs out with "return 0"
>
> - Coverity issues fixed
> - new functions documented
> - qtests added
>
> This series would be much smaller without qtests (less refactor and code
> movement), but I feel more confident having them passing :)

This series isn't going in the direction I expected.

We have two cases:
 (1) we model an SD controller which in hardware calculates its
     own checksums. Most of our SD controllers are like that.
     Since in QEMU there is no chance of data corruption between
     the controller and the card, there's no point in calculating
     a checksum by calling a function, and then checking that we
     get the same result a few function calls later when we call
     the exact same function in the SD card model. So we should
     just unconditionally say "no checksum provided" in the
     SDRequest struct the controller fills in, and then skip
     the check in the card model.
 (2) we model an SD controller which leaves the checksum calculation
     to guest software. The only one of these we have that I know
     of is milkymist-memcard. In this case, the checksum is calculated
     by guest software, and so we do want to check it in the SD
     card model. So the controller should fill in the CRC field
     in SDRequest, and set the flag in SDRequest to say "checksum
     provided".

We don't need to provide a property on the device or the
card objects to control this behaviour, for either case.

I'm not clear why this patchset has removed the SDRequest struct
in favour of a raw buffer, either: that makes it harder to just
say "this request doesn't have a checksum you need to check".

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 04/14] sdcard: Extract sd_frame48/136_calc_checksum()

[Alistair Francis]

On Wed, May 9, 2018 at 5:16 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 05/09/2018 08:04 PM, Alistair Francis wrote:
>> On Wed, May 9, 2018 at 12:15 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> Hi Alistair,
>>>
>>> On 05/09/2018 03:00 PM, Alistair Francis wrote:
>>>> On Tue, May 8, 2018 at 8:46 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>> It will help when moving this around for qtesting in the next commit.
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> ---
>>>>>  hw/sd/sd.c | 21 ++++++++++++++++++---
>>>>>  1 file changed, 18 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>>> index 27a70896cd..06607115a7 100644
>>>>> --- a/hw/sd/sd.c
>>>>> +++ b/hw/sd/sd.c
>>>>> @@ -273,6 +273,21 @@ static uint16_t sd_crc16(const void *message, size_t width)
>>>>>      return shift_reg;
>>>>>  }
>>>>>
>>>>> +enum {
>>>>> +    F48_CONTENT_LENGTH  = 1 /* command */ + 4 /* argument */,
>>>>> +    F136_CONTENT_LENGTH = 15,
>>>>> +};
>>>>> +
>>>>> +static uint8_t sd_frame48_calc_checksum(const void *content)
>>>>> +{
>>>>> +    return sd_crc7(content, F48_CONTENT_LENGTH);
>>>>> +}
>>>>> +
>>>>> +static uint8_t sd_frame136_calc_checksum(const void *content)
>>>>> +{
>>>>> +    return (sd_crc7(content, F136_CONTENT_LENGTH) << 1) | 1;
>>>>> +}
>>>>> +
>>>>>  #define OCR_POWER_DELAY_NS      500000 /* 0.5ms */
>>>>>
>>>>>  FIELD(OCR, VDD_VOLTAGE_WINDOW,          0, 24)
>>>>> @@ -352,7 +367,7 @@ static void sd_set_cid(SDState *sd)
>>>>>      sd->cid[13] = 0x00 |       /* Manufacture date (MDT) */
>>>>>          ((MDT_YR - 2000) / 10);
>>>>>      sd->cid[14] = ((MDT_YR % 10) << 4) | MDT_MON;
>>>>> -    sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
>>>>> +    sd->cid[15] = sd_frame136_calc_checksum(sd->cid);
>>>>>  }
>>>>>
>>>>>  #define HWBLOCK_SHIFT  9                       /* 512 bytes */
>>>>> @@ -416,7 +431,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>>>>>          sd->csd[13] = 0x40;
>>>>>          sd->csd[14] = 0x00;
>>>>>      }
>>>>> -    sd->csd[15] = (sd_crc7(sd->csd, 15) << 1) | 1;
>>>>> +    sd->csd[15] = sd_frame136_calc_checksum(sd->csd);
>>>>>  }
>>>>>
>>>>>  static void sd_set_rca(SDState *sd)
>>>>> @@ -491,7 +506,7 @@ static int sd_req_crc_validate(SDRequest *req)
>>>>>      buffer[0] = 0x40 | req->cmd;
>>>>>      stl_be_p(&buffer[1], req->arg);
>>>>>      return 0;
>>>>> -    return sd_crc7(buffer, 5) != req->crc;     /* TODO */
>>>>> +    return sd_frame48_calc_checksum(buffer) != req->crc; /* TODO */
>>>>
>>>> This 5 has changed to a 15. Is that on purpose? It should be mentioned
>>>> in the commit message if it is.
>>>
>>> I just extracted this function:
>>>
>>>   static uint8_t sd_frame48_calc_checksum(const void *content)
>>>   {
>>>       return sd_crc7(content, F48_CONTENT_LENGTH);
>>>   }
>>>
>>> Having:
>>>
>>>   enum {
>>>       F48_CONTENT_LENGTH  = 1 /* command */ + 4 /* argument */,
>>>
>>> So F48_CONTENT_LENGTH = 5 as previous.
>>
>> Ah, I missed the '+ 4 '. I just stopped reading at the comment.
>
> This way looked clearer to me, but it might not be...
> Would this be clearer?
>
>    F48_CONTENT_LENGTH  = 1 + 4 /* command + argument */,

I think this is clearer, but the way you have it now is fine as well.

>
>>
>> Looks good then:
>>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Thanks for your review time :)

No worries :)

Alistair

>
>>
>> Alistair
>>
>>>
>>> This function is later verified with tests from patch 12 of this series.
>>>
>>>>
>>>> Alistair
>>>>
>>>>>  }
>>>>>
>>>>>  static void sd_response_r1_make(SDState *sd, uint8_t *response)
>>>>> --
>>>>> 2.17.0
>>>>>
>>>>>

Re: [Qemu-devel] [PATCH v2 13/14] sdcard: Add a "validate-crc" property

[Michael Walle]

Am 2018-05-09 05:46, schrieb Philippe Mathieu-Daudé:
> Since not all modelled controllers use the CRC verification (which is
> somehow expensive), let the controller have a configurable property
> to enable verification.
> 
> So far only the Milkymist controller uses it.
> 
> This silent the Coverity warning:
> 
>   "Code block is unreachable because of the syntactic structure of the
> code (CWE-561)"
> 
> and fixes the following issue:
> 
> - CID1005332 (hw/sd/sd.c::sd_req_crc_validate) Structurally dead code
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Tested-by: Michael Walle <michael@walle.cc>

-michael

Re: [Qemu-devel] [PATCH v2 01/14] sdcard: Use the ldst API

[Peter Maydell]

On 9 May 2018 at 04:46, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> The load/store API will ease further code movement.
>
> Per the Physical Layer Simplified Spec. "3.6 Bus Protocol":
>
>   "In the CMD line the Most Significant Bit (MSB) is transmitted
>    first, the Least Significant Bit (LSB) is the last."
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

This is a nice cleanup so I'm just going to fish this patch
out of the series and apply it to target-arm.next.

thanks
-- PMM