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
<address@hidden> 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 <address@hidden>
> ---
> 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
>
>