[qemu,v16,02/19] memory: Call region_del() callbacks on memory listener unregistering

When a new memory listener is registered, listener_add_address_space()
is called and which in turn calls region_add() callbacks of memory regions.
However when unregistering the memory listener, it is just removed from
the listening chain and no region_del() is called.

This adds listener_del_address_space() and uses it in
memory_listener_unregister(). listener_add_address_space() was used as
a template with the following changes:
s/log_global_start/log_global_stop/
s/log_start/log_stop/
s/region_add/region_del/

This will allow the following patches to add/remove DMA windows
dynamically from VFIO's PCI address space's region_add()/region_del().

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 memory.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

On Wed,  4 May 2016 16:52:14 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> When a new memory listener is registered, listener_add_address_space()
> is called and which in turn calls region_add() callbacks of memory regions.
> However when unregistering the memory listener, it is just removed from
> the listening chain and no region_del() is called.
> 
> This adds listener_del_address_space() and uses it in
> memory_listener_unregister(). listener_add_address_space() was used as
> a template with the following changes:
> s/log_global_start/log_global_stop/
> s/log_start/log_stop/
> s/region_add/region_del/
> 
> This will allow the following patches to add/remove DMA windows
> dynamically from VFIO's PCI address space's region_add()/region_del().

Following patch 1 comments, it would be a bug if the kernel actually
needed this to do cleanup, we must release everything if QEMU gets shot
with a SIGKILL anyway.  So what does this cleanup facilitate in QEMU?
Having QEMU trigger an unmap for each region_del is not going to be as
efficient as just dropping the container and letting the kernel handle
the cleanup all in one go.  Thanks,

Alex

> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  memory.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/memory.c b/memory.c
> index f76f85d..f762a34 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2185,6 +2185,49 @@ static void listener_add_address_space(MemoryListener *listener,
>      flatview_unref(view);
>  }
>  
> +static void listener_del_address_space(MemoryListener *listener,
> +                                       AddressSpace *as)
> +{
> +    FlatView *view;
> +    FlatRange *fr;
> +
> +    if (listener->address_space_filter
> +        && listener->address_space_filter != as) {
> +        return;
> +    }
> +
> +    if (listener->begin) {
> +        listener->begin(listener);
> +    }
> +    if (global_dirty_log) {
> +        if (listener->log_global_stop) {
> +            listener->log_global_stop(listener);
> +        }
> +    }
> +
> +    view = address_space_get_flatview(as);
> +    FOR_EACH_FLAT_RANGE(fr, view) {
> +        MemoryRegionSection section = {
> +            .mr = fr->mr,
> +            .address_space = as,
> +            .offset_within_region = fr->offset_in_region,
> +            .size = fr->addr.size,
> +            .offset_within_address_space = int128_get64(fr->addr.start),
> +            .readonly = fr->readonly,
> +        };
> +        if (fr->dirty_log_mask && listener->log_stop) {
> +            listener->log_stop(listener, &section, 0, fr->dirty_log_mask);
> +        }
> +        if (listener->region_del) {
> +            listener->region_del(listener, &section);
> +        }
> +    }
> +    if (listener->commit) {
> +        listener->commit(listener);
> +    }
> +    flatview_unref(view);
> +}
> +
>  void memory_listener_register(MemoryListener *listener, AddressSpace *filter)
>  {
>      MemoryListener *other = NULL;
> @@ -2211,6 +2254,11 @@ void memory_listener_register(MemoryListener *listener, AddressSpace *filter)
>  
>  void memory_listener_unregister(MemoryListener *listener)
>  {
> +    AddressSpace *as;
> +
> +    QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> +        listener_del_address_space(listener, as);
> +    }
>      QTAILQ_REMOVE(&memory_listeners, listener, link);
>  }
>

On Thu, May 05, 2016 at 04:45:04PM -0600, Alex Williamson wrote:
> On Wed,  4 May 2016 16:52:14 +1000
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
> > When a new memory listener is registered, listener_add_address_space()
> > is called and which in turn calls region_add() callbacks of memory regions.
> > However when unregistering the memory listener, it is just removed from
> > the listening chain and no region_del() is called.
> > 
> > This adds listener_del_address_space() and uses it in
> > memory_listener_unregister(). listener_add_address_space() was used as
> > a template with the following changes:
> > s/log_global_start/log_global_stop/
> > s/log_start/log_stop/
> > s/region_add/region_del/
> > 
> > This will allow the following patches to add/remove DMA windows
> > dynamically from VFIO's PCI address space's region_add()/region_del().
> 
> Following patch 1 comments, it would be a bug if the kernel actually
> needed this to do cleanup, we must release everything if QEMU gets shot
> with a SIGKILL anyway.  So what does this cleanup facilitate in QEMU?
> Having QEMU trigger an unmap for each region_del is not going to be as
> efficient as just dropping the container and letting the kernel handle
> the cleanup all in one go.  Thanks,

So, what the kernel does is kind of a red herring, because that's only
relevant to the specific case of the VFIO listener, whereas this is a
change to the behaviour of all memory listeners.

It seems plausible that some memory listeners could have a legitimate
reason to want clean up region_del calls when unregistered.  But, we
know this could be expensive for other listeners, so I don't think we
should make that behaviour standard.

So I'd be thinking either a special unregister_with_delete() call, or
a standalone "delete all" helper function.

Assuming this is still needed at all, once the other changes to the
reference counting we've discussed have been done.