Re: [PATCH 3/3] vfio: Make VFIOIOMMUClass::add_window() and its wrapper return bool

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

On 5/6/24 10:33, Zhenzhong Duan wrote:
> Make VFIOIOMMUClass::add_window() and its wrapper function
> vfio_container_add_section_window() return bool.
> 
> This is to follow the coding standand to return bool if 'Error **'
> is used to pass error.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

One comment below for the spapr maintainers,


> ---
>   include/hw/vfio/vfio-container-base.h | 12 ++++++------
>   hw/vfio/common.c                      |  2 +-
>   hw/vfio/container-base.c              |  8 ++++----
>   hw/vfio/spapr.c                       | 16 ++++++++--------
>   4 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index 68539e3bed..e96cda78c8 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -76,9 +76,9 @@ int vfio_container_dma_map(VFIOContainerBase *bcontainer,
>   int vfio_container_dma_unmap(VFIOContainerBase *bcontainer,
>                                hwaddr iova, ram_addr_t size,
>                                IOMMUTLBEntry *iotlb);
> -int vfio_container_add_section_window(VFIOContainerBase *bcontainer,
> -                                      MemoryRegionSection *section,
> -                                      Error **errp);
> +bool vfio_container_add_section_window(VFIOContainerBase *bcontainer,
> +                                       MemoryRegionSection *section,
> +                                       Error **errp);
>   void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>                                          MemoryRegionSection *section);
>   int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
> @@ -131,9 +131,9 @@ struct VFIOIOMMUClass {
>       int (*pci_hot_reset)(VFIODevice *vbasedev, bool single);
>   
>       /* SPAPR specific */
> -    int (*add_window)(VFIOContainerBase *bcontainer,
> -                      MemoryRegionSection *section,
> -                      Error **errp);
> +    bool (*add_window)(VFIOContainerBase *bcontainer,
> +                       MemoryRegionSection *section,
> +                       Error **errp);
>       void (*del_window)(VFIOContainerBase *bcontainer,
>                          MemoryRegionSection *section);
>       void (*release)(VFIOContainerBase *bcontainer);
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 890d30910e..9f1f2e19f7 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -585,7 +585,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>           return;
>       }
>   
> -    if (vfio_container_add_section_window(bcontainer, section, &err)) {
> +    if (!vfio_container_add_section_window(bcontainer, section, &err)) {
>           goto fail;
>       }
>   
> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
> index 913ae49077..98d71b3144 100644
> --- a/hw/vfio/container-base.c
> +++ b/hw/vfio/container-base.c
> @@ -31,12 +31,12 @@ int vfio_container_dma_unmap(VFIOContainerBase *bcontainer,
>       return bcontainer->ops->dma_unmap(bcontainer, iova, size, iotlb);
>   }
>   
> -int vfio_container_add_section_window(VFIOContainerBase *bcontainer,
> -                                      MemoryRegionSection *section,
> -                                      Error **errp)
> +bool vfio_container_add_section_window(VFIOContainerBase *bcontainer,
> +                                       MemoryRegionSection *section,
> +                                       Error **errp)
>   {
>       if (!bcontainer->ops->add_window) {
> -        return 0;
> +        return true;
>       }
>   
>       return bcontainer->ops->add_window(bcontainer, section, errp);
> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> index 148b257c9c..47b040f1bc 100644
> --- a/hw/vfio/spapr.c
> +++ b/hw/vfio/spapr.c
> @@ -323,7 +323,7 @@ static int vfio_spapr_create_window(VFIOContainer *container,
>       return 0;
>   }
>   
> -static int
> +static bool
>   vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer,
>                                           MemoryRegionSection *section,
>                                           Error **errp)
> @@ -351,13 +351,13 @@ vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer,
>               error_setg(errp, "Container %p can't map guest IOVA region"
>                          " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx, container,
>                          iova, end);
> -            return -EINVAL;
> +            return false;
>           }
> -        return 0;
> +        return true;
>       }
>   
>       if (container->iommu_type != VFIO_SPAPR_TCE_v2_IOMMU) {
> -        return 0;
> +        return true;
>       }
>   
>       /* For now intersections are not allowed, we may relax this later */
> @@ -373,14 +373,14 @@ vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer,
>                   section->offset_within_address_space +
>                       int128_get64(section->size) - 1,
>                   hostwin->min_iova, hostwin->max_iova);
> -            return -EINVAL;
> +            return false;
>           }
>       }
>   
>       ret = vfio_spapr_create_window(container, section, &pgsize);

vfio_spapr_create_window() contains several calls to error_report() which
would be good to replace with error_setg().


Thanks,

C.




>       if (ret) {
>           error_setg_errno(errp, -ret, "Failed to create SPAPR window");
> -        return ret;
> +        return false;
>       }
>   
>       vfio_host_win_add(scontainer, section->offset_within_address_space,
> @@ -406,14 +406,14 @@ vfio_spapr_container_add_section_window(VFIOContainerBase *bcontainer,
>                                        "vfio: failed GROUP_SET_SPAPR_TCE for "
>                                        "KVM VFIO device %d and group fd %d",
>                                        param.tablefd, param.groupfd);
> -                    return -errno;
> +                    return false;
>                   }
>                   trace_vfio_spapr_group_attach(param.groupfd, param.tablefd);
>               }
>           }
>       }
>   #endif
> -    return 0;
> +    return true;
>   }
>   
>   static void