[Qemu-devel] [PATCH 00/20] sdcard: proper implementation of CRC

[Qemu-devel] [PATCH 00/20] sdcard: proper implementation of CRC

[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 (up to patch 6)
- crc16 now works
- new functions documented
- qtests added

Regards,

Phil.

Philippe Mathieu-Daudé (20):
  sdcard: Use the ldst API
  sdcard: Extract sd_calc_frame48_crc7() from sd_req_crc_validate()
  sdcard: Rename the SDRequest as SDFrame48
  sdcard: Add sd_prepare_request[_with_crc]()
  sdcard: Use the sd_prepare_request*() functions
  sdcard: Add a "validate-crc" property
  sdcard: Constify sd_crc*()'s message argument
  sdcard: Fix sd_crc*() style
  sdcard: Expose sd_crc*() functions for QTest use
  sdcard: Expose sd_prepare_request*() functions for QTest use
  sdcard: Add test_sd_request_frame_crc7() qtest (request command CRC7)
  sdcard: Let sd_frame48_crc7_calc() work on response frames
  sdcard: Expose sd_prepare_frame48() for QTest use
  sdcard: Add test_sd_response_frame48_crc7 qtest (command response CRC7)
  sdcard: Add SDFrame136 struct and 136-bit SD response frames functions
  sdcard: Add test_sd_response_frame136_crc7() qtest
  sdcard: Add SDFrameData struct and data frame checksum functions
  sdcard: Fix sd_crc16()
  sdcard: Add test_sd_data_frame_crc16() qtest
  sdcard: Add test_sd_verify_cksum_frame48() qtest

 hw/sd/sdmmc-internal.h    |  24 +++++++
 include/hw/sd/sd.h        | 136 +++++++++++++++++++++++++++++++++++++-
 hw/sd/bcm2835_sdhost.c    |   3 +-
 hw/sd/milkymist-memcard.c |   9 ++-
 hw/sd/omap_mmc.c          |   4 +-
 hw/sd/pxa2xx_mmci.c       |   4 +-
 hw/sd/sd.c                |  48 +++-----------
 hw/sd/sdhci.c             |   6 +-
 hw/sd/sdmmc-internal.c    | 114 ++++++++++++++++++++++++++++++++
 hw/sd/ssi-sd.c            |   6 +-
 tests/sdcard-test.c       | 116 ++++++++++++++++++++++++++++++++
 tests/Makefile.include    |   2 +
 12 files changed, 409 insertions(+), 63 deletions(-)
 create mode 100644 tests/sdcard-test.c

-- 
2.17.0

[Qemu-devel] [PATCH 01/20] 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/milkymist-memcard.c | 3 +--
 hw/sd/ssi-sd.c            | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

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/ssi-sd.c b/hw/sd/ssi-sd.c
index ae04b6641b..0bb26e596d 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) {
-- 
2.17.0

[Qemu-devel] [PATCH 02/20] sdcard: Extract sd_calc_frame48_crc7() from sd_req_crc_validate()

[Philippe Mathieu-Daudé]

Extract calculate() from validate() so we can calculate the CRC
out of the validate() function.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 235e0518d6..861bba197d 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -481,13 +481,18 @@ static void sd_set_sdstatus(SDState *sd)
     memset(sd->sd_status, 0, 64);
 }
 
-static int sd_req_crc_validate(SDRequest *req)
+static uint8_t sd_calc_frame48_crc7(uint8_t cmd, uint32_t arg)
 {
     uint8_t buffer[5];
-    buffer[0] = 0x40 | req->cmd;
-    stl_be_p(&buffer[1], req->arg);
+    buffer[0] = 0x40 | cmd;
+    stl_be_p(&buffer[1], arg);
+    return sd_crc7(buffer, sizeof(buffer));
+}
+
+static int sd_req_crc_validate(SDRequest *req)
+{
     return 0;
-    return sd_crc7(buffer, 5) != req->crc;	/* TODO */
+    return sd_calc_frame48_crc7(req->cmd, req->arg) != req->crc; /* TODO */
 }
 
 static void sd_response_r1_make(SDState *sd, uint8_t *response)
-- 
2.17.0

[Qemu-devel] [PATCH 03/20] sdcard: Rename the SDRequest as SDFrame48

[Philippe Mathieu-Daudé]

SD requests are 48-bit SD frames, while SD responses can be
48-bit or 136-bit frames. The 48-bit response frames share
the same CRC logic than request frames.
Unify the 48-bit framing to reuse the same CRC logic between
requests and responses.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/sd/sd.h | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 9bdb3c9285..f0b41232f7 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -76,11 +76,22 @@ typedef enum {
     sd_adtc,	/* addressed with data transfer */
 } sd_cmd_type_t;
 
-typedef struct {
+/**
+ * SDFrame48: 48 bits commands or responses
+ *
+ * @cmd: request:  command
+ *       response: mirrored command
+ * @arg: request:  address information or parameter
+ *       response: status information, OCR register, RCA
+ * @crc: 7-bit CRC checksum
+ */
+typedef struct SDFrame48 {
     uint8_t cmd;
     uint32_t arg;
     uint8_t crc;
-} SDRequest;
+} SDFrame48;
+
+typedef struct SDFrame48 SDRequest;
 
 typedef struct SDState SDState;
 typedef struct SDBus SDBus;
-- 
2.17.0

[Qemu-devel] [PATCH 04/20] sdcard: Add sd_prepare_request[_with_crc]()

[Philippe Mathieu-Daudé]

The sd_prepare_request*() functions fill all the request frame members.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/sd/sd.h | 27 +++++++++++++++++++++++++++
 hw/sd/sd.c         | 37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index f0b41232f7..7d9c906897 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -141,6 +141,33 @@ typedef struct {
     void (*set_readonly)(DeviceState *dev, bool readonly);
 } SDBusClass;
 
+/**
+ * sd_prepare_request:
+ * @req: the #SDRequest to be filled
+ * @cmd: the SD command
+ * @arg: the SD command argument
+ * @gen_crc: generates the frame CRC7 if true, else fill with zeroes
+ *
+ * Initialize a SD request buffer.
+ *
+ * If @gen_crc the frame checksum will be calculated
+ * and filled in the request buffer.
+ */
+void sd_prepare_request(SDRequest *req, uint8_t cmd, uint32_t arg,
+                        bool gen_crc);
+
+/**
+ * sd_prepare_request_with_crc:
+ * @req: the #SDRequest to be filled
+ * @cmd: the SD command
+ * @arg: the SD command argument
+ * @crc: the calculated request CRC7
+ *
+ * Initialize a SD request buffer.
+ */
+void sd_prepare_request_with_crc(SDRequest *req, uint8_t cmd, uint32_t arg,
+                                 uint8_t crc);
+
 /* 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 861bba197d..be75e118c0 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -489,10 +489,45 @@ static uint8_t sd_calc_frame48_crc7(uint8_t cmd, uint32_t arg)
     return sd_crc7(buffer, sizeof(buffer));
 }
 
+static bool sd_verify_frame48_checksum(SDFrame48 *frame)
+{
+    uint8_t crc = sd_calc_frame48_crc7(frame->cmd, frame->arg);
+
+    return crc == frame->crc;
+}
+
 static int sd_req_crc_validate(SDRequest *req)
 {
     return 0;
-    return sd_calc_frame48_crc7(req->cmd, req->arg) != req->crc; /* TODO */
+    return !sd_verify_frame48_checksum(req); /* TODO */
+}
+
+static void sd_update_frame48_checksum(SDFrame48 *frame)
+{
+    frame->crc = sd_calc_frame48_crc7(frame->cmd, frame->arg);
+}
+
+static void sd_prepare_frame48(SDFrame48 *frame, uint8_t cmd, uint32_t arg,
+                               bool gen_crc)
+{
+    frame->cmd = cmd;
+    frame->arg = arg;
+    frame->crc = 0x00;
+    if (gen_crc) {
+        sd_update_frame48_checksum(frame);
+    }
+}
+
+void sd_prepare_request(SDFrame48 *req, uint8_t cmd, uint32_t arg, bool gen_crc)
+{
+    sd_prepare_frame48(req, cmd, arg, gen_crc);
+}
+
+void sd_prepare_request_with_crc(SDRequest *req, uint8_t cmd, uint32_t arg,
+                                 uint8_t crc)
+{
+    sd_prepare_frame48(req, cmd, arg, /* gen_crc */ false);
+    req->crc = crc;
 }
 
 static void sd_response_r1_make(SDState *sd, uint8_t *response)
