* [Qemu-devel] [RFC PATCH 0/6] RAM migration overhaul
@ 2010-06-08 19:14 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 19:14 UTC (permalink / raw)
To: qemu-devel, anthony; +Cc: chrisw, alex.williamson, kvm, quintela
As we discussed at the KVM developer call this morning, there are
a number of issues with how we migrate RAM in the presence of
hotplug, particularly:
- RAM allocated on the source may not match the target
- Abiguity of ram_addr_t between source and target
- Inability to remove RAM
- etc...
This series is a start at overhauling the infrastructure and at least
addresses the first two issues above. I could use some help working on
the freeing part, but I think this provides a solid basis for that.
I've only addressed the qemu-kvm/x86 bits, but the impact to other archs
and drivers is simply to add a unique name field when calling
qemu_ram_map/alloc().
Let me know what you think. Thanks,
Alex
---
Alex Williamson (6):
savevm: Use RAM blocks for basis of migration
savevm: Migrate RAM based on name/offset
Remove uses of ram.last_offset (aka last_ram_offset)
RAMBlock: Add a name field
ram_blocks: Convert to a QLIST
qemu_ram_alloc: Remove duplicate code
arch_init.c | 161 ++++++++++++++++++++++++++++++++++++++++--------
cpu-all.h | 28 ++++++--
cpu-common.h | 4 +
exec.c | 126 ++++++++++++++------------------------
hw/device-assignment.c | 8 ++
hw/pc.c | 8 +-
hw/pci.c | 5 +
hw/vga.c | 2 -
hw/vmware_vga.c | 2 -
vl.c | 2 -
10 files changed, 219 insertions(+), 127 deletions(-)
^ permalink raw reply [flat|nested] 94+ messages in thread* [RFC PATCH 1/6] qemu_ram_alloc: Remove duplicate code
2010-06-08 19:14 ` [Qemu-devel] " Alex Williamson
@ 2010-06-08 19:15 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 19:15 UTC (permalink / raw)
To: qemu-devel, anthony; +Cc: chrisw, alex.williamson, kvm, quintela
No reason not to call qemu_ram_map() once we have the allocation
and remove duplicate code.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
exec.c | 37 ++++++++++---------------------------
1 files changed, 10 insertions(+), 27 deletions(-)
diff --git a/exec.c b/exec.c
index 7b0e1c5..c60f9e7 100644
--- a/exec.c
+++ b/exec.c
@@ -2816,18 +2816,17 @@ ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
ram_addr_t qemu_ram_alloc(ram_addr_t size)
{
- RAMBlock *new_block;
+ void *host;
size = TARGET_PAGE_ALIGN(size);
- new_block = qemu_malloc(sizeof(*new_block));
if (mem_path) {
#if defined (__linux__) && !defined(TARGET_S390X)
- new_block->host = file_ram_alloc(size, mem_path);
- if (!new_block->host) {
- new_block->host = qemu_vmalloc(size);
+ host = file_ram_alloc(size, mem_path);
+ if (!host) {
+ host = qemu_vmalloc(size);
#ifdef MADV_MERGEABLE
- madvise(new_block->host, size, MADV_MERGEABLE);
+ madvise(host, size, MADV_MERGEABLE);
#endif
}
#else
@@ -2837,33 +2836,17 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
} else {
#if defined(TARGET_S390X) && defined(CONFIG_KVM)
/* XXX S390 KVM requires the topmost vma of the RAM to be < 256GB */
- new_block->host = mmap((void*)0x1000000, size,
- PROT_EXEC|PROT_READ|PROT_WRITE,
- MAP_SHARED | MAP_ANONYMOUS, -1, 0);
+ host = mmap((void*)0x1000000, size, PROT_EXEC|PROT_READ|PROT_WRITE,
+ MAP_SHARED | MAP_ANONYMOUS, -1, 0);
#else
- new_block->host = qemu_vmalloc(size);
+ host = qemu_vmalloc(size);
#endif
#ifdef MADV_MERGEABLE
- madvise(new_block->host, size, MADV_MERGEABLE);
+ madvise(host, size, MADV_MERGEABLE);
#endif
}
- new_block->offset = last_ram_offset;
- new_block->length = size;
-
- new_block->next = ram_blocks;
- ram_blocks = new_block;
-
- phys_ram_dirty = qemu_realloc(phys_ram_dirty,
- (last_ram_offset + size) >> TARGET_PAGE_BITS);
- memset(phys_ram_dirty + (last_ram_offset >> TARGET_PAGE_BITS),
- 0xff, size >> TARGET_PAGE_BITS);
-
- last_ram_offset += size;
- if (kvm_enabled())
- kvm_setup_guest_memory(new_block->host, size);
-
- return new_block->offset;
+ return qemu_ram_map(size, host);
}
void qemu_ram_free(ram_addr_t addr)
^ permalink raw reply related [flat|nested] 94+ messages in thread* [Qemu-devel] [RFC PATCH 1/6] qemu_ram_alloc: Remove duplicate code
@ 2010-06-08 19:15 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 19:15 UTC (permalink / raw)
To: qemu-devel, anthony; +Cc: chrisw, alex.williamson, kvm, quintela
No reason not to call qemu_ram_map() once we have the allocation
and remove duplicate code.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
exec.c | 37 ++++++++++---------------------------
1 files changed, 10 insertions(+), 27 deletions(-)
diff --git a/exec.c b/exec.c
index 7b0e1c5..c60f9e7 100644
--- a/exec.c
+++ b/exec.c
@@ -2816,18 +2816,17 @@ ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
ram_addr_t qemu_ram_alloc(ram_addr_t size)
{
- RAMBlock *new_block;
+ void *host;
size = TARGET_PAGE_ALIGN(size);
- new_block = qemu_malloc(sizeof(*new_block));
if (mem_path) {
#if defined (__linux__) && !defined(TARGET_S390X)
- new_block->host = file_ram_alloc(size, mem_path);
- if (!new_block->host) {
- new_block->host = qemu_vmalloc(size);
+ host = file_ram_alloc(size, mem_path);
+ if (!host) {
+ host = qemu_vmalloc(size);
#ifdef MADV_MERGEABLE
- madvise(new_block->host, size, MADV_MERGEABLE);
+ madvise(host, size, MADV_MERGEABLE);
#endif
}
#else
@@ -2837,33 +2836,17 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
} else {
#if defined(TARGET_S390X) && defined(CONFIG_KVM)
/* XXX S390 KVM requires the topmost vma of the RAM to be < 256GB */
- new_block->host = mmap((void*)0x1000000, size,
- PROT_EXEC|PROT_READ|PROT_WRITE,
- MAP_SHARED | MAP_ANONYMOUS, -1, 0);
+ host = mmap((void*)0x1000000, size, PROT_EXEC|PROT_READ|PROT_WRITE,
+ MAP_SHARED | MAP_ANONYMOUS, -1, 0);
#else
- new_block->host = qemu_vmalloc(size);
+ host = qemu_vmalloc(size);
#endif
#ifdef MADV_MERGEABLE
- madvise(new_block->host, size, MADV_MERGEABLE);
+ madvise(host, size, MADV_MERGEABLE);
#endif
}
- new_block->offset = last_ram_offset;
- new_block->length = size;
-
- new_block->next = ram_blocks;
- ram_blocks = new_block;
-
- phys_ram_dirty = qemu_realloc(phys_ram_dirty,
- (last_ram_offset + size) >> TARGET_PAGE_BITS);
- memset(phys_ram_dirty + (last_ram_offset >> TARGET_PAGE_BITS),
- 0xff, size >> TARGET_PAGE_BITS);
-
- last_ram_offset += size;
- if (kvm_enabled())
- kvm_setup_guest_memory(new_block->host, size);
-
- return new_block->offset;
+ return qemu_ram_map(size, host);
}
void qemu_ram_free(ram_addr_t addr)
^ permalink raw reply related [flat|nested] 94+ messages in thread* Re: [RFC PATCH 1/6] qemu_ram_alloc: Remove duplicate code
2010-06-08 19:15 ` [Qemu-devel] " Alex Williamson
@ 2010-06-08 21:00 ` Chris Wright
-1 siblings, 0 replies; 94+ messages in thread
From: Chris Wright @ 2010-06-08 21:00 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel, anthony, kvm, quintela, chrisw
* Alex Williamson (alex.williamson@redhat.com) wrote:
> No reason not to call qemu_ram_map() once we have the allocation
> and remove duplicate code.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
good cleanup regardless
Acked-by: Chris Wright <chrisw@redhat.com>
^ permalink raw reply [flat|nested] 94+ messages in thread
* [Qemu-devel] Re: [RFC PATCH 1/6] qemu_ram_alloc: Remove duplicate code
@ 2010-06-08 21:00 ` Chris Wright
0 siblings, 0 replies; 94+ messages in thread
From: Chris Wright @ 2010-06-08 21:00 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, quintela, qemu-devel, kvm
* Alex Williamson (alex.williamson@redhat.com) wrote:
> No reason not to call qemu_ram_map() once we have the allocation
> and remove duplicate code.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
good cleanup regardless
Acked-by: Chris Wright <chrisw@redhat.com>
^ permalink raw reply [flat|nested] 94+ messages in thread
* [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
2010-06-08 19:14 ` [Qemu-devel] " Alex Williamson
@ 2010-06-08 19:15 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 19:15 UTC (permalink / raw)
To: qemu-devel, anthony; +Cc: kvm, quintela, chrisw, alex.williamson
This makes the RAM block list easier to manipulate. Also incorporate
relevant variables into the RAMList struct.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
arch_init.c | 14 ++++++-----
cpu-all.h | 28 ++++++++++++++++-------
exec.c | 72 ++++++++++++++++++-----------------------------------------
3 files changed, 49 insertions(+), 65 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 8e849a8..43e42ba 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -110,7 +110,7 @@ static int ram_save_block(QEMUFile *f)
ram_addr_t addr = 0;
int bytes_sent = 0;
- while (addr < last_ram_offset) {
+ while (addr < ram.last_offset) {
if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
uint8_t *p;
@@ -133,7 +133,7 @@ static int ram_save_block(QEMUFile *f)
break;
}
addr += TARGET_PAGE_SIZE;
- current_addr = (saved_addr + addr) % last_ram_offset;
+ current_addr = (saved_addr + addr) % ram.last_offset;
}
return bytes_sent;
@@ -146,7 +146,7 @@ static ram_addr_t ram_save_remaining(void)
ram_addr_t addr;
ram_addr_t count = 0;
- for (addr = 0; addr < last_ram_offset; addr += TARGET_PAGE_SIZE) {
+ for (addr = 0; addr < ram.last_offset; addr += TARGET_PAGE_SIZE) {
if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
count++;
}
@@ -167,7 +167,7 @@ uint64_t ram_bytes_transferred(void)
uint64_t ram_bytes_total(void)
{
- return last_ram_offset;
+ return ram.last_offset;
}
int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
@@ -191,7 +191,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
bytes_transferred = 0;
/* Make sure all dirty bits are set */
- for (addr = 0; addr < last_ram_offset; addr += TARGET_PAGE_SIZE) {
+ for (addr = 0; addr < ram.last_offset; addr += TARGET_PAGE_SIZE) {
if (!cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
cpu_physical_memory_set_dirty(addr);
}
@@ -200,7 +200,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
/* Enable dirty memory tracking */
cpu_physical_memory_set_dirty_tracking(1);
- qemu_put_be64(f, last_ram_offset | RAM_SAVE_FLAG_MEM_SIZE);
+ qemu_put_be64(f, ram.last_offset | RAM_SAVE_FLAG_MEM_SIZE);
}
bytes_transferred_last = bytes_transferred;
@@ -259,7 +259,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
addr &= TARGET_PAGE_MASK;
if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
- if (addr != last_ram_offset) {
+ if (addr != ram.last_offset) {
return -EINVAL;
}
}
diff --git a/cpu-all.h b/cpu-all.h
index 77eaf85..458cb4b 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -859,9 +859,21 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
/* memory API */
extern int phys_ram_fd;
-extern uint8_t *phys_ram_dirty;
extern ram_addr_t ram_size;
-extern ram_addr_t last_ram_offset;
+
+typedef struct RAMBlock {
+ uint8_t *host;
+ ram_addr_t offset;
+ ram_addr_t length;
+ QLIST_ENTRY(RAMBlock) next;
+} RAMBlock;
+
+typedef struct RAMList {
+ uint8_t *phys_dirty;
+ ram_addr_t last_offset;
+ QLIST_HEAD(ram, RAMBlock) blocks;
+} RAMList;
+extern RAMList ram;
extern const char *mem_path;
extern int mem_prealloc;
@@ -891,29 +903,29 @@ extern int mem_prealloc;
/* read dirty bit (return 0 or 1) */
static inline int cpu_physical_memory_is_dirty(ram_addr_t addr)
{
- return phys_ram_dirty[addr >> TARGET_PAGE_BITS] == 0xff;
+ return ram.phys_dirty[addr >> TARGET_PAGE_BITS] == 0xff;
}
static inline int cpu_physical_memory_get_dirty_flags(ram_addr_t addr)
{
- return phys_ram_dirty[addr >> TARGET_PAGE_BITS];
+ return ram.phys_dirty[addr >> TARGET_PAGE_BITS];
}
static inline int cpu_physical_memory_get_dirty(ram_addr_t addr,
int dirty_flags)
{
- return phys_ram_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
+ return ram.phys_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
}
static inline void cpu_physical_memory_set_dirty(ram_addr_t addr)
{
- phys_ram_dirty[addr >> TARGET_PAGE_BITS] = 0xff;
+ ram.phys_dirty[addr >> TARGET_PAGE_BITS] = 0xff;
}
static inline int cpu_physical_memory_set_dirty_flags(ram_addr_t addr,
int dirty_flags)
{
- return phys_ram_dirty[addr >> TARGET_PAGE_BITS] |= dirty_flags;
+ return ram.phys_dirty[addr >> TARGET_PAGE_BITS] |= dirty_flags;
}
static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
@@ -925,7 +937,7 @@ static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
len = length >> TARGET_PAGE_BITS;
mask = ~dirty_flags;
- p = phys_ram_dirty + (start >> TARGET_PAGE_BITS);
+ p = ram.phys_dirty + (start >> TARGET_PAGE_BITS);
for (i = 0; i < len; i++) {
p[i] &= mask;
}
diff --git a/exec.c b/exec.c
index c60f9e7..d785de3 100644
--- a/exec.c
+++ b/exec.c
@@ -116,21 +116,9 @@ uint8_t *code_gen_ptr;
#if !defined(CONFIG_USER_ONLY)
int phys_ram_fd;
-uint8_t *phys_ram_dirty;
static int in_migration;
-typedef struct RAMBlock {
- uint8_t *host;
- ram_addr_t offset;
- ram_addr_t length;
- struct RAMBlock *next;
-} RAMBlock;
-
-static RAMBlock *ram_blocks;
-/* TODO: When we implement (and use) ram deallocation (e.g. for hotplug)
- then we can no longer assume contiguous ram offsets, and external uses
- of this variable will break. */
-ram_addr_t last_ram_offset;
+RAMList ram = { .blocks = QLIST_HEAD_INITIALIZER(ram) };
#endif
CPUState *first_cpu;
@@ -2795,18 +2783,17 @@ ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
new_block->host = host;
- new_block->offset = last_ram_offset;
+ new_block->offset = ram.last_offset;
new_block->length = size;
- new_block->next = ram_blocks;
- ram_blocks = new_block;
+ QLIST_INSERT_HEAD(&ram.blocks, new_block, next);
- phys_ram_dirty = qemu_realloc(phys_ram_dirty,
- (last_ram_offset + size) >> TARGET_PAGE_BITS);
- memset(phys_ram_dirty + (last_ram_offset >> TARGET_PAGE_BITS),
+ ram.phys_dirty = qemu_realloc(ram.phys_dirty,
+ (ram.last_offset + size) >> TARGET_PAGE_BITS);
+ memset(ram.phys_dirty + (ram.last_offset >> TARGET_PAGE_BITS),
0xff, size >> TARGET_PAGE_BITS);
- last_ram_offset += size;
+ ram.last_offset += size;
if (kvm_enabled())
kvm_setup_guest_memory(new_block->host, size);
@@ -2864,31 +2851,17 @@ void qemu_ram_free(ram_addr_t addr)
*/
void *qemu_get_ram_ptr(ram_addr_t addr)
{
- RAMBlock *prev;
- RAMBlock **prevp;
RAMBlock *block;
- prev = NULL;
- prevp = &ram_blocks;
- block = ram_blocks;
- while (block && (block->offset > addr
- || block->offset + block->length <= addr)) {
- if (prev)
- prevp = &prev->next;
- prev = block;
- block = block->next;
- }
- if (!block) {
- fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
- abort();
- }
- /* Move this entry to to start of the list. */
- if (prev) {
- prev->next = block->next;
- block->next = *prevp;
- *prevp = block;
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ if (addr - block->offset < block->length) {
+ QLIST_REMOVE(block, next);
+ QLIST_INSERT_HEAD(&ram.blocks, block, next);
+ return block->host + (addr - block->offset);
+ }
}
- return block->host + (addr - block->offset);
+
+ return NULL;
}
int do_qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
@@ -2896,15 +2869,14 @@ int do_qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
RAMBlock *block;
uint8_t *host = ptr;
- block = ram_blocks;
- while (block && (block->host > host
- || block->host + block->length <= host)) {
- block = block->next;
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ if (host - block->host < block->length) {
+ *ram_addr = block->offset + (host - block->host);
+ return 0;
+ }
}
- if (!block)
- return -1;
- *ram_addr = block->offset + (host - block->host);
- return 0;
+
+ return -1;
}
/* Some of the softmmu routines need to translate from a host pointer
^ permalink raw reply related [flat|nested] 94+ messages in thread* [Qemu-devel] [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
@ 2010-06-08 19:15 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 19:15 UTC (permalink / raw)
To: qemu-devel, anthony; +Cc: chrisw, alex.williamson, kvm, quintela
This makes the RAM block list easier to manipulate. Also incorporate
relevant variables into the RAMList struct.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
arch_init.c | 14 ++++++-----
cpu-all.h | 28 ++++++++++++++++-------
exec.c | 72 ++++++++++++++++++-----------------------------------------
3 files changed, 49 insertions(+), 65 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 8e849a8..43e42ba 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -110,7 +110,7 @@ static int ram_save_block(QEMUFile *f)
ram_addr_t addr = 0;
int bytes_sent = 0;
- while (addr < last_ram_offset) {
+ while (addr < ram.last_offset) {
if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
uint8_t *p;
@@ -133,7 +133,7 @@ static int ram_save_block(QEMUFile *f)
break;
}
addr += TARGET_PAGE_SIZE;
- current_addr = (saved_addr + addr) % last_ram_offset;
+ current_addr = (saved_addr + addr) % ram.last_offset;
}
return bytes_sent;
@@ -146,7 +146,7 @@ static ram_addr_t ram_save_remaining(void)
ram_addr_t addr;
ram_addr_t count = 0;
- for (addr = 0; addr < last_ram_offset; addr += TARGET_PAGE_SIZE) {
+ for (addr = 0; addr < ram.last_offset; addr += TARGET_PAGE_SIZE) {
if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
count++;
}
@@ -167,7 +167,7 @@ uint64_t ram_bytes_transferred(void)
uint64_t ram_bytes_total(void)
{
- return last_ram_offset;
+ return ram.last_offset;
}
int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
@@ -191,7 +191,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
bytes_transferred = 0;
/* Make sure all dirty bits are set */
- for (addr = 0; addr < last_ram_offset; addr += TARGET_PAGE_SIZE) {
+ for (addr = 0; addr < ram.last_offset; addr += TARGET_PAGE_SIZE) {
if (!cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
cpu_physical_memory_set_dirty(addr);
}
@@ -200,7 +200,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
/* Enable dirty memory tracking */
cpu_physical_memory_set_dirty_tracking(1);
- qemu_put_be64(f, last_ram_offset | RAM_SAVE_FLAG_MEM_SIZE);
+ qemu_put_be64(f, ram.last_offset | RAM_SAVE_FLAG_MEM_SIZE);
}
bytes_transferred_last = bytes_transferred;
@@ -259,7 +259,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
addr &= TARGET_PAGE_MASK;
if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
- if (addr != last_ram_offset) {
+ if (addr != ram.last_offset) {
return -EINVAL;
}
}
diff --git a/cpu-all.h b/cpu-all.h
index 77eaf85..458cb4b 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -859,9 +859,21 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
/* memory API */
extern int phys_ram_fd;
-extern uint8_t *phys_ram_dirty;
extern ram_addr_t ram_size;
-extern ram_addr_t last_ram_offset;
+
+typedef struct RAMBlock {
+ uint8_t *host;
+ ram_addr_t offset;
+ ram_addr_t length;
+ QLIST_ENTRY(RAMBlock) next;
+} RAMBlock;
+
+typedef struct RAMList {
+ uint8_t *phys_dirty;
+ ram_addr_t last_offset;
+ QLIST_HEAD(ram, RAMBlock) blocks;
+} RAMList;
+extern RAMList ram;
extern const char *mem_path;
extern int mem_prealloc;
@@ -891,29 +903,29 @@ extern int mem_prealloc;
/* read dirty bit (return 0 or 1) */
static inline int cpu_physical_memory_is_dirty(ram_addr_t addr)
{
- return phys_ram_dirty[addr >> TARGET_PAGE_BITS] == 0xff;
+ return ram.phys_dirty[addr >> TARGET_PAGE_BITS] == 0xff;
}
static inline int cpu_physical_memory_get_dirty_flags(ram_addr_t addr)
{
- return phys_ram_dirty[addr >> TARGET_PAGE_BITS];
+ return ram.phys_dirty[addr >> TARGET_PAGE_BITS];
}
static inline int cpu_physical_memory_get_dirty(ram_addr_t addr,
int dirty_flags)
{
- return phys_ram_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
+ return ram.phys_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
}
static inline void cpu_physical_memory_set_dirty(ram_addr_t addr)
{
- phys_ram_dirty[addr >> TARGET_PAGE_BITS] = 0xff;
+ ram.phys_dirty[addr >> TARGET_PAGE_BITS] = 0xff;
}
static inline int cpu_physical_memory_set_dirty_flags(ram_addr_t addr,
int dirty_flags)
{
- return phys_ram_dirty[addr >> TARGET_PAGE_BITS] |= dirty_flags;
+ return ram.phys_dirty[addr >> TARGET_PAGE_BITS] |= dirty_flags;
}
static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
@@ -925,7 +937,7 @@ static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
len = length >> TARGET_PAGE_BITS;
mask = ~dirty_flags;
- p = phys_ram_dirty + (start >> TARGET_PAGE_BITS);
+ p = ram.phys_dirty + (start >> TARGET_PAGE_BITS);
for (i = 0; i < len; i++) {
p[i] &= mask;
}
diff --git a/exec.c b/exec.c
index c60f9e7..d785de3 100644
--- a/exec.c
+++ b/exec.c
@@ -116,21 +116,9 @@ uint8_t *code_gen_ptr;
#if !defined(CONFIG_USER_ONLY)
int phys_ram_fd;
-uint8_t *phys_ram_dirty;
static int in_migration;
-typedef struct RAMBlock {
- uint8_t *host;
- ram_addr_t offset;
- ram_addr_t length;
- struct RAMBlock *next;
-} RAMBlock;
-
-static RAMBlock *ram_blocks;
-/* TODO: When we implement (and use) ram deallocation (e.g. for hotplug)
- then we can no longer assume contiguous ram offsets, and external uses
- of this variable will break. */
-ram_addr_t last_ram_offset;
+RAMList ram = { .blocks = QLIST_HEAD_INITIALIZER(ram) };
#endif
CPUState *first_cpu;
@@ -2795,18 +2783,17 @@ ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
new_block->host = host;
- new_block->offset = last_ram_offset;
+ new_block->offset = ram.last_offset;
new_block->length = size;
- new_block->next = ram_blocks;
- ram_blocks = new_block;
+ QLIST_INSERT_HEAD(&ram.blocks, new_block, next);
- phys_ram_dirty = qemu_realloc(phys_ram_dirty,
- (last_ram_offset + size) >> TARGET_PAGE_BITS);
- memset(phys_ram_dirty + (last_ram_offset >> TARGET_PAGE_BITS),
+ ram.phys_dirty = qemu_realloc(ram.phys_dirty,
+ (ram.last_offset + size) >> TARGET_PAGE_BITS);
+ memset(ram.phys_dirty + (ram.last_offset >> TARGET_PAGE_BITS),
0xff, size >> TARGET_PAGE_BITS);
- last_ram_offset += size;
+ ram.last_offset += size;
if (kvm_enabled())
kvm_setup_guest_memory(new_block->host, size);
@@ -2864,31 +2851,17 @@ void qemu_ram_free(ram_addr_t addr)
*/
void *qemu_get_ram_ptr(ram_addr_t addr)
{
- RAMBlock *prev;
- RAMBlock **prevp;
RAMBlock *block;
- prev = NULL;
- prevp = &ram_blocks;
- block = ram_blocks;
- while (block && (block->offset > addr
- || block->offset + block->length <= addr)) {
- if (prev)
- prevp = &prev->next;
- prev = block;
- block = block->next;
- }
- if (!block) {
- fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
- abort();
- }
- /* Move this entry to to start of the list. */
- if (prev) {
- prev->next = block->next;
- block->next = *prevp;
- *prevp = block;
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ if (addr - block->offset < block->length) {
+ QLIST_REMOVE(block, next);
+ QLIST_INSERT_HEAD(&ram.blocks, block, next);
+ return block->host + (addr - block->offset);
+ }
}
- return block->host + (addr - block->offset);
+
+ return NULL;
}
int do_qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
@@ -2896,15 +2869,14 @@ int do_qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
RAMBlock *block;
uint8_t *host = ptr;
- block = ram_blocks;
- while (block && (block->host > host
- || block->host + block->length <= host)) {
- block = block->next;
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ if (host - block->host < block->length) {
+ *ram_addr = block->offset + (host - block->host);
+ return 0;
+ }
}
- if (!block)
- return -1;
- *ram_addr = block->offset + (host - block->host);
- return 0;
+
+ return -1;
}
/* Some of the softmmu routines need to translate from a host pointer
^ permalink raw reply related [flat|nested] 94+ messages in thread* Re: [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
2010-06-08 19:15 ` [Qemu-devel] " Alex Williamson
@ 2010-06-08 21:26 ` Chris Wright
-1 siblings, 0 replies; 94+ messages in thread
From: Chris Wright @ 2010-06-08 21:26 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel, anthony, kvm, quintela, chrisw
* Alex Williamson (alex.williamson@redhat.com) wrote:
> extern int phys_ram_fd;
> -extern uint8_t *phys_ram_dirty;
> extern ram_addr_t ram_size;
> -extern ram_addr_t last_ram_offset;
> +
> +typedef struct RAMBlock {
> + uint8_t *host;
> + ram_addr_t offset;
> + ram_addr_t length;
> + QLIST_ENTRY(RAMBlock) next;
> +} RAMBlock;
> +
> +typedef struct RAMList {
> + uint8_t *phys_dirty;
> + ram_addr_t last_offset;
> + QLIST_HEAD(ram, RAMBlock) blocks;
> +} RAMList;
> +extern RAMList ram;
such a generic name for global namespace
> void *qemu_get_ram_ptr(ram_addr_t addr)
> {
> - RAMBlock *prev;
> - RAMBlock **prevp;
> RAMBlock *block;
>
> - prev = NULL;
> - prevp = &ram_blocks;
> - block = ram_blocks;
> - while (block && (block->offset > addr
> - || block->offset + block->length <= addr)) {
> - if (prev)
> - prevp = &prev->next;
> - prev = block;
> - block = block->next;
> - }
> - if (!block) {
> - fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
> - abort();
> - }
> - /* Move this entry to to start of the list. */
> - if (prev) {
> - prev->next = block->next;
> - block->next = *prevp;
> - *prevp = block;
> + QLIST_FOREACH(block, &ram.blocks, next) {
> + if (addr - block->offset < block->length) {
> + QLIST_REMOVE(block, next);
> + QLIST_INSERT_HEAD(&ram.blocks, block, next);
> + return block->host + (addr - block->offset);
> + }
> }
> - return block->host + (addr - block->offset);
> +
> + return NULL;
Why not preserve the error message and abort()? In error cases this
would now just segfault.
Minor nits aside, this too looks like a nice cleanup.
Acked-by: Chris Wright <chrisw@redhat.com>
^ permalink raw reply [flat|nested] 94+ messages in thread* [Qemu-devel] Re: [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
@ 2010-06-08 21:26 ` Chris Wright
0 siblings, 0 replies; 94+ messages in thread
From: Chris Wright @ 2010-06-08 21:26 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, quintela, qemu-devel, kvm
* Alex Williamson (alex.williamson@redhat.com) wrote:
> extern int phys_ram_fd;
> -extern uint8_t *phys_ram_dirty;
> extern ram_addr_t ram_size;
> -extern ram_addr_t last_ram_offset;
> +
> +typedef struct RAMBlock {
> + uint8_t *host;
> + ram_addr_t offset;
> + ram_addr_t length;
> + QLIST_ENTRY(RAMBlock) next;
> +} RAMBlock;
> +
> +typedef struct RAMList {
> + uint8_t *phys_dirty;
> + ram_addr_t last_offset;
> + QLIST_HEAD(ram, RAMBlock) blocks;
> +} RAMList;
> +extern RAMList ram;
such a generic name for global namespace
> void *qemu_get_ram_ptr(ram_addr_t addr)
> {
> - RAMBlock *prev;
> - RAMBlock **prevp;
> RAMBlock *block;
>
> - prev = NULL;
> - prevp = &ram_blocks;
> - block = ram_blocks;
> - while (block && (block->offset > addr
> - || block->offset + block->length <= addr)) {
> - if (prev)
> - prevp = &prev->next;
> - prev = block;
> - block = block->next;
> - }
> - if (!block) {
> - fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
> - abort();
> - }
> - /* Move this entry to to start of the list. */
> - if (prev) {
> - prev->next = block->next;
> - block->next = *prevp;
> - *prevp = block;
> + QLIST_FOREACH(block, &ram.blocks, next) {
> + if (addr - block->offset < block->length) {
> + QLIST_REMOVE(block, next);
> + QLIST_INSERT_HEAD(&ram.blocks, block, next);
> + return block->host + (addr - block->offset);
> + }
> }
> - return block->host + (addr - block->offset);
> +
> + return NULL;
Why not preserve the error message and abort()? In error cases this
would now just segfault.
Minor nits aside, this too looks like a nice cleanup.
Acked-by: Chris Wright <chrisw@redhat.com>
^ permalink raw reply [flat|nested] 94+ messages in thread* Re: [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
2010-06-08 21:26 ` [Qemu-devel] " Chris Wright
@ 2010-06-08 21:45 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 21:45 UTC (permalink / raw)
To: Chris Wright; +Cc: qemu-devel, anthony, kvm, quintela
On Tue, 2010-06-08 at 14:26 -0700, Chris Wright wrote:
> * Alex Williamson (alex.williamson@redhat.com) wrote:
> > extern int phys_ram_fd;
> > -extern uint8_t *phys_ram_dirty;
> > extern ram_addr_t ram_size;
> > -extern ram_addr_t last_ram_offset;
> > +
> > +typedef struct RAMBlock {
> > + uint8_t *host;
> > + ram_addr_t offset;
> > + ram_addr_t length;
> > + QLIST_ENTRY(RAMBlock) next;
> > +} RAMBlock;
> > +
> > +typedef struct RAMList {
> > + uint8_t *phys_dirty;
> > + ram_addr_t last_offset;
> > + QLIST_HEAD(ram, RAMBlock) blocks;
> > +} RAMList;
> > +extern RAMList ram;
>
> such a generic name for global namespace
Well it is _the_ ram, but yea... ;) Suggestions?
> > - if (!block) {
> > - fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
> > - abort();
> > - }
> > - /* Move this entry to to start of the list. */
> > - if (prev) {
> > - prev->next = block->next;
> > - block->next = *prevp;
> > - *prevp = block;
> > + QLIST_FOREACH(block, &ram.blocks, next) {
> > + if (addr - block->offset < block->length) {
> > + QLIST_REMOVE(block, next);
> > + QLIST_INSERT_HEAD(&ram.blocks, block, next);
> > + return block->host + (addr - block->offset);
> > + }
> > }
> > - return block->host + (addr - block->offset);
> > +
> > + return NULL;
>
> Why not preserve the error message and abort()? In error cases this
> would now just segfault.
Guess I was hoping the caller might do something smart, but I'll put it
back since that hasn't happened. Thanks,
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread* [Qemu-devel] Re: [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
@ 2010-06-08 21:45 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 21:45 UTC (permalink / raw)
To: Chris Wright; +Cc: quintela, qemu-devel, kvm
On Tue, 2010-06-08 at 14:26 -0700, Chris Wright wrote:
> * Alex Williamson (alex.williamson@redhat.com) wrote:
> > extern int phys_ram_fd;
> > -extern uint8_t *phys_ram_dirty;
> > extern ram_addr_t ram_size;
> > -extern ram_addr_t last_ram_offset;
> > +
> > +typedef struct RAMBlock {
> > + uint8_t *host;
> > + ram_addr_t offset;
> > + ram_addr_t length;
> > + QLIST_ENTRY(RAMBlock) next;
> > +} RAMBlock;
> > +
> > +typedef struct RAMList {
> > + uint8_t *phys_dirty;
> > + ram_addr_t last_offset;
> > + QLIST_HEAD(ram, RAMBlock) blocks;
> > +} RAMList;
> > +extern RAMList ram;
>
> such a generic name for global namespace
Well it is _the_ ram, but yea... ;) Suggestions?
> > - if (!block) {
> > - fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
> > - abort();
> > - }
> > - /* Move this entry to to start of the list. */
> > - if (prev) {
> > - prev->next = block->next;
> > - block->next = *prevp;
> > - *prevp = block;
> > + QLIST_FOREACH(block, &ram.blocks, next) {
> > + if (addr - block->offset < block->length) {
> > + QLIST_REMOVE(block, next);
> > + QLIST_INSERT_HEAD(&ram.blocks, block, next);
> > + return block->host + (addr - block->offset);
> > + }
> > }
> > - return block->host + (addr - block->offset);
> > +
> > + return NULL;
>
> Why not preserve the error message and abort()? In error cases this
> would now just segfault.
Guess I was hoping the caller might do something smart, but I'll put it
back since that hasn't happened. Thanks,
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread* Re: [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
2010-06-08 21:45 ` [Qemu-devel] " Alex Williamson
@ 2010-06-08 21:51 ` Chris Wright
-1 siblings, 0 replies; 94+ messages in thread
From: Chris Wright @ 2010-06-08 21:51 UTC (permalink / raw)
To: Alex Williamson; +Cc: Chris Wright, qemu-devel, anthony, kvm, quintela
* Alex Williamson (alex.williamson@redhat.com) wrote:
> On Tue, 2010-06-08 at 14:26 -0700, Chris Wright wrote:
> > * Alex Williamson (alex.williamson@redhat.com) wrote:
> > > extern int phys_ram_fd;
> > > -extern uint8_t *phys_ram_dirty;
> > > extern ram_addr_t ram_size;
> > > -extern ram_addr_t last_ram_offset;
> > > +
> > > +typedef struct RAMBlock {
> > > + uint8_t *host;
> > > + ram_addr_t offset;
> > > + ram_addr_t length;
> > > + QLIST_ENTRY(RAMBlock) next;
> > > +} RAMBlock;
> > > +
> > > +typedef struct RAMList {
> > > + uint8_t *phys_dirty;
> > > + ram_addr_t last_offset;
> > > + QLIST_HEAD(ram, RAMBlock) blocks;
> > > +} RAMList;
> > > +extern RAMList ram;
> >
> > such a generic name for global namespace
>
> Well it is _the_ ram, but yea... ;) Suggestions?
_the_ram
/me ducks
^ permalink raw reply [flat|nested] 94+ messages in thread* [Qemu-devel] Re: [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
@ 2010-06-08 21:51 ` Chris Wright
0 siblings, 0 replies; 94+ messages in thread
From: Chris Wright @ 2010-06-08 21:51 UTC (permalink / raw)
To: Alex Williamson; +Cc: Chris Wright, quintela, qemu-devel, kvm
* Alex Williamson (alex.williamson@redhat.com) wrote:
> On Tue, 2010-06-08 at 14:26 -0700, Chris Wright wrote:
> > * Alex Williamson (alex.williamson@redhat.com) wrote:
> > > extern int phys_ram_fd;
> > > -extern uint8_t *phys_ram_dirty;
> > > extern ram_addr_t ram_size;
> > > -extern ram_addr_t last_ram_offset;
> > > +
> > > +typedef struct RAMBlock {
> > > + uint8_t *host;
> > > + ram_addr_t offset;
> > > + ram_addr_t length;
> > > + QLIST_ENTRY(RAMBlock) next;
> > > +} RAMBlock;
> > > +
> > > +typedef struct RAMList {
> > > + uint8_t *phys_dirty;
> > > + ram_addr_t last_offset;
> > > + QLIST_HEAD(ram, RAMBlock) blocks;
> > > +} RAMList;
> > > +extern RAMList ram;
> >
> > such a generic name for global namespace
>
> Well it is _the_ ram, but yea... ;) Suggestions?
_the_ram
/me ducks
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
2010-06-08 21:45 ` [Qemu-devel] " Alex Williamson
@ 2010-06-09 8:19 ` Juan Quintela
-1 siblings, 0 replies; 94+ messages in thread
From: Juan Quintela @ 2010-06-09 8:19 UTC (permalink / raw)
To: Alex Williamson; +Cc: Chris Wright, qemu-devel, anthony, kvm
Alex Williamson <alex.williamson@redhat.com> wrote:
> On Tue, 2010-06-08 at 14:26 -0700, Chris Wright wrote:
>> * Alex Williamson (alex.williamson@redhat.com) wrote:
>> > extern int phys_ram_fd;
>> > -extern uint8_t *phys_ram_dirty;
>> > extern ram_addr_t ram_size;
>> > -extern ram_addr_t last_ram_offset;
>> > +
>> > +typedef struct RAMBlock {
>> > + uint8_t *host;
>> > + ram_addr_t offset;
>> > + ram_addr_t length;
>> > + QLIST_ENTRY(RAMBlock) next;
>> > +} RAMBlock;
>> > +
>> > +typedef struct RAMList {
>> > + uint8_t *phys_dirty;
>> > + ram_addr_t last_offset;
>> > + QLIST_HEAD(ram, RAMBlock) blocks;
>> > +} RAMList;
>> > +extern RAMList ram;
>>
>> such a generic name for global namespace
>
> Well it is _the_ ram, but yea... ;) Suggestions?
ram_block_list?
Later, Juan.
^ permalink raw reply [flat|nested] 94+ messages in thread* [Qemu-devel] Re: [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
@ 2010-06-09 8:19 ` Juan Quintela
0 siblings, 0 replies; 94+ messages in thread
From: Juan Quintela @ 2010-06-09 8:19 UTC (permalink / raw)
To: Alex Williamson; +Cc: Chris Wright, qemu-devel, kvm
Alex Williamson <alex.williamson@redhat.com> wrote:
> On Tue, 2010-06-08 at 14:26 -0700, Chris Wright wrote:
>> * Alex Williamson (alex.williamson@redhat.com) wrote:
>> > extern int phys_ram_fd;
>> > -extern uint8_t *phys_ram_dirty;
>> > extern ram_addr_t ram_size;
>> > -extern ram_addr_t last_ram_offset;
>> > +
>> > +typedef struct RAMBlock {
>> > + uint8_t *host;
>> > + ram_addr_t offset;
>> > + ram_addr_t length;
>> > + QLIST_ENTRY(RAMBlock) next;
>> > +} RAMBlock;
>> > +
>> > +typedef struct RAMList {
>> > + uint8_t *phys_dirty;
>> > + ram_addr_t last_offset;
>> > + QLIST_HEAD(ram, RAMBlock) blocks;
>> > +} RAMList;
>> > +extern RAMList ram;
>>
>> such a generic name for global namespace
>
> Well it is _the_ ram, but yea... ;) Suggestions?
ram_block_list?
Later, Juan.
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
2010-06-08 19:15 ` [Qemu-devel] " Alex Williamson
@ 2010-06-09 20:11 ` Cam Macdonell
-1 siblings, 0 replies; 94+ messages in thread
From: Cam Macdonell @ 2010-06-09 20:11 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel, anthony, chrisw, kvm, quintela
On Tue, Jun 8, 2010 at 1:15 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> This makes the RAM block list easier to manipulate. Also incorporate
> relevant variables into the RAMList struct.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
> arch_init.c | 14 ++++++-----
> cpu-all.h | 28 ++++++++++++++++-------
> exec.c | 72 ++++++++++++++++++-----------------------------------------
> 3 files changed, 49 insertions(+), 65 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 8e849a8..43e42ba 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -110,7 +110,7 @@ static int ram_save_block(QEMUFile *f)
> ram_addr_t addr = 0;
> int bytes_sent = 0;
>
> - while (addr < last_ram_offset) {
> + while (addr < ram.last_offset) {
> if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
> uint8_t *p;
>
> @@ -133,7 +133,7 @@ static int ram_save_block(QEMUFile *f)
> break;
> }
> addr += TARGET_PAGE_SIZE;
> - current_addr = (saved_addr + addr) % last_ram_offset;
> + current_addr = (saved_addr + addr) % ram.last_offset;
> }
>
> return bytes_sent;
> @@ -146,7 +146,7 @@ static ram_addr_t ram_save_remaining(void)
> ram_addr_t addr;
> ram_addr_t count = 0;
>
> - for (addr = 0; addr < last_ram_offset; addr += TARGET_PAGE_SIZE) {
> + for (addr = 0; addr < ram.last_offset; addr += TARGET_PAGE_SIZE) {
> if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
> count++;
> }
> @@ -167,7 +167,7 @@ uint64_t ram_bytes_transferred(void)
>
> uint64_t ram_bytes_total(void)
> {
> - return last_ram_offset;
> + return ram.last_offset;
> }
>
> int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
> @@ -191,7 +191,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
> bytes_transferred = 0;
>
> /* Make sure all dirty bits are set */
> - for (addr = 0; addr < last_ram_offset; addr += TARGET_PAGE_SIZE) {
> + for (addr = 0; addr < ram.last_offset; addr += TARGET_PAGE_SIZE) {
> if (!cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
> cpu_physical_memory_set_dirty(addr);
> }
> @@ -200,7 +200,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
> /* Enable dirty memory tracking */
> cpu_physical_memory_set_dirty_tracking(1);
>
> - qemu_put_be64(f, last_ram_offset | RAM_SAVE_FLAG_MEM_SIZE);
> + qemu_put_be64(f, ram.last_offset | RAM_SAVE_FLAG_MEM_SIZE);
> }
>
> bytes_transferred_last = bytes_transferred;
> @@ -259,7 +259,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> addr &= TARGET_PAGE_MASK;
>
> if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
> - if (addr != last_ram_offset) {
> + if (addr != ram.last_offset) {
> return -EINVAL;
> }
> }
> diff --git a/cpu-all.h b/cpu-all.h
> index 77eaf85..458cb4b 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -859,9 +859,21 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
> /* memory API */
>
> extern int phys_ram_fd;
> -extern uint8_t *phys_ram_dirty;
> extern ram_addr_t ram_size;
> -extern ram_addr_t last_ram_offset;
> +
> +typedef struct RAMBlock {
> + uint8_t *host;
> + ram_addr_t offset;
> + ram_addr_t length;
> + QLIST_ENTRY(RAMBlock) next;
> +} RAMBlock;
For my shared memory device I need a way to mark device memory as not
to be migrated. Can a flag to be added to this struct to accomplish
this?
> +
> +typedef struct RAMList {
> + uint8_t *phys_dirty;
> + ram_addr_t last_offset;
> + QLIST_HEAD(ram, RAMBlock) blocks;
> +} RAMList;
> +extern RAMList ram;
>
> extern const char *mem_path;
> extern int mem_prealloc;
> @@ -891,29 +903,29 @@ extern int mem_prealloc;
> /* read dirty bit (return 0 or 1) */
> static inline int cpu_physical_memory_is_dirty(ram_addr_t addr)
> {
> - return phys_ram_dirty[addr >> TARGET_PAGE_BITS] == 0xff;
> + return ram.phys_dirty[addr >> TARGET_PAGE_BITS] == 0xff;
> }
>
> static inline int cpu_physical_memory_get_dirty_flags(ram_addr_t addr)
> {
> - return phys_ram_dirty[addr >> TARGET_PAGE_BITS];
> + return ram.phys_dirty[addr >> TARGET_PAGE_BITS];
> }
>
> static inline int cpu_physical_memory_get_dirty(ram_addr_t addr,
> int dirty_flags)
> {
> - return phys_ram_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
> + return ram.phys_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
> }
>
> static inline void cpu_physical_memory_set_dirty(ram_addr_t addr)
> {
> - phys_ram_dirty[addr >> TARGET_PAGE_BITS] = 0xff;
> + ram.phys_dirty[addr >> TARGET_PAGE_BITS] = 0xff;
> }
>
> static inline int cpu_physical_memory_set_dirty_flags(ram_addr_t addr,
> int dirty_flags)
> {
> - return phys_ram_dirty[addr >> TARGET_PAGE_BITS] |= dirty_flags;
> + return ram.phys_dirty[addr >> TARGET_PAGE_BITS] |= dirty_flags;
> }
>
> static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
> @@ -925,7 +937,7 @@ static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
>
> len = length >> TARGET_PAGE_BITS;
> mask = ~dirty_flags;
> - p = phys_ram_dirty + (start >> TARGET_PAGE_BITS);
> + p = ram.phys_dirty + (start >> TARGET_PAGE_BITS);
> for (i = 0; i < len; i++) {
> p[i] &= mask;
> }
> diff --git a/exec.c b/exec.c
> index c60f9e7..d785de3 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -116,21 +116,9 @@ uint8_t *code_gen_ptr;
>
> #if !defined(CONFIG_USER_ONLY)
> int phys_ram_fd;
> -uint8_t *phys_ram_dirty;
> static int in_migration;
>
> -typedef struct RAMBlock {
> - uint8_t *host;
> - ram_addr_t offset;
> - ram_addr_t length;
> - struct RAMBlock *next;
> -} RAMBlock;
> -
> -static RAMBlock *ram_blocks;
> -/* TODO: When we implement (and use) ram deallocation (e.g. for hotplug)
> - then we can no longer assume contiguous ram offsets, and external uses
> - of this variable will break. */
> -ram_addr_t last_ram_offset;
> +RAMList ram = { .blocks = QLIST_HEAD_INITIALIZER(ram) };
> #endif
>
> CPUState *first_cpu;
> @@ -2795,18 +2783,17 @@ ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
>
> new_block->host = host;
>
> - new_block->offset = last_ram_offset;
> + new_block->offset = ram.last_offset;
> new_block->length = size;
>
> - new_block->next = ram_blocks;
> - ram_blocks = new_block;
> + QLIST_INSERT_HEAD(&ram.blocks, new_block, next);
>
> - phys_ram_dirty = qemu_realloc(phys_ram_dirty,
> - (last_ram_offset + size) >> TARGET_PAGE_BITS);
> - memset(phys_ram_dirty + (last_ram_offset >> TARGET_PAGE_BITS),
> + ram.phys_dirty = qemu_realloc(ram.phys_dirty,
> + (ram.last_offset + size) >> TARGET_PAGE_BITS);
> + memset(ram.phys_dirty + (ram.last_offset >> TARGET_PAGE_BITS),
> 0xff, size >> TARGET_PAGE_BITS);
>
> - last_ram_offset += size;
> + ram.last_offset += size;
>
> if (kvm_enabled())
> kvm_setup_guest_memory(new_block->host, size);
> @@ -2864,31 +2851,17 @@ void qemu_ram_free(ram_addr_t addr)
> */
> void *qemu_get_ram_ptr(ram_addr_t addr)
> {
> - RAMBlock *prev;
> - RAMBlock **prevp;
> RAMBlock *block;
>
> - prev = NULL;
> - prevp = &ram_blocks;
> - block = ram_blocks;
> - while (block && (block->offset > addr
> - || block->offset + block->length <= addr)) {
> - if (prev)
> - prevp = &prev->next;
> - prev = block;
> - block = block->next;
> - }
> - if (!block) {
> - fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
> - abort();
> - }
> - /* Move this entry to to start of the list. */
> - if (prev) {
> - prev->next = block->next;
> - block->next = *prevp;
> - *prevp = block;
> + QLIST_FOREACH(block, &ram.blocks, next) {
> + if (addr - block->offset < block->length) {
> + QLIST_REMOVE(block, next);
> + QLIST_INSERT_HEAD(&ram.blocks, block, next);
> + return block->host + (addr - block->offset);
> + }
> }
> - return block->host + (addr - block->offset);
> +
> + return NULL;
> }
>
> int do_qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
> @@ -2896,15 +2869,14 @@ int do_qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
> RAMBlock *block;
> uint8_t *host = ptr;
>
> - block = ram_blocks;
> - while (block && (block->host > host
> - || block->host + block->length <= host)) {
> - block = block->next;
> + QLIST_FOREACH(block, &ram.blocks, next) {
> + if (host - block->host < block->length) {
> + *ram_addr = block->offset + (host - block->host);
> + return 0;
> + }
> }
> - if (!block)
> - return -1;
> - *ram_addr = block->offset + (host - block->host);
> - return 0;
> +
> + return -1;
> }
>
> /* Some of the softmmu routines need to translate from a host pointer
>
>
^ permalink raw reply [flat|nested] 94+ messages in thread* Re: [Qemu-devel] [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
@ 2010-06-09 20:11 ` Cam Macdonell
0 siblings, 0 replies; 94+ messages in thread
From: Cam Macdonell @ 2010-06-09 20:11 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, quintela, qemu-devel, kvm
On Tue, Jun 8, 2010 at 1:15 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> This makes the RAM block list easier to manipulate. Also incorporate
> relevant variables into the RAMList struct.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
> arch_init.c | 14 ++++++-----
> cpu-all.h | 28 ++++++++++++++++-------
> exec.c | 72 ++++++++++++++++++-----------------------------------------
> 3 files changed, 49 insertions(+), 65 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 8e849a8..43e42ba 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -110,7 +110,7 @@ static int ram_save_block(QEMUFile *f)
> ram_addr_t addr = 0;
> int bytes_sent = 0;
>
> - while (addr < last_ram_offset) {
> + while (addr < ram.last_offset) {
> if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
> uint8_t *p;
>
> @@ -133,7 +133,7 @@ static int ram_save_block(QEMUFile *f)
> break;
> }
> addr += TARGET_PAGE_SIZE;
> - current_addr = (saved_addr + addr) % last_ram_offset;
> + current_addr = (saved_addr + addr) % ram.last_offset;
> }
>
> return bytes_sent;
> @@ -146,7 +146,7 @@ static ram_addr_t ram_save_remaining(void)
> ram_addr_t addr;
> ram_addr_t count = 0;
>
> - for (addr = 0; addr < last_ram_offset; addr += TARGET_PAGE_SIZE) {
> + for (addr = 0; addr < ram.last_offset; addr += TARGET_PAGE_SIZE) {
> if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
> count++;
> }
> @@ -167,7 +167,7 @@ uint64_t ram_bytes_transferred(void)
>
> uint64_t ram_bytes_total(void)
> {
> - return last_ram_offset;
> + return ram.last_offset;
> }
>
> int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
> @@ -191,7 +191,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
> bytes_transferred = 0;
>
> /* Make sure all dirty bits are set */
> - for (addr = 0; addr < last_ram_offset; addr += TARGET_PAGE_SIZE) {
> + for (addr = 0; addr < ram.last_offset; addr += TARGET_PAGE_SIZE) {
> if (!cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
> cpu_physical_memory_set_dirty(addr);
> }
> @@ -200,7 +200,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
> /* Enable dirty memory tracking */
> cpu_physical_memory_set_dirty_tracking(1);
>
> - qemu_put_be64(f, last_ram_offset | RAM_SAVE_FLAG_MEM_SIZE);
> + qemu_put_be64(f, ram.last_offset | RAM_SAVE_FLAG_MEM_SIZE);
> }
>
> bytes_transferred_last = bytes_transferred;
> @@ -259,7 +259,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> addr &= TARGET_PAGE_MASK;
>
> if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
> - if (addr != last_ram_offset) {
> + if (addr != ram.last_offset) {
> return -EINVAL;
> }
> }
> diff --git a/cpu-all.h b/cpu-all.h
> index 77eaf85..458cb4b 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -859,9 +859,21 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
> /* memory API */
>
> extern int phys_ram_fd;
> -extern uint8_t *phys_ram_dirty;
> extern ram_addr_t ram_size;
> -extern ram_addr_t last_ram_offset;
> +
> +typedef struct RAMBlock {
> + uint8_t *host;
> + ram_addr_t offset;
> + ram_addr_t length;
> + QLIST_ENTRY(RAMBlock) next;
> +} RAMBlock;
For my shared memory device I need a way to mark device memory as not
to be migrated. Can a flag to be added to this struct to accomplish
this?
> +
> +typedef struct RAMList {
> + uint8_t *phys_dirty;
> + ram_addr_t last_offset;
> + QLIST_HEAD(ram, RAMBlock) blocks;
> +} RAMList;
> +extern RAMList ram;
>
> extern const char *mem_path;
> extern int mem_prealloc;
> @@ -891,29 +903,29 @@ extern int mem_prealloc;
> /* read dirty bit (return 0 or 1) */
> static inline int cpu_physical_memory_is_dirty(ram_addr_t addr)
> {
> - return phys_ram_dirty[addr >> TARGET_PAGE_BITS] == 0xff;
> + return ram.phys_dirty[addr >> TARGET_PAGE_BITS] == 0xff;
> }
>
> static inline int cpu_physical_memory_get_dirty_flags(ram_addr_t addr)
> {
> - return phys_ram_dirty[addr >> TARGET_PAGE_BITS];
> + return ram.phys_dirty[addr >> TARGET_PAGE_BITS];
> }
>
> static inline int cpu_physical_memory_get_dirty(ram_addr_t addr,
> int dirty_flags)
> {
> - return phys_ram_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
> + return ram.phys_dirty[addr >> TARGET_PAGE_BITS] & dirty_flags;
> }
>
> static inline void cpu_physical_memory_set_dirty(ram_addr_t addr)
> {
> - phys_ram_dirty[addr >> TARGET_PAGE_BITS] = 0xff;
> + ram.phys_dirty[addr >> TARGET_PAGE_BITS] = 0xff;
> }
>
> static inline int cpu_physical_memory_set_dirty_flags(ram_addr_t addr,
> int dirty_flags)
> {
> - return phys_ram_dirty[addr >> TARGET_PAGE_BITS] |= dirty_flags;
> + return ram.phys_dirty[addr >> TARGET_PAGE_BITS] |= dirty_flags;
> }
>
> static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
> @@ -925,7 +937,7 @@ static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t start,
>
> len = length >> TARGET_PAGE_BITS;
> mask = ~dirty_flags;
> - p = phys_ram_dirty + (start >> TARGET_PAGE_BITS);
> + p = ram.phys_dirty + (start >> TARGET_PAGE_BITS);
> for (i = 0; i < len; i++) {
> p[i] &= mask;
> }
> diff --git a/exec.c b/exec.c
> index c60f9e7..d785de3 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -116,21 +116,9 @@ uint8_t *code_gen_ptr;
>
> #if !defined(CONFIG_USER_ONLY)
> int phys_ram_fd;
> -uint8_t *phys_ram_dirty;
> static int in_migration;
>
> -typedef struct RAMBlock {
> - uint8_t *host;
> - ram_addr_t offset;
> - ram_addr_t length;
> - struct RAMBlock *next;
> -} RAMBlock;
> -
> -static RAMBlock *ram_blocks;
> -/* TODO: When we implement (and use) ram deallocation (e.g. for hotplug)
> - then we can no longer assume contiguous ram offsets, and external uses
> - of this variable will break. */
> -ram_addr_t last_ram_offset;
> +RAMList ram = { .blocks = QLIST_HEAD_INITIALIZER(ram) };
> #endif
>
> CPUState *first_cpu;
> @@ -2795,18 +2783,17 @@ ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
>
> new_block->host = host;
>
> - new_block->offset = last_ram_offset;
> + new_block->offset = ram.last_offset;
> new_block->length = size;
>
> - new_block->next = ram_blocks;
> - ram_blocks = new_block;
> + QLIST_INSERT_HEAD(&ram.blocks, new_block, next);
>
> - phys_ram_dirty = qemu_realloc(phys_ram_dirty,
> - (last_ram_offset + size) >> TARGET_PAGE_BITS);
> - memset(phys_ram_dirty + (last_ram_offset >> TARGET_PAGE_BITS),
> + ram.phys_dirty = qemu_realloc(ram.phys_dirty,
> + (ram.last_offset + size) >> TARGET_PAGE_BITS);
> + memset(ram.phys_dirty + (ram.last_offset >> TARGET_PAGE_BITS),
> 0xff, size >> TARGET_PAGE_BITS);
>
> - last_ram_offset += size;
> + ram.last_offset += size;
>
> if (kvm_enabled())
> kvm_setup_guest_memory(new_block->host, size);
> @@ -2864,31 +2851,17 @@ void qemu_ram_free(ram_addr_t addr)
> */
> void *qemu_get_ram_ptr(ram_addr_t addr)
> {
> - RAMBlock *prev;
> - RAMBlock **prevp;
> RAMBlock *block;
>
> - prev = NULL;
> - prevp = &ram_blocks;
> - block = ram_blocks;
> - while (block && (block->offset > addr
> - || block->offset + block->length <= addr)) {
> - if (prev)
> - prevp = &prev->next;
> - prev = block;
> - block = block->next;
> - }
> - if (!block) {
> - fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
> - abort();
> - }
> - /* Move this entry to to start of the list. */
> - if (prev) {
> - prev->next = block->next;
> - block->next = *prevp;
> - *prevp = block;
> + QLIST_FOREACH(block, &ram.blocks, next) {
> + if (addr - block->offset < block->length) {
> + QLIST_REMOVE(block, next);
> + QLIST_INSERT_HEAD(&ram.blocks, block, next);
> + return block->host + (addr - block->offset);
> + }
> }
> - return block->host + (addr - block->offset);
> +
> + return NULL;
> }
>
> int do_qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
> @@ -2896,15 +2869,14 @@ int do_qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr)
> RAMBlock *block;
> uint8_t *host = ptr;
>
> - block = ram_blocks;
> - while (block && (block->host > host
> - || block->host + block->length <= host)) {
> - block = block->next;
> + QLIST_FOREACH(block, &ram.blocks, next) {
> + if (host - block->host < block->length) {
> + *ram_addr = block->offset + (host - block->host);
> + return 0;
> + }
> }
> - if (!block)
> - return -1;
> - *ram_addr = block->offset + (host - block->host);
> - return 0;
> +
> + return -1;
> }
>
> /* Some of the softmmu routines need to translate from a host pointer
>
>
^ permalink raw reply [flat|nested] 94+ messages in thread* Re: [Qemu-devel] [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
2010-06-09 20:11 ` Cam Macdonell
@ 2010-06-09 20:55 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-09 20:55 UTC (permalink / raw)
To: Cam Macdonell; +Cc: qemu-devel, anthony, chrisw, kvm, quintela
On Wed, 2010-06-09 at 14:11 -0600, Cam Macdonell wrote:
> On Tue, Jun 8, 2010 at 1:15 PM, Alex Williamson
> > diff --git a/cpu-all.h b/cpu-all.h
> > index 77eaf85..458cb4b 100644
> > --- a/cpu-all.h
> > +++ b/cpu-all.h
> > @@ -859,9 +859,21 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
> > /* memory API */
> >
> > extern int phys_ram_fd;
> > -extern uint8_t *phys_ram_dirty;
> > extern ram_addr_t ram_size;
> > -extern ram_addr_t last_ram_offset;
> > +
> > +typedef struct RAMBlock {
> > + uint8_t *host;
> > + ram_addr_t offset;
> > + ram_addr_t length;
> > + QLIST_ENTRY(RAMBlock) next;
> > +} RAMBlock;
>
> For my shared memory device I need a way to mark device memory as not
> to be migrated. Can a flag to be added to this struct to accomplish
> this?
Yep, it should be easy to skip blocks during migration based on a flag
here. I guess that probably means you'd also want a flag when you alloc
the block, so maybe it should be:
qemu_ram_alloc(DeviceState *dev, const char *name, ram_addr_t size, int flags)
Best to make that change now, then we can add it to the RAMBlock as we need.
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread* Re: [Qemu-devel] [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
@ 2010-06-09 20:55 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-09 20:55 UTC (permalink / raw)
To: Cam Macdonell; +Cc: chrisw, quintela, qemu-devel, kvm
On Wed, 2010-06-09 at 14:11 -0600, Cam Macdonell wrote:
> On Tue, Jun 8, 2010 at 1:15 PM, Alex Williamson
> > diff --git a/cpu-all.h b/cpu-all.h
> > index 77eaf85..458cb4b 100644
> > --- a/cpu-all.h
> > +++ b/cpu-all.h
> > @@ -859,9 +859,21 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
> > /* memory API */
> >
> > extern int phys_ram_fd;
> > -extern uint8_t *phys_ram_dirty;
> > extern ram_addr_t ram_size;
> > -extern ram_addr_t last_ram_offset;
> > +
> > +typedef struct RAMBlock {
> > + uint8_t *host;
> > + ram_addr_t offset;
> > + ram_addr_t length;
> > + QLIST_ENTRY(RAMBlock) next;
> > +} RAMBlock;
>
> For my shared memory device I need a way to mark device memory as not
> to be migrated. Can a flag to be added to this struct to accomplish
> this?
Yep, it should be easy to skip blocks during migration based on a flag
here. I guess that probably means you'd also want a flag when you alloc
the block, so maybe it should be:
qemu_ram_alloc(DeviceState *dev, const char *name, ram_addr_t size, int flags)
Best to make that change now, then we can add it to the RAMBlock as we need.
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread* Re: [Qemu-devel] [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
2010-06-09 20:55 ` Alex Williamson
@ 2010-06-09 21:12 ` Yoshiaki Tamura
-1 siblings, 0 replies; 94+ messages in thread
From: Yoshiaki Tamura @ 2010-06-09 21:12 UTC (permalink / raw)
To: Alex Williamson; +Cc: Cam Macdonell, qemu-devel, anthony, chrisw, kvm, quintela
2010/6/10 Alex Williamson <alex.williamson@redhat.com>:
> On Wed, 2010-06-09 at 14:11 -0600, Cam Macdonell wrote:
>> On Tue, Jun 8, 2010 at 1:15 PM, Alex Williamson
>> > diff --git a/cpu-all.h b/cpu-all.h
>> > index 77eaf85..458cb4b 100644
>> > --- a/cpu-all.h
>> > +++ b/cpu-all.h
>> > @@ -859,9 +859,21 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
>> > /* memory API */
>> >
>> > extern int phys_ram_fd;
>> > -extern uint8_t *phys_ram_dirty;
>> > extern ram_addr_t ram_size;
>> > -extern ram_addr_t last_ram_offset;
>> > +
>> > +typedef struct RAMBlock {
>> > + uint8_t *host;
>> > + ram_addr_t offset;
>> > + ram_addr_t length;
>> > + QLIST_ENTRY(RAMBlock) next;
>> > +} RAMBlock;
>>
>> For my shared memory device I need a way to mark device memory as not
>> to be migrated. Can a flag to be added to this struct to accomplish
>> this?
>
> Yep, it should be easy to skip blocks during migration based on a flag
> here. I guess that probably means you'd also want a flag when you alloc
> the block, so maybe it should be:
>
> qemu_ram_alloc(DeviceState *dev, const char *name, ram_addr_t size, int flags)
>
> Best to make that change now, then we can add it to the RAMBlock as we need.
I have a question that if a device doesn't want to be migrated, why is
it still connected during live migration? Shouldn't that be hot
unplugged before going into the live migration procedure?
Thanks,
Yoshi
>
> Alex
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 94+ messages in thread* Re: [Qemu-devel] [RFC PATCH 2/6] ram_blocks: Convert to a QLIST
@ 2010-06-09 21:12 ` Yoshiaki Tamura
0 siblings, 0 replies; 94+ messages in thread
From: Yoshiaki Tamura @ 2010-06-09 21:12 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, kvm, quintela, qemu-devel, Cam Macdonell
2010/6/10 Alex Williamson <alex.williamson@redhat.com>:
> On Wed, 2010-06-09 at 14:11 -0600, Cam Macdonell wrote:
>> On Tue, Jun 8, 2010 at 1:15 PM, Alex Williamson
>> > diff --git a/cpu-all.h b/cpu-all.h
>> > index 77eaf85..458cb4b 100644
>> > --- a/cpu-all.h
>> > +++ b/cpu-all.h
>> > @@ -859,9 +859,21 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr);
>> > /* memory API */
>> >
>> > extern int phys_ram_fd;
>> > -extern uint8_t *phys_ram_dirty;
>> > extern ram_addr_t ram_size;
>> > -extern ram_addr_t last_ram_offset;
>> > +
>> > +typedef struct RAMBlock {
>> > + uint8_t *host;
>> > + ram_addr_t offset;
>> > + ram_addr_t length;
>> > + QLIST_ENTRY(RAMBlock) next;
>> > +} RAMBlock;
>>
>> For my shared memory device I need a way to mark device memory as not
>> to be migrated. Can a flag to be added to this struct to accomplish
>> this?
>
> Yep, it should be easy to skip blocks during migration based on a flag
> here. I guess that probably means you'd also want a flag when you alloc
> the block, so maybe it should be:
>
> qemu_ram_alloc(DeviceState *dev, const char *name, ram_addr_t size, int flags)
>
> Best to make that change now, then we can add it to the RAMBlock as we need.
I have a question that if a device doesn't want to be migrated, why is
it still connected during live migration? Shouldn't that be hot
unplugged before going into the live migration procedure?
Thanks,
Yoshi
>
> Alex
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 94+ messages in thread
* [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-08 19:14 ` [Qemu-devel] " Alex Williamson
@ 2010-06-08 19:15 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 19:15 UTC (permalink / raw)
To: qemu-devel, anthony; +Cc: kvm, quintela, chrisw, alex.williamson
The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
let the caller specify a name so we can make a positive match.
Note, this only addresses the qemu-kvm callers so far.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
cpu-all.h | 1 +
cpu-common.h | 4 ++--
exec.c | 8 +++++---
hw/device-assignment.c | 8 ++++++--
hw/pc.c | 8 ++++----
hw/pci.c | 5 ++++-
hw/vga.c | 2 +-
hw/vmware_vga.c | 2 +-
8 files changed, 24 insertions(+), 14 deletions(-)
diff --git a/cpu-all.h b/cpu-all.h
index 458cb4b..a5b886a 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -865,6 +865,7 @@ typedef struct RAMBlock {
uint8_t *host;
ram_addr_t offset;
ram_addr_t length;
+ char name[64];
QLIST_ENTRY(RAMBlock) next;
} RAMBlock;
diff --git a/cpu-common.h b/cpu-common.h
index 4b0ba60..5b00544 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -40,8 +40,8 @@ static inline void cpu_register_physical_memory(target_phys_addr_t start_addr,
}
ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
-ram_addr_t qemu_ram_map(ram_addr_t size, void *host);
-ram_addr_t qemu_ram_alloc(ram_addr_t);
+ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host);
+ram_addr_t qemu_ram_alloc(const char *name, ram_addr_t);
void qemu_ram_free(ram_addr_t addr);
/* This should only be used for ram local to a device. */
void *qemu_get_ram_ptr(ram_addr_t addr);
diff --git a/exec.c b/exec.c
index d785de3..dd57515 100644
--- a/exec.c
+++ b/exec.c
@@ -2774,13 +2774,15 @@ static void *file_ram_alloc(ram_addr_t memory, const char *path)
}
#endif
-ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
+ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host)
{
RAMBlock *new_block;
size = TARGET_PAGE_ALIGN(size);
new_block = qemu_malloc(sizeof(*new_block));
+ // XXX check duplicates
+ snprintf(new_block->name, sizeof(new_block->name), "%s", strdup(name));
new_block->host = host;
new_block->offset = ram.last_offset;
@@ -2801,7 +2803,7 @@ ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
return new_block->offset;
}
-ram_addr_t qemu_ram_alloc(ram_addr_t size)
+ram_addr_t qemu_ram_alloc(const char *name, ram_addr_t size)
{
void *host;
@@ -2833,7 +2835,7 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
#endif
}
- return qemu_ram_map(size, host);
+ return qemu_ram_map(name, size, host);
}
void qemu_ram_free(ram_addr_t addr)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 2b963b5..1d631f6 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -569,9 +569,13 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
if (!slow_map) {
void *virtbase = pci_dev->v_addrs[i].u.r_virtbase;
+ char name[14];
- pci_dev->v_addrs[i].memory_index = qemu_ram_map(cur_region->size,
- virtbase);
+ snprintf(name, sizeof(name), "pci:%02x.%x.bar%d",
+ PCI_SLOT(pci_dev->dev.devfn),
+ PCI_FUNC(pci_dev->dev.devfn), i);
+ pci_dev->v_addrs[i].memory_index = qemu_ram_map(name,
+ cur_region->size, virtbase);
} else
pci_dev->v_addrs[i].memory_index = 0;
diff --git a/hw/pc.c b/hw/pc.c
index eae0db4..76be151 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -834,7 +834,7 @@ void pc_memory_init(ram_addr_t ram_size,
linux_boot = (kernel_filename != NULL);
/* allocate RAM */
- ram_addr = qemu_ram_alloc(below_4g_mem_size);
+ ram_addr = qemu_ram_alloc("ram.pc.lowmem", below_4g_mem_size);
cpu_register_physical_memory(0, 0xa0000, ram_addr);
cpu_register_physical_memory(0x100000,
below_4g_mem_size - 0x100000,
@@ -845,7 +845,7 @@ void pc_memory_init(ram_addr_t ram_size,
#if TARGET_PHYS_ADDR_BITS == 32
hw_error("To much RAM for 32-bit physical address");
#else
- ram_addr = qemu_ram_alloc(above_4g_mem_size);
+ ram_addr = qemu_ram_alloc("ram.pc.highmem", above_4g_mem_size);
cpu_register_physical_memory(0x100000000ULL,
above_4g_mem_size,
ram_addr);
@@ -866,7 +866,7 @@ void pc_memory_init(ram_addr_t ram_size,
(bios_size % 65536) != 0) {
goto bios_error;
}
- bios_offset = qemu_ram_alloc(bios_size);
+ bios_offset = qemu_ram_alloc("ram.pc.bios", bios_size);
ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size));
if (ret != 0) {
bios_error:
@@ -892,7 +892,7 @@ void pc_memory_init(ram_addr_t ram_size,
}
option_rom[nb_option_roms++] = qemu_strdup(VAPIC_FILENAME);
- option_rom_offset = qemu_ram_alloc(PC_ROM_SIZE);
+ option_rom_offset = qemu_ram_alloc("ram.pc.rom", PC_ROM_SIZE);
cpu_register_physical_memory(PC_ROM_MIN_VGA, PC_ROM_SIZE, option_rom_offset);
/* map all the bios at the top of memory */
diff --git a/hw/pci.c b/hw/pci.c
index f0b6790..b2632a8 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1889,6 +1889,7 @@ static int pci_add_option_rom(PCIDevice *pdev)
int size;
char *path;
void *ptr;
+ char name[13];
if (!pdev->romfile)
return 0;
@@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
size = 1 << qemu_fls(size);
}
- pdev->rom_offset = qemu_ram_alloc(size);
+ snprintf(name, sizeof(name), "pci:%02x.%x.rom",
+ PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+ pdev->rom_offset = qemu_ram_alloc(name, size);
ptr = qemu_get_ram_ptr(pdev->rom_offset);
load_image(path, ptr);
diff --git a/hw/vga.c b/hw/vga.c
index 0a535ae..47b800f 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2292,7 +2292,7 @@ void vga_common_init(VGACommonState *s, int vga_ram_size)
#else
s->is_vbe_vmstate = 0;
#endif
- s->vram_offset = qemu_ram_alloc(vga_ram_size);
+ s->vram_offset = qemu_ram_alloc("ram.vga.vram", vga_ram_size);
s->vram_ptr = qemu_get_ram_ptr(s->vram_offset);
s->vram_size = vga_ram_size;
s->get_bpp = vga_get_bpp;
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index bf2a699..9df8c08 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1164,7 +1164,7 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size)
s->fifo_size = SVGA_FIFO_SIZE;
- s->fifo_offset = qemu_ram_alloc(s->fifo_size);
+ s->fifo_offset = qemu_ram_alloc("ram.vmsvga.fifo", s->fifo_size);
s->fifo_ptr = qemu_get_ram_ptr(s->fifo_offset);
vga_common_init(&s->vga, vga_ram_size);
^ permalink raw reply related [flat|nested] 94+ messages in thread* [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-08 19:15 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 19:15 UTC (permalink / raw)
To: qemu-devel, anthony; +Cc: chrisw, alex.williamson, kvm, quintela
The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
let the caller specify a name so we can make a positive match.
Note, this only addresses the qemu-kvm callers so far.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
cpu-all.h | 1 +
cpu-common.h | 4 ++--
exec.c | 8 +++++---
hw/device-assignment.c | 8 ++++++--
hw/pc.c | 8 ++++----
hw/pci.c | 5 ++++-
hw/vga.c | 2 +-
hw/vmware_vga.c | 2 +-
8 files changed, 24 insertions(+), 14 deletions(-)
diff --git a/cpu-all.h b/cpu-all.h
index 458cb4b..a5b886a 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -865,6 +865,7 @@ typedef struct RAMBlock {
uint8_t *host;
ram_addr_t offset;
ram_addr_t length;
+ char name[64];
QLIST_ENTRY(RAMBlock) next;
} RAMBlock;
diff --git a/cpu-common.h b/cpu-common.h
index 4b0ba60..5b00544 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -40,8 +40,8 @@ static inline void cpu_register_physical_memory(target_phys_addr_t start_addr,
}
ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
-ram_addr_t qemu_ram_map(ram_addr_t size, void *host);
-ram_addr_t qemu_ram_alloc(ram_addr_t);
+ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host);
+ram_addr_t qemu_ram_alloc(const char *name, ram_addr_t);
void qemu_ram_free(ram_addr_t addr);
/* This should only be used for ram local to a device. */
void *qemu_get_ram_ptr(ram_addr_t addr);
diff --git a/exec.c b/exec.c
index d785de3..dd57515 100644
--- a/exec.c
+++ b/exec.c
@@ -2774,13 +2774,15 @@ static void *file_ram_alloc(ram_addr_t memory, const char *path)
}
#endif
-ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
+ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host)
{
RAMBlock *new_block;
size = TARGET_PAGE_ALIGN(size);
new_block = qemu_malloc(sizeof(*new_block));
+ // XXX check duplicates
+ snprintf(new_block->name, sizeof(new_block->name), "%s", strdup(name));
new_block->host = host;
new_block->offset = ram.last_offset;
@@ -2801,7 +2803,7 @@ ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
return new_block->offset;
}
-ram_addr_t qemu_ram_alloc(ram_addr_t size)
+ram_addr_t qemu_ram_alloc(const char *name, ram_addr_t size)
{
void *host;
@@ -2833,7 +2835,7 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
#endif
}
- return qemu_ram_map(size, host);
+ return qemu_ram_map(name, size, host);
}
void qemu_ram_free(ram_addr_t addr)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 2b963b5..1d631f6 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -569,9 +569,13 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
if (!slow_map) {
void *virtbase = pci_dev->v_addrs[i].u.r_virtbase;
+ char name[14];
- pci_dev->v_addrs[i].memory_index = qemu_ram_map(cur_region->size,
- virtbase);
+ snprintf(name, sizeof(name), "pci:%02x.%x.bar%d",
+ PCI_SLOT(pci_dev->dev.devfn),
+ PCI_FUNC(pci_dev->dev.devfn), i);
+ pci_dev->v_addrs[i].memory_index = qemu_ram_map(name,
+ cur_region->size, virtbase);
} else
pci_dev->v_addrs[i].memory_index = 0;
diff --git a/hw/pc.c b/hw/pc.c
index eae0db4..76be151 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -834,7 +834,7 @@ void pc_memory_init(ram_addr_t ram_size,
linux_boot = (kernel_filename != NULL);
/* allocate RAM */
- ram_addr = qemu_ram_alloc(below_4g_mem_size);
+ ram_addr = qemu_ram_alloc("ram.pc.lowmem", below_4g_mem_size);
cpu_register_physical_memory(0, 0xa0000, ram_addr);
cpu_register_physical_memory(0x100000,
below_4g_mem_size - 0x100000,
@@ -845,7 +845,7 @@ void pc_memory_init(ram_addr_t ram_size,
#if TARGET_PHYS_ADDR_BITS == 32
hw_error("To much RAM for 32-bit physical address");
#else
- ram_addr = qemu_ram_alloc(above_4g_mem_size);
+ ram_addr = qemu_ram_alloc("ram.pc.highmem", above_4g_mem_size);
cpu_register_physical_memory(0x100000000ULL,
above_4g_mem_size,
ram_addr);
@@ -866,7 +866,7 @@ void pc_memory_init(ram_addr_t ram_size,
(bios_size % 65536) != 0) {
goto bios_error;
}
- bios_offset = qemu_ram_alloc(bios_size);
+ bios_offset = qemu_ram_alloc("ram.pc.bios", bios_size);
ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size));
if (ret != 0) {
bios_error:
@@ -892,7 +892,7 @@ void pc_memory_init(ram_addr_t ram_size,
}
option_rom[nb_option_roms++] = qemu_strdup(VAPIC_FILENAME);
- option_rom_offset = qemu_ram_alloc(PC_ROM_SIZE);
+ option_rom_offset = qemu_ram_alloc("ram.pc.rom", PC_ROM_SIZE);
cpu_register_physical_memory(PC_ROM_MIN_VGA, PC_ROM_SIZE, option_rom_offset);
/* map all the bios at the top of memory */
diff --git a/hw/pci.c b/hw/pci.c
index f0b6790..b2632a8 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1889,6 +1889,7 @@ static int pci_add_option_rom(PCIDevice *pdev)
int size;
char *path;
void *ptr;
+ char name[13];
if (!pdev->romfile)
return 0;
@@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
size = 1 << qemu_fls(size);
}
- pdev->rom_offset = qemu_ram_alloc(size);
+ snprintf(name, sizeof(name), "pci:%02x.%x.rom",
+ PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+ pdev->rom_offset = qemu_ram_alloc(name, size);
ptr = qemu_get_ram_ptr(pdev->rom_offset);
load_image(path, ptr);
diff --git a/hw/vga.c b/hw/vga.c
index 0a535ae..47b800f 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2292,7 +2292,7 @@ void vga_common_init(VGACommonState *s, int vga_ram_size)
#else
s->is_vbe_vmstate = 0;
#endif
- s->vram_offset = qemu_ram_alloc(vga_ram_size);
+ s->vram_offset = qemu_ram_alloc("ram.vga.vram", vga_ram_size);
s->vram_ptr = qemu_get_ram_ptr(s->vram_offset);
s->vram_size = vga_ram_size;
s->get_bpp = vga_get_bpp;
diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
index bf2a699..9df8c08 100644
--- a/hw/vmware_vga.c
+++ b/hw/vmware_vga.c
@@ -1164,7 +1164,7 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size)
s->fifo_size = SVGA_FIFO_SIZE;
- s->fifo_offset = qemu_ram_alloc(s->fifo_size);
+ s->fifo_offset = qemu_ram_alloc("ram.vmsvga.fifo", s->fifo_size);
s->fifo_ptr = qemu_get_ram_ptr(s->fifo_offset);
vga_common_init(&s->vga, vga_ram_size);
^ permalink raw reply related [flat|nested] 94+ messages in thread* Re: [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-08 19:15 ` [Qemu-devel] " Alex Williamson
@ 2010-06-08 20:07 ` Anthony Liguori
-1 siblings, 0 replies; 94+ messages in thread
From: Anthony Liguori @ 2010-06-08 20:07 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel, kvm, quintela, chrisw
On 06/08/2010 02:15 PM, Alex Williamson wrote:
> The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
> let the caller specify a name so we can make a positive match.
>
> Note, this only addresses the qemu-kvm callers so far.
>
> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
> ---
>
> cpu-all.h | 1 +
> cpu-common.h | 4 ++--
> exec.c | 8 +++++---
> hw/device-assignment.c | 8 ++++++--
> hw/pc.c | 8 ++++----
> hw/pci.c | 5 ++++-
> hw/vga.c | 2 +-
> hw/vmware_vga.c | 2 +-
> 8 files changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/cpu-all.h b/cpu-all.h
> index 458cb4b..a5b886a 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -865,6 +865,7 @@ typedef struct RAMBlock {
> uint8_t *host;
> ram_addr_t offset;
> ram_addr_t length;
> + char name[64];
> QLIST_ENTRY(RAMBlock) next;
> } RAMBlock;
>
> diff --git a/cpu-common.h b/cpu-common.h
> index 4b0ba60..5b00544 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -40,8 +40,8 @@ static inline void cpu_register_physical_memory(target_phys_addr_t start_addr,
> }
>
> ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
> -ram_addr_t qemu_ram_map(ram_addr_t size, void *host);
> -ram_addr_t qemu_ram_alloc(ram_addr_t);
> +ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host);
> +ram_addr_t qemu_ram_alloc(const char *name, ram_addr_t);
> void qemu_ram_free(ram_addr_t addr);
> /* This should only be used for ram local to a device. */
> void *qemu_get_ram_ptr(ram_addr_t addr);
> diff --git a/exec.c b/exec.c
> index d785de3..dd57515 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2774,13 +2774,15 @@ static void *file_ram_alloc(ram_addr_t memory, const char *path)
> }
> #endif
>
> -ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
> +ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host)
> {
> RAMBlock *new_block;
>
> size = TARGET_PAGE_ALIGN(size);
> new_block = qemu_malloc(sizeof(*new_block));
>
> + // XXX check duplicates
> + snprintf(new_block->name, sizeof(new_block->name), "%s", strdup(name));
>
That strdup() is probably unintentional.
> new_block->host = host;
>
> new_block->offset = ram.last_offset;
> @@ -2801,7 +2803,7 @@ ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
> return new_block->offset;
> }
>
> -ram_addr_t qemu_ram_alloc(ram_addr_t size)
> +ram_addr_t qemu_ram_alloc(const char *name, ram_addr_t size)
> {
> void *host;
>
> @@ -2833,7 +2835,7 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
> #endif
> }
>
> - return qemu_ram_map(size, host);
> + return qemu_ram_map(name, size, host);
> }
>
> void qemu_ram_free(ram_addr_t addr)
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 2b963b5..1d631f6 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -569,9 +569,13 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
>
> if (!slow_map) {
> void *virtbase = pci_dev->v_addrs[i].u.r_virtbase;
> + char name[14];
>
> - pci_dev->v_addrs[i].memory_index = qemu_ram_map(cur_region->size,
> - virtbase);
> + snprintf(name, sizeof(name), "pci:%02x.%x.bar%d",
> + PCI_SLOT(pci_dev->dev.devfn),
> + PCI_FUNC(pci_dev->dev.devfn), i);
> + pci_dev->v_addrs[i].memory_index = qemu_ram_map(name,
> + cur_region->size, virtbase);
> } else
> pci_dev->v_addrs[i].memory_index = 0;
>
> diff --git a/hw/pc.c b/hw/pc.c
> index eae0db4..76be151 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -834,7 +834,7 @@ void pc_memory_init(ram_addr_t ram_size,
> linux_boot = (kernel_filename != NULL);
>
> /* allocate RAM */
> - ram_addr = qemu_ram_alloc(below_4g_mem_size);
> + ram_addr = qemu_ram_alloc("ram.pc.lowmem", below_4g_mem_size);
> cpu_register_physical_memory(0, 0xa0000, ram_addr);
> cpu_register_physical_memory(0x100000,
> below_4g_mem_size - 0x100000,
> @@ -845,7 +845,7 @@ void pc_memory_init(ram_addr_t ram_size,
> #if TARGET_PHYS_ADDR_BITS == 32
> hw_error("To much RAM for 32-bit physical address");
> #else
> - ram_addr = qemu_ram_alloc(above_4g_mem_size);
> + ram_addr = qemu_ram_alloc("ram.pc.highmem", above_4g_mem_size);
> cpu_register_physical_memory(0x100000000ULL,
> above_4g_mem_size,
> ram_addr);
> @@ -866,7 +866,7 @@ void pc_memory_init(ram_addr_t ram_size,
> (bios_size % 65536) != 0) {
> goto bios_error;
> }
> - bios_offset = qemu_ram_alloc(bios_size);
> + bios_offset = qemu_ram_alloc("ram.pc.bios", bios_size);
> ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size));
> if (ret != 0) {
> bios_error:
> @@ -892,7 +892,7 @@ void pc_memory_init(ram_addr_t ram_size,
> }
> option_rom[nb_option_roms++] = qemu_strdup(VAPIC_FILENAME);
>
> - option_rom_offset = qemu_ram_alloc(PC_ROM_SIZE);
> + option_rom_offset = qemu_ram_alloc("ram.pc.rom", PC_ROM_SIZE);
> cpu_register_physical_memory(PC_ROM_MIN_VGA, PC_ROM_SIZE, option_rom_offset);
>
> /* map all the bios at the top of memory */
> diff --git a/hw/pci.c b/hw/pci.c
> index f0b6790..b2632a8 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1889,6 +1889,7 @@ static int pci_add_option_rom(PCIDevice *pdev)
> int size;
> char *path;
> void *ptr;
> + char name[13];
>
> if (!pdev->romfile)
> return 0;
> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
> size = 1<< qemu_fls(size);
> }
>
> - pdev->rom_offset = qemu_ram_alloc(size);
> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> + pdev->rom_offset = qemu_ram_alloc(name, size);
>
> ptr = qemu_get_ram_ptr(pdev->rom_offset);
> load_image(path, ptr);
> diff --git a/hw/vga.c b/hw/vga.c
> index 0a535ae..47b800f 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -2292,7 +2292,7 @@ void vga_common_init(VGACommonState *s, int vga_ram_size)
> #else
> s->is_vbe_vmstate = 0;
> #endif
> - s->vram_offset = qemu_ram_alloc(vga_ram_size);
> + s->vram_offset = qemu_ram_alloc("ram.vga.vram", vga_ram_size);
> s->vram_ptr = qemu_get_ram_ptr(s->vram_offset);
> s->vram_size = vga_ram_size;
> s->get_bpp = vga_get_bpp;
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index bf2a699..9df8c08 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -1164,7 +1164,7 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size)
>
>
> s->fifo_size = SVGA_FIFO_SIZE;
> - s->fifo_offset = qemu_ram_alloc(s->fifo_size);
> + s->fifo_offset = qemu_ram_alloc("ram.vmsvga.fifo", s->fifo_size);
> s->fifo_ptr = qemu_get_ram_ptr(s->fifo_offset);
>
> vga_common_init(&s->vga, vga_ram_size);
>
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 94+ messages in thread* [Qemu-devel] Re: [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-08 20:07 ` Anthony Liguori
0 siblings, 0 replies; 94+ messages in thread
From: Anthony Liguori @ 2010-06-08 20:07 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, qemu-devel, kvm, quintela
On 06/08/2010 02:15 PM, Alex Williamson wrote:
> The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
> let the caller specify a name so we can make a positive match.
>
> Note, this only addresses the qemu-kvm callers so far.
>
> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
> ---
>
> cpu-all.h | 1 +
> cpu-common.h | 4 ++--
> exec.c | 8 +++++---
> hw/device-assignment.c | 8 ++++++--
> hw/pc.c | 8 ++++----
> hw/pci.c | 5 ++++-
> hw/vga.c | 2 +-
> hw/vmware_vga.c | 2 +-
> 8 files changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/cpu-all.h b/cpu-all.h
> index 458cb4b..a5b886a 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -865,6 +865,7 @@ typedef struct RAMBlock {
> uint8_t *host;
> ram_addr_t offset;
> ram_addr_t length;
> + char name[64];
> QLIST_ENTRY(RAMBlock) next;
> } RAMBlock;
>
> diff --git a/cpu-common.h b/cpu-common.h
> index 4b0ba60..5b00544 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -40,8 +40,8 @@ static inline void cpu_register_physical_memory(target_phys_addr_t start_addr,
> }
>
> ram_addr_t cpu_get_physical_page_desc(target_phys_addr_t addr);
> -ram_addr_t qemu_ram_map(ram_addr_t size, void *host);
> -ram_addr_t qemu_ram_alloc(ram_addr_t);
> +ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host);
> +ram_addr_t qemu_ram_alloc(const char *name, ram_addr_t);
> void qemu_ram_free(ram_addr_t addr);
> /* This should only be used for ram local to a device. */
> void *qemu_get_ram_ptr(ram_addr_t addr);
> diff --git a/exec.c b/exec.c
> index d785de3..dd57515 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2774,13 +2774,15 @@ static void *file_ram_alloc(ram_addr_t memory, const char *path)
> }
> #endif
>
> -ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
> +ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host)
> {
> RAMBlock *new_block;
>
> size = TARGET_PAGE_ALIGN(size);
> new_block = qemu_malloc(sizeof(*new_block));
>
> + // XXX check duplicates
> + snprintf(new_block->name, sizeof(new_block->name), "%s", strdup(name));
>
That strdup() is probably unintentional.
> new_block->host = host;
>
> new_block->offset = ram.last_offset;
> @@ -2801,7 +2803,7 @@ ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
> return new_block->offset;
> }
>
> -ram_addr_t qemu_ram_alloc(ram_addr_t size)
> +ram_addr_t qemu_ram_alloc(const char *name, ram_addr_t size)
> {
> void *host;
>
> @@ -2833,7 +2835,7 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size)
> #endif
> }
>
> - return qemu_ram_map(size, host);
> + return qemu_ram_map(name, size, host);
> }
>
> void qemu_ram_free(ram_addr_t addr)
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 2b963b5..1d631f6 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -569,9 +569,13 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
>
> if (!slow_map) {
> void *virtbase = pci_dev->v_addrs[i].u.r_virtbase;
> + char name[14];
>
> - pci_dev->v_addrs[i].memory_index = qemu_ram_map(cur_region->size,
> - virtbase);
> + snprintf(name, sizeof(name), "pci:%02x.%x.bar%d",
> + PCI_SLOT(pci_dev->dev.devfn),
> + PCI_FUNC(pci_dev->dev.devfn), i);
> + pci_dev->v_addrs[i].memory_index = qemu_ram_map(name,
> + cur_region->size, virtbase);
> } else
> pci_dev->v_addrs[i].memory_index = 0;
>
> diff --git a/hw/pc.c b/hw/pc.c
> index eae0db4..76be151 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -834,7 +834,7 @@ void pc_memory_init(ram_addr_t ram_size,
> linux_boot = (kernel_filename != NULL);
>
> /* allocate RAM */
> - ram_addr = qemu_ram_alloc(below_4g_mem_size);
> + ram_addr = qemu_ram_alloc("ram.pc.lowmem", below_4g_mem_size);
> cpu_register_physical_memory(0, 0xa0000, ram_addr);
> cpu_register_physical_memory(0x100000,
> below_4g_mem_size - 0x100000,
> @@ -845,7 +845,7 @@ void pc_memory_init(ram_addr_t ram_size,
> #if TARGET_PHYS_ADDR_BITS == 32
> hw_error("To much RAM for 32-bit physical address");
> #else
> - ram_addr = qemu_ram_alloc(above_4g_mem_size);
> + ram_addr = qemu_ram_alloc("ram.pc.highmem", above_4g_mem_size);
> cpu_register_physical_memory(0x100000000ULL,
> above_4g_mem_size,
> ram_addr);
> @@ -866,7 +866,7 @@ void pc_memory_init(ram_addr_t ram_size,
> (bios_size % 65536) != 0) {
> goto bios_error;
> }
> - bios_offset = qemu_ram_alloc(bios_size);
> + bios_offset = qemu_ram_alloc("ram.pc.bios", bios_size);
> ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size));
> if (ret != 0) {
> bios_error:
> @@ -892,7 +892,7 @@ void pc_memory_init(ram_addr_t ram_size,
> }
> option_rom[nb_option_roms++] = qemu_strdup(VAPIC_FILENAME);
>
> - option_rom_offset = qemu_ram_alloc(PC_ROM_SIZE);
> + option_rom_offset = qemu_ram_alloc("ram.pc.rom", PC_ROM_SIZE);
> cpu_register_physical_memory(PC_ROM_MIN_VGA, PC_ROM_SIZE, option_rom_offset);
>
> /* map all the bios at the top of memory */
> diff --git a/hw/pci.c b/hw/pci.c
> index f0b6790..b2632a8 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1889,6 +1889,7 @@ static int pci_add_option_rom(PCIDevice *pdev)
> int size;
> char *path;
> void *ptr;
> + char name[13];
>
> if (!pdev->romfile)
> return 0;
> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
> size = 1<< qemu_fls(size);
> }
>
> - pdev->rom_offset = qemu_ram_alloc(size);
> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> + pdev->rom_offset = qemu_ram_alloc(name, size);
>
> ptr = qemu_get_ram_ptr(pdev->rom_offset);
> load_image(path, ptr);
> diff --git a/hw/vga.c b/hw/vga.c
> index 0a535ae..47b800f 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -2292,7 +2292,7 @@ void vga_common_init(VGACommonState *s, int vga_ram_size)
> #else
> s->is_vbe_vmstate = 0;
> #endif
> - s->vram_offset = qemu_ram_alloc(vga_ram_size);
> + s->vram_offset = qemu_ram_alloc("ram.vga.vram", vga_ram_size);
> s->vram_ptr = qemu_get_ram_ptr(s->vram_offset);
> s->vram_size = vga_ram_size;
> s->get_bpp = vga_get_bpp;
> diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c
> index bf2a699..9df8c08 100644
> --- a/hw/vmware_vga.c
> +++ b/hw/vmware_vga.c
> @@ -1164,7 +1164,7 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size)
>
>
> s->fifo_size = SVGA_FIFO_SIZE;
> - s->fifo_offset = qemu_ram_alloc(s->fifo_size);
> + s->fifo_offset = qemu_ram_alloc("ram.vmsvga.fifo", s->fifo_size);
> s->fifo_ptr = qemu_get_ram_ptr(s->fifo_offset);
>
> vga_common_init(&s->vga, vga_ram_size);
>
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 94+ messages in thread* Re: [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-08 20:07 ` [Qemu-devel] " Anthony Liguori
@ 2010-06-08 20:09 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 20:09 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel, kvm, quintela, chrisw
On Tue, 2010-06-08 at 15:07 -0500, Anthony Liguori wrote:
> On 06/08/2010 02:15 PM, Alex Williamson wrote:
> > The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
> > let the caller specify a name so we can make a positive match.
> >
> > Note, this only addresses the qemu-kvm callers so far.
> >
> > -ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
> > +ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host)
> > {
> > RAMBlock *new_block;
> >
> > size = TARGET_PAGE_ALIGN(size);
> > new_block = qemu_malloc(sizeof(*new_block));
> >
> > + // XXX check duplicates
> > + snprintf(new_block->name, sizeof(new_block->name), "%s", strdup(name));
> >
>
> That strdup() is probably unintentional.
Yep, thanks.
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread* [Qemu-devel] Re: [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-08 20:09 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 20:09 UTC (permalink / raw)
To: Anthony Liguori; +Cc: chrisw, qemu-devel, kvm, quintela
On Tue, 2010-06-08 at 15:07 -0500, Anthony Liguori wrote:
> On 06/08/2010 02:15 PM, Alex Williamson wrote:
> > The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
> > let the caller specify a name so we can make a positive match.
> >
> > Note, this only addresses the qemu-kvm callers so far.
> >
> > -ram_addr_t qemu_ram_map(ram_addr_t size, void *host)
> > +ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host)
> > {
> > RAMBlock *new_block;
> >
> > size = TARGET_PAGE_ALIGN(size);
> > new_block = qemu_malloc(sizeof(*new_block));
> >
> > + // XXX check duplicates
> > + snprintf(new_block->name, sizeof(new_block->name), "%s", strdup(name));
> >
>
> That strdup() is probably unintentional.
Yep, thanks.
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-08 19:15 ` [Qemu-devel] " Alex Williamson
@ 2010-06-08 21:41 ` Chris Wright
-1 siblings, 0 replies; 94+ messages in thread
From: Chris Wright @ 2010-06-08 21:41 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel, anthony, kvm, quintela, chrisw
* Alex Williamson (alex.williamson@redhat.com) wrote:
> + // XXX check duplicates
Yes, definitely. You created a notion of a hierarchical namespace,
can this be formalized any more? Currently scattered...
> + char name[14];
> + snprintf(name, sizeof(name), "pci:%02x.%x.bar%d",
> + PCI_SLOT(pci_dev->dev.devfn),
> + PCI_FUNC(pci_dev->dev.devfn), i);
> + pci_dev->v_addrs[i].memory_index = qemu_ram_map(name,
> + cur_region->size, virtbase);
> + ram_addr = qemu_ram_alloc("ram.pc.lowmem", below_4g_mem_size);
> + ram_addr = qemu_ram_alloc("ram.pc.highmem", above_4g_mem_size);
> + bios_offset = qemu_ram_alloc("ram.pc.bios", bios_size);
> + option_rom_offset = qemu_ram_alloc("ram.pc.rom", PC_ROM_SIZE);
> + char name[13];
(13, 14...good to be consistent, maybe w/ helper like, pci_ram_alloc_{bar,rom})
> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> + pdev->rom_offset = qemu_ram_alloc(name, size);
> + s->vram_offset = qemu_ram_alloc("ram.vga.vram", vga_ram_size);
> + s->fifo_offset = qemu_ram_alloc("ram.vmsvga.fifo", s->fifo_size);
So far we have:
ram.
pc.
lowmem
highmem
bios
rom
vga.
vram
vmsvga.
fifo
pci.
D.F. (B:D.F?)
bar
rom
^ permalink raw reply [flat|nested] 94+ messages in thread* [Qemu-devel] Re: [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-08 21:41 ` Chris Wright
0 siblings, 0 replies; 94+ messages in thread
From: Chris Wright @ 2010-06-08 21:41 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, quintela, qemu-devel, kvm
* Alex Williamson (alex.williamson@redhat.com) wrote:
> + // XXX check duplicates
Yes, definitely. You created a notion of a hierarchical namespace,
can this be formalized any more? Currently scattered...
> + char name[14];
> + snprintf(name, sizeof(name), "pci:%02x.%x.bar%d",
> + PCI_SLOT(pci_dev->dev.devfn),
> + PCI_FUNC(pci_dev->dev.devfn), i);
> + pci_dev->v_addrs[i].memory_index = qemu_ram_map(name,
> + cur_region->size, virtbase);
> + ram_addr = qemu_ram_alloc("ram.pc.lowmem", below_4g_mem_size);
> + ram_addr = qemu_ram_alloc("ram.pc.highmem", above_4g_mem_size);
> + bios_offset = qemu_ram_alloc("ram.pc.bios", bios_size);
> + option_rom_offset = qemu_ram_alloc("ram.pc.rom", PC_ROM_SIZE);
> + char name[13];
(13, 14...good to be consistent, maybe w/ helper like, pci_ram_alloc_{bar,rom})
> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> + pdev->rom_offset = qemu_ram_alloc(name, size);
> + s->vram_offset = qemu_ram_alloc("ram.vga.vram", vga_ram_size);
> + s->fifo_offset = qemu_ram_alloc("ram.vmsvga.fifo", s->fifo_size);
So far we have:
ram.
pc.
lowmem
highmem
bios
rom
vga.
vram
vmsvga.
fifo
pci.
D.F. (B:D.F?)
bar
rom
^ permalink raw reply [flat|nested] 94+ messages in thread* Re: [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-08 21:41 ` [Qemu-devel] " Chris Wright
@ 2010-06-09 3:13 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-09 3:13 UTC (permalink / raw)
To: Chris Wright; +Cc: qemu-devel, anthony, kvm, quintela
On Tue, 2010-06-08 at 14:41 -0700, Chris Wright wrote:
> * Alex Williamson (alex.williamson@redhat.com) wrote:
> > + // XXX check duplicates
>
> Yes, definitely.
Yep, I was just thinking that without freeing, the uniqueness really
falls apart. If we hotplug a nic multiple times on the source, we're
going to have multiple pci:d.f for the slot. If we're lucky, they're
all for the same type of device and therefore the same ROM size and the
ordering will cause them to be merged into one on the target. That
requires a lot of luck though. Maybe once we have freeing we just blow
up and call it a device bug if it didn't free and tries to add more ram
with the same name.
> You created a notion of a hierarchical namespace,
> can this be formalized any more? Currently scattered...
Yeah, it's pretty haphazard, I probably haven't spent enough time
working out what makes sense across all devices. I started with
"ram.<device/driver>.<context>", where context was whatever I could
guess from the surrounding code. Then I jumped over to "pci:d.f" (which
should at least be "pci.s:b:d.f") just because I thought the PCI address
space already provided a nicely unique layout.
> > + char name[14];
> > + snprintf(name, sizeof(name), "pci:%02x.%x.bar%d",
> > + PCI_SLOT(pci_dev->dev.devfn),
> > + PCI_FUNC(pci_dev->dev.devfn), i);
> > + pci_dev->v_addrs[i].memory_index = qemu_ram_map(name,
> > + cur_region->size, virtbase);
> > + ram_addr = qemu_ram_alloc("ram.pc.lowmem", below_4g_mem_size);
> > + ram_addr = qemu_ram_alloc("ram.pc.highmem", above_4g_mem_size);
> > + bios_offset = qemu_ram_alloc("ram.pc.bios", bios_size);
> > + option_rom_offset = qemu_ram_alloc("ram.pc.rom", PC_ROM_SIZE);
> > + char name[13];
> (13, 14...good to be consistent, maybe w/ helper like, pci_ram_alloc_{bar,rom})
That probably makes sense.
> > + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> > + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> > + pdev->rom_offset = qemu_ram_alloc(name, size);
> > + s->vram_offset = qemu_ram_alloc("ram.vga.vram", vga_ram_size);
> > + s->fifo_offset = qemu_ram_alloc("ram.vmsvga.fifo", s->fifo_size);
>
> So far we have:
> ram.
> pc.
> lowmem
> highmem
> bios
> rom
> vga.
> vram
> vmsvga.
> fifo
So maybe we say "ram." = "platform devices", things that have single
instances or are easy to split up into fixed, unique names.
> pci.
> D.F. (B:D.F?)
> bar
> rom
This one is pretty obvious. If this is a reasonable starting point, I
can start hashing through the other archs and take a guess at what makes
sense. Thanks,
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread* [Qemu-devel] Re: [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-09 3:13 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-09 3:13 UTC (permalink / raw)
To: Chris Wright; +Cc: quintela, qemu-devel, kvm
On Tue, 2010-06-08 at 14:41 -0700, Chris Wright wrote:
> * Alex Williamson (alex.williamson@redhat.com) wrote:
> > + // XXX check duplicates
>
> Yes, definitely.
Yep, I was just thinking that without freeing, the uniqueness really
falls apart. If we hotplug a nic multiple times on the source, we're
going to have multiple pci:d.f for the slot. If we're lucky, they're
all for the same type of device and therefore the same ROM size and the
ordering will cause them to be merged into one on the target. That
requires a lot of luck though. Maybe once we have freeing we just blow
up and call it a device bug if it didn't free and tries to add more ram
with the same name.
> You created a notion of a hierarchical namespace,
> can this be formalized any more? Currently scattered...
Yeah, it's pretty haphazard, I probably haven't spent enough time
working out what makes sense across all devices. I started with
"ram.<device/driver>.<context>", where context was whatever I could
guess from the surrounding code. Then I jumped over to "pci:d.f" (which
should at least be "pci.s:b:d.f") just because I thought the PCI address
space already provided a nicely unique layout.
> > + char name[14];
> > + snprintf(name, sizeof(name), "pci:%02x.%x.bar%d",
> > + PCI_SLOT(pci_dev->dev.devfn),
> > + PCI_FUNC(pci_dev->dev.devfn), i);
> > + pci_dev->v_addrs[i].memory_index = qemu_ram_map(name,
> > + cur_region->size, virtbase);
> > + ram_addr = qemu_ram_alloc("ram.pc.lowmem", below_4g_mem_size);
> > + ram_addr = qemu_ram_alloc("ram.pc.highmem", above_4g_mem_size);
> > + bios_offset = qemu_ram_alloc("ram.pc.bios", bios_size);
> > + option_rom_offset = qemu_ram_alloc("ram.pc.rom", PC_ROM_SIZE);
> > + char name[13];
> (13, 14...good to be consistent, maybe w/ helper like, pci_ram_alloc_{bar,rom})
That probably makes sense.
> > + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> > + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> > + pdev->rom_offset = qemu_ram_alloc(name, size);
> > + s->vram_offset = qemu_ram_alloc("ram.vga.vram", vga_ram_size);
> > + s->fifo_offset = qemu_ram_alloc("ram.vmsvga.fifo", s->fifo_size);
>
> So far we have:
> ram.
> pc.
> lowmem
> highmem
> bios
> rom
> vga.
> vram
> vmsvga.
> fifo
So maybe we say "ram." = "platform devices", things that have single
instances or are easy to split up into fixed, unique names.
> pci.
> D.F. (B:D.F?)
> bar
> rom
This one is pretty obvious. If this is a reasonable starting point, I
can start hashing through the other archs and take a guess at what makes
sense. Thanks,
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-08 21:41 ` [Qemu-devel] " Chris Wright
@ 2010-06-09 11:55 ` Avi Kivity
-1 siblings, 0 replies; 94+ messages in thread
From: Avi Kivity @ 2010-06-09 11:55 UTC (permalink / raw)
To: Chris Wright; +Cc: Alex Williamson, qemu-devel, anthony, kvm, quintela
On 06/09/2010 12:41 AM, Chris Wright wrote:
> pci.
> D.F. (B:D.F?)
>
D:B:D.F
> bar
> rom
>
bar.n
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] Re: [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-08 21:41 ` [Qemu-devel] " Chris Wright
@ 2010-06-09 12:56 ` Paul Brook
-1 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-09 12:56 UTC (permalink / raw)
To: qemu-devel; +Cc: Chris Wright, Alex Williamson, quintela, kvm
> * Alex Williamson (alex.williamson@redhat.com) wrote:
> > + // XXX check duplicates
>
> Yes, definitely. You created a notion of a hierarchical namespace,
> can this be formalized any more?
We already have one: The qdev tree.
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] Re: [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-09 12:56 ` Paul Brook
0 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-09 12:56 UTC (permalink / raw)
To: qemu-devel; +Cc: Chris Wright, Alex Williamson, kvm, quintela
> * Alex Williamson (alex.williamson@redhat.com) wrote:
> > + // XXX check duplicates
>
> Yes, definitely. You created a notion of a hierarchical namespace,
> can this be formalized any more?
We already have one: The qdev tree.
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-08 19:15 ` [Qemu-devel] " Alex Williamson
@ 2010-06-09 2:30 ` Paul Brook
-1 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-09 2:30 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Williamson, anthony, chrisw, kvm, quintela
> The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
> let the caller specify a name so we can make a positive match.
> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> + pdev->rom_offset = qemu_ram_alloc(name, size);
This looks pretty bogus. It should be associated with the device, rather than
incorrectly trying to generate a globally unique name.
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-09 2:30 ` Paul Brook
0 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-09 2:30 UTC (permalink / raw)
To: qemu-devel; +Cc: chrisw, Alex Williamson, kvm, quintela
> The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
> let the caller specify a name so we can make a positive match.
> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> + pdev->rom_offset = qemu_ram_alloc(name, size);
This looks pretty bogus. It should be associated with the device, rather than
incorrectly trying to generate a globally unique name.
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-09 2:30 ` Paul Brook
@ 2010-06-09 2:40 ` Anthony Liguori
-1 siblings, 0 replies; 94+ messages in thread
From: Anthony Liguori @ 2010-06-09 2:40 UTC (permalink / raw)
To: Paul Brook; +Cc: qemu-devel, chrisw, Alex Williamson, kvm, quintela
On 06/08/2010 09:30 PM, Paul Brook wrote:
>> The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
>> let the caller specify a name so we can make a positive match.
>>
>
>> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
>> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
>> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>> + pdev->rom_offset = qemu_ram_alloc(name, size);
>>
> This looks pretty bogus. It should be associated with the device, rather than
> incorrectly trying to generate a globally unique name.
>
Not all ram is associated with a device.
For instance, the base ram for a guest.
Regards,
Anthony Liguori
> Paul
>
>
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-09 2:40 ` Anthony Liguori
0 siblings, 0 replies; 94+ messages in thread
From: Anthony Liguori @ 2010-06-09 2:40 UTC (permalink / raw)
To: Paul Brook; +Cc: chrisw, Alex Williamson, qemu-devel, kvm, quintela
On 06/08/2010 09:30 PM, Paul Brook wrote:
>> The offset given to a block created via qemu_ram_alloc/map() is arbitrary,
>> let the caller specify a name so we can make a positive match.
>>
>
>> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
>> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
>> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>> + pdev->rom_offset = qemu_ram_alloc(name, size);
>>
> This looks pretty bogus. It should be associated with the device, rather than
> incorrectly trying to generate a globally unique name.
>
Not all ram is associated with a device.
For instance, the base ram for a guest.
Regards,
Anthony Liguori
> Paul
>
>
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-09 2:40 ` Anthony Liguori
@ 2010-06-09 2:54 ` Paul Brook
-1 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-09 2:54 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel, chrisw, Alex Williamson, kvm, quintela
> On 06/08/2010 09:30 PM, Paul Brook wrote:
> >> The offset given to a block created via qemu_ram_alloc/map() is
> >> arbitrary, let the caller specify a name so we can make a positive
> >> match.
> >>
> >>
> >> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
> >> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> >> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> >> + pdev->rom_offset = qemu_ram_alloc(name, size);
> >
> > This looks pretty bogus. It should be associated with the device, rather
> > than incorrectly trying to generate a globally unique name.
>
> Not all ram is associated with a device.
Maybe not, but where it is we should be using that information.
Absolute minimum we should be using the existing qdev address rather than
inventing a new one. Duplicating this logic inside every device seems like a
bad idea so I suggest identifying ram blocks by a (name, device) pair. For now
we can allow a NULL device for system memory.
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-09 2:54 ` Paul Brook
0 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-09 2:54 UTC (permalink / raw)
To: Anthony Liguori; +Cc: chrisw, Alex Williamson, qemu-devel, kvm, quintela
> On 06/08/2010 09:30 PM, Paul Brook wrote:
> >> The offset given to a block created via qemu_ram_alloc/map() is
> >> arbitrary, let the caller specify a name so we can make a positive
> >> match.
> >>
> >>
> >> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
> >> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> >> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> >> + pdev->rom_offset = qemu_ram_alloc(name, size);
> >
> > This looks pretty bogus. It should be associated with the device, rather
> > than incorrectly trying to generate a globally unique name.
>
> Not all ram is associated with a device.
Maybe not, but where it is we should be using that information.
Absolute minimum we should be using the existing qdev address rather than
inventing a new one. Duplicating this logic inside every device seems like a
bad idea so I suggest identifying ram blocks by a (name, device) pair. For now
we can allow a NULL device for system memory.
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-09 2:54 ` Paul Brook
@ 2010-06-09 4:19 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-09 4:19 UTC (permalink / raw)
To: Paul Brook; +Cc: Anthony Liguori, qemu-devel, chrisw, kvm, quintela
On Wed, 2010-06-09 at 03:54 +0100, Paul Brook wrote:
> > On 06/08/2010 09:30 PM, Paul Brook wrote:
> > >> The offset given to a block created via qemu_ram_alloc/map() is
> > >> arbitrary, let the caller specify a name so we can make a positive
> > >> match.
> > >>
> > >>
> > >> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
> > >> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> > >> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> > >> + pdev->rom_offset = qemu_ram_alloc(name, size);
> > >
> > > This looks pretty bogus. It should be associated with the device, rather
> > > than incorrectly trying to generate a globally unique name.
> >
> > Not all ram is associated with a device.
>
> Maybe not, but where it is we should be using that information.
> Absolute minimum we should be using the existing qdev address rather than
> inventing a new one. Duplicating this logic inside every device seems like a
> bad idea so I suggest identifying ram blocks by a (name, device) pair. For now
> we can allow a NULL device for system memory.
I'm not sure if this is what you're after, but what if we change it to
something like:
+ snprintf(name, sizeof(name), "%s.%02x.%x.rom-%s",
+ pdev->bus->qbus.name, PCI_SLOT(pdev->devfn),
+ PCI_FUNC(pdev->devfn), pdev->qdev.info->name);
This gives us things like "pci.0.03.0.rom-rtl8139". For pci-assign, we
can expand on the host details, such as:
+ snprintf(name, sizeof(name),
+ "%s.%02x.%x.bar%d-%s(%04x:%02x:%02x.%d)",
+ pci_dev->dev.bus->qbus.name,
+ PCI_SLOT(pci_dev->dev.devfn),
+ PCI_FUNC(pci_dev->dev.devfn), i,
+ pci_dev->dev.qdev.info->name,
+ pci_dev->host.seg, pci_dev->host.bus,
+ pci_dev->host.dev, pci_dev->host.func);
Giving us "pci.0.04.0.bar0-pci-assign(0000:01:10.0)". Anywhere closer?
Thanks,
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-09 4:19 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-09 4:19 UTC (permalink / raw)
To: Paul Brook; +Cc: chrisw, quintela, qemu-devel, kvm
On Wed, 2010-06-09 at 03:54 +0100, Paul Brook wrote:
> > On 06/08/2010 09:30 PM, Paul Brook wrote:
> > >> The offset given to a block created via qemu_ram_alloc/map() is
> > >> arbitrary, let the caller specify a name so we can make a positive
> > >> match.
> > >>
> > >>
> > >> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
> > >> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
> > >> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> > >> + pdev->rom_offset = qemu_ram_alloc(name, size);
> > >
> > > This looks pretty bogus. It should be associated with the device, rather
> > > than incorrectly trying to generate a globally unique name.
> >
> > Not all ram is associated with a device.
>
> Maybe not, but where it is we should be using that information.
> Absolute minimum we should be using the existing qdev address rather than
> inventing a new one. Duplicating this logic inside every device seems like a
> bad idea so I suggest identifying ram blocks by a (name, device) pair. For now
> we can allow a NULL device for system memory.
I'm not sure if this is what you're after, but what if we change it to
something like:
+ snprintf(name, sizeof(name), "%s.%02x.%x.rom-%s",
+ pdev->bus->qbus.name, PCI_SLOT(pdev->devfn),
+ PCI_FUNC(pdev->devfn), pdev->qdev.info->name);
This gives us things like "pci.0.03.0.rom-rtl8139". For pci-assign, we
can expand on the host details, such as:
+ snprintf(name, sizeof(name),
+ "%s.%02x.%x.bar%d-%s(%04x:%02x:%02x.%d)",
+ pci_dev->dev.bus->qbus.name,
+ PCI_SLOT(pci_dev->dev.devfn),
+ PCI_FUNC(pci_dev->dev.devfn), i,
+ pci_dev->dev.qdev.info->name,
+ pci_dev->host.seg, pci_dev->host.bus,
+ pci_dev->host.dev, pci_dev->host.func);
Giving us "pci.0.04.0.bar0-pci-assign(0000:01:10.0)". Anywhere closer?
Thanks,
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-09 4:19 ` Alex Williamson
@ 2010-06-09 12:18 ` Paul Brook
-1 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-09 12:18 UTC (permalink / raw)
To: Alex Williamson; +Cc: Anthony Liguori, qemu-devel, chrisw, kvm, quintela
> > > Not all ram is associated with a device.
> >
> > Maybe not, but where it is we should be using that information.
> > Absolute minimum we should be using the existing qdev address rather than
> > inventing a new one. Duplicating this logic inside every device seems
> > like a bad idea so I suggest identifying ram blocks by a (name, device)
> > pair. For now we can allow a NULL device for system memory.
>
> I'm not sure if this is what you're after, but what if we change it to
> something like:
>
> + snprintf(name, sizeof(name), "%s.%02x.%x.rom-%s",
> + pdev->bus->qbus.name, PCI_SLOT(pdev->devfn),
> + PCI_FUNC(pdev->devfn), pdev->qdev.info->name);
>
> This gives us things like "pci.0.03.0.rom-rtl8139". For pci-assign, we
> can expand on the host details, such as:
> ..
> Giving us "pci.0.04.0.bar0-pci-assign(0000:01:10.0)". Anywhere closer?
Not really. This identifier is device and bus independent, which is why I
suggested passing the device to qemu_ram_alloc. This can then figure out how
to the identify the device. It should probably do this the same way that we
identify the saved state for the device. Currently I think this is an
arbitrary vmstate name/id, but I expect this to change to a qdev address
(e.g. /i440FX-pcihost/pci.0/_addr_04.0").
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-09 12:18 ` Paul Brook
0 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-09 12:18 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, quintela, qemu-devel, kvm
> > > Not all ram is associated with a device.
> >
> > Maybe not, but where it is we should be using that information.
> > Absolute minimum we should be using the existing qdev address rather than
> > inventing a new one. Duplicating this logic inside every device seems
> > like a bad idea so I suggest identifying ram blocks by a (name, device)
> > pair. For now we can allow a NULL device for system memory.
>
> I'm not sure if this is what you're after, but what if we change it to
> something like:
>
> + snprintf(name, sizeof(name), "%s.%02x.%x.rom-%s",
> + pdev->bus->qbus.name, PCI_SLOT(pdev->devfn),
> + PCI_FUNC(pdev->devfn), pdev->qdev.info->name);
>
> This gives us things like "pci.0.03.0.rom-rtl8139". For pci-assign, we
> can expand on the host details, such as:
> ..
> Giving us "pci.0.04.0.bar0-pci-assign(0000:01:10.0)". Anywhere closer?
Not really. This identifier is device and bus independent, which is why I
suggested passing the device to qemu_ram_alloc. This can then figure out how
to the identify the device. It should probably do this the same way that we
identify the saved state for the device. Currently I think this is an
arbitrary vmstate name/id, but I expect this to change to a qdev address
(e.g. /i440FX-pcihost/pci.0/_addr_04.0").
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-09 12:18 ` Paul Brook
@ 2010-06-09 16:37 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-09 16:37 UTC (permalink / raw)
To: Paul Brook; +Cc: Anthony Liguori, qemu-devel, chrisw, kvm, quintela
On Wed, 2010-06-09 at 13:18 +0100, Paul Brook wrote:
> > > > Not all ram is associated with a device.
> > >
> > > Maybe not, but where it is we should be using that information.
> > > Absolute minimum we should be using the existing qdev address rather than
> > > inventing a new one. Duplicating this logic inside every device seems
> > > like a bad idea so I suggest identifying ram blocks by a (name, device)
> > > pair. For now we can allow a NULL device for system memory.
> >
> > I'm not sure if this is what you're after, but what if we change it to
> > something like:
> >
> > + snprintf(name, sizeof(name), "%s.%02x.%x.rom-%s",
> > + pdev->bus->qbus.name, PCI_SLOT(pdev->devfn),
> > + PCI_FUNC(pdev->devfn), pdev->qdev.info->name);
> >
> > This gives us things like "pci.0.03.0.rom-rtl8139". For pci-assign, we
> > can expand on the host details, such as:
> > ..
> > Giving us "pci.0.04.0.bar0-pci-assign(0000:01:10.0)". Anywhere closer?
>
> Not really. This identifier is device and bus independent, which is why I
> suggested passing the device to qemu_ram_alloc. This can then figure out how
> to the identify the device. It should probably do this the same way that we
> identify the saved state for the device. Currently I think this is an
> arbitrary vmstate name/id, but I expect this to change to a qdev address
> (e.g. /i440FX-pcihost/pci.0/_addr_04.0").
Ok, that seems fairly reasonable, so from a device pointer we can get
something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add
something like ":rom" or ":bar.0" to it via an extra string.
qemu_ram_alloc(DeviceState *dev, const char *info, size)
Does a function already exist to print out a qdev address path? I don't
want to guess at something based on only this example. Is there a
reserved/unused character we can use to separate the qdev device string
from the supplied name? Thanks,
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-09 16:37 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-09 16:37 UTC (permalink / raw)
To: Paul Brook; +Cc: chrisw, quintela, qemu-devel, kvm
On Wed, 2010-06-09 at 13:18 +0100, Paul Brook wrote:
> > > > Not all ram is associated with a device.
> > >
> > > Maybe not, but where it is we should be using that information.
> > > Absolute minimum we should be using the existing qdev address rather than
> > > inventing a new one. Duplicating this logic inside every device seems
> > > like a bad idea so I suggest identifying ram blocks by a (name, device)
> > > pair. For now we can allow a NULL device for system memory.
> >
> > I'm not sure if this is what you're after, but what if we change it to
> > something like:
> >
> > + snprintf(name, sizeof(name), "%s.%02x.%x.rom-%s",
> > + pdev->bus->qbus.name, PCI_SLOT(pdev->devfn),
> > + PCI_FUNC(pdev->devfn), pdev->qdev.info->name);
> >
> > This gives us things like "pci.0.03.0.rom-rtl8139". For pci-assign, we
> > can expand on the host details, such as:
> > ..
> > Giving us "pci.0.04.0.bar0-pci-assign(0000:01:10.0)". Anywhere closer?
>
> Not really. This identifier is device and bus independent, which is why I
> suggested passing the device to qemu_ram_alloc. This can then figure out how
> to the identify the device. It should probably do this the same way that we
> identify the saved state for the device. Currently I think this is an
> arbitrary vmstate name/id, but I expect this to change to a qdev address
> (e.g. /i440FX-pcihost/pci.0/_addr_04.0").
Ok, that seems fairly reasonable, so from a device pointer we can get
something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add
something like ":rom" or ":bar.0" to it via an extra string.
qemu_ram_alloc(DeviceState *dev, const char *info, size)
Does a function already exist to print out a qdev address path? I don't
want to guess at something based on only this example. Is there a
reserved/unused character we can use to separate the qdev device string
from the supplied name? Thanks,
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-09 16:37 ` Alex Williamson
@ 2010-06-09 20:36 ` Paul Brook
-1 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-09 20:36 UTC (permalink / raw)
To: Alex Williamson; +Cc: Anthony Liguori, qemu-devel, chrisw, kvm, quintela
> > Not really. This identifier is device and bus independent, which is why
> > I suggested passing the device to qemu_ram_alloc. This can then figure
> > out how to the identify the device. It should probably do this the same
> > way that we identify the saved state for the device. Currently I think
> > this is an arbitrary vmstate name/id, but I expect this to change to a
> > qdev address (e.g. /i440FX-pcihost/pci.0/_addr_04.0").
>
> Ok, that seems fairly reasonable, so from a device pointer we can get
> something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add
> something like ":rom" or ":bar.0" to it via an extra string.
>
> qemu_ram_alloc(DeviceState *dev, const char *info, size)
Exactly - though personally I wouldn't call the second argument "info".
> Does a function already exist to print out a qdev address path?
No.
I may have been a bit misleading here. What we really want to do is use the
same matching algorithm as is used by the rest of the device state. Currently
this is a vmstate name and [arbitrary] numeric id. I don't remember whether
there's a convenient link from a device to its associated vmstate - if there
isn't there probably should be.
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-09 20:36 ` Paul Brook
0 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-09 20:36 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, quintela, qemu-devel, kvm
> > Not really. This identifier is device and bus independent, which is why
> > I suggested passing the device to qemu_ram_alloc. This can then figure
> > out how to the identify the device. It should probably do this the same
> > way that we identify the saved state for the device. Currently I think
> > this is an arbitrary vmstate name/id, but I expect this to change to a
> > qdev address (e.g. /i440FX-pcihost/pci.0/_addr_04.0").
>
> Ok, that seems fairly reasonable, so from a device pointer we can get
> something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add
> something like ":rom" or ":bar.0" to it via an extra string.
>
> qemu_ram_alloc(DeviceState *dev, const char *info, size)
Exactly - though personally I wouldn't call the second argument "info".
> Does a function already exist to print out a qdev address path?
No.
I may have been a bit misleading here. What we really want to do is use the
same matching algorithm as is used by the rest of the device state. Currently
this is a vmstate name and [arbitrary] numeric id. I don't remember whether
there's a convenient link from a device to its associated vmstate - if there
isn't there probably should be.
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-09 20:36 ` Paul Brook
@ 2010-06-10 8:23 ` Gerd Hoffmann
-1 siblings, 0 replies; 94+ messages in thread
From: Gerd Hoffmann @ 2010-06-10 8:23 UTC (permalink / raw)
To: Paul Brook
Cc: Alex Williamson, Anthony Liguori, qemu-devel, chrisw, kvm, quintela
> I may have been a bit misleading here. What we really want to do is use the
> same matching algorithm as is used by the rest of the device state. Currently
> this is a vmstate name and [arbitrary] numeric id. I don't remember whether
> there's a convenient link from a device to its associated vmstate - if there
> isn't there probably should be.
DeviceState->info->vmsd->name for the name.
Dunno about the numeric id, I think savevm.c doesn't export it.
cheers,
Gerd
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-10 8:23 ` Gerd Hoffmann
0 siblings, 0 replies; 94+ messages in thread
From: Gerd Hoffmann @ 2010-06-10 8:23 UTC (permalink / raw)
To: Paul Brook; +Cc: chrisw, kvm, quintela, qemu-devel, Alex Williamson
> I may have been a bit misleading here. What we really want to do is use the
> same matching algorithm as is used by the rest of the device state. Currently
> this is a vmstate name and [arbitrary] numeric id. I don't remember whether
> there's a convenient link from a device to its associated vmstate - if there
> isn't there probably should be.
DeviceState->info->vmsd->name for the name.
Dunno about the numeric id, I think savevm.c doesn't export it.
cheers,
Gerd
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-10 8:23 ` Gerd Hoffmann
@ 2010-06-10 14:33 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-10 14:33 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Paul Brook, Anthony Liguori, qemu-devel, chrisw, kvm, quintela
On Thu, 2010-06-10 at 10:23 +0200, Gerd Hoffmann wrote:
> > I may have been a bit misleading here. What we really want to do is use the
> > same matching algorithm as is used by the rest of the device state. Currently
> > this is a vmstate name and [arbitrary] numeric id. I don't remember whether
> > there's a convenient link from a device to its associated vmstate - if there
> > isn't there probably should be.
>
> DeviceState->info->vmsd->name for the name.
> Dunno about the numeric id, I think savevm.c doesn't export it.
Ok, we can certainly do <vmsd->name>.<vmsd->instance>\<driver string>.
It seems like this highlights a deficiency in the vmstate matching
though. If on the source we do:
> pci_add addr=4 nic model=e1000
> pci_add addr=3 nic model=e1000
Then we start the target, ordering the nics sequentially, are we going
to store the vmstate into the opposite nics? AIUI, libvirt does this
correctly today, but I don't like the idea of being required to remember
the history of a vm to migrate it.
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-10 14:33 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-10 14:33 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: chrisw, kvm, quintela, qemu-devel, Paul Brook
On Thu, 2010-06-10 at 10:23 +0200, Gerd Hoffmann wrote:
> > I may have been a bit misleading here. What we really want to do is use the
> > same matching algorithm as is used by the rest of the device state. Currently
> > this is a vmstate name and [arbitrary] numeric id. I don't remember whether
> > there's a convenient link from a device to its associated vmstate - if there
> > isn't there probably should be.
>
> DeviceState->info->vmsd->name for the name.
> Dunno about the numeric id, I think savevm.c doesn't export it.
Ok, we can certainly do <vmsd->name>.<vmsd->instance>\<driver string>.
It seems like this highlights a deficiency in the vmstate matching
though. If on the source we do:
> pci_add addr=4 nic model=e1000
> pci_add addr=3 nic model=e1000
Then we start the target, ordering the nics sequentially, are we going
to store the vmstate into the opposite nics? AIUI, libvirt does this
correctly today, but I don't like the idea of being required to remember
the history of a vm to migrate it.
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-10 14:33 ` Alex Williamson
@ 2010-06-10 14:49 ` Paul Brook
-1 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-10 14:49 UTC (permalink / raw)
To: Alex Williamson
Cc: Gerd Hoffmann, Anthony Liguori, qemu-devel, chrisw, kvm, quintela
> On Thu, 2010-06-10 at 10:23 +0200, Gerd Hoffmann wrote:
> > > I may have been a bit misleading here. What we really want to do is use
> > > the same matching algorithm as is used by the rest of the device
> > > state. Currently this is a vmstate name and [arbitrary] numeric id. I
> > > don't remember whether there's a convenient link from a device to its
> > > associated vmstate - if there isn't there probably should be.
> >
> > DeviceState->info->vmsd->name for the name.
> > Dunno about the numeric id, I think savevm.c doesn't export it.
>
> Ok, we can certainly do <vmsd->name>.<vmsd->instance>\<driver string>.
> It seems like this highlights a deficiency in the vmstate matching
Why are you forcing this to be a string?
> Then we start the target, ordering the nics sequentially, are we going
> to store the vmstate into the opposite nics?
That's a separate problem. As long as you use the same matching as for the
rest of the device state then it should just work. If it doesn't work then
migration is already broken so it doen't matter.
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-10 14:49 ` Paul Brook
0 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-10 14:49 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, kvm, quintela, qemu-devel, Gerd Hoffmann
> On Thu, 2010-06-10 at 10:23 +0200, Gerd Hoffmann wrote:
> > > I may have been a bit misleading here. What we really want to do is use
> > > the same matching algorithm as is used by the rest of the device
> > > state. Currently this is a vmstate name and [arbitrary] numeric id. I
> > > don't remember whether there's a convenient link from a device to its
> > > associated vmstate - if there isn't there probably should be.
> >
> > DeviceState->info->vmsd->name for the name.
> > Dunno about the numeric id, I think savevm.c doesn't export it.
>
> Ok, we can certainly do <vmsd->name>.<vmsd->instance>\<driver string>.
> It seems like this highlights a deficiency in the vmstate matching
Why are you forcing this to be a string?
> Then we start the target, ordering the nics sequentially, are we going
> to store the vmstate into the opposite nics?
That's a separate problem. As long as you use the same matching as for the
rest of the device state then it should just work. If it doesn't work then
migration is already broken so it doen't matter.
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-10 14:49 ` Paul Brook
@ 2010-06-10 15:21 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-10 15:21 UTC (permalink / raw)
To: Paul Brook
Cc: Gerd Hoffmann, Anthony Liguori, qemu-devel, chrisw, kvm, quintela
On Thu, 2010-06-10 at 15:49 +0100, Paul Brook wrote:
> > On Thu, 2010-06-10 at 10:23 +0200, Gerd Hoffmann wrote:
> > > > I may have been a bit misleading here. What we really want to do is use
> > > > the same matching algorithm as is used by the rest of the device
> > > > state. Currently this is a vmstate name and [arbitrary] numeric id. I
> > > > don't remember whether there's a convenient link from a device to its
> > > > associated vmstate - if there isn't there probably should be.
> > >
> > > DeviceState->info->vmsd->name for the name.
> > > Dunno about the numeric id, I think savevm.c doesn't export it.
> >
> > Ok, we can certainly do <vmsd->name>.<vmsd->instance>\<driver string>.
> > It seems like this highlights a deficiency in the vmstate matching
>
> Why are you forcing this to be a string?
It seemed like a good way to send an identifier. What do you suggest?
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-10 15:21 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-10 15:21 UTC (permalink / raw)
To: Paul Brook; +Cc: chrisw, kvm, quintela, qemu-devel, Gerd Hoffmann
On Thu, 2010-06-10 at 15:49 +0100, Paul Brook wrote:
> > On Thu, 2010-06-10 at 10:23 +0200, Gerd Hoffmann wrote:
> > > > I may have been a bit misleading here. What we really want to do is use
> > > > the same matching algorithm as is used by the rest of the device
> > > > state. Currently this is a vmstate name and [arbitrary] numeric id. I
> > > > don't remember whether there's a convenient link from a device to its
> > > > associated vmstate - if there isn't there probably should be.
> > >
> > > DeviceState->info->vmsd->name for the name.
> > > Dunno about the numeric id, I think savevm.c doesn't export it.
> >
> > Ok, we can certainly do <vmsd->name>.<vmsd->instance>\<driver string>.
> > It seems like this highlights a deficiency in the vmstate matching
>
> Why are you forcing this to be a string?
It seemed like a good way to send an identifier. What do you suggest?
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-10 15:21 ` Alex Williamson
@ 2010-06-11 8:48 ` Gerd Hoffmann
-1 siblings, 0 replies; 94+ messages in thread
From: Gerd Hoffmann @ 2010-06-11 8:48 UTC (permalink / raw)
To: Alex Williamson
Cc: Paul Brook, Anthony Liguori, qemu-devel, chrisw, kvm, quintela
On 06/10/10 17:21, Alex Williamson wrote:
> On Thu, 2010-06-10 at 15:49 +0100, Paul Brook wrote:
>>> Ok, we can certainly do<vmsd->name>.<vmsd->instance>\<driver string>.
>>> It seems like this highlights a deficiency in the vmstate matching
>>
>> Why are you forcing this to be a string?
>
> It seemed like a good way to send an identifier. What do you suggest?
Do it the same way savevm handles device state? I think it simply puts
a u32 for the instance id.
cheers,
Gerd
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-11 8:48 ` Gerd Hoffmann
0 siblings, 0 replies; 94+ messages in thread
From: Gerd Hoffmann @ 2010-06-11 8:48 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, kvm, quintela, qemu-devel, Paul Brook
On 06/10/10 17:21, Alex Williamson wrote:
> On Thu, 2010-06-10 at 15:49 +0100, Paul Brook wrote:
>>> Ok, we can certainly do<vmsd->name>.<vmsd->instance>\<driver string>.
>>> It seems like this highlights a deficiency in the vmstate matching
>>
>> Why are you forcing this to be a string?
>
> It seemed like a good way to send an identifier. What do you suggest?
Do it the same way savevm handles device state? I think it simply puts
a u32 for the instance id.
cheers,
Gerd
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-11 8:48 ` Gerd Hoffmann
@ 2010-06-11 15:50 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-11 15:50 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Paul Brook, Anthony Liguori, qemu-devel, chrisw, kvm, quintela
On Fri, 2010-06-11 at 10:48 +0200, Gerd Hoffmann wrote:
> On 06/10/10 17:21, Alex Williamson wrote:
> > On Thu, 2010-06-10 at 15:49 +0100, Paul Brook wrote:
> >>> Ok, we can certainly do<vmsd->name>.<vmsd->instance>\<driver string>.
> >>> It seems like this highlights a deficiency in the vmstate matching
> >>
> >> Why are you forcing this to be a string?
> >
> > It seemed like a good way to send an identifier. What do you suggest?
>
> Do it the same way savevm handles device state? I think it simply puts
> a u32 for the instance id.
I see, so then we'd have:
uint8 - string length (should we decide to go with a variable length)
buffer - "<vmsd->name>\<driver string>"
uint32 - instance_id
I'm not sure I see the benefit to that since then we'd have to do both a
strcmp and an instance_id match. It's unlikely we'll have instance_ids
large enough that even make it a space savings in the protocol stream
versus "name.id" ("e1000.0").
The trouble I'm running into is that the SaveStateEntry.instance_id is
effectively private, and there's no easy way to associate a
SaveStateEntry to a device since it passes an opaque pointer, which
could be whatever the driver decides it wants. I'm wondering if we
should pass the DeviceState pointer in when registering the vmstate so
that we can stuff the instance_id into the DeviceInfo. Or maybe
DeviceInfo should include a pointer to the SaveStateEntry. It all seems
pretty messy. Any thoughts?
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-11 15:50 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-11 15:50 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: chrisw, kvm, quintela, qemu-devel, Paul Brook
On Fri, 2010-06-11 at 10:48 +0200, Gerd Hoffmann wrote:
> On 06/10/10 17:21, Alex Williamson wrote:
> > On Thu, 2010-06-10 at 15:49 +0100, Paul Brook wrote:
> >>> Ok, we can certainly do<vmsd->name>.<vmsd->instance>\<driver string>.
> >>> It seems like this highlights a deficiency in the vmstate matching
> >>
> >> Why are you forcing this to be a string?
> >
> > It seemed like a good way to send an identifier. What do you suggest?
>
> Do it the same way savevm handles device state? I think it simply puts
> a u32 for the instance id.
I see, so then we'd have:
uint8 - string length (should we decide to go with a variable length)
buffer - "<vmsd->name>\<driver string>"
uint32 - instance_id
I'm not sure I see the benefit to that since then we'd have to do both a
strcmp and an instance_id match. It's unlikely we'll have instance_ids
large enough that even make it a space savings in the protocol stream
versus "name.id" ("e1000.0").
The trouble I'm running into is that the SaveStateEntry.instance_id is
effectively private, and there's no easy way to associate a
SaveStateEntry to a device since it passes an opaque pointer, which
could be whatever the driver decides it wants. I'm wondering if we
should pass the DeviceState pointer in when registering the vmstate so
that we can stuff the instance_id into the DeviceInfo. Or maybe
DeviceInfo should include a pointer to the SaveStateEntry. It all seems
pretty messy. Any thoughts?
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-11 15:50 ` Alex Williamson
@ 2010-06-11 16:12 ` Paul Brook
-1 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-11 16:12 UTC (permalink / raw)
To: Alex Williamson
Cc: Gerd Hoffmann, Anthony Liguori, qemu-devel, chrisw, kvm, quintela
> The trouble I'm running into is that the SaveStateEntry.instance_id is
> effectively private, and there's no easy way to associate a
> SaveStateEntry to a device since it passes an opaque pointer, which
> could be whatever the driver decides it wants. I'm wondering if we
> should pass the DeviceState pointer in when registering the vmstate so
> that we can stuff the instance_id into the DeviceInfo. Or maybe
> DeviceInfo should include a pointer to the SaveStateEntry. It all seems
> pretty messy. Any thoughts?
DeviceInfo is effectively a const structure (it may be modified at startup,
but there's only one of it shared between multiple devices). I suspect you
mean DeviceState.
Most likely a lot of the messyness is because we still have devices that have
not been qdev-ified, so the VMState code can't assume it has a device. We
should fix this.
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-11 16:12 ` Paul Brook
0 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-11 16:12 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, kvm, quintela, qemu-devel, Gerd Hoffmann
> The trouble I'm running into is that the SaveStateEntry.instance_id is
> effectively private, and there's no easy way to associate a
> SaveStateEntry to a device since it passes an opaque pointer, which
> could be whatever the driver decides it wants. I'm wondering if we
> should pass the DeviceState pointer in when registering the vmstate so
> that we can stuff the instance_id into the DeviceInfo. Or maybe
> DeviceInfo should include a pointer to the SaveStateEntry. It all seems
> pretty messy. Any thoughts?
DeviceInfo is effectively a const structure (it may be modified at startup,
but there's only one of it shared between multiple devices). I suspect you
mean DeviceState.
Most likely a lot of the messyness is because we still have devices that have
not been qdev-ified, so the VMState code can't assume it has a device. We
should fix this.
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-10 14:33 ` Alex Williamson
@ 2010-06-11 8:45 ` Gerd Hoffmann
-1 siblings, 0 replies; 94+ messages in thread
From: Gerd Hoffmann @ 2010-06-11 8:45 UTC (permalink / raw)
To: Alex Williamson
Cc: Paul Brook, Anthony Liguori, qemu-devel, chrisw, kvm, quintela
On 06/10/10 16:33, Alex Williamson wrote:
> On Thu, 2010-06-10 at 10:23 +0200, Gerd Hoffmann wrote:
>>> I may have been a bit misleading here. What we really want to do is use the
>>> same matching algorithm as is used by the rest of the device state. Currently
>>> this is a vmstate name and [arbitrary] numeric id. I don't remember whether
>>> there's a convenient link from a device to its associated vmstate - if there
>>> isn't there probably should be.
>>
>> DeviceState->info->vmsd->name for the name.
>> Dunno about the numeric id, I think savevm.c doesn't export it.
>
> Ok, we can certainly do<vmsd->name>.<vmsd->instance>\<driver string>.
> It seems like this highlights a deficiency in the vmstate matching
> though. If on the source we do:
>
>> pci_add addr=4 nic model=e1000
>> pci_add addr=3 nic model=e1000
>
> Then we start the target, ordering the nics sequentially, are we going
> to store the vmstate into the opposite nics?
I think so. We should be able to handle that better. Nevertheless it
makes sense to use the same naming scheme for device state and device
ram. If we fix this for the device state some day (using qdev most
likely), we'll go change device ram handling the same way.
cheers,
Gerd
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-11 8:45 ` Gerd Hoffmann
0 siblings, 0 replies; 94+ messages in thread
From: Gerd Hoffmann @ 2010-06-11 8:45 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, kvm, quintela, qemu-devel, Paul Brook
On 06/10/10 16:33, Alex Williamson wrote:
> On Thu, 2010-06-10 at 10:23 +0200, Gerd Hoffmann wrote:
>>> I may have been a bit misleading here. What we really want to do is use the
>>> same matching algorithm as is used by the rest of the device state. Currently
>>> this is a vmstate name and [arbitrary] numeric id. I don't remember whether
>>> there's a convenient link from a device to its associated vmstate - if there
>>> isn't there probably should be.
>>
>> DeviceState->info->vmsd->name for the name.
>> Dunno about the numeric id, I think savevm.c doesn't export it.
>
> Ok, we can certainly do<vmsd->name>.<vmsd->instance>\<driver string>.
> It seems like this highlights a deficiency in the vmstate matching
> though. If on the source we do:
>
>> pci_add addr=4 nic model=e1000
>> pci_add addr=3 nic model=e1000
>
> Then we start the target, ordering the nics sequentially, are we going
> to store the vmstate into the opposite nics?
I think so. We should be able to handle that better. Nevertheless it
makes sense to use the same naming scheme for device state and device
ram. If we fix this for the device state some day (using qdev most
likely), we'll go change device ram handling the same way.
cheers,
Gerd
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-09 20:36 ` Paul Brook
@ 2010-06-10 15:05 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-10 15:05 UTC (permalink / raw)
To: Paul Brook; +Cc: Anthony Liguori, qemu-devel, chrisw, kvm, quintela
On Wed, 2010-06-09 at 21:36 +0100, Paul Brook wrote:
> > > Not really. This identifier is device and bus independent, which is why
> > > I suggested passing the device to qemu_ram_alloc. This can then figure
> > > out how to the identify the device. It should probably do this the same
> > > way that we identify the saved state for the device. Currently I think
> > > this is an arbitrary vmstate name/id, but I expect this to change to a
> > > qdev address (e.g. /i440FX-pcihost/pci.0/_addr_04.0").
> >
> > Ok, that seems fairly reasonable, so from a device pointer we can get
> > something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add
> > something like ":rom" or ":bar.0" to it via an extra string.
> >
> > qemu_ram_alloc(DeviceState *dev, const char *info, size)
>
> Exactly - though personally I wouldn't call the second argument "info".
Hmm, this gets a little hairy for patch 5/6 where we try to create a
block on the fly to match the migration source. For now, this is mainly
to catch things like devices that are hot plugged then removed before
migration, but don't currently have a functional qemu_ram_free() to
clean up. However, if we could get past that and clean up drivers, it
might be nice for the string to provide enough information to
instantiate the missing device on the target. I suddenly see that
char[64] name becoming insufficient. Maybe we should follow the vmstate
example and use a variable length string preceded by a length byte (or
two).
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-10 15:05 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-10 15:05 UTC (permalink / raw)
To: Paul Brook; +Cc: chrisw, quintela, qemu-devel, kvm
On Wed, 2010-06-09 at 21:36 +0100, Paul Brook wrote:
> > > Not really. This identifier is device and bus independent, which is why
> > > I suggested passing the device to qemu_ram_alloc. This can then figure
> > > out how to the identify the device. It should probably do this the same
> > > way that we identify the saved state for the device. Currently I think
> > > this is an arbitrary vmstate name/id, but I expect this to change to a
> > > qdev address (e.g. /i440FX-pcihost/pci.0/_addr_04.0").
> >
> > Ok, that seems fairly reasonable, so from a device pointer we can get
> > something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add
> > something like ":rom" or ":bar.0" to it via an extra string.
> >
> > qemu_ram_alloc(DeviceState *dev, const char *info, size)
>
> Exactly - though personally I wouldn't call the second argument "info".
Hmm, this gets a little hairy for patch 5/6 where we try to create a
block on the fly to match the migration source. For now, this is mainly
to catch things like devices that are hot plugged then removed before
migration, but don't currently have a functional qemu_ram_free() to
clean up. However, if we could get past that and clean up drivers, it
might be nice for the string to provide enough information to
instantiate the missing device on the target. I suddenly see that
char[64] name becoming insufficient. Maybe we should follow the vmstate
example and use a variable length string preceded by a length byte (or
two).
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-09 16:37 ` Alex Williamson
@ 2010-06-10 16:40 ` Chris Wright
-1 siblings, 0 replies; 94+ messages in thread
From: Chris Wright @ 2010-06-10 16:40 UTC (permalink / raw)
To: Alex Williamson
Cc: Paul Brook, Anthony Liguori, qemu-devel, chrisw, kvm, quintela
* Alex Williamson (alex.williamson@redhat.com) wrote:
> On Wed, 2010-06-09 at 13:18 +0100, Paul Brook wrote:
> > to the identify the device. It should probably do this the same way that we
> > identify the saved state for the device. Currently I think this is an
> > arbitrary vmstate name/id, but I expect this to change to a qdev address
> > (e.g. /i440FX-pcihost/pci.0/_addr_04.0").
>
> Ok, that seems fairly reasonable, so from a device pointer we can get
> something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add
> something like ":rom" or ":bar.0" to it via an extra string.
In the fun game of what ifs...
The cmdline starts w/ device A placed at pci bus addr 00:04.0 (so
matched on source and target). The source does hotunplug of 04.0 and
replaces it w/ new device. I think we need something that is more
uniquely identifying the block. Not sure that device name is correct or
a generation ID.
thanks,
-chris
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-10 16:40 ` Chris Wright
0 siblings, 0 replies; 94+ messages in thread
From: Chris Wright @ 2010-06-10 16:40 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, kvm, quintela, qemu-devel, Paul Brook
* Alex Williamson (alex.williamson@redhat.com) wrote:
> On Wed, 2010-06-09 at 13:18 +0100, Paul Brook wrote:
> > to the identify the device. It should probably do this the same way that we
> > identify the saved state for the device. Currently I think this is an
> > arbitrary vmstate name/id, but I expect this to change to a qdev address
> > (e.g. /i440FX-pcihost/pci.0/_addr_04.0").
>
> Ok, that seems fairly reasonable, so from a device pointer we can get
> something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add
> something like ":rom" or ":bar.0" to it via an extra string.
In the fun game of what ifs...
The cmdline starts w/ device A placed at pci bus addr 00:04.0 (so
matched on source and target). The source does hotunplug of 04.0 and
replaces it w/ new device. I think we need something that is more
uniquely identifying the block. Not sure that device name is correct or
a generation ID.
thanks,
-chris
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-10 16:40 ` Chris Wright
@ 2010-06-10 16:45 ` Paul Brook
-1 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-10 16:45 UTC (permalink / raw)
To: Chris Wright; +Cc: Alex Williamson, Anthony Liguori, qemu-devel, kvm, quintela
> > > to the identify the device. It should probably do this the same way
> > > that we identify the saved state for the device. Currently I think
> > > this is an arbitrary vmstate name/id, but I expect this to change to a
> > > qdev address (e.g. /i440FX-pcihost/pci.0/_addr_04.0").
> >
> > Ok, that seems fairly reasonable, so from a device pointer we can get
> > something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add
> > something like ":rom" or ":bar.0" to it via an extra string.
>
> In the fun game of what ifs...
>
> The cmdline starts w/ device A placed at pci bus addr 00:04.0 (so
> matched on source and target). The source does hotunplug of 04.0 and
> replaces it w/ new device. I think we need something that is more
> uniquely identifying the block. Not sure that device name is correct or
> a generation ID.
You shouldn't be solving this problem for RAM blocks. You should be solving it
for the device state.
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-10 16:45 ` Paul Brook
0 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-10 16:45 UTC (permalink / raw)
To: Chris Wright; +Cc: Alex Williamson, quintela, qemu-devel, kvm
> > > to the identify the device. It should probably do this the same way
> > > that we identify the saved state for the device. Currently I think
> > > this is an arbitrary vmstate name/id, but I expect this to change to a
> > > qdev address (e.g. /i440FX-pcihost/pci.0/_addr_04.0").
> >
> > Ok, that seems fairly reasonable, so from a device pointer we can get
> > something like "/i440FX-pcihost/pci.0/_addr_04.0", then we can add
> > something like ":rom" or ":bar.0" to it via an extra string.
>
> In the fun game of what ifs...
>
> The cmdline starts w/ device A placed at pci bus addr 00:04.0 (so
> matched on source and target). The source does hotunplug of 04.0 and
> replaces it w/ new device. I think we need something that is more
> uniquely identifying the block. Not sure that device name is correct or
> a generation ID.
You shouldn't be solving this problem for RAM blocks. You should be solving it
for the device state.
Paul
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-09 2:54 ` Paul Brook
@ 2010-06-09 11:58 ` Avi Kivity
-1 siblings, 0 replies; 94+ messages in thread
From: Avi Kivity @ 2010-06-09 11:58 UTC (permalink / raw)
To: Paul Brook
Cc: Anthony Liguori, qemu-devel, chrisw, Alex Williamson, kvm, quintela
On 06/09/2010 05:54 AM, Paul Brook wrote:
>> On 06/08/2010 09:30 PM, Paul Brook wrote:
>>
>>>> The offset given to a block created via qemu_ram_alloc/map() is
>>>> arbitrary, let the caller specify a name so we can make a positive
>>>> match.
>>>>
>>>>
>>>> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
>>>> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
>>>> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>>>> + pdev->rom_offset = qemu_ram_alloc(name, size);
>>>>
>>> This looks pretty bogus. It should be associated with the device, rather
>>> than incorrectly trying to generate a globally unique name.
>>>
>> Not all ram is associated with a device.
>>
> Maybe not, but where it is we should be using that information.
> Absolute minimum we should be using the existing qdev address rather than
> inventing a new one. Duplicating this logic inside every device seems like a
> bad idea so I suggest identifying ram blocks by a (name, device) pair. For now
> we can allow a NULL device for system memory.
>
>
I agree, this is duplicating the qdev tree in a string. Devices/BARs
should have ram qdev fields and so ram can be enumerated completely via
qdev.
System memory can be part of a system device.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-09 11:58 ` Avi Kivity
0 siblings, 0 replies; 94+ messages in thread
From: Avi Kivity @ 2010-06-09 11:58 UTC (permalink / raw)
To: Paul Brook; +Cc: chrisw, kvm, quintela, qemu-devel, Alex Williamson
On 06/09/2010 05:54 AM, Paul Brook wrote:
>> On 06/08/2010 09:30 PM, Paul Brook wrote:
>>
>>>> The offset given to a block created via qemu_ram_alloc/map() is
>>>> arbitrary, let the caller specify a name so we can make a positive
>>>> match.
>>>>
>>>>
>>>> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
>>>> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
>>>> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>>>> + pdev->rom_offset = qemu_ram_alloc(name, size);
>>>>
>>> This looks pretty bogus. It should be associated with the device, rather
>>> than incorrectly trying to generate a globally unique name.
>>>
>> Not all ram is associated with a device.
>>
> Maybe not, but where it is we should be using that information.
> Absolute minimum we should be using the existing qdev address rather than
> inventing a new one. Duplicating this logic inside every device seems like a
> bad idea so I suggest identifying ram blocks by a (name, device) pair. For now
> we can allow a NULL device for system memory.
>
>
I agree, this is duplicating the qdev tree in a string. Devices/BARs
should have ram qdev fields and so ram can be enumerated completely via
qdev.
System memory can be part of a system device.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-09 11:58 ` Avi Kivity
@ 2010-06-09 13:57 ` Anthony Liguori
-1 siblings, 0 replies; 94+ messages in thread
From: Anthony Liguori @ 2010-06-09 13:57 UTC (permalink / raw)
To: Avi Kivity; +Cc: Paul Brook, qemu-devel, chrisw, Alex Williamson, kvm, quintela
On 06/09/2010 06:58 AM, Avi Kivity wrote:
> On 06/09/2010 05:54 AM, Paul Brook wrote:
>>> On 06/08/2010 09:30 PM, Paul Brook wrote:
>>>>> The offset given to a block created via qemu_ram_alloc/map() is
>>>>> arbitrary, let the caller specify a name so we can make a positive
>>>>> match.
>>>>>
>>>>>
>>>>> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
>>>>> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
>>>>> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>>>>> + pdev->rom_offset = qemu_ram_alloc(name, size);
>>>> This looks pretty bogus. It should be associated with the device,
>>>> rather
>>>> than incorrectly trying to generate a globally unique name.
>>> Not all ram is associated with a device.
>> Maybe not, but where it is we should be using that information.
>> Absolute minimum we should be using the existing qdev address rather
>> than
>> inventing a new one. Duplicating this logic inside every device
>> seems like a
>> bad idea so I suggest identifying ram blocks by a (name, device)
>> pair. For now
>> we can allow a NULL device for system memory.
>>
>
> I agree, this is duplicating the qdev tree in a string. Devices/BARs
> should have ram qdev fields and so ram can be enumerated completely
> via qdev.
Keep in mind, this has to be a stable string across versions of qemu
since this is savevm/migration. Are we absolutely confident that the
full qdev path isn't going to change? I'm more confident that a unique
device name is going to be static across qemu versions.
Regards,
Anthony Liguori
> System memory can be part of a system device.
>
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-09 13:57 ` Anthony Liguori
0 siblings, 0 replies; 94+ messages in thread
From: Anthony Liguori @ 2010-06-09 13:57 UTC (permalink / raw)
To: Avi Kivity; +Cc: chrisw, kvm, quintela, qemu-devel, Alex Williamson, Paul Brook
On 06/09/2010 06:58 AM, Avi Kivity wrote:
> On 06/09/2010 05:54 AM, Paul Brook wrote:
>>> On 06/08/2010 09:30 PM, Paul Brook wrote:
>>>>> The offset given to a block created via qemu_ram_alloc/map() is
>>>>> arbitrary, let the caller specify a name so we can make a positive
>>>>> match.
>>>>>
>>>>>
>>>>> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
>>>>> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
>>>>> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>>>>> + pdev->rom_offset = qemu_ram_alloc(name, size);
>>>> This looks pretty bogus. It should be associated with the device,
>>>> rather
>>>> than incorrectly trying to generate a globally unique name.
>>> Not all ram is associated with a device.
>> Maybe not, but where it is we should be using that information.
>> Absolute minimum we should be using the existing qdev address rather
>> than
>> inventing a new one. Duplicating this logic inside every device
>> seems like a
>> bad idea so I suggest identifying ram blocks by a (name, device)
>> pair. For now
>> we can allow a NULL device for system memory.
>>
>
> I agree, this is duplicating the qdev tree in a string. Devices/BARs
> should have ram qdev fields and so ram can be enumerated completely
> via qdev.
Keep in mind, this has to be a stable string across versions of qemu
since this is savevm/migration. Are we absolutely confident that the
full qdev path isn't going to change? I'm more confident that a unique
device name is going to be static across qemu versions.
Regards,
Anthony Liguori
> System memory can be part of a system device.
>
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-09 13:57 ` Anthony Liguori
@ 2010-06-09 14:09 ` Paul Brook
-1 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-09 14:09 UTC (permalink / raw)
To: Anthony Liguori
Cc: Avi Kivity, qemu-devel, chrisw, Alex Williamson, kvm, quintela
> Keep in mind, this has to be a stable string across versions of qemu
> since this is savevm/migration. Are we absolutely confident that the
> full qdev path isn't going to change? I'm more confident that a unique
> device name is going to be static across qemu versions.
The actual representation of the device address is a secondary issue. The
important point is that ram blocks should be associated with devices[*], and
matched in exactly the same way. Devices should not be duplicating this on an
ad-hoc basis.
Paul
[*] Ignore that we don't currently have a root system device node. A null
device will suffice for now.
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-09 14:09 ` Paul Brook
0 siblings, 0 replies; 94+ messages in thread
From: Paul Brook @ 2010-06-09 14:09 UTC (permalink / raw)
To: Anthony Liguori
Cc: chrisw, kvm, quintela, qemu-devel, Alex Williamson, Avi Kivity
> Keep in mind, this has to be a stable string across versions of qemu
> since this is savevm/migration. Are we absolutely confident that the
> full qdev path isn't going to change? I'm more confident that a unique
> device name is going to be static across qemu versions.
The actual representation of the device address is a secondary issue. The
important point is that ram blocks should be associated with devices[*], and
matched in exactly the same way. Devices should not be duplicating this on an
ad-hoc basis.
Paul
[*] Ignore that we don't currently have a root system device node. A null
device will suffice for now.
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
2010-06-09 2:40 ` Anthony Liguori
@ 2010-06-09 7:28 ` Gerd Hoffmann
-1 siblings, 0 replies; 94+ messages in thread
From: Gerd Hoffmann @ 2010-06-09 7:28 UTC (permalink / raw)
To: Anthony Liguori
Cc: Paul Brook, qemu-devel, chrisw, Alex Williamson, kvm, quintela
On 06/09/10 04:40, Anthony Liguori wrote:
> On 06/08/2010 09:30 PM, Paul Brook wrote:
>>> The offset given to a block created via qemu_ram_alloc/map() is
>>> arbitrary,
>>> let the caller specify a name so we can make a positive match.
>>> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
>>> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
>>> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>>> + pdev->rom_offset = qemu_ram_alloc(name, size);
>> This looks pretty bogus. It should be associated with the device,
>> rather than
>> incorrectly trying to generate a globally unique name.
>
> Not all ram is associated with a device.
Nevertheless qdev could carry a list of any device specific ram, so
qdev_free() could automagically cleanup for you on unplug. You could
also reuse DeviceState->info->vmsd->name for the ram section naming.
cheers,
Gerd
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/6] RAMBlock: Add a name field
@ 2010-06-09 7:28 ` Gerd Hoffmann
0 siblings, 0 replies; 94+ messages in thread
From: Gerd Hoffmann @ 2010-06-09 7:28 UTC (permalink / raw)
To: Anthony Liguori
Cc: chrisw, kvm, quintela, qemu-devel, Alex Williamson, Paul Brook
On 06/09/10 04:40, Anthony Liguori wrote:
> On 06/08/2010 09:30 PM, Paul Brook wrote:
>>> The offset given to a block created via qemu_ram_alloc/map() is
>>> arbitrary,
>>> let the caller specify a name so we can make a positive match.
>>> @@ -1924,7 +1925,9 @@ static int pci_add_option_rom(PCIDevice *pdev)
>>> + snprintf(name, sizeof(name), "pci:%02x.%x.rom",
>>> + PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>>> + pdev->rom_offset = qemu_ram_alloc(name, size);
>> This looks pretty bogus. It should be associated with the device,
>> rather than
>> incorrectly trying to generate a globally unique name.
>
> Not all ram is associated with a device.
Nevertheless qdev could carry a list of any device specific ram, so
qdev_free() could automagically cleanup for you on unplug. You could
also reuse DeviceState->info->vmsd->name for the ram section naming.
cheers,
Gerd
^ permalink raw reply [flat|nested] 94+ messages in thread
* [RFC PATCH 4/6] Remove uses of ram.last_offset (aka last_ram_offset)
2010-06-08 19:14 ` [Qemu-devel] " Alex Williamson
@ 2010-06-08 19:16 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 19:16 UTC (permalink / raw)
To: qemu-devel, anthony; +Cc: kvm, quintela, chrisw, alex.williamson
We currently need this either to allocate the next ram_addr_t for a
new block, or for total memory to be migrated. Both of which we can
calculate without need of this to keep us in a contiguous address space.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
arch_init.c | 23 ++++++++++++++++-------
cpu-all.h | 1 -
exec.c | 19 ++++++++++++++-----
3 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 43e42ba..6103461 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -108,9 +108,10 @@ static int ram_save_block(QEMUFile *f)
static ram_addr_t current_addr = 0;
ram_addr_t saved_addr = current_addr;
ram_addr_t addr = 0;
+ uint64_t total_ram = ram_bytes_total();
int bytes_sent = 0;
- while (addr < ram.last_offset) {
+ while (addr < total_ram) {
if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
uint8_t *p;
@@ -133,7 +134,7 @@ static int ram_save_block(QEMUFile *f)
break;
}
addr += TARGET_PAGE_SIZE;
- current_addr = (saved_addr + addr) % ram.last_offset;
+ current_addr = (saved_addr + addr) % total_ram;
}
return bytes_sent;
@@ -145,8 +146,9 @@ static ram_addr_t ram_save_remaining(void)
{
ram_addr_t addr;
ram_addr_t count = 0;
+ uint64_t total_ram = ram_bytes_total();
- for (addr = 0; addr < ram.last_offset; addr += TARGET_PAGE_SIZE) {
+ for (addr = 0; addr < total_ram; addr += TARGET_PAGE_SIZE) {
if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
count++;
}
@@ -167,7 +169,13 @@ uint64_t ram_bytes_transferred(void)
uint64_t ram_bytes_total(void)
{
- return ram.last_offset;
+ RAMBlock *block;
+ uint64_t total = 0;
+
+ QLIST_FOREACH(block, &ram.blocks, next)
+ total += block->length;
+
+ return total;
}
int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
@@ -188,10 +196,11 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
}
if (stage == 1) {
+ uint64_t total_ram = ram_bytes_total();
bytes_transferred = 0;
/* Make sure all dirty bits are set */
- for (addr = 0; addr < ram.last_offset; addr += TARGET_PAGE_SIZE) {
+ for (addr = 0; addr < total_ram; addr += TARGET_PAGE_SIZE) {
if (!cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
cpu_physical_memory_set_dirty(addr);
}
@@ -200,7 +209,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
/* Enable dirty memory tracking */
cpu_physical_memory_set_dirty_tracking(1);
- qemu_put_be64(f, ram.last_offset | RAM_SAVE_FLAG_MEM_SIZE);
+ qemu_put_be64(f, total_ram | RAM_SAVE_FLAG_MEM_SIZE);
}
bytes_transferred_last = bytes_transferred;
@@ -259,7 +268,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
addr &= TARGET_PAGE_MASK;
if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
- if (addr != ram.last_offset) {
+ if (addr != ram_bytes_total()) {
return -EINVAL;
}
}
diff --git a/cpu-all.h b/cpu-all.h
index a5b886a..23b1d1d 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -871,7 +871,6 @@ typedef struct RAMBlock {
typedef struct RAMList {
uint8_t *phys_dirty;
- ram_addr_t last_offset;
QLIST_HEAD(ram, RAMBlock) blocks;
} RAMList;
extern RAMList ram;
diff --git a/exec.c b/exec.c
index dd57515..13c0ca0 100644
--- a/exec.c
+++ b/exec.c
@@ -2774,6 +2774,17 @@ static void *file_ram_alloc(ram_addr_t memory, const char *path)
}
#endif
+static ram_addr_t find_ram_offset(ram_addr_t size)
+{
+ RAMBlock *block;
+ ram_addr_t last = 0;
+
+ QLIST_FOREACH(block, &ram.blocks, next)
+ last = MAX(last, block->offset + block->length);
+
+ return last;
+}
+
ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host)
{
RAMBlock *new_block;
@@ -2785,18 +2796,16 @@ ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host)
snprintf(new_block->name, sizeof(new_block->name), "%s", strdup(name));
new_block->host = host;
- new_block->offset = ram.last_offset;
+ new_block->offset = find_ram_offset(size);
new_block->length = size;
QLIST_INSERT_HEAD(&ram.blocks, new_block, next);
ram.phys_dirty = qemu_realloc(ram.phys_dirty,
- (ram.last_offset + size) >> TARGET_PAGE_BITS);
- memset(ram.phys_dirty + (ram.last_offset >> TARGET_PAGE_BITS),
+ (new_block->offset + size) >> TARGET_PAGE_BITS);
+ memset(ram.phys_dirty + (new_block->offset >> TARGET_PAGE_BITS),
0xff, size >> TARGET_PAGE_BITS);
- ram.last_offset += size;
-
if (kvm_enabled())
kvm_setup_guest_memory(new_block->host, size);
^ permalink raw reply related [flat|nested] 94+ messages in thread* [Qemu-devel] [RFC PATCH 4/6] Remove uses of ram.last_offset (aka last_ram_offset)
@ 2010-06-08 19:16 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 19:16 UTC (permalink / raw)
To: qemu-devel, anthony; +Cc: chrisw, alex.williamson, kvm, quintela
We currently need this either to allocate the next ram_addr_t for a
new block, or for total memory to be migrated. Both of which we can
calculate without need of this to keep us in a contiguous address space.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
arch_init.c | 23 ++++++++++++++++-------
cpu-all.h | 1 -
exec.c | 19 ++++++++++++++-----
3 files changed, 30 insertions(+), 13 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 43e42ba..6103461 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -108,9 +108,10 @@ static int ram_save_block(QEMUFile *f)
static ram_addr_t current_addr = 0;
ram_addr_t saved_addr = current_addr;
ram_addr_t addr = 0;
+ uint64_t total_ram = ram_bytes_total();
int bytes_sent = 0;
- while (addr < ram.last_offset) {
+ while (addr < total_ram) {
if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
uint8_t *p;
@@ -133,7 +134,7 @@ static int ram_save_block(QEMUFile *f)
break;
}
addr += TARGET_PAGE_SIZE;
- current_addr = (saved_addr + addr) % ram.last_offset;
+ current_addr = (saved_addr + addr) % total_ram;
}
return bytes_sent;
@@ -145,8 +146,9 @@ static ram_addr_t ram_save_remaining(void)
{
ram_addr_t addr;
ram_addr_t count = 0;
+ uint64_t total_ram = ram_bytes_total();
- for (addr = 0; addr < ram.last_offset; addr += TARGET_PAGE_SIZE) {
+ for (addr = 0; addr < total_ram; addr += TARGET_PAGE_SIZE) {
if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
count++;
}
@@ -167,7 +169,13 @@ uint64_t ram_bytes_transferred(void)
uint64_t ram_bytes_total(void)
{
- return ram.last_offset;
+ RAMBlock *block;
+ uint64_t total = 0;
+
+ QLIST_FOREACH(block, &ram.blocks, next)
+ total += block->length;
+
+ return total;
}
int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
@@ -188,10 +196,11 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
}
if (stage == 1) {
+ uint64_t total_ram = ram_bytes_total();
bytes_transferred = 0;
/* Make sure all dirty bits are set */
- for (addr = 0; addr < ram.last_offset; addr += TARGET_PAGE_SIZE) {
+ for (addr = 0; addr < total_ram; addr += TARGET_PAGE_SIZE) {
if (!cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
cpu_physical_memory_set_dirty(addr);
}
@@ -200,7 +209,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
/* Enable dirty memory tracking */
cpu_physical_memory_set_dirty_tracking(1);
- qemu_put_be64(f, ram.last_offset | RAM_SAVE_FLAG_MEM_SIZE);
+ qemu_put_be64(f, total_ram | RAM_SAVE_FLAG_MEM_SIZE);
}
bytes_transferred_last = bytes_transferred;
@@ -259,7 +268,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
addr &= TARGET_PAGE_MASK;
if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
- if (addr != ram.last_offset) {
+ if (addr != ram_bytes_total()) {
return -EINVAL;
}
}
diff --git a/cpu-all.h b/cpu-all.h
index a5b886a..23b1d1d 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -871,7 +871,6 @@ typedef struct RAMBlock {
typedef struct RAMList {
uint8_t *phys_dirty;
- ram_addr_t last_offset;
QLIST_HEAD(ram, RAMBlock) blocks;
} RAMList;
extern RAMList ram;
diff --git a/exec.c b/exec.c
index dd57515..13c0ca0 100644
--- a/exec.c
+++ b/exec.c
@@ -2774,6 +2774,17 @@ static void *file_ram_alloc(ram_addr_t memory, const char *path)
}
#endif
+static ram_addr_t find_ram_offset(ram_addr_t size)
+{
+ RAMBlock *block;
+ ram_addr_t last = 0;
+
+ QLIST_FOREACH(block, &ram.blocks, next)
+ last = MAX(last, block->offset + block->length);
+
+ return last;
+}
+
ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host)
{
RAMBlock *new_block;
@@ -2785,18 +2796,16 @@ ram_addr_t qemu_ram_map(const char *name, ram_addr_t size, void *host)
snprintf(new_block->name, sizeof(new_block->name), "%s", strdup(name));
new_block->host = host;
- new_block->offset = ram.last_offset;
+ new_block->offset = find_ram_offset(size);
new_block->length = size;
QLIST_INSERT_HEAD(&ram.blocks, new_block, next);
ram.phys_dirty = qemu_realloc(ram.phys_dirty,
- (ram.last_offset + size) >> TARGET_PAGE_BITS);
- memset(ram.phys_dirty + (ram.last_offset >> TARGET_PAGE_BITS),
+ (new_block->offset + size) >> TARGET_PAGE_BITS);
+ memset(ram.phys_dirty + (new_block->offset >> TARGET_PAGE_BITS),
0xff, size >> TARGET_PAGE_BITS);
- ram.last_offset += size;
-
if (kvm_enabled())
kvm_setup_guest_memory(new_block->host, size);
^ permalink raw reply related [flat|nested] 94+ messages in thread
* [RFC PATCH 5/6] savevm: Migrate RAM based on name/offset
2010-06-08 19:14 ` [Qemu-devel] " Alex Williamson
@ 2010-06-08 19:16 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 19:16 UTC (permalink / raw)
To: qemu-devel, anthony; +Cc: kvm, quintela, chrisw, alex.williamson
Synchronize RAM blocks with the target and migrate using name/offset
pairs. This ensures both source and target have the same view of
RAM and that we get the right bits into the right slot.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
arch_init.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
vl.c | 2 +
2 files changed, 93 insertions(+), 12 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 6103461..8ba92ec 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -113,20 +113,29 @@ static int ram_save_block(QEMUFile *f)
while (addr < total_ram) {
if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
+ RAMBlock *block;
+ ram_addr_t offset;
uint8_t *p;
cpu_physical_memory_reset_dirty(current_addr,
current_addr + TARGET_PAGE_SIZE,
MIGRATION_DIRTY_FLAG);
- p = qemu_get_ram_ptr(current_addr);
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ if (current_addr - block->offset < block->length)
+ break;
+ }
+ offset = current_addr - block->offset;
+ p = block->host + offset;
if (is_dup_page(p, *p)) {
- qemu_put_be64(f, current_addr | RAM_SAVE_FLAG_COMPRESS);
+ qemu_put_be64(f, offset | RAM_SAVE_FLAG_COMPRESS);
+ qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
qemu_put_byte(f, *p);
bytes_sent = 1;
} else {
- qemu_put_be64(f, current_addr | RAM_SAVE_FLAG_PAGE);
+ qemu_put_be64(f, offset | RAM_SAVE_FLAG_PAGE);
+ qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
bytes_sent = TARGET_PAGE_SIZE;
}
@@ -196,6 +205,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
}
if (stage == 1) {
+ RAMBlock *block;
uint64_t total_ram = ram_bytes_total();
bytes_transferred = 0;
@@ -210,6 +220,11 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
cpu_physical_memory_set_dirty_tracking(1);
qemu_put_be64(f, total_ram | RAM_SAVE_FLAG_MEM_SIZE);
+
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
+ qemu_put_be64(f, block->length);
+ }
}
bytes_transferred_last = bytes_transferred;
@@ -257,7 +272,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
ram_addr_t addr;
int flags;
- if (version_id != 3) {
+ if (version_id < 3) {
return -EINVAL;
}
@@ -268,23 +283,89 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
addr &= TARGET_PAGE_MASK;
if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
- if (addr != ram_bytes_total()) {
- return -EINVAL;
+ if (version_id == 3) {
+ if (addr != ram_bytes_total()) {
+ return -EINVAL;
+ }
+ } else {
+ /* Synchronize RAM block list */
+ char name[64];
+ ram_addr_t length;
+ ram_addr_t total_ram_bytes = addr;
+
+ while (total_ram_bytes) {
+ RAMBlock *block;
+ qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
+ length = qemu_get_be64(f);
+
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ if (!strncmp(name, block->name, sizeof(name))) {
+ if (block->length != length)
+ return -EINVAL;
+ break;
+ }
+ }
+
+ if (!block) {
+ if (!qemu_ram_alloc(name, length))
+ return -ENOMEM;
+ }
+
+ total_ram_bytes -= length;
+ }
}
}
if (flags & RAM_SAVE_FLAG_COMPRESS) {
- uint8_t ch = qemu_get_byte(f);
- memset(qemu_get_ram_ptr(addr), ch, TARGET_PAGE_SIZE);
+ void *host;
+ uint8_t ch;
+
+ if (version_id == 3) {
+ host = qemu_get_ram_ptr(addr);
+ } else {
+ RAMBlock *block;
+ char name[64];
+
+ qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
+
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ if (!strncmp(name, block->name, sizeof(name)))
+ break;
+ }
+ if (!block)
+ return -EINVAL;
+
+ host = block->host + addr;
+ }
+ ch = qemu_get_byte(f);
+ memset(host, ch, TARGET_PAGE_SIZE);
#ifndef _WIN32
if (ch == 0 &&
(!kvm_enabled() || kvm_has_sync_mmu())) {
- madvise(qemu_get_ram_ptr(addr), TARGET_PAGE_SIZE,
- MADV_DONTNEED);
+ madvise(host, TARGET_PAGE_SIZE, MADV_DONTNEED);
}
#endif
} else if (flags & RAM_SAVE_FLAG_PAGE) {
- qemu_get_buffer(f, qemu_get_ram_ptr(addr), TARGET_PAGE_SIZE);
+ void *host;
+
+ if (version_id == 3) {
+ host = qemu_get_ram_ptr(addr);
+ } else {
+ RAMBlock *block;
+ char name[64];
+
+ qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
+
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ if (!strncmp(name, block->name, sizeof(name)))
+ break;
+ }
+ if (!block)
+ return -EINVAL;
+
+ host = block->host + addr;
+ }
+ qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
}
if (qemu_file_has_error(f)) {
return -EIO;
diff --git a/vl.c b/vl.c
index 9e9c176..a871895 100644
--- a/vl.c
+++ b/vl.c
@@ -3731,7 +3731,7 @@ int main(int argc, char **argv, char **envp)
if (qemu_opts_foreach(&qemu_drive_opts, drive_init_func, machine, 1) != 0)
exit(1);
- register_savevm_live("ram", 0, 3, NULL, ram_save_live, NULL,
+ register_savevm_live("ram", 0, 4, NULL, ram_save_live, NULL,
ram_load, NULL);
if (nb_numa_nodes > 0) {
^ permalink raw reply related [flat|nested] 94+ messages in thread* [Qemu-devel] [RFC PATCH 5/6] savevm: Migrate RAM based on name/offset
@ 2010-06-08 19:16 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 19:16 UTC (permalink / raw)
To: qemu-devel, anthony; +Cc: chrisw, alex.williamson, kvm, quintela
Synchronize RAM blocks with the target and migrate using name/offset
pairs. This ensures both source and target have the same view of
RAM and that we get the right bits into the right slot.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
arch_init.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
vl.c | 2 +
2 files changed, 93 insertions(+), 12 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 6103461..8ba92ec 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -113,20 +113,29 @@ static int ram_save_block(QEMUFile *f)
while (addr < total_ram) {
if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
+ RAMBlock *block;
+ ram_addr_t offset;
uint8_t *p;
cpu_physical_memory_reset_dirty(current_addr,
current_addr + TARGET_PAGE_SIZE,
MIGRATION_DIRTY_FLAG);
- p = qemu_get_ram_ptr(current_addr);
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ if (current_addr - block->offset < block->length)
+ break;
+ }
+ offset = current_addr - block->offset;
+ p = block->host + offset;
if (is_dup_page(p, *p)) {
- qemu_put_be64(f, current_addr | RAM_SAVE_FLAG_COMPRESS);
+ qemu_put_be64(f, offset | RAM_SAVE_FLAG_COMPRESS);
+ qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
qemu_put_byte(f, *p);
bytes_sent = 1;
} else {
- qemu_put_be64(f, current_addr | RAM_SAVE_FLAG_PAGE);
+ qemu_put_be64(f, offset | RAM_SAVE_FLAG_PAGE);
+ qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
bytes_sent = TARGET_PAGE_SIZE;
}
@@ -196,6 +205,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
}
if (stage == 1) {
+ RAMBlock *block;
uint64_t total_ram = ram_bytes_total();
bytes_transferred = 0;
@@ -210,6 +220,11 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
cpu_physical_memory_set_dirty_tracking(1);
qemu_put_be64(f, total_ram | RAM_SAVE_FLAG_MEM_SIZE);
+
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
+ qemu_put_be64(f, block->length);
+ }
}
bytes_transferred_last = bytes_transferred;
@@ -257,7 +272,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
ram_addr_t addr;
int flags;
- if (version_id != 3) {
+ if (version_id < 3) {
return -EINVAL;
}
@@ -268,23 +283,89 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
addr &= TARGET_PAGE_MASK;
if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
- if (addr != ram_bytes_total()) {
- return -EINVAL;
+ if (version_id == 3) {
+ if (addr != ram_bytes_total()) {
+ return -EINVAL;
+ }
+ } else {
+ /* Synchronize RAM block list */
+ char name[64];
+ ram_addr_t length;
+ ram_addr_t total_ram_bytes = addr;
+
+ while (total_ram_bytes) {
+ RAMBlock *block;
+ qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
+ length = qemu_get_be64(f);
+
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ if (!strncmp(name, block->name, sizeof(name))) {
+ if (block->length != length)
+ return -EINVAL;
+ break;
+ }
+ }
+
+ if (!block) {
+ if (!qemu_ram_alloc(name, length))
+ return -ENOMEM;
+ }
+
+ total_ram_bytes -= length;
+ }
}
}
if (flags & RAM_SAVE_FLAG_COMPRESS) {
- uint8_t ch = qemu_get_byte(f);
- memset(qemu_get_ram_ptr(addr), ch, TARGET_PAGE_SIZE);
+ void *host;
+ uint8_t ch;
+
+ if (version_id == 3) {
+ host = qemu_get_ram_ptr(addr);
+ } else {
+ RAMBlock *block;
+ char name[64];
+
+ qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
+
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ if (!strncmp(name, block->name, sizeof(name)))
+ break;
+ }
+ if (!block)
+ return -EINVAL;
+
+ host = block->host + addr;
+ }
+ ch = qemu_get_byte(f);
+ memset(host, ch, TARGET_PAGE_SIZE);
#ifndef _WIN32
if (ch == 0 &&
(!kvm_enabled() || kvm_has_sync_mmu())) {
- madvise(qemu_get_ram_ptr(addr), TARGET_PAGE_SIZE,
- MADV_DONTNEED);
+ madvise(host, TARGET_PAGE_SIZE, MADV_DONTNEED);
}
#endif
} else if (flags & RAM_SAVE_FLAG_PAGE) {
- qemu_get_buffer(f, qemu_get_ram_ptr(addr), TARGET_PAGE_SIZE);
+ void *host;
+
+ if (version_id == 3) {
+ host = qemu_get_ram_ptr(addr);
+ } else {
+ RAMBlock *block;
+ char name[64];
+
+ qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
+
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ if (!strncmp(name, block->name, sizeof(name)))
+ break;
+ }
+ if (!block)
+ return -EINVAL;
+
+ host = block->host + addr;
+ }
+ qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
}
if (qemu_file_has_error(f)) {
return -EIO;
diff --git a/vl.c b/vl.c
index 9e9c176..a871895 100644
--- a/vl.c
+++ b/vl.c
@@ -3731,7 +3731,7 @@ int main(int argc, char **argv, char **envp)
if (qemu_opts_foreach(&qemu_drive_opts, drive_init_func, machine, 1) != 0)
exit(1);
- register_savevm_live("ram", 0, 3, NULL, ram_save_live, NULL,
+ register_savevm_live("ram", 0, 4, NULL, ram_save_live, NULL,
ram_load, NULL);
if (nb_numa_nodes > 0) {
^ permalink raw reply related [flat|nested] 94+ messages in thread* Re: [RFC PATCH 5/6] savevm: Migrate RAM based on name/offset
2010-06-08 19:16 ` [Qemu-devel] " Alex Williamson
@ 2010-06-08 20:12 ` Anthony Liguori
-1 siblings, 0 replies; 94+ messages in thread
From: Anthony Liguori @ 2010-06-08 20:12 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, qemu-devel, kvm, quintela
On 06/08/2010 02:16 PM, Alex Williamson wrote:
> Synchronize RAM blocks with the target and migrate using name/offset
> pairs. This ensures both source and target have the same view of
> RAM and that we get the right bits into the right slot.
>
> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
> ---
>
> arch_init.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
> vl.c | 2 +
> 2 files changed, 93 insertions(+), 12 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 6103461..8ba92ec 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -113,20 +113,29 @@ static int ram_save_block(QEMUFile *f)
>
> while (addr< total_ram) {
> if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
> + RAMBlock *block;
> + ram_addr_t offset;
> uint8_t *p;
>
> cpu_physical_memory_reset_dirty(current_addr,
> current_addr + TARGET_PAGE_SIZE,
> MIGRATION_DIRTY_FLAG);
>
> - p = qemu_get_ram_ptr(current_addr);
> + QLIST_FOREACH(block,&ram.blocks, next) {
> + if (current_addr - block->offset< block->length)
> + break;
> + }
> + offset = current_addr - block->offset;
> + p = block->host + offset;
>
> if (is_dup_page(p, *p)) {
> - qemu_put_be64(f, current_addr | RAM_SAVE_FLAG_COMPRESS);
> + qemu_put_be64(f, offset | RAM_SAVE_FLAG_COMPRESS);
> + qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
> qemu_put_byte(f, *p);
>
I think we could use some trickery like use another flag in
current_address to determine whether this was a different section from
the previous section and then only encode the section name if that's true.
Regards,
Anthony Liguori
> bytes_sent = 1;
> } else {
> - qemu_put_be64(f, current_addr | RAM_SAVE_FLAG_PAGE);
> + qemu_put_be64(f, offset | RAM_SAVE_FLAG_PAGE);
> + qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
> qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
> bytes_sent = TARGET_PAGE_SIZE;
> }
> @@ -196,6 +205,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
> }
>
> if (stage == 1) {
> + RAMBlock *block;
> uint64_t total_ram = ram_bytes_total();
> bytes_transferred = 0;
>
> @@ -210,6 +220,11 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
> cpu_physical_memory_set_dirty_tracking(1);
>
> qemu_put_be64(f, total_ram | RAM_SAVE_FLAG_MEM_SIZE);
> +
> + QLIST_FOREACH(block,&ram.blocks, next) {
> + qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
> + qemu_put_be64(f, block->length);
> + }
> }
>
> bytes_transferred_last = bytes_transferred;
> @@ -257,7 +272,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> ram_addr_t addr;
> int flags;
>
> - if (version_id != 3) {
> + if (version_id< 3) {
> return -EINVAL;
> }
>
> @@ -268,23 +283,89 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> addr&= TARGET_PAGE_MASK;
>
> if (flags& RAM_SAVE_FLAG_MEM_SIZE) {
> - if (addr != ram_bytes_total()) {
> - return -EINVAL;
> + if (version_id == 3) {
> + if (addr != ram_bytes_total()) {
> + return -EINVAL;
> + }
> + } else {
> + /* Synchronize RAM block list */
> + char name[64];
> + ram_addr_t length;
> + ram_addr_t total_ram_bytes = addr;
> +
> + while (total_ram_bytes) {
> + RAMBlock *block;
> + qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
> + length = qemu_get_be64(f);
> +
> + QLIST_FOREACH(block,&ram.blocks, next) {
> + if (!strncmp(name, block->name, sizeof(name))) {
> + if (block->length != length)
> + return -EINVAL;
> + break;
> + }
> + }
> +
> + if (!block) {
> + if (!qemu_ram_alloc(name, length))
> + return -ENOMEM;
> + }
> +
> + total_ram_bytes -= length;
> + }
> }
> }
>
> if (flags& RAM_SAVE_FLAG_COMPRESS) {
> - uint8_t ch = qemu_get_byte(f);
> - memset(qemu_get_ram_ptr(addr), ch, TARGET_PAGE_SIZE);
> + void *host;
> + uint8_t ch;
> +
> + if (version_id == 3) {
> + host = qemu_get_ram_ptr(addr);
> + } else {
> + RAMBlock *block;
> + char name[64];
> +
> + qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
> +
> + QLIST_FOREACH(block,&ram.blocks, next) {
> + if (!strncmp(name, block->name, sizeof(name)))
> + break;
> + }
> + if (!block)
> + return -EINVAL;
> +
> + host = block->host + addr;
> + }
> + ch = qemu_get_byte(f);
> + memset(host, ch, TARGET_PAGE_SIZE);
> #ifndef _WIN32
> if (ch == 0&&
> (!kvm_enabled() || kvm_has_sync_mmu())) {
> - madvise(qemu_get_ram_ptr(addr), TARGET_PAGE_SIZE,
> - MADV_DONTNEED);
> + madvise(host, TARGET_PAGE_SIZE, MADV_DONTNEED);
> }
> #endif
> } else if (flags& RAM_SAVE_FLAG_PAGE) {
> - qemu_get_buffer(f, qemu_get_ram_ptr(addr), TARGET_PAGE_SIZE);
> + void *host;
> +
> + if (version_id == 3) {
> + host = qemu_get_ram_ptr(addr);
> + } else {
> + RAMBlock *block;
> + char name[64];
> +
> + qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
> +
> + QLIST_FOREACH(block,&ram.blocks, next) {
> + if (!strncmp(name, block->name, sizeof(name)))
> + break;
> + }
> + if (!block)
> + return -EINVAL;
> +
> + host = block->host + addr;
> + }
> + qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> }
> if (qemu_file_has_error(f)) {
> return -EIO;
> diff --git a/vl.c b/vl.c
> index 9e9c176..a871895 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3731,7 +3731,7 @@ int main(int argc, char **argv, char **envp)
> if (qemu_opts_foreach(&qemu_drive_opts, drive_init_func, machine, 1) != 0)
> exit(1);
>
> - register_savevm_live("ram", 0, 3, NULL, ram_save_live, NULL,
> + register_savevm_live("ram", 0, 4, NULL, ram_save_live, NULL,
> ram_load, NULL);
>
> if (nb_numa_nodes> 0) {
>
>
^ permalink raw reply [flat|nested] 94+ messages in thread* [Qemu-devel] Re: [RFC PATCH 5/6] savevm: Migrate RAM based on name/offset
@ 2010-06-08 20:12 ` Anthony Liguori
0 siblings, 0 replies; 94+ messages in thread
From: Anthony Liguori @ 2010-06-08 20:12 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, qemu-devel, kvm, quintela
On 06/08/2010 02:16 PM, Alex Williamson wrote:
> Synchronize RAM blocks with the target and migrate using name/offset
> pairs. This ensures both source and target have the same view of
> RAM and that we get the right bits into the right slot.
>
> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
> ---
>
> arch_init.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
> vl.c | 2 +
> 2 files changed, 93 insertions(+), 12 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 6103461..8ba92ec 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -113,20 +113,29 @@ static int ram_save_block(QEMUFile *f)
>
> while (addr< total_ram) {
> if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
> + RAMBlock *block;
> + ram_addr_t offset;
> uint8_t *p;
>
> cpu_physical_memory_reset_dirty(current_addr,
> current_addr + TARGET_PAGE_SIZE,
> MIGRATION_DIRTY_FLAG);
>
> - p = qemu_get_ram_ptr(current_addr);
> + QLIST_FOREACH(block,&ram.blocks, next) {
> + if (current_addr - block->offset< block->length)
> + break;
> + }
> + offset = current_addr - block->offset;
> + p = block->host + offset;
>
> if (is_dup_page(p, *p)) {
> - qemu_put_be64(f, current_addr | RAM_SAVE_FLAG_COMPRESS);
> + qemu_put_be64(f, offset | RAM_SAVE_FLAG_COMPRESS);
> + qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
> qemu_put_byte(f, *p);
>
I think we could use some trickery like use another flag in
current_address to determine whether this was a different section from
the previous section and then only encode the section name if that's true.
Regards,
Anthony Liguori
> bytes_sent = 1;
> } else {
> - qemu_put_be64(f, current_addr | RAM_SAVE_FLAG_PAGE);
> + qemu_put_be64(f, offset | RAM_SAVE_FLAG_PAGE);
> + qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
> qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
> bytes_sent = TARGET_PAGE_SIZE;
> }
> @@ -196,6 +205,7 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
> }
>
> if (stage == 1) {
> + RAMBlock *block;
> uint64_t total_ram = ram_bytes_total();
> bytes_transferred = 0;
>
> @@ -210,6 +220,11 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
> cpu_physical_memory_set_dirty_tracking(1);
>
> qemu_put_be64(f, total_ram | RAM_SAVE_FLAG_MEM_SIZE);
> +
> + QLIST_FOREACH(block,&ram.blocks, next) {
> + qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
> + qemu_put_be64(f, block->length);
> + }
> }
>
> bytes_transferred_last = bytes_transferred;
> @@ -257,7 +272,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> ram_addr_t addr;
> int flags;
>
> - if (version_id != 3) {
> + if (version_id< 3) {
> return -EINVAL;
> }
>
> @@ -268,23 +283,89 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> addr&= TARGET_PAGE_MASK;
>
> if (flags& RAM_SAVE_FLAG_MEM_SIZE) {
> - if (addr != ram_bytes_total()) {
> - return -EINVAL;
> + if (version_id == 3) {
> + if (addr != ram_bytes_total()) {
> + return -EINVAL;
> + }
> + } else {
> + /* Synchronize RAM block list */
> + char name[64];
> + ram_addr_t length;
> + ram_addr_t total_ram_bytes = addr;
> +
> + while (total_ram_bytes) {
> + RAMBlock *block;
> + qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
> + length = qemu_get_be64(f);
> +
> + QLIST_FOREACH(block,&ram.blocks, next) {
> + if (!strncmp(name, block->name, sizeof(name))) {
> + if (block->length != length)
> + return -EINVAL;
> + break;
> + }
> + }
> +
> + if (!block) {
> + if (!qemu_ram_alloc(name, length))
> + return -ENOMEM;
> + }
> +
> + total_ram_bytes -= length;
> + }
> }
> }
>
> if (flags& RAM_SAVE_FLAG_COMPRESS) {
> - uint8_t ch = qemu_get_byte(f);
> - memset(qemu_get_ram_ptr(addr), ch, TARGET_PAGE_SIZE);
> + void *host;
> + uint8_t ch;
> +
> + if (version_id == 3) {
> + host = qemu_get_ram_ptr(addr);
> + } else {
> + RAMBlock *block;
> + char name[64];
> +
> + qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
> +
> + QLIST_FOREACH(block,&ram.blocks, next) {
> + if (!strncmp(name, block->name, sizeof(name)))
> + break;
> + }
> + if (!block)
> + return -EINVAL;
> +
> + host = block->host + addr;
> + }
> + ch = qemu_get_byte(f);
> + memset(host, ch, TARGET_PAGE_SIZE);
> #ifndef _WIN32
> if (ch == 0&&
> (!kvm_enabled() || kvm_has_sync_mmu())) {
> - madvise(qemu_get_ram_ptr(addr), TARGET_PAGE_SIZE,
> - MADV_DONTNEED);
> + madvise(host, TARGET_PAGE_SIZE, MADV_DONTNEED);
> }
> #endif
> } else if (flags& RAM_SAVE_FLAG_PAGE) {
> - qemu_get_buffer(f, qemu_get_ram_ptr(addr), TARGET_PAGE_SIZE);
> + void *host;
> +
> + if (version_id == 3) {
> + host = qemu_get_ram_ptr(addr);
> + } else {
> + RAMBlock *block;
> + char name[64];
> +
> + qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
> +
> + QLIST_FOREACH(block,&ram.blocks, next) {
> + if (!strncmp(name, block->name, sizeof(name)))
> + break;
> + }
> + if (!block)
> + return -EINVAL;
> +
> + host = block->host + addr;
> + }
> + qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> }
> if (qemu_file_has_error(f)) {
> return -EIO;
> diff --git a/vl.c b/vl.c
> index 9e9c176..a871895 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3731,7 +3731,7 @@ int main(int argc, char **argv, char **envp)
> if (qemu_opts_foreach(&qemu_drive_opts, drive_init_func, machine, 1) != 0)
> exit(1);
>
> - register_savevm_live("ram", 0, 3, NULL, ram_save_live, NULL,
> + register_savevm_live("ram", 0, 4, NULL, ram_save_live, NULL,
> ram_load, NULL);
>
> if (nb_numa_nodes> 0) {
>
>
^ permalink raw reply [flat|nested] 94+ messages in thread* Re: [RFC PATCH 5/6] savevm: Migrate RAM based on name/offset
2010-06-08 20:12 ` [Qemu-devel] " Anthony Liguori
@ 2010-06-08 21:12 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 21:12 UTC (permalink / raw)
To: Anthony Liguori; +Cc: qemu-devel, kvm, quintela, chrisw
On Tue, 2010-06-08 at 15:12 -0500, Anthony Liguori wrote:
> On 06/08/2010 02:16 PM, Alex Williamson wrote:
> > if (is_dup_page(p, *p)) {
> > - qemu_put_be64(f, current_addr | RAM_SAVE_FLAG_COMPRESS);
> > + qemu_put_be64(f, offset | RAM_SAVE_FLAG_COMPRESS);
> > + qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
> > qemu_put_byte(f, *p);
> >
>
> I think we could use some trickery like use another flag in
> current_address to determine whether this was a different section from
> the previous section and then only encode the section name if that's true.
Good suggestion, see patch 7/6 ;)
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread* [Qemu-devel] Re: [RFC PATCH 5/6] savevm: Migrate RAM based on name/offset
@ 2010-06-08 21:12 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 21:12 UTC (permalink / raw)
To: Anthony Liguori; +Cc: chrisw, qemu-devel, kvm, quintela
On Tue, 2010-06-08 at 15:12 -0500, Anthony Liguori wrote:
> On 06/08/2010 02:16 PM, Alex Williamson wrote:
> > if (is_dup_page(p, *p)) {
> > - qemu_put_be64(f, current_addr | RAM_SAVE_FLAG_COMPRESS);
> > + qemu_put_be64(f, offset | RAM_SAVE_FLAG_COMPRESS);
> > + qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
> > qemu_put_byte(f, *p);
> >
>
> I think we could use some trickery like use another flag in
> current_address to determine whether this was a different section from
> the previous section and then only encode the section name if that's true.
Good suggestion, see patch 7/6 ;)
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [RFC PATCH 5/6] savevm: Migrate RAM based on name/offset
2010-06-08 19:16 ` [Qemu-devel] " Alex Williamson
@ 2010-06-08 20:54 ` Chris Wright
-1 siblings, 0 replies; 94+ messages in thread
From: Chris Wright @ 2010-06-08 20:54 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel, anthony, kvm, quintela, chrisw
* Alex Williamson (alex.williamson@redhat.com) wrote:
> @@ -257,7 +272,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> ram_addr_t addr;
> int flags;
>
> - if (version_id != 3) {
> + if (version_id < 3) {
> return -EINVAL;
Should we clamp to 3 and 4?
> }
>
> @@ -268,23 +283,89 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> addr &= TARGET_PAGE_MASK;
>
> if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
Does it simplify anything to simply add a new flag?
> - if (addr != ram_bytes_total()) {
> - return -EINVAL;
> + if (version_id == 3) {
> + if (addr != ram_bytes_total()) {
> + return -EINVAL;
> + }
> + } else {
> + /* Synchronize RAM block list */
> + char name[64];
> + ram_addr_t length;
> + ram_addr_t total_ram_bytes = addr;
> +
> + while (total_ram_bytes) {
> + RAMBlock *block;
> + qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
> + length = qemu_get_be64(f);
> +
> + QLIST_FOREACH(block, &ram.blocks, next) {
> + if (!strncmp(name, block->name, sizeof(name))) {
> + if (block->length != length)
> + return -EINVAL;
> + break;
> + }
> + }
> +
> + if (!block) {
> + if (!qemu_ram_alloc(name, length))
> + return -ENOMEM;
Is there any usee to finding blocks in stream such that we simply allocate
them all dynamically?
thanks,
-chris
^ permalink raw reply [flat|nested] 94+ messages in thread* [Qemu-devel] Re: [RFC PATCH 5/6] savevm: Migrate RAM based on name/offset
@ 2010-06-08 20:54 ` Chris Wright
0 siblings, 0 replies; 94+ messages in thread
From: Chris Wright @ 2010-06-08 20:54 UTC (permalink / raw)
To: Alex Williamson; +Cc: chrisw, quintela, qemu-devel, kvm
* Alex Williamson (alex.williamson@redhat.com) wrote:
> @@ -257,7 +272,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> ram_addr_t addr;
> int flags;
>
> - if (version_id != 3) {
> + if (version_id < 3) {
> return -EINVAL;
Should we clamp to 3 and 4?
> }
>
> @@ -268,23 +283,89 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> addr &= TARGET_PAGE_MASK;
>
> if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
Does it simplify anything to simply add a new flag?
> - if (addr != ram_bytes_total()) {
> - return -EINVAL;
> + if (version_id == 3) {
> + if (addr != ram_bytes_total()) {
> + return -EINVAL;
> + }
> + } else {
> + /* Synchronize RAM block list */
> + char name[64];
> + ram_addr_t length;
> + ram_addr_t total_ram_bytes = addr;
> +
> + while (total_ram_bytes) {
> + RAMBlock *block;
> + qemu_get_buffer(f, (uint8_t *)name, sizeof(name));
> + length = qemu_get_be64(f);
> +
> + QLIST_FOREACH(block, &ram.blocks, next) {
> + if (!strncmp(name, block->name, sizeof(name))) {
> + if (block->length != length)
> + return -EINVAL;
> + break;
> + }
> + }
> +
> + if (!block) {
> + if (!qemu_ram_alloc(name, length))
> + return -ENOMEM;
Is there any usee to finding blocks in stream such that we simply allocate
them all dynamically?
thanks,
-chris
^ permalink raw reply [flat|nested] 94+ messages in thread* Re: [RFC PATCH 5/6] savevm: Migrate RAM based on name/offset
2010-06-08 20:54 ` [Qemu-devel] " Chris Wright
@ 2010-06-08 21:22 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 21:22 UTC (permalink / raw)
To: Chris Wright; +Cc: qemu-devel, anthony, kvm, quintela
On Tue, 2010-06-08 at 13:54 -0700, Chris Wright wrote:
> * Alex Williamson (alex.williamson@redhat.com) wrote:
> > @@ -257,7 +272,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> > ram_addr_t addr;
> > int flags;
> >
> > - if (version_id != 3) {
> > + if (version_id < 3) {
> > return -EINVAL;
>
> Should we clamp to 3 and 4?
Yep, definitely a good idea.
> > }
> >
> > @@ -268,23 +283,89 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> > addr &= TARGET_PAGE_MASK;
> >
> > if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
>
> Does it simplify anything to simply add a new flag?
Not that I can see. Appending it to this existing flag conveniently
lets the receiving side know when it's done since SUM(block->length)
equals MEM_SIZE. Let me know if I'm missing an optimization or use case
you're thinking of.
> > + QLIST_FOREACH(block, &ram.blocks, next) {
> > + if (!strncmp(name, block->name, sizeof(name))) {
> > + if (block->length != length)
> > + return -EINVAL;
> > + break;
> > + }
> > + }
> > +
> > + if (!block) {
> > + if (!qemu_ram_alloc(name, length))
> > + return -ENOMEM;
>
> Is there any use to finding blocks in stream such that we simply allocate
> them all dynamically?
Maybe. It seems like we'd be doing a lot of reallocs as we go though.
I think we're pretty locked down once the migration starts, so nothing
should be changing on the source once we get started. If that's the
case (someone correct me if it's not), sending the block list layout
first seems more efficient. Thanks,
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread* [Qemu-devel] Re: [RFC PATCH 5/6] savevm: Migrate RAM based on name/offset
@ 2010-06-08 21:22 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 21:22 UTC (permalink / raw)
To: Chris Wright; +Cc: quintela, qemu-devel, kvm
On Tue, 2010-06-08 at 13:54 -0700, Chris Wright wrote:
> * Alex Williamson (alex.williamson@redhat.com) wrote:
> > @@ -257,7 +272,7 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> > ram_addr_t addr;
> > int flags;
> >
> > - if (version_id != 3) {
> > + if (version_id < 3) {
> > return -EINVAL;
>
> Should we clamp to 3 and 4?
Yep, definitely a good idea.
> > }
> >
> > @@ -268,23 +283,89 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
> > addr &= TARGET_PAGE_MASK;
> >
> > if (flags & RAM_SAVE_FLAG_MEM_SIZE) {
>
> Does it simplify anything to simply add a new flag?
Not that I can see. Appending it to this existing flag conveniently
lets the receiving side know when it's done since SUM(block->length)
equals MEM_SIZE. Let me know if I'm missing an optimization or use case
you're thinking of.
> > + QLIST_FOREACH(block, &ram.blocks, next) {
> > + if (!strncmp(name, block->name, sizeof(name))) {
> > + if (block->length != length)
> > + return -EINVAL;
> > + break;
> > + }
> > + }
> > +
> > + if (!block) {
> > + if (!qemu_ram_alloc(name, length))
> > + return -ENOMEM;
>
> Is there any use to finding blocks in stream such that we simply allocate
> them all dynamically?
Maybe. It seems like we'd be doing a lot of reallocs as we go though.
I think we're pretty locked down once the migration starts, so nothing
should be changing on the source once we get started. If that's the
case (someone correct me if it's not), sending the block list layout
first seems more efficient. Thanks,
Alex
^ permalink raw reply [flat|nested] 94+ messages in thread
* [RFC PATCH 6/6] savevm: Use RAM blocks for basis of migration
2010-06-08 19:14 ` [Qemu-devel] " Alex Williamson
@ 2010-06-08 19:16 ` Alex Williamson
-1 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 19:16 UTC (permalink / raw)
To: qemu-devel, anthony; +Cc: kvm, quintela, chrisw, alex.williamson
We don't want to assume a contiguous address space, so migrate based
on RAM blocks instead of a fixed linear address map.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
arch_init.c | 67 +++++++++++++++++++++++++++++++++++++----------------------
1 files changed, 42 insertions(+), 25 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 8ba92ec..5780195 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -105,27 +105,26 @@ static int is_dup_page(uint8_t *page, uint8_t ch)
static int ram_save_block(QEMUFile *f)
{
- static ram_addr_t current_addr = 0;
- ram_addr_t saved_addr = current_addr;
- ram_addr_t addr = 0;
- uint64_t total_ram = ram_bytes_total();
+ static RAMBlock *last_block = NULL;
+ static ram_addr_t last_offset = 0;
+ RAMBlock *block = last_block;
+ ram_addr_t offset = last_offset;
+ ram_addr_t current_addr;
int bytes_sent = 0;
- while (addr < total_ram) {
+ if (!block)
+ block = QLIST_FIRST(&ram.blocks);
+
+ current_addr = block->offset + offset;
+
+ do {
if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
- RAMBlock *block;
- ram_addr_t offset;
uint8_t *p;
cpu_physical_memory_reset_dirty(current_addr,
current_addr + TARGET_PAGE_SIZE,
MIGRATION_DIRTY_FLAG);
- QLIST_FOREACH(block, &ram.blocks, next) {
- if (current_addr - block->offset < block->length)
- break;
- }
- offset = current_addr - block->offset;
p = block->host + offset;
if (is_dup_page(p, *p)) {
@@ -142,9 +141,21 @@ static int ram_save_block(QEMUFile *f)
break;
}
- addr += TARGET_PAGE_SIZE;
- current_addr = (saved_addr + addr) % total_ram;
- }
+
+ offset += TARGET_PAGE_SIZE;
+ if (offset >= block->length) {
+ offset = 0;
+ block = QLIST_NEXT(block, next);
+ if (!block)
+ block = QLIST_FIRST(&ram.blocks);
+ }
+
+ current_addr = block->offset + offset;
+
+ } while (current_addr != last_block->offset + last_offset);
+
+ last_block = block;
+ last_offset = offset;
return bytes_sent;
}
@@ -153,13 +164,16 @@ static uint64_t bytes_transferred;
static ram_addr_t ram_save_remaining(void)
{
- ram_addr_t addr;
+ RAMBlock *block;
ram_addr_t count = 0;
- uint64_t total_ram = ram_bytes_total();
- for (addr = 0; addr < total_ram; addr += TARGET_PAGE_SIZE) {
- if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
- count++;
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ ram_addr_t addr;
+ for (addr = block->offset; addr < block->offset + block->length;
+ addr += TARGET_PAGE_SIZE) {
+ if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
+ count++;
+ }
}
}
@@ -206,20 +220,23 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
if (stage == 1) {
RAMBlock *block;
- uint64_t total_ram = ram_bytes_total();
bytes_transferred = 0;
/* Make sure all dirty bits are set */
- for (addr = 0; addr < total_ram; addr += TARGET_PAGE_SIZE) {
- if (!cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
- cpu_physical_memory_set_dirty(addr);
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ for (addr = block->offset; addr < block->offset + block->length;
+ addr += TARGET_PAGE_SIZE) {
+ if (!cpu_physical_memory_get_dirty(addr,
+ MIGRATION_DIRTY_FLAG)) {
+ cpu_physical_memory_set_dirty(addr);
+ }
}
}
/* Enable dirty memory tracking */
cpu_physical_memory_set_dirty_tracking(1);
- qemu_put_be64(f, total_ram | RAM_SAVE_FLAG_MEM_SIZE);
+ qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
QLIST_FOREACH(block, &ram.blocks, next) {
qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
^ permalink raw reply related [flat|nested] 94+ messages in thread* [Qemu-devel] [RFC PATCH 6/6] savevm: Use RAM blocks for basis of migration
@ 2010-06-08 19:16 ` Alex Williamson
0 siblings, 0 replies; 94+ messages in thread
From: Alex Williamson @ 2010-06-08 19:16 UTC (permalink / raw)
To: qemu-devel, anthony; +Cc: chrisw, alex.williamson, kvm, quintela
We don't want to assume a contiguous address space, so migrate based
on RAM blocks instead of a fixed linear address map.
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
arch_init.c | 67 +++++++++++++++++++++++++++++++++++++----------------------
1 files changed, 42 insertions(+), 25 deletions(-)
diff --git a/arch_init.c b/arch_init.c
index 8ba92ec..5780195 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -105,27 +105,26 @@ static int is_dup_page(uint8_t *page, uint8_t ch)
static int ram_save_block(QEMUFile *f)
{
- static ram_addr_t current_addr = 0;
- ram_addr_t saved_addr = current_addr;
- ram_addr_t addr = 0;
- uint64_t total_ram = ram_bytes_total();
+ static RAMBlock *last_block = NULL;
+ static ram_addr_t last_offset = 0;
+ RAMBlock *block = last_block;
+ ram_addr_t offset = last_offset;
+ ram_addr_t current_addr;
int bytes_sent = 0;
- while (addr < total_ram) {
+ if (!block)
+ block = QLIST_FIRST(&ram.blocks);
+
+ current_addr = block->offset + offset;
+
+ do {
if (cpu_physical_memory_get_dirty(current_addr, MIGRATION_DIRTY_FLAG)) {
- RAMBlock *block;
- ram_addr_t offset;
uint8_t *p;
cpu_physical_memory_reset_dirty(current_addr,
current_addr + TARGET_PAGE_SIZE,
MIGRATION_DIRTY_FLAG);
- QLIST_FOREACH(block, &ram.blocks, next) {
- if (current_addr - block->offset < block->length)
- break;
- }
- offset = current_addr - block->offset;
p = block->host + offset;
if (is_dup_page(p, *p)) {
@@ -142,9 +141,21 @@ static int ram_save_block(QEMUFile *f)
break;
}
- addr += TARGET_PAGE_SIZE;
- current_addr = (saved_addr + addr) % total_ram;
- }
+
+ offset += TARGET_PAGE_SIZE;
+ if (offset >= block->length) {
+ offset = 0;
+ block = QLIST_NEXT(block, next);
+ if (!block)
+ block = QLIST_FIRST(&ram.blocks);
+ }
+
+ current_addr = block->offset + offset;
+
+ } while (current_addr != last_block->offset + last_offset);
+
+ last_block = block;
+ last_offset = offset;
return bytes_sent;
}
@@ -153,13 +164,16 @@ static uint64_t bytes_transferred;
static ram_addr_t ram_save_remaining(void)
{
- ram_addr_t addr;
+ RAMBlock *block;
ram_addr_t count = 0;
- uint64_t total_ram = ram_bytes_total();
- for (addr = 0; addr < total_ram; addr += TARGET_PAGE_SIZE) {
- if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
- count++;
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ ram_addr_t addr;
+ for (addr = block->offset; addr < block->offset + block->length;
+ addr += TARGET_PAGE_SIZE) {
+ if (cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
+ count++;
+ }
}
}
@@ -206,20 +220,23 @@ int ram_save_live(Monitor *mon, QEMUFile *f, int stage, void *opaque)
if (stage == 1) {
RAMBlock *block;
- uint64_t total_ram = ram_bytes_total();
bytes_transferred = 0;
/* Make sure all dirty bits are set */
- for (addr = 0; addr < total_ram; addr += TARGET_PAGE_SIZE) {
- if (!cpu_physical_memory_get_dirty(addr, MIGRATION_DIRTY_FLAG)) {
- cpu_physical_memory_set_dirty(addr);
+ QLIST_FOREACH(block, &ram.blocks, next) {
+ for (addr = block->offset; addr < block->offset + block->length;
+ addr += TARGET_PAGE_SIZE) {
+ if (!cpu_physical_memory_get_dirty(addr,
+ MIGRATION_DIRTY_FLAG)) {
+ cpu_physical_memory_set_dirty(addr);
+ }
}
}
/* Enable dirty memory tracking */
cpu_physical_memory_set_dirty_tracking(1);
- qemu_put_be64(f, total_ram | RAM_SAVE_FLAG_MEM_SIZE);
+ qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
QLIST_FOREACH(block, &ram.blocks, next) {
qemu_put_buffer(f, (uint8_t *)block->name, sizeof(block->name));
^ permalink raw reply related [flat|nested] 94+ messages in thread