[RFC PATCH 00/10] hw/sd: Start splitting SD vs SPI protocols

[RFC PATCH 00/10] hw/sd: Start splitting SD vs SPI protocols

[Philippe Mathieu-Daudé]

Hi Cédric,

After our discussion yesterday about how to add support for MMC
(and eMMC) I looked at how to easily add these bus protocols,
which might have commands quite different, avoiding to have big
unreadable if/else statements.

I'm not yet happy enough with the result but it is a starting
point which keeps things still simple.
What I'm wondering is if we could include the command classes
(as another dimension in the array). Also if we could use the
older spec version supported as base set of commands, and if the
user asks for more recent spec version, for each version we
overwrite the array of commands. Thoughts?

Phil.

Philippe Mathieu-Daudé (10):
  hw/sd: When card is in wrong state, log which state it is
  hw/sd: Extract address_in_range() helper, log invalid accesses
  hw/sd: Move proto_name to SDProto structure
  hw/sd: Introduce sd_cmd_handler type
  hw/sd: Add sd_cmd_illegal() handler
  hw/sd: Add sd_cmd_unimplemented() handler
  hw/sd: Add sd_cmd_GO_IDLE_STATE() handler
  hw/sd: Add sd_cmd_SEND_OP_CMD() handler
  hw/sd: Add sd_cmd_ALL_SEND_CID() handler
  hw/sd: Add sd_cmd_SEND_RELATIVE_ADDR() handler

 hw/sd/sd.c | 251 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 143 insertions(+), 108 deletions(-)

-- 
2.31.1

[RFC PATCH 01/10] hw/sd: When card is in wrong state, log which state it is

[Philippe Mathieu-Daudé]

We report the card is in an inconsistent state, but don't precise
in which state it is. Add this information, as it is useful when
debugging problems.

Since we will reuse this code, extract as sd_invalid_state_for_cmd()
helper.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 282d39a7042..288ae059e3d 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -937,6 +937,14 @@ static void sd_lock_command(SDState *sd)
         sd->card_status &= ~CARD_IS_LOCKED;
 }
 
+static sd_rsp_type_t sd_invalid_state_for_cmd(SDState *sd, SDRequest req)
+{
+    qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state: %s\n",
+                  req.cmd, sd_state_name(sd->state));
+
+    return sd_illegal;
+}
+
 static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 {
     uint32_t rca = 0x0000;
@@ -1504,8 +1512,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         return sd_illegal;
     }
 
-    qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state\n", req.cmd);
-    return sd_illegal;
+    return sd_invalid_state_for_cmd(sd, req);
 }
 
 static sd_rsp_type_t sd_app_command(SDState *sd,
-- 
2.31.1

[RFC PATCH 02/10] hw/sd: Extract address_in_range() helper, log invalid accesses

[Philippe Mathieu-Daudé]

Multiple commands have to check the address requested is valid.
Extract this code pattern as a new address_in_range() helper, and
log invalid accesses as guest errors.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 288ae059e3d..d71ec81c22a 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -937,6 +937,18 @@ static void sd_lock_command(SDState *sd)
         sd->card_status &= ~CARD_IS_LOCKED;
 }
 