-- 
2.17.0

[Qemu-devel] [PATCH 05/20] sdcard: Use the sd_prepare_request*() functions

[Philippe Mathieu-Daudé]

This guaranties all SDRequest members are initialized.

This 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>
---
 hw/sd/bcm2835_sdhost.c    | 3 +--
 hw/sd/milkymist-memcard.c | 7 +++----
 hw/sd/omap_mmc.c          | 4 +---
 hw/sd/pxa2xx_mmci.c       | 4 +---
 hw/sd/sdhci.c             | 6 ++----
 hw/sd/ssi-sd.c            | 5 ++---
 6 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/hw/sd/bcm2835_sdhost.c b/hw/sd/bcm2835_sdhost.c
index ebf3b926c2..5134d7b5c7 100644
--- a/hw/sd/bcm2835_sdhost.c
+++ b/hw/sd/bcm2835_sdhost.c
@@ -110,8 +110,7 @@ static void bcm2835_sdhost_send_command(BCM2835SDHostState *s)
     uint8_t rsp[16];
     int rlen;
 
-    request.cmd = s->cmd & SDCMD_CMD_MASK;
-    request.arg = s->cmdarg;
+    sd_prepare_request(&request, s->cmd & SDCMD_CMD_MASK, s->cmdarg, false);
 
     rlen = sdbus_do_command(&s->sdbus, &request, rsp);
     if (rlen < 0) {
diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index ff2b92dc64..d8cbb7b681 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -99,10 +99,9 @@ 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];
-
+    sd_prepare_request_with_crc(&req, s->command[0] & 0x3f,
+                                ldl_be_p(s->command + 1),
+                                s->command[5]);
     s->response[0] = req.cmd;
     s->response_len = sdbus_do_command(&s->sdbus, &req, s->response + 1);
     s->response_read_ptr = 0;
diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index 5b47cadf11..7b71984115 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -135,9 +135,7 @@ 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_prepare_request(&request, cmd, host->arg, false /* FIXME */);
 
     rsplen = sd_do_command(host->card, &request, response);
 
diff --git a/hw/sd/pxa2xx_mmci.c b/hw/sd/pxa2xx_mmci.c
index 82f8ec0d50..fd5c1e7686 100644
--- a/hw/sd/pxa2xx_mmci.c
+++ b/hw/sd/pxa2xx_mmci.c
@@ -224,9 +224,7 @@ 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_prepare_request(&request, s->cmd, s->arg, false /* FIXME */);
 
     rsplen = sdbus_do_command(&s->sdbus, &request, response);
     s->intreq |= INT_END_CMD;
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 63c44a4ee8..9260f59e80 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -336,8 +336,7 @@ static void sdhci_send_command(SDHCIState *s)
 
     s->errintsts = 0;
     s->acmd12errsts = 0;
-    request.cmd = s->cmdreg >> 8;
-    request.arg = s->argument;
+    sd_prepare_request(&request, s->cmdreg >> 8, s->argument, false);
 
     trace_sdhci_send_command(request.cmd, request.arg);
     rlen = sdbus_do_command(&s->sdbus, &request, response);
@@ -393,8 +392,7 @@ static void sdhci_end_transfer(SDHCIState *s)
         SDRequest request;
         uint8_t response[16];
 
-        request.cmd = 0x0C;
-        request.arg = 0;
+        sd_prepare_request(&request, 12, 0, false);
         trace_sdhci_end_transfer(request.cmd, request.arg);
         sdbus_do_command(&s->sdbus, &request, response);
         /* Auto CMD12 response goes to the upper Response register */
diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 0bb26e596d..c22967170c 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -95,9 +95,8 @@ static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
         if (s->arglen == 4) {
             SDRequest request;
             uint8_t longresp[16];
-            /* FIXME: Check CRC.  */
-            request.cmd = s->cmd;
-            request.arg = ldl_be_p(s->cmdarg);
+            sd_prepare_request(&request, s->cmd,
+                               ldl_be_p(s->cmdarg), false /* FIXME */);
             DPRINTF("CMD%d arg 0x%08x\n", s->cmd, request.arg);
             s->arglen = sdbus_do_command(&s->sdbus, &request, longresp);
             if (s->arglen <= 0) {
-- 
2.17.0

[Qemu-devel] [PATCH 06/20] 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                | 12 ++++++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
index d8cbb7b681..04babc092f 100644
--- a/hw/sd/milkymist-memcard.c
+++ b/hw/sd/milkymist-memcard.c
@@ -278,6 +278,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 be75e118c0..801ddc2cb5 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 */
@@ -496,10 +497,12 @@ static bool sd_verify_frame48_checksum(SDFrame48 *frame)
     return crc == frame->crc;
 }
 
-static int sd_req_crc_validate(SDRequest *req)
+static bool sd_req_crc_validate(SDState *sd, SDRequest *req)
 {
-    return 0;
-    return !sd_verify_frame48_checksum(req); /* TODO */
+    if (sd->validate_crc) {
+        return sd_verify_frame48_checksum(req);
+    }
+    return true;
 }
 
 static void sd_update_frame48_checksum(SDFrame48 *frame)
@@ -1685,7 +1688,7 @@ int sd_do_command(SDState *sd, SDRequest *req,
         return 0;
     }
 
-    if (sd_req_crc_validate(req)) {
+    if (!sd_req_crc_validate(sd, req)) {
         sd->card_status |= COM_CRC_ERROR;
         rtype = sd_illegal;
         goto send_response;
@@ -2134,6 +2137,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] [PATCH 07/20] 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 801ddc2cb5..3708ec1d72 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -237,11 +237,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 --) {
@@ -253,11 +253,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 08/20] 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>
---
 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 3708ec1d72..a28ef8de5e 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -243,12 +243,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;
 }
@@ -260,12 +262,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 09/20] sdcard: Expose sd_crc*() functions for QTest use

[Philippe Mathieu-Daudé]

Take those functions out of hw/sd/sd.c (via "sdmmc-internal.h")
to be able to write QTests for them.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdmmc-internal.h | 22 ++++++++++++++++++++++
 hw/sd/sd.c             | 37 -------------------------------------
 hw/sd/sdmmc-internal.c | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 37 deletions(-)

diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
index 9aa04766fc..62c0ff250d 100644
--- a/hw/sd/sdmmc-internal.h
+++ b/hw/sd/sdmmc-internal.h
@@ -36,4 +36,26 @@ const char *sd_cmd_name(uint8_t cmd);
  */
 const char *sd_acmd_name(uint8_t cmd);
 
