[Qemu-devel] [PATCH v3 0/3] SD emulation fixes for Pi2 Tianocore EDK2 UEFI

[Qemu-devel] [PATCH v3 0/3] SD emulation fixes for Pi2 Tianocore EDK2 UEFI

[Andrew Baumann]

This series contains fixes to the SD card emulation that are needed to
unblock Tianocore EDK2 UEFI (specifically, the bootloader for Windows
on Raspberry Pi 2).

Changes in v2, based on feedback from Peter Crosthwaite:
 * correct implementation of CMD23 to switch to transfer state on completion
 * use an actual timer for the power-up delay, rather than relying on
   the guest polling ACMD41 twice
 * added patch 3: replace fprintfs with guest error logging

Change in v3:
 * rebased on Peter Maydell's SD QOMification series:
   https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04425.html
   (although the conflicts are minor -- this could be independent)
 * use a subsection for the new OCR vmstate (patch 2/3), rather than
   bumping the version number, to ensure backward-compatibility

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

Cheers,
Andrew


Andrew Baumann (3):
  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: use guest error logging rather than fprintf to stderr

 hw/sd/sd.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 126 insertions(+), 15 deletions(-)
 mode change 100644 => 100755 hw/sd/sd.c

-- 
2.5.3

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

[Andrew Baumann]

CMD23 is optional for SD but required for MMC, and the UEFI bootloader
used for Windows on Raspberry Pi 2 issues it.

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
 hw/sd/sd.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 9e3be2c..8514ac7 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -97,6 +97,7 @@ struct SDState {
     int32_t wpgrps_size;
     uint64_t size;
     uint32_t blk_len;
+    uint32_t multi_blk_cnt;
     uint32_t erase_start;
     uint32_t erase_end;
     uint8_t pwd[16];
@@ -429,6 +430,7 @@ static void sd_reset(DeviceState *dev)
     sd->blk_len = 0x200;
     sd->pwd_len = 0;
     sd->expecting_acmd = false;
+    sd->multi_blk_cnt = 0;
 }
 
 static bool sd_get_inserted(SDState *sd)
@@ -488,6 +490,7 @@ 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_UINT32(erase_start, SDState),
         VMSTATE_UINT32(erase_end, SDState),
         VMSTATE_UINT8_ARRAY(pwd, SDState, 16),
@@ -696,6 +699,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_cnt != 0 && !(req.cmd == 18 || req.cmd == 25)) {
+        sd->multi_blk_cnt = 0;
+    }
+
     DPRINTF("CMD%d 0x%08x state %d\n", req.cmd, req.arg, sd->state);
     switch (req.cmd) {
     /* Basic commands (Class 0 and Class 1) */
@@ -991,6 +1000,17 @@ 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;
+            return sd_r1;
+
+        default:
+            break;
+        }
+        break;
+
     /* Block write commands (Class 4) */
     case 24:	/* CMD24:  WRITE_SINGLE_BLOCK */
         if (sd->spi)
@@ -1590,6 +1610,14 @@ void sd_write_data(SDState *sd, uint8_t value)
             sd->csd[14] |= 0x40;
 
             /* Bzzzzzzztt .... Operation complete.  */
+            if (sd->multi_blk_cnt != 0) {
+                if (--sd->multi_blk_cnt == 0) {
+                    /* Stop! */
+                    sd->state = sd_transfer_state;
+                    break;
+                }
+            }
+
             sd->state = sd_receivingdata_state;
         }
         break;
