[Qemu-devel] [PATCH 0/2] SD emulation fixes for Tianocore EDK2 UEFI

[Qemu-devel] [PATCH 0/2] SD emulation fixes for Tianocore EDK2 UEFI

[Andrew Baumann]

This series contains two fixes to the SD card emulation that are
needed to unblock Tianocore EDK2 UEFI builds (including the bootloader
for Windows on Raspberry Pi 2) from booting.

(I'm guessing at the CC list here, since this code appears to be
unmaintained. Apologies if I guessed wrong!)

Cheers,
Andrew

Andrew Baumann (2):
  hw/sd: implement CMD23 (SET_BLOCK_COUNT) for MMC compatibility
  hw/sd: model a power-up delay, as a workaround for an EDK2 bug

 hw/sd/sd.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 74 insertions(+), 4 deletions(-)

-- 
2.5.3

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

[Andrew Baumann]

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--;
+            }
+
             /* 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");
+            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

[Qemu-devel] [PATCH 2/2] hw/sd: model a power-up delay, as a workaround for an EDK2 bug

[Andrew Baumann]

The SD spec for ACMD41 says that a zero argument is an "inquiry"
ACMD41, which does not start initialisation and is used only for
retrieving the OCR. However, Tianocore EDK2 (UEFI) has a bug [1]: it
first sends an inquiry (zero) ACMD41. If that first request returns an
OCR value with the power up bit (0x80000000) set, it assumes the card
is ready and continues, leaving the card in the wrong state. (My
assumption is that this works on hardware, because no real card is
immediately powered up upon reset.)

This change implements a simple workaround: the first time an ACMD41
is issued, the OCR is returned without the power up bit set. Upon
subsequent reads, the OCR reports power up.

[1] https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c#L279
(This is the loop starting with "We need to wait for the MMC or SD
card is ready")

Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
Obviously this is a bug that should be fixed in EDK2. However, this
initialisation appears to have been around for quite a while in EDK2
(in various forms), and the fact that it has obviously worked with so
many real SD/MMC cards makes me think that it would be pragmatic to
have the workaround in QEMU as well.

 hw/sd/sd.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 98af8bc..31a25bc 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -80,6 +80,7 @@ struct SDState {
     uint32_t mode;    /* current card mode, one of SDCardModes */
     int32_t state;    /* current card state, one of SDCardStates */
     uint32_t ocr;
+    bool ocr_initdelay;
     uint8_t scr[8];
     uint8_t cid[16];
     uint8_t csd[16];
@@ -195,8 +196,21 @@ static uint16_t sd_crc16(void *message, size_t width)
 
 static void sd_set_ocr(SDState *sd)
 {
-    /* All voltages OK, card power-up OK, Standard Capacity SD Memory Card */
-    sd->ocr = 0x80ffff00;
+    /* All voltages OK, Standard Capacity SD Memory Card, not yet powered up */
+    sd->ocr = 0x00ffff00;
+    sd->ocr_initdelay = false;
+}
+
+static bool sd_model_ocr_init_delay(SDState *sd)
+{
+    if (!sd->ocr_initdelay) {
+        sd->ocr_initdelay = true;
+        return false;
+    } else {
+        /* Set powered up bit in OCR */
+        sd->ocr |= 0x80000000;
+        return true;
+    }
 }
 
 static void sd_set_scr(SDState *sd)
