| Message ID | 4F4EE167.5000507@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
From: Wen Congyang <wency@cn.fujitsu.com> Subject: [RFC][PATCH 01/14 v7] Add API to create memory mapping list Date: Thu, 01 Mar 2012 10:39:35 +0800 > +static void memory_mapping_list_add_mapping_sorted(MemoryMappingList *list, > + MemoryMapping *mapping) > +void memory_mapping_list_add_sorted(MemoryMappingList *list, These functions not only sort but also merge elements of mapping list. Is there another name that represents what these are doing more properly? > + target_phys_addr_t phys_addr, > + target_phys_addr_t virt_addr, > + ram_addr_t length) > +{ > + MemoryMapping *memory_mapping, *last_mapping; > + > + if (QTAILQ_EMPTY(&list->head)) { > + create_new_memory_mapping(list, phys_addr, virt_addr, length); > + return; > + } > + > + last_mapping = list->last_mapping; > + if (last_mapping) { > + if ((phys_addr == (last_mapping->phys_addr + last_mapping->length)) && > + (virt_addr == (last_mapping->virt_addr + last_mapping->length))) { > + last_mapping->length += length; > + return; > + } > + } > + > + QTAILQ_FOREACH(memory_mapping, &list->head, next) { > + last_mapping = memory_mapping; > + if ((phys_addr == (last_mapping->phys_addr + last_mapping->length)) && > + (virt_addr == (last_mapping->virt_addr + last_mapping->length))) { > + last_mapping->length += length; > + list->last_mapping = last_mapping; > + return; > + } Please don't use a single variable in multiple meanings in the same function. It appears to me that the variable last_mapping is used as n-th entry connected to the list->head within this for loop. So this_mapping, for example, is reasonable rather than last_mapping. All use of last_mapping within the for loop can be replaced with memory_mapping, right? > + > + if (phys_addr + length < last_mapping->phys_addr) { > + /* create a new region before last_mapping */ > + break; > + } > + > + if (phys_addr >= (last_mapping->phys_addr + last_mapping->length)) { > + /* last_mapping does not contain this region */ > + continue; > + } > + > + if ((virt_addr - last_mapping->virt_addr) != > + (phys_addr - last_mapping->phys_addr)) { > + /* > + * last_mapping contains this region, but we cannot merge this > + * region into last_mapping. Try the next memory mapping. > + */ > + continue; > + } Does this condition means the current mapping and the last mapping are contiguous both phisically and viurtually? If so, it's better to write the condition so readers can understand that more easily. > + > + /* merge this region into last_mapping */ > + if (virt_addr < last_mapping->virt_addr) { > + last_mapping->length += last_mapping->virt_addr - virt_addr; > + last_mapping->virt_addr = virt_addr; > + } > + > + if ((virt_addr + length) > > + (last_mapping->virt_addr + last_mapping->length)) { > + last_mapping->length = virt_addr + length - last_mapping->virt_addr; > + } > + > + list->last_mapping = last_mapping; > + return; > + } > + > + /* this region can not be merged into any existed memory mapping. */ > + create_new_memory_mapping(list, phys_addr, virt_addr, length); > +} How about adding helper functions for expressing each conditionals? Just like below. Then I think the code gets more readable. bool mapping_proper_succeor(MemoryMapping *map, target_phys_addr_t phys_addr, target_virt_addr_t virt_addr); bool mapping_physically_contains(MemoryMapping *map, target_phys_addr_t phys_addr); bool mapping_physically_virtually_contiguous(MemoryMapping *map, target_phys_addr_t phys_addr, target_virt_addr_t virt_addr); void mapping_merge(MemoryMapping *map, target_phys_addr_t phys_addr, target_virt_addr_t virt_addr); I'm not confident of the naming, these are example, and assuming define all as static inline functions. Thanks. HATAYAMA, Daisuke
At 03/01/2012 04:33 PM, HATAYAMA Daisuke Wrote: > From: Wen Congyang <wency@cn.fujitsu.com> > Subject: [RFC][PATCH 01/14 v7] Add API to create memory mapping list > Date: Thu, 01 Mar 2012 10:39:35 +0800 > >> +static void memory_mapping_list_add_mapping_sorted(MemoryMappingList *list, >> + MemoryMapping *mapping) > >> +void memory_mapping_list_add_sorted(MemoryMappingList *list, > > These functions not only sort but also merge elements of mapping > list. Is there another name that represents what these are doing more > properly? > >> + target_phys_addr_t phys_addr, >> + target_phys_addr_t virt_addr, >> + ram_addr_t length) >> +{ >> + MemoryMapping *memory_mapping, *last_mapping; >> + >> + if (QTAILQ_EMPTY(&list->head)) { >> + create_new_memory_mapping(list, phys_addr, virt_addr, length); >> + return; >> + } >> + >> + last_mapping = list->last_mapping; >> + if (last_mapping) { >> + if ((phys_addr == (last_mapping->phys_addr + last_mapping->length)) && >> + (virt_addr == (last_mapping->virt_addr + last_mapping->length))) { >> + last_mapping->length += length; >> + return; >> + } >> + } >> + >> + QTAILQ_FOREACH(memory_mapping, &list->head, next) { >> + last_mapping = memory_mapping; >> + if ((phys_addr == (last_mapping->phys_addr + last_mapping->length)) && >> + (virt_addr == (last_mapping->virt_addr + last_mapping->length))) { >> + last_mapping->length += length; >> + list->last_mapping = last_mapping; >> + return; >> + } > > Please don't use a single variable in multiple meanings in the same > function. It appears to me that the variable last_mapping is used as > n-th entry connected to the list->head within this for loop. So > this_mapping, for example, is reasonable rather than last_mapping. All > use of last_mapping within the for loop can be replaced with > memory_mapping, right? OK > >> + >> + if (phys_addr + length < last_mapping->phys_addr) { >> + /* create a new region before last_mapping */ >> + break; >> + } >> + >> + if (phys_addr >= (last_mapping->phys_addr + last_mapping->length)) { >> + /* last_mapping does not contain this region */ >> + continue; >> + } >> + >> + if ((virt_addr - last_mapping->virt_addr) != >> + (phys_addr - last_mapping->phys_addr)) { >> + /* >> + * last_mapping contains this region, but we cannot merge this >> + * region into last_mapping. Try the next memory mapping. >> + */ >> + continue; >> + } > > Does this condition means the current mapping and the last mapping are > contiguous both phisically and viurtually? If so, it's better to write > the condition so readers can understand that more easily. current mapping and last mapping are always contiguous both phisically and virtually. > >> + >> + /* merge this region into last_mapping */ >> + if (virt_addr < last_mapping->virt_addr) { >> + last_mapping->length += last_mapping->virt_addr - virt_addr; >> + last_mapping->virt_addr = virt_addr; >> + } >> + >> + if ((virt_addr + length) > >> + (last_mapping->virt_addr + last_mapping->length)) { >> + last_mapping->length = virt_addr + length - last_mapping->virt_addr; >> + } >> + >> + list->last_mapping = last_mapping; >> + return; >> + } >> + >> + /* this region can not be merged into any existed memory mapping. */ >> + create_new_memory_mapping(list, phys_addr, virt_addr, length); >> +} > > How about adding helper functions for expressing each conditionals? > Just like below. Then I think the code gets more readable. > > bool mapping_proper_succeor(MemoryMapping *map, > target_phys_addr_t phys_addr, > target_virt_addr_t virt_addr); > bool mapping_physically_contains(MemoryMapping *map, > target_phys_addr_t phys_addr); > bool mapping_physically_virtually_contiguous(MemoryMapping *map, > target_phys_addr_t phys_addr, > target_virt_addr_t virt_addr); > void mapping_merge(MemoryMapping *map, target_phys_addr_t phys_addr, > target_virt_addr_t virt_addr); > > I'm not confident of the naming, these are example, and assuming > define all as static inline functions. Hmm, I think this inline functions make the code more readable. So I will modify my code. Thanks Wen Congyang > > Thanks. > HATAYAMA, Daisuke > >
diff --git a/Makefile.target b/Makefile.target index 68a5641..9227e4e 100644 --- a/Makefile.target +++ b/Makefile.target @@ -208,6 +208,7 @@ obj-$(CONFIG_KVM) += kvm.o kvm-all.o obj-$(CONFIG_NO_KVM) += kvm-stub.o obj-$(CONFIG_VGA) += vga.o obj-y += memory.o savevm.o +obj-y += memory_mapping.o LIBS+=-lz obj-i386-$(CONFIG_KVM) += hyperv.o diff --git a/memory_mapping.c b/memory_mapping.c new file mode 100644 index 0000000..84fb2c8 --- /dev/null +++ b/memory_mapping.c @@ -0,0 +1,134 @@ +/* + * QEMU memory mapping + * + * Copyright Fujitsu, Corp. 2011, 2012 + * + * Authors: + * Wen Congyang <wency@cn.fujitsu.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + * + */ + +#include "cpu.h" +#include "cpu-all.h" +#include "memory_mapping.h" + +static void memory_mapping_list_add_mapping_sorted(MemoryMappingList *list, + MemoryMapping *mapping) +{ + MemoryMapping *p; + + QTAILQ_FOREACH(p, &list->head, next) { + if (p->phys_addr >= mapping->phys_addr) { + QTAILQ_INSERT_BEFORE(p, mapping, next); + return; + } + } + QTAILQ_INSERT_TAIL(&list->head, mapping, next); +} + +static void create_new_memory_mapping(MemoryMappingList *list, + target_phys_addr_t phys_addr, + target_phys_addr_t virt_addr, + ram_addr_t length) +{ + MemoryMapping *memory_mapping; + + memory_mapping = g_malloc(sizeof(MemoryMapping)); + memory_mapping->phys_addr = phys_addr; + memory_mapping->virt_addr = virt_addr; + memory_mapping->length = length; + list->last_mapping = memory_mapping; + list->num++; + memory_mapping_list_add_mapping_sorted(list, memory_mapping); +} + +void memory_mapping_list_add_sorted(MemoryMappingList *list, + target_phys_addr_t phys_addr, + target_phys_addr_t virt_addr, + ram_addr_t length) +{ + MemoryMapping *memory_mapping, *last_mapping; + + if (QTAILQ_EMPTY(&list->head)) { + create_new_memory_mapping(list, phys_addr, virt_addr, length); + return; + } + + last_mapping = list->last_mapping; + if (last_mapping) { + if ((phys_addr == (last_mapping->phys_addr + last_mapping->length)) && + (virt_addr == (last_mapping->virt_addr + last_mapping->length))) { + last_mapping->length += length; + return; + } + } + + QTAILQ_FOREACH(memory_mapping, &list->head, next) { + last_mapping = memory_mapping; + if ((phys_addr == (last_mapping->phys_addr + last_mapping->length)) && + (virt_addr == (last_mapping->virt_addr + last_mapping->length))) { + last_mapping->length += length; + list->last_mapping = last_mapping; + return; + } + + if (phys_addr + length < last_mapping->phys_addr) { + /* create a new region before last_mapping */ + break; + } + + if (phys_addr >= (last_mapping->phys_addr + last_mapping->length)) { + /* last_mapping does not contain this region */ + continue; + } + + if ((virt_addr - last_mapping->virt_addr) != + (phys_addr - last_mapping->phys_addr)) { + /* + * last_mapping contains this region, but we cannot merge this + * region into last_mapping. Try the next memory mapping. + */ + continue; + } + + /* merge this region into last_mapping */ + if (virt_addr < last_mapping->virt_addr) { + last_mapping->length += last_mapping->virt_addr - virt_addr; + last_mapping->virt_addr = virt_addr; + } + + if ((virt_addr + length) > + (last_mapping->virt_addr + last_mapping->length)) { + last_mapping->length = virt_addr + length - last_mapping->virt_addr; + } + + list->last_mapping = last_mapping; + return; + } + + /* this region can not be merged into any existed memory mapping. */ + create_new_memory_mapping(list, phys_addr, virt_addr, length); +} + +void memory_mapping_list_free(MemoryMappingList *list) +{ + MemoryMapping *p, *q; + + QTAILQ_FOREACH_SAFE(p, &list->head, next, q) { + QTAILQ_REMOVE(&list->head, p, next); + g_free(p); + } + + list->num = 0; + list->last_mapping = NULL; +} + +void memory_mapping_list_init(MemoryMappingList *list) +{ + list->num = 0; + list->last_mapping = NULL; + QTAILQ_INIT(&list->head); +} diff --git a/memory_mapping.h b/memory_mapping.h new file mode 100644 index 0000000..633fcb9 --- /dev/null +++ b/memory_mapping.h @@ -0,0 +1,44 @@ +/* + * QEMU memory mapping + * + * Copyright Fujitsu, Corp. 2011, 2012 + * + * Authors: + * Wen Congyang <wency@cn.fujitsu.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + * + */ + +#ifndef MEMORY_MAPPING_H +#define MEMORY_MAPPING_H + +#include "qemu-queue.h" + +typedef struct MemoryMapping { + target_phys_addr_t phys_addr; + target_ulong virt_addr; + ram_addr_t length; + QTAILQ_ENTRY(MemoryMapping) next; +} MemoryMapping; + +typedef struct MemoryMappingList { + unsigned int num; + MemoryMapping *last_mapping; + QTAILQ_HEAD(, MemoryMapping) head; +} MemoryMappingList; + +/* + * add or merge the memory region into the memory mapping's list. The list is + * sorted by phys_addr. + */ +void memory_mapping_list_add_sorted(MemoryMappingList *list, + target_phys_addr_t phys_addr, + target_phys_addr_t virt_addr, + ram_addr_t length); + +void memory_mapping_list_free(MemoryMappingList *list); +void memory_mapping_list_init(MemoryMappingList *list); + +#endif
The memory mapping list stores virtual address and physical address mapping. The folloing patch will use this information to create PT_LOAD in the vmcore. Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> --- Makefile.target | 1 + memory_mapping.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ memory_mapping.h | 44 ++++++++++++++++++ 3 files changed, 179 insertions(+), 0 deletions(-) create mode 100644 memory_mapping.c create mode 100644 memory_mapping.h