[RFC PATCH 0/2] fix SD card migration

[RFC PATCH 0/2] fix SD card migration

[Daniel Henrique Barboza]

Hi, 

I found this bug by accident when doing avocado tests with the RISC-V
machines. Trying to migrate the sifive_u machine, which always has a SD
card, fails every time:

qemu-system-riscv64: ../hw/sd/sd.c:297: sd_ocr_powerup: Assertion
`!FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP)' failed.

I'm sending it as RFC because I am not sure if this fix is a bandaid for
something else that should be worked on. The code in question was
introduced a while ago to circumvent a power on bug with EDK2, where a
timer was introduced to power on the card after receiving a ACMD41
event/command.  There is a possibility that the assumptions made back
then no longer hold true.

Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
Cc: Bin Meng <bin.meng@windriver.com>

Daniel Henrique Barboza (2):
  hw/sd/sd.c: add sd_card_powered_up()
  hw/sd: skip double power-up in sd_vmstate_pre_load()

 hw/sd/sd.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

-- 
2.39.0

[RFC PATCH 1/2] hw/sd/sd.c: add sd_card_powered_up()

[Daniel Henrique Barboza]

We're going to add another verification with CARD_POWER_UP. Do a helper
to make the code easier to follow.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/sd/sd.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index da5bdd134a..bd88c1a8f0 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -289,12 +289,17 @@ FIELD(OCR, CARD_POWER_UP,              31,  1)
                                | R_OCR_CARD_CAPACITY_MASK \
                                | R_OCR_CARD_POWER_UP_MASK)
 
+static bool sd_card_powered_up(SDState *sd)
+{
+    return FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP);
+}
+
 static void sd_ocr_powerup(void *opaque)
 {
     SDState *sd = opaque;
 
     trace_sdcard_powerup();
-    assert(!FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP));
+    assert(!sd_card_powered_up(sd));
 
     /* card power-up OK */
     sd->ocr = FIELD_DP32(sd->ocr, OCR, CARD_POWER_UP, 1);
@@ -640,7 +645,7 @@ 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 !FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP);
+    return !sd_card_powered_up(sd);
 }
 
 static const VMStateDescription sd_ocr_vmstate = {
@@ -1616,7 +1621,7 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
          * 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 (!FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP)) {
+        if (!sd_card_powered_up(sd)) {
             if ((req.arg & ACMD41_ENQUIRY_MASK) != 0) {
                 timer_del(sd->ocr_power_timer);
                 sd_ocr_powerup(sd);
-- 
2.39.0

[RFC PATCH 2/2] hw/sd: skip double power-up in sd_vmstate_pre_load()

[Daniel Henrique Barboza]

At this moment any migration with the RISC-V sifive_u machine
fails with the following error:

qemu-system-riscv64: ../hw/sd/sd.c:297: sd_ocr_powerup: Assertion
`!FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP)' failed.