+static bool address_in_range(SDState *sd, const char *desc,
+                             uint64_t addr, uint32_t length)
+{
+    if (addr + length > sd->size) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s offset %lu > card %lu [%%%u]\n",
+                      desc, addr, sd->size, length);
+        sd->card_status |= ADDRESS_ERROR;
+        return false;
+    }
+    return true;
+}
+
 static sd_rsp_type_t sd_invalid_state_for_cmd(SDState *sd, SDRequest req)
 {
     qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state: %s\n",
@@ -1226,8 +1238,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         switch (sd->state) {
         case sd_transfer_state:
 
-            if (addr + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
+            if (!address_in_range(sd, "READ_BLOCK", addr, sd->blk_len)) {
                 return sd_r1;
             }
 
@@ -1272,8 +1283,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         switch (sd->state) {
         case sd_transfer_state:
 
-            if (addr + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
+            if (!address_in_range(sd, "WRITE_BLOCK", addr, sd->blk_len)) {
                 return sd_r1;
             }
 
@@ -1333,8 +1343,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
         switch (sd->state) {
         case sd_transfer_state:
-            if (addr >= sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
+            if (!address_in_range(sd, "SET_WRITE_PROT", addr, 1)) {
                 return sd_r1b;
             }
 
@@ -1356,8 +1365,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
         switch (sd->state) {
         case sd_transfer_state:
-            if (addr >= sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
+            if (!address_in_range(sd, "CLR_WRITE_PROT", addr, 1)) {
                 return sd_r1b;
             }
 
@@ -1832,8 +1840,8 @@ void sd_write_byte(SDState *sd, uint8_t value)
     case 25:	/* CMD25:  WRITE_MULTIPLE_BLOCK */
         if (sd->data_offset == 0) {
             /* Start of the block - let's check the address is valid */
-            if (sd->data_start + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
+            if (!address_in_range(sd, "WRITE_MULTIPLE_BLOCK",
+                                  sd->data_start, sd->blk_len)) {
                 break;
             }
             if (sd->size <= SDSC_MAX_CAPACITY) {
@@ -2005,8 +2013,8 @@ uint8_t sd_read_byte(SDState *sd)
 
     case 18:	/* CMD18:  READ_MULTIPLE_BLOCK */
         if (sd->data_offset == 0) {
-            if (sd->data_start + io_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
+            if (!address_in_range(sd, "READ_MULTIPLE_BLOCK",
+                                  sd->data_start, io_len)) {
                 return 0x00;
             }
             BLK_READ_BLOCK(sd->data_start, io_len);
-- 
2.31.1

[RFC PATCH 03/10] hw/sd: Move proto_name to SDProto structure

[Philippe Mathieu-Daudé]

Introduce a new structure to hold the bus protocol specific
fields: SDProto. The first field is the protocol name.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index d71ec81c22a..a1cc8ab0be8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -88,6 +88,10 @@ enum SDCardStates {
     sd_disconnect_state,
 };
 
+typedef struct SDProto {
+    const char *name;
+} SDProto;
+
 struct SDState {
     DeviceState parent_obj;
 
@@ -112,6 +116,7 @@ struct SDState {
 
     /* Runtime changeables */
 
+    const SDProto *proto;   /* Bus protocol */
     uint32_t mode;    /* current card mode, one of SDCardModes */
     int32_t state;    /* current card state, one of SDCardStates */
     uint32_t vhs;
@@ -138,7 +143,6 @@ struct SDState {
     qemu_irq readonly_cb;
     qemu_irq inserted_cb;
     QEMUTimer *ocr_power_timer;
-    const char *proto_name;
     bool enable;
     uint8_t dat_lines;
     bool cmd_line;
@@ -951,8 +955,8 @@ static bool address_in_range(SDState *sd, const char *desc,
 
 static sd_rsp_type_t sd_invalid_state_for_cmd(SDState *sd, SDRequest req)
 {
-    qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state: %s\n",
-                  req.cmd, sd_state_name(sd->state));
+    qemu_log_mask(LOG_GUEST_ERROR, "%s: CMD%i in a wrong state: %s\n",
+                  sd->proto->name, req.cmd, sd_state_name(sd->state));
 
     return sd_illegal;
 }
@@ -966,7 +970,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
      * However there is no ACMD55, so we want to trace this particular case.
      */
     if (req.cmd != 55 || sd->expecting_acmd) {
-        trace_sdcard_normal_command(sd->proto_name,
+        trace_sdcard_normal_command(sd->proto->name,
                                     sd_cmd_name(req.cmd), req.cmd,
                                     req.arg, sd_state_name(sd->state));
     }
@@ -1526,7 +1530,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 static sd_rsp_type_t sd_app_command(SDState *sd,
                                     SDRequest req)
 {
-    trace_sdcard_app_command(sd->proto_name, sd_acmd_name(req.cmd),
+    trace_sdcard_app_command(sd->proto->name, sd_acmd_name(req.cmd),
                              req.cmd, req.arg, sd_state_name(sd->state));
     sd->card_status |= APP_CMD;
     switch (req.cmd) {
@@ -1820,7 +1824,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
     if (sd->card_status & (ADDRESS_ERROR | WP_VIOLATION))
         return;
 
-    trace_sdcard_write_data(sd->proto_name,
+    trace_sdcard_write_data(sd->proto->name,
                             sd_acmd_name(sd->current_cmd),
                             sd->current_cmd, value);
     switch (sd->current_cmd) {
@@ -1976,7 +1980,7 @@ uint8_t sd_read_byte(SDState *sd)
 
     io_len = (sd->ocr & (1 << 30)) ? 512 : sd->blk_len;
 
-    trace_sdcard_read_data(sd->proto_name,
+    trace_sdcard_read_data(sd->proto->name,
                            sd_acmd_name(sd->current_cmd),
                            sd->current_cmd, io_len);
     switch (sd->current_cmd) {
@@ -2095,6 +2099,14 @@ void sd_enable(SDState *sd, bool enable)
     sd->enable = enable;
 }
 
+static const SDProto sd_proto_spi = {
+    .name = "SPI",
+};
+
+static const SDProto sd_proto_sd = {
+    .name = "SD",
+};
+
 static void sd_instance_init(Object *obj)
 {
     SDState *sd = SD_CARD(obj);
@@ -2115,7 +2127,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
     SDState *sd = SD_CARD(dev);
     int ret;
 
-    sd->proto_name = sd->spi ? "SPI" : "SD";
+    sd->proto = sd->spi ? &sd_proto_spi : &sd_proto_sd;
 
     switch (sd->spec_version) {
     case SD_PHY_SPECv1_10_VERS
-- 
2.31.1

[RFC PATCH 04/10] hw/sd: Introduce sd_cmd_handler type

[Philippe Mathieu-Daudé]

Add 2 command handler arrays in SDProto, for CMD and ACMD.
Have sd_normal_command() / sd_app_command() use these arrays:
if an command handler is registered, call it, otherwise fall
back to current code base.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a1cc8ab0be8..ce1eec0374f 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -88,8 +88,12 @@ enum SDCardStates {
     sd_disconnect_state,
 };
 
+typedef sd_rsp_type_t (*sd_cmd_handler)(SDState *sd, SDRequest req);
+
 typedef struct SDProto {
     const char *name;
+    sd_cmd_handler cmd[SDMMC_CMD_MAX];
+    sd_cmd_handler acmd[SDMMC_CMD_MAX];
 } SDProto;
 
 struct SDState {
@@ -994,6 +998,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         return sd_illegal;
     }
 
+    if (sd->proto->cmd[req.cmd]) {
+        return sd->proto->cmd[req.cmd](sd, req);
+    }
+
     switch (req.cmd) {
     /* Basic commands (Class 0 and Class 1) */
     case 0:	/* CMD0:   GO_IDLE_STATE */
@@ -1533,6 +1541,11 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
     trace_sdcard_app_command(sd->proto->name, sd_acmd_name(req.cmd),
                              req.cmd, req.arg, sd_state_name(sd->state));
     sd->card_status |= APP_CMD;
+
+    if (sd->proto->acmd[req.cmd]) {
+        return sd->proto->acmd[req.cmd](sd, req);
+    }
+
     switch (req.cmd) {
     case 6:	/* ACMD6:  SET_BUS_WIDTH */
         if (sd->spi) {
-- 
2.31.1

[RFC PATCH 05/10] hw/sd: Add sd_cmd_illegal() handler

[Philippe Mathieu-Daudé]

Log illegal commands as GUEST_ERROR.

Note: we are logging back the SDIO commands (CMD5, CMD52-54).

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index ce1eec0374f..0215bdb3689 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -965,6 +965,14 @@ static sd_rsp_type_t sd_invalid_state_for_cmd(SDState *sd, SDRequest req)
     return sd_illegal;
 }
 
+static sd_rsp_type_t sd_cmd_illegal(SDState *sd, SDRequest req)
+{
+    qemu_log_mask(LOG_GUEST_ERROR, "%s: Unknown CMD%i\n",
+                  sd->proto->name, req.cmd);
+
+    return sd_illegal;
+}
+
 static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 {
     uint32_t rca = 0x0000;
@@ -1017,15 +1025,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 1:	/* CMD1:   SEND_OP_CMD */
-        if (!sd->spi)
-            goto bad_cmd;
-
         sd->state = sd_transfer_state;
         return sd_r1;
 
     case 2:	/* CMD2:   ALL_SEND_CID */
-        if (sd->spi)
-            goto bad_cmd;
         switch (sd->state) {
         case sd_ready_state:
             sd->state = sd_identification_state;
@@ -1037,8 +1040,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 3:	/* CMD3:   SEND_RELATIVE_ADDR */
-        if (sd->spi)
-            goto bad_cmd;
         switch (sd->state) {
         case sd_identification_state:
         case sd_standby_state:
@@ -1052,8 +1053,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 4:	/* CMD4:   SEND_DSR */
-        if (sd->spi)
-            goto bad_cmd;
         switch (sd->state) {
         case sd_standby_state:
             break;
@@ -1063,9 +1062,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         }
         break;
 
-    case 5: /* CMD5: reserved for SDIO cards */
-        return sd_illegal;
-
     case 6:	/* CMD6:   SWITCH_FUNCTION */
         switch (sd->mode) {
         case sd_data_transfer_mode:
@@ -1081,8 +1077,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 7:	/* CMD7:   SELECT/DESELECT_CARD */
-        if (sd->spi)
-            goto bad_cmd;
         switch (sd->state) {
         case sd_standby_state:
             if (sd->rca != rca)
@@ -1212,8 +1206,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 15:	/* CMD15:  GO_INACTIVE_STATE */
-        if (sd->spi)
-            goto bad_cmd;
         switch (sd->mode) {
         case sd_data_transfer_mode:
             if (sd->rca != rca)
@@ -1320,8 +1312,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 26:	/* CMD26:  PROGRAM_CID */
-        if (sd->spi)
-            goto bad_cmd;
         switch (sd->state) {
         case sd_transfer_state:
             sd->state = sd_receivingdata_state;
@@ -1466,15 +1456,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         }
         break;
 
-    case 52 ... 54:
-        /* CMD52, CMD53, CMD54: reserved for SDIO cards
-         * (see the SDIO Simplified Specification V2.0)
-         * Handle as illegal command but do not complain
-         * on stderr, as some OSes may use these in their
-         * probing for presence of an SDIO card.
-         */
-        return sd_illegal;
-
     /* Application specific commands (Class 8) */
     case 55:	/* CMD55:  APP_CMD */
         switch (sd->state) {
@@ -1515,19 +1496,12 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         break;
 
     case 58:    /* CMD58:   READ_OCR (SPI) */
-        if (!sd->spi) {
-            goto bad_cmd;
-        }
         return sd_r3;
 
     case 59:    /* CMD59:   CRC_ON_OFF (SPI) */
-        if (!sd->spi) {
-            goto bad_cmd;
-        }
         return sd_r1;
 
     default:
-    bad_cmd:
         qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", req.cmd);
         return sd_illegal;
     }
@@ -2114,10 +2088,25 @@ void sd_enable(SDState *sd, bool enable)
 
 static const SDProto sd_proto_spi = {
     .name = "SPI",
+    .cmd = {
+        [2 ... 4]   = sd_cmd_illegal,
+        [5]         = sd_cmd_illegal,
+        [7]         = sd_cmd_illegal,
+        [15]        = sd_cmd_illegal,
+        [26]        = sd_cmd_illegal,
+        [52 ... 54] = sd_cmd_illegal,
+    },
 };
 
 static const SDProto sd_proto_sd = {
     .name = "SD",
+    .cmd = {
+        [1]         = sd_cmd_illegal,
+        [5]         = sd_cmd_illegal,
+        [52 ... 54] = sd_cmd_illegal,
+        [58]        = sd_cmd_illegal,
+        [59]        = sd_cmd_illegal,
+    },
 };
 
 static void sd_instance_init(Object *obj)
-- 
2.31.1

[RFC PATCH 06/10] hw/sd: Add sd_cmd_unimplemented() handler

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 0215bdb3689..2647fd26566 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -973,6 +973,15 @@ static sd_rsp_type_t sd_cmd_illegal(SDState *sd, SDRequest req)
     return sd_illegal;
 }
 
+/* Commands that are recognised but not yet implemented. */
+static sd_rsp_type_t sd_cmd_unimplemented(SDState *sd, SDRequest req)
+{
+    qemu_log_mask(LOG_UNIMP, "%s: CMD%i not implemented\n",
+                  sd->proto->name, req.cmd);
+
+    return sd_illegal;
+}
+
 static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 {
     uint32_t rca = 0x0000;
@@ -1522,9 +1531,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
 
     switch (req.cmd) {
     case 6:	/* ACMD6:  SET_BUS_WIDTH */
-        if (sd->spi) {
-            goto unimplemented_spi_cmd;
-        }
         switch (sd->state) {
         case sd_transfer_state:
             sd->sd_status[0] &= 0x3f;
@@ -1655,12 +1661,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
     default:
         /* Fall back to standard commands.  */
         return sd_normal_command(sd, req);
-
-    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);
-        return sd_illegal;
     }
 
     qemu_log_mask(LOG_GUEST_ERROR, "SD: ACMD%i in a wrong state\n", req.cmd);
@@ -2096,6 +2096,9 @@ static const SDProto sd_proto_spi = {
         [26]        = sd_cmd_illegal,
         [52 ... 54] = sd_cmd_illegal,
     },
+    .cmd = {
+        [6]         = sd_cmd_unimplemented,
+    },
 };
 
 static const SDProto sd_proto_sd = {
-- 
2.31.1

[RFC PATCH 07/10] hw/sd: Add sd_cmd_GO_IDLE_STATE() handler

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 2647fd26566..3ef6aca89da 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -982,6 +982,16 @@ static sd_rsp_type_t sd_cmd_unimplemented(SDState *sd, SDRequest req)
     return sd_illegal;
 }
 
+static sd_rsp_type_t sd_cmd_GO_IDLE_STATE(SDState *sd, SDRequest req)
+{
+    if (sd->state != sd_inactive_state) {
+        sd->state = sd_idle_state;
+        sd_reset(DEVICE(sd));
+    }
+
+    return sd->spi ? sd_r1 : sd_r0;
+}
+
 static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 {
     uint32_t rca = 0x0000;
@@ -1021,18 +1031,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
     switch (req.cmd) {
     /* Basic commands (Class 0 and Class 1) */
-    case 0:	/* CMD0:   GO_IDLE_STATE */
-        switch (sd->state) {
-        case sd_inactive_state:
-            return sd->spi ? sd_r1 : sd_r0;
-
-        default:
-            sd->state = sd_idle_state;
-            sd_reset(DEVICE(sd));
-            return sd->spi ? sd_r1 : sd_r0;
-        }
-        break;
-
     case 1:	/* CMD1:   SEND_OP_CMD */
         sd->state = sd_transfer_state;
         return sd_r1;
@@ -2089,6 +2087,7 @@ void sd_enable(SDState *sd, bool enable)
 static const SDProto sd_proto_spi = {
     .name = "SPI",
     .cmd = {
+        [0]         = sd_cmd_GO_IDLE_STATE,
         [2 ... 4]   = sd_cmd_illegal,
         [5]         = sd_cmd_illegal,
         [7]         = sd_cmd_illegal,
@@ -2104,6 +2103,7 @@ static const SDProto sd_proto_spi = {
 static const SDProto sd_proto_sd = {
     .name = "SD",
     .cmd = {
+        [0]         = sd_cmd_GO_IDLE_STATE,
         [1]         = sd_cmd_illegal,
         [5]         = sd_cmd_illegal,
         [52 ... 54] = sd_cmd_illegal,
-- 
2.31.1

[RFC PATCH 08/10] hw/sd: Add sd_cmd_SEND_OP_CMD() handler

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 3ef6aca89da..b63d99f7da0 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -992,6 +992,13 @@ static sd_rsp_type_t sd_cmd_GO_IDLE_STATE(SDState *sd, SDRequest req)
     return sd->spi ? sd_r1 : sd_r0;
 }
 
+static sd_rsp_type_t sd_cmd_SEND_OP_CMD(SDState *sd, SDRequest req)
+{
+    sd->state = sd_transfer_state;
+
+    return sd_r1;
+}
+
 static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 {
     uint32_t rca = 0x0000;
@@ -1031,10 +1038,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
     switch (req.cmd) {
     /* Basic commands (Class 0 and Class 1) */
-    case 1:	/* CMD1:   SEND_OP_CMD */
-        sd->state = sd_transfer_state;
-        return sd_r1;
-
     case 2:	/* CMD2:   ALL_SEND_CID */
         switch (sd->state) {
         case sd_ready_state:
@@ -1579,11 +1582,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
         break;
 
     case 41:	/* ACMD41: SD_APP_OP_COND */
-        if (sd->spi) {
-            /* SEND_OP_CMD */
-            sd->state = sd_transfer_state;
-            return sd_r1;
-        }
         if (sd->state != sd_idle_state) {
             break;
         }
@@ -2088,6 +2086,7 @@ static const SDProto sd_proto_spi = {
     .name = "SPI",
     .cmd = {
         [0]         = sd_cmd_GO_IDLE_STATE,
+        [1]         = sd_cmd_SEND_OP_CMD,
         [2 ... 4]   = sd_cmd_illegal,
         [5]         = sd_cmd_illegal,
         [7]         = sd_cmd_illegal,
@@ -2097,6 +2096,7 @@ static const SDProto sd_proto_spi = {
     },
     .cmd = {
         [6]         = sd_cmd_unimplemented,
+        [41]        = sd_cmd_SEND_OP_CMD,
     },
 };
 
-- 
2.31.1

[RFC PATCH 09/10] hw/sd: Add sd_cmd_ALL_SEND_CID() handler

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b63d99f7da0..70dde5ae0b8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -999,6 +999,17 @@ static sd_rsp_type_t sd_cmd_SEND_OP_CMD(SDState *sd, SDRequest req)
     return sd_r1;
 }
 
+static sd_rsp_type_t sd_cmd_ALL_SEND_CID(SDState *sd, SDRequest req)
+{
+    if (sd->state != sd_ready_state) {
+        return sd_invalid_state_for_cmd(sd, req);
+    }
+
+    sd->state = sd_identification_state;
+
+    return sd_r2_i;
+}
+
 static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 {
     uint32_t rca = 0x0000;
@@ -1038,17 +1049,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
     switch (req.cmd) {
     /* Basic commands (Class 0 and Class 1) */
-    case 2:	/* CMD2:   ALL_SEND_CID */
-        switch (sd->state) {
-        case sd_ready_state:
-            sd->state = sd_identification_state;
-            return sd_r2_i;
-
-        default:
-            break;
-        }
-        break;
-
     case 3:	/* CMD3:   SEND_RELATIVE_ADDR */
         switch (sd->state) {
         case sd_identification_state:
@@ -2105,6 +2105,7 @@ static const SDProto sd_proto_sd = {
     .cmd = {
         [0]         = sd_cmd_GO_IDLE_STATE,
         [1]         = sd_cmd_illegal,
+        [2]         = sd_cmd_ALL_SEND_CID,
         [5]         = sd_cmd_illegal,
         [52 ... 54] = sd_cmd_illegal,
         [58]        = sd_cmd_illegal,
-- 
2.31.1

[RFC PATCH 10/10] hw/sd: Add sd_cmd_SEND_RELATIVE_ADDR() handler

[Philippe Mathieu-Daudé]

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 70dde5ae0b8..43a59b34ee8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1010,6 +1010,20 @@ static sd_rsp_type_t sd_cmd_ALL_SEND_CID(SDState *sd, SDRequest req)
     return sd_r2_i;
 }
 
+static sd_rsp_type_t sd_cmd_SEND_RELATIVE_ADDR(SDState *sd, SDRequest req)
+{
+    switch (sd->state) {
+    case sd_identification_state:
+    case sd_standby_state:
+        sd->state = sd_standby_state;
+        sd_set_rca(sd);
+        return sd_r6;
+
+    default:
+        return sd_invalid_state_for_cmd(sd, req);
+    }
+}
+
 static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 {
     uint32_t rca = 0x0000;
@@ -1049,19 +1063,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
     switch (req.cmd) {
     /* Basic commands (Class 0 and Class 1) */
-    case 3:	/* CMD3:   SEND_RELATIVE_ADDR */
-        switch (sd->state) {
-        case sd_identification_state:
-        case sd_standby_state:
-            sd->state = sd_standby_state;
-            sd_set_rca(sd);
-            return sd_r6;
-
-        default:
-            break;
-        }
-        break;
-
     case 4:	/* CMD4:   SEND_DSR */
         switch (sd->state) {
         case sd_standby_state:
@@ -2106,6 +2107,7 @@ static const SDProto sd_proto_sd = {
         [0]         = sd_cmd_GO_IDLE_STATE,
         [1]         = sd_cmd_illegal,
         [2]         = sd_cmd_ALL_SEND_CID,
+        [3]         = sd_cmd_SEND_RELATIVE_ADDR,
         [5]         = sd_cmd_illegal,
         [52 ... 54] = sd_cmd_illegal,
         [58]        = sd_cmd_illegal,
-- 
2.31.1

Re: [RFC PATCH 01/10] hw/sd: When card is in wrong state, log which state it is

[Bin Meng]

On Thu, Jun 24, 2021 at 10:22 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> We report the card is in an inconsistent state, but don't precise

%s/don't/is not

> in which state it is. Add this information, as it is useful when
> debugging problems.
>
> Since we will reuse this code, extract as sd_invalid_state_for_cmd()
> helper.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [RFC PATCH 02/10] hw/sd: Extract address_in_range() helper, log invalid accesses

[Bin Meng]

On Thu, Jun 24, 2021 at 10:24 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Multiple commands have to check the address requested is valid.
> Extract this code pattern as a new address_in_range() helper, and
> log invalid accesses as guest errors.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [RFC PATCH 03/10] hw/sd: Move proto_name to SDProto structure

[Bin Meng]

On Thu, Jun 24, 2021 at 10:24 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Introduce a new structure to hold the bus protocol specific
> fields: SDProto. The first field is the protocol name.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [RFC PATCH 04/10] hw/sd: Introduce sd_cmd_handler type

[Bin Meng]

On Thu, Jun 24, 2021 at 10:27 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Add 2 command handler arrays in SDProto, for CMD and ACMD.
> Have sd_normal_command() / sd_app_command() use these arrays:
> if an command handler is registered, call it, otherwise fall

a command

> back to current code base.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [RFC PATCH 05/10] hw/sd: Add sd_cmd_illegal() handler

[Bin Meng]

On Thu, Jun 24, 2021 at 10:23 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Log illegal commands as GUEST_ERROR.
>
> Note: we are logging back the SDIO commands (CMD5, CMD52-54).
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 57 ++++++++++++++++++++++--------------------------------
>  1 file changed, 23 insertions(+), 34 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index ce1eec0374f..0215bdb3689 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -965,6 +965,14 @@ static sd_rsp_type_t sd_invalid_state_for_cmd(SDState *sd, SDRequest req)
>      return sd_illegal;
>  }
>
> +static sd_rsp_type_t sd_cmd_illegal(SDState *sd, SDRequest req)
> +{
> +    qemu_log_mask(LOG_GUEST_ERROR, "%s: Unknown CMD%i\n",
> +                  sd->proto->name, req.cmd);
> +
> +    return sd_illegal;
> +}
> +
>  static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>  {
>      uint32_t rca = 0x0000;
> @@ -1017,15 +1025,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>
>      case 1:    /* CMD1:   SEND_OP_CMD */
> -        if (!sd->spi)
> -            goto bad_cmd;
> -
>          sd->state = sd_transfer_state;
>          return sd_r1;
>
>      case 2:    /* CMD2:   ALL_SEND_CID */
> -        if (sd->spi)
> -            goto bad_cmd;
>          switch (sd->state) {
>          case sd_ready_state:
>              sd->state = sd_identification_state;
> @@ -1037,8 +1040,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>
>      case 3:    /* CMD3:   SEND_RELATIVE_ADDR */
> -        if (sd->spi)
> -            goto bad_cmd;
>          switch (sd->state) {
>          case sd_identification_state:
>          case sd_standby_state:
> @@ -1052,8 +1053,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>
>      case 4:    /* CMD4:   SEND_DSR */
> -        if (sd->spi)
> -            goto bad_cmd;
>          switch (sd->state) {
>          case sd_standby_state:
>              break;
> @@ -1063,9 +1062,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          }
>          break;
>
> -    case 5: /* CMD5: reserved for SDIO cards */
> -        return sd_illegal;
> -
>      case 6:    /* CMD6:   SWITCH_FUNCTION */
>          switch (sd->mode) {
>          case sd_data_transfer_mode:
> @@ -1081,8 +1077,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>
>      case 7:    /* CMD7:   SELECT/DESELECT_CARD */
> -        if (sd->spi)
> -            goto bad_cmd;
>          switch (sd->state) {
>          case sd_standby_state:
>              if (sd->rca != rca)
> @@ -1212,8 +1206,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>
>      case 15:   /* CMD15:  GO_INACTIVE_STATE */
> -        if (sd->spi)
> -            goto bad_cmd;
>          switch (sd->mode) {
>          case sd_data_transfer_mode:
>              if (sd->rca != rca)
> @@ -1320,8 +1312,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>
>      case 26:   /* CMD26:  PROGRAM_CID */
> -        if (sd->spi)
> -            goto bad_cmd;
>          switch (sd->state) {
>          case sd_transfer_state:
>              sd->state = sd_receivingdata_state;
> @@ -1466,15 +1456,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          }
>          break;
>
> -    case 52 ... 54:
> -        /* CMD52, CMD53, CMD54: reserved for SDIO cards
> -         * (see the SDIO Simplified Specification V2.0)
> -         * Handle as illegal command but do not complain
> -         * on stderr, as some OSes may use these in their
> -         * probing for presence of an SDIO card.
> -         */
> -        return sd_illegal;
> -
>      /* Application specific commands (Class 8) */
>      case 55:   /* CMD55:  APP_CMD */
>          switch (sd->state) {
> @@ -1515,19 +1496,12 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>
>      case 58:    /* CMD58:   READ_OCR (SPI) */
> -        if (!sd->spi) {
> -            goto bad_cmd;
> -        }
>          return sd_r3;
>
>      case 59:    /* CMD59:   CRC_ON_OFF (SPI) */
> -        if (!sd->spi) {
> -            goto bad_cmd;
> -        }
>          return sd_r1;
>
>      default:
> -    bad_cmd:
>          qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", req.cmd);
>          return sd_illegal;
>      }
> @@ -2114,10 +2088,25 @@ void sd_enable(SDState *sd, bool enable)
>
>  static const SDProto sd_proto_spi = {
>      .name = "SPI",
> +    .cmd = {
> +        [2 ... 4]   = sd_cmd_illegal,
> +        [5]         = sd_cmd_illegal,

Above 2 can be merged into [2 ... 5]

> +        [7]         = sd_cmd_illegal,
> +        [15]        = sd_cmd_illegal,
> +        [26]        = sd_cmd_illegal,
> +        [52 ... 54] = sd_cmd_illegal,
> +    },
>  };
>
>  static const SDProto sd_proto_sd = {
>      .name = "SD",
> +    .cmd = {
> +        [1]         = sd_cmd_illegal,
> +        [5]         = sd_cmd_illegal,
> +        [52 ... 54] = sd_cmd_illegal,
> +        [58]        = sd_cmd_illegal,
> +        [59]        = sd_cmd_illegal,
> +    },
>  };
>
>  static void sd_instance_init(Object *obj)

Otherwise LGTM:
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [RFC PATCH 06/10] hw/sd: Add sd_cmd_unimplemented() handler

[Bin Meng]

On Thu, Jun 24, 2021 at 10:28 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 0215bdb3689..2647fd26566 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -973,6 +973,15 @@ static sd_rsp_type_t sd_cmd_illegal(SDState *sd, SDRequest req)
>      return sd_illegal;
>  }
>
> +/* Commands that are recognised but not yet implemented. */
> +static sd_rsp_type_t sd_cmd_unimplemented(SDState *sd, SDRequest req)
> +{
> +    qemu_log_mask(LOG_UNIMP, "%s: CMD%i not implemented\n",
> +                  sd->proto->name, req.cmd);
> +
> +    return sd_illegal;
> +}
> +
>  static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>  {
>      uint32_t rca = 0x0000;
> @@ -1522,9 +1531,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>
>      switch (req.cmd) {
>      case 6:    /* ACMD6:  SET_BUS_WIDTH */
> -        if (sd->spi) {
> -            goto unimplemented_spi_cmd;
> -        }
>          switch (sd->state) {
>          case sd_transfer_state:
>              sd->sd_status[0] &= 0x3f;
> @@ -1655,12 +1661,6 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>      default:
>          /* Fall back to standard commands.  */
>          return sd_normal_command(sd, req);
> -
> -    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);
> -        return sd_illegal;
>      }
>
>      qemu_log_mask(LOG_GUEST_ERROR, "SD: ACMD%i in a wrong state\n", req.cmd);
> @@ -2096,6 +2096,9 @@ static const SDProto sd_proto_spi = {
>          [26]        = sd_cmd_illegal,
>          [52 ... 54] = sd_cmd_illegal,
>      },
> +    .cmd = {
> +        [6]         = sd_cmd_unimplemented,
> +    },
>  };

Does this compile? Or is this another GCC extension to C?

But I think you wanted to say .acmd = ?

Regards,
Bin

Re: [RFC PATCH 07/10] hw/sd: Add sd_cmd_GO_IDLE_STATE() handler

[Bin Meng]

On Thu, Jun 24, 2021 at 10:29 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [RFC PATCH 09/10] hw/sd: Add sd_cmd_ALL_SEND_CID() handler

[Bin Meng]

On Thu, Jun 24, 2021 at 10:23 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [RFC PATCH 10/10] hw/sd: Add sd_cmd_SEND_RELATIVE_ADDR() handler

[Bin Meng]

On Thu, Jun 24, 2021 at 10:28 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [RFC PATCH 06/10] hw/sd: Add sd_cmd_unimplemented() handler

[Philippe Mathieu-Daudé]

On 6/25/21 3:49 PM, Bin Meng wrote:
> On Thu, Jun 24, 2021 at 10:28 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sd.c | 21 ++++++++++++---------
>>  1 file changed, 12 insertions(+), 9 deletions(-)

>>      qemu_log_mask(LOG_GUEST_ERROR, "SD: ACMD%i in a wrong state\n", req.cmd);
>> @@ -2096,6 +2096,9 @@ static const SDProto sd_proto_spi = {
>>          [26]        = sd_cmd_illegal,
>>          [52 ... 54] = sd_cmd_illegal,
>>      },
>> +    .cmd = {
>> +        [6]         = sd_cmd_unimplemented,
>> +    },
>>  };
> 
> Does this compile?

Yes.

> Or is this another GCC extension to C?

I haven't checked when this was introduced, but QEMU uses it since
quite some time now.

Apparently this is:
https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

 "In ISO C99 you can give the elements in any order, specifying
  the array indices or structure field names they apply to, and
  GNU C allows this as an extension in C90 mode as well."

> But I think you wanted to say .acmd = ?

Oops!

Thanks for the review,

Phil.

Re: [RFC PATCH 06/10] hw/sd: Add sd_cmd_unimplemented() handler

[Bin Meng]

On Sat, Jun 26, 2021 at 1:17 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 6/25/21 3:49 PM, Bin Meng wrote:
> > On Thu, Jun 24, 2021 at 10:28 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >>  hw/sd/sd.c | 21 ++++++++++++---------
> >>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> >>      qemu_log_mask(LOG_GUEST_ERROR, "SD: ACMD%i in a wrong state\n", req.cmd);
> >> @@ -2096,6 +2096,9 @@ static const SDProto sd_proto_spi = {
> >>          [26]        = sd_cmd_illegal,
> >>          [52 ... 54] = sd_cmd_illegal,
> >>      },
> >> +    .cmd = {
> >> +        [6]         = sd_cmd_unimplemented,
> >> +    },
> >>  };
> >
> > Does this compile?
>
> Yes.
>
> > Or is this another GCC extension to C?
>
> I haven't checked when this was introduced, but QEMU uses it since
> quite some time now.
>
> Apparently this is:
> https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

Yep, I know designated initialization of a C array, but I don't know
gcc does not complain two .cmd here

>
>  "In ISO C99 you can give the elements in any order, specifying
>   the array indices or structure field names they apply to, and
>   GNU C allows this as an extension in C90 mode as well."
>
> > But I think you wanted to say .acmd = ?
>
> Oops!
>
> Thanks for the review,

Regards,
Bin

Re: [RFC PATCH 06/10] hw/sd: Add sd_cmd_unimplemented() handler

[Philippe Mathieu-Daudé]

On 6/26/21 5:31 AM, Bin Meng wrote:
> On Sat, Jun 26, 2021 at 1:17 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 6/25/21 3:49 PM, Bin Meng wrote:
>>> On Thu, Jun 24, 2021 at 10:28 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>>  hw/sd/sd.c | 21 ++++++++++++---------
>>>>  1 file changed, 12 insertions(+), 9 deletions(-)
>>
>>>>      qemu_log_mask(LOG_GUEST_ERROR, "SD: ACMD%i in a wrong state\n", req.cmd);
>>>> @@ -2096,6 +2096,9 @@ static const SDProto sd_proto_spi = {
>>>>          [26]        = sd_cmd_illegal,
>>>>          [52 ... 54] = sd_cmd_illegal,
>>>>      },
>>>> +    .cmd = {
>>>> +        [6]         = sd_cmd_unimplemented,
>>>> +    },
>>>>  };
>>>
>>> Does this compile?
>>
>> Yes.
>>
>>> Or is this another GCC extension to C?
>>
>> I haven't checked when this was introduced, but QEMU uses it since
>> quite some time now.
>>
>> Apparently this is:
>> https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
> 
> Yep, I know designated initialization of a C array, but I don't know
> gcc does not complain two .cmd here

IIUC GCC would warn if we were using -Woverride-init (but we are not):

   -Woverride-init (C and Objective-C only)

       Warn if an initialized field without side effects
       is overridden when using designated initializers.

>>  "In ISO C99 you can give the elements in any order, specifying
>>   the array indices or structure field names they apply to, and
>>   GNU C allows this as an extension in C90 mode as well."
>>
>>> But I think you wanted to say .acmd = ?
>>
>> Oops!
>>
>> Thanks for the review,
> 
> Regards,
> Bin
> 

Re: [RFC PATCH 05/10] hw/sd: Add sd_cmd_illegal() handler

[Philippe Mathieu-Daudé]

On 6/25/21 3:47 PM, Bin Meng wrote:
> On Thu, Jun 24, 2021 at 10:23 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Log illegal commands as GUEST_ERROR.
>>
>> Note: we are logging back the SDIO commands (CMD5, CMD52-54).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sd.c | 57 ++++++++++++++++++++++--------------------------------
>>  1 file changed, 23 insertions(+), 34 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index ce1eec0374f..0215bdb3689 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -965,6 +965,14 @@ static sd_rsp_type_t sd_invalid_state_for_cmd(SDState *sd, SDRequest req)
>>      return sd_illegal;
>>  }
>>
>> +static sd_rsp_type_t sd_cmd_illegal(SDState *sd, SDRequest req)
>> +{
>> +    qemu_log_mask(LOG_GUEST_ERROR, "%s: Unknown CMD%i\n",
>> +                  sd->proto->name, req.cmd);
>> +
>> +    return sd_illegal;
>> +}
>> +
>>  static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>>  {
>>      uint32_t rca = 0x0000;
>> @@ -1017,15 +1025,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>>          break;
>>
>>      case 1:    /* CMD1:   SEND_OP_CMD */
>> -        if (!sd->spi)
>> -            goto bad_cmd;
>> -
>>          sd->state = sd_transfer_state;
>>          return sd_r1;
>>
>>      case 2:    /* CMD2:   ALL_SEND_CID */
>> -        if (sd->spi)
>> -            goto bad_cmd;
>>          switch (sd->state) {
>>          case sd_ready_state:
>>              sd->state = sd_identification_state;
>> @@ -1037,8 +1040,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>>          break;
>>
>>      case 3:    /* CMD3:   SEND_RELATIVE_ADDR */
>> -        if (sd->spi)
>> -            goto bad_cmd;
>>          switch (sd->state) {
>>          case sd_identification_state:
>>          case sd_standby_state:
>> @@ -1052,8 +1053,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>>          break;
>>
>>      case 4:    /* CMD4:   SEND_DSR */
>> -        if (sd->spi)
>> -            goto bad_cmd;
>>          switch (sd->state) {
>>          case sd_standby_state:
>>              break;
>> @@ -1063,9 +1062,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>>          }
>>          break;
>>
>> -    case 5: /* CMD5: reserved for SDIO cards */
>> -        return sd_illegal;
>> -
>>      case 6:    /* CMD6:   SWITCH_FUNCTION */
>>          switch (sd->mode) {
>>          case sd_data_transfer_mode:
>> @@ -1081,8 +1077,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>>          break;
>>
>>      case 7:    /* CMD7:   SELECT/DESELECT_CARD */
>> -        if (sd->spi)
>> -            goto bad_cmd;
>>          switch (sd->state) {
>>          case sd_standby_state:
>>              if (sd->rca != rca)
>> @@ -1212,8 +1206,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>>          break;
>>
>>      case 15:   /* CMD15:  GO_INACTIVE_STATE */
>> -        if (sd->spi)
>> -            goto bad_cmd;
>>          switch (sd->mode) {
>>          case sd_data_transfer_mode:
>>              if (sd->rca != rca)
>> @@ -1320,8 +1312,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>>          break;
>>
>>      case 26:   /* CMD26:  PROGRAM_CID */
>> -        if (sd->spi)
>> -            goto bad_cmd;
>>          switch (sd->state) {
>>          case sd_transfer_state:
>>              sd->state = sd_receivingdata_state;
>> @@ -1466,15 +1456,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>>          }
>>          break;
>>
>> -    case 52 ... 54:
>> -        /* CMD52, CMD53, CMD54: reserved for SDIO cards
>> -         * (see the SDIO Simplified Specification V2.0)
>> -         * Handle as illegal command but do not complain
>> -         * on stderr, as some OSes may use these in their
>> -         * probing for presence of an SDIO card.
>> -         */
>> -        return sd_illegal;
>> -
>>      /* Application specific commands (Class 8) */
>>      case 55:   /* CMD55:  APP_CMD */
>>          switch (sd->state) {
>> @@ -1515,19 +1496,12 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>>          break;
>>
>>      case 58:    /* CMD58:   READ_OCR (SPI) */
>> -        if (!sd->spi) {
>> -            goto bad_cmd;
>> -        }
>>          return sd_r3;
>>
>>      case 59:    /* CMD59:   CRC_ON_OFF (SPI) */
>> -        if (!sd->spi) {
>> -            goto bad_cmd;
>> -        }
>>          return sd_r1;
>>
>>      default:
>> -    bad_cmd:
>>          qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", req.cmd);
>>          return sd_illegal;
>>      }
>> @@ -2114,10 +2088,25 @@ void sd_enable(SDState *sd, bool enable)
>>
>>  static const SDProto sd_proto_spi = {
>>      .name = "SPI",
>> +    .cmd = {
>> +        [2 ... 4]   = sd_cmd_illegal,
>> +        [5]         = sd_cmd_illegal,
> 
> Above 2 can be merged into [2 ... 5]

I let it apart because currently we are ignoring SDIO commands
(CMD5, CMD52-54), so maybe it is desirable to keep doing so,
adding a handler such sd_cmd_illegal_but_ignored? Not sure
yet, I need to do more testing.

> 
>> +        [7]         = sd_cmd_illegal,
>> +        [15]        = sd_cmd_illegal,
>> +        [26]        = sd_cmd_illegal,
>> +        [52 ... 54] = sd_cmd_illegal,
>> +    },
>>  };
>>
>>  static const SDProto sd_proto_sd = {
>>      .name = "SD",
>> +    .cmd = {
>> +        [1]         = sd_cmd_illegal,
>> +        [5]         = sd_cmd_illegal,
>> +        [52 ... 54] = sd_cmd_illegal,
>> +        [58]        = sd_cmd_illegal,
>> +        [59]        = sd_cmd_illegal,
>> +    },
>>  };
>>
>>  static void sd_instance_init(Object *obj)
> 
> Otherwise LGTM:
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> 

Re: [RFC PATCH 03/10] hw/sd: Move proto_name to SDProto structure

[Cédric Le Goater]

On 6/24/21 4:22 PM, Philippe Mathieu-Daudé wrote:
> Introduce a new structure to hold the bus protocol specific
> fields: SDProto. The first field is the protocol name.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index d71ec81c22a..a1cc8ab0be8 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -88,6 +88,10 @@ enum SDCardStates {
>      sd_disconnect_state,
>  };
>  
> +typedef struct SDProto {
> +    const char *name;
> +} SDProto;


Why not use an Object class ? 

C.


> +
>  struct SDState {
>      DeviceState parent_obj;
>  
> @@ -112,6 +116,7 @@ struct SDState {
>  
>      /* Runtime changeables */
>  
> +    const SDProto *proto;   /* Bus protocol */
>      uint32_t mode;    /* current card mode, one of SDCardModes */
>      int32_t state;    /* current card state, one of SDCardStates */
>      uint32_t vhs;
> @@ -138,7 +143,6 @@ struct SDState {
>      qemu_irq readonly_cb;
>      qemu_irq inserted_cb;
>      QEMUTimer *ocr_power_timer;
> -    const char *proto_name;
>      bool enable;
>      uint8_t dat_lines;
>      bool cmd_line;
> @@ -951,8 +955,8 @@ static bool address_in_range(SDState *sd, const char *desc,
>  
>  static sd_rsp_type_t sd_invalid_state_for_cmd(SDState *sd, SDRequest req)
>  {
> -    qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state: %s\n",
> -                  req.cmd, sd_state_name(sd->state));
> +    qemu_log_mask(LOG_GUEST_ERROR, "%s: CMD%i in a wrong state: %s\n",
> +                  sd->proto->name, req.cmd, sd_state_name(sd->state));
>  
>      return sd_illegal;
>  }
> @@ -966,7 +970,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>       * However there is no ACMD55, so we want to trace this particular case.
>       */
>      if (req.cmd != 55 || sd->expecting_acmd) {
> -        trace_sdcard_normal_command(sd->proto_name,
> +        trace_sdcard_normal_command(sd->proto->name,
>                                      sd_cmd_name(req.cmd), req.cmd,
>                                      req.arg, sd_state_name(sd->state));
>      }
> @@ -1526,7 +1530,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>  static sd_rsp_type_t sd_app_command(SDState *sd,
>                                      SDRequest req)
>  {
> -    trace_sdcard_app_command(sd->proto_name, sd_acmd_name(req.cmd),
> +    trace_sdcard_app_command(sd->proto->name, sd_acmd_name(req.cmd),
>                               req.cmd, req.arg, sd_state_name(sd->state));
>      sd->card_status |= APP_CMD;
>      switch (req.cmd) {
> @@ -1820,7 +1824,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
>      if (sd->card_status & (ADDRESS_ERROR | WP_VIOLATION))
>          return;
>  
> -    trace_sdcard_write_data(sd->proto_name,
> +    trace_sdcard_write_data(sd->proto->name,
>                              sd_acmd_name(sd->current_cmd),
>                              sd->current_cmd, value);
>      switch (sd->current_cmd) {
> @@ -1976,7 +1980,7 @@ uint8_t sd_read_byte(SDState *sd)
>  
>      io_len = (sd->ocr & (1 << 30)) ? 512 : sd->blk_len;
>  
> -    trace_sdcard_read_data(sd->proto_name,
> +    trace_sdcard_read_data(sd->proto->name,
>                             sd_acmd_name(sd->current_cmd),
>                             sd->current_cmd, io_len);
>      switch (sd->current_cmd) {
> @@ -2095,6 +2099,14 @@ void sd_enable(SDState *sd, bool enable)
>      sd->enable = enable;
>  }
>  
> +static const SDProto sd_proto_spi = {
> +    .name = "SPI",
> +};
> +
> +static const SDProto sd_proto_sd = {
> +    .name = "SD",
> +};
> +
>  static void sd_instance_init(Object *obj)
>  {
>      SDState *sd = SD_CARD(obj);
> @@ -2115,7 +2127,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
>      SDState *sd = SD_CARD(dev);
>      int ret;
>  
> -    sd->proto_name = sd->spi ? "SPI" : "SD";
> +    sd->proto = sd->spi ? &sd_proto_spi : &sd_proto_sd;
>  
>      switch (sd->spec_version) {
>      case SD_PHY_SPECv1_10_VERS
> 

Re: [RFC PATCH 04/10] hw/sd: Introduce sd_cmd_handler type

[Cédric Le Goater]

On 6/24/21 4:22 PM, Philippe Mathieu-Daudé wrote:
> Add 2 command handler arrays in SDProto, for CMD and ACMD.
> Have sd_normal_command() / sd_app_command() use these arrays:
> if an command handler is registered, call it, otherwise fall
> back to current code base.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index a1cc8ab0be8..ce1eec0374f 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -88,8 +88,12 @@ enum SDCardStates {
>      sd_disconnect_state,
>  };
>  
> +typedef sd_rsp_type_t (*sd_cmd_handler)(SDState *sd, SDRequest req);
> +
>  typedef struct SDProto {
>      const char *name;
> +    sd_cmd_handler cmd[SDMMC_CMD_MAX];
> +    sd_cmd_handler acmd[SDMMC_CMD_MAX];
>  } SDProto;


A class would be better but it's no big deal for the moment.

>  
>  struct SDState {
> @@ -994,6 +998,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          return sd_illegal;
>      }
>  
> +    if (sd->proto->cmd[req.cmd]) {
> +        return sd->proto->cmd[req.cmd](sd, req);
> +    }
> +

I expect that some default array will be used to initialize ->cmd ? 

Thanks,

C.


>      switch (req.cmd) {
>      /* Basic commands (Class 0 and Class 1) */
>      case 0:	/* CMD0:   GO_IDLE_STATE */
> @@ -1533,6 +1541,11 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>      trace_sdcard_app_command(sd->proto->name, sd_acmd_name(req.cmd),
>                               req.cmd, req.arg, sd_state_name(sd->state));
>      sd->card_status |= APP_CMD;
> +
> +    if (sd->proto->acmd[req.cmd]) {
> +        return sd->proto->acmd[req.cmd](sd, req);
> +    }
> +
>      switch (req.cmd) {
>      case 6:	/* ACMD6:  SET_BUS_WIDTH */
>          if (sd->spi) {
> 

Re: [RFC PATCH 05/10] hw/sd: Add sd_cmd_illegal() handler

[Cédric Le Goater]

On 6/24/21 4:22 PM, Philippe Mathieu-Daudé wrote:
> Log illegal commands as GUEST_ERROR.
> 
> Note: we are logging back the SDIO commands (CMD5, CMD52-54).
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 57 ++++++++++++++++++++++--------------------------------
>  1 file changed, 23 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index ce1eec0374f..0215bdb3689 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -965,6 +965,14 @@ static sd_rsp_type_t sd_invalid_state_for_cmd(SDState *sd, SDRequest req)
>      return sd_illegal;
>  }
>  
> +static sd_rsp_type_t sd_cmd_illegal(SDState *sd, SDRequest req)
> +{
> +    qemu_log_mask(LOG_GUEST_ERROR, "%s: Unknown CMD%i\n",
> +                  sd->proto->name, req.cmd);
> +
> +    return sd_illegal;
> +}>  static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>  {
>      uint32_t rca = 0x0000;
> @@ -1017,15 +1025,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>  
>      case 1:	/* CMD1:   SEND_OP_CMD */
> -        if (!sd->spi)
> -            goto bad_cmd;
> -
>          sd->state = sd_transfer_state;
>          return sd_r1;
>  
>      case 2:	/* CMD2:   ALL_SEND_CID */
> -        if (sd->spi)
> -            goto bad_cmd;
>          switch (sd->state) {
>          case sd_ready_state:
>              sd->state = sd_identification_state;
> @@ -1037,8 +1040,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>  
>      case 3:	/* CMD3:   SEND_RELATIVE_ADDR */
> -        if (sd->spi)
> -            goto bad_cmd;
>          switch (sd->state) {
>          case sd_identification_state:
>          case sd_standby_state:
> @@ -1052,8 +1053,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>  
>      case 4:	/* CMD4:   SEND_DSR */
> -        if (sd->spi)
> -            goto bad_cmd;
>          switch (sd->state) {
>          case sd_standby_state:
>              break;
> @@ -1063,9 +1062,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          }
>          break;
>  
> -    case 5: /* CMD5: reserved for SDIO cards */
> -        return sd_illegal;
> -
>      case 6:	/* CMD6:   SWITCH_FUNCTION */
>          switch (sd->mode) {
>          case sd_data_transfer_mode:
> @@ -1081,8 +1077,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>  
>      case 7:	/* CMD7:   SELECT/DESELECT_CARD */
> -        if (sd->spi)
> -            goto bad_cmd;
>          switch (sd->state) {
>          case sd_standby_state:
>              if (sd->rca != rca)
> @@ -1212,8 +1206,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>  
>      case 15:	/* CMD15:  GO_INACTIVE_STATE */
> -        if (sd->spi)
> -            goto bad_cmd;
>          switch (sd->mode) {
>          case sd_data_transfer_mode:
>              if (sd->rca != rca)
> @@ -1320,8 +1312,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>  
>      case 26:	/* CMD26:  PROGRAM_CID */
> -        if (sd->spi)
> -            goto bad_cmd;
>          switch (sd->state) {
>          case sd_transfer_state:
>              sd->state = sd_receivingdata_state;
> @@ -1466,15 +1456,6 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          }
>          break;
>  
> -    case 52 ... 54:
> -        /* CMD52, CMD53, CMD54: reserved for SDIO cards
> -         * (see the SDIO Simplified Specification V2.0)
> -         * Handle as illegal command but do not complain
> -         * on stderr, as some OSes may use these in their
> -         * probing for presence of an SDIO card.
> -         */
> -        return sd_illegal;
> -
>      /* Application specific commands (Class 8) */
>      case 55:	/* CMD55:  APP_CMD */
>          switch (sd->state) {
> @@ -1515,19 +1496,12 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          break;
>  
>      case 58:    /* CMD58:   READ_OCR (SPI) */
> -        if (!sd->spi) {
> -            goto bad_cmd;
> -        }
>          return sd_r3;
>  
>      case 59:    /* CMD59:   CRC_ON_OFF (SPI) */
> -        if (!sd->spi) {
> -            goto bad_cmd;
> -        }
>          return sd_r1;
>  
>      default:
> -    bad_cmd:
>          qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", req.cmd);
>          return sd_illegal;
>      }
> @@ -2114,10 +2088,25 @@ void sd_enable(SDState *sd, bool enable)
>  
>  static const SDProto sd_proto_spi = {
>      .name = "SPI",
> +    .cmd = {
> +        [2 ... 4]   = sd_cmd_illegal,
> +        [5]         = sd_cmd_illegal,
> +        [7]         = sd_cmd_illegal,
> +        [15]        = sd_cmd_illegal,
> +        [26]        = sd_cmd_illegal,
> +        [52 ... 54] = sd_cmd_illegal,
> +    },
>  };
>  
>  static const SDProto sd_proto_sd = {
>      .name = "SD",
> +    .cmd = {
> +        [1]         = sd_cmd_illegal,
> +        [5]         = sd_cmd_illegal,
> +        [52 ... 54] = sd_cmd_illegal,
> +        [58]        = sd_cmd_illegal,
> +        [59]        = sd_cmd_illegal,
> +    },
>  };
>  
>  static void sd_instance_init(Object *obj)
> 



Looks good. 

I would start to move these commands in a sd_cmd.c file or sd_common.c
may be.

Thanks,

C.

Re: [RFC PATCH 00/10] hw/sd: Start splitting SD vs SPI protocols

[Cédric Le Goater]

On 6/24/21 4:21 PM, Philippe Mathieu-Daudé wrote:
> Hi Cédric,
> 
> After our discussion yesterday about how to add support for MMC
> (and eMMC) I looked at how to easily add these bus protocols,
> which might have commands quite different, avoiding to have big
> unreadable if/else statements.
> 
> I'm not yet happy enough with the result but it is a starting
> point which keeps things still simple.

this is a good framework but I would introduce a Class.

> What I'm wondering is if we could include the command classes
> (as another dimension in the array). 
I don't quite get what you mean ? 

> Also if we could use the
> older spec version supported as base set of commands, and if the
> user asks for more recent spec version, for each version we
> overwrite the array of commands. Thoughts?

Yes. I think we need another RFC to see how it looks :) 

I expect these command arrays to be static. Duplicating the base 
array to add custom handlers for a new version of a protocol 
should be ok. 

Thanks,

C.
  

> 
> Phil.
> 
> Philippe Mathieu-Daudé (10):
>   hw/sd: When card is in wrong state, log which state it is
>   hw/sd: Extract address_in_range() helper, log invalid accesses
>   hw/sd: Move proto_name to SDProto structure
>   hw/sd: Introduce sd_cmd_handler type
>   hw/sd: Add sd_cmd_illegal() handler
>   hw/sd: Add sd_cmd_unimplemented() handler
>   hw/sd: Add sd_cmd_GO_IDLE_STATE() handler
>   hw/sd: Add sd_cmd_SEND_OP_CMD() handler
>   hw/sd: Add sd_cmd_ALL_SEND_CID() handler
>   hw/sd: Add sd_cmd_SEND_RELATIVE_ADDR() handler
> 
>  hw/sd/sd.c | 251 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 143 insertions(+), 108 deletions(-)
> 

Re: [RFC PATCH 04/10] hw/sd: Introduce sd_cmd_handler type

[Philippe Mathieu-Daudé]

On 6/28/21 9:29 AM, Cédric Le Goater wrote:
> On 6/24/21 4:22 PM, Philippe Mathieu-Daudé wrote:
>> Add 2 command handler arrays in SDProto, for CMD and ACMD.
>> Have sd_normal_command() / sd_app_command() use these arrays:
>> if an command handler is registered, call it, otherwise fall
>> back to current code base.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sd.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index a1cc8ab0be8..ce1eec0374f 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -88,8 +88,12 @@ enum SDCardStates {
>>      sd_disconnect_state,
>>  };
>>  
>> +typedef sd_rsp_type_t (*sd_cmd_handler)(SDState *sd, SDRequest req);
>> +
>>  typedef struct SDProto {
>>      const char *name;
>> +    sd_cmd_handler cmd[SDMMC_CMD_MAX];
>> +    sd_cmd_handler acmd[SDMMC_CMD_MAX];
>>  } SDProto;
> 
> 
> A class would be better but it's no big deal for the moment.

Could be. Easily modifiable later. For now I'd rather focus on
finding the easiest code path keeping maintenance simple enough,
and worry about implementation details later.

> 
>>  
>>  struct SDState {
>> @@ -994,6 +998,10 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>>          return sd_illegal;
>>      }
>>  
>> +    if (sd->proto->cmd[req.cmd]) {
>> +        return sd->proto->cmd[req.cmd](sd, req);
>> +    }
>> +
> 
> I expect that some default array will be used to initialize ->cmd ?

Maybe at the end of the conversion. For now it is NULL-initialized.
See patch 3:

+static const SDProto sd_proto_sd = {
+    .name = "SD",
+};

> 
> Thanks,
> 
> C.
> 
> 
>>      switch (req.cmd) {
>>      /* Basic commands (Class 0 and Class 1) */
>>      case 0:	/* CMD0:   GO_IDLE_STATE */
>> @@ -1533,6 +1541,11 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>>      trace_sdcard_app_command(sd->proto->name, sd_acmd_name(req.cmd),
>>                               req.cmd, req.arg, sd_state_name(sd->state));
>>      sd->card_status |= APP_CMD;
>> +
>> +    if (sd->proto->acmd[req.cmd]) {
>> +        return sd->proto->acmd[req.cmd](sd, req);
>> +    }
>> +
>>      switch (req.cmd) {
>>      case 6:	/* ACMD6:  SET_BUS_WIDTH */
>>          if (sd->spi) {
>>
> 
>