@@ -1736,6 +1764,15 @@ 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_cnt != 0) {
+                if (--sd->multi_blk_cnt == 0) {
+                    /* Stop! */
+                    sd->state = sd_transfer_state;
+                    break;
+                }
+            }
+
             if (sd->data_start + io_len > sd->size) {
                 sd->card_status |= ADDRESS_ERROR;
                 break;
-- 
2.5.3

[Qemu-devel] [PATCH v3 2/3] 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 models a delay of 0.5ms from the first ACMD41 to the power
being up. However, it also immediately sets the power on upon seeing a
non-zero (non-enquiry) ACMD41. This speeds up UEFI boot, it should
also account for guests that simply delay after card reset and then
issue an ACMD41 that they expect will succeed.

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

You might argue that the delay timer should start on sd_reset(), and
not the first ACMD41. However, that doesn't work reliably with UEFI,
because a large delay often elapses between the two (particularly in
debug builds that do lots of printing to the serial port). If the
timer fires too early, we'll still hit the bug, but we also don't want
to set a huge timeout value, because some guests may depend on it
expiring.

 hw/sd/sd.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 77 insertions(+), 6 deletions(-)
 mode change 100644 => 100755 hw/sd/sd.c

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
old mode 100644
new mode 100755
index 8514ac7..473d4a0
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -36,6 +36,7 @@
 #include "qemu/bitmap.h"
 #include "hw/qdev-properties.h"
 #include "qemu/error-report.h"
+#include "qemu/timer.h"
 
 //#define DEBUG_SD 1
 
@@ -46,7 +47,9 @@ do { fprintf(stderr, "SD: " fmt , ## __VA_ARGS__); } while (0)
 #define DPRINTF(fmt, ...) do {} while(0)
 #endif
 
-#define ACMD41_ENQUIRY_MASK 0x00ffffff
+#define ACMD41_ENQUIRY_MASK     0x00ffffff
+#define OCR_POWER_UP            0x80000000
+#define OCR_POWER_DELAY         (get_ticks_per_sec() / 2000) /* 0.5ms */
 
 typedef enum {
     sd_r0 = 0,    /* no response */
@@ -85,6 +88,7 @@ struct SDState {
     uint32_t mode;    /* current card mode, one of SDCardModes */
     int32_t state;    /* current card state, one of SDCardStates */
     uint32_t ocr;
+    QEMUTimer *ocr_power_timer;
     uint8_t scr[8];
     uint8_t cid[16];
     uint8_t csd[16];
@@ -199,8 +203,17 @@ 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;
+}
+
+static void sd_ocr_powerup(void *opaque)
+{
+    SDState *sd = opaque;
+
+    /* Set powered up bit in OCR */
+    assert(!(sd->ocr & OCR_POWER_UP));
+    sd->ocr |= OCR_POWER_UP;
 }
 
 static void sd_set_scr(SDState *sd)
@@ -475,10 +488,44 @@ static const BlockDevOps sd_block_ops = {
     .change_media_cb = sd_cardchange,
 };
 
+static bool sd_ocr_vmstate_needed(void *opaque)
+{
+    SDState *sd = opaque;
+
+    /* Include the OCR state (and timer) if it is not yet powered up */
+    return !(sd->ocr & OCR_POWER_UP);
+}
+
+static const VMStateDescription sd_ocr_vmstate = {
+    .name = "sd-card/ocr-state",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = sd_ocr_vmstate_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(ocr, SDState),
+        VMSTATE_TIMER_PTR(ocr_power_timer, SDState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static int sd_vmstate_pre_load(void *opaque)
+{
+    SDState *sd = opaque;
+
+    /* If the OCR state is not included (prior versions, or not
+     * needed), then the OCR must be set as powered up. If the OCR state
+     * is included, this will be replaced by the state restore.
+     */
+    sd_ocr_powerup(sd);
+
+    return 0;
+}
+
 static const VMStateDescription sd_vmstate = {
     .name = "sd-card",
     .version_id = 1,
     .minimum_version_id = 1,
+    .pre_load = sd_vmstate_pre_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(mode, SDState),
         VMSTATE_INT32(state, SDState),
@@ -505,7 +552,11 @@ static const VMStateDescription sd_vmstate = {
         VMSTATE_BUFFER_POINTER_UNSAFE(buf, SDState, 1, 512),
         VMSTATE_BOOL(enable, SDState),
         VMSTATE_END_OF_LIST()
-    }
+    },
+    .subsections = (const VMStateDescription*[]) {
+        &sd_ocr_vmstate,
+        NULL
+    },
 };
 
 /* Legacy initialization function for use by non-qdevified callers */
@@ -1320,12 +1371,31 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
         }
         switch (sd->state) {
         case sd_idle_state:
+            /* If it's the first ACMD41 since reset, we need to decide
+             * whether to power up. If this is not an enquiry ACMD41,
+             * we immediately report power on and proceed below to the
+             * ready state, but if it is, we set a timer to model a
+             * delay for power up. This works around a bug in EDK2
+             * UEFI, which sends an initial enquiry ACMD41, but
+             * assumes that the card is in ready state as soon as it
+             * sees the power up bit set. */
+            if (!(sd->ocr & OCR_POWER_UP)) {
+                if ((req.arg & ACMD41_ENQUIRY_MASK) != 0) {
+                    timer_del(sd->ocr_power_timer);
+                    sd_ocr_powerup(sd);
+                } else if (!timer_pending(sd->ocr_power_timer)) {
+                    timer_mod(sd->ocr_power_timer,
+                              (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)
+                               + OCR_POWER_DELAY));
+                }
+            }
+
             /* We accept any voltage.  10000 V is nothing.
              *
-             * We don't model init delay so just advance straight to ready state
+             * Once we're powered up, we advance straight to ready state
              * unless it's an enquiry ACMD41 (bits 23:0 == 0).
              */
-            if (req.arg & ACMD41_ENQUIRY_MASK) {
+            if ((sd->ocr & OCR_POWER_UP) && (req.arg & ACMD41_ENQUIRY_MASK)) {
                 sd->state = sd_ready_state;
             }
 
@@ -1833,6 +1903,7 @@ static void sd_instance_init(Object *obj)
     SDState *sd = SD_CARD(obj);
 
     sd->enable = true;
+    sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd);
 }
 
 static void sd_realize(DeviceState *dev, Error **errp)
-- 
2.5.3

[Qemu-devel] [PATCH v3 3/3] hw/sd: use guest error logging rather than fprintf to stderr

[Andrew Baumann]

Some of these errors may be harmless (e.g. probing unimplemented
commands, or issuing CMD12 in the wrong state), and may also be quite
frequent. Spamming the standard error output isn't desirable in such
cases.

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
It might also be desirable to have a squelch mechanism for these
messages, but at least for my use-case, this is sufficient, since they
only occur during boot time.

 hw/sd/sd.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 473d4a0..4794538 100755
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1294,16 +1294,17 @@ static sd_rsp_type_t sd_normal_command(SDState *sd,
 
     default:
     bad_cmd:
-        fprintf(stderr, "SD: Unknown CMD%i\n", req.cmd);
+        qemu_log_mask(LOG_GUEST_ERROR, "SD: Unknown CMD%i\n", req.cmd);
         return sd_illegal;
 
     unimplemented_cmd:
         /* Commands that are recognised but not yet implemented in SPI mode.  */
-        fprintf(stderr, "SD: CMD%i not implemented in SPI mode\n", req.cmd);
+        qemu_log_mask(LOG_UNIMP, "SD: CMD%i not implemented in SPI mode\n",
+                      req.cmd);
         return sd_illegal;
     }
 
-    fprintf(stderr, "SD: CMD%i in a wrong state\n", req.cmd);
+    qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state\n", req.cmd);
     return sd_illegal;
 }
 
@@ -1435,7 +1436,7 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
         return sd_normal_command(sd, req);
     }
 
-    fprintf(stderr, "SD: ACMD%i in a wrong state\n", req.cmd);
+    qemu_log_mask(LOG_GUEST_ERROR, "SD: ACMD%i in a wrong state\n", req.cmd);
     return sd_illegal;
 }
 
