Re: [PATCH v3 7/7] vfio: Remove superfluous error report in vfio_listener_region_add()

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

On 2/6/25 18:44, Cédric Le Goater wrote:
> For pseries machines, commit 567b5b309abe ("vfio/pci: Relax DMA map
> errors for MMIO regions") introduced 2 error reports to notify the
> user of MMIO mapping errors. Consolidate both code paths under one.
> 
> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Cc: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/vfio/common.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 4fca4c6166f761acceb7b57b52e379603ea53876..abbdc56b6dbb5eed22e7a2d2d55ee5695279661e 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -594,8 +594,9 @@ static void vfio_listener_region_add(MemoryListener *listener,
>           return;
>       }
>   
> +    /* PPC64/pseries machine only */
>       if (!vfio_container_add_section_window(bcontainer, section, &err)) {
> -        goto fail;
> +        goto mmio_dma_error;
>       }
>   
>       memory_region_ref(section->mr);
> @@ -680,6 +681,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>                      "0x%"HWADDR_PRIx", %p) = %d (%s)",
>                      bcontainer, iova, int128_get64(llsize), vaddr, ret,
>                      strerror(-ret));
> +    mmio_dma_error:
>           if (memory_region_is_ram_device(section->mr)) {
>               /* Allow unexpected mappings not to be fatal for RAM devices */
>               VFIODevice *vbasedev =
> @@ -694,11 +696,6 @@ static void vfio_listener_region_add(MemoryListener *listener,
>       return;
>   
>   fail:
> -    if (memory_region_is_ram_device(section->mr)) {
> -        error_reportf_err(err, "PCI p2p may not work: ");

Not sure if this specific error msg was intended for a specific case, 
but since both ifs check for same condition before returning with error, 
looks logically same.

Shiva, any concerns here from vfio perspective?

Otherwise,
Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> -        return;
> -    }
> -
>       if (!bcontainer->initialized) {
>           /*
>            * At machine init time or when the device is attached to the
> @@ -806,6 +803,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
>   
>       memory_region_unref(section->mr);
>   
> +    /* PPC64/pseries machine only */
>       vfio_container_del_section_window(bcontainer, section);
>   }
>