It turns out that we don't reset the SD card model if it's
attached to a legacy-API controller. For new-style controllers
where the SD card object is attached to an SD_BUS, the generic
qbus infrastructure takes care of ensuring that the card's
DeviceState::reset method gets called. But for the legacy
API where the card is created by calling sd_init(), the card
object has no SD_BUS to plug into, and so the reset function
must be called manually.

This mostly had no ill effects because a guest OS will typically
perform an SD reset command as part of its device driver init
sequence. However it does mean that migration won't work, because
it's in sd_reset() that we set up the wpgrps_size field, and
without that the migration struct on reload won't match.

This patchset fixes up the four legacy controllers to call
reset on the SD card device in their controller reset functions.
The first 3 are cc:stable as migration bugfixes.

This fixes https://bugs.launchpad.net/qemu/+bug/1739378

Philippe: this probably clashes slightly with your sdcard
patchsets (and where you've updated devices to the new APIs
it becomes irrelevant), but I think we should apply these
first so we can have a simple patch to backport to the stable
tree.

Notes:
 * OMAP doesn't migrate anyway, so isn't cc:stable
 * for vexpress-a15, you also need "-machine secure=off":
   a config with trustzone emulation enabled doesn't
   successfully migrate (haven't yet debugged why)

thanks
-- PMM

Peter Maydell (4):
  hw/sd/pl181: Reset SD card on controller reset
  hw/sd/milkymist-memcard: Reset SD card on controller reset
  hw/sd/ssi-sd: Reset SD card on controller reset
  hw/sd/omap_mmc: Reset SD card on controller reset

 hw/sd/milkymist-memcard.c |  4 ++++
 hw/sd/omap_mmc.c          | 14 ++++++++++----
 hw/sd/pl181.c             |  4 ++++
 hw/sd/ssi-sd.c            | 24 +++++++++++++++++++++++-
 4 files changed, 41 insertions(+), 5 deletions(-)

-- 
2.7.4

Hi Peter,

On 01/09/2018 11:01 AM, Peter Maydell wrote:
> It turns out that we don't reset the SD card model if it's
> attached to a legacy-API controller. For new-style controllers
> where the SD card object is attached to an SD_BUS, the generic
> qbus infrastructure takes care of ensuring that the card's
> DeviceState::reset method gets called. But for the legacy
> API where the card is created by calling sd_init(), the card
> object has no SD_BUS to plug into, and so the reset function
> must be called manually.
> 
> This mostly had no ill effects because a guest OS will typically
> perform an SD reset command as part of its device driver init
> sequence. However it does mean that migration won't work, because
> it's in sd_reset() that we set up the wpgrps_size field, and
> without that the migration struct on reload won't match.
> 
> This patchset fixes up the four legacy controllers to call
> reset on the SD card device in their controller reset functions.
> The first 3 are cc:stable as migration bugfixes.
> 
> This fixes https://bugs.launchpad.net/qemu/+bug/1739378
> 
> Philippe: this probably clashes slightly with your sdcard
> patchsets (and where you've updated devices to the new APIs
> it becomes irrelevant), but I think we should apply these
> first so we can have a simple patch to backport to the stable
> tree.

Sure, it makes sens.

I didn't test migrating a card (but have a "sdcard migration qtest"
entry in my TODO).

this series:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> Notes:
>  * OMAP doesn't migrate anyway, so isn't cc:stable
>  * for vexpress-a15, you also need "-machine secure=off":
>    a config with trustzone emulation enabled doesn't
>    successfully migrate (haven't yet debugged why)
> 
> thanks
> -- PMM
> 
> Peter Maydell (4):
>   hw/sd/pl181: Reset SD card on controller reset
>   hw/sd/milkymist-memcard: Reset SD card on controller reset
>   hw/sd/ssi-sd: Reset SD card on controller reset
>   hw/sd/omap_mmc: Reset SD card on controller reset
> 
>  hw/sd/milkymist-memcard.c |  4 ++++
>  hw/sd/omap_mmc.c          | 14 ++++++++++----
>  hw/sd/pl181.c             |  4 ++++
>  hw/sd/ssi-sd.c            | 24 +++++++++++++++++++++++-
>  4 files changed, 41 insertions(+), 5 deletions(-)
> 

On 9 January 2018 at 16:13, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> I didn't test migrating a card (but have a "sdcard migration qtest"
> entry in my TODO).

I usually test not by actually migrating but using 'savevm foo' in
the monitor to save a snapshot and then -loadvm foo on the command
line to resume it.

thanks
-- PMM

On 9 January 2018 at 16:13, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,
>
> On 01/09/2018 11:01 AM, Peter Maydell wrote:
>> It turns out that we don't reset the SD card model if it's
>> attached to a legacy-API controller. For new-style controllers
>> where the SD card object is attached to an SD_BUS, the generic
>> qbus infrastructure takes care of ensuring that the card's
>> DeviceState::reset method gets called. But for the legacy
>> API where the card is created by calling sd_init(), the card
>> object has no SD_BUS to plug into, and so the reset function
>> must be called manually.
>>
>> This mostly had no ill effects because a guest OS will typically
>> perform an SD reset command as part of its device driver init
>> sequence. However it does mean that migration won't work, because
>> it's in sd_reset() that we set up the wpgrps_size field, and
>> without that the migration struct on reload won't match.
>>
>> This patchset fixes up the four legacy controllers to call
>> reset on the SD card device in their controller reset functions.
>> The first 3 are cc:stable as migration bugfixes.
>>
>> This fixes https://bugs.launchpad.net/qemu/+bug/1739378
>>
>> Philippe: this probably clashes slightly with your sdcard
>> patchsets (and where you've updated devices to the new APIs
>> it becomes irrelevant), but I think we should apply these
>> first so we can have a simple patch to backport to the stable
>> tree.
>
> Sure, it makes sens.
>
> I didn't test migrating a card (but have a "sdcard migration qtest"
> entry in my TODO).
>
> this series:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks for the review. I've applied this series to target-arm.next
(with the fix to the milkymist commit message and the removal
of s->mod = SSI_SD_CMD from the ssi-sd realize function that you
suggested).

-- PMM