@@ -1478,7 +1479,7 @@ int sd_do_command(SDState *sd, SDRequest *req,
         if (!cmd_valid_while_locked(sd, req)) {
             sd->card_status |= ILLEGAL_COMMAND;
             sd->expecting_acmd = false;
-            fprintf(stderr, "SD: Card is locked\n");
+            qemu_log_mask(LOG_GUEST_ERROR, "SD: Card is locked\n");
             rtype = sd_illegal;
             goto send_response;
         }
@@ -1636,7 +1637,8 @@ void sd_write_data(SDState *sd, uint8_t value)
         return;
 
     if (sd->state != sd_receivingdata_state) {
-        fprintf(stderr, "sd_write_data: not in Receiving-Data state\n");
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "sd_write_data: not in Receiving-Data state\n");
         return;
     }
 
@@ -1755,7 +1757,7 @@ void sd_write_data(SDState *sd, uint8_t value)
         break;
 
     default:
-        fprintf(stderr, "sd_write_data: unknown command\n");
+        qemu_log_mask(LOG_GUEST_ERROR, "sd_write_data: unknown command\n");
         break;
     }
 }
@@ -1770,7 +1772,8 @@ uint8_t sd_read_data(SDState *sd)
         return 0x00;
 
     if (sd->state != sd_sendingdata_state) {
-        fprintf(stderr, "sd_read_data: not in Sending-Data state\n");
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "sd_read_data: not in Sending-Data state\n");
         return 0x00;
     }
 
