* [PATCH v3] failover: unregister ROM on unplug
@ 2021-07-21 9:39 Laurent Vivier
2021-07-21 11:10 ` Dr. David Alan Gilbert
2021-07-21 13:56 ` Juan Quintela
0 siblings, 2 replies; 7+ messages in thread
From: Laurent Vivier @ 2021-07-21 9:39 UTC (permalink / raw)
To: qemu-devel
Cc: Juan Quintela, Jason Wang, Michael S. Tsirkin,
Dr . David Alan Gilbert, Igor Mammedov, Jens Freimann
The intend of failover is to allow to migrate a VM with a VFIO
networking card without disrupting the network operation by switching
to a virtio-net device during the migration.
This simple change allows to test failover with a simulated device
like e1000e rather than a vfio device, even if it's useless in real
life it can help to debug failover.
This is interesting to developers that want to test failover on
a system with no vfio device. Moreover it simplifies host networking
configuration as we can use the same bridge for virtio-net and
the other failover networking device.
Without this change the migration of a system configured with failover
fails with:
...
-device virtio-net-pci,id=virtionet0,failover=on,... \
-device e1000,failover_pair_id=virtionet0,... \
...
(qemu) migrate ...
Unknown ramblock "0000:00:01.1:00.0/e1000e.rom", cannot accept migration
error while loading state for instance 0x0 of device 'ram'
load of migration failed: Invalid argument
This happens because QEMU correctly unregisters the interface vmstate but
not the ROM one. This patch fixes that.
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
Notes:
v3:
remove useless space before comma
v2:
reset has_rom to false
update commit log message
hw/net/virtio-net.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 16d20cdee52a..c0c2ec1ebb98 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3256,6 +3256,10 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
if (migration_in_setup(s) && !should_be_hidden) {
if (failover_unplug_primary(n, dev)) {
vmstate_unregister(VMSTATE_IF(dev), qdev_get_vmsd(dev), dev);
+ if (PCI_DEVICE(dev)->has_rom) {
+ PCI_DEVICE(dev)->has_rom = false;
+ vmstate_unregister_ram(&PCI_DEVICE(dev)->rom, dev);
+ }
qapi_event_send_unplug_primary(dev->id);
qatomic_set(&n->failover_primary_hidden, true);
} else {
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] failover: unregister ROM on unplug
2021-07-21 9:39 [PATCH v3] failover: unregister ROM on unplug Laurent Vivier
@ 2021-07-21 11:10 ` Dr. David Alan Gilbert
2021-07-21 11:22 ` Laurent Vivier
2021-07-21 13:41 ` Juan Quintela
2021-07-21 13:56 ` Juan Quintela
1 sibling, 2 replies; 7+ messages in thread
From: Dr. David Alan Gilbert @ 2021-07-21 11:10 UTC (permalink / raw)
To: Laurent Vivier
Cc: Juan Quintela, Jason Wang, Michael S. Tsirkin, qemu-devel,
Igor Mammedov, Jens Freimann
* Laurent Vivier (lvivier@redhat.com) wrote:
> The intend of failover is to allow to migrate a VM with a VFIO
> networking card without disrupting the network operation by switching
> to a virtio-net device during the migration.
>
> This simple change allows to test failover with a simulated device
> like e1000e rather than a vfio device, even if it's useless in real
> life it can help to debug failover.
>
> This is interesting to developers that want to test failover on
> a system with no vfio device. Moreover it simplifies host networking
> configuration as we can use the same bridge for virtio-net and
> the other failover networking device.
>
> Without this change the migration of a system configured with failover
> fails with:
>
> ...
> -device virtio-net-pci,id=virtionet0,failover=on,... \
> -device e1000,failover_pair_id=virtionet0,... \
> ...
>
> (qemu) migrate ...
>
> Unknown ramblock "0000:00:01.1:00.0/e1000e.rom", cannot accept migration
> error while loading state for instance 0x0 of device 'ram'
> load of migration failed: Invalid argument
>
> This happens because QEMU correctly unregisters the interface vmstate but
> not the ROM one. This patch fixes that.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>
> Notes:
> v3:
> remove useless space before comma
>
> v2:
> reset has_rom to false
> update commit log message
>
> hw/net/virtio-net.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 16d20cdee52a..c0c2ec1ebb98 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3256,6 +3256,10 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
> if (migration_in_setup(s) && !should_be_hidden) {
> if (failover_unplug_primary(n, dev)) {
> vmstate_unregister(VMSTATE_IF(dev), qdev_get_vmsd(dev), dev);
> + if (PCI_DEVICE(dev)->has_rom) {
> + PCI_DEVICE(dev)->has_rom = false;
> + vmstate_unregister_ram(&PCI_DEVICE(dev)->rom, dev);
> + }
Not actually originated by your fix, but....
Why doesn't failover_replug_primary re-add the vmstates?
(I did wonder if passing rom-file="" to the e1000 would help in your
testing case, but it still creates the RAM image).
Dave
> qapi_event_send_unplug_primary(dev->id);
> qatomic_set(&n->failover_primary_hidden, true);
> } else {
> --
> 2.31.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] failover: unregister ROM on unplug
2021-07-21 11:10 ` Dr. David Alan Gilbert
@ 2021-07-21 11:22 ` Laurent Vivier
2021-07-21 13:41 ` Juan Quintela
1 sibling, 0 replies; 7+ messages in thread
From: Laurent Vivier @ 2021-07-21 11:22 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Juan Quintela, Jason Wang, Michael S. Tsirkin, qemu-devel,
Igor Mammedov, Jens Freimann
On 21/07/2021 13:10, Dr. David Alan Gilbert wrote:
> * Laurent Vivier (lvivier@redhat.com) wrote:
>> The intend of failover is to allow to migrate a VM with a VFIO
>> networking card without disrupting the network operation by switching
>> to a virtio-net device during the migration.
>>
>> This simple change allows to test failover with a simulated device
>> like e1000e rather than a vfio device, even if it's useless in real
>> life it can help to debug failover.
>>
>> This is interesting to developers that want to test failover on
>> a system with no vfio device. Moreover it simplifies host networking
>> configuration as we can use the same bridge for virtio-net and
>> the other failover networking device.
>>
>> Without this change the migration of a system configured with failover
>> fails with:
>>
>> ...
>> -device virtio-net-pci,id=virtionet0,failover=on,... \
>> -device e1000,failover_pair_id=virtionet0,... \
>> ...
>>
>> (qemu) migrate ...
>>
>> Unknown ramblock "0000:00:01.1:00.0/e1000e.rom", cannot accept migration
>> error while loading state for instance 0x0 of device 'ram'
>> load of migration failed: Invalid argument
>>
>> This happens because QEMU correctly unregisters the interface vmstate but
>> not the ROM one. This patch fixes that.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>
>> Notes:
>> v3:
>> remove useless space before comma
>>
>> v2:
>> reset has_rom to false
>> update commit log message
>>
>> hw/net/virtio-net.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 16d20cdee52a..c0c2ec1ebb98 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -3256,6 +3256,10 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
>> if (migration_in_setup(s) && !should_be_hidden) {
>> if (failover_unplug_primary(n, dev)) {
>> vmstate_unregister(VMSTATE_IF(dev), qdev_get_vmsd(dev), dev);
>> + if (PCI_DEVICE(dev)->has_rom) {
>> + PCI_DEVICE(dev)->has_rom = false;
>> + vmstate_unregister_ram(&PCI_DEVICE(dev)->rom, dev);
>> + }
>
> Not actually originated by your fix, but....
>
> Why doesn't failover_replug_primary re-add the vmstates?
Good point.
I'm going to check but I think the vmstates are re-added by the hotplug operations that
are used in failover_replug_primary().
Thanks,
Laurent
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] failover: unregister ROM on unplug
2021-07-21 11:10 ` Dr. David Alan Gilbert
2021-07-21 11:22 ` Laurent Vivier
@ 2021-07-21 13:41 ` Juan Quintela
2021-07-21 13:45 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 7+ messages in thread
From: Juan Quintela @ 2021-07-21 13:41 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Laurent Vivier, Michael S. Tsirkin, Jason Wang, qemu-devel,
Igor Mammedov, Jens Freimann
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Laurent Vivier (lvivier@redhat.com) wrote:
>> The intend of failover is to allow to migrate a VM with a VFIO
>> networking card without disrupting the network operation by switching
>> to a virtio-net device during the migration.
>>
>> This simple change allows to test failover with a simulated device
>> like e1000e rather than a vfio device, even if it's useless in real
>> life it can help to debug failover.
>>
>> This is interesting to developers that want to test failover on
>> a system with no vfio device. Moreover it simplifies host networking
>> configuration as we can use the same bridge for virtio-net and
>> the other failover networking device.
>>
>> Without this change the migration of a system configured with failover
>> fails with:
>>
>> ...
>> -device virtio-net-pci,id=virtionet0,failover=on,... \
>> -device e1000,failover_pair_id=virtionet0,... \
>> ...
>>
>> (qemu) migrate ...
>>
>> Unknown ramblock "0000:00:01.1:00.0/e1000e.rom", cannot accept migration
>> error while loading state for instance 0x0 of device 'ram'
>> load of migration failed: Invalid argument
>>
>> This happens because QEMU correctly unregisters the interface vmstate but
>> not the ROM one. This patch fixes that.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>
>> Notes:
>> v3:
>> remove useless space before comma
>>
>> v2:
>> reset has_rom to false
>> update commit log message
>>
>> hw/net/virtio-net.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 16d20cdee52a..c0c2ec1ebb98 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -3256,6 +3256,10 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
>> if (migration_in_setup(s) && !should_be_hidden) {
>> if (failover_unplug_primary(n, dev)) {
>> vmstate_unregister(VMSTATE_IF(dev), qdev_get_vmsd(dev), dev);
>> + if (PCI_DEVICE(dev)->has_rom) {
>> + PCI_DEVICE(dev)->has_rom = false;
>> + vmstate_unregister_ram(&PCI_DEVICE(dev)->rom, dev);
>> + }
>
> Not actually originated by your fix, but....
>
> Why doesn't failover_replug_primary re-add the vmstates?
Because we can't migrate until the "unplug" has happened.
Yes, it is a mess.
I think this is the saner patch that I can think of for that
functionality.
What I wonder is why we register rom as ram, but I guess that the rom
can be updated from userspace, or who knows.
Later, Juan.
> (I did wonder if passing rom-file="" to the e1000 would help in your
> testing case, but it still creates the RAM image).
>
> Dave
>
>> qapi_event_send_unplug_primary(dev->id);
>> qatomic_set(&n->failover_primary_hidden, true);
>> } else {
>> --
>> 2.31.1
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] failover: unregister ROM on unplug
2021-07-21 13:41 ` Juan Quintela
@ 2021-07-21 13:45 ` Dr. David Alan Gilbert
2021-07-21 14:01 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 7+ messages in thread
From: Dr. David Alan Gilbert @ 2021-07-21 13:45 UTC (permalink / raw)
To: Juan Quintela
Cc: Laurent Vivier, Michael S. Tsirkin, Jason Wang, qemu-devel,
Igor Mammedov, Jens Freimann
* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Laurent Vivier (lvivier@redhat.com) wrote:
> >> The intend of failover is to allow to migrate a VM with a VFIO
> >> networking card without disrupting the network operation by switching
> >> to a virtio-net device during the migration.
> >>
> >> This simple change allows to test failover with a simulated device
> >> like e1000e rather than a vfio device, even if it's useless in real
> >> life it can help to debug failover.
> >>
> >> This is interesting to developers that want to test failover on
> >> a system with no vfio device. Moreover it simplifies host networking
> >> configuration as we can use the same bridge for virtio-net and
> >> the other failover networking device.
> >>
> >> Without this change the migration of a system configured with failover
> >> fails with:
> >>
> >> ...
> >> -device virtio-net-pci,id=virtionet0,failover=on,... \
> >> -device e1000,failover_pair_id=virtionet0,... \
> >> ...
> >>
> >> (qemu) migrate ...
> >>
> >> Unknown ramblock "0000:00:01.1:00.0/e1000e.rom", cannot accept migration
> >> error while loading state for instance 0x0 of device 'ram'
> >> load of migration failed: Invalid argument
> >>
> >> This happens because QEMU correctly unregisters the interface vmstate but
> >> not the ROM one. This patch fixes that.
> >>
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> ---
> >>
> >> Notes:
> >> v3:
> >> remove useless space before comma
> >>
> >> v2:
> >> reset has_rom to false
> >> update commit log message
> >>
> >> hw/net/virtio-net.c | 4 ++++
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index 16d20cdee52a..c0c2ec1ebb98 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -3256,6 +3256,10 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
> >> if (migration_in_setup(s) && !should_be_hidden) {
> >> if (failover_unplug_primary(n, dev)) {
> >> vmstate_unregister(VMSTATE_IF(dev), qdev_get_vmsd(dev), dev);
> >> + if (PCI_DEVICE(dev)->has_rom) {
> >> + PCI_DEVICE(dev)->has_rom = false;
> >> + vmstate_unregister_ram(&PCI_DEVICE(dev)->rom, dev);
> >> + }
> >
> > Not actually originated by your fix, but....
> >
> > Why doesn't failover_replug_primary re-add the vmstates?
>
> Because we can't migrate until the "unplug" has happened.
> Yes, it is a mess.
But if the migrate fails, shouldn't it add it back?
Dave
> I think this is the saner patch that I can think of for that
> functionality.
>
> What I wonder is why we register rom as ram, but I guess that the rom
> can be updated from userspace, or who knows.
>
> Later, Juan.
>
> > (I did wonder if passing rom-file="" to the e1000 would help in your
> > testing case, but it still creates the RAM image).
> >
> > Dave
> >
> >> qapi_event_send_unplug_primary(dev->id);
> >> qatomic_set(&n->failover_primary_hidden, true);
> >> } else {
> >> --
> >> 2.31.1
> >>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] failover: unregister ROM on unplug
2021-07-21 9:39 [PATCH v3] failover: unregister ROM on unplug Laurent Vivier
2021-07-21 11:10 ` Dr. David Alan Gilbert
@ 2021-07-21 13:56 ` Juan Quintela
1 sibling, 0 replies; 7+ messages in thread
From: Juan Quintela @ 2021-07-21 13:56 UTC (permalink / raw)
To: Laurent Vivier
Cc: Michael S. Tsirkin, Jason Wang, qemu-devel,
Dr . David Alan Gilbert, Igor Mammedov, Jens Freimann
Laurent Vivier <lvivier@redhat.com> wrote:
> The intend of failover is to allow to migrate a VM with a VFIO
> networking card without disrupting the network operation by switching
> to a virtio-net device during the migration.
>
> This simple change allows to test failover with a simulated device
> like e1000e rather than a vfio device, even if it's useless in real
> life it can help to debug failover.
>
> This is interesting to developers that want to test failover on
> a system with no vfio device. Moreover it simplifies host networking
> configuration as we can use the same bridge for virtio-net and
> the other failover networking device.
>
> Without this change the migration of a system configured with failover
> fails with:
>
> ...
> -device virtio-net-pci,id=virtionet0,failover=on,... \
> -device e1000,failover_pair_id=virtionet0,... \
> ...
>
> (qemu) migrate ...
>
> Unknown ramblock "0000:00:01.1:00.0/e1000e.rom", cannot accept migration
> error while loading state for instance 0x0 of device 'ram'
> load of migration failed: Invalid argument
>
> This happens because QEMU correctly unregisters the interface vmstate but
> not the ROM one. This patch fixes that.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
As this is only for testing.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] failover: unregister ROM on unplug
2021-07-21 13:45 ` Dr. David Alan Gilbert
@ 2021-07-21 14:01 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-21 14:01 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Juan Quintela
Cc: Laurent Vivier, Michael S. Tsirkin, Jason Wang, qemu-devel,
Igor Mammedov, Jens Freimann
On 7/21/21 3:45 PM, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>>> * Laurent Vivier (lvivier@redhat.com) wrote:
>>>> The intend of failover is to allow to migrate a VM with a VFIO
>>>> networking card without disrupting the network operation by switching
>>>> to a virtio-net device during the migration.
>>>>
>>>> This simple change allows to test failover with a simulated device
>>>> like e1000e rather than a vfio device, even if it's useless in real
>>>> life it can help to debug failover.
>>>>
>>>> This is interesting to developers that want to test failover on
>>>> a system with no vfio device. Moreover it simplifies host networking
>>>> configuration as we can use the same bridge for virtio-net and
>>>> the other failover networking device.
>>>>
>>>> Without this change the migration of a system configured with failover
>>>> fails with:
>>>>
>>>> ...
>>>> -device virtio-net-pci,id=virtionet0,failover=on,... \
>>>> -device e1000,failover_pair_id=virtionet0,... \
>>>> ...
>>>>
>>>> (qemu) migrate ...
>>>>
>>>> Unknown ramblock "0000:00:01.1:00.0/e1000e.rom", cannot accept migration
>>>> error while loading state for instance 0x0 of device 'ram'
>>>> load of migration failed: Invalid argument
>>>>
>>>> This happens because QEMU correctly unregisters the interface vmstate but
>>>> not the ROM one. This patch fixes that.
>>>>
>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>> ---
>>>>
>>>> Notes:
>>>> v3:
>>>> remove useless space before comma
>>>>
>>>> v2:
>>>> reset has_rom to false
>>>> update commit log message
>>>>
>>>> hw/net/virtio-net.c | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index 16d20cdee52a..c0c2ec1ebb98 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -3256,6 +3256,10 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
>>>> if (migration_in_setup(s) && !should_be_hidden) {
>>>> if (failover_unplug_primary(n, dev)) {
>>>> vmstate_unregister(VMSTATE_IF(dev), qdev_get_vmsd(dev), dev);
>>>> + if (PCI_DEVICE(dev)->has_rom) {
>>>> + PCI_DEVICE(dev)->has_rom = false;
>>>> + vmstate_unregister_ram(&PCI_DEVICE(dev)->rom, dev);
>>>> + }
>>>
>>> Not actually originated by your fix, but....
>>>
>>> Why doesn't failover_replug_primary re-add the vmstates?
>>
>> Because we can't migrate until the "unplug" has happened.
>> Yes, it is a mess.
>
> But if the migrate fails, shouldn't it add it back?
>
> Dave
>
>> I think this is the saner patch that I can think of for that
>> functionality.
>>
>> What I wonder is why we register rom as ram, but I guess that the rom
>> can be updated from userspace, or who knows.
Unlikely, and if we can, this is a bug.
We call memory_region_init_rom() in pci_add_option_rom().
* memory_region_init_rom: Initialize a ROM memory region.
*
* This has the same effect as calling memory_region_init_ram()
* and then marking the resulting region read-only with
* memory_region_set_readonly(). This includes arranging for the
* contents to be migrated.
I agree it would be clearer to have a vmstate_unregister_rom()
function internally calling vmstate_unregister_ram().
>>
>> Later, Juan.
>>
>>> (I did wonder if passing rom-file="" to the e1000 would help in your
>>> testing case, but it still creates the RAM image).
>>>
>>> Dave
>>>
>>>> qapi_event_send_unplug_primary(dev->id);
>>>> qatomic_set(&n->failover_primary_hidden, true);
>>>> } else {
>>>> --
>>>> 2.31.1
>>>>
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-21 14:03 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21 9:39 [PATCH v3] failover: unregister ROM on unplug Laurent Vivier
2021-07-21 11:10 ` Dr. David Alan Gilbert
2021-07-21 11:22 ` Laurent Vivier
2021-07-21 13:41 ` Juan Quintela
2021-07-21 13:45 ` Dr. David Alan Gilbert
2021-07-21 14:01 ` Philippe Mathieu-Daudé
2021-07-21 13:56 ` Juan Quintela
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.