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

[Qemu-devel] [PATCH v2 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

(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 | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 90 insertions(+), 14 deletions(-)

-- 
2.5.3

[Qemu-devel] [PATCH v2 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.

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 1a9935c..1a24933 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -92,6 +92,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];
@@ -423,6 +424,7 @@ static void sd_reset(SDState *sd)
     sd->blk_len = 0x200;
     sd->pwd_len = 0;
     sd->expecting_acmd = false;
+    sd->multi_blk_cnt = 0;
 }
 
 static void sd_cardchange(void *opaque, bool load)
@@ -455,6 +457,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),
@@ -671,6 +674,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) */
@@ -966,6 +975,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)
@@ -1564,6 +1584,14 @@ void sd_write_data(SDState *sd, uint8_t value)
             sd->data_offset = 0;
             sd->csd[14] |= 0x40;
 
+            if (sd->multi_blk_cnt != 0) {
+                if (--sd->multi_blk_cnt == 0) {
+                    /* Stop! */
+                    sd->state = sd_transfer_state;
+                    break;
+                }
+            }
+
             /* Bzzzzzzztt .... Operation complete.  */
             sd->state = sd_receivingdata_state;
         }
@@ -1711,6 +1739,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 v2 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 | 45 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 1a24933..1011785 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -33,6 +33,7 @@
 #include "sysemu/block-backend.h"
 #include "hw/sd/sd.h"
 #include "qemu/bitmap.h"
+#include "qemu/timer.h"
 
 //#define DEBUG_SD 1
 
@@ -43,7 +44,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 */
@@ -80,6 +83,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];
@@ -194,8 +198,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)
@@ -449,6 +462,8 @@ static const VMStateDescription sd_vmstate = {
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(mode, SDState),
         VMSTATE_INT32(state, SDState),
+        VMSTATE_UINT32(ocr, SDState),
+        VMSTATE_TIMER_PTR(ocr_power_timer, SDState),
         VMSTATE_UINT8_ARRAY(cid, SDState, 16),
         VMSTATE_UINT8_ARRAY(csd, SDState, 16),
         VMSTATE_UINT16(rca, SDState),
@@ -493,6 +508,7 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
     sd->spi = is_spi;
     sd->enable = true;
     sd->blk = blk;
+    sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd);
     sd_reset(sd);
     if (sd->blk) {
         /* Attach dev if not already attached.  (This call ignores an
@@ -1295,12 +1311,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;
             }
 
-- 
2.5.3

[Qemu-devel] [PATCH v2 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.

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 | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 1011785..9b76b47 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1234,16 +1234,18 @@ 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_GUEST_ERROR,
+                      "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;
 }
 
@@ -1375,7 +1377,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;
 }
 
@@ -1418,7 +1420,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;
         }
@@ -1576,7 +1578,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;
     }
 
@@ -1695,7 +1698,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;
     }
 }
@@ -1710,7 +1713,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;
     }
 
@@ -1821,7 +1825,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 v2 2/3] hw/sd: model a power-up delay, as a workaround for an EDK2 bug

[Peter Crosthwaite]

On Wed, Dec 16, 2015 at 11:02 AM, 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.
>
> 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 | 45 ++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 1a24933..1011785 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -33,6 +33,7 @@
>  #include "sysemu/block-backend.h"
>  #include "hw/sd/sd.h"
>  #include "qemu/bitmap.h"
> +#include "qemu/timer.h"
>
>  //#define DEBUG_SD 1
>
> @@ -43,7 +44,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 */
> @@ -80,6 +83,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];
> @@ -194,8 +198,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)
> @@ -449,6 +462,8 @@ static const VMStateDescription sd_vmstate = {
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(mode, SDState),
>          VMSTATE_INT32(state, SDState),
> +        VMSTATE_UINT32(ocr, SDState),
> +        VMSTATE_TIMER_PTR(ocr_power_timer, SDState),
>          VMSTATE_UINT8_ARRAY(cid, SDState, 16),
>          VMSTATE_UINT8_ARRAY(csd, SDState, 16),
>          VMSTATE_UINT16(rca, SDState),
> @@ -493,6 +508,7 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
>      sd->spi = is_spi;
>      sd->enable = true;
>      sd->blk = blk;
> +    sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd);
>      sd_reset(sd);
>      if (sd->blk) {
>          /* Attach dev if not already attached.  (This call ignores an
> @@ -1295,12 +1311,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. */

Can this be done in a non-rPI specific way by just kicking off the
timer on card reset?

Regards,
Peter

> +            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;
>              }
>
> --
> 2.5.3
>

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

