[Qemu-devel] [PATCH 0/2] VT-d migration support

[Qemu-devel] [PATCH 0/2] VT-d migration support

[Peter Xu]

This series enables VT-d IOMMU migration.

First patch is an enhancement to current migration framework to allow
prioritized save state entry. This is required to let VT-d migrate
properly.

The second patch enables the migration for the vIOMMU.

Please check commit message for more information.

Test done:

Smoke test is done with parameter:

  $qemu -M q35,kernel-irqchip=off -enable-kvm \
        -device intel-iommu,intremap=on \
        -netdev user,id=net0 \
        -device e1000,netdev=net0 \
        -m 512M -monitor stdio \
        $param /var/lib/libvirt/images/vm1.qcow2

Please review, thanks.

(P.S. I found that split irqchip cannot work well with migration. Is
 this an known issue?)

Peter Xu (2):
  migration: allow to prioritize save state entries
  intel_iommu: allow migration

 hw/i386/intel_iommu.c       | 21 ++++++++++++++++++++-
 include/migration/vmstate.h |  1 +
 migration/savevm.c          | 27 +++++++++++++++++++++++----
 3 files changed, 44 insertions(+), 5 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] migration: allow to prioritize save state entries

[Peter Xu]

During migration, save state entries are saved/loaded without a specific
order - we just traverse the savevm_state.handlers list and do it one by
one. This might not be enough in the future.

There is case that we need to load specific device's vmstate first
before others. For example, VT-d IOMMU contains DMA address remapping
information, which is required by all the PCI devices to do address
translations. We need to make sure IOMMU's device state is loaded before
the rest of the PCI devices, so that DMA address translation can work
properly.

This patch provide a VMStateDescription.priority value to allow specify
the priority of the saved states. The loadvm operation will be done with
those devices with higher vmsd priority.

Current ordering logic is still naive and slow, but after all that's not
a critical path so IMO it's a workable solution for now.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/vmstate.h |  1 +
 migration/savevm.c          | 27 +++++++++++++++++++++++----
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1638ee5..dd5e26a 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -207,6 +207,7 @@ struct VMStateDescription {
     int version_id;
     int minimum_version_id;
     int minimum_version_id_old;
+    int priority;
     LoadStateHandler *load_state_old;
     int (*pre_load)(void *opaque);
     int (*post_load)(void *opaque, int version_id);
diff --git a/migration/savevm.c b/migration/savevm.c
index 0363372..93a2837 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -257,6 +257,7 @@ typedef struct SaveStateEntry {
     void *opaque;
     CompatEntry *compat;
     int is_ram;
+    int priority;
 } SaveStateEntry;
 
 typedef struct SaveState {
@@ -532,6 +533,23 @@ static int calculate_compat_instance_id(const char *idstr)
     return instance_id;
 }
 
+static void savevm_state_handler_insert(SaveStateEntry *nse)
+{
+    SaveStateEntry *se;
+
+    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+        if (se->priority < nse->priority) {
+            break;
+        }
+    }
+
+    if (se) {
+        QTAILQ_INSERT_BEFORE(se, nse, entry);
+    } else {
+        QTAILQ_INSERT_TAIL(&savevm_state.handlers, nse, entry);
+    }
+}
+
 /* TODO: Individual devices generally have very little idea about the rest
    of the system, so instance_id should be removed/replaced.
    Meanwhile pass -1 as instance_id if you do not already have a clearly
@@ -551,6 +569,8 @@ int register_savevm_live(DeviceState *dev,
     se->ops = ops;
     se->opaque = opaque;
     se->vmsd = NULL;
+    se->priority = 0;
+
     /* if this is a live_savem then set is_ram */
     if (ops->save_live_setup != NULL) {
         se->is_ram = 1;
@@ -578,8 +598,7 @@ int register_savevm_live(DeviceState *dev,
         se->instance_id = instance_id;
     }
     assert(!se->compat || se->instance_id == 0);
-    /* add at the end of list */
-    QTAILQ_INSERT_TAIL(&savevm_state.handlers, se, entry);
+    savevm_state_handler_insert(se);
     return 0;
 }
 
@@ -639,6 +658,7 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
     se->opaque = opaque;
     se->vmsd = vmsd;
     se->alias_id = alias_id;
