Re: [PATCH v4 10/13] memory: Change NotifyStateClear() definition to return the result

[Chenyi Qiang <chenyi.qiang@intel.com>]

Hi David,

Any thought on patch 10-12, which is to move the change attribute into a
priority listener. A problem is how to handle the error handling of
private_to_shared failure. Previously, we thought it would never be able
to fail, but right now, it is possible in corner cases (e.g. -ENOMEM) in
set_attribute_private(). At present, I simply raise an assert instead of
adding any rollback work (see patch 11).

On 4/7/2025 3:49 PM, Chenyi Qiang wrote:
> So that the caller can check the result of NotifyStateClear() handler if
> the operation fails.
> 
> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
> ---
> Changes in v4:
>     - Newly added.
> ---
>  hw/vfio/common.c      | 18 ++++++++++--------
>  include/exec/memory.h |  4 ++--
>  2 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 48468a12c3..6e49ae597d 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -335,8 +335,8 @@ out:
>      rcu_read_unlock();
>  }
>  
> -static void vfio_state_change_notify_to_state_clear(VFIOContainerBase *bcontainer,
> -                                                    MemoryRegionSection *section)
> +static int vfio_state_change_notify_to_state_clear(VFIOContainerBase *bcontainer,
> +                                                   MemoryRegionSection *section)
>  {
>      const hwaddr size = int128_get64(section->size);
>      const hwaddr iova = section->offset_within_address_space;
> @@ -348,24 +348,26 @@ static void vfio_state_change_notify_to_state_clear(VFIOContainerBase *bcontaine
>          error_report("%s: vfio_container_dma_unmap() failed: %s", __func__,
>                       strerror(-ret));
>      }
> +
> +    return ret;
>  }
>  
> -static void vfio_ram_discard_notify_discard(StateChangeListener *scl,
> -                                            MemoryRegionSection *section)
> +static int vfio_ram_discard_notify_discard(StateChangeListener *scl,
> +                                           MemoryRegionSection *section)
>  {
>      RamDiscardListener *rdl = container_of(scl, RamDiscardListener, scl);
>      VFIORamDiscardListener *vrdl = container_of(rdl, VFIORamDiscardListener,
>                                                  listener);
> -    vfio_state_change_notify_to_state_clear(vrdl->bcontainer, section);
> +    return vfio_state_change_notify_to_state_clear(vrdl->bcontainer, section);
>  }
>  
> -static void vfio_private_shared_notify_to_private(StateChangeListener *scl,
> -                                                  MemoryRegionSection *section)
> +static int vfio_private_shared_notify_to_private(StateChangeListener *scl,
> +                                                 MemoryRegionSection *section)
>  {
>      PrivateSharedListener *psl = container_of(scl, PrivateSharedListener, scl);
>      VFIOPrivateSharedListener *vpsl = container_of(psl, VFIOPrivateSharedListener,
>                                                     listener);
> -    vfio_state_change_notify_to_state_clear(vpsl->bcontainer, section);
> +    return vfio_state_change_notify_to_state_clear(vpsl->bcontainer, section);
>  }
>  
>  static int vfio_state_change_notify_to_state_set(VFIOContainerBase *bcontainer,
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index a61896251c..9472d9e9b4 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -523,8 +523,8 @@ typedef int (*ReplayStateChange)(MemoryRegionSection *section, void *opaque);
>  typedef struct StateChangeListener StateChangeListener;
>  typedef int (*NotifyStateSet)(StateChangeListener *scl,
>                                MemoryRegionSection *section);
> -typedef void (*NotifyStateClear)(StateChangeListener *scl,
> -                                 MemoryRegionSection *section);
> +typedef int (*NotifyStateClear)(StateChangeListener *scl,
> +                                MemoryRegionSection *section);
>  
>  struct StateChangeListener {
>      /*