>-----Original Message-----
>From: Steven Sistare <steven.sist...@oracle.com>
>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 <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.
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.
>
>>> }
>>> 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);
> }
> }
>}
Looks good, with above changes,
Reviewed-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
Thanks
Zhenzhong
