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

Philippe Mathieu-Daudé posted 10 patches 4 years, 5 months ago
[RFC PATCH 05/10] hw/sd: Add sd_cmd_illegal() handler
Posted by Philippe Mathieu-Daudé 4 years, 5 months ago
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

Re: [RFC PATCH 05/10] hw/sd: Add sd_cmd_illegal() handler
Posted by Bin Meng 4 years, 5 months ago
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 05/10] hw/sd: Add sd_cmd_illegal() handler
Posted by Philippe Mathieu-Daudé 4 years, 5 months ago
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 05/10] hw/sd: Add sd_cmd_illegal() handler
Posted by Cédric Le Goater 4 years, 5 months ago
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.