Re: [PATCH V1 22/26] vfio/iommufd: invariant device name

2025-02-05 Thread Steven Sistare 

On 2/5/2025 12:42 PM, Cédric Le Goater wrote:

On 1/29/25 15:43, Steve Sistare wrote:

cpr-transfer will use the device name as a key to find the value
of the device descriptor in new QEMU.  However, if the descriptor
number is specified by a command-line fd parameter, then
vfio_device_get_name creates a name that includes the fd number.
This causes a chicken-and-egg problem: new QEMU must know the fd
number to construct a name to find the fd number.

To fix, create an invariant name based on the id command-line
parameter.  If id is not defined, add a CPR blocker.

Signed-off-by: Steve Sistare 
---
  hw/vfio/helpers.c | 18 +++---
  hw/vfio/iommufd.c |  2 ++
  include/hw/vfio/vfio-common.h |  1 +
  3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
index 913796f..bd94b86 100644
--- a/hw/vfio/helpers.c
+++ b/hw/vfio/helpers.c
@@ -25,6 +25,8 @@
  #include "hw/vfio/vfio-common.h"
  #include "hw/hw.h"
  #include "trace.h"
+#include "migration/blocker.h"
+#include "migration/cpr.h"
  #include "qapi/error.h"
  #include "qemu/error-report.h"
  #include "qemu/units.h"
@@ -636,6 +638,7 @@ bool vfio_device_get_name(VFIODevice *vbasedev, Error 
**errp)
  {
  ERRP_GUARD();
  struct stat st;
+    bool ret = true;
  if (vbasedev->fd < 0) {
  if (stat(vbasedev->sysfsdev, &st) < 0) {
@@ -653,15 +656,24 @@ bool vfio_device_get_name(VFIODevice *vbasedev, Error 
**errp)
  return false;
  }
  /*
- * Give a name with fd so any function printing out vbasedev->name
+ * Give a name so any function printing out vbasedev->name
   * will not break.
   */
  if (!vbasedev->name) {
-    vbasedev->name = g_strdup_printf("VFIO_FD%d", vbasedev->fd);
+    if (vbasedev->dev->id) {
+    vbasedev->name = g_strdup(vbasedev->dev->id);
+    } else {
+    vbasedev->name = g_strdup_printf("VFIO_FD%d", vbasedev->fd);
+    error_setg(&vbasedev->cpr_id_blocker,
+   "vfio device with fd=%d needs an id property",
+   vbasedev->fd);
+    ret = migrate_add_blocker_modes(&vbasedev->cpr_id_blocker, 
errp,
+    MIG_MODE_CPR_TRANSFER, -1) == 
0;


cpr helper please.


OK.


+    }
  }
  }
-    return true;
+    return ret;
  }
  void vfio_device_set_fd(VFIODevice *vbasedev, const char *str, Error **errp)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 2f888e5..8308715 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -24,6 +24,7 @@
  #include "system/reset.h"
  #include "qemu/cutils.h"
  #include "qemu/chardev_open.h"
+#include "migration/blocker.h"
  #include "pci.h"
  #include "exec/ram_addr.h"
@@ -657,6 +658,7 @@ static void iommufd_cdev_detach(VFIODevice *vbasedev)
  iommufd_cdev_container_destroy(container);
  vfio_put_address_space(space);
