| Message ID | 20230120120133.666993-3-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix SD card migration | expand |
+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; > }
* 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; > > } >
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.
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; }
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(-)