+    se->priority = vmsd->priority;
 
     if (dev) {
         char *id = qdev_get_dev_path(dev);
@@ -662,8 +682,7 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
         se->instance_id = instance_id;
     }
     assert(!se->compat || se->instance_id == 0);
-    /* add at the end of list */
-    QTAILQ_INSERT_TAIL(&savevm_state.handlers, se, entry);
+    savevm_state_handler_insert(se);
     return 0;
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] intel_iommu: allow migration

[Peter Xu]

IOMMU needs to be migrated before all the PCI devices (in case there are
devices that will request for address translation). So marking it with
priority 100. Migration framework helped to do all the rest.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 5f3e351..535ed2c 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1996,7 +1996,26 @@ static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu,
 
 static const VMStateDescription vtd_vmstate = {
     .name = "iommu-intel",
-    .unmigratable = 1,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .priority = 100,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(root, IntelIOMMUState),
+        VMSTATE_UINT64(intr_root, IntelIOMMUState),
+        VMSTATE_UINT64(iq, IntelIOMMUState),
+        VMSTATE_UINT32(intr_size, IntelIOMMUState),
+        VMSTATE_UINT16(iq_head, IntelIOMMUState),
+        VMSTATE_UINT16(iq_tail, IntelIOMMUState),
+        VMSTATE_UINT16(iq_size, IntelIOMMUState),
+        VMSTATE_UINT16(next_frcd_reg, IntelIOMMUState),
+        VMSTATE_UINT8(iq_last_desc_type, IntelIOMMUState),
+        VMSTATE_BOOL(root_extended, IntelIOMMUState),
+        VMSTATE_BOOL(dmar_enabled, IntelIOMMUState),
+        VMSTATE_BOOL(qi_enabled, IntelIOMMUState),
+        VMSTATE_BOOL(intr_enabled, IntelIOMMUState),
+        VMSTATE_BOOL(intr_eime, IntelIOMMUState),
+        VMSTATE_END_OF_LIST()
+    }
 };
 
 static const MemoryRegionOps vtd_mem_ops = {
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] migration: allow to prioritize save state entries

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> During migration, save state entries are saved/loaded without a specific
> order - we just traverse the savevm_state.handlers list and do it one by
> one. This might not be enough in the future.
> 
> There is case that we need to load specific device's vmstate first
> before others. For example, VT-d IOMMU contains DMA address remapping
> information, which is required by all the PCI devices to do address
> translations. We need to make sure IOMMU's device state is loaded before
> the rest of the PCI devices, so that DMA address translation can work
> properly.
> 
> This patch provide a VMStateDescription.priority value to allow specify
> the priority of the saved states. The loadvm operation will be done with
> those devices with higher vmsd priority.
> 
> Current ordering logic is still naive and slow, but after all that's not
> a critical path so IMO it's a workable solution for now.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/migration/vmstate.h |  1 +
>  migration/savevm.c          | 27 +++++++++++++++++++++++----
>  2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1638ee5..dd5e26a 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -207,6 +207,7 @@ struct VMStateDescription {
>      int version_id;
>      int minimum_version_id;
>      int minimum_version_id_old;
> +    int priority;

Would it be possible to make this an 'enum' and define
a migration_priority_default  then you can add
your migration_priority_iommu  rather than the magic '100';
so we'd then end up with something like:

enum migration_priority {
   migration_priority_default = 0,
   migration_priority_iommu,       /* Must happen before PCI devices */
}

and that way we'd have one place where we could see all
the priorities next to each other.

I know there are some other existing ordering requirements that happen
to work because of the order devices are created - however
I dont think they're documented anywhere and I don't think any one knows
them all!

Dave

>      LoadStateHandler *load_state_old;
>      int (*pre_load)(void *opaque);
>      int (*post_load)(void *opaque, int version_id);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 0363372..93a2837 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -257,6 +257,7 @@ typedef struct SaveStateEntry {
>      void *opaque;
>      CompatEntry *compat;
>      int is_ram;
> +    int priority;
>  } SaveStateEntry;
>  
>  typedef struct SaveState {
> @@ -532,6 +533,23 @@ static int calculate_compat_instance_id(const char *idstr)
>      return instance_id;
>  }
>  
> +static void savevm_state_handler_insert(SaveStateEntry *nse)
> +{
> +    SaveStateEntry *se;
> +
> +    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> +        if (se->priority < nse->priority) {
> +            break;
> +        }
> +    }
> +
> +    if (se) {
> +        QTAILQ_INSERT_BEFORE(se, nse, entry);
> +    } else {
> +        QTAILQ_INSERT_TAIL(&savevm_state.handlers, nse, entry);
> +    }
> +}
> +
>  /* TODO: Individual devices generally have very little idea about the rest
>     of the system, so instance_id should be removed/replaced.
>     Meanwhile pass -1 as instance_id if you do not already have a clearly
> @@ -551,6 +569,8 @@ int register_savevm_live(DeviceState *dev,
>      se->ops = ops;
>      se->opaque = opaque;
>      se->vmsd = NULL;
> +    se->priority = 0;
> +
>      /* if this is a live_savem then set is_ram */
>      if (ops->save_live_setup != NULL) {
>          se->is_ram = 1;
> @@ -578,8 +598,7 @@ int register_savevm_live(DeviceState *dev,
>          se->instance_id = instance_id;
>      }
>      assert(!se->compat || se->instance_id == 0);
> -    /* add at the end of list */
> -    QTAILQ_INSERT_TAIL(&savevm_state.handlers, se, entry);
> +    savevm_state_handler_insert(se);
>      return 0;
>  }
>  
> @@ -639,6 +658,7 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
>      se->opaque = opaque;
>      se->vmsd = vmsd;
>      se->alias_id = alias_id;
> +    se->priority = vmsd->priority;
>  
>      if (dev) {
>          char *id = qdev_get_dev_path(dev);
> @@ -662,8 +682,7 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id,
>          se->instance_id = instance_id;
>      }
>      assert(!se->compat || se->instance_id == 0);
> -    /* add at the end of list */
> -    QTAILQ_INSERT_TAIL(&savevm_state.handlers, se, entry);
> +    savevm_state_handler_insert(se);
>      return 0;
>  }
>  
> -- 
> 2.7.4
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/2] migration: allow to prioritize save state entries