+/**
+ * sd_crc7:
+ * @data: pointer to the data buffer
+ * @data_len: data length
+ *
+ * Calculate the 7-bit CRC of a SD frame.
+ *
+ * Returns: The frame CRC.
+ */
+uint8_t sd_crc7(const void *data, size_t data_len);
+
+/**
+ * sd_crc16:
+ * @data: pointer to the data buffer
+ * @data_len: data length
+ *
+ * Calculate the 16-bit CRC of a SD data frame.
+ *
+ * Returns: The frame CRC.
+ */
+uint16_t sd_crc16(const void *data, size_t data_len);
+
 #endif
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a28ef8de5e..11b4606051 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -237,43 +237,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;
-    uint16_t shift_reg = 0x0000;
-    const uint16_t *msg = (const uint16_t *)message;
-    width <<= 1;
-
-    for (i = 0; i < width; i++, msg++) {
-        for (bit = 15; bit >= 0; bit--) {
-            shift_reg <<= 1;
-            if ((shift_reg >> 15) ^ ((*msg >> bit) & 1)) {
-                shift_reg ^= 0x1011;
-            }
-        }
-    }
-
-    return shift_reg;
-}
-
 #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..69ad0a99e6 100644
--- a/hw/sd/sdmmc-internal.c
+++ b/hw/sd/sdmmc-internal.c
@@ -70,3 +70,41 @@ 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 */
+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;
+}
+
+uint16_t sd_crc16(const void *message, size_t width)
+{
+    int i, bit;
+    uint16_t shift_reg = 0x0000;
+    const uint16_t *msg = (const uint16_t *)message;
+    width <<= 1;
+
+    for (i = 0; i < width; i++, msg++) {
+        for (bit = 15; bit >= 0; bit--) {
+            shift_reg <<= 1;
+            if ((shift_reg >> 15) ^ ((*msg >> bit) & 1)) {
+                shift_reg ^= 0x1011;
+            }
+        }
+    }
+
+    return shift_reg;
+}
-- 
2.17.0

[Qemu-devel] [PATCH 10/20] sdcard: Expose sd_prepare_request*() functions for QTest use

[Philippe Mathieu-Daudé]

Take those functions out of hw/sd/sd.c (via "sdmmc-internal.h")
to be able to write QTests for them.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdmmc-internal.h |  2 ++
 include/hw/sd/sd.h     | 18 ++++++++++++++++++
 hw/sd/sd.c             | 43 ------------------------------------------
 hw/sd/sdmmc-internal.c | 43 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 63 insertions(+), 43 deletions(-)

diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
index 62c0ff250d..d672112200 100644
--- a/hw/sd/sdmmc-internal.h
+++ b/hw/sd/sdmmc-internal.h
@@ -10,6 +10,8 @@
 #ifndef SD_INTERNAL_H
 #define SD_INTERNAL_H
 
+#include "hw/sd/sd.h"
+
 #define SDMMC_CMD_MAX 64
 
 /**
diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 7d9c906897..b65107ffe1 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -168,6 +168,24 @@ void sd_prepare_request(SDRequest *req, uint8_t cmd, uint32_t arg,
 void sd_prepare_request_with_crc(SDRequest *req, uint8_t cmd, uint32_t arg,
                                  uint8_t crc);
 
+/**
+ * sd_update_frame48_checksum:
+ * @frame: the #SDFrame48 to verify
+ *
+ * Update the 7-bit CRC checksum of a SD 48-bit frame.
+ */
+void sd_update_frame48_checksum(SDFrame48 *frame);
+
+/**
+ * sd_verify_frame48_checksum:
+ * @frame: the #SDFrame48 to verify
+ *
+ * Verify the 7-bit CRC checksum of a SD 48-bit frame.
+ *
+ * Returns: A boolean indicating whether the frame 7-bit CRC is correct.
+ */
+bool sd_verify_frame48_checksum(SDFrame48 *frame);
+
 /* 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 11b4606051..2d2c31d308 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -449,21 +449,6 @@ static void sd_set_sdstatus(SDState *sd)
     memset(sd->sd_status, 0, 64);
 }
 
-static uint8_t sd_calc_frame48_crc7(uint8_t cmd, uint32_t arg)
-{
-    uint8_t buffer[5];
-    buffer[0] = 0x40 | cmd;
-    stl_be_p(&buffer[1], arg);
-    return sd_crc7(buffer, sizeof(buffer));
-}
-
-static bool sd_verify_frame48_checksum(SDFrame48 *frame)
-{
-    uint8_t crc = sd_calc_frame48_crc7(frame->cmd, frame->arg);
-
-    return crc == frame->crc;
-}
-
 static bool sd_req_crc_validate(SDState *sd, SDRequest *req)
 {
     if (sd->validate_crc) {
@@ -472,34 +457,6 @@ static bool sd_req_crc_validate(SDState *sd, SDRequest *req)
     return true;
 }
 
-static void sd_update_frame48_checksum(SDFrame48 *frame)
-{
-    frame->crc = sd_calc_frame48_crc7(frame->cmd, frame->arg);
-}
-
-static void sd_prepare_frame48(SDFrame48 *frame, uint8_t cmd, uint32_t arg,
-                               bool gen_crc)
-{
-    frame->cmd = cmd;
-    frame->arg = arg;
-    frame->crc = 0x00;
-    if (gen_crc) {
-        sd_update_frame48_checksum(frame);
-    }
-}
-
-void sd_prepare_request(SDFrame48 *req, uint8_t cmd, uint32_t arg, bool gen_crc)
-{
-    sd_prepare_frame48(req, cmd, arg, gen_crc);
-}
-
-void sd_prepare_request_with_crc(SDRequest *req, uint8_t cmd, uint32_t arg,
-                                 uint8_t crc)
-{
-    sd_prepare_frame48(req, cmd, arg, /* gen_crc */ false);
-    req->crc = crc;
-}
-
 static void sd_response_r1_make(SDState *sd, uint8_t *response)
 {
     stl_be_p(response, sd->card_status);
diff --git a/hw/sd/sdmmc-internal.c b/hw/sd/sdmmc-internal.c
index 69ad0a99e6..97f5f71569 100644
--- a/hw/sd/sdmmc-internal.c
+++ b/hw/sd/sdmmc-internal.c
@@ -108,3 +108,46 @@ uint16_t sd_crc16(const void *message, size_t width)
 
     return shift_reg;
 }
+
+static uint8_t sd_calc_frame48_crc7(uint8_t cmd, uint32_t arg)
+{
+    uint8_t buffer[5];
+    buffer[0] = 0x40 | cmd;
+    stl_be_p(&buffer[1], arg);
+    return sd_crc7(buffer, sizeof(buffer));
+}
+
+bool sd_verify_frame48_checksum(SDFrame48 *frame)
+{
+    uint8_t crc = sd_calc_frame48_crc7(frame->cmd, frame->arg);
+
+    return crc == frame->crc;
+}
+
+void sd_update_frame48_checksum(SDFrame48 *frame)
+{
+    frame->crc = sd_calc_frame48_crc7(frame->cmd, frame->arg);
+}
+
+static void sd_prepare_frame48(SDFrame48 *frame, uint8_t cmd, uint32_t arg,
+                               bool gen_crc)
+{
+    frame->cmd = cmd;
+    frame->arg = arg;
+    frame->crc = 0x00;
+    if (gen_crc) {
+        sd_update_frame48_checksum(frame);
+    }
+}
+
+void sd_prepare_request(SDFrame48 *req, uint8_t cmd, uint32_t arg, bool gen_crc)
+{
+    sd_prepare_frame48(req, cmd, arg, gen_crc);
+}
+
+void sd_prepare_request_with_crc(SDRequest *req, uint8_t cmd, uint32_t arg,
+                                 uint8_t crc)
+{
+    sd_prepare_frame48(req, cmd, arg, /* gen_crc */ false);
+    req->crc = crc;
+}
-- 
2.17.0

