Re: [Qemu-devel] [PATCH qom-cpu v4 13/18] memory_mapping: Cleanup qemu_get_guest_memory_mapping()

[Luiz Capitulino <lcapitulino@redhat.com>]

On Sun,  9 Jun 2013 18:10:42 +0200
Andreas Färber <afaerber@suse.de> wrote:

> We relied on the CPUClass::get_memory_mapping() implementation being a
> no-op when paging is disabled for that CPU. Therefore we can just as well
> iterate over all CPUs to retrieve mappings.
> 
> This allows to use qemu_for_each_cpu() instead of open-coding CPU loops.
> 
> Pass any Error out into dump_init() and have it actually stop on errors.
> Whether it is unsupported on a certain CPU can be checked by looking for
> a NULL CPUClass::get_memory_mapping field.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  dump.c                          |  7 +++++-
>  include/sysemu/memory_mapping.h |  8 +------
>  memory_mapping.c                | 52 ++++++++++++++++++++++++-----------------
>  3 files changed, 38 insertions(+), 29 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index c0d3da5..b44dafc 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -706,6 +706,7 @@ static int dump_init(DumpState *s, int fd, bool paging, bool has_filter,
>  {
>      CPUArchState *env;
>      int nr_cpus;
> +    Error *err = NULL;
>      int ret;
>  
>      if (runstate_is_running()) {
> @@ -756,7 +757,11 @@ static int dump_init(DumpState *s, int fd, bool paging, bool has_filter,
>      /* get memory mapping */
>      memory_mapping_list_init(&s->list);
>      if (paging) {
> -        qemu_get_guest_memory_mapping(&s->list);
> +        qemu_get_guest_memory_mapping(&s->list, &err);
> +        if (err != NULL) {
> +            error_propagate(errp, err);
> +            goto cleanup;
> +        }
>      } else {
>          qemu_get_guest_simple_memory_mapping(&s->list);
>      }
> diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
> index c47e6ee..6dfb68d 100644
> --- a/include/sysemu/memory_mapping.h
> +++ b/include/sysemu/memory_mapping.h
> @@ -45,13 +45,7 @@ void memory_mapping_list_free(MemoryMappingList *list);
>  
>  void memory_mapping_list_init(MemoryMappingList *list);
>  
> -/*
> - * Return value:
> - *    0: success
> - *   -1: failed
> - *   -2: unsupported
> - */
> -int qemu_get_guest_memory_mapping(MemoryMappingList *list);
> +void qemu_get_guest_memory_mapping(MemoryMappingList *list, Error **errp);
>  
>  /* get guest's memory mapping without do paging(virtual address is 0). */
>  void qemu_get_guest_simple_memory_mapping(MemoryMappingList *list);
> diff --git a/memory_mapping.c b/memory_mapping.c
> index 9bd24ce..a19be54 100644
> --- a/memory_mapping.c
> +++ b/memory_mapping.c
> @@ -165,36 +165,48 @@ void memory_mapping_list_init(MemoryMappingList *list)
>      QTAILQ_INIT(&list->head);
>  }
>  
> -static CPUArchState *find_paging_enabled_cpu(CPUArchState *start_cpu)
> +static void find_paging_enabled_cpu(CPUState *cpu, void *data)
>  {
> -    CPUArchState *env;
> +    bool *ret = data;
>  
> -    for (env = start_cpu; env != NULL; env = env->next_cpu) {
> -        if (cpu_paging_enabled(ENV_GET_CPU(env))) {
> -            return env;
> -        }
> +    if (*ret) {
> +        return;
>      }
> +    *ret = cpu_paging_enabled(cpu);
> +}
> +
> +typedef struct GetGuestMemoryMappingData {
> +    MemoryMappingList *list;
> +    Error *err;
> +} GetGuestMemoryMappingData;
> +
> +static void qemu_get_one_guest_memory_mapping(CPUState *cpu, void *data)
> +{
> +    GetGuestMemoryMappingData *s = data;
>  
> -    return NULL;
> +    if (s->err != NULL || !cpu_paging_enabled(cpu)) {
> +        return;
> +    }
> +    cpu_get_memory_mapping(cpu, s->list, &s->err);
>  }
>  
> -int qemu_get_guest_memory_mapping(MemoryMappingList *list)
> +void qemu_get_guest_memory_mapping(MemoryMappingList *list, Error **errp)
>  {
> -    CPUArchState *env, *first_paging_enabled_cpu;
> +    GetGuestMemoryMappingData s = {
> +        .list = list,
> +        .err = NULL,
> +    };
> +    bool paging_enabled = false;
>      RAMBlock *block;
>      ram_addr_t offset, length;
>  
> -    first_paging_enabled_cpu = find_paging_enabled_cpu(first_cpu);
> -    if (first_paging_enabled_cpu) {
> -        for (env = first_paging_enabled_cpu; env != NULL; env = env->next_cpu) {
> -            Error *err = NULL;
> -            cpu_get_memory_mapping(ENV_GET_CPU(env), list, &err);
> -            if (err) {
> -                error_free(err);
> -                return -1;
> -            }
> +    qemu_for_each_cpu(find_paging_enabled_cpu, &paging_enabled);
> +    if (paging_enabled) {
> +        qemu_for_each_cpu(qemu_get_one_guest_memory_mapping, &s);
> +        if (s.err != NULL) {
> +            error_propagate(errp, s.err);
>          }
> -        return 0;
> +        return;
>      }
>  
>      /*
> @@ -206,8 +218,6 @@ int qemu_get_guest_memory_mapping(MemoryMappingList *list)
>          length = block->length;
>          create_new_memory_mapping(list, offset, offset, length);
>      }
> -
> -    return 0;
>  }
>  
>  void qemu_get_guest_simple_memory_mapping(MemoryMappingList *list)