| Message ID | 1276618603-24184-1-git-send-email-cam@cs.ualberta.ca |
|---|---|
| State | New |
| Headers | show |
On 06/15/2010 11:16 AM, Cam Macdonell wrote: > How does this look for marking the device as non-migratable? It adds a field > 'no_migrate' to the SaveStateEntry and tests for it in vmstate_save. This would > replace anything that touches memory. > > Cam > > --- > hw/hw.h | 1 + > savevm.c | 32 +++++++++++++++++++++++++++++--- > 2 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/hw/hw.h b/hw/hw.h > index d78d814..7c93f08 100644 > --- a/hw/hw.h > +++ b/hw/hw.h > @@ -263,6 +263,7 @@ int register_savevm_live(const char *idstr, > void *opaque); > > void unregister_savevm(const char *idstr, void *opaque); > +void mark_no_migrate(const char *idstr, void *opaque); > I'm not thrilled with the name but the functionality is spot on. I lack the creativity to offer a better name suggestion :-) Regards, Anthony Liguori > typedef void QEMUResetHandler(void *opaque); > > diff --git a/savevm.c b/savevm.c > index 017695b..2642a9c 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -1023,6 +1023,7 @@ typedef struct SaveStateEntry { > LoadStateHandler *load_state; > const VMStateDescription *vmsd; > void *opaque; > + int no_migrate; > } SaveStateEntry; > > > @@ -1069,6 +1070,7 @@ int register_savevm_live(const char *idstr, > se->load_state = load_state; > se->opaque = opaque; > se->vmsd = NULL; > + se->no_migrate = 0; > > if (instance_id == -1) { > se->instance_id = calculate_new_instance_id(idstr); > @@ -1103,6 +1105,19 @@ void unregister_savevm(const char *idstr, void *opaque) > } > } > > +/* mark a device as not to be migrated, that is the device should be > + unplugged before migration */ > +void mark_no_migrate(const char *idstr, void *opaque) > +{ > + SaveStateEntry *se; > + > + QTAILQ_FOREACH(se,&savevm_handlers, entry) { > + if (strcmp(se->idstr, idstr) == 0&& se->opaque == opaque) { > + se->no_migrate = 1; > + } > + } > +} > + > int vmstate_register_with_alias_id(int instance_id, > const VMStateDescription *vmsd, > void *opaque, int alias_id, > @@ -1277,13 +1292,19 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id) > return vmstate_load_state(f, se->vmsd, se->opaque, version_id); > } > > -static void vmstate_save(QEMUFile *f, SaveStateEntry *se) > +static int vmstate_save(QEMUFile *f, SaveStateEntry *se) > { > + if (se->no_migrate) { > + return -1; > + } > + > if (!se->vmsd) { /* Old style */ > se->save_state(f, se->opaque); > - return; > + return 0; > } > vmstate_save_state(f,se->vmsd, se->opaque); > + > + return 0; > } > > #define QEMU_VM_FILE_MAGIC 0x5145564d > @@ -1377,6 +1398,7 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f) > int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) > { > SaveStateEntry *se; > + int r; > > cpu_synchronize_all_states(); > > @@ -1409,7 +1431,11 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) > qemu_put_be32(f, se->instance_id); > qemu_put_be32(f, se->version_id); > > - vmstate_save(f, se); > + r = vmstate_save(f, se); > + if (r< 0) { > + monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr); > + return r; > + } > } > > qemu_put_byte(f, QEMU_VM_EOF); >
Anthony Liguori <anthony@codemonkey.ws> writes: > On 06/15/2010 11:16 AM, Cam Macdonell wrote: >> How does this look for marking the device as non-migratable? It adds a field >> 'no_migrate' to the SaveStateEntry and tests for it in vmstate_save. This would >> replace anything that touches memory. >> >> Cam >> >> --- >> hw/hw.h | 1 + >> savevm.c | 32 +++++++++++++++++++++++++++++--- >> 2 files changed, 30 insertions(+), 3 deletions(-) >> >> diff --git a/hw/hw.h b/hw/hw.h >> index d78d814..7c93f08 100644 >> --- a/hw/hw.h >> +++ b/hw/hw.h >> @@ -263,6 +263,7 @@ int register_savevm_live(const char *idstr, >> void *opaque); >> >> void unregister_savevm(const char *idstr, void *opaque); >> +void mark_no_migrate(const char *idstr, void *opaque); >> > > I'm not thrilled with the name but the functionality is spot on. I > lack the creativity to offer a better name suggestion :-) Tongue firmly in cheek: mark_sedentary()?
On Tue, Jun 15, 2010 at 10:32 AM, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 06/15/2010 11:16 AM, Cam Macdonell wrote: >> >> How does this look for marking the device as non-migratable? It adds a >> field >> 'no_migrate' to the SaveStateEntry and tests for it in vmstate_save. This >> would >> replace anything that touches memory. >> >> Cam >> >> --- >> hw/hw.h | 1 + >> savevm.c | 32 +++++++++++++++++++++++++++++--- >> 2 files changed, 30 insertions(+), 3 deletions(-) >> >> diff --git a/hw/hw.h b/hw/hw.h >> index d78d814..7c93f08 100644 >> --- a/hw/hw.h >> +++ b/hw/hw.h >> @@ -263,6 +263,7 @@ int register_savevm_live(const char *idstr, >> void *opaque); >> >> void unregister_savevm(const char *idstr, void *opaque); >> +void mark_no_migrate(const char *idstr, void *opaque); >> > > I'm not thrilled with the name but the functionality is spot on. I lack the > creativity to offer a better name suggestion :-) > > Regards, > > Anthony Liguori Hmmm, in working on this it seems that the memory (from qemu_ram_map()) is still attached even when the device is removed (which causes migration to fail because there is an unexpected memory). Is something like cpu_unregister_physical_memory()/qemu_ram_free() needed? Cam
On 06/15/2010 05:26 PM, Cam Macdonell wrote: > On Tue, Jun 15, 2010 at 10:32 AM, Anthony Liguori<anthony@codemonkey.ws> wrote: > >> On 06/15/2010 11:16 AM, Cam Macdonell wrote: >> >>> How does this look for marking the device as non-migratable? It adds a >>> field >>> 'no_migrate' to the SaveStateEntry and tests for it in vmstate_save. This >>> would >>> replace anything that touches memory. >>> >>> Cam >>> >>> --- >>> hw/hw.h | 1 + >>> savevm.c | 32 +++++++++++++++++++++++++++++--- >>> 2 files changed, 30 insertions(+), 3 deletions(-) >>> >>> diff --git a/hw/hw.h b/hw/hw.h >>> index d78d814..7c93f08 100644 >>> --- a/hw/hw.h >>> +++ b/hw/hw.h >>> @@ -263,6 +263,7 @@ int register_savevm_live(const char *idstr, >>> void *opaque); >>> >>> void unregister_savevm(const char *idstr, void *opaque); >>> +void mark_no_migrate(const char *idstr, void *opaque); >>> >>> >> I'm not thrilled with the name but the functionality is spot on. I lack the >> creativity to offer a better name suggestion :-) >> >> Regards, >> >> Anthony Liguori >> > Hmmm, in working on this it seems that the memory (from > qemu_ram_map()) is still attached even when the device is removed > (which causes migration to fail because there is an unexpected > memory). > > Is something like cpu_unregister_physical_memory()/qemu_ram_free() needed? > Yes. You need to unregister any memory that you have registered upon device removal. Regards, Anthony Liguori > Cam > >
On Tue, Jun 15, 2010 at 4:33 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 06/15/2010 05:26 PM, Cam Macdonell wrote: >> >> On Tue, Jun 15, 2010 at 10:32 AM, Anthony Liguori<anthony@codemonkey.ws> >> wrote: >> >>> >>> On 06/15/2010 11:16 AM, Cam Macdonell wrote: >>> >>>> >>>> How does this look for marking the device as non-migratable? It adds a >>>> field >>>> 'no_migrate' to the SaveStateEntry and tests for it in vmstate_save. >>>> This >>>> would >>>> replace anything that touches memory. >>>> >>>> Cam >>>> >>>> --- >>>> hw/hw.h | 1 + >>>> savevm.c | 32 +++++++++++++++++++++++++++++--- >>>> 2 files changed, 30 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/hw/hw.h b/hw/hw.h >>>> index d78d814..7c93f08 100644 >>>> --- a/hw/hw.h >>>> +++ b/hw/hw.h >>>> @@ -263,6 +263,7 @@ int register_savevm_live(const char *idstr, >>>> void *opaque); >>>> >>>> void unregister_savevm(const char *idstr, void *opaque); >>>> +void mark_no_migrate(const char *idstr, void *opaque); >>>> >>>> >>> >>> I'm not thrilled with the name but the functionality is spot on. I lack >>> the >>> creativity to offer a better name suggestion :-) >>> >>> Regards, >>> >>> Anthony Liguori >>> >> >> Hmmm, in working on this it seems that the memory (from >> qemu_ram_map()) is still attached even when the device is removed >> (which causes migration to fail because there is an unexpected >> memory). >> >> Is something like cpu_unregister_physical_memory()/qemu_ram_free() needed? >> > > Yes. You need to unregister any memory that you have registered upon device > removal. Is there an established way to achieve this? I can't seem find another device that unregisters memory registered with cpu_register_physical_memory(). Is something like cpu_unregister_physical_memory() needed? Thanks, Cam > > Regards, > > Anthony Liguori > >> Cam >> >> > >
On 06/16/2010 12:05 AM, Cam Macdonell wrote: > On Tue, Jun 15, 2010 at 4:33 PM, Anthony Liguori<anthony@codemonkey.ws> wrote: > >> On 06/15/2010 05:26 PM, Cam Macdonell wrote: >> >>> On Tue, Jun 15, 2010 at 10:32 AM, Anthony Liguori<anthony@codemonkey.ws> >>> wrote: >>> >>> >>>> On 06/15/2010 11:16 AM, Cam Macdonell wrote: >>>> >>>> >>>>> How does this look for marking the device as non-migratable? It adds a >>>>> field >>>>> 'no_migrate' to the SaveStateEntry and tests for it in vmstate_save. >>>>> This >>>>> would >>>>> replace anything that touches memory. >>>>> >>>>> Cam >>>>> >>>>> --- >>>>> hw/hw.h | 1 + >>>>> savevm.c | 32 +++++++++++++++++++++++++++++--- >>>>> 2 files changed, 30 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/hw/hw.h b/hw/hw.h >>>>> index d78d814..7c93f08 100644 >>>>> --- a/hw/hw.h >>>>> +++ b/hw/hw.h >>>>> @@ -263,6 +263,7 @@ int register_savevm_live(const char *idstr, >>>>> void *opaque); >>>>> >>>>> void unregister_savevm(const char *idstr, void *opaque); >>>>> +void mark_no_migrate(const char *idstr, void *opaque); >>>>> >>>>> >>>>> >>>> I'm not thrilled with the name but the functionality is spot on. I lack >>>> the >>>> creativity to offer a better name suggestion :-) >>>> >>>> Regards, >>>> >>>> Anthony Liguori >>>> >>>> >>> Hmmm, in working on this it seems that the memory (from >>> qemu_ram_map()) is still attached even when the device is removed >>> (which causes migration to fail because there is an unexpected >>> memory). >>> >>> Is something like cpu_unregister_physical_memory()/qemu_ram_free() needed? >>> >>> >> Yes. You need to unregister any memory that you have registered upon device >> removal. >> > Is there an established way to achieve this? I can't seem find > another device that unregisters memory registered with > cpu_register_physical_memory(). Is something like > cpu_unregister_physical_memory() needed? > cpu_register_physical_memory(IO_MEM_UNASSIGNED). If you look at pci.c, you'll see that it automatically unregisters any mapped io regions on remove. Regards, Anthony Liguori > Thanks, > Cam > > >> Regards, >> >> Anthony Liguori >> >> >>> Cam >>> >>> >>> >> >>
On Wed, Jun 16, 2010 at 6:34 AM, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 06/16/2010 12:05 AM, Cam Macdonell wrote: >> >> On Tue, Jun 15, 2010 at 4:33 PM, Anthony Liguori<anthony@codemonkey.ws> >> wrote: >> >>> >>> On 06/15/2010 05:26 PM, Cam Macdonell wrote: >>> >>>> >>>> On Tue, Jun 15, 2010 at 10:32 AM, Anthony Liguori<anthony@codemonkey.ws> >>>> wrote: >>>> >>>> >>>>> >>>>> On 06/15/2010 11:16 AM, Cam Macdonell wrote: >>>>> >>>>> >>>>>> >>>>>> How does this look for marking the device as non-migratable? It adds >>>>>> a >>>>>> field >>>>>> 'no_migrate' to the SaveStateEntry and tests for it in vmstate_save. >>>>>> This >>>>>> would >>>>>> replace anything that touches memory. >>>>>> >>>>>> Cam >>>>>> >>>>>> --- >>>>>> hw/hw.h | 1 + >>>>>> savevm.c | 32 +++++++++++++++++++++++++++++--- >>>>>> 2 files changed, 30 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/hw/hw.h b/hw/hw.h >>>>>> index d78d814..7c93f08 100644 >>>>>> --- a/hw/hw.h >>>>>> +++ b/hw/hw.h >>>>>> @@ -263,6 +263,7 @@ int register_savevm_live(const char *idstr, >>>>>> void *opaque); >>>>>> >>>>>> void unregister_savevm(const char *idstr, void *opaque); >>>>>> +void mark_no_migrate(const char *idstr, void *opaque); >>>>>> >>>>>> >>>>>> >>>>> >>>>> I'm not thrilled with the name but the functionality is spot on. I >>>>> lack >>>>> the >>>>> creativity to offer a better name suggestion :-) >>>>> >>>>> Regards, >>>>> >>>>> Anthony Liguori >>>>> >>>>> >>>> >>>> Hmmm, in working on this it seems that the memory (from >>>> qemu_ram_map()) is still attached even when the device is removed >>>> (which causes migration to fail because there is an unexpected >>>> memory). >>>> >>>> Is something like cpu_unregister_physical_memory()/qemu_ram_free() >>>> needed? >>>> >>>> >>> >>> Yes. You need to unregister any memory that you have registered upon >>> device >>> removal. >>> >> >> Is there an established way to achieve this? I can't seem find >> another device that unregisters memory registered with >> cpu_register_physical_memory(). Is something like >> cpu_unregister_physical_memory() needed? >> > > cpu_register_physical_memory(IO_MEM_UNASSIGNED). > > If you look at pci.c, you'll see that it automatically unregisters any > mapped io regions on remove. > It appears that the 'peer' migration won't work until memory hotplug is supported, correct? AFAICT the memory sizes will not match between the source and destination VMs after the device is removed and the memory system currently doesn't support gaps. A technique similar to my patch for non-migratable memory would be needed to mark free'd memory pages without Alex's patches in. For the purposes of my patch, can it be merged without the 'peer' case (pending Alex's patches and hotplug)? Thanks, Cam
diff --git a/hw/hw.h b/hw/hw.h index d78d814..7c93f08 100644 --- a/hw/hw.h +++ b/hw/hw.h @@ -263,6 +263,7 @@ int register_savevm_live(const char *idstr, void *opaque); void unregister_savevm(const char *idstr, void *opaque); +void mark_no_migrate(const char *idstr, void *opaque); typedef void QEMUResetHandler(void *opaque); diff --git a/savevm.c b/savevm.c index 017695b..2642a9c 100644 --- a/savevm.c +++ b/savevm.c @@ -1023,6 +1023,7 @@ typedef struct SaveStateEntry { LoadStateHandler *load_state; const VMStateDescription *vmsd; void *opaque; + int no_migrate; } SaveStateEntry; @@ -1069,6 +1070,7 @@ int register_savevm_live(const char *idstr, se->load_state = load_state; se->opaque = opaque; se->vmsd = NULL; + se->no_migrate = 0; if (instance_id == -1) { se->instance_id = calculate_new_instance_id(idstr); @@ -1103,6 +1105,19 @@ void unregister_savevm(const char *idstr, void *opaque) } } +/* mark a device as not to be migrated, that is the device should be + unplugged before migration */ +void mark_no_migrate(const char *idstr, void *opaque) +{ + SaveStateEntry *se; + + QTAILQ_FOREACH(se, &savevm_handlers, entry) { + if (strcmp(se->idstr, idstr) == 0 && se->opaque == opaque) { + se->no_migrate = 1; + } + } +} + int vmstate_register_with_alias_id(int instance_id, const VMStateDescription *vmsd, void *opaque, int alias_id, @@ -1277,13 +1292,19 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se, int version_id) return vmstate_load_state(f, se->vmsd, se->opaque, version_id); } -static void vmstate_save(QEMUFile *f, SaveStateEntry *se) +static int vmstate_save(QEMUFile *f, SaveStateEntry *se) { + if (se->no_migrate) { + return -1; + } + if (!se->vmsd) { /* Old style */ se->save_state(f, se->opaque); - return; + return 0; } vmstate_save_state(f,se->vmsd, se->opaque); + + return 0; } #define QEMU_VM_FILE_MAGIC 0x5145564d @@ -1377,6 +1398,7 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f) int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) { SaveStateEntry *se; + int r; cpu_synchronize_all_states(); @@ -1409,7 +1431,11 @@ int qemu_savevm_state_complete(Monitor *mon, QEMUFile *f) qemu_put_be32(f, se->instance_id); qemu_put_be32(f, se->version_id); - vmstate_save(f, se); + r = vmstate_save(f, se); + if (r < 0) { + monitor_printf(mon, "cannot migrate with device '%s'\n", se->idstr); + return r; + } } qemu_put_byte(f, QEMU_VM_EOF);