On 6/25/2025 7:40 AM, Duan, Zhenzhong wrote:
-----Original Message-----
From: Steve Sistare <steven.sist...@oracle.com>
Subject: [PATCH V5 32/38] vfio/iommufd: preserve descriptors
Save the iommu and vfio device fd in CPR state when it is created.
After CPR, the fd number is found in CPR state and reused.
Signed-off-by: Steve Sistare <steven.sist...@oracle.com>
---
backends/iommufd.c | 25 ++++++++++++++++++++++++-
hw/vfio/cpr-iommufd.c | 10 ++++++++++
hw/vfio/device.c | 9 +--------
3 files changed, 35 insertions(+), 9 deletions(-)
diff --git a/backends/iommufd.c b/backends/iommufd.c
index c554ce5..e02f06e 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -16,12 +16,18 @@
#include "qemu/module.h"
#include "qom/object_interfaces.h"
#include "qemu/error-report.h"
+#include "migration/cpr.h"
#include "monitor/monitor.h"
#include "trace.h"
#include "hw/vfio/vfio-device.h"
#include <sys/ioctl.h>
#include <linux/iommufd.h>
+static const char *iommufd_fd_name(IOMMUFDBackend *be)
+{
+ return object_get_canonical_path_component(OBJECT(be));
+}
+
static void iommufd_backend_init(Object *obj)
{
IOMMUFDBackend *be = IOMMUFD_BACKEND(obj);
@@ -64,11 +70,27 @@ static bool
iommufd_backend_can_be_deleted(UserCreatable *uc)
return !be->users;
}
+static void iommufd_backend_complete(UserCreatable *uc, Error **errp)
+{
+ IOMMUFDBackend *be = IOMMUFD_BACKEND(uc);
+ const char *name = iommufd_fd_name(be);
+
+ if (!be->owned) {
+ /* fd came from the command line. Fetch updated value from cpr state.
*/
+ if (cpr_is_incoming()) {
+ be->fd = cpr_find_fd(name, 0);
+ } else {
+ cpr_save_fd(name, 0, be->fd);
+ }
Maybe this can be handled in iommufd_backend_set_fd() instead of introducing
complete callback?
Afraid not. iommufd_fd_name -> object_get_canonical_path_component needs the
parent, which is not set yet in iommufd_backend_set_fd. The complete callback
solved that problem nicely.
Can we call cpr_get_fd_param()?
No. That one expects to get the name from monitor_fd_param.
+ }
+}
+
static void iommufd_backend_class_init(ObjectClass *oc, const void *data)
{
UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
ucc->can_be_deleted = iommufd_backend_can_be_deleted;
+ ucc->complete = iommufd_backend_complete;
object_class_property_add_str(oc, "fd", NULL, iommufd_backend_set_fd);
}
@@ -102,7 +124,7 @@ bool iommufd_backend_connect(IOMMUFDBackend *be,
Error **errp)
int fd;
if (be->owned && !be->users) {
- fd = qemu_open("/dev/iommu", O_RDWR, errp);
+ fd = cpr_open_fd("/dev/iommu", O_RDWR, iommufd_fd_name(be), 0, errp);
if (fd < 0) {
return false;
}
@@ -134,6 +156,7 @@ void iommufd_backend_disconnect(IOMMUFDBackend
*be)
out:
if (!be->users) {
vfio_iommufd_cpr_unregister_iommufd(be);
+ cpr_delete_fd(iommufd_fd_name(be), 0);
I think we shouldn't call this if not owned.
I agree, thanks, and a mismerge during rebase put the out label in the wrong
place.
It should be:
void iommufd_backend_disconnect(IOMMUFDBackend *be)
{
if (!be->users) {
goto out;
}
be->users--;
if (!be->users) {
vfio_iommufd_cpr_unregister_iommufd(be);
if (be->owned) {
cpr_delete_fd(iommufd_fd_name(be), 0);
close(be->fd);
be->fd = -1;
}
}
out:
trace_iommufd_backend_disconnect(be->fd, be->users);
}
}
trace_iommufd_backend_disconnect(be->fd, be->users);
}
diff --git a/hw/vfio/cpr-iommufd.c b/hw/vfio/cpr-iommufd.c
index 2eca8a6..152a661 100644
--- a/hw/vfio/cpr-iommufd.c
+++ b/hw/vfio/cpr-iommufd.c
@@ -162,17 +162,27 @@ void
vfio_iommufd_cpr_unregister_container(VFIOIOMMUFDContainer *container)
void vfio_iommufd_cpr_register_device(VFIODevice *vbasedev)
{
if (!cpr_is_incoming()) {
+ /*
+ * Beware fd may have already been saved by vfio_device_set_fd,
+ * so call resave to avoid a duplicate entry.
+ */
+ cpr_resave_fd(vbasedev->name, 0, vbasedev->fd);
vfio_cpr_save_device(vbasedev);
}
}
void vfio_iommufd_cpr_unregister_device(VFIODevice *vbasedev)
{
+ cpr_delete_fd(vbasedev->name, 0);
vfio_cpr_delete_device(vbasedev->name);
}
void vfio_cpr_load_device(VFIODevice *vbasedev)
{
+ if (vbasedev->fd < 0) {
+ vbasedev->fd = cpr_find_fd(vbasedev->name, 0);
Maybe call this after checking cpr_is_incoming()?
That is not necessary, because cpr_find_fd returns -1 if !cpr_is_incoming(),
but I'll change it so the intent becomes clearer:
void vfio_cpr_load_device(VFIODevice *vbasedev)
{
if (cpr_is_incoming()) {
bool ret = vfio_cpr_find_device(vbasedev);
g_assert(ret);
if (vbasedev->fd < 0) {
vbasedev->fd = cpr_find_fd(vbasedev->name, 0);
}
}
}
- Steve
+ }
+
if (cpr_is_incoming()) {
bool ret = vfio_cpr_find_device(vbasedev);
g_assert(ret);
diff --git a/hw/vfio/device.c b/hw/vfio/device.c
index 8c3835b..6bcc65c 100644
--- a/hw/vfio/device.c
+++ b/hw/vfio/device.c
@@ -335,14 +335,7 @@ void vfio_device_free_name(VFIODevice *vbasedev)
void vfio_device_set_fd(VFIODevice *vbasedev, const char *str, Error **errp)
{
- ERRP_GUARD();
- int fd = monitor_fd_param(monitor_cur(), str, errp);
-
- if (fd < 0) {
- error_prepend(errp, "Could not parse remote object fd %s:", str);
- return;
- }
- vbasedev->fd = fd;
+ vbasedev->fd = cpr_get_fd_param(vbasedev->dev->id, str, 0, errp);
}
static VFIODeviceIOOps vfio_device_io_ops_ioctl;
--
1.8.3.1
