>-----Original Message-----
>From: Steven Sistare <steven.sist...@oracle.com>
>Subject: Re: [PATCH V5 31/38] vfio/iommufd: cpr state
>
>On 6/23/2025 6:45 AM, Duan, Zhenzhong wrote:
>>> -----Original Message-----
>>> From: Steve Sistare <steven.sist...@oracle.com>
>>> Subject: [PATCH V5 31/38] vfio/iommufd: cpr state
>>>
>>> VFIO iommufd devices will need access to ioas_id, devid, and hwpt_id in
>>> new QEMU at realize time, so add them to CPR state. Define
>CprVFIODevice
>>> as the object which holds the state and is serialized to the vmstate file.
>>> Define accessors to copy state between VFIODevice and CprVFIODevice.
>>>
>>> Signed-off-by: Steve Sistare <steven.sist...@oracle.com>
>>> ---
>>> include/hw/vfio/vfio-cpr.h | 3 ++
>>> hw/vfio/cpr-iommufd.c | 96
>>> +++++++++++++++++++++++++++++++++++++++++++++-
>>> hw/vfio/iommufd.c | 2 +
>>> 3 files changed, 100 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h
>>> index 619af07..f88e4ba 100644
>>> --- a/include/hw/vfio/vfio-cpr.h
>>> +++ b/include/hw/vfio/vfio-cpr.h
>>> @@ -33,6 +33,8 @@ typedef struct VFIOContainerCPR {
>>> typedef struct VFIODeviceCPR {
>>> Error *mdev_blocker;
>>> Error *id_blocker;
>>> + uint32_t hwpt_id;
>>> + uint32_t ioas_id;
>>> } VFIODeviceCPR;
>>>
>>> bool vfio_legacy_cpr_register_container(struct VFIOContainer *container,
>>> @@ -54,6 +56,7 @@ bool vfio_iommufd_cpr_register_iommufd(struct
>>> IOMMUFDBackend *be, Error **errp);
>>> void vfio_iommufd_cpr_unregister_iommufd(struct IOMMUFDBackend
>*be);
>>> void vfio_iommufd_cpr_register_device(struct VFIODevice *vbasedev);
>>> void vfio_iommufd_cpr_unregister_device(struct VFIODevice *vbasedev);
>>> +void vfio_cpr_load_device(struct VFIODevice *vbasedev);
>>>
>>> int vfio_cpr_group_get_device_fd(int d, const char *name);
>>>
>>> diff --git a/hw/vfio/cpr-iommufd.c b/hw/vfio/cpr-iommufd.c
>>> index 3e78265..2eca8a6 100644
>>> --- a/hw/vfio/cpr-iommufd.c
>>> +++ b/hw/vfio/cpr-iommufd.c
>>> @@ -7,6 +7,7 @@
>>> #include "qemu/osdep.h"
>>> #include "qapi/error.h"
>>> #include "hw/vfio/vfio-cpr.h"
>>> +#include "hw/vfio/vfio-device.h"
>>> #include "migration/blocker.h"
>>> #include "migration/cpr.h"
>>> #include "migration/migration.h"
>>> @@ -14,7 +15,88 @@
>>> #include "system/iommufd.h"
>>> #include "vfio-iommufd.h"
>>>
>>> -const VMStateDescription vmstate_cpr_vfio_devices; /* TBD in a later
>patch */
>>> +typedef struct CprVFIODevice {
>>> + char *name;
>>> + unsigned int namelen;
>>> + uint32_t ioas_id;
>>> + int devid;
>>> + uint32_t hwpt_id;
>>> + QLIST_ENTRY(CprVFIODevice) next;
>>> +} CprVFIODevice;
>>> +
>>> +static const VMStateDescription vmstate_cpr_vfio_device = {
>>> + .name = "cpr vfio device",
>>> + .version_id = 1,
>>> + .minimum_version_id = 1,
>>> + .fields = (VMStateField[]) {
>>> + VMSTATE_UINT32(namelen, CprVFIODevice),
>>> + VMSTATE_VBUFFER_ALLOC_UINT32(name, CprVFIODevice, 0,
>NULL,
>>> namelen),
>>> + VMSTATE_INT32(devid, CprVFIODevice),
>>> + VMSTATE_UINT32(ioas_id, CprVFIODevice),
>>> + VMSTATE_UINT32(hwpt_id, CprVFIODevice),
>>> + VMSTATE_END_OF_LIST()
>>> + }
>>> +};
>>> +
>>> +const VMStateDescription vmstate_cpr_vfio_devices = {
>>> + .name = CPR_STATE "/vfio devices",
>>> + .version_id = 1,
>>> + .minimum_version_id = 1,
>>> + .fields = (const VMStateField[]){
>>> + VMSTATE_QLIST_V(vfio_devices, CprState, 1,
>vmstate_cpr_vfio_device,
>>> + CprVFIODevice, next),
>>> + VMSTATE_END_OF_LIST()
>>> + }
>>> +};
>>> +
>>> +static void vfio_cpr_save_device(VFIODevice *vbasedev)
>>> +{
>>> + CprVFIODevice *elem = g_new0(CprVFIODevice, 1);
>>> +
>>> + elem->name = g_strdup(vbasedev->name);
>>> + elem->namelen = strlen(vbasedev->name) + 1;
>>> + elem->ioas_id = vbasedev->cpr.ioas_id;
>>> + elem->devid = vbasedev->devid;
>>> + elem->hwpt_id = vbasedev->cpr.hwpt_id;
>>> + QLIST_INSERT_HEAD(&cpr_state.vfio_devices, elem, next);
>>> +}
>>> +
>>> +static CprVFIODevice *find_device(const char *name)
>>> +{
>>> + CprVFIODeviceList *head = &cpr_state.vfio_devices;
>>> + CprVFIODevice *elem;
>>> +
>>> + QLIST_FOREACH(elem, head, next) {
>>> + if (!strcmp(elem->name, name)) {
>>> + return elem;
>>> + }
>>> + }
>>> + return NULL;
>>> +}
>>> +
>>> +static void vfio_cpr_delete_device(const char *name)
>>> +{
>>> + CprVFIODevice *elem = find_device(name);
>>> +
>>> + if (elem) {
>>> + QLIST_REMOVE(elem, next);
>>> + g_free(elem->name);
>>> + g_free(elem);
>>> + }
>>> +}
>>> +
>>> +static bool vfio_cpr_find_device(VFIODevice *vbasedev)
>>
>> Better to rename as vfio_cpr_load_device
>
>This is already called by a function named vfio_cpr_load_device.
>The usage is the same as cpr_find_fd, so "find" is a consistent name.
I thought this is find and load. This is trival, I'm ok with current change.
>
>>> +{
>>> + CprVFIODevice *elem = find_device(vbasedev->name);
>>> +
>>> + if (elem) {
>>> + vbasedev->cpr.ioas_id = elem->ioas_id;
>>> + vbasedev->devid = elem->devid;
>>> + vbasedev->cpr.hwpt_id = elem->hwpt_id;
>>> + return true;
>>> + }
>>> + return false;
>>> +}
>>>
>>> static bool vfio_cpr_supported(IOMMUFDBackend *be, Error **errp)
>>> {
>>> @@ -79,8 +161,20 @@ void
>>> vfio_iommufd_cpr_unregister_container(VFIOIOMMUFDContainer
>*container)
>>>
>>> void vfio_iommufd_cpr_register_device(VFIODevice *vbasedev)
>>> {
>>> + if (!cpr_is_incoming()) {
>>> + vfio_cpr_save_device(vbasedev);
>>> + }
>>> }
>>>
>>> void vfio_iommufd_cpr_unregister_device(VFIODevice *vbasedev)
>>> {
>>> + vfio_cpr_delete_device(vbasedev->name);
>>> +}
>>> +
>>> +void vfio_cpr_load_device(VFIODevice *vbasedev)
>>> +{
>>> + if (cpr_is_incoming()) {
>>> + bool ret = vfio_cpr_find_device(vbasedev);
>>> + g_assert(ret);
>>> + }
>>> }
>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>>> index ff291be..f0d57ea 100644
>>> --- a/hw/vfio/iommufd.c
>>> +++ b/hw/vfio/iommufd.c
>>> @@ -515,6 +515,8 @@ static bool iommufd_cdev_attach(const char
>*name,
>>> VFIODevice *vbasedev,
>>> const VFIOIOMMUClass *iommufd_vioc =
>>>
>>>
>VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD
>));
>>>
>>> + vfio_cpr_load_device(vbasedev);
>>
>> This can be open coded.
>
>vfio_cpr_load_device grows in patch "preserve descriptors", so I would
>rather keep it closed.
>
>- Steve
>
>>> +
>>> if (vbasedev->fd < 0) {
>>> devfd = iommufd_cdev_getfd(vbasedev->sysfsdev, errp);
>>> if (devfd < 0) {
>>> --
>>> 1.8.3.1
>>
OK.
Reviewed-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
Thanks
Zhenzhong
