On 09/07/2024 12:45, Joao Martins wrote:
> On 09/07/2024 09:56, Joao Martins wrote:
>> On 09/07/2024 04:43, Duan, Zhenzhong wrote:
>>> Hi Joao,
>>>
>>>> -----Original Message-----
>>>> From: Joao Martins <joao.m.mart...@oracle.com>
>>>> Subject: [PATCH v3 01/10] vfio/iommufd: Don't fail to realize on
>>>> IOMMU_GET_HW_INFO failure
>>>>
>>>> mdevs aren't "physical" devices and when asking for backing IOMMU info, it
>>>> fails the entire provisioning of the guest. Fix that by filling caps info
>>>> when IOMMU_GET_HW_INFO succeeds plus discarding the error we would
>>>> get into
>>>> iommufd_backend_get_device_info().
>>>>
>>>> Cc: Zhenzhong Duan <zhenzhong.d...@intel.com>
>>>> Fixes: 930589520128 ("vfio/iommufd: Implement
>>>> HostIOMMUDeviceClass::realize() handler")
>>>> Signed-off-by: Joao Martins <joao.m.mart...@oracle.com>
>>>> ---
>>>> hw/vfio/iommufd.c | 12 +++++-------
>>>> 1 file changed, 5 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>>>> index c2f158e60386..a4d23f488b01 100644
>>>> --- a/hw/vfio/iommufd.c
>>>> +++ b/hw/vfio/iommufd.c
>>>> @@ -631,15 +631,13 @@ static bool
>>>> hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>>>
>>>> hiod->agent = opaque;
>>>>
>>>> - if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
>>>> - &type, &data, sizeof(data),
>>>> errp)) {
>>>> - return false;
>>>> + if (iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
>>>> + &type, &data, sizeof(data),
>>>> NULL)) {
>>>
>>> This will make us miss the real error. What about bypassing host IOMMU
>>> device
>>> creation for mdev as it's not "physical device", passing corresponding host
>>> IOMMU
>>> device to vIOMMU make no sense.
>>
>> Yeap -- This was my second alternative.
>>
>> I can add an helper for vfio_is_mdev()) and just call
>> iommufd_backend_get_device_info() if !vfio_is_mdev(). I am assuming you
>> meant
>> to skip the initialization of HostIOMMUDeviceCaps::caps as I think that
>> initializing hiod still makes sense as we are still using a
>> TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO somewhat?
>>
> Something like this is what I've done with this patch, see below. I think it
> matches what you suggested? Naturally there's a precedent patch that
> introduces
> vfio_is_mdev().
>
Sorry ignore the previous snip, it was the wrong version, see below instead.
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index c2f158e60386..987dd9779f94 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -631,6 +631,10 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice
*hiod, void *opaque,
hiod->agent = opaque;
+ if (vfio_is_mdev(vdev)) {
+ return true;
+ }
+
if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
&type, &data, sizeof(data), errp)) {
return false;
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d95aa6b65788..f092c1537999 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3115,7 +3115,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
vfio_bars_register(vdev);
- if (!pci_device_set_iommu_device(pdev, vbasedev->hiod, errp)) {
+ if (!is_mdev && !pci_device_set_iommu_device(pdev, vbasedev->hiod, errp)) {
error_prepend(errp, "Failed to set iommu_device: ");
goto out_teardown;
}
@@ -3238,7 +3238,9 @@ out_deregister:
timer_free(vdev->intx.mmap_timer);
}
out_unset_idev:
- pci_device_unset_iommu_device(pdev);
+ if (!is_mdev) {
+ pci_device_unset_iommu_device(pdev);
+ }
out_teardown:
vfio_teardown_msi(vdev);
vfio_bars_exit(vdev);
@@ -3268,6 +3270,7 @@ static void vfio_exitfn(PCIDevice *pdev)
{
VFIOPCIDevice *vdev = VFIO_PCI(pdev);
VFIODevice *vbasedev = &vdev->vbasedev;
+ bool is_mdev = vfio_is_mdev(vbasedev);
vfio_unregister_req_notifier(vdev);
vfio_unregister_err_notifier(vdev);
@@ -3283,7 +3286,9 @@ static void vfio_exitfn(PCIDevice *pdev)
vfio_pci_disable_rp_atomics(vdev);
vfio_bars_exit(vdev);
vfio_migration_exit(vbasedev);
- pci_device_unset_iommu_device(pdev);
+ if (!is_mdev) {
+ pci_device_unset_iommu_device(pdev);
+ }
}