[Qemu-devel] [PATCH 11/20] sdcard: Add test_sd_request_frame_crc7() qtest (request command CRC7)

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/sdcard-test.c    | 54 ++++++++++++++++++++++++++++++++++++++++++
 tests/Makefile.include |  2 ++
 2 files changed, 56 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..1358ee28c6
--- /dev/null
+++ b/tests/sdcard-test.c
@@ -0,0 +1,54 @@
+/*
+ * 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 sd_prepare_request48(SDFrame48 *frame, uint8_t cmd, uint32_t arg)
+{
+    sd_prepare_request(frame, cmd, arg, /* gen_crc */ true);
+}
+
+static void test_sd_request_frame_crc7(void)
+{
+    SDFrame48 frame;
+
+    /* CMD0 */
+    sd_prepare_request48(&frame, 0, 0);
+    g_assert_cmphex(frame.crc, ==, 0b1001010);
+
+    /* CMD17 */
+    sd_prepare_request48(&frame, 17, 0);
+    g_assert_cmphex(frame.crc, ==, 0b0101010);
+
+    /* APP_CMD */
+    sd_prepare_request48(&frame, 55, 0);
+    g_assert_cmphex(frame.crc, ==, 0x32);
+
+    /* ACMD41 SEND_OP_COND */
+    sd_prepare_request48(&frame, 41, 0x00100000);
+    g_assert_cmphex(frame.crc, ==, 0x5f >> 1);
+
+    /* CMD2 ALL_SEND_CID */
+    sd_prepare_request48(&frame, 2, 0);
+    g_assert_cmphex(frame.crc, ==, 0x4d >> 1);
+}
+
+int main(int argc, char *argv[])
+{
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_add_func("sd/req_crc7", test_sd_request_frame_crc7);
+
+    return g_test_run();
+}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3b9a5e31a2..6a70a1dbab 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -380,6 +380,7 @@ check-qtest-arm-y += tests/test-arm-mptimer$(EXESUF)
 gcov-files-arm-y += hw/timer/arm_mptimer.c
 check-qtest-arm-y += tests/boot-serial-test$(EXESUF)
 check-qtest-arm-y += tests/sdhci-test$(EXESUF)
+check-qtest-arm-y += tests/sdcard-test$(EXESUF)
 
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/sdhci-test$(EXESUF)
@@ -835,6 +836,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 12/20] sdcard: Let sd_frame48_crc7_calc() work on response frames

[Philippe Mathieu-Daudé]

Currently sd_calc_frame48_crc7() only works for request frames, having
the bit 6 hardwire to 1.

Extend it to handle response frames, adding a 'is_response' boolean argument.

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

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index b65107ffe1..b1f865fe20 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -171,20 +171,22 @@ void sd_prepare_request_with_crc(SDRequest *req, uint8_t cmd, uint32_t arg,
 /**
  * sd_update_frame48_checksum:
  * @frame: the #SDFrame48 to verify
+ * @is_response: whether the frame is a command request or response
  *
  * Update the 7-bit CRC checksum of a SD 48-bit frame.
  */
-void sd_update_frame48_checksum(SDFrame48 *frame);
+void sd_update_frame48_checksum(SDFrame48 *frame, bool is_response);
 
 /**
  * sd_verify_frame48_checksum:
  * @frame: the #SDFrame48 to verify
+ * @is_response: whether the frame is a command request or response
  *
  * Verify the 7-bit CRC checksum of a SD 48-bit frame.
  *
  * Returns: A boolean indicating whether the frame 7-bit CRC is correct.
  */
-bool sd_verify_frame48_checksum(SDFrame48 *frame);
+bool sd_verify_frame48_checksum(SDFrame48 *frame, bool is_response);
 
 /* Legacy functions to be used only by non-qdevified callers */
 SDState *sd_init(BlockBackend *bs, bool is_spi);
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 2d2c31d308..5e0b5c2b87 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -452,7 +452,7 @@ static void sd_set_sdstatus(SDState *sd)
 static bool sd_req_crc_validate(SDState *sd, SDRequest *req)
 {
     if (sd->validate_crc) {
-        return sd_verify_frame48_checksum(req);
+        return sd_verify_frame48_checksum(req, false);
     }
     return true;
 }
diff --git a/hw/sd/sdmmc-internal.c b/hw/sd/sdmmc-internal.c
index 97f5f71569..04da81e665 100644
--- a/hw/sd/sdmmc-internal.c
+++ b/hw/sd/sdmmc-internal.c
@@ -109,45 +109,45 @@ uint16_t sd_crc16(const void *message, size_t width)
     return shift_reg;
 }
 
-static uint8_t sd_calc_frame48_crc7(uint8_t cmd, uint32_t arg)
+static uint8_t sd_calc_frame48_crc7(uint8_t cmd, uint32_t arg, bool is_response)
 {
     uint8_t buffer[5];
-    buffer[0] = 0x40 | cmd;
+    buffer[0] = (!is_response << 6) | cmd;
     stl_be_p(&buffer[1], arg);
     return sd_crc7(buffer, sizeof(buffer));
 }
 
-bool sd_verify_frame48_checksum(SDFrame48 *frame)
+bool sd_verify_frame48_checksum(SDFrame48 *frame, bool is_response)
 {
-    uint8_t crc = sd_calc_frame48_crc7(frame->cmd, frame->arg);
+    uint8_t crc = sd_calc_frame48_crc7(frame->cmd, frame->arg, is_response);
 
     return crc == frame->crc;
 }
 
-void sd_update_frame48_checksum(SDFrame48 *frame)
+void sd_update_frame48_checksum(SDFrame48 *frame, bool is_response)
 {
-    frame->crc = sd_calc_frame48_crc7(frame->cmd, frame->arg);
+    frame->crc = sd_calc_frame48_crc7(frame->cmd, frame->arg, is_response);
 }
 
 static void sd_prepare_frame48(SDFrame48 *frame, uint8_t cmd, uint32_t arg,
-                               bool gen_crc)
+                               bool is_response, bool gen_crc)
 {
     frame->cmd = cmd;
     frame->arg = arg;
     frame->crc = 0x00;
     if (gen_crc) {
-        sd_update_frame48_checksum(frame);
+        sd_update_frame48_checksum(frame, is_response);
     }
 }
 
 void sd_prepare_request(SDFrame48 *req, uint8_t cmd, uint32_t arg, bool gen_crc)
 {
-    sd_prepare_frame48(req, cmd, arg, gen_crc);
+    sd_prepare_frame48(req, cmd, arg, /* is_response */ false, gen_crc);
 }
 
 void sd_prepare_request_with_crc(SDRequest *req, uint8_t cmd, uint32_t arg,
                                  uint8_t crc)
 {
-    sd_prepare_frame48(req, cmd, arg, /* gen_crc */ false);
+    sd_prepare_request(req, cmd, arg, /* gen_crc */ false);
     req->crc = crc;
 }
-- 
2.17.0