@@ -1881,7 +1884,7 @@ uint8_t sd_read_data(SDState *sd)
         break;
 
     default:
-        fprintf(stderr, "sd_read_data: unknown command\n");
+        qemu_log_mask(LOG_GUEST_ERROR, "sd_read_data: unknown command\n");
         return 0x00;
     }
 
-- 
2.5.3

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

[Peter Maydell]

On 1 February 2016 at 22:15, 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 models a delay of 0.5ms from the first ACMD41 to the power
> being up. However, it also immediately sets the power on upon seeing a
> non-zero (non-enquiry) ACMD41. This speeds up UEFI boot, it should
> also account for guests that simply delay after card reset and then
> issue an ACMD41 that they expect will succeed.
>
> [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.

Have you filed it as an EDK2 bug, just out of interest?

> -#define ACMD41_ENQUIRY_MASK 0x00ffffff
> +#define ACMD41_ENQUIRY_MASK     0x00ffffff
> +#define OCR_POWER_UP            0x80000000
> +#define OCR_POWER_DELAY         (get_ticks_per_sec() / 2000) /* 0.5ms */

It's kind of odd to have something here scaled by get_ticks_per_sec(),
but then later add it to a pure nanoseconds value. (It works because
get_ticks_per_sec() always returns a value indicating 1 tick per ns.)
I think it would be cleaner to:
 * have this #define be a nanosecond value, with no call to
   get_ticks_per_sec()
   (we have a NANOSECONDS_PER_SECOND constant if you want it)
 * call timer_mod_ns() rather than timer_mod()

The ticks-per-sec stuff is legacy which we don't need for new code.

>  /* Legacy initialization function for use by non-qdevified callers */
> @@ -1320,12 +1371,31 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>          }
>          switch (sd->state) {
>          case sd_idle_state:
> +            /* If it's the first ACMD41 since reset, we need to decide
> +             * whether to power up. If this is not an enquiry ACMD41,
> +             * we immediately report power on and proceed below to the
> +             * ready state, but if it is, we set a timer to model a
> +             * delay for power up. This works around a bug in EDK2
> +             * UEFI, which sends an initial enquiry ACMD41, but
> +             * assumes that the card is in ready state as soon as it
> +             * sees the power up bit set. */
> +            if (!(sd->ocr & OCR_POWER_UP)) {
> +                if ((req.arg & ACMD41_ENQUIRY_MASK) != 0) {
> +                    timer_del(sd->ocr_power_timer);
> +                    sd_ocr_powerup(sd);
> +                } else if (!timer_pending(sd->ocr_power_timer)) {
> +                    timer_mod(sd->ocr_power_timer,
> +                              (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)
> +                               + OCR_POWER_DELAY));
> +                }
> +            }
> +
>              /* We accept any voltage.  10000 V is nothing.
>               *
> -             * We don't model init delay so just advance straight to ready state
> +             * Once we're powered up, we advance straight to ready state
>               * unless it's an enquiry ACMD41 (bits 23:0 == 0).
>               */
> -            if (req.arg & ACMD41_ENQUIRY_MASK) {
> +            if ((sd->ocr & OCR_POWER_UP) && (req.arg & ACMD41_ENQUIRY_MASK)) {
>                  sd->state = sd_ready_state;
>              }

Isn't (sd->ocr & OCR_POWER_UP) redundant in this check? If
(req.arg & ACMD41_ENQUIRY_MASK) is true then either:
 (a) OCR_POWER_UP was set when we came in to the function
 (b) OCR_POWER_UP wasn't set, but we went through the code path that
     deletes the timer and calls sd_ocr_powerup(), which will set
     OCR_POWER_UP
So the enquiry-mask bits being nonzero here implies OCR_POWER_UP must
be set.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Andrew Baumann]

