Re: [Qemu-devel] [PATCH 3/9] vfio: unplug failover primary device before migration
On Tue, 13 Aug 2019 08:45:49 +0200 Jens Freimann wrote: > On Mon, Aug 12, 2019 at 03:22:52PM -0600, Alex Williamson wrote: > >On Mon, 12 Aug 2019 17:18:54 +0200 > >Cornelia Huck wrote: > > > >> On Fri, 2 Aug 2019 17:05:59 +0200 > >> Jens Freimann 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 > >> > --- > >> > hw/vfio/pci.c | 24 +++- > >> > hw/vfio/pci.h | 1 + > >> > 2 files changed, 24 insertions(+), 1 deletion(-) > >> This patch interacts with support for vfio migration (last posted in > >> <1562665760-26158-1-git-send-email-kwankh...@nvidia.com>, I've not seen > >> a later version yet.) > >> > >> With that, we'd have three cases to consider: > >> 1) device is a failover primary > >> 2) device has a migration region > >> 3) none of the above > >> > >> Can 1) and 2) happen simultaneously? If yes, what should take > >> precedence? > > > >Great questions. I would assume that a user specifying this option > >intends the behavior here regardless of the device's support for > >migration, which could be made more clear and easier to test by adding > >this option to other, otherwise migratable, QEMU NICs. > > I agree and think it makes sense that if a user intentionally marks a > device as a primary device of a failover pair then it should override > the use of an existing migration region of the device. Yes, that makes sense to me as well.
Re: [Qemu-devel] [PATCH 3/9] vfio: unplug failover primary device before migration
On Mon, Aug 12, 2019 at 05:18:54PM +0200, Cornelia Huck wrote:
On Fri, 2 Aug 2019 17:05:59 +0200
Jens Freimann 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
---
hw/vfio/pci.c | 24 +++-
hw/vfio/pci.h | 1 +
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d6ae9bd4ac..398d26669b 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -35,6 +35,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)
@@ -2693,6 +2696,12 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice
*vdev)
vdev->req_enabled = false;
}
+static int has_standby_arg(void *opaque, const char *name,
+ const char *value, Error **errp)
+{
+return strcmp(name, "standby") == 0;
+}
+
static void vfio_realize(PCIDevice *pdev, Error **errp)
{
VFIOPCIDevice *vdev = PCI_VFIO(pdev);
@@ -2706,6 +2715,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
int i, ret;
bool is_mdev;
+if (qemu_opt_foreach(pdev->qdev.opts, has_standby_arg,
+ (void *) pdev->qdev.opts, ) == 0) {
+error_setg(>migration_blocker,
+"VFIO device doesn't support migration");
+ret = migrate_add_blocker(vdev->migration_blocker, );
+if (err) {
+error_propagate(errp, err);
+error_free(vdev->migration_blocker);
+}
+} else {
+pdev->qdev.allow_unplug_during_migration = true;
I think you add this only in the next patch?
you're right, will fix
+}
+
if (!vdev->vbasedev.sysfsdev) {
if (!(~vdev->host.domain || ~vdev->host.bus ||
~vdev->host.slot || ~vdev->host.function)) {
@@ -3148,7 +3170,7 @@ static Property vfio_pci_dev_properties[] = {
static const VMStateDescription vfio_pci_vmstate = {
.name = "vfio-pci",
-.unmigratable = 1,
+.unmigratable = 0,
};
static void vfio_pci_dev_class_init(ObjectClass *klass, void *data)
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 27d58fc55b..0f6f8cb395 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -169,6 +169,7 @@ typedef struct VFIOPCIDevice {
bool no_vfio_ioeventfd;
bool enable_ramfb;
VFIODisplay *dpy;
+Error *migration_blocker;
} VFIOPCIDevice;
uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
This patch interacts with support for vfio migration (last posted in
<1562665760-26158-1-git-send-email-kwankh...@nvidia.com>, I've not seen
a later version yet.)
With that, we'd have three cases to consider:
1) device is a failover primary
2) device has a migration region
3) none of the above
Can 1) and 2) happen simultaneously? If yes, what should take
precedence?
See my reply to Alex. If I understand it right nothing needs to be
explicitly enabled for the migration region by the user. So if a user
explicitly sets the failover parameters for the devie I think it makes
sense that it should take precedence.
regards,
Jens
Re: [Qemu-devel] [PATCH 3/9] vfio: unplug failover primary device before migration
On Mon, Aug 12, 2019 at 03:22:52PM -0600, Alex Williamson wrote:
On Mon, 12 Aug 2019 17:18:54 +0200
Cornelia Huck wrote:
On Fri, 2 Aug 2019 17:05:59 +0200
Jens Freimann 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
> ---
> hw/vfio/pci.c | 24 +++-
> hw/vfio/pci.h | 1 +
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index d6ae9bd4ac..398d26669b 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -35,6 +35,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)
> @@ -2693,6 +2696,12 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice
*vdev)
> vdev->req_enabled = false;
> }
>
> +static int has_standby_arg(void *opaque, const char *name,
> + const char *value, Error **errp)
> +{
> +return strcmp(name, "standby") == 0;
> +}
> +
> static void vfio_realize(PCIDevice *pdev, Error **errp)
> {
> VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> @@ -2706,6 +2715,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> int i, ret;
> bool is_mdev;
>
> +if (qemu_opt_foreach(pdev->qdev.opts, has_standby_arg,
> + (void *) pdev->qdev.opts, ) == 0) {
> +error_setg(>migration_blocker,
> +"VFIO device doesn't support migration");
> +ret = migrate_add_blocker(vdev->migration_blocker, );
> +if (err) {
> +error_propagate(errp, err);
> +error_free(vdev->migration_blocker);
> +}
> +} else {
> +pdev->qdev.allow_unplug_during_migration = true;
I think you add this only in the next patch?
> +}
> +
> if (!vdev->vbasedev.sysfsdev) {
> if (!(~vdev->host.domain || ~vdev->host.bus ||
>~vdev->host.slot || ~vdev->host.function)) {
> @@ -3148,7 +3170,7 @@ static Property vfio_pci_dev_properties[] = {
>
> static const VMStateDescription vfio_pci_vmstate = {
> .name = "vfio-pci",
> -.unmigratable = 1,
> +.unmigratable = 0,
> };
>
> static void vfio_pci_dev_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 27d58fc55b..0f6f8cb395 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -169,6 +169,7 @@ typedef struct VFIOPCIDevice {
> bool no_vfio_ioeventfd;
> bool enable_ramfb;
> VFIODisplay *dpy;
> +Error *migration_blocker;
> } VFIOPCIDevice;
>
> uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
This patch interacts with support for vfio migration (last posted in
<1562665760-26158-1-git-send-email-kwankh...@nvidia.com>, I've not seen
a later version yet.)
With that, we'd have three cases to consider:
1) device is a failover primary
2) device has a migration region
3) none of the above
Can 1) and 2) happen simultaneously? If yes, what should take
precedence?
Great questions. I would assume that a user specifying this option
intends the behavior here regardless of the device's support for
migration, which could be made more clear and easier to test by adding
this option to other, otherwise migratable, QEMU NICs.
I agree and think it makes sense that if a user intentionally marks a
device as a primary device of a failover pair then it should override
the use of an existing migration region of the device.
Also, I thought we agreed that "standby" was not a sufficiently
descriptive name for this device option and that this option would be
rejected with an error on non-Ethernet class devices[1]. Thanks,
We did agree on that, sorry. Will fix in next version.
regards,
Jens
Re: [Qemu-devel] [PATCH 3/9] vfio: unplug failover primary device before migration
On Mon, 12 Aug 2019 17:18:54 +0200
Cornelia Huck wrote:
> On Fri, 2 Aug 2019 17:05:59 +0200
> Jens Freimann 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
> > ---
> > hw/vfio/pci.c | 24 +++-
> > hw/vfio/pci.h | 1 +
> > 2 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index d6ae9bd4ac..398d26669b 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -35,6 +35,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)
> > @@ -2693,6 +2696,12 @@ static void
> > vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
> > vdev->req_enabled = false;
> > }
> >
> > +static int has_standby_arg(void *opaque, const char *name,
> > + const char *value, Error **errp)
> > +{
> > +return strcmp(name, "standby") == 0;
> > +}
> > +
> > static void vfio_realize(PCIDevice *pdev, Error **errp)
> > {
> > VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> > @@ -2706,6 +2715,19 @@ static void vfio_realize(PCIDevice *pdev, Error
> > **errp)
> > int i, ret;
> > bool is_mdev;
> >
> > +if (qemu_opt_foreach(pdev->qdev.opts, has_standby_arg,
> > + (void *) pdev->qdev.opts, ) == 0) {
> > +error_setg(>migration_blocker,
> > +"VFIO device doesn't support migration");
> > +ret = migrate_add_blocker(vdev->migration_blocker, );
> > +if (err) {
> > +error_propagate(errp, err);
> > +error_free(vdev->migration_blocker);
> > +}
> > +} else {
> > +pdev->qdev.allow_unplug_during_migration = true;
>
> I think you add this only in the next patch?
>
> > +}
> > +
> > if (!vdev->vbasedev.sysfsdev) {
> > if (!(~vdev->host.domain || ~vdev->host.bus ||
> >~vdev->host.slot || ~vdev->host.function)) {
> > @@ -3148,7 +3170,7 @@ static Property vfio_pci_dev_properties[] = {
> >
> > static const VMStateDescription vfio_pci_vmstate = {
> > .name = "vfio-pci",
> > -.unmigratable = 1,
> > +.unmigratable = 0,
> > };
> >
> > static void vfio_pci_dev_class_init(ObjectClass *klass, void *data)
> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> > index 27d58fc55b..0f6f8cb395 100644
> > --- a/hw/vfio/pci.h
> > +++ b/hw/vfio/pci.h
> > @@ -169,6 +169,7 @@ typedef struct VFIOPCIDevice {
> > bool no_vfio_ioeventfd;
> > bool enable_ramfb;
> > VFIODisplay *dpy;
> > +Error *migration_blocker;
> > } VFIOPCIDevice;
> >
> > uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
>
> This patch interacts with support for vfio migration (last posted in
> <1562665760-26158-1-git-send-email-kwankh...@nvidia.com>, I've not seen
> a later version yet.)
>
> With that, we'd have three cases to consider:
> 1) device is a failover primary
> 2) device has a migration region
> 3) none of the above
>
> Can 1) and 2) happen simultaneously? If yes, what should take
> precedence?
Great questions. I would assume that a user specifying this option
intends the behavior here regardless of the device's support for
migration, which could be made more clear and easier to test by adding
this option to other, otherwise migratable, QEMU NICs.
Also, I thought we agreed that "standby" was not a sufficiently
descriptive name for this device option and that this option would be
rejected with an error on non-Ethernet class devices[1]. Thanks,
Alex
[1] https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg04727.html
Re: [Qemu-devel] [PATCH 3/9] vfio: unplug failover primary device before migration
On Fri, 2 Aug 2019 17:05:59 +0200
Jens Freimann 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
> ---
> hw/vfio/pci.c | 24 +++-
> hw/vfio/pci.h | 1 +
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index d6ae9bd4ac..398d26669b 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -35,6 +35,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)
> @@ -2693,6 +2696,12 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice
> *vdev)
> vdev->req_enabled = false;
> }
>
> +static int has_standby_arg(void *opaque, const char *name,
> + const char *value, Error **errp)
> +{
> +return strcmp(name, "standby") == 0;
> +}
> +
> static void vfio_realize(PCIDevice *pdev, Error **errp)
> {
> VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> @@ -2706,6 +2715,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> int i, ret;
> bool is_mdev;
>
> +if (qemu_opt_foreach(pdev->qdev.opts, has_standby_arg,
> + (void *) pdev->qdev.opts, ) == 0) {
> +error_setg(>migration_blocker,
> +"VFIO device doesn't support migration");
> +ret = migrate_add_blocker(vdev->migration_blocker, );
> +if (err) {
> +error_propagate(errp, err);
> +error_free(vdev->migration_blocker);
> +}
> +} else {
> +pdev->qdev.allow_unplug_during_migration = true;
I think you add this only in the next patch?
> +}
> +
> if (!vdev->vbasedev.sysfsdev) {
> if (!(~vdev->host.domain || ~vdev->host.bus ||
>~vdev->host.slot || ~vdev->host.function)) {
> @@ -3148,7 +3170,7 @@ static Property vfio_pci_dev_properties[] = {
>
> static const VMStateDescription vfio_pci_vmstate = {
> .name = "vfio-pci",
> -.unmigratable = 1,
> +.unmigratable = 0,
> };
>
> static void vfio_pci_dev_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 27d58fc55b..0f6f8cb395 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -169,6 +169,7 @@ typedef struct VFIOPCIDevice {
> bool no_vfio_ioeventfd;
> bool enable_ramfb;
> VFIODisplay *dpy;
> +Error *migration_blocker;
> } VFIOPCIDevice;
>
> uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
This patch interacts with support for vfio migration (last posted in
<1562665760-26158-1-git-send-email-kwankh...@nvidia.com>, I've not seen
a later version yet.)
With that, we'd have three cases to consider:
1) device is a failover primary
2) device has a migration region
3) none of the above
Can 1) and 2) happen simultaneously? If yes, what should take
precedence?
