>-----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.
I'm not sure if that's a case we need to support. If not, feel free to add my
RB.
>>
>
>If my explanation makes sense, any chance of getting an RB for this and the
>related patch?
> backends/iommufd: change process ioctl
> vfio/iommufd: change process
>
>They are not affected by the other changes we have discussed.
>
>- Steve
>
>>>>>> How about I just add a comment:
>>>>>>
>>>>>> bool iommufd_change_process_capable(IOMMUFDBackend *be)
>>>>>> {
>>>>>> /*
>>>>>> * Call IOMMU_IOAS_CHANGE_PROCESS to verify it is a recognized
>ioctl.
>>>>>> * This is a no-op if the process has not changed since DMA was
>mapped.
>>>>>> */
>>>>>>
>>>>>> - Steve
>>>>>
>>>
>>
