Re: [PATCH v2 02/10] hw/vfio/region: Add vfio_region_setup_with_ops()

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

On 9/16/26 20:44, mhonap@nvidia.com wrote:
> From: Manish Honap <mhonap@nvidia.com>
> 
> A CXL Type-2 device traps its HDM decoder block, so that region needs
> custom MemoryRegionOps rather than the pass-through default. Split the
> setup body out and let a caller supply the ops; NULL keeps the existing
> behaviour.
> 
> AI-used-for: code (prototype)
> Signed-off-by: Manish Honap <mhonap@nvidia.com>
> ---
>   hw/vfio/region.c      | 27 ++++++++++++++++++++++++---
>   hw/vfio/vfio-region.h |  3 +++
>   2 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/vfio/region.c b/hw/vfio/region.c
> index 54ad11a6c8..21b54978b5 100644
> --- a/hw/vfio/region.c
> +++ b/hw/vfio/region.c
> @@ -228,8 +228,9 @@ static int vfio_setup_region_sparse_mmaps(VFIORegion *region,
>       return 0;
>   }
>   
> -int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
> -                      int index, const char *name, Error **errp)
> +static int vfio_region_do_setup(Object *obj, VFIODevice *vbasedev,
> +                                VFIORegion *region, int index, const char *name,
> +                                const MemoryRegionOps *ops, Error **errp)
>   {
>       struct vfio_region_info *info = NULL;
>       int ret;
> @@ -249,7 +250,7 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
>   
>       if (region->size) {
>           region->mem = g_new0(MemoryRegion, 1);
> -        memory_region_init_io(region->mem, obj, &vfio_region_ops,
> +        memory_region_init_io(region->mem, obj, ops,
>                                 region, name, region->size);
>   
>           if (!vbasedev->no_mmap &&
> @@ -273,6 +274,26 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
>       return 0;
>   }
>   
> +int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
> +                      int index, const char *name, Error **errp)
> +{
> +    return vfio_region_do_setup(obj, vbasedev, region, index, name,
> +                                &vfio_region_ops, errp);
> +}
> +
> +/*
> + * Like vfio_region_setup() but traps the region through @ops instead of the
> + * default pass-through, so a caller can intercept accesses (the CXL HDM
> + * decoder block). A NULL @ops keeps the default.
> + */
> +int vfio_region_setup_with_ops(Object *obj, VFIODevice *vbasedev,
> +                               VFIORegion *region, int index, const char *name,
> +                               const MemoryRegionOps *ops, Error **errp)
> +{
> +    return vfio_region_do_setup(obj, vbasedev, region, index, name,
> +                                ops ? ops : &vfio_region_ops, errp);

if you're calling _with_ops(), you have custom ops, that's whole point.
Passing NULL is a caller bug, no need to have a fall back. Or assert().

Thanks,

C.



> +}
> +
>   static void vfio_subregion_unmap(VFIORegion *region, int index)
>   {
>       trace_vfio_region_unmap(memory_region_name(&region->mmaps[index].mem),
> diff --git a/hw/vfio/vfio-region.h b/hw/vfio/vfio-region.h
> index 58b236f113..8d5699013a 100644
> --- a/hw/vfio/vfio-region.h
> +++ b/hw/vfio/vfio-region.h
> @@ -39,6 +39,9 @@ uint64_t vfio_region_read(void *opaque,
>                             hwaddr addr, unsigned size);
>   int vfio_region_setup(Object *obj, VFIODevice *vbasedev, VFIORegion *region,
>                         int index, const char *name, Error **errp);
> +int vfio_region_setup_with_ops(Object *obj, VFIODevice *vbasedev,
> +                               VFIORegion *region, int index, const char *name,
> +                               const MemoryRegionOps *ops, Error **errp);
>   int vfio_region_mmap(VFIORegion *region);
>   void vfio_region_mmaps_set_enabled(VFIORegion *region, bool enabled);
>   void vfio_region_exit(VFIORegion *region);