[Qemu-devel] [PATCH 13/20] sdcard: Expose sd_prepare_frame48() for QTest use

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/sd/sd.h     | 16 ++++++++++++++++
 hw/sd/sdmmc-internal.c |  4 ++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index b1f865fe20..13de1b30c3 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -168,6 +168,22 @@ void sd_prepare_request(SDRequest *req, uint8_t cmd, uint32_t arg,
 void sd_prepare_request_with_crc(SDRequest *req, uint8_t cmd, uint32_t arg,
                                  uint8_t crc);
 
+/**
+ * sd_prepare_response48: Initialize a SD response buffer
+ *
+ * If @gen_crc the frame checksum will be calculated
+ * and filled in the request buffer.
+ *
+ * @req: the #SDRequest to be filled
+ * @cmd: the SD command
+ * @arg: the SD command argument
+ * @is_response: whether the frame is a command request or response
+ * @gen_crc: generates the frame CRC7 if true, else fill with zeroes
+ */
+
+void sd_prepare_frame48(SDFrame48 *frame, uint8_t cmd, uint32_t arg,
+                        bool is_response, bool gen_crc);
+
 /**
  * sd_update_frame48_checksum:
  * @frame: the #SDFrame48 to verify
diff --git a/hw/sd/sdmmc-internal.c b/hw/sd/sdmmc-internal.c
index 04da81e665..c990cc9e8e 100644
--- a/hw/sd/sdmmc-internal.c
+++ b/hw/sd/sdmmc-internal.c
@@ -129,8 +129,8 @@ void sd_update_frame48_checksum(SDFrame48 *frame, bool is_response)
     frame->crc = sd_calc_frame48_crc7(frame->cmd, frame->arg, is_response);
 }
 
-static void sd_prepare_frame48(SDFrame48 *frame, uint8_t cmd, uint32_t arg,
-                               bool is_response, bool gen_crc)
+void sd_prepare_frame48(SDFrame48 *frame, uint8_t cmd, uint32_t arg,
+                        bool is_response, bool gen_crc)
 {
     frame->cmd = cmd;
     frame->arg = arg;
-- 
2.17.0

[Qemu-devel] [PATCH 14/20] sdcard: Add test_sd_response_frame48_crc7 qtest (command response CRC7)

[Philippe Mathieu-Daudé]

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

diff --git a/tests/sdcard-test.c b/tests/sdcard-test.c
index 1358ee28c6..2de0a5a0ee 100644
--- a/tests/sdcard-test.c
+++ b/tests/sdcard-test.c
@@ -19,6 +19,11 @@ static void sd_prepare_request48(SDFrame48 *frame, uint8_t cmd, uint32_t arg)
     sd_prepare_request(frame, cmd, arg, /* gen_crc */ true);
 }
 
+static void sd_prepare_response48(SDFrame48 *frame, uint8_t cmd, uint32_t arg)
+{
+    sd_prepare_frame48(frame, cmd, arg, /* is_resp */ true, /* gen_crc */ true);
+}
+
 static void test_sd_request_frame_crc7(void)
 {
     SDFrame48 frame;
@@ -44,11 +49,29 @@ static void test_sd_request_frame_crc7(void)
     g_assert_cmphex(frame.crc, ==, 0x4d >> 1);
 }
 
+static void test_sd_response_frame48_crc7(void)
+{
+    SDFrame48 frame;
+
+    /* response to CMD17 */
+    sd_prepare_response48(&frame, 17, 0x00000900);
+    g_assert_cmphex(frame.crc, ==, 0b0110011);
+
+    /* response to the APP_CMD */
+    sd_prepare_response48(&frame, 55, 0x00000120);
+    g_assert_cmphex(frame.crc, ==, 0x41);
+
+    /* response to CMD3 SEND_RELATIVE_ADDR (Relative Card Address is 0xb368) */
+    sd_prepare_response48(&frame, 3, 0xb3680500);
+    g_assert_cmphex(frame.crc, ==, 0x0c);
+}
+
 int main(int argc, char *argv[])
 {
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_func("sd/req_crc7", test_sd_request_frame_crc7);
+    qtest_add_func("sd/resp48_crc7", test_sd_response_frame48_crc7);
 
     return g_test_run();
 }
-- 
2.17.0

[Qemu-devel] [PATCH 15/20] sdcard: Add SDFrame136 struct and 136-bit SD response frames functions

[Philippe Mathieu-Daudé]

The 'read CID/CSD register' request returns a 136-bit frame
containing those 128-bit registers.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/sd/sd.h     | 29 +++++++++++++++++++++++++++++
 hw/sd/sdmmc-internal.c | 15 +++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index 13de1b30c3..c76be51b32 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -91,6 +91,17 @@ typedef struct SDFrame48 {
     uint8_t crc;
 } SDFrame48;
 
+/**
+ * SDFrame136: 136 bits CID or CSD responses
+ *
+ * @content: CID or CSD register
+ * @crc:     7-bit CRC checksum
+ */
+typedef struct SDFrame136 {
+    uint8_t content[15];
+    uint8_t crc;
+} SDFrame136;
+
 typedef struct SDFrame48 SDRequest;
 
 typedef struct SDState SDState;
@@ -193,6 +204,14 @@ void sd_prepare_frame48(SDFrame48 *frame, uint8_t cmd, uint32_t arg,
  */
 void sd_update_frame48_checksum(SDFrame48 *frame, bool is_response);
 
+/**
+ * sd_update_frame136_checksum:
+ * @frame: the #SDFrame136 to verify
+ *
+ * Update the 16-bit CRC checksum of a SD 136-bit frame.
+ */
+void sd_update_frame136_checksum(SDFrame136 *frame);
+
 /**
  * sd_verify_frame48_checksum:
  * @frame: the #SDFrame48 to verify
@@ -204,6 +223,16 @@ void sd_update_frame48_checksum(SDFrame48 *frame, bool is_response);
  */
 bool sd_verify_frame48_checksum(SDFrame48 *frame, bool is_response);
 
+/**
+ * sd_verify_frame136_checksum:
+ * @frame: the #SDFrame48 to verify
+ *
+ * Verify the 16-bit CRC checksum of a SD 136-bit frame.
+ *
+ * Returns: A boolean indicating whether the frame 16-bit CRC is correct.
+ */
+bool sd_verify_frame136_checksum(SDFrame136 *frame);
+
 /* 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 c990cc9e8e..68350a2304 100644
--- a/hw/sd/sdmmc-internal.c
+++ b/hw/sd/sdmmc-internal.c
@@ -117,6 +117,11 @@ static uint8_t sd_calc_frame48_crc7(uint8_t cmd, uint32_t arg, bool is_response)
     return sd_crc7(buffer, sizeof(buffer));
 }
 
+static uint8_t sd_calc_frame136_crc7(SDFrame136 *frame)
+{
+    return (sd_crc7(frame->content, sizeof(frame->content)) << 1) | 1;
+}
+
 bool sd_verify_frame48_checksum(SDFrame48 *frame, bool is_response)
 {
     uint8_t crc = sd_calc_frame48_crc7(frame->cmd, frame->arg, is_response);
@@ -124,11 +129,21 @@ bool sd_verify_frame48_checksum(SDFrame48 *frame, bool is_response)
     return crc == frame->crc;
 }
 
+bool sd_verify_frame136_checksum(SDFrame136 *frame)
+{
+    return sd_calc_frame136_crc7(frame) == frame->crc;
+}
+
 void sd_update_frame48_checksum(SDFrame48 *frame, bool is_response)
 {
     frame->crc = sd_calc_frame48_crc7(frame->cmd, frame->arg, is_response);
 }
 
+void sd_update_frame136_checksum(SDFrame136 *frame)
+{
+    frame->crc = (sd_crc7(frame->content, sizeof(frame->content)) << 1) | 1;
+}
+
 void sd_prepare_frame48(SDFrame48 *frame, uint8_t cmd, uint32_t arg,
                         bool is_response, bool gen_crc)
 {
-- 
2.17.0

[Qemu-devel] [PATCH 16/20] sdcard: Add test_sd_response_frame136_crc7() qtest

[Philippe Mathieu-Daudé]

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

diff --git a/tests/sdcard-test.c b/tests/sdcard-test.c
index 2de0a5a0ee..efcfbfc033 100644
--- a/tests/sdcard-test.c
+++ b/tests/sdcard-test.c
@@ -66,12 +66,25 @@ static void test_sd_response_frame48_crc7(void)
     g_assert_cmphex(frame.crc, ==, 0x0c);
 }
 
+static void test_sd_response_frame136_crc7(void)
+{
+    SDFrame136 frame;
+
+    /* response to CMD2 ALL_SEND_CID */
+    memcpy(&frame.content,
+           "\x1d\x41\x44\x53\x44\x20\x20\x20\x10\xa0\x40\x0b\xc1\x00\x88",
+           sizeof(frame.content));
+    sd_update_frame136_checksum(&frame);
+    g_assert_cmphex(frame.crc, ==, 0xad);
+}
+
 int main(int argc, char *argv[])
 {
     g_test_init(&argc, &argv, NULL);
 
     qtest_add_func("sd/req_crc7", test_sd_request_frame_crc7);
     qtest_add_func("sd/resp48_crc7", test_sd_response_frame48_crc7);
+    qtest_add_func("sd/resp136_crc7", test_sd_response_frame136_crc7);
 
     return g_test_run();
 }
