Re: [Qemu-devel] [PATCH 1/2] hw/sd: implement CMD23 (SET_BLOCK_COUNT) for MMC compatibility

[Peter Crosthwaite <crosthwaitepeter@gmail.com>]

On Fri, Dec 4, 2015 at 1:16 PM, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
> CMD23 is optional for SD but required for MMC, and Tianocore EDK2
> (UEFI) uses it at boot.
>
> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> ---
> For EDK2 to boot all we actually need to do is return success and
> ignore the command, but it seemed much safer to implement the full
> semantics.
>
>  hw/sd/sd.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index ce4d44b..98af8bc 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -92,6 +92,8 @@ struct SDState {
>      int32_t wpgrps_size;
>      uint64_t size;
>      uint32_t blk_len;
> +    uint32_t multi_blk_cnt;
> +    bool multi_blk_active;
>      uint32_t erase_start;
>      uint32_t erase_end;
>      uint8_t pwd[16];
> @@ -423,6 +425,8 @@ static void sd_reset(SDState *sd)
>      sd->blk_len = 0x200;
>      sd->pwd_len = 0;
>      sd->expecting_acmd = false;
> +    sd->multi_blk_cnt = 0;
> +    sd->multi_blk_active = false;
>  }
>
>  static void sd_cardchange(void *opaque, bool load)
> @@ -455,6 +459,8 @@ static const VMStateDescription sd_vmstate = {
>          VMSTATE_UINT32(vhs, SDState),
>          VMSTATE_BITMAP(wp_groups, SDState, 0, wpgrps_size),
>          VMSTATE_UINT32(blk_len, SDState),
> +        VMSTATE_UINT32(multi_blk_cnt, SDState),
> +        VMSTATE_BOOL(multi_blk_active, SDState),
>          VMSTATE_UINT32(erase_start, SDState),
>          VMSTATE_UINT32(erase_end, SDState),
>          VMSTATE_UINT8_ARRAY(pwd, SDState, 16),
> @@ -670,6 +676,12 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
>      if (sd_cmd_type[req.cmd] == sd_ac || sd_cmd_type[req.cmd] == sd_adtc)
>          rca = req.arg >> 16;
>
> +    /* CMD23 (set block count) must be immediately followed by CMD18 or CMD25
> +     * if not, its effects are cancelled */
> +    if (sd->multi_blk_active && !(req.cmd == 18 || req.cmd == 25)) {
> +        sd->multi_blk_active = false;
> +    }
> +
>      DPRINTF("CMD%d 0x%08x state %d\n", req.cmd, req.arg, sd->state);
>      switch (req.cmd) {
>      /* Basic commands (Class 0 and Class 1) */
> @@ -965,6 +977,18 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
>          }
>          break;
>
> +    case 23:    /* CMD23: SET_BLOCK_COUNT */
> +        switch (sd->state) {
> +        case sd_transfer_state:
> +            sd->multi_blk_cnt = req.arg;
> +            sd->multi_blk_active = req.arg != 0;
> +            return sd_r1;
> +
> +        default:
> +            break;
> +        }
> +        break;
> +
>      /* Block write commands (Class 4) */
>      case 24:   /* CMD24:  WRITE_SINGLE_BLOCK */
>          if (sd->spi)
> @@ -1542,6 +1566,11 @@ void sd_write_data(SDState *sd, uint8_t value)
>          break;
>
>      case 25:   /* CMD25:  WRITE_MULTIPLE_BLOCK */
> +        if (sd->multi_blk_active && sd->multi_blk_cnt == 0) {
> +            /* just ignore this write -- we've overrun the block count */
> +            fprintf(stderr, "sd_write_data: overrun of multi-block transfer\n");
> +            break;
> +        }
>          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) {
> @@ -1563,6 +1592,11 @@ void sd_write_data(SDState *sd, uint8_t value)
>              sd->data_offset = 0;
>              sd->csd[14] |= 0x40;
>
> +            if (sd->multi_blk_active) {
> +                assert(sd->multi_blk_cnt > 0);
> +                sd->multi_blk_cnt--;

So reading the spec, it says that this command is an alternative to a
host issued CMD12. Does this mean completion of the length-specified
read should move back to sd_transfer_state the same way CMD12 does?

> +            }
> +
>              /* Bzzzzzzztt .... Operation complete.  */
>              sd->state = sd_receivingdata_state;
>          }
> @@ -1703,6 +1737,12 @@ uint8_t sd_read_data(SDState *sd)
>          break;
>
>      case 18:   /* CMD18:  READ_MULTIPLE_BLOCK */
> +        if (sd->multi_blk_active && sd->multi_blk_cnt == 0) {
> +            /* we've overrun the block count */
> +            fprintf(stderr, "sd_read_data: overrun of multi-block transfer\n");

So if the card stays in-state and the intention is to discard overrun,
this message is likely to be flood prone. From my understanding (which
is pretty fresh) CMD23 may be designed to gracefully handle this
condition from the guest, which would suggest this is not an error at
all and no message is needed. If you do want to keep a message it
should be GUEST_ERROR and have a squelching mechanism so reading one
block past the end of CMD23 doesn't cause a large number of messages.

Regards,
Peter

> +            ret = 0;
> +            break;
> +        }
>          if (sd->data_offset == 0)
>              BLK_READ_BLOCK(sd->data_start, io_len);
>          ret = sd->data[sd->data_offset ++];
> @@ -1710,6 +1750,14 @@ uint8_t sd_read_data(SDState *sd)
>          if (sd->data_offset >= io_len) {
>              sd->data_start += io_len;
>              sd->data_offset = 0;
> +
> +            if (sd->multi_blk_active) {
> +                assert(sd->multi_blk_cnt > 0);
> +                if (--sd->multi_blk_cnt == 0) {
> +                    break;
> +                }
> +            }
> +
>              if (sd->data_start + io_len > sd->size) {
>                  sd->card_status |= ADDRESS_ERROR;
>                  break;
> --
> 2.5.3
>
>