[Qemu-devel] [RFC 0/4] vITS save/restore

[Qemu-devel] [RFC 0/4] vITS save/restore

[Eric Auger]

This series allows ITS save/restore and migration use cases.
It relies on not upstreamed kernel series ([1] & [2]) and
QEMU not upstreamed series [3].

ITS tables are flushed into guest RAM on VM stop while registers
are save on pre_save() callback. Tables and registers are restored
on ITS post_load().

The code is based on Vijaya's "GICv3 live migration support"
series [3], based on v2.8.0-rc4.

The series also uses one patch from Peter Xu that aims at allowing
the explicit ordering of save/restore handlers [4], now upstreamed.

That work was tested on Cavium ThunderX using virsh save/restore and
virt-manager live migration.

Best Regards

Eric

Host Kernel dependencies:
- [1] [RFC 00/13] vITS save/restore
  (https://www.spinics.net/lists/arm-kernel/msg553854.html)
- [2] [PATCH v10 0/8] arm/arm64: vgic: Implement API for vGICv3 live
  migration
  http://www.spinics.net/lists/arm-kernel/msg546383.html

QEMU dependencies:
- [3] [PATCH v6 0/2] GICv3 live migration support
- [4] migration: allow to prioritize save state entries

This QEMU series can be found at:
https://github.com/eauger/qemu/tree/mig-gicv3-v6-its-rfc

Eric Auger (4):
  linux-headers: Partial update for vITS save/restore
  hw/intc/arm_gicv3_kvm: Rename KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS
  hw/intc/arm_gicv3_its: Implement state save/restore
  hw/intc/arm_gicv3_its: Allow save/restore

 hw/intc/arm_gicv3_common.c             |  1 +
 hw/intc/arm_gicv3_its_common.c         | 11 +++-
 hw/intc/arm_gicv3_its_kvm.c            | 94 +++++++++++++++++++++++++++++++++-
 hw/intc/arm_gicv3_kvm.c                |  2 +-
 include/hw/intc/arm_gicv3_its_common.h |  6 +++
 include/migration/vmstate.h            |  2 +
 linux-headers/asm-arm/kvm.h            | 25 ++++++---
 linux-headers/asm-arm64/kvm.h          | 14 +++--
 8 files changed, 140 insertions(+), 15 deletions(-)

-- 
2.5.5

[Qemu-devel] [RFC 1/4] linux-headers: Partial update for vITS save/restore

[Eric Auger]

This is a partial update aiming at enhancing the KVM user
API with vITS save/restore capability. This consists in two
new groups for the ARM_VGIC_ITS KVM device, named:
KVM_DEV_ARM_VGIC_GRP_ITS_REGS, KVM_DEV_ARM_VGIC_GRP_ITS_TABLES.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---
The goal is to import KVM_DEV_ARM_VGIC_GRP_ITS_REGS. Applying
scripts/update-linux-headers.sh pulls other diffs and especially
a rename of KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS into
KVM_DEV_ARM_VGIC_CPU_SYSREGS.

Conflicts:
	linux-headers/asm-arm/kvm.h
---
 linux-headers/asm-arm/kvm.h   | 25 +++++++++++++++++++------
 linux-headers/asm-arm64/kvm.h | 14 +++++++++-----
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/linux-headers/asm-arm/kvm.h b/linux-headers/asm-arm/kvm.h
index e3dd0e1..84fdfb0 100644
--- a/linux-headers/asm-arm/kvm.h
+++ b/linux-headers/asm-arm/kvm.h
@@ -84,6 +84,15 @@ struct kvm_regs {
 #define KVM_VGIC_V2_DIST_SIZE		0x1000
 #define KVM_VGIC_V2_CPU_SIZE		0x2000
 
+/* Supported VGICv3 address types  */
+#define KVM_VGIC_V3_ADDR_TYPE_DIST	2
+#define KVM_VGIC_V3_ADDR_TYPE_REDIST	3
+#define KVM_VGIC_ITS_ADDR_TYPE		4
+
+#define KVM_VGIC_V3_DIST_SIZE		SZ_64K
+#define KVM_VGIC_V3_REDIST_SIZE		(2 * SZ_64K)
+#define KVM_VGIC_V3_ITS_SIZE		(2 * SZ_64K)
+
 #define KVM_ARM_VCPU_POWER_OFF		0 /* CPU is started in OFF state */
 #define KVM_ARM_VCPU_PSCI_0_2		1 /* CPU uses PSCI v0.2 */
 
@@ -174,20 +183,22 @@ struct kvm_arch_memory_slot {
 #define   KVM_DEV_ARM_VGIC_CPUID_MASK	(0xffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
 #define   KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT 32
 #define   KVM_DEV_ARM_VGIC_V3_MPIDR_MASK \
-                       (0xffffffffULL << KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
+			(0xffffffffULL << KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
 #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT	0
 #define   KVM_DEV_ARM_VGIC_OFFSET_MASK	(0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
 #define   KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0xffff)
 #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS	3
 #define KVM_DEV_ARM_VGIC_GRP_CTRL       4
 #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
-#define KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS 6
+#define KVM_DEV_ARM_VGIC_CPU_SYSREGS    6
 #define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7
-#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT 10
+#define KVM_DEV_ARM_VGIC_GRP_ITS_REGS	8
+#define KVM_DEV_ARM_VGIC_GRP_ITS_TABLES	9
+#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT	10
 #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \
-                       (0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT)
-#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff
-#define VGIC_LEVEL_INFO_LINE_LEVEL     0
+			(0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT)
+#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK	0x3ff
+#define VGIC_LEVEL_INFO_LINE_LEVEL	0
 
 #define   KVM_DEV_ARM_VGIC_CTRL_INIT    0
 
@@ -213,7 +224,9 @@ struct kvm_arch_memory_slot {
  * and only here to provide source code level compatibility with older
  * userland. The highest SPI number can be set via KVM_DEV_ARM_VGIC_GRP_NR_IRQS.
  */
+#ifndef __KERNEL__
 #define KVM_ARM_IRQ_GIC_MAX		127
+#endif
 
 /* One single KVM irqchip, ie. the VGIC */
 #define KVM_NR_IRQCHIPS          1
diff --git a/linux-headers/asm-arm64/kvm.h b/linux-headers/asm-arm64/kvm.h
index 6698bdd..31e1137 100644
--- a/linux-headers/asm-arm64/kvm.h
+++ b/linux-headers/asm-arm64/kvm.h
@@ -203,19 +203,21 @@ struct kvm_arch_memory_slot {
 #define   KVM_DEV_ARM_VGIC_CPUID_MASK	(0xffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
 #define   KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT 32
 #define   KVM_DEV_ARM_VGIC_V3_MPIDR_MASK \
-                       (0xffffffffULL << KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
+			(0xffffffffULL << KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
 #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT	0
 #define   KVM_DEV_ARM_VGIC_OFFSET_MASK	(0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
 #define   KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0xffff)
 #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS	3
 #define KVM_DEV_ARM_VGIC_GRP_CTRL	4
 #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
-#define KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS 6
+#define KVM_DEV_ARM_VGIC_CPU_SYSREGS    6
 #define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7
-#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT 10
+#define KVM_DEV_ARM_VGIC_GRP_ITS_REGS 8
+#define KVM_DEV_ARM_VGIC_GRP_ITS_TABLES 9
+#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT	10
 #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \
-                       (0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT)
-#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff
+			(0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT)
+#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK	0x3ff
 #define VGIC_LEVEL_INFO_LINE_LEVEL	0
 
 #define   KVM_DEV_ARM_VGIC_CTRL_INIT	0
@@ -247,7 +249,9 @@ struct kvm_arch_memory_slot {
  * and only here to provide source code level compatibility with older
  * userland. The highest SPI number can be set via KVM_DEV_ARM_VGIC_GRP_NR_IRQS.
  */
+#ifndef __KERNEL__
 #define KVM_ARM_IRQ_GIC_MAX		127
+#endif
 
 /* One single KVM irqchip, ie. the VGIC */
 #define KVM_NR_IRQCHIPS          1
-- 
2.5.5

[Qemu-devel] [RFC 2/4] hw/intc/arm_gicv3_kvm: Rename KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS

[Eric Auger]

Rename KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS into KVM_DEV_ARM_VGIC_CPU_SYSREGS
as exposed in the kernel user API and pulled by update-linux-headers.sh.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

KVM_DEV_ARM_VGIC_CPU_SYSREGS may be fixed at kernel level instead
---
 hw/intc/arm_gicv3_kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 267c2d6..d8ecbc3 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -108,7 +108,7 @@ static inline void kvm_gicr_access(GICv3State *s, int offset, int cpu,
 static inline void kvm_gicc_access(GICv3State *s, uint64_t reg, int cpu,
                                    uint64_t *val, bool write)
 {
-    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_CPU_SYSREGS,
                       KVM_VGIC_ATTR(reg, s->cpu[cpu].gicr_typer),
                       val, write);
 }
-- 
2.5.5

[Qemu-devel] [RFC 3/4] hw/intc/arm_gicv3_its: Implement state save/restore

[Eric Auger]

We need to handle both registers and ITS tables. While
register handling is standard, ITS table handling is more
challenging since the kernel API is devised so that the
tables are flushed into guest RAM and not in vmstate buffers.

Flushing the ITS tables on device pre_save() is too late
since the guest RAM had already been saved at this point.

Table flushing needs to happen when we are sure the vcpus
are stopped and before the last dirty page saving. The
right point is RUN_STATE_FINISH_MIGRATE but sometimes the
VM gets stopped before migration launch so let's simply
flush the tables each time the VM gets stopped.

For regular ITS registers we just can use vmstate pre_save
and post_load callbacks.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---
---
 hw/intc/arm_gicv3_its_common.c         |  8 ++++
 hw/intc/arm_gicv3_its_kvm.c            | 86 ++++++++++++++++++++++++++++++++++
 include/hw/intc/arm_gicv3_its_common.h |  6 +++
 3 files changed, 100 insertions(+)

diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
index 9d67c5c..75b9f04 100644
--- a/hw/intc/arm_gicv3_its_common.c
+++ b/hw/intc/arm_gicv3_its_common.c
@@ -49,6 +49,14 @@ static const VMStateDescription vmstate_its = {
     .pre_save = gicv3_its_pre_save,
     .post_load = gicv3_its_post_load,
     .unmigratable = true,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(ctlr, GICv3ITSState),
+        VMSTATE_UINT64(cbaser, GICv3ITSState),
+        VMSTATE_UINT64(cwriter, GICv3ITSState),
+        VMSTATE_UINT64(creadr, GICv3ITSState),
+        VMSTATE_UINT64_ARRAY(baser, GICv3ITSState, 8),
+        VMSTATE_END_OF_LIST()
+    },
 };
 
 static MemTxResult gicv3_its_trans_read(void *opaque, hwaddr offset,
diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index fc246e0..3f8017d 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -53,6 +53,24 @@ static int kvm_its_send_msi(GICv3ITSState *s, uint32_t value, uint16_t devid)
     return kvm_vm_ioctl(kvm_state, KVM_SIGNAL_MSI, &msi);
 }
 
+/**
+ * vm_change_state_handler - VM change state callback aiming at flushing
+ * ITS tables into guest RAM
+ *
+ * The tables get flushed to guest RAM whenever the VM gets stopped.
+ */
+static void vm_change_state_handler(void *opaque, int running,
+                                    RunState state)
+{
+    GICv3ITSState *s = (GICv3ITSState *)opaque;
+
+    if (running) {
+        return;
+    }
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_TABLES,
+                      0, NULL, false);
+}
+
 static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
 {
     GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
@@ -83,6 +101,8 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
     kvm_msi_use_devid = true;
     kvm_gsi_direct_mapping = false;
     kvm_msi_via_irqfd_allowed = kvm_irqfds_enabled();
+
+    qemu_add_vm_change_state_handler(vm_change_state_handler, s);
 }
 
 static void kvm_arm_its_init(Object *obj)
@@ -96,6 +116,70 @@ static void kvm_arm_its_init(Object *obj)
                              &error_abort);
 }
 
+/**
+ * kvm_arm_its_get - handles the saving of ITS registers.
+ * ITS tables, being flushed into guest RAM needs to be saved before
+ * the pre_save() callback, hence the migration state change notifiers
+ */
+static void kvm_arm_its_get(GICv3ITSState *s)
+{
+    uint64_t reg;
+    int i;
+
+    for (i = 0; i < 8; i++) {
+        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                          GITS_BASER + i * 8, &s->baser[i], false);
+    }
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CTLR, &reg, false);
+    s->ctlr = extract64(reg, 0, 32);
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CBASER, &s->cbaser, false);
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CREADR, &s->creadr, false);
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CWRITER, &s->cwriter, false);
+}
+
+/**
+ * kvm_arm_its_put - Restore both the ITS registers and guest RAM tables
+ * ITS tables, being flushed into guest RAM needs to be saved before
+ * the pre_save() callback. The restoration order matters since there
+ * are dependencies between register settings, as specified by the
+ * architecture specification
+ */
+static void kvm_arm_its_put(GICv3ITSState *s)
+{
+    uint64_t reg;
+    int i;
+
+    /* must be written before GITS_CREADR since it resets this latter*/
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CBASER, &s->cbaser, true);
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CREADR, &s->creadr, true);
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CWRITER, &s->cwriter, true);
+
+    for (i = 0; i < 8; i++) {
+        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                          GITS_BASER + i * 8, &s->baser[i], true);
+    }
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_TABLES,
+                      0, NULL, true);
+
+    reg = s->ctlr;
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CTLR, &reg, true);
+}
+
 static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -103,6 +187,8 @@ static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
 
     dc->realize = kvm_arm_its_realize;
     icc->send_msi = kvm_its_send_msi;
