Re: [PATCH V3 29/42] backends/iommufd: change process ioctl

[Steven Sistare <steven.sistare@oracle.com>]

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.sistare@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.sistare@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.sistare@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.sistare@oracle.com>
>>>>>>>>>> Subject: [PATCH V3 29/42] backends/iommufd: change process ioctl
>>>>>>>>>>
>>>>>>>>>> Define the change process ioctl
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Steve Sistare <steven.sistare@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.
> 

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
>>>>
>>
>