>-----Original Message-----
>From: Steven Sistare <steven.sist...@oracle.com>
>Subject: Re: [PATCH V3 29/42] backends/iommufd: change process ioctl
>
>On 5/25/2025 10:31 PM, Duan, Zhenzhong wrote:
>>> -----Original Message-----
>>> From: Steven Sistare <steven.sist...@oracle.com>
>>> Subject: Re: [PATCH V3 29/42] backends/iommufd: change process ioctl
>>>
>>> On 5/23/2025 10:56 AM, Steven Sistare wrote:
>>>> On 5/23/2025 4:56 AM, Duan, Zhenzhong wrote:
>>>>>> -----Original Message-----
>>>>>> From: Steven Sistare <steven.sist...@oracle.com>
>>>>>> Subject: Re: [PATCH V3 29/42] backends/iommufd: change process ioctl
>>>>>>
>>>>>> On 5/21/2025 11:19 PM, Duan, Zhenzhong wrote:
>>>>>>>> -----Original Message-----
>>>>>>>> From: Steven Sistare <steven.sist...@oracle.com>
>>>>>>>> Subject: Re: [PATCH V3 29/42] backends/iommufd: change process ioctl
>>>>>>>>
>>>>>>>> On 5/20/2025 11:11 PM, Duan, Zhenzhong wrote:
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Steven Sistare <steven.sist...@oracle.com>
>>>>>>>>>> Subject: Re: [PATCH V3 29/42] backends/iommufd: change process
>ioctl
>>>>>>>>>>
>>>>>>>>>> On 5/19/2025 11:51 AM, Steven Sistare wrote:
>>>>>>>>>>> On 5/16/2025 4:42 AM, Duan, Zhenzhong wrote:
>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>> From: Steve Sistare <steven.sist...@oracle.com>
>>>>>>>>>>>>> Subject: [PATCH V3 29/42] backends/iommufd: change process
>ioctl
>>>>>>>>>>>>>
>>>>>>>>>>>>> Define the change process ioctl
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Steve Sistare <steven.sist...@oracle.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> backends/iommufd.c | 20 ++++++++++++++++++++
>>>>>>>>>>>>> backends/trace-events | 1 +
>>>>>>>>>>>>> include/system/iommufd.h | 2 ++
>>>>>>>>>>>>> 3 files changed, 23 insertions(+)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>>>>>>>>>>>>> index 5c1958f..6fed1c1 100644
>>>>>>>>>>>>> --- a/backends/iommufd.c
>>>>>>>>>>>>> +++ b/backends/iommufd.c
>>>>>>>>>>>>> @@ -73,6 +73,26 @@ static void
>>>>>>>> iommufd_backend_class_init(ObjectClass
>>>>>>>>>> *oc,
>>>>>>>>>>>>> const void *data)
>>>>>>>>>>>>> object_class_property_add_str(oc, "fd", NULL,
>>>>>>>> iommufd_backend_set_fd);
>>>>>>>>>>>>> }
>>>>>>>>>>>>>
>>>>>>>>>>>>> +bool iommufd_change_process_capable(IOMMUFDBackend *be)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + struct iommu_ioas_change_process args = {.size =
>sizeof(args)};
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + return !ioctl(be->fd, IOMMU_IOAS_CHANGE_PROCESS, &args);
>>>>>>>>>>>>> +}
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +bool iommufd_change_process(IOMMUFDBackend *be, Error
>>> **errp)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + struct iommu_ioas_change_process args = {.size =
>sizeof(args)};
>>>>>>>>>>>>> + bool ret = !ioctl(be->fd, IOMMU_IOAS_CHANGE_PROCESS,
>>> &args);
>>>>>>>>>>>>
>>>>>>>>>>>> This is same ioctl as above check, could it be called more than
>>>>>>>>>>>> once
>>> for
>>>>>>>> same
>>>>>>>>>> process?
>>>>>>>>>>>
>>>>>>>>>>> Yes, and it is a no-op if the process has not changed since the last
>time
>>>>>> DMA
>>>>>>>>>>> was mapped.
>>>>>>>>>>
>>>>>>>>>> More questions?
>>>>>>>>>
>>>>>>>>> Looks a bit redundant for me, meanwhile if
>>>>>> iommufd_change_process_capable()
>>>>>>>> is called on target qemu, may it do both checking and change?
>>>>>>>>>
>>>>>>>>> I would suggest to define only iommufd_change_process() and
>comment
>>> that
>>>>>>>> it's no-op if process not changed...
>>>>>>>>
>>>>>>>> We need to check if IOMMU_IOAS_CHANGE_PROCESS is allowed before
>>>>>>>> performing
>>>>>>>> live update so we can add a blocker and prevent live update cleanly:
>>>>>>>>
>>>>>>>> vfio_iommufd_cpr_register_container
>>>>>>>> if !vfio_cpr_supported() // calls
>iommufd_change_process_capable
>>>>>>>> migrate_add_blocker_modes()
>>>>>>>
>>>>>>> This reminds me of other questions, is this ioctl() suitable for
>>>>>>> checking if
>cpr-
>>>>>> transfer supported?
>>>>>>> If there is vIOMMU, there can be no mapping and process_capable()
>check
>>> will
>>>>>> pass,
>>>>>>> but if memory is not file backed...
>>>>>>> Does cpr-transfer support vIOMMU or not?
>>>>>>
>>>>>> I don't know, I have not tried your sample args yet, but I will.
>>>>>> With vIOMMU, what entity/interface pins memory for the vfio device?
>>>>>
>>>>> Oh, I don't mean virtio-iommu, it can be intel-iommu or virtio-iommu for
>this
>>> issue.
>>>>> I mean when guest attach device to a DMA domain, there can be no
>mapping
>>> in that domain initially.
>>>>>
>>>>>>
>>>>>>> QEMU knows details of all memory backends, why not checking memory
>>>>>> backends directly instead of a system call?
>>>>>>
>>>>>> IOMMU_IOAS_CHANGE_PROCESS is relatively new. The ioctl verifies that
>the
>>>>>> kernel
>>>>>> supports it. And if supported, it also verifies that all dma mappings
>>>>>> are
>>>>>> of the file type.
>>>>>
>>>>> But the dma mappings are dynamic if there is vIOMMU, so checking dma
>>> mappings are checking nothing if there is no mapping in the DMA domain.
>>>>
>>>> Yes, so there are 2 checks:
>>>> * at realize -> cpr register time. if cpr can never work because
>>>> IOMMU_IOAS_CHANGE_PROCESS is not supported, then adds a blocker.
>>>>
>>>> * at cpr time, in vfio_container_pre_save. refuses to proceed if
>>>> iommufd_change_process() fails because non-file mappings are present.
>>>> Allows cpr if there are no mappings present.
>>
>> Let me explain further.
>>
>> There is a corner case that could bypass above checks. Source qemu starts
>> with
>> vIOMMU and non-file memory backend, then hotplug VFIO device, if guest
>> driver doesn't setup any mapping or no guest driver attached, the mapping on
>> host side can be empty, then above checks will both pass.
>
>That is OK. CPR is allowed in that case and succeeds because
>iommufd_change_process
>has nothing to do.
>
>However, after CPR, if non-file mappings are added, then the next CPR operation
>would be blocked.
Clear, no problem. Thanks for your explanation.
Zhenzhong
