RE: [PATCH V5 32/38] vfio/iommufd: preserve descriptors
>-Original Message-
>From: Steven Sistare
>Subject: Re: [PATCH V5 32/38] vfio/iommufd: preserve descriptors
>
>On 6/25/2025 7:40 AM, Duan, Zhenzhong wrote:
>>> -Original Message-
>>> From: Steve Sistare
>>> 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
>>> ---
>>> 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
>>> #include
>>>
>>> +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.
OK
>
>>> +}
>>> +}
>>> +
>>> 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);
>}
Looks good.
Re: [PATCH V5 32/38] vfio/iommufd: preserve descriptors
On 6/25/2025 7:40 AM, Duan, Zhenzhong wrote:
-Original Message-
From: Steve Sistare
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
---
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
#include
+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->
RE: [PATCH V5 32/38] vfio/iommufd: preserve descriptors
>-Original Message-
>From: Steve Sistare
>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
>---
> 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
> #include
>
>+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? Can we call cpr_get_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.
> }
> 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()?
>+}
>+
> 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
[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
---
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
#include
+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);
+}
+}
+}
+
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);
}
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);
+}
+
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