[Peter Crosthwaite]

On Wed, Dec 16, 2015 at 11:02 AM, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
> 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.
>
> 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 | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 1011785..9b76b47 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1234,16 +1234,18 @@ 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_GUEST_ERROR,
> +                      "SD: CMD%i not implemented in SPI mode\n",
> +                      req.cmd);

Are these unimplemented by specification or unimplemented in QEMU? If
they are unimplemented in QEMU it should be a LOG_UNIMP.

Otherwise:

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

Regards,
Peter

>          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;
>  }
>
> @@ -1375,7 +1377,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;
>  }
>
> @@ -1418,7 +1420,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;
>          }
> @@ -1576,7 +1578,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;
>      }
>
> @@ -1695,7 +1698,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;
>      }
>  }
> @@ -1710,7 +1713,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;
>      }
>
> @@ -1821,7 +1825,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 v2 2/3] hw/sd: model a power-up delay, as a workaround for an EDK2 bug

[Andrew Baumann]

> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> Sent: Sunday, 20 December 2015 14:58
> On Wed, Dec 16, 2015 at 11:02 AM, 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.
[...]
> Can this be done in a non-rPI specific way by just kicking off the
> timer on card reset?

No :( I tried that first, but there is no value for the timeout that works reliably and isn't huge. UEFI doesn't reset the card itself (it relies on the hardware reset), and there can be a large/variable delay after power on before its gets to the SD init (particularly for debug builds, which do lots of printing to the serial port).

BTW, this is generic to UEFI; I don't believe it's Pi-specific.

Andrew

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

[Peter Crosthwaite]

On Mon, Dec 21, 2015 at 1:04 PM, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
>> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
>> Sent: Sunday, 20 December 2015 14:58
>> On Wed, Dec 16, 2015 at 11:02 AM, 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.
> [...]
>> Can this be done in a non-rPI specific way by just kicking off the
>> timer on card reset?
>
> No :( I tried that first, but there is no value for the timeout that works reliably and isn't huge. UEFI doesn't reset the card itself (it relies on the hardware reset), and there can be a large/variable delay after power on before its gets to the SD init (particularly for debug builds, which do lots of printing to the serial port).
>
> BTW, this is generic to UEFI; I don't believe it's Pi-specific.
>

Ok fair enough,

This way works and handles pretty much every case. There is one more
review comment RE the VMSD ill make on original mail.

Regards,
Peter

> Andrew

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

[Peter Crosthwaite]

On Wed, Dec 16, 2015 at 11:02 AM, 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.
>
> 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 | 45 ++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 1a24933..1011785 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -33,6 +33,7 @@
>  #include "sysemu/block-backend.h"
>  #include "hw/sd/sd.h"
>  #include "qemu/bitmap.h"
> +#include "qemu/timer.h"
>
>  //#define DEBUG_SD 1
>
> @@ -43,7 +44,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 */
> @@ -80,6 +83,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];
> @@ -194,8 +198,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)
> @@ -449,6 +462,8 @@ static const VMStateDescription sd_vmstate = {
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(mode, SDState),
>          VMSTATE_INT32(state, SDState),
> +        VMSTATE_UINT32(ocr, SDState),
> +        VMSTATE_TIMER_PTR(ocr_power_timer, SDState),

If you change the VMSTATE layout, you need to bump the version. As
this is very common code, it may have stricter version bump
requirements. Last I knew however, there was a way to add new fields
at the end of VMSD without breaking backwards compatibility. Peter or
Juan may know more.

Regards,
Peter

>          VMSTATE_UINT8_ARRAY(cid, SDState, 16),
>          VMSTATE_UINT8_ARRAY(csd, SDState, 16),
>          VMSTATE_UINT16(rca, SDState),
> @@ -493,6 +508,7 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
>      sd->spi = is_spi;
>      sd->enable = true;
>      sd->blk = blk;
> +    sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd);
>      sd_reset(sd);
>      if (sd->blk) {
>          /* Attach dev if not already attached.  (This call ignores an
> @@ -1295,12 +1311,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;
>              }
>
> --
> 2.5.3
>

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

[Andrew Baumann]

> From: qemu-devel-bounces+andrew.baumann=microsoft.com@nongnu.org
> [mailto:qemu-devel-
> bounces+andrew.baumann=microsoft.com@nongnu.org] On Behalf Of
> Peter Crosthwaite
> Sent: Monday, 21 December 2015 13:46
> On Wed, Dec 16, 2015 at 11:02 AM, 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.
[...]
> > @@ -449,6 +462,8 @@ static const VMStateDescription sd_vmstate = {
> >      .fields = (VMStateField[]) {
> >          VMSTATE_UINT32(mode, SDState),
> >          VMSTATE_INT32(state, SDState),
> > +        VMSTATE_UINT32(ocr, SDState),
> > +        VMSTATE_TIMER_PTR(ocr_power_timer, SDState),
> 
> If you change the VMSTATE layout, you need to bump the version. As
> this is very common code, it may have stricter version bump
> requirements. Last I knew however, there was a way to add new fields
> at the end of VMSD without breaking backwards compatibility. Peter or
> Juan may know more.

I'll admit that I didn't think about these issues when adding the fields, but after a (quick) look at vmstate_save_state() and vmstate_load_state(), they seem to be using named fields in a json format, so I don't think the order of fields should matter if we are adding new ones. However, if we want to be able to migrate sd instances across across this change, then we'll need to arrange for the OCR to appear already powered-on if we're coming from a previous version. Does VMState have a way to do that? Essentially I just need to specify a default value for the ocr field if coming from an old vmstate version <= 1 that differs from the value set in sd_reset().

Andrew

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

[Peter Crosthwaite]

On Mon, Dec 21, 2015 at 2:25 PM, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
>> From: qemu-devel-bounces+andrew.baumann=microsoft.com@nongnu.org
>> [mailto:qemu-devel-
>> bounces+andrew.baumann=microsoft.com@nongnu.org] On Behalf Of
>> Peter Crosthwaite
>> Sent: Monday, 21 December 2015 13:46
>> On Wed, Dec 16, 2015 at 11:02 AM, 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.
> [...]
>> > @@ -449,6 +462,8 @@ static const VMStateDescription sd_vmstate = {
>> >      .fields = (VMStateField[]) {
>> >          VMSTATE_UINT32(mode, SDState),
>> >          VMSTATE_INT32(state, SDState),
>> > +        VMSTATE_UINT32(ocr, SDState),
>> > +        VMSTATE_TIMER_PTR(ocr_power_timer, SDState),
>>
>> If you change the VMSTATE layout, you need to bump the version. As
>> this is very common code, it may have stricter version bump
>> requirements. Last I knew however, there was a way to add new fields
>> at the end of VMSD without breaking backwards compatibility. Peter or
>> Juan may know more.
>
> I'll admit that I didn't think about these issues when adding the fields, but after a (quick) look at vmstate_save_state() and vmstate_load_state(), they seem to be using named fields in a json format, so I don't think the order of fields should matter if we are adding new ones. However, if we want to be able to migrate sd instances across across this change, then we'll need to arrange for the OCR to appear already powered-on if we're coming from a previous version. Does VMState have a way to do that? Essentially I just need to specify a default value for the ocr field if coming from an old vmstate version <= 1 that differs from the value set in sd_reset().
>

You can open code post_load logic as a callback, and I think you have
access to the image version from there.

Regards,
Peter

> Andrew

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

[Andrew Baumann]

> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> Sent: Monday, 21 December 2015 14:57
> On Mon, Dec 21, 2015 at 2:25 PM, Andrew Baumann
> <Andrew.Baumann@microsoft.com> wrote:
> >> From: qemu-devel-
> bounces+andrew.baumann=microsoft.com@nongnu.org
> >> [mailto:qemu-devel-
> >> bounces+andrew.baumann=microsoft.com@nongnu.org] On Behalf Of
> >> Peter Crosthwaite
> >> Sent: Monday, 21 December 2015 13:46
> >> On Wed, Dec 16, 2015 at 11:02 AM, 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.
> > [...]
> >> > @@ -449,6 +462,8 @@ static const VMStateDescription sd_vmstate = {
> >> >      .fields = (VMStateField[]) {
> >> >          VMSTATE_UINT32(mode, SDState),
> >> >          VMSTATE_INT32(state, SDState),
> >> > +        VMSTATE_UINT32(ocr, SDState),
> >> > +        VMSTATE_TIMER_PTR(ocr_power_timer, SDState),
> >>
> >> If you change the VMSTATE layout, you need to bump the version. As
> >> this is very common code, it may have stricter version bump
> >> requirements. Last I knew however, there was a way to add new fields
> >> at the end of VMSD without breaking backwards compatibility. Peter or
> >> Juan may know more.
> >
> > I'll admit that I didn't think about these issues when adding the fields, but
> after a (quick) look at vmstate_save_state() and vmstate_load_state(), they
> seem to be using named fields in a json format, so I don't think the order of
> fields should matter if we are adding new ones. However, if we want to be
> able to migrate sd instances across across this change, then we'll need to
> arrange for the OCR to appear already powered-on if we're coming from a
> previous version. Does VMState have a way to do that? Essentially I just
> need to specify a default value for the ocr field if coming from an old vmstate
> version <= 1 that differs from the value set in sd_reset().
> >
> 
> You can open code post_load logic as a callback, and I think you have
> access to the image version from there.

So, how about something like this:

+static int sd_vmstate_post_load(void *opaque, int version_id)
+{
+    SDState *sd = opaque;
+
+    if (version_id < 2) {
+        /* prior versions did not save the OCR or model a power up
+         * delay, so we need to mirror this state */
+        sd_ocr_powerup(sd);
+    }
+
+    return 0;
+}
+
 static const VMStateDescription sd_vmstate = {
     .name = "sd-card",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
+    .post_load = sd_vmstate_post_load,
     .fields = (VMStateField[]) {

Any idea how I can easily test this? Sorry, I am not at all familiar with the vmstate/migration stuff. Is there a good/known working device model to test that uses SD? I guess I just do savevm on the old one and loadvm on the new one?

Thanks,
Andrew

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

[Peter Maydell]

On 21 December 2015 at 22:25, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:

>> If you change the VMSTATE layout, you need to bump the version. As
>> this is very common code, it may have stricter version bump
>> requirements. Last I knew however, there was a way to add new fields
>> at the end of VMSD without breaking backwards compatibility. Peter or
>> Juan may know more.
>
> I'll admit that I didn't think about these issues when adding the
> fields, but after a (quick) look at vmstate_save_state() and
> vmstate_load_state(), they seem to be using named fields in a json
> format, so I don't think the order of fields should matter if we are
> adding new ones.

The on-the-wire format is not json, and order is important; field
names are not sent.

If we care about migration compatibility I think the recommendation
is to use subsections, rather than marking fields as only existing
in particular versions. (see docs/migration.txt for a discussion
of subsections).

thanks
-- PMM

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

[Andrew Baumann]

> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Wednesday, 23 December 2015 11:21
> On 21 December 2015 at 22:25, Andrew Baumann
> <Andrew.Baumann@microsoft.com> wrote:
> 
> >> If you change the VMSTATE layout, you need to bump the version. As
> >> this is very common code, it may have stricter version bump
> >> requirements. Last I knew however, there was a way to add new fields
> >> at the end of VMSD without breaking backwards compatibility. Peter or
> >> Juan may know more.
> >
> > I'll admit that I didn't think about these issues when adding the
> > fields, but after a (quick) look at vmstate_save_state() and
> > vmstate_load_state(), they seem to be using named fields in a json
> > format, so I don't think the order of fields should matter if we are
> > adding new ones.
> 
> The on-the-wire format is not json, and order is important; field
> names are not sent.
> 
> If we care about migration compatibility I think the recommendation
  ^^^^^^^^^^
> is to use subsections, rather than marking fields as only existing
> in particular versions. (see docs/migration.txt for a discussion
> of subsections).

Do we care about migration compatibility for this code?

(As far as I can tell, this only matters for migration of SD interfaces across qemu versions, where the previous state was saved in the window between reset and driver initialisation.)

Andrew

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

[Peter Maydell]

On 16 December 2015 at 19:02, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
> 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
>
> (I'm guessing at the CC list here, since this code appears to be
> unmaintained. Apologies if I guessed wrong!)

I still have this patchset lurking on my to-review list, but I have
a feeling it's been superseded by other patches or possibly even
patches committed to master. Am I right, or does it still need review?

thanks
-- PMM

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

[Andrew Baumann]

> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Monday, 25 January 2016 05:57
> 
> On 16 December 2015 at 19:02, Andrew Baumann
> <Andrew.Baumann@microsoft.com> wrote:
> > 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
> >
> > (I'm guessing at the CC list here, since this code appears to be
> > unmaintained. Apologies if I guessed wrong!)
> 
> I still have this patchset lurking on my to-review list, but I have
> a feeling it's been superseded by other patches or possibly even
> patches committed to master. Am I right, or does it still need review?

This is the most recent version of the patch series. However, there was an unresolved question about migration compatibility for the vmstate layout (patch 2/3). First, it's not clear to me that we care about migrating SD instances across qemu versions -- is this important? If not, it seems that the simple fix is to bump the vmstate version number and be done with it. If we do need to support migration, I'm very hazy on the correct way to handle this, or how to test it. I posted a suggestion here:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg343178.html
... it would be helpful to get some guidance on that, if you want me to pursue this path.

The other two patches in the series already got a Reviewed-By from Peter C. Once we nail the migration issue, I can rebase the patches and they should be ready to go.

Thanks,
Andrew

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

[Peter Maydell]

On 23 December 2015 at 19:37, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
>> From: Peter Maydell [mailto:peter.maydell@linaro.org]
>> If we care about migration compatibility I think the recommendation
>   ^^^^^^^^^^
>> is to use subsections, rather than marking fields as only existing
>> in particular versions. (see docs/migration.txt for a discussion
>> of subsections).
>
> Do we care about migration compatibility for this code?

This is a good question. The answer might well be 'no', but it's
pretty hard to tell because the SD card model is used by a lot of
machines. (For devices that are board-specific it's much easier to say.)
I think that getting migration compatibility right is probably the better
approach; it's not that much harder.

So we should use a subsection for the new data. There's some
reasonably good documentation on how to do that in
docs/migration.txt (in the 'Subsections' subsection :-)).

> (As far as I can tell, this only matters for migration of SD interfaces
> across qemu versions, where the previous state was saved in the window
> between reset and driver initialisation.)

If you just add fields, as your patch does, then you break migration
completely across versions, whatever the state of the incoming
device, because the size of the data incoming will be different.
If you don't bump the version number in the vmstate struct then
it will go wrong in a confusing way whereby the reader gets out
of sync with the incoming data (probably resulting in an error
when the next device along tries to slurp in its data). If you
do bump the version number you at least get a comprehensible
error message. (This all happens because our migration data
stream is not self-describing: we rely on both ends having the
same interpretation of the serialised data.)

thanks
-- PMM

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

[Peter Maydell]

On 25 January 2016 at 18:06, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
> This is the most recent version of the patch series. However,
> there was an unresolved question about migration compatibility
> for the vmstate layout (patch 2/3).

Ah yes, that got lost in the Christmas holiday shuffle I think.
I've replied to that patch with my opinion.

> The other two patches in the series already got a Reviewed-By from
> Peter C. Once we nail the migration issue, I can rebase the patches
> and they should be ready to go.

You might find it easier to rebase on top of my sd card QOMification
series:
  https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04425.html

as that will probably get into master first, though hopefully there
won't be any major clashes with this series.

thanks
-- PMM

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

[Andrew Baumann]

> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Monday, 25 January 2016 10:37
> 
> On 25 January 2016 at 18:06, Andrew Baumann
> <Andrew.Baumann@microsoft.com> wrote:
> > This is the most recent version of the patch series. However,
> > there was an unresolved question about migration compatibility
> > for the vmstate layout (patch 2/3).
> 
> Ah yes, that got lost in the Christmas holiday shuffle I think.
> I've replied to that patch with my opinion.
> 
> > The other two patches in the series already got a Reviewed-By from
> > Peter C. Once we nail the migration issue, I can rebase the patches
> > and they should be ready to go.
> 
> You might find it easier to rebase on top of my sd card QOMification
> series:
>   https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04425.html
> 
> as that will probably get into master first, though hopefully there
> won't be any major clashes with this series.

Thanks. I've just sent v3 to the list, which hopefully solves the migration problem. It is based on your sd qom series, but as you expected out the conflicts are really minor.

Andrew