Re: [PATCH V3 34/42] vfio/iommufd: invariant device name

["Cédric Le Goater" <clg@redhat.com>]

On 5/20/25 23:00, Steven Sistare wrote:
> On 5/20/2025 9:55 AM, Cédric Le Goater wrote:
>> On 5/12/25 17:32, 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 <steven.sistare@oracle.com>
>>> ---
>>>   hw/vfio/cpr.c              | 21 +++++++++++++++++++++
>>>   hw/vfio/device.c           | 10 ++++------
>>>   hw/vfio/iommufd.c          |  2 ++
>>>   include/hw/vfio/vfio-cpr.h |  4 ++++
>>>   4 files changed, 31 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/vfio/cpr.c b/hw/vfio/cpr.c
>>> index 6081a89..7609c62 100644
>>> --- a/hw/vfio/cpr.c
>>> +++ b/hw/vfio/cpr.c
>>> @@ -11,6 +11,7 @@
>>>   #include "hw/vfio/pci.h"
>>>   #include "hw/pci/msix.h"
>>>   #include "hw/pci/msi.h"
>>> +#include "migration/blocker.h"
>>>   #include "migration/cpr.h"
>>>   #include "qapi/error.h"
>>>   #include "system/runstate.h"
>>> @@ -184,3 +185,23 @@ const VMStateDescription vfio_cpr_pci_vmstate = {
>>>           VMSTATE_END_OF_LIST()
>>>       }
>>>   };
>>> +
>>> +bool vfio_cpr_set_device_name(VFIODevice *vbasedev, Error **errp)
>>> +{
>>> +    if (vbasedev->dev->id) {
>>> +        vbasedev->name = g_strdup(vbasedev->dev->id);
>>> +        return true;
>>> +    } else {
>>> +        /*
>>> +         * Assign a name so any function printing it will not break, but the
>>> +         * fd number changes across processes, so this cannot be used as an
>>> +         * invariant name for CPR.
>>> +         */
>>> +        vbasedev->name = g_strdup_printf("VFIO_FD%d", vbasedev->fd);
>>
>> The code above should be in vfio_device_get_name() proposed in its own path.
> 
> I understand, "in its own patch".  Will do.

yes. This typo could clearly be misunderstood :/ Sorry for the noise.


> 
>>> +        error_setg(&vbasedev->cpr.id_blocker,
>>> +                   "vfio device with fd=%d needs an id property",
>>> +                   vbasedev->fd);
>>> +        return migrate_add_blocker_modes(&vbasedev->cpr.id_blocker, errp,
>>> +                                         MIG_MODE_CPR_TRANSFER, -1) == 0;
>>
>> The cpr blocker should proposed in a second patch, maybe with a small
>> wrapper to set the 'Error *'.
> 
> will do.


Thanks,

C.