pci_dev_realize() use the local error variable, which requires
`error_setg()` API to allocate the error object at first.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
---
 hw/vfio/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 0a99e55247..d994ad8bb9 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3117,7 +3117,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
 
     if (!vbasedev->mdev &&
         !pci_device_set_iommu_device(pdev, vbasedev->hiod, errp)) {
-        error_prepend(errp, "Failed to set iommu_device: ");
+        error_setg(errp, "Failed to set iommu_device: ");
         goto out_teardown;
     }
 
-- 
2.17.1

Hello Jim,

On 9/12/24 07:17, Jim Shu wrote:
> pci_dev_realize() use the local error variable, which requires
> `error_setg()` API to allocate the error object at first.
> 
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> ---
>   hw/vfio/pci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 0a99e55247..d994ad8bb9 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3117,7 +3117,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>   
>       if (!vbasedev->mdev &&
>           !pci_device_set_iommu_device(pdev, vbasedev->hiod, errp)) {

'errp' will be set by pci_device_set_iommu_device() in case of
failure and, in this case, calling error_prepend() is a valid
thing to do. I think we are fine.

Thanks,

C.



> -        error_prepend(errp, "Failed to set iommu_device: ");
> +        error_setg(errp, "Failed to set iommu_device: ");
>           goto out_teardown;
>       }
>

Hi Cédric,

Thank you very much for the quick response!

I have checked the error API again. It seems to be my porting issue of
set_iommu_device() callback.
I think "pci_device_set_iommu_device(..., *errp)" should set 'errp' if
this function returns false, right?

Thanks,
Jim

On Thu, Sep 12, 2024 at 2:18 PM Cédric Le Goater <clg@redhat.com> wrote:
>
> Hello Jim,
>
> On 9/12/24 07:17, Jim Shu wrote:
> > pci_dev_realize() use the local error variable, which requires
> > `error_setg()` API to allocate the error object at first.
> >
> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > ---
> >   hw/vfio/pci.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 0a99e55247..d994ad8bb9 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -3117,7 +3117,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> >
> >       if (!vbasedev->mdev &&
> >           !pci_device_set_iommu_device(pdev, vbasedev->hiod, errp)) {
>
> 'errp' will be set by pci_device_set_iommu_device() in case of
> failure and, in this case, calling error_prepend() is a valid
> thing to do. I think we are fine.
>
> Thanks,
>
> C.
>
>
>
> > -        error_prepend(errp, "Failed to set iommu_device: ");
> > +        error_setg(errp, "Failed to set iommu_device: ");
> >           goto out_teardown;
> >       }
> >
>

Hello Jim,

On 9/12/24 08:36, Jim Shu wrote:
> Hi Cédric,
> 
> Thank you very much for the quick response!
> 
> I have checked the error API again. It seems to be my porting issue of
> set_iommu_device() callback.

Are you adding support for a new IOMMU ?

> I think "pci_device_set_iommu_device(..., *errp)" should set 'errp' if
> this function returns false, right?

yes, this is a requirement for routines using an Error parameter.
You can take a look at the "= Rules =" section in include/qapi/error.h
for more info.

Thanks,

C.

On Thu, Sep 12, 2024 at 5:56 PM Cédric Le Goater <clg@redhat.com> wrote:
>
> Hello Jim,
>
> On 9/12/24 08:36, Jim Shu wrote:
> > Hi Cédric,
> >
> > Thank you very much for the quick response!
> >
> > I have checked the error API again. It seems to be my porting issue of
> > set_iommu_device() callback.
>
> Are you adding support for a new IOMMU ?

Yes, I am working on RISC-V IOMMU support, based on Zhenzhong's
iommufd nesting series [1] and RISC-V IOMMU patch v7.
[1] https://github.com/yiliu1765/qemu/commits/zhenzhong/iommufd_nesting_rfcv2

>
> > I think "pci_device_set_iommu_device(..., *errp)" should set 'errp' if
> > this function returns false, right?
>
> yes, this is a requirement for routines using an Error parameter.
> You can take a look at the "= Rules =" section in include/qapi/error.h
> for more info.

Thanks for the information! I will take a look.

>
> Thanks,
>
> C.
>