-- 
2.17.0

[Qemu-devel] [PATCH 17/20] sdcard: Add SDFrameData struct and data frame checksum functions

[Philippe Mathieu-Daudé]

The block transfers data frames are upto 512 bytes and use a 16-bit CRC.

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

diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
index c76be51b32..6bf9daf559 100644
--- a/include/hw/sd/sd.h
+++ b/include/hw/sd/sd.h
@@ -102,6 +102,17 @@ typedef struct SDFrame136 {
     uint8_t crc;
 } SDFrame136;
 
+/**
+ * SDFrameData: 512 bytes block transfers
+ *
+ * @content: block data
+ * @crc:     16-bit CRC checksum
+ */
+typedef struct SDFrameData {
+    uint8_t content[512];
+    uint16_t crc;
+} SDFrameData;
+
 typedef struct SDFrame48 SDRequest;
 
 typedef struct SDState SDState;
@@ -212,6 +223,14 @@ void sd_update_frame48_checksum(SDFrame48 *frame, bool is_response);
  */
 void sd_update_frame136_checksum(SDFrame136 *frame);
 
+/**
+ * sd_update_framedata_checksum:
+ * @frame: the #SDFrameData to verify
+ *
+ * Update the 16-bit CRC checksum of a SD data frame (up to 512 bytes).
+ */
+void sd_update_framedata_checksum(SDFrameData *frame);
+
 /**
  * sd_verify_frame48_checksum:
  * @frame: the #SDFrame48 to verify
@@ -233,6 +252,16 @@ bool sd_verify_frame48_checksum(SDFrame48 *frame, bool is_response);
  */
 bool sd_verify_frame136_checksum(SDFrame136 *frame);
 
+/**
+ * sd_verify_framedata_checksum:
+ * @frame: the #SDFrameData to verify
+ *
+ * Verify the 16-bit CRC checksum of a SD data frame.
+ *
+ * Returns: A boolean indicating whether the frame 16-bit CRC is correct.
+ */
+bool sd_verify_framedata_checksum(SDFrameData *frame);
+
 /* 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 68350a2304..0e82e69d99 100644
--- a/hw/sd/sdmmc-internal.c
+++ b/hw/sd/sdmmc-internal.c
@@ -134,6 +134,11 @@ bool sd_verify_frame136_checksum(SDFrame136 *frame)
     return sd_calc_frame136_crc7(frame) == frame->crc;
 }
 
+bool sd_verify_framedata_checksum(SDFrameData *frame)
+{
+    return sd_crc16(frame->content, sizeof(frame->content)) == frame->crc;
+}
+
 void sd_update_frame48_checksum(SDFrame48 *frame, bool is_response)
 {
     frame->crc = sd_calc_frame48_crc7(frame->cmd, frame->arg, is_response);
@@ -144,6 +149,11 @@ void sd_update_frame136_checksum(SDFrame136 *frame)
     frame->crc = (sd_crc7(frame->content, sizeof(frame->content)) << 1) | 1;
 }
 
+void sd_update_framedata_checksum(SDFrameData *frame)
+{
+    frame->crc = sd_crc16(frame->content, sizeof(frame->content));
+}
+
 void sd_prepare_frame48(SDFrame48 *frame, uint8_t cmd, uint32_t arg,
                         bool is_response, bool gen_crc)
 {
-- 
2.17.0

[Qemu-devel] [PATCH 18/20] sdcard: Fix sd_crc16()

[Philippe Mathieu-Daudé]

The current sd_crc16() function does not pass the Spec test,
change it by a working one.

Code generated with pycrc v0.9.1 (https://pycrc.org) using the configuration:

 - Width         = 16
 - Poly          = 0x1021
 - XorIn         = 0x0000
 - ReflectIn     = False
 - XorOut        = 0x0000
 - ReflectOut    = False
 - Algorithm     = bit-by-bit-fast

  Copyright of the generated source code
  ======================================

  The code generated by pycrc is not considered a substantial portion of the
  software, therefore the licence does not cover the generated code, and the
  author of pycrc will not claim any copyright on the generated code.

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

diff --git a/hw/sd/sdmmc-internal.c b/hw/sd/sdmmc-internal.c
index 0e82e69d99..d6f9b3b51c 100644
--- a/hw/sd/sdmmc-internal.c
+++ b/hw/sd/sdmmc-internal.c
@@ -90,23 +90,31 @@ uint8_t sd_crc7(const void *message, size_t width)
     return shift_reg;
 }
 
-uint16_t sd_crc16(const void *message, size_t width)
+/* 16 bit XMODEM CRC (polynomial 0x1021) */
+uint16_t sd_crc16(const void *data, size_t data_len)
 {
-    int i, bit;
-    uint16_t shift_reg = 0x0000;
-    const uint16_t *msg = (const uint16_t *)message;
-    width <<= 1;
-
-    for (i = 0; i < width; i++, msg++) {
-        for (bit = 15; bit >= 0; bit--) {
-            shift_reg <<= 1;
-            if ((shift_reg >> 15) ^ ((*msg >> bit) & 1)) {
-                shift_reg ^= 0x1011;
+    const unsigned char *d = (const unsigned char *)data;
+    uint16_t crc = 0x0000;
+    unsigned char c;
+    unsigned int i;
+    bool bit;
+
+    while (data_len--) {
+        c = *d++;
+        for (i = 0x80; i > 0; i >>= 1) {
+            bit = crc & 0x8000;
+            if (c & i) {
+                bit = !bit;
+            }
+            crc <<= 1;
+            if (bit) {
+                crc ^= 0x1021;
             }
         }
+        crc &= 0xffff;
     }
 
-    return shift_reg;
+    return crc;
 }
 
 static uint8_t sd_calc_frame48_crc7(uint8_t cmd, uint32_t arg, bool is_response)
-- 
2.17.0

[Qemu-devel] [PATCH 19/20] sdcard: Add test_sd_data_frame_crc16() qtest

[Philippe Mathieu-Daudé]

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

diff --git a/tests/sdcard-test.c b/tests/sdcard-test.c
index efcfbfc033..c51e8095b6 100644
--- a/tests/sdcard-test.c
+++ b/tests/sdcard-test.c
@@ -78,6 +78,15 @@ static void test_sd_response_frame136_crc7(void)
     g_assert_cmphex(frame.crc, ==, 0xad);
 }
 
+static void test_sd_data_frame_crc16(void)
+{
+    SDFrameData frame;
+
+    memset(frame.content, 0xff, sizeof(frame.content));
+    sd_update_framedata_checksum(&frame);
+    g_assert_cmphex(frame.crc, ==, 0x7fa1);
+}
+
 int main(int argc, char *argv[])
 {
     g_test_init(&argc, &argv, NULL);
@@ -85,6 +94,7 @@ int main(int argc, char *argv[])
     qtest_add_func("sd/req_crc7", test_sd_request_frame_crc7);
     qtest_add_func("sd/resp48_crc7", test_sd_response_frame48_crc7);
     qtest_add_func("sd/resp136_crc7", test_sd_response_frame136_crc7);
+    qtest_add_func("sd/data_crc16", test_sd_data_frame_crc16);
 
     return g_test_run();
 }
-- 
2.17.0

[Qemu-devel] [PATCH 20/20] sdcard: Add test_sd_verify_cksum_frame48() qtest

[Philippe Mathieu-Daudé]

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

diff --git a/tests/sdcard-test.c b/tests/sdcard-test.c
index c51e8095b6..fa45188e48 100644
--- a/tests/sdcard-test.c
+++ b/tests/sdcard-test.c
@@ -47,6 +47,8 @@ static void test_sd_request_frame_crc7(void)
     /* CMD2 ALL_SEND_CID */
     sd_prepare_request48(&frame, 2, 0);
     g_assert_cmphex(frame.crc, ==, 0x4d >> 1);
+
+    g_assert_true(sd_verify_frame48_checksum(&frame, false));
 }
 
 static void test_sd_response_frame48_crc7(void)
@@ -64,6 +66,8 @@ static void test_sd_response_frame48_crc7(void)
     /* response to CMD3 SEND_RELATIVE_ADDR (Relative Card Address is 0xb368) */
     sd_prepare_response48(&frame, 3, 0xb3680500);
     g_assert_cmphex(frame.crc, ==, 0x0c);
+
+    g_assert_true(sd_verify_frame48_checksum(&frame, true));
 }
 
 static void test_sd_response_frame136_crc7(void)
