Re: [PATCH for-6.0] esp: fix setting of ESPState mig_version_id when launching QEMU with -S option

[Thomas Huth <thuth@redhat.com>]

On 07/04/2021 14.48, Mark Cave-Ayland wrote:
> If QEMU is launched with the -S option then the ESPState mig_version_id property
> is left unset due to the ordering of the VMState fields in the VMStateDescription
> for sysbusespscsi and pciespscsi. If the VM is migrated and restored in this
> stopped state, the version tests in the vmstate_esp VMStateDescription and
> esp_post_load() become confused causing the migration to fail.
> 
> Fix the ordering problem by moving the setting of mig_version_id to a common
> esp_pre_save() function which is invoked first by both sysbusespscsi and
> pciespscsi rather than at the point where ESPState is itself serialised into the
> migration stream.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1922611
> Fixes: 0bd005be78 ("esp: add vmstate_esp version to embedded ESPState")
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp-pci.c     | 1 +
>   hw/scsi/esp.c         | 7 ++++---
>   include/hw/scsi/esp.h | 1 +
>   3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
> index c3d3dab05e..9db10b1a48 100644
> --- a/hw/scsi/esp-pci.c
> +++ b/hw/scsi/esp-pci.c
> @@ -332,6 +332,7 @@ static const VMStateDescription vmstate_esp_pci_scsi = {
>       .name = "pciespscsi",
>       .version_id = 2,
>       .minimum_version_id = 1,
> +    .pre_save = esp_pre_save,
>       .fields = (VMStateField[]) {
>           VMSTATE_PCI_DEVICE(parent_obj, PCIESPState),
>           VMSTATE_BUFFER_UNSAFE(dma_regs, PCIESPState, 0, 8 * sizeof(uint32_t)),
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 3b9037e4f4..0037197bdb 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -1089,9 +1089,10 @@ static bool esp_is_version_5(void *opaque, int version_id)
>       return version_id == 5;
>   }
>   
> -static int esp_pre_save(void *opaque)
> +int esp_pre_save(void *opaque)
>   {
> -    ESPState *s = ESP(opaque);
> +    ESPState *s = ESP(object_resolve_path_component(
> +                      OBJECT(opaque), "esp"));
>   
>       s->mig_version_id = vmstate_esp.version_id;
>       return 0;
> @@ -1127,7 +1128,6 @@ const VMStateDescription vmstate_esp = {
>       .name = "esp",
>       .version_id = 5,
>       .minimum_version_id = 3,
> -    .pre_save = esp_pre_save,
>       .post_load = esp_post_load,
>       .fields = (VMStateField[]) {
>           VMSTATE_BUFFER(rregs, ESPState),
> @@ -1317,6 +1317,7 @@ static const VMStateDescription vmstate_sysbus_esp_scsi = {
>       .name = "sysbusespscsi",
>       .version_id = 2,
>       .minimum_version_id = 1,
> +    .pre_save = esp_pre_save,
>       .fields = (VMStateField[]) {
>           VMSTATE_UINT8_V(esp.mig_version_id, SysBusESPState, 2),
>           VMSTATE_STRUCT(esp, SysBusESPState, 0, vmstate_esp, ESPState),
> diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
> index 95088490aa..aada3680b7 100644
> --- a/include/hw/scsi/esp.h
> +++ b/include/hw/scsi/esp.h
> @@ -157,5 +157,6 @@ void esp_hard_reset(ESPState *s);
>   uint64_t esp_reg_read(ESPState *s, uint32_t saddr);
>   void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val);
>   extern const VMStateDescription vmstate_esp;
> +int esp_pre_save(void *opaque);

Reviewed-by: Thomas Huth <thuth@redhat.com>

Which tree should this patch go through? Your Sparc tree? Migration? Scsi? 
Trivial?