Patchwork [RFC] Mark a device as non-migratable

login
register
mail settings
Submitter Cam Macdonell
Date June 15, 2010, 4:16 p.m.
Message ID <1276618603-24184-1-git-send-email-cam@cs.ualberta.ca>
Download mbox | patch
Permalink /patch/55762/
State New
Headers show

Comments

Cam Macdonell - June 15, 2010, 4:16 p.m.
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(-)
Anthony Liguori - June 15, 2010, 4:32 p.m.
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);
>
Markus Armbruster - June 15, 2010, 5:45 p.m.
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()?
Cam Macdonell - June 15, 2010, 10:26 p.m.
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
Anthony Liguori - June 15, 2010, 10:33 p.m.
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
>
>
Cam Macdonell - June 16, 2010, 5:05 a.m.
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
>>
>>
>
>
Anthony Liguori - June 16, 2010, 12:34 p.m.
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
>>>
>>>
>>>        
>>
>>
Cam Macdonell - June 17, 2010, 4:18 a.m.
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

Patch

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);