[Peter Xu]

On Thu, Jan 05, 2017 at 12:49:59PM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > During migration, save state entries are saved/loaded without a specific
> > order - we just traverse the savevm_state.handlers list and do it one by
> > one. This might not be enough in the future.
> > 
> > There is case that we need to load specific device's vmstate first
> > before others. For example, VT-d IOMMU contains DMA address remapping
> > information, which is required by all the PCI devices to do address
> > translations. We need to make sure IOMMU's device state is loaded before
> > the rest of the PCI devices, so that DMA address translation can work
> > properly.
> > 
> > This patch provide a VMStateDescription.priority value to allow specify
> > the priority of the saved states. The loadvm operation will be done with
> > those devices with higher vmsd priority.
> > 
> > Current ordering logic is still naive and slow, but after all that's not
> > a critical path so IMO it's a workable solution for now.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/migration/vmstate.h |  1 +
> >  migration/savevm.c          | 27 +++++++++++++++++++++++----
> >  2 files changed, 24 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index 1638ee5..dd5e26a 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -207,6 +207,7 @@ struct VMStateDescription {
> >      int version_id;
> >      int minimum_version_id;
> >      int minimum_version_id_old;
> > +    int priority;
> 
> Would it be possible to make this an 'enum' and define
> a migration_priority_default  then you can add
> your migration_priority_iommu  rather than the magic '100';
> so we'd then end up with something like:
> 
> enum migration_priority {
>    migration_priority_default = 0,
>    migration_priority_iommu,       /* Must happen before PCI devices */
> }
> 
> and that way we'd have one place where we could see all
> the priorities next to each other.

Yes this sounds better. :-)

> 
> I know there are some other existing ordering requirements that happen
> to work because of the order devices are created - however
> I dont think they're documented anywhere and I don't think any one knows
> them all!

Thank you for solving this "unsolved mistery" for me since I felt like
we should have such an ordering before but failed to find it... Looks
like it's hard to do this ordering thing once and for all, then let's
do it starting from this VT-d migration.

Let me prepare another version. Thanks Dave!

-- peterx