Re: [Qemu-devel] [RFC][PATCH 04/14 v7] Add API to get memory mapping

[HATAYAMA Daisuke]

From: Wen Congyang <address@hidden>
Subject: [RFC][PATCH 04/14 v7] Add API to get memory mapping
Date: Thu, 01 Mar 2012 10:43:13 +0800

> +int qemu_get_guest_memory_mapping(MemoryMappingList *list)
> +{
> +    CPUState *env;
> +    MemoryMapping *memory_mapping;
> +    RAMBlock *block;
> +    ram_addr_t offset, length;
> +    int ret;
> +
> +#if defined(CONFIG_HAVE_GET_MEMORY_MAPPING)
> +    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> +        ret = cpu_get_memory_mapping(list, env);
> +        if (ret < 0) {
> +            return -1;
> +        }
> +    }
> +#else
> +    return -2;
> +#endif
> +
> +    /* some memory may be not mapped, add them into memory mapping's list */

The part from here is logic fully for 2nd kernel? If so, I think it
better to describe why this addtional mapping is needed; we should
assume most people doesn't know kdump mechanism.

> +    QLIST_FOREACH(block, &ram_list.blocks, next) {
> +        offset = block->offset;
> +        length = block->length;
> +
> +        QTAILQ_FOREACH(memory_mapping, &list->head, next) {
> +            if (memory_mapping->phys_addr >= (offset + length)) {
> +                /*
> +                 * memory_mapping's list does not conatin the region
> +                 * [offset, offset+length)
> +                 */
> +                create_new_memory_mapping(list, offset, 0, length);
> +                length = 0;
> +                break;
> +            }
> +
> +            if ((memory_mapping->phys_addr + memory_mapping->length) <=
> +                offset) {
> +                continue;
> +            }
> +
> +            if (memory_mapping->phys_addr > offset) {
> +                /*
> +                 * memory_mapping's list does not conatin the region
> +                 * [offset, memory_mapping->phys_addr)
> +                 */
> +                create_new_memory_mapping(list, offset, 0,
> +                                          memory_mapping->phys_addr - 
> offset);
> +            }
> +
> +            if ((offset + length) <=
> +                (memory_mapping->phys_addr + memory_mapping->length)) {
> +                length = 0;
> +                break;
> +            }
> +            length -= memory_mapping->phys_addr + memory_mapping->length -
> +                      offset;
> +            offset = memory_mapping->phys_addr + memory_mapping->length;
> +        }
> +
> +        if (length > 0) {
> +            /*
> +             * memory_mapping's list does not conatin the region
> +             * [offset, memory_mapping->phys_addr)
> +             */
> +            create_new_memory_mapping(list, offset, 0, length);
> +        }
> +    }
> +
> +    return 0;
> +}

I think it more readable if shortening memory_mapping->phys_addr and
memmory_maping->length at the berinning of the innermost foreach loop.

  m_phys_addr = memory_mapping->phys_addr;
  m_length = memory_mapping->length;

Then, each conditionals becomes compact.

Thanks.
HATAYAMA, Daisuke