+    migrate_del_blocker(&vbasedev->cpr_id_blocker);
  iommufd_cdev_unbind_and_disconnect(vbasedev);
  close(vbasedev->fd);
  }
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index ca10abc..37e7c26 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -147,6 +147,7 @@ typedef struct VFIODevice {
  VFIOMigration *migration;
  Error *migration_blocker;
  Error *cpr_mdev_blocker;
+    Error *cpr_id_blocker;


a struct VFIODeviceCPR would be welcome.


OK.

- Steve


  OnOffAuto pre_copy_dirty_page_tracking;
  OnOffAuto device_dirty_page_tracking;
  bool dirty_pages_supported;

Re: [PATCH V1 22/26] vfio/iommufd: invariant device name

2025-02-05 Thread Cédric Le Goater 

On 1/29/25 15:43, Steve Sistare wrote:

cpr-transfer will use the device name as a key to find the value
of the device descriptor in new QEMU.  However, if the descriptor
number is specified by a command-line fd parameter, then
vfio_device_get_name creates a name that includes the fd number.
This causes a chicken-and-egg problem: new QEMU must know the fd
number to construct a name to find the fd number.

To fix, create an invariant name based on the id command-line
parameter.  If id is not defined, add a CPR blocker.

Signed-off-by: Steve Sistare 
---
  hw/vfio/helpers.c | 18 +++---
  hw/vfio/iommufd.c |  2 ++
  include/hw/vfio/vfio-common.h |  1 +
  3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
index 913796f..bd94b86 100644
--- a/hw/vfio/helpers.c
+++ b/hw/vfio/helpers.c
@@ -25,6 +25,8 @@
  #include "hw/vfio/vfio-common.h"
  #include "hw/hw.h"
  #include "trace.h"
+#include "migration/blocker.h"
+#include "migration/cpr.h"
  #include "qapi/error.h"
  #include "qemu/error-report.h"
  #include "qemu/units.h"
@@ -636,6 +638,7 @@ bool vfio_device_get_name(VFIODevice *vbasedev, Error 
**errp)
  {
  ERRP_GUARD();
  struct stat st;
+bool ret = true;
  
  if (vbasedev->fd < 0) {

  if (stat(vbasedev->sysfsdev, &st) < 0) {
@@ -653,15 +656,24 @@ bool vfio_device_get_name(VFIODevice *vbasedev, Error 
**errp)
  return false;
  }
  /*
- * Give a name with fd so any function printing out vbasedev->name
+ * Give a name so any function printing out vbasedev->name
   * will not break.
   */
  if (!vbasedev->name) {
-vbasedev->name = g_strdup_printf("VFIO_FD%d", vbasedev->fd);
+if (vbasedev->dev->id) {
+vbasedev->name = g_strdup(vbasedev->dev->id);
+} else {
+vbasedev->name = g_strdup_printf("VFIO_FD%d", vbasedev->fd);
+error_setg(&vbasedev->cpr_id_blocker,
+   "vfio device with fd=%d needs an id property",
+   vbasedev->fd);
+ret = migrate_add_blocker_modes(&vbasedev->cpr_id_blocker, 
errp,
+MIG_MODE_CPR_TRANSFER, -1) == 
0;


cpr helper please.




+}
  }
  }
  
-return true;

+return ret;
  }
  
  void vfio_device_set_fd(VFIODevice *vbasedev, const char *str, Error **errp)

diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 2f888e5..8308715 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -24,6 +24,7 @@
  #include "system/reset.h"
  #include "qemu/cutils.h"
  #include "qemu/chardev_open.h"
+#include "migration/blocker.h"
  #include "pci.h"
  #include "exec/ram_addr.h"
  
@@ -657,6 +658,7 @@ static void iommufd_cdev_detach(VFIODevice *vbasedev)

  iommufd_cdev_container_destroy(container);
  vfio_put_address_space(space);
  
+migrate_del_blocker(&vbasedev->cpr_id_blocker);

  iommufd_cdev_unbind_and_disconnect(vbasedev);
  close(vbasedev->fd);
  }
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index ca10abc..37e7c26 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -147,6 +147,7 @@ typedef struct VFIODevice {
  VFIOMigration *migration;
  Error *migration_blocker;
  Error *cpr_mdev_blocker;
+Error *cpr_id_blocker;


a struct VFIODeviceCPR would be welcome.


Thanks,

C.




  OnOffAuto pre_copy_dirty_page_tracking;
  OnOffAuto device_dirty_page_tracking;
  bool dirty_pages_supported;