Hi Peter,

> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Thursday, 4 February 2016 1:22 AM
> 
> On 1 February 2016 at 22:15, 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 models a delay of 0.5ms from the first ACMD41 to the power
> > being up. However, it also immediately sets the power on upon seeing a
> > non-zero (non-enquiry) ACMD41. This speeds up UEFI boot, it should
> > also account for guests that simply delay after card reset and then
> > issue an ACMD41 that they expect will succeed.
> >
> > [1]
> >
> https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Universal/
> Mm
> > cDxe/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.
> 
> Have you filed it as an EDK2 bug, just out of interest?

No, I haven't. I didn't see an obvious path to do so; I'm also lazy :)

> > -#define ACMD41_ENQUIRY_MASK 0x00ffffff
> > +#define ACMD41_ENQUIRY_MASK     0x00ffffff
> > +#define OCR_POWER_UP            0x80000000
> > +#define OCR_POWER_DELAY         (get_ticks_per_sec() / 2000) /* 0.5ms */
> 
> It's kind of odd to have something here scaled by get_ticks_per_sec(), but
> then later add it to a pure nanoseconds value. (It works because
> get_ticks_per_sec() always returns a value indicating 1 tick per ns.) I think it
> would be cleaner to:
>  * have this #define be a nanosecond value, with no call to
>    get_ticks_per_sec()
>    (we have a NANOSECONDS_PER_SECOND constant if you want it)
>  * call timer_mod_ns() rather than timer_mod()
> 
> The ticks-per-sec stuff is legacy which we don't need for new code.

Makes sense. I was obviously copying something legacy. I will change this.

> >  /* Legacy initialization function for use by non-qdevified callers */
> > @@ -1320,12 +1371,31 @@ static sd_rsp_type_t
> sd_app_command(SDState *sd,
> >          }
> >          switch (sd->state) {
> >          case sd_idle_state:
> > +            /* If it's the first ACMD41 since reset, we need to decide
> > +             * whether to power up. If this is not an enquiry ACMD41,
> > +             * we immediately report power on and proceed below to the
> > +             * ready state, but if it is, we set a timer to model a
> > +             * delay for power up. This works around a bug in EDK2
> > +             * UEFI, which sends an initial enquiry ACMD41, but
> > +             * assumes that the card is in ready state as soon as it
> > +             * sees the power up bit set. */
> > +            if (!(sd->ocr & OCR_POWER_UP)) {
> > +                if ((req.arg & ACMD41_ENQUIRY_MASK) != 0) {
> > +                    timer_del(sd->ocr_power_timer);
> > +                    sd_ocr_powerup(sd);
> > +                } else if (!timer_pending(sd->ocr_power_timer)) {
> > +                    timer_mod(sd->ocr_power_timer,
> > +                              (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)
> > +                               + OCR_POWER_DELAY));
> > +                }
> > +            }
> > +
> >              /* We accept any voltage.  10000 V is nothing.
> >               *
> > -             * We don't model init delay so just advance straight to ready state
> > +             * Once we're powered up, we advance straight to ready
> > + state
> >               * unless it's an enquiry ACMD41 (bits 23:0 == 0).
> >               */
> > -            if (req.arg & ACMD41_ENQUIRY_MASK) {
> > +            if ((sd->ocr & OCR_POWER_UP) && (req.arg &
> > + ACMD41_ENQUIRY_MASK)) {
> >                  sd->state = sd_ready_state;
> >              }
> 
> Isn't (sd->ocr & OCR_POWER_UP) redundant in this check? If (req.arg &
> ACMD41_ENQUIRY_MASK) is true then either:
>  (a) OCR_POWER_UP was set when we came in to the function
>  (b) OCR_POWER_UP wasn't set, but we went through the code path that
>      deletes the timer and calls sd_ocr_powerup(), which will set
>      OCR_POWER_UP
> So the enquiry-mask bits being nonzero here implies OCR_POWER_UP must
> be set.

You're right. I think this was a hangover from the previous version. I'll clean it up.

> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks!
Andrew