@@ -87,6 +91,17 @@ static void test_sd_data_frame_crc16(void)
     g_assert_cmphex(frame.crc, ==, 0x7fa1);
 }
 
+static void test_sd_verify_cksum_frame48(void)
+{
+    SDFrame48 frame;
+
+    sd_prepare_request48(&frame, 42, 0x12345678);
+    g_assert_true(sd_verify_frame48_checksum(&frame, false));
+
+    sd_prepare_response48(&frame, 69, 0x98765432);
+    g_assert_true(sd_verify_frame48_checksum(&frame, true));
+}
+
 int main(int argc, char *argv[])
 {
     g_test_init(&argc, &argv, NULL);
@@ -95,6 +110,7 @@ int main(int argc, char *argv[])
     qtest_add_func("sd/resp48_crc7", test_sd_response_frame48_crc7);
     qtest_add_func("sd/resp136_crc7", test_sd_response_frame136_crc7);
     qtest_add_func("sd/data_crc16", test_sd_data_frame_crc16);
+    qtest_add_func("sd/verify_cksum_frame48", test_sd_verify_cksum_frame48);
 
     return g_test_run();
 }
-- 
2.17.0

Re: [Qemu-devel] [PATCH 15/20] sdcard: Add SDFrame136 struct and 136-bit SD response frames functions

[Philippe Mathieu-Daudé]

On 05/04/2018 12:59 PM, Philippe Mathieu-Daudé wrote:
> The 'read CID/CSD register' request returns a 136-bit frame
> containing those 128-bit registers.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/sd/sd.h     | 29 +++++++++++++++++++++++++++++
>  hw/sd/sdmmc-internal.c | 15 +++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
> index 13de1b30c3..c76be51b32 100644
> --- a/include/hw/sd/sd.h
> +++ b/include/hw/sd/sd.h
> @@ -91,6 +91,17 @@ typedef struct SDFrame48 {
>      uint8_t crc;
>  } SDFrame48;
>  
> +/**
> + * SDFrame136: 136 bits CID or CSD responses
> + *
> + * @content: CID or CSD register
> + * @crc:     7-bit CRC checksum
> + */
> +typedef struct SDFrame136 {
> +    uint8_t content[15];
> +    uint8_t crc;
> +} SDFrame136;
> +
>  typedef struct SDFrame48 SDRequest;
>  
>  typedef struct SDState SDState;
> @@ -193,6 +204,14 @@ void sd_prepare_frame48(SDFrame48 *frame, uint8_t cmd, uint32_t arg,
>   */
>  void sd_update_frame48_checksum(SDFrame48 *frame, bool is_response);
>  
> +/**
> + * sd_update_frame136_checksum:
> + * @frame: the #SDFrame136 to verify
> + *
> + * Update the 16-bit CRC checksum of a SD 136-bit frame.

Oops this should be:

    "Update the 7-bit CRC checksum of a SD 136-bit frame."

> + */
> +void sd_update_frame136_checksum(SDFrame136 *frame);
> +
>  /**
>   * sd_verify_frame48_checksum:
>   * @frame: the #SDFrame48 to verify
> @@ -204,6 +223,16 @@ void sd_update_frame48_checksum(SDFrame48 *frame, bool is_response);
>   */
>  bool sd_verify_frame48_checksum(SDFrame48 *frame, bool is_response);
>  
> +/**
> + * sd_verify_frame136_checksum:
> + * @frame: the #SDFrame48 to verify
> + *
> + * Verify the 16-bit CRC checksum of a SD 136-bit frame.

And:

    "Verify the 7-bit CRC checksum of a SD 136-bit frame."

> + *
> + * Returns: A boolean indicating whether the frame 16-bit CRC is correct.
> + */
> +bool sd_verify_frame136_checksum(SDFrame136 *frame);
> +
>  /* 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 c990cc9e8e..68350a2304 100644
> --- a/hw/sd/sdmmc-internal.c
> +++ b/hw/sd/sdmmc-internal.c
> @@ -117,6 +117,11 @@ static uint8_t sd_calc_frame48_crc7(uint8_t cmd, uint32_t arg, bool is_response)
>      return sd_crc7(buffer, sizeof(buffer));
>  }
>  
> +static uint8_t sd_calc_frame136_crc7(SDFrame136 *frame)
> +{
> +    return (sd_crc7(frame->content, sizeof(frame->content)) << 1) | 1;
> +}
> +
>  bool sd_verify_frame48_checksum(SDFrame48 *frame, bool is_response)
>  {
>      uint8_t crc = sd_calc_frame48_crc7(frame->cmd, frame->arg, is_response);
> @@ -124,11 +129,21 @@ bool sd_verify_frame48_checksum(SDFrame48 *frame, bool is_response)
>      return crc == frame->crc;
>  }
>  
> +bool sd_verify_frame136_checksum(SDFrame136 *frame)
> +{
> +    return sd_calc_frame136_crc7(frame) == frame->crc;
> +}
> +
>  void sd_update_frame48_checksum(SDFrame48 *frame, bool is_response)
>  {
>      frame->crc = sd_calc_frame48_crc7(frame->cmd, frame->arg, is_response);
>  }
>  
> +void sd_update_frame136_checksum(SDFrame136 *frame)
> +{
> +    frame->crc = (sd_crc7(frame->content, sizeof(frame->content)) << 1) | 1;
> +}
> +
>  void sd_prepare_frame48(SDFrame48 *frame, uint8_t cmd, uint32_t arg,
>                          bool is_response, bool gen_crc)
>  {
> 

Re: [Qemu-devel] [PATCH 06/20] sdcard: Add a "validate-crc" property

[Alistair Francis]

On Fri, May 4, 2018 at 9:00 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> 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>

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

Alistair

> ---
>   hw/sd/milkymist-memcard.c |  1 +
>   hw/sd/sd.c                | 12 ++++++++----
>   2 files changed, 9 insertions(+), 4 deletions(-)

> diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c
> index d8cbb7b681..04babc092f 100644
> --- a/hw/sd/milkymist-memcard.c
> +++ b/hw/sd/milkymist-memcard.c
> @@ -278,6 +278,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 be75e118c0..801ddc2cb5 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 */
> @@ -496,10 +497,12 @@ static bool sd_verify_frame48_checksum(SDFrame48
*frame)
>       return crc == frame->crc;
>   }

