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

[Ani Sinha]

> On 18 Sep 2024, at 12:28 AM, Peter Xu <peterx@redhat.com> wrote:
> 
> 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?

Yes currently there is no use case for deregistering a listener and then 
re-registering. Hence there seems to be a gap in the implementation. I am 
working on something that would require de-register and re-register so we will 
need to fix this.

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

I think this is not a stable material as up until now, people will not hit this 
gap in the implementation.

> 
>> ---
>> 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,

All of the above suggestions are great. I will fix them in a re-spin. Thanks 
Peter.

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