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

["Cédric Le Goater" <clg@redhat.com>]

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?


Thanks,

C.