> -static int sd_req_crc_validate(SDRequest *req)
> +static bool sd_req_crc_validate(SDState *sd, SDRequest *req)
>   {
> -    return 0;
> -    return !sd_verify_frame48_checksum(req); /* TODO */
> +    if (sd->validate_crc) {
> +        return sd_verify_frame48_checksum(req);
> +    }
> +    return true;
>   }

>   static void sd_update_frame48_checksum(SDFrame48 *frame)
> @@ -1685,7 +1688,7 @@ int sd_do_command(SDState *sd, SDRequest *req,
>           return 0;
>       }

> -    if (sd_req_crc_validate(req)) {
> +    if (!sd_req_crc_validate(sd, req)) {
>           sd->card_status |= COM_CRC_ERROR;
>           rtype = sd_illegal;
>           goto send_response;
> @@ -2134,6 +2137,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

Re: [Qemu-devel] [PATCH 08/20] sdcard: Fix sd_crc*() style

[Alistair Francis]

On Fri, May 4, 2018 at 9:03 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> 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>

Alistair

> ---
>   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 3708ec1d72..a28ef8de5e 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -243,12 +243,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;
>   }
> @@ -260,12 +262,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

Re: [Qemu-devel] [PATCH 17/20] sdcard: Add SDFrameData struct and data frame checksum functions

[Philippe Mathieu-Daudé]

On 05/04/2018 12:59 PM, Philippe Mathieu-Daudé wrote:
> The block transfers data frames are upto 512 bytes and use a 16-bit CRC.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/sd/sd.h     | 29 +++++++++++++++++++++++++++++
>  hw/sd/sdmmc-internal.c | 10 ++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/include/hw/sd/sd.h b/include/hw/sd/sd.h
> index c76be51b32..6bf9daf559 100644
> --- a/include/hw/sd/sd.h
> +++ b/include/hw/sd/sd.h
> @@ -102,6 +102,17 @@ typedef struct SDFrame136 {
>      uint8_t crc;
>  } SDFrame136;
>  
> +/**
> + * SDFrameData: 512 bytes block transfers
> + *
> + * @content: block data
> + * @crc:     16-bit CRC checksum
> + */
> +typedef struct SDFrameData {
> +    uint8_t content[512];
> +    uint16_t crc;
> +} SDFrameData;
> +
>  typedef struct SDFrame48 SDRequest;
>  
>  typedef struct SDState SDState;
> @@ -212,6 +223,14 @@ void sd_update_frame48_checksum(SDFrame48 *frame, bool is_response);
>   */
>  void sd_update_frame136_checksum(SDFrame136 *frame);
>  
> +/**
> + * sd_update_framedata_checksum:
> + * @frame: the #SDFrameData to verify
> + *
> + * Update the 16-bit CRC checksum of a SD data frame (up to 512 bytes).
> + */
> +void sd_update_framedata_checksum(SDFrameData *frame);

Since the frame size is variable, this function needs a size_t argument.

> +
>  /**
>   * sd_verify_frame48_checksum:
>   * @frame: the #SDFrame48 to verify
> @@ -233,6 +252,16 @@ bool sd_verify_frame48_checksum(SDFrame48 *frame, bool is_response);
>   */
>  bool sd_verify_frame136_checksum(SDFrame136 *frame);
>  
> +/**
> + * sd_verify_framedata_checksum:
> + * @frame: the #SDFrameData to verify
> + *
> + * Verify the 16-bit CRC checksum of a SD data frame.
> + *
> + * Returns: A boolean indicating whether the frame 16-bit CRC is correct.
> + */
> +bool sd_verify_framedata_checksum(SDFrameData *frame);

Ditto.

> +
>  /* 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 68350a2304..0e82e69d99 100644
> --- a/hw/sd/sdmmc-internal.c
> +++ b/hw/sd/sdmmc-internal.c
> @@ -134,6 +134,11 @@ bool sd_verify_frame136_checksum(SDFrame136 *frame)
>      return sd_calc_frame136_crc7(frame) == frame->crc;
>  }
>  
> +bool sd_verify_framedata_checksum(SDFrameData *frame)
> +{
> +    return sd_crc16(frame->content, sizeof(frame->content)) == frame->crc;

I'll probably replace sizeof(frame->content) by a 'length' argument,
or add a 'length' member to SDFrameData.

> +}
> +
>  void sd_update_frame48_checksum(SDFrame48 *frame, bool is_response)
>  {
>      frame->crc = sd_calc_frame48_crc7(frame->cmd, frame->arg, is_response);
> @@ -144,6 +149,11 @@ void sd_update_frame136_checksum(SDFrame136 *frame)
>      frame->crc = (sd_crc7(frame->content, sizeof(frame->content)) << 1) | 1;
>  }
>  
> +void sd_update_framedata_checksum(SDFrameData *frame)
> +{
> +    frame->crc = sd_crc16(frame->content, sizeof(frame->content));
> +}
> +
>  void sd_prepare_frame48(SDFrame48 *frame, uint8_t cmd, uint32_t arg,
>                          bool is_response, bool gen_crc)
>  {
> 

Re: [Qemu-devel] [PATCH 00/20] sdcard: proper implementation of CRC

[Philippe Mathieu-Daudé]

On 05/04/2018 12:58 PM, Philippe Mathieu-Daudé wrote:
> 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 (up to patch 6)
> - crc16 now works
> - new functions documented
> - qtests added
> 
> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (20):
>   sdcard: Use the ldst API
>   sdcard: Extract sd_calc_frame48_crc7() from sd_req_crc_validate()
>   sdcard: Rename the SDRequest as SDFrame48
>   sdcard: Add sd_prepare_request[_with_crc]()
>   sdcard: Use the sd_prepare_request*() functions
>   sdcard: Add a "validate-crc" property
>   sdcard: Constify sd_crc*()'s message argument
>   sdcard: Fix sd_crc*() style
>   sdcard: Expose sd_crc*() functions for QTest use
>   sdcard: Expose sd_prepare_request*() functions for QTest use
>   sdcard: Add test_sd_request_frame_crc7() qtest (request command CRC7)
>   sdcard: Let sd_frame48_crc7_calc() work on response frames
>   sdcard: Expose sd_prepare_frame48() for QTest use
>   sdcard: Add test_sd_response_frame48_crc7 qtest (command response CRC7)
>   sdcard: Add SDFrame136 struct and 136-bit SD response frames functions
>   sdcard: Add test_sd_response_frame136_crc7() qtest
>   sdcard: Add SDFrameData struct and data frame checksum functions
>   sdcard: Fix sd_crc16()
>   sdcard: Add test_sd_data_frame_crc16() qtest
>   sdcard: Add test_sd_verify_cksum_frame48() qtest

Please disregard this series, I'll respin a simpler approach.