RE: [PATCH v6 01/21] backends/iommufd: Introduce the iommufd object

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Tuesday, November 21, 2023 1:09 AM
>Subject: Re: [PATCH v6 01/21] backends/iommufd: Introduce the iommufd object
>
>Hello Zhenzhong
>
>On 11/20/23 11:07, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Sent: Monday, November 20, 2023 4:25 PM
>>> Subject: Re: [PATCH v6 01/21] backends/iommufd: Introduce the iommufd
>object
>>>
>>>>>>> A similar issue with a fix submitted below, ccing related people.
>>>>>>> https://lists.gnu.org/archive/html/qemu-devel/2023-11/msg02937.html
>>>>>>> It looks the fix will not work for hotplug.
>>>>>>>
>>>>>>> Or below qemu cmdline may help:
>>>>>>> "-cpu host,host-phys-bits-limit=39"
>>>>>>
>>>>>> don't you have the same issue with legacy VFIO code, you should?
>>>>>
>>>>> I tend to be lazy and use seabios for guests on the command line.
>>>>> I do see the error with legacy VFIO and uefi.
>>>>>
>>>>> However, with the address space size work-around and iommufd, the
>>>>> error is different, an EFAULT now. Some page pinning issue it seems.
>>>>
>>>> Yes, this reminds me of iommufd not supporting p2p mapping yet.
>>>
>>> OK. Should we transform this error in a warning ? The code needs
>>> at least a comment.
>>
>> Make sense, though I'm not clear if there is other corner case return EFAULT.
>
>yep. That's the problem.
>
>> I plan below change in v7:
>>
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> index 53fdac4cc0..ba58a0eb0d 100644
>> --- a/backends/iommufd.c
>> +++ b/backends/iommufd.c
>> @@ -178,7 +178,13 @@ int iommufd_backend_map_dma(IOMMUFDBackend
>*be, uint32_t ioas_id, hwaddr iova,
>>                                     vaddr, readonly, ret);
>>       if (ret) {
>>           ret = -errno;
>> -        error_report("IOMMU_IOAS_MAP failed: %m");
>> +
>> +        /* TODO: Not support mapping hardware PCI BAR region for now. */
>> +        if (errno == EFAULT) {
>> +            warn_report("IOMMU_IOAS_MAP failed: %m, PCI BAR?");
>> +        } else {
>> +            error_report("IOMMU_IOAS_MAP failed: %m");
>> +        }
>>       }
>>       return ret;
>>   }
>>
>> I failed to change vfio_container_dma_map print as warning because for legacy
>container, it's real errro.
>> So print after fix:
>>
>> qemu-system-x86_64: warning: IOMMU_IOAS_MAP failed: Bad address, PCI
>BAR?
>> qemu-system-x86_64: vfio_container_dma_map(0x560cb6cb1620,
>0xe000000021000, 0x3000, 0x7f32ed55c000) = -14 (Bad address)
>
>I am OK with that. Let's see what the others have to say.
>
>>>
>>>> So EFAULT is expected. Maybe I should add a comment in docs/devel/vfio-
>>> iommufd.rst
>>>
>>> Yes. It would be good to have a list of gaps and effects in the
>>> documentation. See Jason's presentation at LPC.
>>>
>>>
>>>
>https://lpc.events/event/17/contributions/1418/attachments/1297/2607/LPC202
>>> 3_iommufd.pdf
>>
>> I see, PCI Peer to Peer and POWER/SPAPR are related to qemu iommufd
>implementation.
>> For POWER/SPAPR, we have "Supported platform" section.
>
>yes.
>
>> Below are other gaps I can think of for now:
>>
>> Gaps:
>> 1. dirty page sync, WIP (Joao)
>> 2. p2p dma not supported yet.
>> 3. fd passing with mdev not support ram discard(vfio-pci) as no way to know 
>> it's
>a mdev from a fd.
>
>Call the section Caveats maybe?

Got it.

Thanks
Zhenzhong