+    icc->pre_save = kvm_arm_its_get;
+    icc->post_load = kvm_arm_its_put;
 }
 
 static const TypeInfo kvm_arm_its_info = {
diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
index 1ba1894..ed5d6df 100644
--- a/include/hw/intc/arm_gicv3_its_common.h
+++ b/include/hw/intc/arm_gicv3_its_common.h
@@ -28,6 +28,12 @@
 #define ITS_TRANS_SIZE   0x10000
 #define ITS_SIZE         (ITS_CONTROL_SIZE + ITS_TRANS_SIZE)
 
+#define GITS_CTLR        0x0
+#define GITS_CBASER      0x80
+#define GITS_CWRITER     0x88
+#define GITS_CREADR      0x90
+#define GITS_BASER       0x100
+
 struct GICv3ITSState {
     SysBusDevice parent_obj;
 
-- 
2.5.5

[Qemu-devel] [RFC 4/4] hw/intc/arm_gicv3_its: Allow save/restore

[Eric Auger]

We change the restoration priority of both the GICv3 and ITS. The
GICv3 must be restored before the ITS and the ITS needs to be restored
before PCIe devices since it translates their MSI transactions.

We typically observe the virtio-pci-net device sending MSI transactions
very early (even before the first vcpu run) which looks weird. It
appears that not servicing those transactions cause the virtio-pci-net
to stall.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/intc/arm_gicv3_common.c     | 1 +
 hw/intc/arm_gicv3_its_common.c | 3 ++-
 hw/intc/arm_gicv3_its_kvm.c    | 8 ++++++--
 include/migration/vmstate.h    | 2 ++
 4 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 0f8c4b8..f80e60d 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -84,6 +84,7 @@ static const VMStateDescription vmstate_gicv3 = {
     .minimum_version_id = 1,
     .pre_save = gicv3_pre_save,
     .post_load = gicv3_post_load,
+    .priority = MIG_PRI_GICV3,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(gicd_ctlr, GICv3State),
         VMSTATE_UINT32_ARRAY(gicd_statusr, GICv3State, 2),
diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
index 75b9f04..854709f 100644
--- a/hw/intc/arm_gicv3_its_common.c
+++ b/hw/intc/arm_gicv3_its_common.c
@@ -48,7 +48,8 @@ static const VMStateDescription vmstate_its = {
     .name = "arm_gicv3_its",
     .pre_save = gicv3_its_pre_save,
     .post_load = gicv3_its_post_load,
-    .unmigratable = true,
+    .unmigratable = false,
+    .priority = MIG_PRI_GICV3_ITS,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(ctlr, GICv3ITSState),
         VMSTATE_UINT64(cbaser, GICv3ITSState),
diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index 3f8017d..7f81d33 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -95,8 +95,12 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
      * Block migration of a KVM GICv3 ITS device: the API for saving and
      * restoring the state in the kernel is not yet available
      */
-    error_setg(&s->migration_blocker, "vITS migration is not implemented");
-    migrate_add_blocker(s->migration_blocker);
+    if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                               GITS_CTLR)) {
+        error_setg(&s->migration_blocker, "This operating system kernel does "
+                                          "not support vITS migration");
+        migrate_add_blocker(s->migration_blocker);
+    }
 
     kvm_msi_use_devid = true;
     kvm_gsi_direct_mapping = false;
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1a22887..ebd755c 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -188,6 +188,8 @@ enum VMStateFlags {
 
 typedef enum {
     MIG_PRI_DEFAULT = 0,
+    MIG_PRI_GICV3_ITS,
+    MIG_PRI_GICV3,
     MIG_PRI_MAX,
 } MigrationPriority;
 
-- 
2.5.5

Re: [Qemu-devel] [RFC 4/4] hw/intc/arm_gicv3_its: Allow save/restore

[Dr. David Alan Gilbert]

* Eric Auger (eric.auger@redhat.com) wrote:
> We change the restoration priority of both the GICv3 and ITS. The
> GICv3 must be restored before the ITS and the ITS needs to be restored
> before PCIe devices since it translates their MSI transactions.
> 
> We typically observe the virtio-pci-net device sending MSI transactions
> very early (even before the first vcpu run) which looks weird. It
> appears that not servicing those transactions cause the virtio-pci-net
> to stall.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

<snip>

> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
> index 3f8017d..7f81d33 100644
> --- a/hw/intc/arm_gicv3_its_kvm.c
> +++ b/hw/intc/arm_gicv3_its_kvm.c
> @@ -95,8 +95,12 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
>       * Block migration of a KVM GICv3 ITS device: the API for saving and
>       * restoring the state in the kernel is not yet available
>       */
> -    error_setg(&s->migration_blocker, "vITS migration is not implemented");
> -    migrate_add_blocker(s->migration_blocker);
> +    if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                               GITS_CTLR)) {
> +        error_setg(&s->migration_blocker, "This operating system kernel does "
> +                                          "not support vITS migration");
> +        migrate_add_blocker(s->migration_blocker);
> +    }

Watch out, a change went in to the parameters/return value of migrate_add_blocker
earlier in the week - it can now fail.

>      kvm_msi_use_devid = true;
>      kvm_gsi_direct_mapping = false;
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1a22887..ebd755c 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -188,6 +188,8 @@ enum VMStateFlags {
>  
>  typedef enum {
>      MIG_PRI_DEFAULT = 0,
> +    MIG_PRI_GICV3_ITS,
> +    MIG_PRI_GICV3,
>      MIG_PRI_MAX,

Can we keep this commented so it's trivially easy to see the order, something like:

 typedef enum {
     MIG_PRI_DEFAULT = 0,
+    MIG_PRI_GICV3_ITS,    /* Needs to be before PCI devices */
+    MIG_PRI_GICV3,        /* Must be before ITS */
     MIG_PRI_MAX,
 } MigrationPriority;

Dave

>  
> -- 
> 2.5.5
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC 4/4] hw/intc/arm_gicv3_its: Allow save/restore

[Auger Eric]

Hi Dave,

On 26/01/2017 11:06, Dr. David Alan Gilbert wrote:
> * Eric Auger (eric.auger@redhat.com) wrote:
>> We change the restoration priority of both the GICv3 and ITS. The
>> GICv3 must be restored before the ITS and the ITS needs to be restored
>> before PCIe devices since it translates their MSI transactions.
>>
>> We typically observe the virtio-pci-net device sending MSI transactions
>> very early (even before the first vcpu run) which looks weird. It
>> appears that not servicing those transactions cause the virtio-pci-net
>> to stall.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> <snip>
> 
>> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
>> index 3f8017d..7f81d33 100644
>> --- a/hw/intc/arm_gicv3_its_kvm.c
>> +++ b/hw/intc/arm_gicv3_its_kvm.c
>> @@ -95,8 +95,12 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
>>       * Block migration of a KVM GICv3 ITS device: the API for saving and
>>       * restoring the state in the kernel is not yet available
>>       */
>> -    error_setg(&s->migration_blocker, "vITS migration is not implemented");
>> -    migrate_add_blocker(s->migration_blocker);
>> +    if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
>> +                               GITS_CTLR)) {
>> +        error_setg(&s->migration_blocker, "This operating system kernel does "
>> +                                          "not support vITS migration");
>> +        migrate_add_blocker(s->migration_blocker);
>> +    }
> 
> Watch out, a change went in to the parameters/return value of migrate_add_blocker
> earlier in the week - it can now fail.
OK thanks for the notice
> 
>>      kvm_msi_use_devid = true;
>>      kvm_gsi_direct_mapping = false;
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 1a22887..ebd755c 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -188,6 +188,8 @@ enum VMStateFlags {
>>  
>>  typedef enum {
>>      MIG_PRI_DEFAULT = 0,
>> +    MIG_PRI_GICV3_ITS,
>> +    MIG_PRI_GICV3,
>>      MIG_PRI_MAX,
> 
> Can we keep this commented so it's trivially easy to see the order, something like:
> 
>  typedef enum {
>      MIG_PRI_DEFAULT = 0,
> +    MIG_PRI_GICV3_ITS,    /* Needs to be before PCI devices */
> +    MIG_PRI_GICV3,        /* Must be before ITS */
Sure

Thanks!

Eric
>      MIG_PRI_MAX,
>  } MigrationPriority;
> 
> Dave
> 
>>  
>> -- 
>> 2.5.5
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] [RFC 2/4] hw/intc/arm_gicv3_kvm: Rename KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS

[Vijay Kilari]

Hi Eric,

On Thu, Jan 26, 2017 at 2:49 PM, Eric Auger <eric.auger@redhat.com> wrote:
> Rename KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS into KVM_DEV_ARM_VGIC_CPU_SYSREGS
> as exposed in the kernel user API and pulled by update-linux-headers.sh.

I will fix it in my next qemu patch series.
I have updated kernel to use KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS in
latest kernel patch
version v11. I so you can drop this patch.

>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> KVM_DEV_ARM_VGIC_CPU_SYSREGS may be fixed at kernel level instead
> ---
>  hw/intc/arm_gicv3_kvm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
> index 267c2d6..d8ecbc3 100644
> --- a/hw/intc/arm_gicv3_kvm.c
> +++ b/hw/intc/arm_gicv3_kvm.c
> @@ -108,7 +108,7 @@ static inline void kvm_gicr_access(GICv3State *s, int offset, int cpu,
>  static inline void kvm_gicc_access(GICv3State *s, uint64_t reg, int cpu,
>                                     uint64_t *val, bool write)
>  {
> -    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_CPU_SYSREGS,
>                        KVM_VGIC_ATTR(reg, s->cpu[cpu].gicr_typer),
>                        val, write);
>  }
> --
> 2.5.5
>

Re: [Qemu-devel] [RFC 3/4] hw/intc/arm_gicv3_its: Implement state save/restore

[Vijay Kilari]

Hi Eric,

On Thu, Jan 26, 2017 at 2:49 PM, Eric Auger <eric.auger@redhat.com> wrote:
> We need to handle both registers and ITS tables. While
> register handling is standard, ITS table handling is more
> challenging since the kernel API is devised so that the
> tables are flushed into guest RAM and not in vmstate buffers.
>
> Flushing the ITS tables on device pre_save() is too late
> since the guest RAM had already been saved at this point.
>
> Table flushing needs to happen when we are sure the vcpus
> are stopped and before the last dirty page saving. The
> right point is RUN_STATE_FINISH_MIGRATE but sometimes the
> VM gets stopped before migration launch so let's simply
> flush the tables each time the VM gets stopped.
>
> For regular ITS registers we just can use vmstate pre_save
> and post_load callbacks.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
> ---
>  hw/intc/arm_gicv3_its_common.c         |  8 ++++
>  hw/intc/arm_gicv3_its_kvm.c            | 86 ++++++++++++++++++++++++++++++++++
>  include/hw/intc/arm_gicv3_its_common.h |  6 +++
>  3 files changed, 100 insertions(+)
>
> diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
> index 9d67c5c..75b9f04 100644
> --- a/hw/intc/arm_gicv3_its_common.c
> +++ b/hw/intc/arm_gicv3_its_common.c
> @@ -49,6 +49,14 @@ static const VMStateDescription vmstate_its = {
>      .pre_save = gicv3_its_pre_save,
>      .post_load = gicv3_its_post_load,
>      .unmigratable = true,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(ctlr, GICv3ITSState),
> +        VMSTATE_UINT64(cbaser, GICv3ITSState),
> +        VMSTATE_UINT64(cwriter, GICv3ITSState),
> +        VMSTATE_UINT64(creadr, GICv3ITSState),
> +        VMSTATE_UINT64_ARRAY(baser, GICv3ITSState, 8),
> +        VMSTATE_END_OF_LIST()
> +    },
>  };
>
>  static MemTxResult gicv3_its_trans_read(void *opaque, hwaddr offset,
> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
> index fc246e0..3f8017d 100644
> --- a/hw/intc/arm_gicv3_its_kvm.c
> +++ b/hw/intc/arm_gicv3_its_kvm.c
> @@ -53,6 +53,24 @@ static int kvm_its_send_msi(GICv3ITSState *s, uint32_t value, uint16_t devid)
>      return kvm_vm_ioctl(kvm_state, KVM_SIGNAL_MSI, &msi);
>  }
>
> +/**
> + * vm_change_state_handler - VM change state callback aiming at flushing
> + * ITS tables into guest RAM
> + *
> + * The tables get flushed to guest RAM whenever the VM gets stopped.
> + */
> +static void vm_change_state_handler(void *opaque, int running,
> +                                    RunState state)
> +{
> +    GICv3ITSState *s = (GICv3ITSState *)opaque;
> +
> +    if (running) {
> +        return;
> +    }
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_TABLES,
> +                      0, NULL, false);
> +}
> +
>  static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
>  {
>      GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
> @@ -83,6 +101,8 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
>      kvm_msi_use_devid = true;
>      kvm_gsi_direct_mapping = false;
>      kvm_msi_via_irqfd_allowed = kvm_irqfds_enabled();
> +
> +    qemu_add_vm_change_state_handler(vm_change_state_handler, s);
>  }
>
>  static void kvm_arm_its_init(Object *obj)
> @@ -96,6 +116,70 @@ static void kvm_arm_its_init(Object *obj)
>                               &error_abort);
>  }
>
> +/**
> + * kvm_arm_its_get - handles the saving of ITS registers.
> + * ITS tables, being flushed into guest RAM needs to be saved before
> + * the pre_save() callback, hence the migration state change notifiers
> + */
> +static void kvm_arm_its_get(GICv3ITSState *s)
> +{
> +    uint64_t reg;
> +    int i;
> +

     Don't we need to check for LPI support before save/restore?.
I mean, reading GITS_TYPER and check for LPI support?

> +    for (i = 0; i < 8; i++) {
> +        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                          GITS_BASER + i * 8, &s->baser[i], false);
> +    }
> +
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                      GITS_CTLR, &reg, false);
> +    s->ctlr = extract64(reg, 0, 32);
> +
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                      GITS_CBASER, &s->cbaser, false);
> +
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                      GITS_CREADR, &s->creadr, false);
> +
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                      GITS_CWRITER, &s->cwriter, false);
> +}
> +
> +/**
> + * kvm_arm_its_put - Restore both the ITS registers and guest RAM tables
> + * ITS tables, being flushed into guest RAM needs to be saved before
> + * the pre_save() callback. The restoration order matters since there
> + * are dependencies between register settings, as specified by the
> + * architecture specification
> + */
> +static void kvm_arm_its_put(GICv3ITSState *s)
> +{
> +    uint64_t reg;
> +    int i;
> +
> +    /* must be written before GITS_CREADR since it resets this latter*/
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                      GITS_CBASER, &s->cbaser, true);
> +
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                      GITS_CREADR, &s->creadr, true);
> +
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                      GITS_CWRITER, &s->cwriter, true);
> +
> +    for (i = 0; i < 8; i++) {
> +        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                          GITS_BASER + i * 8, &s->baser[i], true);
> +    }
> +
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_TABLES,
> +                      0, NULL, true);
> +
> +    reg = s->ctlr;
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
> +                      GITS_CTLR, &reg, true);
> +}
> +
>  static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -103,6 +187,8 @@ static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
>
>      dc->realize = kvm_arm_its_realize;
>      icc->send_msi = kvm_its_send_msi;
> +    icc->pre_save = kvm_arm_its_get;
> +    icc->post_load = kvm_arm_its_put;
>  }
>
>  static const TypeInfo kvm_arm_its_info = {
> diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
> index 1ba1894..ed5d6df 100644
> --- a/include/hw/intc/arm_gicv3_its_common.h
> +++ b/include/hw/intc/arm_gicv3_its_common.h
> @@ -28,6 +28,12 @@
>  #define ITS_TRANS_SIZE   0x10000
>  #define ITS_SIZE         (ITS_CONTROL_SIZE + ITS_TRANS_SIZE)
>
> +#define GITS_CTLR        0x0
> +#define GITS_CBASER      0x80
> +#define GITS_CWRITER     0x88
> +#define GITS_CREADR      0x90
> +#define GITS_BASER       0x100
> +
>  struct GICv3ITSState {
>      SysBusDevice parent_obj;
>
> --
> 2.5.5
>

Re: [Qemu-devel] [RFC 3/4] hw/intc/arm_gicv3_its: Implement state save/restore

[Auger Eric]

Hi Vijaya,

On 27/01/2017 08:17, Vijay Kilari wrote:
> Hi Eric,
> 
> On Thu, Jan 26, 2017 at 2:49 PM, Eric Auger <eric.auger@redhat.com> wrote:
>> We need to handle both registers and ITS tables. While
>> register handling is standard, ITS table handling is more
>> challenging since the kernel API is devised so that the
>> tables are flushed into guest RAM and not in vmstate buffers.
>>
>> Flushing the ITS tables on device pre_save() is too late
>> since the guest RAM had already been saved at this point.
>>
>> Table flushing needs to happen when we are sure the vcpus
>> are stopped and before the last dirty page saving. The
>> right point is RUN_STATE_FINISH_MIGRATE but sometimes the
>> VM gets stopped before migration launch so let's simply
>> flush the tables each time the VM gets stopped.
>>
>> For regular ITS registers we just can use vmstate pre_save
>> and post_load callbacks.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>> ---
>>  hw/intc/arm_gicv3_its_common.c         |  8 ++++
>>  hw/intc/arm_gicv3_its_kvm.c            | 86 ++++++++++++++++++++++++++++++++++
>>  include/hw/intc/arm_gicv3_its_common.h |  6 +++
>>  3 files changed, 100 insertions(+)
>>
>> diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
>> index 9d67c5c..75b9f04 100644
>> --- a/hw/intc/arm_gicv3_its_common.c
>> +++ b/hw/intc/arm_gicv3_its_common.c
>> @@ -49,6 +49,14 @@ static const VMStateDescription vmstate_its = {
>>      .pre_save = gicv3_its_pre_save,
>>      .post_load = gicv3_its_post_load,
>>      .unmigratable = true,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(ctlr, GICv3ITSState),
>> +        VMSTATE_UINT64(cbaser, GICv3ITSState),
>> +        VMSTATE_UINT64(cwriter, GICv3ITSState),
>> +        VMSTATE_UINT64(creadr, GICv3ITSState),
>> +        VMSTATE_UINT64_ARRAY(baser, GICv3ITSState, 8),
>> +        VMSTATE_END_OF_LIST()
>> +    },
>>  };
>>
>>  static MemTxResult gicv3_its_trans_read(void *opaque, hwaddr offset,
>> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
>> index fc246e0..3f8017d 100644
>> --- a/hw/intc/arm_gicv3_its_kvm.c
>> +++ b/hw/intc/arm_gicv3_its_kvm.c
>> @@ -53,6 +53,24 @@ static int kvm_its_send_msi(GICv3ITSState *s, uint32_t value, uint16_t devid)
>>      return kvm_vm_ioctl(kvm_state, KVM_SIGNAL_MSI, &msi);
>>  }
>>
>> +/**
>> + * vm_change_state_handler - VM change state callback aiming at flushing
>> + * ITS tables into guest RAM
>> + *
>> + * The tables get flushed to guest RAM whenever the VM gets stopped.
>> + */
>> +static void vm_change_state_handler(void *opaque, int running,
>> +                                    RunState state)
>> +{
>> +    GICv3ITSState *s = (GICv3ITSState *)opaque;
>> +
>> +    if (running) {
>> +        return;
>> +    }
>> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_TABLES,
>> +                      0, NULL, false);
>> +}
>> +
>>  static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
>>  {
>>      GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
>> @@ -83,6 +101,8 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
>>      kvm_msi_use_devid = true;
>>      kvm_gsi_direct_mapping = false;
>>      kvm_msi_via_irqfd_allowed = kvm_irqfds_enabled();
>> +
>> +    qemu_add_vm_change_state_handler(vm_change_state_handler, s);
>>  }
>>
>>  static void kvm_arm_its_init(Object *obj)
>> @@ -96,6 +116,70 @@ static void kvm_arm_its_init(Object *obj)
>>                               &error_abort);
>>  }
>>
>> +/**
>> + * kvm_arm_its_get - handles the saving of ITS registers.
>> + * ITS tables, being flushed into guest RAM needs to be saved before
>> + * the pre_save() callback, hence the migration state change notifiers
>> + */
>> +static void kvm_arm_its_get(GICv3ITSState *s)
>> +{
>> +    uint64_t reg;
>> +    int i;
>> +
> 
>      Don't we need to check for LPI support before save/restore?.
> I mean, reading GITS_TYPER and check for LPI support?
I understand this is not needed since The GITS_TYPER Physical Bit is
RES1 indicating the ITS always supports physical LPIs.

Thanks

Eric
> 
>> +    for (i = 0; i < 8; i++) {
>> +        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
>> +                          GITS_BASER + i * 8, &s->baser[i], false);
>> +    }
>> +
>> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
>> +                      GITS_CTLR, &reg, false);
>> +    s->ctlr = extract64(reg, 0, 32);
>> +
>> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
>> +                      GITS_CBASER, &s->cbaser, false);
>> +
>> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
>> +                      GITS_CREADR, &s->creadr, false);
>> +
>> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
>> +                      GITS_CWRITER, &s->cwriter, false);
>> +}
>> +
>> +/**
>> + * kvm_arm_its_put - Restore both the ITS registers and guest RAM tables
>> + * ITS tables, being flushed into guest RAM needs to be saved before
>> + * the pre_save() callback. The restoration order matters since there
>> + * are dependencies between register settings, as specified by the
>> + * architecture specification
>> + */
>> +static void kvm_arm_its_put(GICv3ITSState *s)
>> +{
>> +    uint64_t reg;
>> +    int i;
>> +
>> +    /* must be written before GITS_CREADR since it resets this latter*/
>> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
>> +                      GITS_CBASER, &s->cbaser, true);
>> +
>> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
>> +                      GITS_CREADR, &s->creadr, true);
>> +
>> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
>> +                      GITS_CWRITER, &s->cwriter, true);
>> +
>> +    for (i = 0; i < 8; i++) {
>> +        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
>> +                          GITS_BASER + i * 8, &s->baser[i], true);
>> +    }
>> +
>> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_TABLES,
>> +                      0, NULL, true);
>> +
>> +    reg = s->ctlr;
>> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
>> +                      GITS_CTLR, &reg, true);
>> +}
>> +
>>  static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -103,6 +187,8 @@ static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
>>
>>      dc->realize = kvm_arm_its_realize;
>>      icc->send_msi = kvm_its_send_msi;
>> +    icc->pre_save = kvm_arm_its_get;
>> +    icc->post_load = kvm_arm_its_put;
>>  }
>>
>>  static const TypeInfo kvm_arm_its_info = {
>> diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
>> index 1ba1894..ed5d6df 100644
>> --- a/include/hw/intc/arm_gicv3_its_common.h
>> +++ b/include/hw/intc/arm_gicv3_its_common.h
>> @@ -28,6 +28,12 @@
>>  #define ITS_TRANS_SIZE   0x10000
>>  #define ITS_SIZE         (ITS_CONTROL_SIZE + ITS_TRANS_SIZE)
>>
>> +#define GITS_CTLR        0x0
>> +#define GITS_CBASER      0x80
>> +#define GITS_CWRITER     0x88
>> +#define GITS_CREADR      0x90
>> +#define GITS_BASER       0x100
>> +
>>  struct GICv3ITSState {
>>      SysBusDevice parent_obj;
>>
>> --
>> 2.5.5
>>

Re: [Qemu-devel] [RFC 2/4] hw/intc/arm_gicv3_kvm: Rename KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS

[Auger Eric]

Hi Vijaya,

On 27/01/2017 08:02, Vijay Kilari wrote:
> Hi Eric,
> 
> On Thu, Jan 26, 2017 at 2:49 PM, Eric Auger <eric.auger@redhat.com> wrote:
>> Rename KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS into KVM_DEV_ARM_VGIC_CPU_SYSREGS
>> as exposed in the kernel user API and pulled by update-linux-headers.sh.
> 
> I will fix it in my next qemu patch series.
> I have updated kernel to use KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS in
> latest kernel patch
> version v11. I so you can drop this patch.

Sure I saw the change in your v11.

Thanks

Eric
> 
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> KVM_DEV_ARM_VGIC_CPU_SYSREGS may be fixed at kernel level instead
>> ---
>>  hw/intc/arm_gicv3_kvm.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
>> index 267c2d6..d8ecbc3 100644
>> --- a/hw/intc/arm_gicv3_kvm.c
>> +++ b/hw/intc/arm_gicv3_kvm.c
>> @@ -108,7 +108,7 @@ static inline void kvm_gicr_access(GICv3State *s, int offset, int cpu,
>>  static inline void kvm_gicc_access(GICv3State *s, uint64_t reg, int cpu,
>>                                     uint64_t *val, bool write)
>>  {
>> -    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
>> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_CPU_SYSREGS,
>>                        KVM_VGIC_ATTR(reg, s->cpu[cpu].gicr_typer),
>>                        val, write);
>>  }
>> --
>> 2.5.5
>>

Re: [Qemu-devel] [RFC 3/4] hw/intc/arm_gicv3_its: Implement state save/restore

[Juan Quintela]

Eric Auger <eric.auger@redhat.com> wrote:
> We need to handle both registers and ITS tables. While
> register handling is standard, ITS table handling is more
> challenging since the kernel API is devised so that the
> tables are flushed into guest RAM and not in vmstate buffers.
>
> Flushing the ITS tables on device pre_save() is too late
> since the guest RAM had already been saved at this point.
>
> Table flushing needs to happen when we are sure the vcpus
> are stopped and before the last dirty page saving. The
> right point is RUN_STATE_FINISH_MIGRATE but sometimes the
> VM gets stopped before migration launch so let's simply
> flush the tables each time the VM gets stopped.
>
> For regular ITS registers we just can use vmstate pre_save
> and post_load callbacks.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

Hi


> + * vm_change_state_handler - VM change state callback aiming at flushing
> + * ITS tables into guest RAM
> + *
> + * The tables get flushed to guest RAM whenever the VM gets stopped.
> + */
> +static void vm_change_state_handler(void *opaque, int running,
> +                                    RunState state)
> +{
> +    GICv3ITSState *s = (GICv3ITSState *)opaque;

Cast is unneeded.

> +
> +    if (running) {
> +        return;
> +    }
> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_TABLES,
> +                      0, NULL, false);

As you are adding it to do everytime that we stop the guest, how
expensive/slow is that?

Thanks, Juan.

Re: [Qemu-devel] [RFC 3/4] hw/intc/arm_gicv3_its: Implement state save/restore

[Auger Eric]

Hi Juan,

On 30/01/2017 10:15, Juan Quintela wrote:
> Eric Auger <eric.auger@redhat.com> wrote:
>> We need to handle both registers and ITS tables. While
>> register handling is standard, ITS table handling is more
>> challenging since the kernel API is devised so that the
>> tables are flushed into guest RAM and not in vmstate buffers.
>>
>> Flushing the ITS tables on device pre_save() is too late
>> since the guest RAM had already been saved at this point.
>>
>> Table flushing needs to happen when we are sure the vcpus
>> are stopped and before the last dirty page saving. The
>> right point is RUN_STATE_FINISH_MIGRATE but sometimes the
>> VM gets stopped before migration launch so let's simply
>> flush the tables each time the VM gets stopped.
>>
>> For regular ITS registers we just can use vmstate pre_save
>> and post_load callbacks.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> Hi
> 
> 
>> + * vm_change_state_handler - VM change state callback aiming at flushing
>> + * ITS tables into guest RAM
>> + *
>> + * The tables get flushed to guest RAM whenever the VM gets stopped.
>> + */
>> +static void vm_change_state_handler(void *opaque, int running,
>> +                                    RunState state)
>> +{
>> +    GICv3ITSState *s = (GICv3ITSState *)opaque;
> 
> Cast is unneeded.
> 
>> +
>> +    if (running) {
>> +        return;
>> +    }
>> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_TABLES,
>> +                      0, NULL, false);
> 
> As you are adding it to do everytime that we stop the guest, how
> expensive/slow is that?

This is highly dependent on the number of devices using MSIs and number
of allocated MSIs on guest. The number of bytes to transfer basically is:

(#nb_vcpus + #nb_devices_using_MSI_on_guest  +  2 *
nb_allocated_guest_MSIs bytes ) * 8 bytes

So I would say < 10 kB in real life case. In my virtio-pci test case it
is just 440 Bytes.

For live migration I could hook a callback at RUN_STATE_FINISH_MIGRATE.
However this does not work with virsh save/restore use case since the
notifier is not called (the VM being already paused), hence that choice.

Thanks

Eric

> 
> Thanks, Juan.
> 

Re: [Qemu-devel] [RFC 3/4] hw/intc/arm_gicv3_its: Implement state save/restore

[Juan Quintela]

Auger Eric <eric.auger@redhat.com> wrote:
> Hi Juan,
>
> On 30/01/2017 10:15, Juan Quintela wrote:
>> Eric Auger <eric.auger@redhat.com> wrote:
>>> We need to handle both registers and ITS tables. While
>>> register handling is standard, ITS table handling is more
>>> challenging since the kernel API is devised so that the
>>> tables are flushed into guest RAM and not in vmstate buffers.
>>>
>>> Flushing the ITS tables on device pre_save() is too late
>>> since the guest RAM had already been saved at this point.
>>>
>>> Table flushing needs to happen when we are sure the vcpus
>>> are stopped and before the last dirty page saving. The
>>> right point is RUN_STATE_FINISH_MIGRATE but sometimes the
>>> VM gets stopped before migration launch so let's simply
>>> flush the tables each time the VM gets stopped.
>>>
>>> For regular ITS registers we just can use vmstate pre_save
>>> and post_load callbacks.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> 
>> Hi
>> 
>> 
>>> + * vm_change_state_handler - VM change state callback aiming at flushing
>>> + * ITS tables into guest RAM
>>> + *
>>> + * The tables get flushed to guest RAM whenever the VM gets stopped.
>>> + */
>>> +static void vm_change_state_handler(void *opaque, int running,
>>> +                                    RunState state)
>>> +{
>>> +    GICv3ITSState *s = (GICv3ITSState *)opaque;
>> 
>> Cast is unneeded.
>> 
>>> +
>>> +    if (running) {
>>> +        return;
>>> +    }
>>> +    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_TABLES,
>>> +                      0, NULL, false);
>> 
>> As you are adding it to do everytime that we stop the guest, how
>> expensive/slow is that?
>
> This is highly dependent on the number of devices using MSIs and number
> of allocated MSIs on guest. The number of bytes to transfer basically is:
>
> (#nb_vcpus + #nb_devices_using_MSI_on_guest  +  2 *
> nb_allocated_guest_MSIs bytes ) * 8 bytes
>
> So I would say < 10 kB in real life case. In my virtio-pci test case it
> is just 440 Bytes.
>
> For live migration I could hook a callback at RUN_STATE_FINISH_MIGRATE.
> However this does not work with virsh save/restore use case since the
> notifier is not called (the VM being already paused), hence that choice.

Agreed as a workaround.

We really need two notifiers:
- one that is run before the "completion stage" on source
- another that is run when we start the guest after a migration

But that is independent of this patch.

Later, Juan.

Re: [Qemu-devel] [RFC 4/4] hw/intc/arm_gicv3_its: Allow save/restore

[Peter Xu]

On Thu, Jan 26, 2017 at 02:30:17PM +0100, Auger Eric wrote:

[...]

> >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> >> index 1a22887..ebd755c 100644
> >> --- a/include/migration/vmstate.h
> >> +++ b/include/migration/vmstate.h
> >> @@ -188,6 +188,8 @@ enum VMStateFlags {
> >>  
> >>  typedef enum {
> >>      MIG_PRI_DEFAULT = 0,
> >> +    MIG_PRI_GICV3_ITS,
> >> +    MIG_PRI_GICV3,
> >>      MIG_PRI_MAX,
> > 
> > Can we keep this commented so it's trivially easy to see the order, something like:
> > 
> >  typedef enum {
> >      MIG_PRI_DEFAULT = 0,
> > +    MIG_PRI_GICV3_ITS,    /* Needs to be before PCI devices */
> > +    MIG_PRI_GICV3,        /* Must be before ITS */
> Sure
> 
> Thanks!

Besides above: is it possible that in the future other platforms
(rather than ARM) can leverage these new introduced priority? If so,
would it be nicer that we use general names (like, e.g., INTCxxx? or
better?) rather than platform-specific names (like, GICxxx)?

Thanks,

-- peterx

Re: [Qemu-devel] [RFC 4/4] hw/intc/arm_gicv3_its: Allow save/restore

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Thu, Jan 26, 2017 at 02:30:17PM +0100, Auger Eric wrote:
> 
> [...]
> 
> > >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > >> index 1a22887..ebd755c 100644
> > >> --- a/include/migration/vmstate.h
> > >> +++ b/include/migration/vmstate.h
> > >> @@ -188,6 +188,8 @@ enum VMStateFlags {
> > >>  
> > >>  typedef enum {
> > >>      MIG_PRI_DEFAULT = 0,
> > >> +    MIG_PRI_GICV3_ITS,
> > >> +    MIG_PRI_GICV3,
> > >>      MIG_PRI_MAX,
> > > 
> > > Can we keep this commented so it's trivially easy to see the order, something like:
> > > 
> > >  typedef enum {
> > >      MIG_PRI_DEFAULT = 0,
> > > +    MIG_PRI_GICV3_ITS,    /* Needs to be before PCI devices */
> > > +    MIG_PRI_GICV3,        /* Must be before ITS */
> > Sure
> > 
> > Thanks!
> 
> Besides above: is it possible that in the future other platforms
> (rather than ARM) can leverage these new introduced priority? If so,
> would it be nicer that we use general names (like, e.g., INTCxxx? or
> better?) rather than platform-specific names (like, GICxxx)?

Yes, but the ordering rules on other platforms might be subtly different.

Dave

> Thanks,
> 
> -- peterx
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC 4/4] hw/intc/arm_gicv3_its: Allow save/restore

[Peter Xu]

On Fri, Feb 03, 2017 at 09:57:05AM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Thu, Jan 26, 2017 at 02:30:17PM +0100, Auger Eric wrote:
> > 
> > [...]
> > 
> > > >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > > >> index 1a22887..ebd755c 100644
> > > >> --- a/include/migration/vmstate.h
> > > >> +++ b/include/migration/vmstate.h
> > > >> @@ -188,6 +188,8 @@ enum VMStateFlags {
> > > >>  
> > > >>  typedef enum {
> > > >>      MIG_PRI_DEFAULT = 0,
> > > >> +    MIG_PRI_GICV3_ITS,
> > > >> +    MIG_PRI_GICV3,
> > > >>      MIG_PRI_MAX,
> > > > 
> > > > Can we keep this commented so it's trivially easy to see the order, something like:
> > > > 
> > > >  typedef enum {
> > > >      MIG_PRI_DEFAULT = 0,
> > > > +    MIG_PRI_GICV3_ITS,    /* Needs to be before PCI devices */
> > > > +    MIG_PRI_GICV3,        /* Must be before ITS */
> > > Sure
> > > 
> > > Thanks!
> > 
> > Besides above: is it possible that in the future other platforms
> > (rather than ARM) can leverage these new introduced priority? If so,
> > would it be nicer that we use general names (like, e.g., INTCxxx? or
> > better?) rather than platform-specific names (like, GICxxx)?
> 
> Yes, but the ordering rules on other platforms might be subtly different.

I see. Then I have no problem in either way - we can rearrange the
defines until one day it is really needed. Thanks,

-- peterx

Re: [Qemu-devel] [RFC 0/4] vITS save/restore

[Peter Maydell]

On 26 January 2017 at 09:19, Eric Auger <eric.auger@redhat.com> wrote:
> This series allows ITS save/restore and migration use cases.
> It relies on not upstreamed kernel series ([1] & [2]) and
> QEMU not upstreamed series [3].
>
> ITS tables are flushed into guest RAM on VM stop while registers
> are save on pre_save() callback. Tables and registers are restored
> on ITS post_load().
>
> The code is based on Vijaya's "GICv3 live migration support"
> series [3], based on v2.8.0-rc4.
>
> The series also uses one patch from Peter Xu that aims at allowing
> the explicit ordering of save/restore handlers [4], now upstreamed.
>
> That work was tested on Cavium ThunderX using virsh save/restore and
> virt-manager live migration.
>
> Best Regards
>
> Eric

Looks overall OK to me (you've had some review comments from
others but I don't have anything else particularly to add).

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/4] vITS save/restore

[Auger Eric]

Hi Peter, all,

On 07/02/2017 15:36, Peter Maydell wrote:
> On 26 January 2017 at 09:19, Eric Auger <eric.auger@redhat.com> wrote:
>> This series allows ITS save/restore and migration use cases.
>> It relies on not upstreamed kernel series ([1] & [2]) and
>> QEMU not upstreamed series [3].
>>
>> ITS tables are flushed into guest RAM on VM stop while registers
>> are save on pre_save() callback. Tables and registers are restored
>> on ITS post_load().
>>
>> The code is based on Vijaya's "GICv3 live migration support"
>> series [3], based on v2.8.0-rc4.
>>
>> The series also uses one patch from Peter Xu that aims at allowing
>> the explicit ordering of save/restore handlers [4], now upstreamed.
>>
>> That work was tested on Cavium ThunderX using virsh save/restore and
>> virt-manager live migration.
>>
>> Best Regards
>>
>> Eric
> 
> Looks overall OK to me (you've had some review comments from
> others but I don't have anything else particularly to add).

Thank you for your reviews. Yes I have followed each comment and I will
respin shortly. I have been mostly busy with the kernel series respin
taking into account Marc's comments and supporting 2 stage device tables.

Thanks

Eric
> 
> thanks
> -- PMM
>