* [PATCH for-6.0] esp: fix setting of ESPState mig_version_id when launching QEMU with -S option
@ 2021-04-07 12:48 Mark Cave-Ayland
2021-04-10 5:56 ` Thomas Huth
0 siblings, 1 reply; 4+ messages in thread
From: Mark Cave-Ayland @ 2021-04-07 12:48 UTC (permalink / raw)
To: qemu-devel
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);
#endif
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH for-6.0] esp: fix setting of ESPState mig_version_id when launching QEMU with -S option
2021-04-07 12:48 [PATCH for-6.0] esp: fix setting of ESPState mig_version_id when launching QEMU with -S option Mark Cave-Ayland
@ 2021-04-10 5:56 ` Thomas Huth
2021-04-10 8:31 ` Mark Cave-Ayland
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Huth @ 2021-04-10 5:56 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel, Juan Quintela, David Alan Gilbert,
Paolo Bonzini
Cc: QEMU Trivial
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?
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH for-6.0] esp: fix setting of ESPState mig_version_id when launching QEMU with -S option
2021-04-10 5:56 ` Thomas Huth
@ 2021-04-10 8:31 ` Mark Cave-Ayland
2021-04-10 13:58 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 4+ messages in thread
From: Mark Cave-Ayland @ 2021-04-10 8:31 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, Juan Quintela, David Alan Gilbert,
Paolo Bonzini
Cc: QEMU Trivial
On 10/04/2021 06:56, Thomas Huth wrote:
> 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?
Previously I've considered the ESP patches to be SCSI, although the large ESP
patchset was given approval to go via another tree which is why that was eventually
merged via qemu-sparc.
I don't mind doing the same again although I'm still waiting for the final nod for
this and the ESP security fixes for 6.0 (see
https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01479.html).
Thoughts/other ideas?
ATB,
Mark.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH for-6.0] esp: fix setting of ESPState mig_version_id when launching QEMU with -S option
2021-04-10 8:31 ` Mark Cave-Ayland
@ 2021-04-10 13:58 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-10 13:58 UTC (permalink / raw)
To: Mark Cave-Ayland, Thomas Huth, qemu-devel, Juan Quintela,
David Alan Gilbert, Paolo Bonzini
Cc: QEMU Trivial, Fam Zheng
On 4/10/21 10:31 AM, Mark Cave-Ayland wrote:
> On 10/04/2021 06:56, Thomas Huth wrote:
>
>> 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?
>
> Previously I've considered the ESP patches to be SCSI, although the
> large ESP patchset was given approval to go via another tree which is
> why that was eventually merged via qemu-sparc.
>
> I don't mind doing the same again although I'm still waiting for the
> final nod for this and the ESP security fixes for 6.0 (see
> https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01479.html).
>
> Thoughts/other ideas?
It makes sense to have this go via the SCSI tree, but the maintainers
are pretty busy (you forgot to Cc Fam in both series). Maybe with an Ack
from them you could take the ESP patches via the SPARC tree?
Regards,
Phil.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-10 13:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-07 12:48 [PATCH for-6.0] esp: fix setting of ESPState mig_version_id when launching QEMU with -S option Mark Cave-Ayland
2021-04-10 5:56 ` Thomas Huth
2021-04-10 8:31 ` Mark Cave-Ayland
2021-04-10 13:58 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).