Re: [PATCH v2] memory: notify hypervisor of all eventfds during listener (de)registration

[Peter Xu <peterx@redhat.com>]

Hi, Ani,

On Tue, Sep 10, 2024 at 05:31:00PM +0530, Ani Sinha wrote:
> When a new listener for an address space is registered, the hypervisor must be
> informed of all existing eventfds for that address space by calling
> eventfd_add() for that listener. Similarly, when a listener is de-registered
> from an address space, the hypervisor must be informed of all existing eventfds
> for that address space with a call to eventfd_del().
> 
> Same is also true for coalesced io. Send coalesced io add/del listener
> notifications if any flatrage for the address space registered with the
> listener intersects with any coalesced io range.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Ani Sinha <anisinha@redhat.com>

This is only to make the API clean, as I see that only kvm uses either of
the features (while kvm never registers during VM running).  Am I right?

I'd like to double check whether we need to copy stable, or whether there's
a real bug.

> ---
>  system/memory.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
> 
> changelog:
> v2: tags added, indentation fixed, commit log fixed, code cleanup.
> 
> diff --git a/system/memory.c b/system/memory.c
> index 5e6eb459d5..8379e086fb 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -941,6 +941,38 @@ static void flat_range_coalesced_io_add(FlatRange *fr, AddressSpace *as)
>      }
>  }
>  
> +static void
> +flat_range_coalesced_io_notify_listener_add_del(FlatRange *fr,
> +                                                MemoryRegionSection *mrs,
> +                                                MemoryListener *listener,
> +                                                AddressSpace *as, bool add)
> +{
> +    CoalescedMemoryRange *cmr;
> +    MemoryRegion *mr = fr->mr;
> +    AddrRange tmp;
> +
> +    QTAILQ_FOREACH(cmr, &mr->coalesced, link) {
> +        tmp = addrrange_shift(cmr->addr,
> +                              int128_sub(fr->addr.start,
> +                                         int128_make64(fr->offset_in_region)));
> +
> +        if (!addrrange_intersects(tmp, fr->addr)) {
> +            return;
> +        }
> +        tmp = addrrange_intersection(tmp, fr->addr);
> +
> +        if (add) {

Do we need to check ->coalesced_io_add != NULL here?  I wonder whether this
could crash if some dynamic listener got inserted (with coalesced list
non-empty).

> +            listener->coalesced_io_add(listener, mrs,
> +                                       int128_get64(tmp.start),
> +                                       int128_get64(tmp.size));
> +        } else {
> +            listener->coalesced_io_del(listener, mrs,

Same.

> +                                       int128_get64(tmp.start),
> +                                       int128_get64(tmp.size));
> +        }
> +    }
> +}
> +
>  static void address_space_update_topology_pass(AddressSpace *as,
>                                                 const FlatView *old_view,
>                                                 const FlatView *new_view,
> @@ -3015,8 +3047,10 @@ void memory_global_dirty_log_stop(unsigned int flags)
>  static void listener_add_address_space(MemoryListener *listener,
>                                         AddressSpace *as)
>  {
> +    unsigned i;
>      FlatView *view;
>      FlatRange *fr;
> +    MemoryRegionIoeventfd *fd;
>  
>      if (listener->begin) {
>          listener->begin(listener);
> @@ -3041,10 +3075,31 @@ static void listener_add_address_space(MemoryListener *listener,
>          if (listener->region_add) {
>              listener->region_add(listener, &section);
>          }
> +
> +        /* send coalesced io add notifications */
> +        flat_range_coalesced_io_notify_listener_add_del(fr, &section,
> +                                                        listener, as, true);
> +
>          if (fr->dirty_log_mask && listener->log_start) {
>              listener->log_start(listener, &section, 0, fr->dirty_log_mask);
>          }
>      }
> +
> +    /*
> +     * register all eventfds for this address space for the newly registered
> +     * listener.
> +     */
> +    for (i = 0; i < as->ioeventfd_nb; i++) {
> +        fd = &as->ioeventfds[i];
> +        MemoryRegionSection section = (MemoryRegionSection) {
> +            .fv = address_space_to_flatview(as),

Nitpick: I don't think it'll be anything different if we hold BQL anyway,
but.. might be good to use "view" directly here, as we're holding a
refcount there via address_space_get_flatview().

While it's not crystal clear otherwise (when without knowing BQL held) that
address_space_to_flatview() will return the same flatview.

> +            .offset_within_address_space = int128_get64(fd->addr.start),
> +            .size = fd->addr.size,
> +        };
> +        listener->eventfd_add(listener, &section,
> +                              fd->match_data, fd->data, fd->e);

Check ->eventfd_add != NULL?

> +    }
> +
>      if (listener->commit) {
>          listener->commit(listener);
>      }
> @@ -3054,8 +3109,10 @@ static void listener_add_address_space(MemoryListener *listener,
>  static void listener_del_address_space(MemoryListener *listener,
>                                         AddressSpace *as)
>  {
> +    unsigned i;
>      FlatView *view;
>      FlatRange *fr;
> +    MemoryRegionIoeventfd *fd;
>  
>      if (listener->begin) {
>          listener->begin(listener);
> @@ -3067,10 +3124,30 @@ static void listener_del_address_space(MemoryListener *listener,
>          if (fr->dirty_log_mask && listener->log_stop) {
>              listener->log_stop(listener, &section, fr->dirty_log_mask, 0);
>          }
> +
> +        /* send coalesced io del notifications */
> +        flat_range_coalesced_io_notify_listener_add_del(fr, &section,
> +                                                        listener, as, false);
>          if (listener->region_del) {
>              listener->region_del(listener, &section);
>          }
>      }
> +
> +    /*
> +     * de-register all eventfds for this address space for the current
> +     * listener.
> +     */
> +    for (i = 0; i < as->ioeventfd_nb; i++) {
> +        fd = &as->ioeventfds[i];
> +        MemoryRegionSection section = (MemoryRegionSection) {
> +            .fv = address_space_to_flatview(as),

Same.

> +            .offset_within_address_space = int128_get64(fd->addr.start),
> +            .size = fd->addr.size,
> +        };
> +        listener->eventfd_del(listener, &section,
> +                              fd->match_data, fd->data, fd->e);

Check eventfd_del != NULL?

Thanks,

> +    }
> +
>      if (listener->commit) {
>          listener->commit(listener);
>      }
> -- 
> 2.42.0
> 

-- 
Peter Xu