Re: [PATCH v26 04/17] vfio: Add migration region initialization and finalize function
On 9/26/2020 1:50 AM, Alex Williamson wrote:
On Wed, 23 Sep 2020 04:54:06 +0530
Kirti Wankhede wrote:
Whether the VFIO device supports migration or not is decided based of
migration region query. If migration region query is successful and migration
region initialization is successful then migration is supported else
migration is blocked.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
Acked-by: Dr. David Alan Gilbert
---
hw/vfio/meson.build | 1 +
hw/vfio/migration.c | 142 ++
hw/vfio/trace-events | 5 ++
include/hw/vfio/vfio-common.h | 9 +++
4 files changed, 157 insertions(+)
create mode 100644 hw/vfio/migration.c
diff --git a/hw/vfio/meson.build b/hw/vfio/meson.build
index 37efa74018bc..da9af297a0c5 100644
--- a/hw/vfio/meson.build
+++ b/hw/vfio/meson.build
@@ -2,6 +2,7 @@ vfio_ss = ss.source_set()
vfio_ss.add(files(
'common.c',
'spapr.c',
+ 'migration.c',
))
vfio_ss.add(when: 'CONFIG_VFIO_PCI', if_true: files(
'display.c',
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
new file mode 100644
index ..2f760f1f9c47
--- /dev/null
+++ b/hw/vfio/migration.c
@@ -0,0 +1,142 @@
+/*
+ * Migration support for VFIO devices
+ *
+ * Copyright NVIDIA, Inc. 2020
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include
+
+#include "hw/vfio/vfio-common.h"
+#include "cpu.h"
+#include "migration/migration.h"
+#include "migration/qemu-file.h"
+#include "migration/register.h"
+#include "migration/blocker.h"
+#include "migration/misc.h"
+#include "qapi/error.h"
+#include "exec/ramlist.h"
+#include "exec/ram_addr.h"
+#include "pci.h"
+#include "trace.h"
+
+static void vfio_migration_region_exit(VFIODevice *vbasedev)
+{
+VFIOMigration *migration = vbasedev->migration;
+
+if (!migration) {
+return;
+}
+
+if (migration->region.size) {
+vfio_region_exit(&migration->region);
+vfio_region_finalize(&migration->region);
+}
+}
+
+static int vfio_migration_region_init(VFIODevice *vbasedev, int index)
+{
+VFIOMigration *migration = vbasedev->migration;
+Object *obj = NULL;
Unnecessary initialization.
+int ret = -EINVAL;
return -EINVAL below, this doesn't need to be initialized, use it for
storing actual return values.
+
+obj = vbasedev->ops->vfio_get_object(vbasedev);
+if (!obj) {
+return ret;
+}
vfio_migration_init() tests whether the vbasedev->ops supports
vfio_get_object, then calls this, then calls vfio_get_object itself
(added in a later patch, with a strange inconsistency in failure modes).
Wouldn't it make more sense for vfio_migration_init() to pass the
Object since that function also needs it (eventually) and actually does
the existence test?
+
+ret = vfio_region_setup(obj, vbasedev, &migration->region, index,
+"migration");
+if (ret) {
+error_report("%s: Failed to setup VFIO migration region %d: %s",
+ vbasedev->name, index, strerror(-ret));
+goto err;
+}
+
+if (!migration->region.size) {
+ret = -EINVAL;
+error_report("%s: Invalid region size of VFIO migration region %d: %s",
+ vbasedev->name, index, strerror(-ret));
+goto err;
+}
If the caller were to pass obj, this is nothing more than a wrapper for
calling vfio_region_setup(), which suggests to me we might not even
need this as a separate function outside of vfio_migration_init().
Removed vfio_migration_region_init(), moving vfio_region_setup() to
vfio_migration_init()
Thanks,
Kirti
+
+return 0;
+
+err:
+vfio_migration_region_exit(vbasedev);
+return ret;
+}
+
+static int vfio_migration_init(VFIODevice *vbasedev,
+ struct vfio_region_info *info)
+{
+int ret = -EINVAL;
+
+if (!vbasedev->ops->vfio_get_object) {
+return ret;
+}
+
+vbasedev->migration = g_new0(VFIOMigration, 1);
+
+ret = vfio_migration_region_init(vbasedev, info->index);
+if (ret) {
+error_report("%s: Failed to initialise migration region",
+ vbasedev->name);
+g_free(vbasedev->migration);
+vbasedev->migration = NULL;
+}
+
+return ret;
+}
+
+/* -- */
+
+int vfio_migration_probe(VFIODevice *vbasedev, Error **errp)
+{
+struct vfio_region_info *info = NULL;
Not sure this initialization is strictly necessary either, but it also
seems to be a common convention for this function, so either way.
Connie, does vfio_ccw_get_region() leak this? It appears to call
vfio_get_dev_region_info() and vfio_get_region_info() several times with
the same pointer without freeing it between uses.
Thanks,
Alex
+Error *local_err = NULL;
+int ret;
+
+ret = vfio_g
Re: [PATCH v26 04/17] vfio: Add migration region initialization and finalize function
On 9/24/2020 7:38 PM, Cornelia Huck wrote:
On Wed, 23 Sep 2020 04:54:06 +0530
Kirti Wankhede wrote:
Whether the VFIO device supports migration or not is decided based of
migration region query. If migration region query is successful and migration
region initialization is successful then migration is supported else
migration is blocked.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
Acked-by: Dr. David Alan Gilbert
---
hw/vfio/meson.build | 1 +
hw/vfio/migration.c | 142 ++
hw/vfio/trace-events | 5 ++
include/hw/vfio/vfio-common.h | 9 +++
4 files changed, 157 insertions(+)
create mode 100644 hw/vfio/migration.c
(...)
+static int vfio_migration_region_init(VFIODevice *vbasedev, int index)
+{
+VFIOMigration *migration = vbasedev->migration;
+Object *obj = NULL;
+int ret = -EINVAL;
+
+obj = vbasedev->ops->vfio_get_object(vbasedev);
+if (!obj) {
+return ret;
+}
+
+ret = vfio_region_setup(obj, vbasedev, &migration->region, index,
+"migration");
+if (ret) {
+error_report("%s: Failed to setup VFIO migration region %d: %s",
+ vbasedev->name, index, strerror(-ret));
+goto err;
+}
+
+if (!migration->region.size) {
+ret = -EINVAL;
+error_report("%s: Invalid region size of VFIO migration region %d: %s",
+ vbasedev->name, index, strerror(-ret));
Using strerror on a hardcoded error value is probably not terribly
helpful. I think printing either region.size (if you plan to extend
this check later) or something like "Invalid zero-sized VFIO migration
region" would make more sense.
Updating the error string as you suggested.
+goto err;
+}
+
+return 0;
+
+err:
+vfio_migration_region_exit(vbasedev);
+return ret;
+}
(...)
Apart from that, looks good to me.
Thanks.
Kirti
Re: [PATCH v26 04/17] vfio: Add migration region initialization and finalize function
On Fri, 25 Sep 2020 14:20:06 -0600
Alex Williamson wrote:
> On Wed, 23 Sep 2020 04:54:06 +0530
> Kirti Wankhede wrote:
> > +int vfio_migration_probe(VFIODevice *vbasedev, Error **errp)
> > +{
> > +struct vfio_region_info *info = NULL;
>
> Not sure this initialization is strictly necessary either, but it also
> seems to be a common convention for this function, so either way.
>
> Connie, does vfio_ccw_get_region() leak this? It appears to call
> vfio_get_dev_region_info() and vfio_get_region_info() several times with
> the same pointer without freeing it between uses.
Ugh, indeed, info is reallocated without freeing it inbetween. I'll
cook up a patch, thanks for spotting.
Re: [PATCH v26 04/17] vfio: Add migration region initialization and finalize function
On Wed, 23 Sep 2020 04:54:06 +0530
Kirti Wankhede wrote:
> Whether the VFIO device supports migration or not is decided based of
> migration region query. If migration region query is successful and migration
> region initialization is successful then migration is supported else
> migration is blocked.
>
> Signed-off-by: Kirti Wankhede
> Reviewed-by: Neo Jia
> Acked-by: Dr. David Alan Gilbert
> ---
> hw/vfio/meson.build | 1 +
> hw/vfio/migration.c | 142
> ++
> hw/vfio/trace-events | 5 ++
> include/hw/vfio/vfio-common.h | 9 +++
> 4 files changed, 157 insertions(+)
> create mode 100644 hw/vfio/migration.c
>
> diff --git a/hw/vfio/meson.build b/hw/vfio/meson.build
> index 37efa74018bc..da9af297a0c5 100644
> --- a/hw/vfio/meson.build
> +++ b/hw/vfio/meson.build
> @@ -2,6 +2,7 @@ vfio_ss = ss.source_set()
> vfio_ss.add(files(
>'common.c',
>'spapr.c',
> + 'migration.c',
> ))
> vfio_ss.add(when: 'CONFIG_VFIO_PCI', if_true: files(
>'display.c',
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> new file mode 100644
> index ..2f760f1f9c47
> --- /dev/null
> +++ b/hw/vfio/migration.c
> @@ -0,0 +1,142 @@
> +/*
> + * Migration support for VFIO devices
> + *
> + * Copyright NVIDIA, Inc. 2020
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include
> +
> +#include "hw/vfio/vfio-common.h"
> +#include "cpu.h"
> +#include "migration/migration.h"
> +#include "migration/qemu-file.h"
> +#include "migration/register.h"
> +#include "migration/blocker.h"
> +#include "migration/misc.h"
> +#include "qapi/error.h"
> +#include "exec/ramlist.h"
> +#include "exec/ram_addr.h"
> +#include "pci.h"
> +#include "trace.h"
> +
> +static void vfio_migration_region_exit(VFIODevice *vbasedev)
> +{
> +VFIOMigration *migration = vbasedev->migration;
> +
> +if (!migration) {
> +return;
> +}
> +
> +if (migration->region.size) {
> +vfio_region_exit(&migration->region);
> +vfio_region_finalize(&migration->region);
> +}
> +}
> +
> +static int vfio_migration_region_init(VFIODevice *vbasedev, int index)
> +{
> +VFIOMigration *migration = vbasedev->migration;
> +Object *obj = NULL;
Unnecessary initialization.
> +int ret = -EINVAL;
return -EINVAL below, this doesn't need to be initialized, use it for
storing actual return values.
> +
> +obj = vbasedev->ops->vfio_get_object(vbasedev);
> +if (!obj) {
> +return ret;
> +}
vfio_migration_init() tests whether the vbasedev->ops supports
vfio_get_object, then calls this, then calls vfio_get_object itself
(added in a later patch, with a strange inconsistency in failure modes).
Wouldn't it make more sense for vfio_migration_init() to pass the
Object since that function also needs it (eventually) and actually does
the existence test?
> +
> +ret = vfio_region_setup(obj, vbasedev, &migration->region, index,
> +"migration");
> +if (ret) {
> +error_report("%s: Failed to setup VFIO migration region %d: %s",
> + vbasedev->name, index, strerror(-ret));
> +goto err;
> +}
> +
> +if (!migration->region.size) {
> +ret = -EINVAL;
> +error_report("%s: Invalid region size of VFIO migration region %d:
> %s",
> + vbasedev->name, index, strerror(-ret));
> +goto err;
> +}
If the caller were to pass obj, this is nothing more than a wrapper for
calling vfio_region_setup(), which suggests to me we might not even
need this as a separate function outside of vfio_migration_init().
> +
> +return 0;
> +
> +err:
> +vfio_migration_region_exit(vbasedev);
> +return ret;
> +}
> +
> +static int vfio_migration_init(VFIODevice *vbasedev,
> + struct vfio_region_info *info)
> +{
> +int ret = -EINVAL;
> +
> +if (!vbasedev->ops->vfio_get_object) {
> +return ret;
> +}
> +
> +vbasedev->migration = g_new0(VFIOMigration, 1);
> +
> +ret = vfio_migration_region_init(vbasedev, info->index);
> +if (ret) {
> +error_report("%s: Failed to initialise migration region",
> + vbasedev->name);
> +g_free(vbasedev->migration);
> +vbasedev->migration = NULL;
> +}
> +
> +return ret;
> +}
> +
> +/* -- */
> +
> +int vfio_migration_probe(VFIODevice *vbasedev, Error **errp)
> +{
> +struct vfio_region_info *info = NULL;
Not sure this initialization is strictly necessary either, but it also
seems to be a common convention for this function, so either way.
Connie, does vfio_ccw_get_region() leak this? It appears to call
vfio_get_dev_region_info() and vfio_get_region_info() several times with
the same pointer without freeing it b
Re: [PATCH v26 04/17] vfio: Add migration region initialization and finalize function
On Wed, 23 Sep 2020 04:54:06 +0530
Kirti Wankhede wrote:
> Whether the VFIO device supports migration or not is decided based of
> migration region query. If migration region query is successful and migration
> region initialization is successful then migration is supported else
> migration is blocked.
>
> Signed-off-by: Kirti Wankhede
> Reviewed-by: Neo Jia
> Acked-by: Dr. David Alan Gilbert
> ---
> hw/vfio/meson.build | 1 +
> hw/vfio/migration.c | 142
> ++
> hw/vfio/trace-events | 5 ++
> include/hw/vfio/vfio-common.h | 9 +++
> 4 files changed, 157 insertions(+)
> create mode 100644 hw/vfio/migration.c
(...)
> +static int vfio_migration_region_init(VFIODevice *vbasedev, int index)
> +{
> +VFIOMigration *migration = vbasedev->migration;
> +Object *obj = NULL;
> +int ret = -EINVAL;
> +
> +obj = vbasedev->ops->vfio_get_object(vbasedev);
> +if (!obj) {
> +return ret;
> +}
> +
> +ret = vfio_region_setup(obj, vbasedev, &migration->region, index,
> +"migration");
> +if (ret) {
> +error_report("%s: Failed to setup VFIO migration region %d: %s",
> + vbasedev->name, index, strerror(-ret));
> +goto err;
> +}
> +
> +if (!migration->region.size) {
> +ret = -EINVAL;
> +error_report("%s: Invalid region size of VFIO migration region %d:
> %s",
> + vbasedev->name, index, strerror(-ret));
Using strerror on a hardcoded error value is probably not terribly
helpful. I think printing either region.size (if you plan to extend
this check later) or something like "Invalid zero-sized VFIO migration
region" would make more sense.
> +goto err;
> +}
> +
> +return 0;
> +
> +err:
> +vfio_migration_region_exit(vbasedev);
> +return ret;
> +}
(...)
Apart from that, looks good to me.
