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

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

[Peter Xu]

This series enables VT-d IOMMU migration.

v2:
- remove SaveStateEntry.priority, let priority store only in one
  place, which is VMStateDescription. Meanwhile, provide another
  helper to fetch the priority.
- add enum MigrationPriority to unify the ordering, rather than use
  magic numbers like 100 everywhere [Dave]
- fix commit log where proper

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 |  7 +++++++
 migration/savevm.c          | 34 ++++++++++++++++++++++++++++++----
 3 files changed, 57 insertions(+), 5 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 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.

There are requirements 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.

Before this patch, we are possibly achieving the ordering requirement by
an assumption that the ordering will be the same with the ordering that
objects are created. A better way is to mark it out explicitly in the
VMStateDescription table, like what this patch does.

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 |  6 ++++++
 migration/savevm.c          | 34 ++++++++++++++++++++++++++++++----
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1638ee5..1a22887 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -186,6 +186,11 @@ enum VMStateFlags {
     VMS_MULTIPLY_ELEMENTS = 0x4000,
 };
 
+typedef enum {
+    MIG_PRI_DEFAULT = 0,
+    MIG_PRI_MAX,
+} MigrationPriority;
+
 typedef struct {
     const char *name;
     size_t offset;
@@ -207,6 +212,7 @@ struct VMStateDescription {
     int version_id;
     int minimum_version_id;
     int minimum_version_id_old;
+    MigrationPriority 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..f9c06e9 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -532,6 +532,34 @@ static int calculate_compat_instance_id(const char *idstr)
     return instance_id;
 }
 
+static inline MigrationPriority save_state_priority(SaveStateEntry *se)
+{
+    if (se->vmsd) {
+        return se->vmsd->priority;
+    }
+    return MIG_PRI_DEFAULT;
+}
+
+static void savevm_state_handler_insert(SaveStateEntry *nse)
+{
+    MigrationPriority priority = save_state_priority(nse);
+    SaveStateEntry *se;
+
+    assert(priority <= MIG_PRI_MAX);
+
+    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+        if (save_state_priority(se) < 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
@@ -578,8 +606,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;
 }
 
@@ -662,8 +689,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 v2 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 a
priority higher than the default (which PCI devices and other belong).
Migration framework handled the rest.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c       | 21 ++++++++++++++++++++-
 include/migration/vmstate.h |  1 +
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 5f3e351..ea9e526 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 = MIG_PRI_IOMMU,
+    .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 = {
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1a22887..2125829 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -188,6 +188,7 @@ enum VMStateFlags {
 
 typedef enum {
     MIG_PRI_DEFAULT = 0,
+    MIG_PRI_IOMMU,              /* Must happen before PCI devices */
     MIG_PRI_MAX,
 } MigrationPriority;
 
-- 
2.7.4

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

[Jason Wang]

On 2017年01月06日 11:08, Peter Xu wrote:
> 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 a
> priority higher than the default (which PCI devices and other belong).
> Migration framework handled the rest.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/i386/intel_iommu.c       | 21 ++++++++++++++++++++-
>   include/migration/vmstate.h |  1 +
>   2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 5f3e351..ea9e526 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 = MIG_PRI_IOMMU,
> +    .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()
> +    }
>   };

Looks like register values were missed (e.g csr[]) ?

>   
>   static const MemoryRegionOps vtd_mem_ops = {
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1a22887..2125829 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -188,6 +188,7 @@ enum VMStateFlags {
>   
>   typedef enum {
>       MIG_PRI_DEFAULT = 0,
> +    MIG_PRI_IOMMU,              /* Must happen before PCI devices */
>       MIG_PRI_MAX,
>   } MigrationPriority;
>   

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

[Peter Xu]

On Fri, Jan 06, 2017 at 11:42:30AM +0800, Jason Wang wrote:
> 
> 
> On 2017年01月06日 11:08, Peter Xu wrote:
> >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 a
> >priority higher than the default (which PCI devices and other belong).
> >Migration framework handled the rest.
> >
> >Signed-off-by: Peter Xu <peterx@redhat.com>
> >---
> >  hw/i386/intel_iommu.c       | 21 ++++++++++++++++++++-
> >  include/migration/vmstate.h |  1 +
> >  2 files changed, 21 insertions(+), 1 deletion(-)
> >
> >diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >index 5f3e351..ea9e526 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 = MIG_PRI_IOMMU,
> >+    .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()
> >+    }
> >  };
> 
> Looks like register values were missed (e.g csr[]) ?

Oops, I missed the most important thing. :)

And obviously it even escaped from smoke test... Thanks Jason. Will
repost.

-- peterx