On 6/25/21 3:47 PM, Bin Meng wrote:
> On Thu, Jun 24, 2021 at 10:23 PM Philippe Mathieu-Daudé <f4...@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é <f4...@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...@gmail.com>
>
