On Wed, Oct 16, 2019 at 06:39:58PM -0600, Alex Williamson wrote:
On Wed, 16 Oct 2019 22:18:47 +0200
Jens Freimann <jfreim...@redhat.com> wrote:
On Tue, Oct 15, 2019 at 07:52:12PM -0600, Alex Williamson wrote:
>On Fri, 11 Oct 2019 13:20:15 +0200
>Jens Freimann <jfreim...@redhat.com> wrote:
>
>> As usual block all vfio-pci devices from being migrated, but make an
>> exception for failover primary devices. This is achieved by setting
>> unmigratable to 0 but also add a migration blocker for all vfio-pci
>> devices except failover primary devices. These will be unplugged before
>> migration happens by the migration handler of the corresponding
>> virtio-net standby device.
>>
>> Signed-off-by: Jens Freimann <jfreim...@redhat.com>
>> ---
>> hw/vfio/pci.c | 35 ++++++++++++++++++++++++++++++++++-
>> hw/vfio/pci.h | 2 ++
>> 2 files changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index c5e6fe61cb..64cf8e07d9 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -40,6 +40,9 @@
>> #include "pci.h"
>> #include "trace.h"
>> #include "qapi/error.h"
>> +#include "migration/blocker.h"
>> +#include "qemu/option.h"
>> +#include "qemu/option_int.h"
>>
>> #define TYPE_VFIO_PCI "vfio-pci"
>> #define PCI_VFIO(obj) OBJECT_CHECK(VFIOPCIDevice, obj, TYPE_VFIO_PCI)
>> @@ -2698,6 +2701,12 @@ static void
vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>> vdev->req_enabled = false;
>> }
>>
>> +static int has_net_failover_arg(void *opaque, const char *name,
>> + const char *value, Error **errp)
>> +{
>> + return (strcmp(name, "net_failover_pair_id") == 0);
>> +}
>> +
>> static void vfio_realize(PCIDevice *pdev, Error **errp)
>> {
>> VFIOPCIDevice *vdev = PCI_VFIO(pdev);
>> @@ -2710,6 +2719,20 @@ static void vfio_realize(PCIDevice *pdev, Error
**errp)
>> int groupid;
>> int i, ret;
>> bool is_mdev;
>> + uint16_t class_id;
>> +
>> + if (qemu_opt_foreach(pdev->qdev.opts, has_net_failover_arg,
>> + (void *) pdev->qdev.opts, &err) == 0) {
>
>Why do we need a qemu_opt_foreach here versus testing
>vdev->net_failover_pair_id as you do below or similar to how we test
>sysfsdev immediately below this chunk?
We don't need it, I will change it and move it to where we check for
the PCI class.
>
>> + error_setg(&vdev->migration_blocker,
>> + "VFIO device doesn't support migration");
>> + ret = migrate_add_blocker(vdev->migration_blocker, &err);
>
>Where's the migrate_del_blocker()/error_free() for any other realize
>error or device removal?
>
>> + if (err) {
>> + error_propagate(errp, err);
>> + error_free(vdev->migration_blocker);
>> + }
>
>As Connie noted, unclear if this aborts or continues without a
>migration blocker, which would be bad.
It aborts in my test. PCI realize propagates it further and eventually
it leads to aborting qemu.
It looks like this now:
if (!pdev->net_failover_pair_id) {
error_setg(&vdev->migration_blocker,
"VFIO device doesn't support migration");
ret = migrate_add_blocker(vdev->migration_blocker, &err);
if (err) {
error_propagate(errp, err);
} else {
error_propagate(errp, vdev->migration_blocker);
}
goto error;
This unconditionally goes to error when we don't have a failover pair
set :-\
I suspect we don't want any sort of error propagate in the success
case, the migration_blocker pre-defines the error when the migration is
blocked, right? Thanks,
I shouldn't code or send mails late at night :) I'll fix it in v4
Thanks!
regards,
Jens
