>-----Original Message-----
>From: Steven Sistare <steven.sist...@oracle.com>
>Subject: Re: [PATCH V5 35/38] vfio/iommufd: change process
>
>On 6/25/2025 7:40 AM, Duan, Zhenzhong wrote:
>>> -----Original Message-----
>>> From: Steve Sistare <steven.sist...@oracle.com>
>>> Subject: [PATCH V5 35/38] vfio/iommufd: change process
>>>
>>> Finish CPR by change the owning process of the iommufd device in
>>> post load.
>>>
>>> Signed-off-by: Steve Sistare <steven.sist...@oracle.com>
>>> ---
>>> hw/vfio/cpr-iommufd.c | 30 ++++++++++++++++++++++++++++++
>>> 1 file changed, 30 insertions(+)
>>>
>>> diff --git a/hw/vfio/cpr-iommufd.c b/hw/vfio/cpr-iommufd.c
>>> index 152a661..a9e3f68 100644
>>> --- a/hw/vfio/cpr-iommufd.c
>>> +++ b/hw/vfio/cpr-iommufd.c
>>> @@ -110,10 +110,40 @@ static bool
>vfio_cpr_supported(IOMMUFDBackend *be,
>>> Error **errp)
>>> return true;
>>> }
>>>
>>> +static int iommufd_cpr_pre_save(void *opaque)
>>> +{
>>> + IOMMUFDBackend *be = opaque;
>>> + Error *local_err = NULL;
>>> +
>>> + /*
>>> + * The process has not changed yet, but proactively call the ioctl,
>>> + * and it will fail if any DMA mappings are not supported.
>>> + */
>>> + if (!iommufd_change_process(be, &local_err)) {
>>
>> I'm confused when to call iommufd_change_process_capable and when to
>call iommufd_change_process, could you clarify?
>
>Strictly speaking, we do not need iommufd_change_process_capable,
>because we always
>try iommufd_change_process and recover on failure. But,
>iommufd_change_process_capable
>allows us to install a migration blocker, and fail with a blocker error, which
>is
>considered
>more user friendly for migration.
Though they are same effect, iommufd_change_process_capable still looks better
than iommufd_change_process in pre_save(), because we want to check instead of
really changing anything here. Other than that, this patch looks good to me.
Reviewed-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
Unrelated to this patch, after further thinking, I suspect that blocker is
never installed. Because vfio_iommufd_cpr_register_iommufd() is called for
first VFIO device and before memory listener is installed.
iommufd_change_process_capable() will always return true.
>
>>> + error_report_err(local_err);
>>> + return -1;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +static int iommufd_cpr_post_load(void *opaque, int version_id)
>>> +{
>>> + IOMMUFDBackend *be = opaque;
>>> + Error *local_err = NULL;
>>> +
>>> + if (!iommufd_change_process(be, &local_err)) {
>>> + error_report_err(local_err);
>>> + return -1;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> static const VMStateDescription iommufd_cpr_vmstate = {
>>> .name = "iommufd",
>>> .version_id = 0,
>>> .minimum_version_id = 0,
>>> + .pre_save = iommufd_cpr_pre_save,
>>> + .post_load = iommufd_cpr_post_load,
>>
>> Do we need LOW priority?
>
>No. iommufd_cpr_post_load only calls iommufd_change_process, which
>acts upon
>mappings that are already known to the kernel, independently of vmstate.
OK.