@@ -451,6 +465,7 @@ static const VMStateDescription sd_vmstate = {
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(mode, SDState),
         VMSTATE_INT32(state, SDState),
+        VMSTATE_BOOL(ocr_initdelay, SDState),
         VMSTATE_UINT8_ARRAY(cid, SDState, 16),
         VMSTATE_UINT8_ARRAY(csd, SDState, 16),
         VMSTATE_UINT16(rca, SDState),
@@ -1300,10 +1315,17 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
         case sd_idle_state:
             /* We accept any voltage.  10000 V is nothing.
              *
-             * We don't model init delay so just advance straight to ready state
+             * We model an init "delay" of one polling cycle, as a
+             * workaround for around a bug in TianoCore EDK2 which
+             * sends an initial zero ACMD41, but nevertheless assumes
+             * that the card is in ready state as soon as it sees the
+             * power up bit set.
+             *
+             * Once we've delayed, we advance straight to ready state
              * unless it's an enquiry ACMD41 (bits 23:0 == 0).
              */
-            if (req.arg & ACMD41_ENQUIRY_MASK) {
+            if (sd_model_ocr_init_delay(sd)
+                && (req.arg & ACMD41_ENQUIRY_MASK)) {
                 sd->state = sd_ready_state;
             }
 
-- 
2.5.3

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

[Peter Crosthwaite]

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

Re: [Qemu-devel] [PATCH 2/2] hw/sd: model a power-up delay, as a workaround for an EDK2 bug

[Peter Crosthwaite]

On Fri, Dec 4, 2015 at 1:16 PM, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
> The SD spec for ACMD41 says that a zero argument is an "inquiry"
> ACMD41, which does not start initialisation and is used only for
> retrieving the OCR. However, Tianocore EDK2 (UEFI) has a bug [1]: it
> first sends an inquiry (zero) ACMD41. If that first request returns an
> OCR value with the power up bit (0x80000000) set, it assumes the card
> is ready and continues, leaving the card in the wrong state. (My
> assumption is that this works on hardware, because no real card is
> immediately powered up upon reset.)
>
> This change implements a simple workaround: the first time an ACMD41
> is issued, the OCR is returned without the power up bit set. Upon
> subsequent reads, the OCR reports power up.
>

So mandating two ACMD41's to get a power-up signal would introduce a
bug in guests with the opposite race condition. I.e. you are
facilitating a guest that (incorrectly) assumes a lower bound on the
card power up, but that could break a guest (incorrectly) assuming an
upper bound. The other broken code would look something like this:

do_card_power_on();
sleep(1);
if (!get_acmd_41_power_up()) {
   abort(); /* Card did not power up in one second */
}

granted, this is a bad guest, but I think it can be solved both ways
by a more realistic model of the behaviour. Instead of using the
ACMD41 as the trigger for the state change, use a QEMU timer and model
an actual timed power up delay.

Regards,
Peter

> [1] https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c#L279
> (This is the loop starting with "We need to wait for the MMC or SD
> card is ready")
>
> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> ---
> Obviously this is a bug that should be fixed in EDK2. However, this
> initialisation appears to have been around for quite a while in EDK2
> (in various forms), and the fact that it has obviously worked with so
> many real SD/MMC cards makes me think that it would be pragmatic to
> have the workaround in QEMU as well.
>
>  hw/sd/sd.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 98af8bc..31a25bc 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -80,6 +80,7 @@ struct SDState {
>      uint32_t mode;    /* current card mode, one of SDCardModes */
>      int32_t state;    /* current card state, one of SDCardStates */
>      uint32_t ocr;
> +    bool ocr_initdelay;
>      uint8_t scr[8];
>      uint8_t cid[16];
>      uint8_t csd[16];
> @@ -195,8 +196,21 @@ static uint16_t sd_crc16(void *message, size_t width)
>
>  static void sd_set_ocr(SDState *sd)
>  {
> -    /* All voltages OK, card power-up OK, Standard Capacity SD Memory Card */
> -    sd->ocr = 0x80ffff00;
> +    /* All voltages OK, Standard Capacity SD Memory Card, not yet powered up */
> +    sd->ocr = 0x00ffff00;
> +    sd->ocr_initdelay = false;
> +}
> +
> +static bool sd_model_ocr_init_delay(SDState *sd)
> +{
> +    if (!sd->ocr_initdelay) {
> +        sd->ocr_initdelay = true;
> +        return false;
> +    } else {
> +        /* Set powered up bit in OCR */
> +        sd->ocr |= 0x80000000;
> +        return true;
> +    }
>  }
>
>  static void sd_set_scr(SDState *sd)
> @@ -451,6 +465,7 @@ static const VMStateDescription sd_vmstate = {
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(mode, SDState),
>          VMSTATE_INT32(state, SDState),
> +        VMSTATE_BOOL(ocr_initdelay, SDState),
>          VMSTATE_UINT8_ARRAY(cid, SDState, 16),
>          VMSTATE_UINT8_ARRAY(csd, SDState, 16),
>          VMSTATE_UINT16(rca, SDState),
> @@ -1300,10 +1315,17 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>          case sd_idle_state:
>              /* We accept any voltage.  10000 V is nothing.
>               *
> -             * We don't model init delay so just advance straight to ready state
> +             * We model an init "delay" of one polling cycle, as a
> +             * workaround for around a bug in TianoCore EDK2 which
> +             * sends an initial zero ACMD41, but nevertheless assumes
> +             * that the card is in ready state as soon as it sees the
> +             * power up bit set.
> +             *
> +             * Once we've delayed, we advance straight to ready state
>               * unless it's an enquiry ACMD41 (bits 23:0 == 0).
>               */
> -            if (req.arg & ACMD41_ENQUIRY_MASK) {
> +            if (sd_model_ocr_init_delay(sd)
> +                && (req.arg & ACMD41_ENQUIRY_MASK)) {
>                  sd->state = sd_ready_state;
>              }
>
> --
> 2.5.3
>
>

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

[Andrew Baumann]

> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> Sent: Saturday, 5 December 2015 22:27
> On Fri, Dec 4, 2015 at 1:16 PM, Andrew Baumann
> <Andrew.Baumann@microsoft.com> wrote:
> > @@ -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?

That was my initial assumption (and implementation) too, but no: table 4-34 in the spec shows that you stay in transfer mode, and UEFI issues CMD12 after each multiple-block (CMD23) I/O, which doesn't work if you've already left the transfer state.

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

Point taken. These printfs were more to assure myself that they never happened; they can be removed. (It did seem weird to use a bare printf, but all the code around here seemed to be doing the same thing.)

I'll send a revised patch and respond to the other feedback probably later next week.

Thanks,
Andrew

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

[Peter Crosthwaite]

On Sun, Dec 6, 2015 at 1:20 PM, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
>> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
>> Sent: Saturday, 5 December 2015 22:27
>> On Fri, Dec 4, 2015 at 1:16 PM, Andrew Baumann
>> <Andrew.Baumann@microsoft.com> wrote:
>> > @@ -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?
>
> That was my initial assumption (and implementation) too, but no: table 4-34 in the spec shows that you stay in transfer mode, and UEFI issues CMD12 after each multiple-block (CMD23) I/O, which doesn't work if you've already left the transfer state.

So table 4-34 doesn't really help as that is showing the state
following a command itself, whereas an automatic CMD12 would happen
following the end of a data payload (not an explicit command). It
would if anything be a form of "Operation complete" (first line in
4-34) which would be a state transition. You mention that CMD12
doesn't work if you have left transfer_state, and that is accurate.
However CMD12 normally doesn't transfer out of the transfer_state,
instead it transfers from data->tran or rcv->prog (note rcv->prog is
as good as rcv->tran for us as we model prog state as instantaneous).
If your original implementation transferred to something that is not
tran that would fail.

I have both the eMMC and SD specs infront of me. Here is what I have for eMMC:

"
Multiple block read with pre-defined block count:

The card will transfer the requested number of data blocks, terminate
the transaction and return to transfer state. Stop command is not
required at the end of this type of multiple block read, unless
terminated with an error. In order to start a multiple block read with
pre-defined block count the host must use the SET_BLOCK_COUNT command
(CMD23) immediately preceding the READ_MULTIPLE_BLOCK (CMD18) command.
Otherwise the card will start an open-ended multiple block read which
can be stopped using the STOP_TRANSMISION command.
"

That to me suggests that CMD12 should not be needed at all, but does
mention CMD12 is needed in error paths.

SD spec has this:

"
CMD12 has been used to stop multiple-block Read / Write operation.
However, CMD12 is timing dependent and it is difficult to control
timing to issue CMD12 at exact timing. As UHS104 card has large delay
variation between clock and data, CMD23 is useful for the host to stop
multiple read / write operation instead of CMD12. Host is not
necessary to control timing of CMD12. This command is applicable to
always 512-byte block length read/write operation and then SDSC card
does not support this command. Support of CMD23 is mandatory for
UHS104 card.

Support of CMD23 is defined in SCR. The response type of CMD23 is R1
and busy is not indicated. CMD23 is accepted in transfer state and
effective to the multiple-block read/write command (CMD18 or CMD25)
just  behind CMD23. If another command follows CMD23, set block count
is canceled (including CMD13). If command CRC error occurs, the card
does not return R1 response for CMD23. In this case, Set block count
is not valid and retry of CMD23 is required. If multiple CMD23 are
issued, the last one is valid.

Figure 4-58 shows the definition of CMD23. If block count in the
argument is set to 0, CMD23 has no effect. The block count value set
by CMD23 is not checked by the card and then CMD23 does not indicate
any error in the response (A previous command error is indicated in
the response of CMD23). If illegal block count is set, out of range
error will be indicated during read/write operation (For example, data
transfer is stopped at user area boundary). Host needs to issue CMD12
if any error is detected in the CMD18 and CMD25 operations. If a CMD25
is aborted and the amount of data transferred is less
than the amount of data indicated by the preceding CMD23, then the
area specified by CMD23 that is unwritten may contain undefined data.
If the amount of data transferred is greater than the amount of
data indicated by the preceding CMD23, then the extra data is not written.
"

which is significantly less clear but still suggest that CMD12 is not
needed. The last bit ("If the amount of data transferred is greater
than the amount of data indicated by the preceding CMD23, then the
extra data is not written") is what you have implemented very
directly, but would also be a consequence of an automatic CMD12 at the
end of a data-phase.

Some possibilities:

1: The SD card vendors don't follow the spec (as shown in 4-34) and
just ignore a spurious CMD12 when you have already returned to
transfer state. This would explain everything.
2: UEFI is using early termination in the normal code path.

Can you give me a source code pointer to the CMD12 in UEFI by any
chance? I'd like to have a look at the driver logic.

Regards,
Peter

>
>> >      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.
>
> Point taken. These printfs were more to assure myself that they never happened; they can be removed. (It did seem weird to use a bare printf, but all the code around here seemed to be doing the same thing.)
>
> I'll send a revised patch and respond to the other feedback probably later next week.
>
> Thanks,
> Andrew

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

[Andrew Baumann]

Hi Peter,

> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> Sent: Sunday, 6 December 2015 15:47
> On Sun, Dec 6, 2015 at 1:20 PM, Andrew Baumann
> <Andrew.Baumann@microsoft.com> wrote:
> >> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> >> Sent: Saturday, 5 December 2015 22:27
> >> On Fri, Dec 4, 2015 at 1:16 PM, Andrew Baumann
> >> <Andrew.Baumann@microsoft.com> wrote:
> >> > @@ -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?
> >
> > That was my initial assumption (and implementation) too, but no: table 4-
> 34 in the spec shows that you stay in transfer mode, and UEFI issues CMD12
> after each multiple-block (CMD23) I/O, which doesn't work if you've already
> left the transfer state.
> 
> So table 4-34 doesn't really help as that is showing the state
> following a command itself, whereas an automatic CMD12 would happen
> following the end of a data payload (not an explicit command). It
> would if anything be a form of "Operation complete" (first line in
> 4-34) which would be a state transition. You mention that CMD12
> doesn't work if you have left transfer_state, and that is accurate.
> However CMD12 normally doesn't transfer out of the transfer_state,
> instead it transfers from data->tran or rcv->prog (note rcv->prog is
> as good as rcv->tran for us as we model prog state as instantaneous).
> If your original implementation transferred to something that is not
> tran that would fail.
> 
> I have both the eMMC and SD specs infront of me. Here is what I have for
> eMMC:
> 
> "
> Multiple block read with pre-defined block count:
> 
> The card will transfer the requested number of data blocks, terminate
> the transaction and return to transfer state. Stop command is not
> required at the end of this type of multiple block read, unless
> terminated with an error. In order to start a multiple block read with
> pre-defined block count the host must use the SET_BLOCK_COUNT
> command
> (CMD23) immediately preceding the READ_MULTIPLE_BLOCK (CMD18)
> command.
> Otherwise the card will start an open-ended multiple block read which
> can be stopped using the STOP_TRANSMISION command.
> "
> 
> That to me suggests that CMD12 should not be needed at all, but does
> mention CMD12 is needed in error paths.
> 
> SD spec has this:
> 
> "
> CMD12 has been used to stop multiple-block Read / Write operation.
> However, CMD12 is timing dependent and it is difficult to control
> timing to issue CMD12 at exact timing. As UHS104 card has large delay
> variation between clock and data, CMD23 is useful for the host to stop
> multiple read / write operation instead of CMD12. Host is not
> necessary to control timing of CMD12. This command is applicable to
> always 512-byte block length read/write operation and then SDSC card
> does not support this command. Support of CMD23 is mandatory for
> UHS104 card.
> 
> Support of CMD23 is defined in SCR. The response type of CMD23 is R1
> and busy is not indicated. CMD23 is accepted in transfer state and
> effective to the multiple-block read/write command (CMD18 or CMD25)
> just  behind CMD23. If another command follows CMD23, set block count
> is canceled (including CMD13). If command CRC error occurs, the card
> does not return R1 response for CMD23. In this case, Set block count
> is not valid and retry of CMD23 is required. If multiple CMD23 are
> issued, the last one is valid.
> 
> Figure 4-58 shows the definition of CMD23. If block count in the
> argument is set to 0, CMD23 has no effect. The block count value set
> by CMD23 is not checked by the card and then CMD23 does not indicate
> any error in the response (A previous command error is indicated in
> the response of CMD23). If illegal block count is set, out of range
> error will be indicated during read/write operation (For example, data
> transfer is stopped at user area boundary). Host needs to issue CMD12
> if any error is detected in the CMD18 and CMD25 operations. If a CMD25
> is aborted and the amount of data transferred is less
> than the amount of data indicated by the preceding CMD23, then the
> area specified by CMD23 that is unwritten may contain undefined data.
> If the amount of data transferred is greater than the amount of
> data indicated by the preceding CMD23, then the extra data is not written.
> "
> 
> which is significantly less clear but still suggest that CMD12 is not
> needed. The last bit ("If the amount of data transferred is greater
> than the amount of data indicated by the preceding CMD23, then the
> extra data is not written") is what you have implemented very
> directly, but would also be a consequence of an automatic CMD12 at the
> end of a data-phase.
> 
> Some possibilities:
> 
> 1: The SD card vendors don't follow the spec (as shown in 4-34) and
> just ignore a spurious CMD12 when you have already returned to
> transfer state. This would explain everything.
> 2: UEFI is using early termination in the normal code path.
> 
> Can you give me a source code pointer to the CMD12 in UEFI by any
> chance? I'd like to have a look at the driver logic.

Thanks for the detailed investigation of this. I now actually think you're probably correct. The MMC spec is certainly clear, much more so than the SD spec which I was looking at until now.

I went looking for the UEFI code. It turns out that this use of CMD23 is not in the mainline version of EDK2; it appears to have been added somewhere along the way to building the Pi2 bootloader (I'm not sure why), so we probably shouldn't place a huge amount of faith in it. It also doesn't check for the success/failure of the CMD12 which it issues. After my original implementation, I saw a console full of "CMD12 in a wrong state" printfs and immediately assumed I was doing something wrong, but I now think my first guess was correct and I should have silenced the printf instead. I'll dust that code off and resubmit the patch, assuming it tests ok.

Andrew

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

[Peter Crosthwaite]

On Sun, Dec 6, 2015 at 10:03 PM, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
> Hi Peter,
>
>> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
>> Sent: Sunday, 6 December 2015 15:47
>> On Sun, Dec 6, 2015 at 1:20 PM, Andrew Baumann
>> <Andrew.Baumann@microsoft.com> wrote:
>> >> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
>> >> Sent: Saturday, 5 December 2015 22:27
>> >> On Fri, Dec 4, 2015 at 1:16 PM, Andrew Baumann
>> >> <Andrew.Baumann@microsoft.com> wrote:
>> >> > @@ -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?
>> >
>> > That was my initial assumption (and implementation) too, but no: table 4-
>> 34 in the spec shows that you stay in transfer mode, and UEFI issues CMD12
>> after each multiple-block (CMD23) I/O, which doesn't work if you've already
>> left the transfer state.
>>
>> So table 4-34 doesn't really help as that is showing the state
>> following a command itself, whereas an automatic CMD12 would happen
>> following the end of a data payload (not an explicit command). It
>> would if anything be a form of "Operation complete" (first line in
>> 4-34) which would be a state transition. You mention that CMD12
>> doesn't work if you have left transfer_state, and that is accurate.
>> However CMD12 normally doesn't transfer out of the transfer_state,
>> instead it transfers from data->tran or rcv->prog (note rcv->prog is
>> as good as rcv->tran for us as we model prog state as instantaneous).
>> If your original implementation transferred to something that is not
>> tran that would fail.
>>
>> I have both the eMMC and SD specs infront of me. Here is what I have for
>> eMMC:
>>
>> "
>> Multiple block read with pre-defined block count:
>>
>> The card will transfer the requested number of data blocks, terminate
>> the transaction and return to transfer state. Stop command is not
>> required at the end of this type of multiple block read, unless
>> terminated with an error. In order to start a multiple block read with
>> pre-defined block count the host must use the SET_BLOCK_COUNT
>> command
>> (CMD23) immediately preceding the READ_MULTIPLE_BLOCK (CMD18)
>> command.
>> Otherwise the card will start an open-ended multiple block read which
>> can be stopped using the STOP_TRANSMISION command.
>> "
>>
>> That to me suggests that CMD12 should not be needed at all, but does
>> mention CMD12 is needed in error paths.
>>
>> SD spec has this:
>>
>> "
>> CMD12 has been used to stop multiple-block Read / Write operation.
>> However, CMD12 is timing dependent and it is difficult to control
>> timing to issue CMD12 at exact timing. As UHS104 card has large delay
>> variation between clock and data, CMD23 is useful for the host to stop
>> multiple read / write operation instead of CMD12. Host is not
>> necessary to control timing of CMD12. This command is applicable to
>> always 512-byte block length read/write operation and then SDSC card
>> does not support this command. Support of CMD23 is mandatory for
>> UHS104 card.
>>
>> Support of CMD23 is defined in SCR. The response type of CMD23 is R1
>> and busy is not indicated. CMD23 is accepted in transfer state and
>> effective to the multiple-block read/write command (CMD18 or CMD25)
>> just  behind CMD23. If another command follows CMD23, set block count
>> is canceled (including CMD13). If command CRC error occurs, the card
>> does not return R1 response for CMD23. In this case, Set block count
>> is not valid and retry of CMD23 is required. If multiple CMD23 are
>> issued, the last one is valid.
>>
>> Figure 4-58 shows the definition of CMD23. If block count in the
>> argument is set to 0, CMD23 has no effect. The block count value set
>> by CMD23 is not checked by the card and then CMD23 does not indicate
>> any error in the response (A previous command error is indicated in
>> the response of CMD23). If illegal block count is set, out of range
>> error will be indicated during read/write operation (For example, data
>> transfer is stopped at user area boundary). Host needs to issue CMD12
>> if any error is detected in the CMD18 and CMD25 operations. If a CMD25
>> is aborted and the amount of data transferred is less
>> than the amount of data indicated by the preceding CMD23, then the
>> area specified by CMD23 that is unwritten may contain undefined data.
>> If the amount of data transferred is greater than the amount of
>> data indicated by the preceding CMD23, then the extra data is not written.
>> "
>>
>> which is significantly less clear but still suggest that CMD12 is not
>> needed. The last bit ("If the amount of data transferred is greater
>> than the amount of data indicated by the preceding CMD23, then the
>> extra data is not written") is what you have implemented very
>> directly, but would also be a consequence of an automatic CMD12 at the
>> end of a data-phase.
>>
>> Some possibilities:
>>
>> 1: The SD card vendors don't follow the spec (as shown in 4-34) and
>> just ignore a spurious CMD12 when you have already returned to
>> transfer state. This would explain everything.
>> 2: UEFI is using early termination in the normal code path.
>>
>> Can you give me a source code pointer to the CMD12 in UEFI by any
>> chance? I'd like to have a look at the driver logic.
>
> Thanks for the detailed investigation of this. I now actually think you're probably correct. The MMC spec is certainly clear, much more so than the SD spec which I was looking at until now.
>
> I went looking for the UEFI code. It turns out that this use of CMD23 is not in the mainline version of EDK2; it appears to have been added somewhere along the way to building the Pi2 bootloader (I'm not sure why), so we probably shouldn't place a huge amount of faith in it.

Is that in a git repo that we could run a git blame on?

Regards,
Peter

>It also doesn't check for the success/failure of the CMD12 which it issues. After my original implementation, I saw a console full of "CMD12 in a wrong state" printfs and immediately assumed I was doing something wrong, but I now think my first guess was correct and I should have silenced the printf instead. I'll dust that code off and resubmit the patch, assuming it tests ok.
>
> Andrew