Re: [Qemu-devel] [PATCH qemu v16 02/19] memory: Call region_del() callbacks on memory listener unregistering

[Alex Williamson]

On Wed,  4 May 2016 16:52:14 +1000
Alexey Kardashevskiy <address@hidden> 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 <address@hidden>
> ---
>  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);
>  }
>