| Message ID | 20240708143420.16953-2-joao.m.martins@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/vfio: IOMMUFD Dirty Tracking | expand |
Hi Joao, >-----Original Message----- >From: Joao Martins <joao.m.martins@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.duan@intel.com> >Fixes: 930589520128 ("vfio/iommufd: Implement >HostIOMMUDeviceClass::realize() handler") >Signed-off-by: Joao Martins <joao.m.martins@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. Thanks Zhenzhong >+ hiod->name = g_strdup(vdev->name); >+ caps->type = type; >+ caps->aw_bits = vfio_device_get_aw_bits(vdev); > } > >- hiod->name = g_strdup(vdev->name); >- caps->type = type; >- caps->aw_bits = vfio_device_get_aw_bits(vdev); >- > return true; > } > >-- >2.17.2
On 09/07/2024 04:43, Duan, Zhenzhong wrote: > Hi Joao, > >> -----Original Message----- >> From: Joao Martins <joao.m.martins@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.duan@intel.com> >> Fixes: 930589520128 ("vfio/iommufd: Implement >> HostIOMMUDeviceClass::realize() handler") >> Signed-off-by: Joao Martins <joao.m.martins@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?
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.martins@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.duan@intel.com> >>> Fixes: 930589520128 ("vfio/iommufd: Implement >>> HostIOMMUDeviceClass::realize() handler") >>> Signed-off-by: Joao Martins <joao.m.martins@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(). diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index a4d23f488b01..c0a019dffdb6 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -631,8 +631,9 @@ 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), NULL)) { + if (!vfio_is_mdev(vdev) && + iommufd_backend_get_device_info(vdev->iommufd, vdev->devid, + &type, &data, sizeof(data), errp)) { hiod->name = g_strdup(vdev->name); caps->type = type; caps->aw_bits = vfio_device_get_aw_bits(vdev); 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); + } }
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.martins@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.duan@intel.com> >>>> Fixes: 930589520128 ("vfio/iommufd: Implement >>>> HostIOMMUDeviceClass::realize() handler") >>>> Signed-off-by: Joao Martins <joao.m.martins@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); + } }
>-----Original Message----- >From: Joao Martins <joao.m.martins@oracle.com> >Subject: Re: [PATCH v3 01/10] vfio/iommufd: Don't fail to realize on >IOMMU_GET_HW_INFO failure > >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.martins@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.duan@intel.com> >>>>> Fixes: 930589520128 ("vfio/iommufd: Implement >>>>> HostIOMMUDeviceClass::realize() handler") >>>>> Signed-off-by: Joao Martins <joao.m.martins@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; >+ } >+ Not necessary to create a dummy object. What about bypassing object_new(ops->hiod_typename) in vfio_attach_device()? > 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)) { Yes. > 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); >+ } Yes. Thanks Zhenzhong > }
On 10/07/2024 03:53, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Joao Martins <joao.m.martins@oracle.com> >> Subject: Re: [PATCH v3 01/10] vfio/iommufd: Don't fail to realize on >> IOMMU_GET_HW_INFO failure >> >> 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.martins@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.duan@intel.com> >>>>>> Fixes: 930589520128 ("vfio/iommufd: Implement >>>>>> HostIOMMUDeviceClass::realize() handler") >>>>>> Signed-off-by: Joao Martins <joao.m.martins@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; >> + } >> + > > Not necessary to create a dummy object. > What about bypassing object_new(ops->hiod_typename) in vfio_attach_device()? > Not sure I am parsing this. What dummy object you refer to here if it's not vbasedev::hiod that remains unused? Also in a suggestion by Cedric, and pre-seeding vbasedev::hiod during attach_device()[0]. So I will sort of do that already, but your comments means we are allocating a dummy object anyways too? Or are you perhaps suggesting something like: @@ -1552,17 +1552,20 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev, assert(ops); if (!ops->attach_device(name, vbasedev, as, errp)) { return false; } if (!vfio_mdev(vbasedev) && !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { ? [0] https://lore.kernel.org/qemu-devel/4e85db04-fbaa-4a6b-b133-59170c471e24@oracle.com/
>-----Original Message----- >From: Joao Martins <joao.m.martins@oracle.com> >Subject: Re: [PATCH v3 01/10] vfio/iommufd: Don't fail to realize on >IOMMU_GET_HW_INFO failure > >On 10/07/2024 03:53, Duan, Zhenzhong wrote: >> >> >>> -----Original Message----- >>> From: Joao Martins <joao.m.martins@oracle.com> >>> Subject: Re: [PATCH v3 01/10] vfio/iommufd: Don't fail to realize on >>> IOMMU_GET_HW_INFO failure >>> >>> 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.martins@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.duan@intel.com> >>>>>>> Fixes: 930589520128 ("vfio/iommufd: Implement >>>>>>> HostIOMMUDeviceClass::realize() handler") >>>>>>> Signed-off-by: Joao Martins <joao.m.martins@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; >>> + } >>> + >> >> Not necessary to create a dummy object. >> What about bypassing object_new(ops->hiod_typename) in >vfio_attach_device()? >> >Not sure I am parsing this. What dummy object you refer to here if it's not >vbasedev::hiod that remains unused? Also in a suggestion by Cedric, and >pre-seeding vbasedev::hiod during attach_device()[0]. So I will sort of do that >already, but your comments means we are allocating a dummy object >anyways too? Yes, with your snip change, it's allocated by object_new(ops->hiod_typename) but not realized and never used else where. > >Or are you perhaps suggesting something like: > >@@ -1552,17 +1552,20 @@ bool vfio_attach_device(char *name, >VFIODevice *vbasedev, > > assert(ops); > > if (!ops->attach_device(name, vbasedev, as, errp)) { > return false; > } > > if (!vfio_mdev(vbasedev) && > !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, >errp)) { > >? I mean bypass host IOMMU device thoroughly for mdev, like: --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -1548,6 +1548,10 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev, return false; } + if (vfio_is_mdev(vdev)) { + return true; + } + hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { object_unref(hiod); > > >[0] >https://lore.kernel.org/qemu-devel/4e85db04-fbaa-4a6b-b133- >59170c471e24@oracle.com/
On 10/07/2024 10:54, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: Joao Martins <joao.m.martins@oracle.com> >> Subject: Re: [PATCH v3 01/10] vfio/iommufd: Don't fail to realize on >> IOMMU_GET_HW_INFO failure >> >> On 10/07/2024 03:53, Duan, Zhenzhong wrote: >>> >>> >>>> -----Original Message----- >>>> From: Joao Martins <joao.m.martins@oracle.com> >>>> Subject: Re: [PATCH v3 01/10] vfio/iommufd: Don't fail to realize on >>>> IOMMU_GET_HW_INFO failure >>>> >>>> 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.martins@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.duan@intel.com> >>>>>>>> Fixes: 930589520128 ("vfio/iommufd: Implement >>>>>>>> HostIOMMUDeviceClass::realize() handler") >>>>>>>> Signed-off-by: Joao Martins <joao.m.martins@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; >>>> + } >>>> + >>> >>> Not necessary to create a dummy object. >>> What about bypassing object_new(ops->hiod_typename) in >> vfio_attach_device()? >>> >> Not sure I am parsing this. What dummy object you refer to here if it's not >> vbasedev::hiod that remains unused? Also in a suggestion by Cedric, and >> pre-seeding vbasedev::hiod during attach_device()[0]. So I will sort of do that >> already, but your comments means we are allocating a dummy object >> anyways too? > > Yes, with your snip change, it's allocated by object_new(ops->hiod_typename) but not realized > and never used else where. > >> >> Or are you perhaps suggesting something like: >> >> @@ -1552,17 +1552,20 @@ bool vfio_attach_device(char *name, >> VFIODevice *vbasedev, >> >> assert(ops); >> >> if (!ops->attach_device(name, vbasedev, as, errp)) { >> return false; >> } >> >> if (!vfio_mdev(vbasedev) && >> !HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, >> errp)) { >> >> ? > > I mean bypass host IOMMU device thoroughly for mdev, like: > /me facepalm. Makes sense! I read your comment in my head as "What about by passing object_new(ops->hiod_typename)", when it was 'bypassing' that you wrote. > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -1548,6 +1548,10 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev, > return false; > } > > + if (vfio_is_mdev(vdev)) { > + return true; > + } > + > hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename)); > if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) { > object_unref(hiod); > > >> >> >> [0] >> https://lore.kernel.org/qemu-devel/4e85db04-fbaa-4a6b-b133- >> 59170c471e24@oracle.com/ >
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)) { + hiod->name = g_strdup(vdev->name); + caps->type = type; + caps->aw_bits = vfio_device_get_aw_bits(vdev); } - hiod->name = g_strdup(vdev->name); - caps->type = type; - caps->aw_bits = vfio_device_get_aw_bits(vdev); - return true; }
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.duan@intel.com> Fixes: 930589520128 ("vfio/iommufd: Implement HostIOMMUDeviceClass::realize() handler") Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- hw/vfio/iommufd.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)