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

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

[Peter Xu]

This series enables VT-d IOMMU migration.

v3:
- save/load for csr [Jason]

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

-- 
2.7.4

[Qemu-devel] [PATCH v3 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 v3 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       | 22 +++++++++++++++++++++-
 include/migration/vmstate.h |  1 +
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 5f3e351..119217b 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1996,7 +1996,27 @@ 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_ARRAY(csr, IntelIOMMUState, DMAR_REG_SIZE),
+        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 v3 2/2] intel_iommu: allow migration

[Jason Wang]

On 2017年01月06日 12:06, 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       | 22 +++++++++++++++++++++-
>   include/migration/vmstate.h |  1 +
>   2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 5f3e351..119217b 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1996,7 +1996,27 @@ 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_ARRAY(csr, IntelIOMMUState, DMAR_REG_SIZE),

Do we need migrate wmask, w1cmask, womask too?

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

I think we should reset iotlb and context cache in post_load() too.

Thanks

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

[Peter Xu]

On Fri, Jan 06, 2017 at 03:05:47PM +0800, Jason Wang wrote:
> 
> 
> On 2017年01月06日 12:06, 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       | 22 +++++++++++++++++++++-
> >  include/migration/vmstate.h |  1 +
> >  2 files changed, 22 insertions(+), 1 deletion(-)
> >
> >diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >index 5f3e351..119217b 100644
> >--- a/hw/i386/intel_iommu.c
> >+++ b/hw/i386/intel_iommu.c
> >@@ -1996,7 +1996,27 @@ 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_ARRAY(csr, IntelIOMMUState, DMAR_REG_SIZE),
> 
> Do we need migrate wmask, w1cmask, womask too?

Should not. Although we are using arrays here, they should be only
configured during vtd_define_*() and should be constant values for the
whole lifecycle of the device (which is mostly decided by the spec).

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v3 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.
> 
> 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.

So I think this is quite simple and makes sense; all existing stuff
says in the same order, and things of equal priority get added in the
same order.  I suspect if we start doing it to lots of devices
we'll come up with more complex requirements.

> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@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
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> This series enables VT-d IOMMU migration.
> 
> v3:
> - save/load for csr [Jason]
> 
> 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?)

How did it fail?

Dave

> Peter Xu (2):
>   migration: allow to prioritize save state entries
>   intel_iommu: allow migration
> 
>  hw/i386/intel_iommu.c       | 22 +++++++++++++++++++++-
>  include/migration/vmstate.h |  7 +++++++
>  migration/savevm.c          | 34 ++++++++++++++++++++++++++++++----
>  3 files changed, 58 insertions(+), 5 deletions(-)
> 
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Peter Xu]

On Fri, Jan 06, 2017 at 01:27:38PM +0000, Dr. David Alan Gilbert wrote:

[...]

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

The keyboard hangs after migration.

Command line for reference:

$qemu -M q35,kernel-irqchip=split -enable-kvm \
      -netdev user,id=net0 \
      -device e1000,netdev=net0 \
      -m 512M -monitor stdio \
      $incoming /var/lib/libvirt/images/vm1.qcow2

Here $incoming is "" for init vm, and "-incoming tcp:0:6666" for the
dest side. Not sure whether this is reproducable issue. Looks like
some EOI ack is missing.

Host: 4.10.0-rc1+ (I should also tried 4.6.4-301.fc24)
Qemu: latest master

Looking into it.

-- peterx

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

[Peter Xu]

On Mon, Jan 09, 2017 at 10:18:45AM +0800, Peter Xu wrote:
> On Fri, Jan 06, 2017 at 01:27:38PM +0000, Dr. David Alan Gilbert wrote:
> 
> [...]
> 
> > > (P.S. I found that split irqchip cannot work well with migration. Is
> > >  this an known issue?)
> > 
> > How did it fail?
> 
> The keyboard hangs after migration.
> 
> Command line for reference:
> 
> $qemu -M q35,kernel-irqchip=split -enable-kvm \
>       -netdev user,id=net0 \
>       -device e1000,netdev=net0 \
>       -m 512M -monitor stdio \
>       $incoming /var/lib/libvirt/images/vm1.qcow2
> 
> Here $incoming is "" for init vm, and "-incoming tcp:0:6666" for the
> dest side. Not sure whether this is reproducable issue. Looks like
> some EOI ack is missing.
> 
> Host: 4.10.0-rc1+ (I should also tried 4.6.4-301.fc24)
> Qemu: latest master
> 
> Looking into it.

The problem is that we didn't setup the first 24 gsi routing entries
(which is used by the userspace ioapic) in kernel after the migration.
Will post a fix for it later.

-- peterx