The assert was introduced by dd26eb43337a ("hw/sd: model a power-up
delay, as a workaround for an EDK2 bug"). It introduced a delayed timer
of 0.5ms to power up the card after the first ACMD41 command. The assert
prevents the card from being turned on twice.

When migrating a machine that uses a sd card, e.g. RISC-V sifive_u, the
card is turned on during machine_init() in both source and destination
hosts. When the migration stream finishes in the destination, the
pre_load() hook will attempt to turn on the card before loading its
vmstate. The assert() is always going to hit because the card was
already on.

Change sd_vmstate_pre_load() to check first if the sd card is turned on
before executing a sd_ocr_powerup() and triggering the assert.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/sd/sd.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index bd88c1a8f0..4add719643 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -664,11 +664,19 @@ static int sd_vmstate_pre_load(void *opaque)
 {
     SDState *sd = opaque;
 
-    /* If the OCR state is not included (prior versions, or not
+    /*
+     * 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.
+     *
+     * However, there's a chance that the board will powerup the SD
+     * before reaching INMIGRATE state in the destination host.
+     * Powering up the SD again in this case will cause an assert
+     * inside sd_ocr_powerup(). Skip sd_ocr_powerup() in this case.
      */
-    sd_ocr_powerup(sd);
+    if (!sd_card_powered_up(sd)) {
+        sd_ocr_powerup(sd);
+    }
 
     return 0;
 }
-- 
2.39.0

Re: [RFC PATCH 2/2] hw/sd: skip double power-up in sd_vmstate_pre_load()

[Philippe Mathieu-Daudé]

+David / Juan / Peter for migration and timers.

On 20/1/23 13:01, Daniel Henrique Barboza wrote:
> At this moment any migration with the RISC-V sifive_u machine
> fails with the following error:
> 
> qemu-system-riscv64: ../hw/sd/sd.c:297: sd_ocr_powerup: Assertion
> `!FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP)' failed.
> 
> The assert was introduced by dd26eb43337a ("hw/sd: model a power-up
> delay, as a workaround for an EDK2 bug"). It introduced a delayed timer
> of 0.5ms to power up the card after the first ACMD41 command. The assert
> prevents the card from being turned on twice.
> 
> When migrating a machine that uses a sd card, e.g. RISC-V sifive_u, the
> card is turned on during machine_init() in both source and destination
> hosts. When the migration stream finishes in the destination, the
> pre_load() hook will attempt to turn on the card before loading its
> vmstate. The assert() is always going to hit because the card was
> already on.
> 
> Change sd_vmstate_pre_load() to check first if the sd card is turned on
> before executing a sd_ocr_powerup() and triggering the assert.
> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   hw/sd/sd.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index bd88c1a8f0..4add719643 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -664,11 +664,19 @@ static int sd_vmstate_pre_load(void *opaque)
>   {
>       SDState *sd = opaque;
>   
> -    /* If the OCR state is not included (prior versions, or not
> +    /*
> +     * 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.
> +     *
> +     * However, there's a chance that the board will powerup the SD
> +     * before reaching INMIGRATE state in the destination host.
> +     * Powering up the SD again in this case will cause an assert
> +     * inside sd_ocr_powerup(). Skip sd_ocr_powerup() in this case.
>        */
> -    sd_ocr_powerup(sd);
> +    if (!sd_card_powered_up(sd)) {
> +        sd_ocr_powerup(sd);
> +    }
>   
>       return 0;
>   }

Re: [RFC PATCH 2/2] hw/sd: skip double power-up in sd_vmstate_pre_load()

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@linaro.org) wrote:
> +David / Juan / Peter for migration and timers.
> 
> On 20/1/23 13:01, Daniel Henrique Barboza wrote:
> > At this moment any migration with the RISC-V sifive_u machine
> > fails with the following error:
> > 
> > qemu-system-riscv64: ../hw/sd/sd.c:297: sd_ocr_powerup: Assertion
> > `!FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP)' failed.
> > 
> > The assert was introduced by dd26eb43337a ("hw/sd: model a power-up
> > delay, as a workaround for an EDK2 bug"). It introduced a delayed timer
> > of 0.5ms to power up the card after the first ACMD41 command. The assert
> > prevents the card from being turned on twice.
> > 
> > When migrating a machine that uses a sd card, e.g. RISC-V sifive_u, the
> > card is turned on during machine_init() in both source and destination
> > hosts.

If that's a problem, then why don't you make that machine_init code
check whether we're INMIGRATE, and skip the power on?

> When the migration stream finishes in the destination, the
> > pre_load() hook will attempt to turn on the card before loading its
> > vmstate. The assert() is always going to hit because the card was
> > already on.
> > 
> > Change sd_vmstate_pre_load() to check first if the sd card is turned on
> > before executing a sd_ocr_powerup() and triggering the assert.

Again, not knowing this device, but the other thought is a Post_load
that determines if the OCR state hasn't been loaded, and then turns it
on.

Dave

> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >   hw/sd/sd.c | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index bd88c1a8f0..4add719643 100644
> > --- a/hw/sd/sd.c
> > +++ b/hw/sd/sd.c
> > @@ -664,11 +664,19 @@ static int sd_vmstate_pre_load(void *opaque)
> >   {
> >       SDState *sd = opaque;
> > -    /* If the OCR state is not included (prior versions, or not
> > +    /*
> > +     * 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.
> > +     *
> > +     * However, there's a chance that the board will powerup the SD
> > +     * before reaching INMIGRATE state in the destination host.
> > +     * Powering up the SD again in this case will cause an assert
> > +     * inside sd_ocr_powerup(). Skip sd_ocr_powerup() in this case.
> >        */
> > -    sd_ocr_powerup(sd);
> > +    if (!sd_card_powered_up(sd)) {
> > +        sd_ocr_powerup(sd);
> > +    }
> >       return 0;
> >   }
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC PATCH 2/2] hw/sd: skip double power-up in sd_vmstate_pre_load()

[Juan Quintela]

Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> +David / Juan / Peter for migration and timers.
>
> On 20/1/23 13:01, Daniel Henrique Barboza wrote:
>> At this moment any migration with the RISC-V sifive_u machine
>> fails with the following error:
>> qemu-system-riscv64: ../hw/sd/sd.c:297: sd_ocr_powerup: Assertion
>> `!FIELD_EX32(sd->ocr, OCR, CARD_POWER_UP)' failed.
>> The assert was introduced by dd26eb43337a ("hw/sd: model a power-up
>> delay, as a workaround for an EDK2 bug"). It introduced a delayed timer
>> of 0.5ms to power up the card after the first ACMD41 command. The assert
>> prevents the card from being turned on twice.
>> When migrating a machine that uses a sd card, e.g. RISC-V sifive_u,
>> the
>> card is turned on during machine_init() in both source and destination
>> hosts. When the migration stream finishes in the destination, the
>> pre_load() hook will attempt to turn on the card before loading its
>> vmstate. The assert() is always going to hit because the card was
>> already on.
>> Change sd_vmstate_pre_load() to check first if the sd card is turned
>> on
>> before executing a sd_ocr_powerup() and triggering the assert.
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

I have no idea about sd or orc O:-)

>> ---
>>   hw/sd/sd.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index bd88c1a8f0..4add719643 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -664,11 +664,19 @@ static int sd_vmstate_pre_load(void *opaque)
>>   {
>>       SDState *sd = opaque;
>>   -    /* If the OCR state is not included (prior versions, or not
>> +    /*
>> +     * 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.
>> +     *
>> +     * However, there's a chance that the board will powerup the SD
>> +     * before reaching INMIGRATE state in the destination host.
>> +     * Powering up the SD again in this case will cause an assert
>> +     * inside sd_ocr_powerup(). Skip sd_ocr_powerup() in this case.
>>        */
>> -    sd_ocr_powerup(sd);
>> +    if (!sd_card_powered_up(sd)) {
>> +        sd_ocr_powerup(sd);
>> +    }
>>         return 0;
>>   }

But I agree with David.

You probably should not powerup the machine on the destination host.
But I don't know if that function touches any state of other devices or
if it don't matter at all.