[RFC 00/13] vITS save/restore

[RFC 00/13] vITS save/restore

[Eric Auger]

This series specifies and implements an API aimed at saving and restoring
the state of the in-kernel emulated ITS device.

The ITS is programmed through registers and tables. Those later tables
are allocated by the guest.  Their base address is programmed in
registers or table entries before the ITS is enabled.

The ITS is free to use some of them to flush its internal caches. This
is likely to be used when entering low power state.

Therefore, for save/restore use case, it looks natural to use this
guest RAM allocated space to save the table related data. However,
currently,The ITS in-kernel emulated device does not use all of those
tables and for those it uses, it does not always sync them with its
cached data. Additional sync must happen for:
- the collection table
- the device table
- the per-device translation tables
- the LPI pending tables.

The LPI configration table and the command queues do not need extra
syncs.

So the bulk of the work in this series consists in the table
save/restore rather than register save/restore.

An alternative to flushing the tables into guest RAM could have been
to flush them into a separate user-space buffer. However the drawback
of this alternative is that the virtualizer would allocate dedicated
buffers to store the data that should normally be laid out in guest
RAM. It would also be obliged to re-compute their size from
register/table content.

So saving the tables in guest RAM better fit the ITS programming
model and optimizes the memory usage. The drawback of this solution
is it brings additional challenges at user-space level to make sure
the guest RAM is frozen after table sync.

The code is functional while saving/restoring a guest using
virtio-net-pci.  However many points deserve additional tests.
I share the series at that stage to get the documentation reviewed
and main principles discussed.

The series applies on top of Vijaya's series:
- [PATCH v10 0/8] arm/arm64: vgic: Implement API for vGICv3 live
  migration
  http://www.spinics.net/lists/arm-kernel/msg546383.html

Best Regards

Eric

Git: complete series available at
https://github.com/eauger/linux/tree/v4.10-rc3-its-mig-rfc-v1

* Testing:
- on Cavium using a virtio-net-pci guest and virsh save/restore
  commands

Eric Auger (13):
  KVM: arm/arm64: Add vITS save/restore API documentation
  arm/arm64: vgic: turn vgic_find_mmio_region into public
  KVM: arm64: ITS: KVM_DEV_ARM_VGIC_GRP_ITS_REGS group
  KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access
  KVM: arm64: ITS: Implement vgic_mmio_uaccess_write_its_creadr
  KVM: arm64: ITS: Expose ITT_Entry_Size in GITS_TYPER
  KVM: arm64: ITS: Change entry_size and indirect bit in BASER
  KVM: arm64: ITS: On MAPD interpret and store itt_addr and size
  KVM: arm64: ITS: KVM_DEV_ARM_VGIC_GRP_ITS_TABLES group
  KVM: arm64: ITS: vgic_its_alloc_itte/device
  KVM: arm64: ITS: Collection table save/restore
  KVM: arm64: ITS: Device and translation table flush
  KVM: arm64: ITS: Pending table save/restore

 Documentation/virtual/kvm/devices/arm-vgic-its.txt |  70 +++
 arch/arm/include/uapi/asm/kvm.h                    |   2 +
 arch/arm64/include/uapi/asm/kvm.h                  |   2 +
 include/kvm/arm_vgic.h                             |   3 +
 include/linux/irqchip/arm-gic-v3.h                 |   1 +
 virt/kvm/arm/vgic/vgic-its.c                       | 655 +++++++++++++++++++--
 virt/kvm/arm/vgic/vgic-mmio.c                      |   3 +-
 virt/kvm/arm/vgic/vgic-mmio.h                      |  14 +-
 8 files changed, 705 insertions(+), 45 deletions(-)

-- 
2.5.5

[RFC 00/13] vITS save/restore

[Eric Auger]

This series specifies and implements an API aimed at saving and restoring
the state of the in-kernel emulated ITS device.

The ITS is programmed through registers and tables. Those later tables
are allocated by the guest.  Their base address is programmed in
registers or table entries before the ITS is enabled.

The ITS is free to use some of them to flush its internal caches. This
is likely to be used when entering low power state.

Therefore, for save/restore use case, it looks natural to use this
guest RAM allocated space to save the table related data. However,
currently,The ITS in-kernel emulated device does not use all of those
tables and for those it uses, it does not always sync them with its
cached data. Additional sync must happen for:
- the collection table
- the device table
- the per-device translation tables
- the LPI pending tables.

The LPI configration table and the command queues do not need extra
syncs.

So the bulk of the work in this series consists in the table
save/restore rather than register save/restore.

An alternative to flushing the tables into guest RAM could have been
to flush them into a separate user-space buffer. However the drawback
of this alternative is that the virtualizer would allocate dedicated
buffers to store the data that should normally be laid out in guest
RAM. It would also be obliged to re-compute their size from
register/table content.

So saving the tables in guest RAM better fit the ITS programming
model and optimizes the memory usage. The drawback of this solution
is it brings additional challenges at user-space level to make sure
the guest RAM is frozen after table sync.

The code is functional while saving/restoring a guest using
virtio-net-pci.  However many points deserve additional tests.
I share the series at that stage to get the documentation reviewed
and main principles discussed.

The series applies on top of Vijaya's series:
- [PATCH v10 0/8] arm/arm64: vgic: Implement API for vGICv3 live
  migration
  http://www.spinics.net/lists/arm-kernel/msg546383.html

Best Regards

Eric

Git: complete series available at
https://github.com/eauger/linux/tree/v4.10-rc3-its-mig-rfc-v1

* Testing:
- on Cavium using a virtio-net-pci guest and virsh save/restore
  commands

Eric Auger (13):
  KVM: arm/arm64: Add vITS save/restore API documentation
  arm/arm64: vgic: turn vgic_find_mmio_region into public
  KVM: arm64: ITS: KVM_DEV_ARM_VGIC_GRP_ITS_REGS group
  KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access
  KVM: arm64: ITS: Implement vgic_mmio_uaccess_write_its_creadr
  KVM: arm64: ITS: Expose ITT_Entry_Size in GITS_TYPER
  KVM: arm64: ITS: Change entry_size and indirect bit in BASER
  KVM: arm64: ITS: On MAPD interpret and store itt_addr and size
  KVM: arm64: ITS: KVM_DEV_ARM_VGIC_GRP_ITS_TABLES group
  KVM: arm64: ITS: vgic_its_alloc_itte/device
  KVM: arm64: ITS: Collection table save/restore
  KVM: arm64: ITS: Device and translation table flush
  KVM: arm64: ITS: Pending table save/restore

 Documentation/virtual/kvm/devices/arm-vgic-its.txt |  70 +++
 arch/arm/include/uapi/asm/kvm.h                    |   2 +
 arch/arm64/include/uapi/asm/kvm.h                  |   2 +
 include/kvm/arm_vgic.h                             |   3 +
 include/linux/irqchip/arm-gic-v3.h                 |   1 +
 virt/kvm/arm/vgic/vgic-its.c                       | 655 +++++++++++++++++++--
 virt/kvm/arm/vgic/vgic-mmio.c                      |   3 +-
 virt/kvm/arm/vgic/vgic-mmio.h                      |  14 +-
 8 files changed, 705 insertions(+), 45 deletions(-)

-- 
2.5.5

[RFC 01/13] KVM: arm/arm64: Add vITS save/restore API documentation

[Eric Auger]

Add description for how to access vITS registers and how to flush/restore
vITS tables into/from memory

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 ++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
index 6081a5b..bd74613 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
@@ -36,3 +36,73 @@ Groups:
     -ENXIO:  ITS not properly configured as required prior to setting
              this attribute
     -ENOMEM: Memory shortage when allocating ITS internal data
+
+  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
+  Attributes:
+      The attr field of kvm_device_attr encodes the offset of the
+      ITS register, relative to the ITS control frame base address
+      (ITS_base).
+
+      kvm_device_attr.addr points to a __u64 value whatever the width
+      of the addressed register (32/64 bits).
+
+      Writes to read-only registers are ignored by the kernel except
+      for a single register, GITS_READR. Normally this register is RO
+      but it needs to be restored otherwise commands in the queue will
+      be re-executed after CWRITER setting.
+
+      For other registers, Getting or setting a register has the same
+      effect as reading/writing the register on real hardware.
+  Errors:
+    -ENXIO: Offset does not correspond to any supported register
+    -EFAULT: Invalid user pointer for attr->addr
+    -EINVAL: Offset is not 64-bit aligned
+
+  KVM_DEV_ARM_VGIC_GRP_ITS_TABLES
+  Attributes
+       The attr field of kvm_device_attr is not used.
+
+       request the flush-save/restore of the ITS tables, namely
+       the device table, the collection table, all the ITT tables,
+       the LPI pending tables. On save, the tables are flushed
+       into guest memory at the location provisionned by the guest
+       in GITS_BASER (device and collection tables), on MAPD command
+       (ITT_addr), GICR_PENDBASERs (pending tables).
+
+       This means the GIC should be restored before the ITS and all
+       ITS registers but the GITS_CTRL must be restored before
+       restoring the ITS tables.
+
+       Note the LPI configuration table is read-only for the
+       in-kernel ITS and its save/restore goes through the standard
+       RAM save/restore.
+
+       The layout of the tables in guest memory defines an ABI.
+       The entries are laid out in memory as follows;
+
+    Device Table Entry (DTE) layout: entry size = 16 bytes
+
+    bits:     | 63   ...  32  | 31 ... 6 | 5 | 4   ...   0 |
+    values:   |   DeviceID    |   Resv   | V |    Size     |
+
+    bits:     | 63 ... 44 | 43  ...  0  |
+    values:   |    Resv   |  ITT_addr   |
+
+    Collection Table Entry (CTE) layout: entry size = 8 bytes
+
+    bits:     | 63| 62 ..  52  | 51 ... 16 | 15  ...   0 |
+    values:   | V |    RES0    |  RDBase   |    ICID     |
+
+    Interrupt Translation Table Entry (ITTE) layout: entry size = 16 bytes
+
+    bits:     | 63   ...  32  | 31 ... 17 | 16 | 15  ...  0 |
+    values:   |   DeviceID    |    RES0   | V  |    ICID    |
+
+    bits:     | 63 ...  32    | 31      ...        0 |
+    values:   |   pINTID      |        EventID       |
+
+    LPI Pending Table layout:
+
+    As specified in the ARM Generic Interrupt Controller Architecture
+    Specification GIC Architecture version 3.0 and version 4. The first
+    1kB contains only zeros.
-- 
2.5.5

[RFC 02/13] arm/arm64: vgic: turn vgic_find_mmio_region into public

[Eric Auger]

We plan to use vgic_find_mmio_region in vgic-its.c so let's
turn it into a public function.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 virt/kvm/arm/vgic/vgic-mmio.c | 3 +--
 virt/kvm/arm/vgic/vgic-mmio.h | 5 +++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 28fef92..568bffa 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -431,8 +431,7 @@ static int match_region(const void *key, const void *elt)
 	return 0;
 }
 
-/* Find the proper register handler entry given a certain address offset. */
-static const struct vgic_register_region *
+const struct vgic_register_region *
 vgic_find_mmio_region(const struct vgic_register_region *region, int nr_regions,
 		      unsigned int offset)
 {
diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
index b9423b4..ecf8bfb 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -196,4 +196,9 @@ u64 vgic_sanitise_shareability(u64 reg);
 u64 vgic_sanitise_field(u64 reg, u64 field_mask, int field_shift,
 			u64 (*sanitise_fn)(u64));
 
+/* Find the proper register handler entry given a certain address offset */
+const struct vgic_register_region *
+vgic_find_mmio_region(const struct vgic_register_region *region,
+		      int nr_regions, unsigned int offset);
+
 #endif
-- 
2.5.5

[RFC 03/13] KVM: arm64: ITS: KVM_DEV_ARM_VGIC_GRP_ITS_REGS group

[Eric Auger]

The ITS KVM device exposes a new KVM_DEV_ARM_VGIC_GRP_ITS_REGS
group which allows the userspace to save/restore ITS registers.

At this stage the get/set/has operations are not yet implemented.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arch/arm/include/uapi/asm/kvm.h   |  1 +
 arch/arm64/include/uapi/asm/kvm.h |  1 +
 virt/kvm/arm/vgic/vgic-its.c      | 36 +++++++++++++++++++++++++++++++++++-
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index f347779..f6a167c 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -192,6 +192,7 @@ struct kvm_arch_memory_slot {
 #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
 #define KVM_DEV_ARM_VGIC_CPU_SYSREGS    6
 #define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7
+#define KVM_DEV_ARM_VGIC_GRP_ITS_REGS	8
 #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)
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 4100f8c..8850e9b 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -212,6 +212,7 @@ struct kvm_arch_memory_slot {
 #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
 #define KVM_DEV_ARM_VGIC_CPU_SYSREGS    6
 #define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7
+#define KVM_DEV_ARM_VGIC_GRP_ITS_REGS 8
 #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)
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 8c2b3cd..058034d 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1445,6 +1445,19 @@ static void vgic_its_destroy(struct kvm_device *kvm_dev)
 	kfree(its);
 }
 
+int vgic_its_has_attr_regs(struct kvm_device *dev,
+			   struct kvm_device_attr *attr)
+{
+	return -ENXIO;
+}
+
+int vgic_its_attr_regs_access(struct kvm_device *dev,
+			      struct kvm_device_attr *attr,
+			      u64 *reg, bool is_write)
+{
+	return -ENXIO;
+}
+
 static int vgic_its_has_attr(struct kvm_device *dev,
 			     struct kvm_device_attr *attr)
 {
@@ -1461,6 +1474,8 @@ static int vgic_its_has_attr(struct kvm_device *dev,
 			return 0;
 		}
 		break;
+	case KVM_DEV_ARM_VGIC_GRP_ITS_REGS:
+		return vgic_its_has_attr_regs(dev, attr);
 	}
 	return -ENXIO;
 }
@@ -1500,6 +1515,15 @@ static int vgic_its_set_attr(struct kvm_device *dev,
 			return 0;
 		}
 		break;
+	case KVM_DEV_ARM_VGIC_GRP_ITS_REGS: {
+		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
+		u64 reg;
+
+		if (get_user(reg, uaddr))
+			return -EFAULT;
+
+		return vgic_its_attr_regs_access(dev, attr, &reg, true);
+	}
 	}
 	return -ENXIO;
 }
@@ -1520,10 +1544,20 @@ static int vgic_its_get_attr(struct kvm_device *dev,
 		if (copy_to_user(uaddr, &addr, sizeof(addr)))
 			return -EFAULT;
 		break;
+	}
+	case KVM_DEV_ARM_VGIC_GRP_ITS_REGS: {
+		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
+		u64 reg;
+		int ret;
+
+		ret = vgic_its_attr_regs_access(dev, attr, &reg, false);
+		if (ret)
+			return ret;
+		return put_user(reg, uaddr);
+	}
 	default:
 		return -ENXIO;
 	}
-	}
 
 	return 0;
 }
-- 
2.5.5

[RFC 04/13] KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access

[Eric Auger]

This patch implements vgic_its_has_attr_regs and vgic_its_attr_regs_access
upon the MMIO framework. VGIC ITS KVM device KVM_DEV_ARM_VGIC_GRP_ITS_REGS
group becomes functional.

At least GITS_CREADR requires to differentiate a guest write action from a
user access. As such let's introduce a new uaccess_its_write
vgic_register_region callback.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 virt/kvm/arm/vgic/vgic-its.c  | 74 ++++++++++++++++++++++++++++++++++++-------
 virt/kvm/arm/vgic/vgic-mmio.h |  9 ++++--
 2 files changed, 69 insertions(+), 14 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 058034d..55671585 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1287,13 +1287,14 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
 	*regptr = reg;
 }
 
-#define REGISTER_ITS_DESC(off, rd, wr, length, acc)		\
+#define REGISTER_ITS_DESC(off, rd, wr, uwr, length, acc)		\
 {								\
 	.reg_offset = off,					\
 	.len = length,						\
 	.access_flags = acc,					\
 	.its_read = rd,						\
 	.its_write = wr,					\
+	.uaccess_its_write = uwr,					\
 }
 
 static void its_mmio_write_wi(struct kvm *kvm, struct vgic_its *its,
@@ -1304,28 +1305,28 @@ static void its_mmio_write_wi(struct kvm *kvm, struct vgic_its *its,
 
 static struct vgic_register_region its_registers[] = {
 	REGISTER_ITS_DESC(GITS_CTLR,
-		vgic_mmio_read_its_ctlr, vgic_mmio_write_its_ctlr, 4,
+		vgic_mmio_read_its_ctlr, vgic_mmio_write_its_ctlr, NULL, 4,
 		VGIC_ACCESS_32bit),
 	REGISTER_ITS_DESC(GITS_IIDR,
-		vgic_mmio_read_its_iidr, its_mmio_write_wi, 4,
+		vgic_mmio_read_its_iidr, its_mmio_write_wi, NULL, 4,
 		VGIC_ACCESS_32bit),
 	REGISTER_ITS_DESC(GITS_TYPER,
-		vgic_mmio_read_its_typer, its_mmio_write_wi, 8,
+		vgic_mmio_read_its_typer, its_mmio_write_wi, NULL, 8,
 		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
 	REGISTER_ITS_DESC(GITS_CBASER,
-		vgic_mmio_read_its_cbaser, vgic_mmio_write_its_cbaser, 8,
+		vgic_mmio_read_its_cbaser, vgic_mmio_write_its_cbaser, NULL, 8,
 		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
 	REGISTER_ITS_DESC(GITS_CWRITER,
-		vgic_mmio_read_its_cwriter, vgic_mmio_write_its_cwriter, 8,
-		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
+		vgic_mmio_read_its_cwriter, vgic_mmio_write_its_cwriter, NULL,
+		8, VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
 	REGISTER_ITS_DESC(GITS_CREADR,
-		vgic_mmio_read_its_creadr, its_mmio_write_wi, 8,
+		vgic_mmio_read_its_creadr, its_mmio_write_wi, NULL, 8,
 		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
 	REGISTER_ITS_DESC(GITS_BASER,
-		vgic_mmio_read_its_baser, vgic_mmio_write_its_baser, 0x40,
+		vgic_mmio_read_its_baser, vgic_mmio_write_its_baser, NULL, 0x40,
 		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
 	REGISTER_ITS_DESC(GITS_IDREGS_BASE,
-		vgic_mmio_read_its_idregs, its_mmio_write_wi, 0x30,
+		vgic_mmio_read_its_idregs, its_mmio_write_wi, NULL, 0x30,
 		VGIC_ACCESS_32bit),
 };
 
@@ -1448,14 +1449,63 @@ static void vgic_its_destroy(struct kvm_device *kvm_dev)
 int vgic_its_has_attr_regs(struct kvm_device *dev,
 			   struct kvm_device_attr *attr)
 {
-	return -ENXIO;
+	const struct vgic_register_region *region;
+	struct vgic_io_device iodev = {
+		.regions = its_registers,
+		.nr_regions = ARRAY_SIZE(its_registers),
+	};
+	gpa_t offset;
+
+	offset = attr->attr;
+
+	region = vgic_find_mmio_region(iodev.regions,
+				       iodev.nr_regions,
+				       offset);
+	if (!region)
+		return -ENXIO;
+	return 0;
 }
 
 int vgic_its_attr_regs_access(struct kvm_device *dev,
 			      struct kvm_device_attr *attr,
 			      u64 *reg, bool is_write)
 {
-	return -ENXIO;
+	const struct vgic_register_region *region;
+	struct vgic_io_device iodev = {
+		.regions = its_registers,
+		.nr_regions = ARRAY_SIZE(its_registers),
+	};
+	struct vgic_its *its = dev->private;
+	gpa_t addr, offset;
+	unsigned int len;
+
+	if (IS_VGIC_ADDR_UNDEF(its->vgic_its_base))
+		return -ENXIO;
+
+	offset = attr->attr;
+	if (offset & 0x7)
+		return -EINVAL;
+
+	addr = its->vgic_its_base + offset;
+
+	region = vgic_find_mmio_region(iodev.regions,
+				       iodev.nr_regions,
+				       offset);
+	if (!region)
+		return -ENXIO;
+
+	len = region->access_flags & VGIC_ACCESS_64bit ? 8 : 4;
+
+	if (is_write) {
+		if (region->uaccess_its_write)
+			region->uaccess_its_write(dev->kvm, its, addr,
+						  len, *reg);
+		else
+			region->its_write(dev->kvm, its, addr, len, *reg);
+	} else {
+		*reg = region->its_read(dev->kvm, its, addr, len);
+	}
+	return 0;
 }
 
 static int vgic_its_has_attr(struct kvm_device *dev,
diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
index ecf8bfb..4ab93cf 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -36,8 +36,13 @@ struct vgic_register_region {
 	};
 	unsigned long (*uaccess_read)(struct kvm_vcpu *vcpu, gpa_t addr,
 				      unsigned int len);
-	void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
-			      unsigned int len, unsigned long val);
+	union {
+		void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
+				      unsigned int len, unsigned long val);
+		void (*uaccess_its_write)(struct kvm *kvm, struct vgic_its *its,
+					  gpa_t addr, unsigned int len,
+					  unsigned long val);
+	};
 };
 
 extern struct kvm_io_device_ops kvm_io_gic_ops;
-- 
2.5.5

[RFC 05/13] KVM: arm64: ITS: Implement vgic_mmio_uaccess_write_its_creadr

[Eric Auger]

GITS_CREADR needs to be restored so let's implement the associated
uaccess_write_its callback.

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

---
---
 virt/kvm/arm/vgic/vgic-its.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 55671585..e174220 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1225,6 +1225,25 @@ static unsigned long vgic_mmio_read_its_creadr(struct kvm *kvm,
 	return extract_bytes(its->creadr, addr & 0x7, len);
 }
 
+static void vgic_mmio_uaccess_write_its_creadr(struct kvm *kvm,
+					       struct vgic_its *its,
+					       gpa_t addr, unsigned int len,
+					       unsigned long val)
+{
+	u32 reg;
+
+	mutex_lock(&its->cmd_lock);
+	reg = update_64bit_reg(its->creadr, addr & 7, len, val);
+	reg = ITS_CMD_OFFSET(reg);
+	if (reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
+		mutex_unlock(&its->cmd_lock);
+		return;
+	}
+
+	its->creadr = reg;
+	mutex_unlock(&its->cmd_lock);
+}
+
 #define BASER_INDEX(addr) (((addr) / sizeof(u64)) & 0x7)
 static unsigned long vgic_mmio_read_its_baser(struct kvm *kvm,
 					      struct vgic_its *its,
@@ -1320,7 +1339,8 @@ static struct vgic_register_region its_registers[] = {
 		vgic_mmio_read_its_cwriter, vgic_mmio_write_its_cwriter, NULL,
 		8, VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
 	REGISTER_ITS_DESC(GITS_CREADR,
-		vgic_mmio_read_its_creadr, its_mmio_write_wi, NULL, 8,
+		vgic_mmio_read_its_creadr, its_mmio_write_wi,
+		vgic_mmio_uaccess_write_its_creadr, 8,
 		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
 	REGISTER_ITS_DESC(GITS_BASER,
 		vgic_mmio_read_its_baser, vgic_mmio_write_its_baser, NULL, 0x40,
-- 
2.5.5

[RFC 06/13] KVM: arm64: ITS: Expose ITT_Entry_Size in GITS_TYPER

[Eric Auger]

An ITT_Entry_Size of 2x8Bytes is reported to the guest
to provision for ITTE state storage.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/linux/irqchip/arm-gic-v3.h | 1 +
 virt/kvm/arm/vgic/vgic-its.c       | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 170e00a..8cfd81bc 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -233,6 +233,7 @@
 #define GITS_CTLR_QUIESCENT		(1U << 31)
 
 #define GITS_TYPER_PLPIS		(1UL << 0)
+#define GITS_TYPER_ITT_ENTRY_SIZE_SHIFT	4
 #define GITS_TYPER_IDBITS_SHIFT		8
 #define GITS_TYPER_DEVBITS_SHIFT	13
 #define GITS_TYPER_DEVBITS(r)		((((r) >> GITS_TYPER_DEVBITS_SHIFT) & 0x1f) + 1)
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index e174220..96378b8 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -33,6 +33,8 @@
 #include "vgic.h"
 #include "vgic-mmio.h"
 
+#define ITTE_SIZE 16
+
 /*
  * Creates a new (reference to a) struct vgic_irq for a given LPI.
  * If this LPI is already mapped on another ITS, we increase its refcount
@@ -399,6 +401,7 @@ static unsigned long vgic_mmio_read_its_typer(struct kvm *kvm,
 	 */
 	reg |= 0x0f << GITS_TYPER_DEVBITS_SHIFT;
 	reg |= 0x0f << GITS_TYPER_IDBITS_SHIFT;
+	reg |= ITTE_SIZE << GITS_TYPER_ITT_ENTRY_SIZE_SHIFT;
 
 	return extract_bytes(reg, addr & 7, len);
 }
-- 
2.5.5

[RFC 07/13] KVM: arm64: ITS: Change entry_size and indirect bit in BASER

[Eric Auger]

Change the device table entry_size to 16 bytes instead of 8.
We also Store the device and collection device in the its
struct.

The patch also clears the indirect bit for the device BASER.
The indirect bit is set as read-only.

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

---

TODO: investigate support of 2 level tables, ie. enabling
Indirect = 1. Support of 2 level tables is implementation
defined.
---
 include/kvm/arm_vgic.h       |  3 +++
 virt/kvm/arm/vgic/vgic-its.c | 25 ++++++++++++++++---------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 730a18a..e6fc727 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -156,6 +156,9 @@ struct vgic_its {
 	u64			baser_device_table;
 	u64			baser_coll_table;
 
+	size_t			device_table_size;
+	size_t			collection_table_size;
+
 	/* Protects the command queue */
 	struct mutex		cmd_lock;
 	u64			cbaser;
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 96378b8..358ae38 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -34,6 +34,8 @@
 #include "vgic-mmio.h"
 
 #define ITTE_SIZE 16
+#define DEV_ENTRY_SIZE 16
+#define COLL_ENTRY_SIZE 16
 
 /*
  * Creates a new (reference to a) struct vgic_irq for a given LPI.
@@ -1269,14 +1271,16 @@ static unsigned long vgic_mmio_read_its_baser(struct kvm *kvm,
 	return extract_bytes(reg, addr & 7, len);
 }
 
-#define GITS_BASER_RO_MASK	(GENMASK_ULL(52, 48) | GENMASK_ULL(58, 56))
+#define GITS_BASER_RO_MASK	(GENMASK_ULL(52, 48) | GENMASK_ULL(58, 56) | \
+				 GENMASK_ULL(62, 62))
 static void vgic_mmio_write_its_baser(struct kvm *kvm,
 				      struct vgic_its *its,
 				      gpa_t addr, unsigned int len,
 				      unsigned long val)
 {
 	u64 entry_size, device_type;
-	u64 reg, *regptr, clearbits = 0;
+	u64 reg, *regptr;
+	size_t *psize;
 
 	/* When GITS_CTLR.Enable is 1, we ignore write accesses. */
 	if (its->enabled)
@@ -1285,14 +1289,15 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
 	switch (BASER_INDEX(addr)) {
 	case 0:
 		regptr = &its->baser_device_table;
-		entry_size = 8;
+		entry_size = DEV_ENTRY_SIZE;
 		device_type = GITS_BASER_TYPE_DEVICE;
+		psize = &its->device_table_size;
 		break;
 	case 1:
 		regptr = &its->baser_coll_table;
-		entry_size = 8;
+		entry_size = COLL_ENTRY_SIZE;
 		device_type = GITS_BASER_TYPE_COLLECTION;
-		clearbits = GITS_BASER_INDIRECT;
+		psize = &its->collection_table_size;
 		break;
 	default:
 		return;
@@ -1300,12 +1305,13 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
 
 	reg = update_64bit_reg(*regptr, addr & 7, len, val);
 	reg &= ~GITS_BASER_RO_MASK;
-	reg &= ~clearbits;
 
 	reg |= (entry_size - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
 	reg |= device_type << GITS_BASER_TYPE_SHIFT;
 	reg = vgic_sanitise_its_baser(reg);
 
+	*psize = ((reg & 0xFF) + 1) << 16;
+
 	*regptr = reg;
 }
 
@@ -1390,7 +1396,6 @@ static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its)
 	(GIC_BASER_CACHEABILITY(GITS_BASER, INNER, RaWb)		| \
 	 GIC_BASER_CACHEABILITY(GITS_BASER, OUTER, SameAsInner)		| \
 	 GIC_BASER_SHAREABILITY(GITS_BASER, InnerShareable)		| \
-	 ((8ULL - 1) << GITS_BASER_ENTRY_SIZE_SHIFT)			| \
 	 GITS_BASER_PAGE_SIZE_64K)
 
 #define INITIAL_PROPBASER_VALUE						  \
@@ -1423,9 +1428,11 @@ static int vgic_its_create(struct kvm_device *dev, u32 type)
 	its->dev = dev;
 
 	its->baser_device_table = INITIAL_BASER_VALUE			|
-		((u64)GITS_BASER_TYPE_DEVICE << GITS_BASER_TYPE_SHIFT);
+		((u64)GITS_BASER_TYPE_DEVICE << GITS_BASER_TYPE_SHIFT)  |
+		((u64)(DEV_ENTRY_SIZE - 1) << GITS_BASER_ENTRY_SIZE_SHIFT);
 	its->baser_coll_table = INITIAL_BASER_VALUE |
-		((u64)GITS_BASER_TYPE_COLLECTION << GITS_BASER_TYPE_SHIFT);
+		((u64)GITS_BASER_TYPE_COLLECTION << GITS_BASER_TYPE_SHIFT) |
+		((u64)(COLL_ENTRY_SIZE - 1) << GITS_BASER_ENTRY_SIZE_SHIFT);
 	dev->kvm->arch.vgic.propbaser = INITIAL_PROPBASER_VALUE;
 
 	dev->private = its;
-- 
2.5.5

[RFC 08/13] KVM: arm64: ITS: On MAPD interpret and store itt_addr and size

[Eric Auger]

Up to now the ITT_addr and size fields of the MAPD command were
ignored. We will need them to handle save/restore. Let's record
the reconstructed number of eventid bits, the ITT base gpa and
the size in bytes in the its device struct.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 358ae38..32429fd 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -103,6 +103,9 @@ struct its_device {
 
 	/* the head for the list of ITTEs */
 	struct list_head itt_head;
+	gpa_t itt_addr;
+	size_t itt_size;
+	u32 nb_eventid_bits;
 	u32 device_id;
 };
 
@@ -562,6 +565,8 @@ static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
 #define its_cmd_get_collection(cmd)	its_cmd_mask_field(cmd, 2,  0, 16)
 #define its_cmd_get_target_addr(cmd)	its_cmd_mask_field(cmd, 2, 16, 32)
 #define its_cmd_get_validbit(cmd)	its_cmd_mask_field(cmd, 2, 63,  1)
+#define its_cmd_get_ittaddr(cmd)	its_cmd_mask_field(cmd, 2, 8,  44)
+#define its_cmd_get_size(cmd)		its_cmd_mask_field(cmd, 1, 0,  5)
 
 /*
  * The DISCARD command frees an Interrupt Translation Table Entry (ITTE).
@@ -827,6 +832,8 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
 {
 	u32 device_id = its_cmd_get_deviceid(its_cmd);
 	bool valid = its_cmd_get_validbit(its_cmd);
+	size_t size = its_cmd_get_size(its_cmd);
+	gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
 	struct its_device *device;
 
 	if (!vgic_its_check_id(its, its->baser_device_table, device_id))
@@ -854,6 +861,9 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
 		return -ENOMEM;
 
 	device->device_id = device_id;
+	device->itt_addr = itt_addr << 8;
+	device->nb_eventid_bits = size + 1;
+	device->itt_size = ((2 << (size + 1)) - 1) * 16;
 	INIT_LIST_HEAD(&device->itt_head);
 
 	list_add_tail(&device->dev_list, &its->device_list);
-- 
2.5.5

[RFC 09/13] KVM: arm64: ITS: KVM_DEV_ARM_VGIC_GRP_ITS_TABLES group

[Eric Auger]

Introduce a new group aiming at saving/restoring the ITS
tables to/from the guest memory.

We hold the its lock during the save and restore to prevent
any command from being executed. This also garantees the LPI
list is not going to change and no MSI injection can happen
during the operation.

At this stage the functionality is not yet implemented. Only
the skeleton is put in place.

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

---

At the end of the restore I trigger a map_resources. This aims
at accomodating the virtio-net-pci guest use case. On restore I
can see the virtio-net-pci device sends MSI before the first
VCPU run. The fact we are not able to handle MSIs at that stage
stalls the virtio-net-pci device. We may fix this issue at QEMU
level instead.
---
 arch/arm/include/uapi/asm/kvm.h   |   1 +
 arch/arm64/include/uapi/asm/kvm.h |   1 +
 virt/kvm/arm/vgic/vgic-its.c      | 120 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 121 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index f6a167c..84fdfb0 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -193,6 +193,7 @@ struct kvm_arch_memory_slot {
 #define KVM_DEV_ARM_VGIC_CPU_SYSREGS    6
 #define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7
 #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)
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 8850e9b..31e1137 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -213,6 +213,7 @@ struct kvm_arch_memory_slot {
 #define KVM_DEV_ARM_VGIC_CPU_SYSREGS    6
 #define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7
 #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)
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 32429fd..aeb9d08 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1548,6 +1548,117 @@ int vgic_its_attr_regs_access(struct kvm_device *dev,
 	return 0;
 }
 
+/**
+ * vgic_its_flush_pending_tables - Flush the pending tables into guest RAM
+ */
+static int vgic_its_flush_pending_tables(struct kvm *kvm,
+					 struct vgic_its *its)
+{
+	return -ENXIO;
+}
+
+/**
+ * vgic_its_restore_pending_tables - Restore the pending tables from guest
+ * RAM to internal data structs
+ */
+static int vgic_its_restore_pending_tables(struct kvm *kvm,
+					   struct vgic_its *its)
+{
+	return -ENXIO;
+}
+
+/**
+ * vgic_its_flush_device_tables - flush the device table and all ITT
+ * into guest RAM
+ */
+static int vgic_its_flush_device_tables(struct kvm *kvm, struct vgic_its *its)
+{
+	return -ENXIO;
+}
+
+/**
+ * vgic_its_restore_device_tables - restore the device table and all ITT
+ * from guest RAM to internal data structs
+ */
+static int vgic_its_restore_device_tables(struct kvm *kvm,
+					  struct vgic_its *its)
+{
+	return -ENXIO;
+}
+
+/**
+ * vgic_its_flush_collection_table - flush the collection table into
+ * guest RAM
+ */
+static int vgic_its_flush_collection_table(struct kvm *kvm,
+					   struct vgic_its *its)
+{
+	return -ENXIO;
+}
+
+/**
+ * vgic_its_restore_collection_table - reads the collection table
+ * in guest memory and restores the ITS internal state. Requires the
+ * BASER registers to be restored before.
+ */
+static int vgic_its_restore_collection_table(struct kvm *kvm,
+					     struct vgic_its *its)
+{
+	return -ENXIO;
+}
+
+/**
+ * vgic_its_table_flush - Flush all the tables into guest RAM
+ */
+static int vgic_its_table_flush(struct kvm *kvm,  struct vgic_its *its)
+{
+	int ret;
+
+	mutex_lock(&its->its_lock);
+
+	ret = vgic_its_flush_pending_tables(kvm, its);
+	if (ret)
+		goto out;
+	ret = vgic_its_flush_device_tables(kvm, its);
+	if (ret)
+		goto out;
+
+	ret = vgic_its_flush_collection_table(kvm, its);
+out:
+	mutex_unlock(&its->its_lock);
+	return ret;
+}
+
+/**
+ * vgic_its_table_restore - Restore all tables from guest RAM to internal
+ * data structs
+ */
+static int vgic_its_table_restore(struct kvm *kvm,  struct vgic_its *its)
+{
+	int ret;
+
+	mutex_lock(&its->its_lock);
+	ret = vgic_its_restore_collection_table(kvm, its);
+	if (ret)
+		goto out;
+
+	ret = vgic_its_restore_device_tables(kvm, its);
+	if (ret)
+		goto out;
+
+	ret = vgic_its_restore_pending_tables(kvm, its);
+out:
+	mutex_unlock(&its->its_lock);
+
+	/**
+	 *  HACK: In real use case we observe MSI injection before
+	 *  the first CPU run. This is likely to stall virtio-net-pci
+	 *  This may need a fix at user-space level instead
+	 */
+	ret = kvm_vgic_map_resources(kvm);
+	return ret;
+}
+
 static int vgic_its_has_attr(struct kvm_device *dev,
 			     struct kvm_device_attr *attr)
 {
@@ -1566,6 +1677,8 @@ static int vgic_its_has_attr(struct kvm_device *dev,
 		break;
 	case KVM_DEV_ARM_VGIC_GRP_ITS_REGS:
 		return vgic_its_has_attr_regs(dev, attr);
+	case KVM_DEV_ARM_VGIC_GRP_ITS_TABLES:
+		return 0;
 	}
 	return -ENXIO;
 }
@@ -1614,6 +1727,8 @@ static int vgic_its_set_attr(struct kvm_device *dev,
 
 		return vgic_its_attr_regs_access(dev, attr, &reg, true);
 	}
+	case KVM_DEV_ARM_VGIC_GRP_ITS_TABLES:
+		return vgic_its_table_restore(dev->kvm, its);
 	}
 	return -ENXIO;
 }
@@ -1621,9 +1736,10 @@ static int vgic_its_set_attr(struct kvm_device *dev,
 static int vgic_its_get_attr(struct kvm_device *dev,
 			     struct kvm_device_attr *attr)
 {
+	struct vgic_its *its = dev->private;
+
 	switch (attr->group) {
 	case KVM_DEV_ARM_VGIC_GRP_ADDR: {
-		struct vgic_its *its = dev->private;
 		u64 addr = its->vgic_its_base;
 		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
 		unsigned long type = (unsigned long)attr->attr;
@@ -1644,6 +1760,8 @@ static int vgic_its_get_attr(struct kvm_device *dev,
 		if (ret)
 			return ret;
 		return put_user(reg, uaddr);
+	case KVM_DEV_ARM_VGIC_GRP_ITS_TABLES:
+		return vgic_its_table_flush(dev->kvm, its);
 	}
 	default:
 		return -ENXIO;
-- 
2.5.5

[RFC 10/13] KVM: arm64: ITS: vgic_its_alloc_itte/device

[Eric Auger]

Add two new helpers to allocate an its itte and an its device.
This will avoid duplication on restore path.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 71 ++++++++++++++++++++++++++++++--------------
 1 file changed, 48 insertions(+), 23 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index aeb9d08..07bf180 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -732,6 +732,26 @@ static void vgic_its_free_collection(struct vgic_its *its, u32 coll_id)
 	kfree(collection);
 }
 
+static int vgic_its_alloc_itte(struct its_device *device,
+			       struct its_itte **ittep,
+			       struct its_collection *collection,
+			       u32 lpi_id, u32 event_id)
+{
+	struct its_itte *itte;
+
+	itte = kzalloc(sizeof(itte), GFP_KERNEL);
+	if (!itte)
+		return -ENOMEM;
+
+	itte->event_id	= event_id;
+	itte->collection = collection;
+	itte->lpi = lpi_id;
+
+	list_add_tail(&itte->itte_list, &device->itt_head);
+	*ittep = itte;
+	return 0;
+}
+
 /*
  * The MAPTI and MAPI commands map LPIs to ITTEs.
  * Must be called with its_lock mutex held.
@@ -745,7 +765,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	struct its_itte *itte;
 	struct its_device *device;
 	struct its_collection *collection, *new_coll = NULL;
-	int lpi_nr;
+	int lpi_nr, ret;
 	struct vgic_irq *irq;
 
 	device = find_its_device(its, device_id);
@@ -772,19 +792,13 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 		new_coll = collection;
 	}
 
-	itte = kzalloc(sizeof(struct its_itte), GFP_KERNEL);
-	if (!itte) {
+	ret = vgic_its_alloc_itte(device, &itte, collection, lpi_nr, event_id);
+	if (ret) {
 		if (new_coll)
 			vgic_its_free_collection(its, coll_id);
-		return -ENOMEM;
+		return ret;
 	}
 
-	itte->event_id	= event_id;
-	list_add_tail(&itte->itte_list, &device->itt_head);
-
-	itte->collection = collection;
-	itte->lpi = lpi_nr;
-
 	irq = vgic_add_lpi(kvm, lpi_nr);
 	if (IS_ERR(irq)) {
 		if (new_coll)
@@ -823,6 +837,29 @@ static void vgic_its_unmap_device(struct kvm *kvm, struct its_device *device)
 	kfree(device);
 }
 
+static int vgic_its_alloc_device(struct vgic_its *its,
+				 struct its_device **devp,
+				 u32 device_id, gpa_t itt_addr_field,
+				 size_t size_field)
+{
+	struct its_device *device;
+
+	device = kzalloc(sizeof(*device), GFP_KERNEL);
+	if (!device)
+		return -ENOMEM;
+
+	device->device_id = device_id;
+	device->itt_addr = itt_addr_field << 8;
+	device->nb_eventid_bits = size_field + 1;
+	device->itt_size = ((2 << (size_field + 1)) - 1) * 16;
+	INIT_LIST_HEAD(&device->itt_head);
+
+	list_add_tail(&device->dev_list, &its->device_list);
+	*devp = device;
+
+	return 0;
+}
+
 /*
  * MAPD maps or unmaps a device ID to Interrupt Translation Tables (ITTs).
  * Must be called with the its_lock mutex held.
@@ -856,19 +893,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
 	if (!valid)
 		return 0;
 
-	device = kzalloc(sizeof(struct its_device), GFP_KERNEL);
-	if (!device)
-		return -ENOMEM;
-
-	device->device_id = device_id;
-	device->itt_addr = itt_addr << 8;
-	device->nb_eventid_bits = size + 1;
-	device->itt_size = ((2 << (size + 1)) - 1) * 16;
-	INIT_LIST_HEAD(&device->itt_head);
-
-	list_add_tail(&device->dev_list, &its->device_list);
-
-	return 0;
+	return vgic_its_alloc_device(its, &device, device_id, itt_addr, size);
 }
 
 /*
-- 
2.5.5

[RFC 11/13] KVM: arm64: ITS: Collection table save/restore

[Eric Auger]

The flush path copies the collection entries into guest RAM
at the GPA specified in the BASER register. This obviously
requires the BASER to be set. The last written element
is a dummy collection table entry with valid bit set to 0.

On restore path we re-allocate the collection objects.

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

---
---
 virt/kvm/arm/vgic/vgic-its.c | 66 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 64 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 07bf180..a1861b8 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1618,7 +1618,39 @@ static int vgic_its_restore_device_tables(struct kvm *kvm,
 static int vgic_its_flush_collection_table(struct kvm *kvm,
 					   struct vgic_its *its)
 {
-	return -ENXIO;
+	struct its_collection *collection;
+	u64 val, *ptr;
+	size_t max_size, filled = 0;
+	int ret;
+
+	ptr = (u64 *)(its->baser_coll_table & 0x0000FFFFFFFFF000);
+	if (!ptr)
+		return 0;
+
+	max_size = its->collection_table_size;
+
+	list_for_each_entry(collection, &its->collection_list, coll_list) {
+		if (filled == max_size)
+			return -ENOSPC;
+		val = ((u64)1 << 63 |
+		       ((u64)collection->target_addr << 16) |
+		       collection->collection_id);
+		ret = kvm_write_guest(kvm, (gpa_t)ptr++, &val, 8);
+		if (ret)
+			return ret;
+		filled += COLL_ENTRY_SIZE;
+	}
+
+	if (filled == max_size)
+		return 0;
+
+	/*
+	 * table is not fully filled, add a last dummy element
+	 * with valid bit unset
+	 */
+	val = 0;
+	ret = kvm_write_guest(kvm, (gpa_t)ptr, &val, 8);
+	return ret;
 }
 
 /**
@@ -1629,7 +1661,37 @@ static int vgic_its_flush_collection_table(struct kvm *kvm,
 static int vgic_its_restore_collection_table(struct kvm *kvm,
 					     struct vgic_its *its)
 {
-	return -ENXIO;
+	struct its_collection *collection;
+	size_t max_size, read = 0;
+	u64 val, *ptr;
+	bool valid = true;
+	int ret;
+
+	ptr = (u64 *)(its->baser_coll_table & 0x0000FFFFFFFFF000);
+	if (!ptr)
+		return 0;
+
+	max_size = its->collection_table_size;
+
+	while (read < max_size) {
+		u32 target_addr;
+		u32 coll_id;
+
+		ret = kvm_read_guest(kvm, (gpa_t)ptr++, &val, 8);
+		if (ret)
+			return ret;
+		valid = val >> 63;
+		if (!valid)
+			break;
+		target_addr = (u32)(val >> 16);
+		coll_id = val & 0xFFFF;
+		ret = vgic_its_alloc_collection(its, &collection, coll_id);
+		if (ret)
+			return ret;
+		collection->target_addr = target_addr;
+		read += COLL_ENTRY_SIZE;
+	}
+	return 0;
 }
 
 /**
-- 
2.5.5

[RFC 12/13] KVM: arm64: ITS: Device and translation table flush

[Eric Auger]

This patch flushes the device table into guest RAM, at
the address specified by the BASER register. The last
written element is a dummy element with valid bit set to
zero.

For each device listed in the device table, we also flush
the translation table into the address specific in the
corresponding device table entry.

On restore, devices are re-allocated and their itte are
re-built. LPI is re-created and its config gets updated.

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

---
---
 virt/kvm/arm/vgic/vgic-its.c | 158 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 156 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index a1861b8..16a9aac 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1592,13 +1592,129 @@ static int vgic_its_restore_pending_tables(struct kvm *kvm,
 	return -ENXIO;
 }
 
+static int vgic_its_flush_itt(struct kvm *kvm, struct its_device *device)
+{
+	struct its_itte *itte;
+	size_t max_size, filled = 0;
+	u64 val, *ptr;
+	int ret;
+
+	ptr = (u64 *)device->itt_addr;
+	max_size = device->itt_size;
+
+	list_for_each_entry(itte, &device->itt_head, itte_list) {
+		if (filled == max_size)
+			return -ENOSPC;
+		val = ((u64)device->device_id << 32) | (1ULL << 16) |
+			itte->collection->collection_id;
+		ret = kvm_write_guest(kvm, (gpa_t)ptr++, &val, 8);
+		if (ret)
+			return ret;
+		val = (u64)itte->lpi << 32 | itte->event_id;
+		ret = kvm_write_guest(kvm, (gpa_t)ptr++, &val, 8);
+		if (ret)
+			return ret;
+		filled += ITTE_SIZE;
+	}
+	return 0;
+}
+
+static int vgic_its_restore_itt(struct kvm *kvm, struct vgic_its *its,
+				struct its_device *dev)
+{
+	u64 val, *ptr = (u64 *)dev->itt_addr;
+	size_t max_size, read = 0;
+	bool valid;
+
+	max_size = dev->itt_size;
+
+	while (read < max_size) {
+		u32 coll_id, device_id, lpi_id, event_id;
+		struct its_collection *collection;
+		struct its_itte *itte;
+		struct vgic_irq *irq;
+		int ret;
+
+		ret = kvm_read_guest(kvm, (gpa_t)ptr++, &val, 8);
+		if (ret)
+			return ret;
+		valid = val & 0x10000;
+		if (!valid)
+			break;
+		coll_id = val & 0xFFFF;
+		device_id = val >> 32;
+		ret = kvm_read_guest(kvm, (gpa_t)ptr++, &val, 8);
+		if (ret)
+			return ret;
+		lpi_id = val >> 32;
+		event_id = val & 0xFFFFFFFFFFFFFFFF;
+		collection = find_collection(its, coll_id);
+		if (!collection)
+			return -EINVAL;
+		ret = vgic_its_alloc_itte(dev, &itte, collection,
+					  lpi_id, event_id);
+		if (ret)
+			return ret;
+
+		irq = vgic_add_lpi(kvm, lpi_id);
+		if (IS_ERR(irq))
+			return PTR_ERR(irq);
+		itte->irq = irq;
+
+		/* restores the configuration of the LPI */
+		ret = update_lpi_config(kvm, irq, NULL);
+		if (ret)
+			return ret;
+
+		update_affinity_itte(kvm, itte);
+		read += ITTE_SIZE;
+	}
+	return 0;
+}
+
 /**
  * vgic_its_flush_device_tables - flush the device table and all ITT
  * into guest RAM
  */
 static int vgic_its_flush_device_tables(struct kvm *kvm, struct vgic_its *its)
 {
-	return -ENXIO;
+	struct its_device *dev;
+	size_t max_size, filled = 0;
+	u64 val, *ptr;
+	int ret;
+
+	ptr = (u64 *)(its->baser_device_table & 0x0000FFFFFFFFF000);
+
+	/* max size of the device table (64kB page size) */
+	max_size = its->device_table_size;
+
+	list_for_each_entry(dev, &its->device_list, dev_list) {
+		if (filled == max_size)
+			return -ENOSPC;
+		ret = vgic_its_flush_itt(kvm, dev);
+		if (ret)
+			return ret;
+		val = ((u64)dev->device_id << 32) | (1 << 5) |
+				(dev->nb_eventid_bits - 1);
+		ret = kvm_write_guest(kvm, (gpa_t)ptr++, &val, 8);
+		if (ret)
+			return ret;
+		val = dev->itt_addr >> 8;
+		ret = kvm_write_guest(kvm, (gpa_t)ptr++, &val, 8);
+		if (ret)
+			return ret;
+		filled += DEV_ENTRY_SIZE;
+	}
+	if (filled == max_size)
+		return 0;
+
+	/**
+	 * table is not fully filled, add a last dummy element
+	 * element with valid but unset
+	 */
+	val = 0;
+	ret = kvm_write_guest(kvm, (gpa_t)ptr, &val, 8);
+	return ret;
 }
 
 /**
@@ -1608,7 +1724,45 @@ static int vgic_its_flush_device_tables(struct kvm *kvm, struct vgic_its *its)
 static int vgic_its_restore_device_tables(struct kvm *kvm,
 					  struct vgic_its *its)
 {
-	return -ENXIO;
+	size_t max_size, read = 0;
+	struct its_device *dev;
+	u64 val, *ptr;
+	bool valid;
+	int ret;
+
+	ptr = (u64 *)(its->baser_device_table & 0x0000FFFFFFFFF000);
+	if (!ptr)
+		return 0;
+
+	max_size = its->device_table_size;
+
+	while (read < max_size) {
+		u32 device_id;
+		gpa_t itt_addr;
+		size_t size;
+
+		ret = kvm_read_guest(kvm, (gpa_t)ptr++, &val, 8);
+		if (ret)
+			return ret;
+		valid = val & 0x20;
+		if (!valid)
+			break;
+		device_id =  val >> 32;
+		size = val & 0x1F;
+		ret = kvm_read_guest(kvm, (gpa_t)ptr++, &val, 8);
+		if (ret)
+			return ret;
+		itt_addr = val & 0x000FFFFFFFFFFFFF;
+		ret = vgic_its_alloc_device(its, &dev, device_id,
+					    itt_addr, size);
+		if (ret)
+			return ret;
+		ret = vgic_its_restore_itt(kvm, its, dev);
+		if (ret)
+			return ret;
+		read += DEV_ENTRY_SIZE;
+	}
+	return 0;
 }
 
 /**
-- 
2.5.5

[RFC 13/13] KVM: arm64: ITS: Pending table save/restore

[Eric Auger]

Save and restore the pending tables.

Pending table restore obviously requires the pendbaser to be
already set.

We also make sure the first kB of the pending table is zeroed.

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

---

Not sure what to do with this 1st kB. Should we really care?

Maybe we should read it instead from guest RAM and return
an error if it is non zero, as the spec says on init it should be
zero. Then we do not touch it.
---
 virt/kvm/arm/vgic/vgic-its.c | 94 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 16a9aac..4a2b4de 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -316,6 +316,29 @@ static u32 max_lpis_propbaser(u64 propbaser)
 	return 1U << min(nr_idbits, INTERRUPT_ID_BITS_ITS);
 }
 
+/**
+ * its_zero_pending_table_first1kB - Sets the first kB of the
+ * pending table to 0
+ * @vcpu: vcpu handle the pending table is attached to
+ *
+ * The content of the 1st kB is implementation defined. We force
+ * it to 0. Note the spec says it should already contain zeros on
+ * initial allocation and it must be visible to redist, else the
+ * behavior is unpredicatable
+ */
+static int its_zero_pending_table_first1kB(struct kvm_vcpu *vcpu)
+{
+	gpa_t pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser);
+	u8 *tmp;
+	int ret;
+
+	tmp = kzalloc(1024, GFP_KERNEL);
+	ret = kvm_write_guest(vcpu->kvm, (gpa_t)pendbase, tmp, 1024);
+	kfree(tmp);
+
+	return ret;
+}
+
 /*
  * Scan the whole LPI pending table and sync the pending bit in there
  * with our own data structures. This relies on the LPI being
@@ -1399,6 +1422,8 @@ void vgic_enable_lpis(struct kvm_vcpu *vcpu)
 {
 	if (!(vcpu->arch.vgic_cpu.pendbaser & GICR_PENDBASER_PTZ))
 		its_sync_lpi_pending_table(vcpu);
+
+	its_zero_pending_table_first1kB(vcpu);
 }
 
 static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its)
@@ -1579,7 +1604,47 @@ int vgic_its_attr_regs_access(struct kvm_device *dev,
 static int vgic_its_flush_pending_tables(struct kvm *kvm,
 					 struct vgic_its *its)
 {
-	return -ENXIO;
+	struct vgic_dist *dist = &kvm->arch.vgic;
+	struct vgic_irq *irq;
+	int ret;
+
+	/**
+	 * we do not take the dist->lpi_list_lock this we have a garantee
+	 * the LPI list is not touched while the cmd and its lock are held
+	 */
+	list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
+		struct kvm_vcpu *vcpu;
+		gpa_t pendbase, ptr;
+		bool stored;
+		u8 val;
+
+		vcpu = irq->target_vcpu;
+		if (!vcpu)
+			return -EINVAL;
+
+		pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser);
+
+		ptr = pendbase + (irq->intid / BITS_PER_BYTE);
+
+		ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1);
+		if (ret)
+			return ret;
+
+		stored = val & (irq->intid % BITS_PER_BYTE);
+		if (stored == irq->pending)
+			continue;
+
+		if (irq->pending)
+			val |= 1 << (irq->intid % BITS_PER_BYTE);
+		else
+			val &= ~(1 << (irq->intid % BITS_PER_BYTE));
+
+		ret = kvm_write_guest(kvm, (gpa_t)ptr, &val, 1);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 /**
@@ -1589,7 +1654,32 @@ static int vgic_its_flush_pending_tables(struct kvm *kvm,
 static int vgic_its_restore_pending_tables(struct kvm *kvm,
 					   struct vgic_its *its)
 {
-	return -ENXIO;
+	struct vgic_irq *irq;
+	struct vgic_dist *dist = &kvm->arch.vgic;
+	int ret;
+
+	list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
+		struct kvm_vcpu *vcpu;
+		gpa_t pendbase, ptr;
+		u8 val;
+
+		vcpu = irq->target_vcpu;
+		if (!vcpu)
+			return -EINVAL;
+
+		if (!(vcpu->arch.vgic_cpu.pendbaser & GICR_PENDBASER_PTZ))
+			return 0;
+
+		pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser);
+
+		ptr = pendbase + (irq->intid / BITS_PER_BYTE);
+
+		ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1);
+		if (ret)
+			return ret;
+		irq->pending = val & (1 << (irq->intid % BITS_PER_BYTE));
+	}
+	return 0;
 }
 
 static int vgic_its_flush_itt(struct kvm *kvm, struct its_device *device)
-- 
2.5.5

Re: [RFC 01/13] KVM: arm/arm64: Add vITS save/restore API documentation

[Marc Zyngier]

Hi Eric,

On 12/01/17 15:56, Eric Auger wrote:
> Add description for how to access vITS registers and how to flush/restore
> vITS tables into/from memory
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 ++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> index 6081a5b..bd74613 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> @@ -36,3 +36,73 @@ Groups:
>      -ENXIO:  ITS not properly configured as required prior to setting
>               this attribute
>      -ENOMEM: Memory shortage when allocating ITS internal data
> +
> +  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
> +  Attributes:
> +      The attr field of kvm_device_attr encodes the offset of the
> +      ITS register, relative to the ITS control frame base address
> +      (ITS_base).
> +
> +      kvm_device_attr.addr points to a __u64 value whatever the width
> +      of the addressed register (32/64 bits).
> +
> +      Writes to read-only registers are ignored by the kernel except
> +      for a single register, GITS_READR. Normally this register is RO
> +      but it needs to be restored otherwise commands in the queue will
> +      be re-executed after CWRITER setting.
> +
> +      For other registers, Getting or setting a register has the same
> +      effect as reading/writing the register on real hardware.
> +  Errors:
> +    -ENXIO: Offset does not correspond to any supported register
> +    -EFAULT: Invalid user pointer for attr->addr
> +    -EINVAL: Offset is not 64-bit aligned
> +
> +  KVM_DEV_ARM_VGIC_GRP_ITS_TABLES
> +  Attributes
> +       The attr field of kvm_device_attr is not used.
> +
> +       request the flush-save/restore of the ITS tables, namely
> +       the device table, the collection table, all the ITT tables,
> +       the LPI pending tables. On save, the tables are flushed
> +       into guest memory at the location provisionned by the guest

					    provisioned

> +       in GITS_BASER (device and collection tables), on MAPD command
> +       (ITT_addr), GICR_PENDBASERs (pending tables).
> +
> +       This means the GIC should be restored before the ITS and all
> +       ITS registers but the GITS_CTRL must be restored before
> +       restoring the ITS tables.
> +
> +       Note the LPI configuration table is read-only for the
> +       in-kernel ITS and its save/restore goes through the standard
> +       RAM save/restore.
> +
> +       The layout of the tables in guest memory defines an ABI.
> +       The entries are laid out in memory as follows;
> +
> +    Device Table Entry (DTE) layout: entry size = 16 bytes
> +
> +    bits:     | 63   ...  32  | 31 ... 6 | 5 | 4   ...   0 |
> +    values:   |   DeviceID    |   Resv   | V |    Size     |
> +
> +    bits:     | 63 ... 44 | 43  ...  0  |
> +    values:   |    Resv   |  ITT_addr   |

While I appreciate this layout represents the absolute maximum an ITS
could implement, I'm a bit concerned about the amount of memory we may
end-up requiring here. All the ITSs implementations I know of seem to
get away with 8 bytes per entry. Maybe I'm just too worried.

Also, please mention that ITT_addr is actually ITT_addr[51:8], as we're
guaranteed to have an ITT that is 256 byte aligned.

> +
> +    Collection Table Entry (CTE) layout: entry size = 8 bytes
> +
> +    bits:     | 63| 62 ..  52  | 51 ... 16 | 15  ...   0 |
> +    values:   | V |    RES0    |  RDBase   |    ICID     |
> +
> +    Interrupt Translation Table Entry (ITTE) layout: entry size = 16 bytes

The actual name is Interrupt Translation Entry (ITE). I have a patch
renaming this all over the vgic-its.c file...

> +
> +    bits:     | 63   ...  32  | 31 ... 17 | 16 | 15  ...  0 |
> +    values:   |   DeviceID    |    RES0   | V  |    ICID    |
> +
> +    bits:     | 63 ...  32    | 31      ...        0 |
> +    values:   |   pINTID      |        EventID       |

Same concern here. 32bit DevID, EventID and INTID seem a bit over the
top. But maybe we shouldn't be concerned about memory... ;-)

> +
> +    LPI Pending Table layout:
> +
> +    As specified in the ARM Generic Interrupt Controller Architecture
> +    Specification GIC Architecture version 3.0 and version 4. The first
> +    1kB contains only zeros.
> 

You definitely want to relax this. An ITS implementation is allowed (and
actually encouraged) to maintain a coarse map in the first kB, and use
this to quickly scan the table, which would be very useful on restore.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[RFC 01/13] KVM: arm/arm64: Add vITS save/restore API documentation

[Marc Zyngier]

Hi Eric,

On 12/01/17 15:56, Eric Auger wrote:
> Add description for how to access vITS registers and how to flush/restore
> vITS tables into/from memory
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 ++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> index 6081a5b..bd74613 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> @@ -36,3 +36,73 @@ Groups:
>      -ENXIO:  ITS not properly configured as required prior to setting
>               this attribute
>      -ENOMEM: Memory shortage when allocating ITS internal data
> +
> +  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
> +  Attributes:
> +      The attr field of kvm_device_attr encodes the offset of the
> +      ITS register, relative to the ITS control frame base address
> +      (ITS_base).
> +
> +      kvm_device_attr.addr points to a __u64 value whatever the width
> +      of the addressed register (32/64 bits).
> +
> +      Writes to read-only registers are ignored by the kernel except
> +      for a single register, GITS_READR. Normally this register is RO
> +      but it needs to be restored otherwise commands in the queue will
> +      be re-executed after CWRITER setting.
> +
> +      For other registers, Getting or setting a register has the same
> +      effect as reading/writing the register on real hardware.
> +  Errors:
> +    -ENXIO: Offset does not correspond to any supported register
> +    -EFAULT: Invalid user pointer for attr->addr
> +    -EINVAL: Offset is not 64-bit aligned
> +
> +  KVM_DEV_ARM_VGIC_GRP_ITS_TABLES
> +  Attributes
> +       The attr field of kvm_device_attr is not used.
> +
> +       request the flush-save/restore of the ITS tables, namely
> +       the device table, the collection table, all the ITT tables,
> +       the LPI pending tables. On save, the tables are flushed
> +       into guest memory at the location provisionned by the guest

					    provisioned

> +       in GITS_BASER (device and collection tables), on MAPD command
> +       (ITT_addr), GICR_PENDBASERs (pending tables).
> +
> +       This means the GIC should be restored before the ITS and all
> +       ITS registers but the GITS_CTRL must be restored before
> +       restoring the ITS tables.
> +
> +       Note the LPI configuration table is read-only for the
> +       in-kernel ITS and its save/restore goes through the standard
> +       RAM save/restore.
> +
> +       The layout of the tables in guest memory defines an ABI.
> +       The entries are laid out in memory as follows;
> +
> +    Device Table Entry (DTE) layout: entry size = 16 bytes
> +
> +    bits:     | 63   ...  32  | 31 ... 6 | 5 | 4   ...   0 |
> +    values:   |   DeviceID    |   Resv   | V |    Size     |
> +
> +    bits:     | 63 ... 44 | 43  ...  0  |
> +    values:   |    Resv   |  ITT_addr   |

While I appreciate this layout represents the absolute maximum an ITS
could implement, I'm a bit concerned about the amount of memory we may
end-up requiring here. All the ITSs implementations I know of seem to
get away with 8 bytes per entry. Maybe I'm just too worried.

Also, please mention that ITT_addr is actually ITT_addr[51:8], as we're
guaranteed to have an ITT that is 256 byte aligned.

> +
> +    Collection Table Entry (CTE) layout: entry size = 8 bytes
> +
> +    bits:     | 63| 62 ..  52  | 51 ... 16 | 15  ...   0 |
> +    values:   | V |    RES0    |  RDBase   |    ICID     |
> +
> +    Interrupt Translation Table Entry (ITTE) layout: entry size = 16 bytes

The actual name is Interrupt Translation Entry (ITE). I have a patch
renaming this all over the vgic-its.c file...

> +
> +    bits:     | 63   ...  32  | 31 ... 17 | 16 | 15  ...  0 |
> +    values:   |   DeviceID    |    RES0   | V  |    ICID    |
> +
> +    bits:     | 63 ...  32    | 31      ...        0 |
> +    values:   |   pINTID      |        EventID       |

Same concern here. 32bit DevID, EventID and INTID seem a bit over the
top. But maybe we shouldn't be concerned about memory... ;-)

> +
> +    LPI Pending Table layout:
> +
> +    As specified in the ARM Generic Interrupt Controller Architecture
> +    Specification GIC Architecture version 3.0 and version 4. The first
> +    1kB contains only zeros.
> 

You definitely want to relax this. An ITS implementation is allowed (and
actually encouraged) to maintain a coarse map in the first kB, and use
this to quickly scan the table, which would be very useful on restore.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC 07/13] KVM: arm64: ITS: Change entry_size and indirect bit in BASER

[Marc Zyngier]

On 12/01/17 15:56, Eric Auger wrote:
> Change the device table entry_size to 16 bytes instead of 8.
> We also Store the device and collection device in the its
> struct.
> 
> The patch also clears the indirect bit for the device BASER.
> The indirect bit is set as read-only.

Err... Why? We *really* want to continue supporting indirect tables, as
this is a massive memory saver for the guest.

> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> TODO: investigate support of 2 level tables, ie. enabling
> Indirect = 1. Support of 2 level tables is implementation
> defined.

Clearly, that's a regression. What exactly is the issue that decided you
to disable it?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[RFC 07/13] KVM: arm64: ITS: Change entry_size and indirect bit in BASER

[Marc Zyngier]

On 12/01/17 15:56, Eric Auger wrote:
> Change the device table entry_size to 16 bytes instead of 8.
> We also Store the device and collection device in the its
> struct.
> 
> The patch also clears the indirect bit for the device BASER.
> The indirect bit is set as read-only.

Err... Why? We *really* want to continue supporting indirect tables, as
this is a massive memory saver for the guest.

> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> TODO: investigate support of 2 level tables, ie. enabling
> Indirect = 1. Support of 2 level tables is implementation
> defined.

Clearly, that's a regression. What exactly is the issue that decided you
to disable it?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC 06/13] KVM: arm64: ITS: Expose ITT_Entry_Size in GITS_TYPER

[Andre Przywara]

Hi Eric,

On 12/01/17 15:56, Eric Auger wrote:
> An ITT_Entry_Size of 2x8Bytes is reported to the guest
> to provision for ITTE state storage.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  include/linux/irqchip/arm-gic-v3.h | 1 +
>  virt/kvm/arm/vgic/vgic-its.c       | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 170e00a..8cfd81bc 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -233,6 +233,7 @@
>  #define GITS_CTLR_QUIESCENT		(1U << 31)
>  
>  #define GITS_TYPER_PLPIS		(1UL << 0)
> +#define GITS_TYPER_ITT_ENTRY_SIZE_SHIFT	4
>  #define GITS_TYPER_IDBITS_SHIFT		8
>  #define GITS_TYPER_DEVBITS_SHIFT	13
>  #define GITS_TYPER_DEVBITS(r)		((((r) >> GITS_TYPER_DEVBITS_SHIFT) & 0x1f) + 1)
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index e174220..96378b8 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -33,6 +33,8 @@
>  #include "vgic.h"
>  #include "vgic-mmio.h"
>  
> +#define ITTE_SIZE 16
> +
>  /*
>   * Creates a new (reference to a) struct vgic_irq for a given LPI.
>   * If this LPI is already mapped on another ITS, we increase its refcount
> @@ -399,6 +401,7 @@ static unsigned long vgic_mmio_read_its_typer(struct kvm *kvm,
>  	 */
>  	reg |= 0x0f << GITS_TYPER_DEVBITS_SHIFT;
>  	reg |= 0x0f << GITS_TYPER_IDBITS_SHIFT;
> +	reg |= ITTE_SIZE << GITS_TYPER_ITT_ENTRY_SIZE_SHIFT;

The field is defined as the "... number of bytes per translation table
entry, minus one.". So it should be: (ITTE_SIZE - 1) << ...

Cheers,
Andre.

>  
>  	return extract_bytes(reg, addr & 7, len);
>  }
> 

[RFC 06/13] KVM: arm64: ITS: Expose ITT_Entry_Size in GITS_TYPER

[Andre Przywara]

Hi Eric,

On 12/01/17 15:56, Eric Auger wrote:
> An ITT_Entry_Size of 2x8Bytes is reported to the guest
> to provision for ITTE state storage.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  include/linux/irqchip/arm-gic-v3.h | 1 +
>  virt/kvm/arm/vgic/vgic-its.c       | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 170e00a..8cfd81bc 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -233,6 +233,7 @@
>  #define GITS_CTLR_QUIESCENT		(1U << 31)
>  
>  #define GITS_TYPER_PLPIS		(1UL << 0)
> +#define GITS_TYPER_ITT_ENTRY_SIZE_SHIFT	4
>  #define GITS_TYPER_IDBITS_SHIFT		8
>  #define GITS_TYPER_DEVBITS_SHIFT	13
>  #define GITS_TYPER_DEVBITS(r)		((((r) >> GITS_TYPER_DEVBITS_SHIFT) & 0x1f) + 1)
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index e174220..96378b8 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -33,6 +33,8 @@
>  #include "vgic.h"
>  #include "vgic-mmio.h"
>  
> +#define ITTE_SIZE 16
> +
>  /*
>   * Creates a new (reference to a) struct vgic_irq for a given LPI.
>   * If this LPI is already mapped on another ITS, we increase its refcount
> @@ -399,6 +401,7 @@ static unsigned long vgic_mmio_read_its_typer(struct kvm *kvm,
>  	 */
>  	reg |= 0x0f << GITS_TYPER_DEVBITS_SHIFT;
>  	reg |= 0x0f << GITS_TYPER_IDBITS_SHIFT;
> +	reg |= ITTE_SIZE << GITS_TYPER_ITT_ENTRY_SIZE_SHIFT;

The field is defined as the "... number of bytes per translation table
entry, minus one.". So it should be: (ITTE_SIZE - 1) << ...

Cheers,
Andre.

>  
>  	return extract_bytes(reg, addr & 7, len);
>  }
> 

Re: [RFC 06/13] KVM: arm64: ITS: Expose ITT_Entry_Size in GITS_TYPER

[Auger Eric]

Hi Andre,

On 12/01/2017 18:06, Andre Przywara wrote:
> Hi Eric,
> 
> On 12/01/17 15:56, Eric Auger wrote:
>> An ITT_Entry_Size of 2x8Bytes is reported to the guest
>> to provision for ITTE state storage.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  include/linux/irqchip/arm-gic-v3.h | 1 +
>>  virt/kvm/arm/vgic/vgic-its.c       | 3 +++
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>> index 170e00a..8cfd81bc 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -233,6 +233,7 @@
>>  #define GITS_CTLR_QUIESCENT		(1U << 31)
>>  
>>  #define GITS_TYPER_PLPIS		(1UL << 0)
>> +#define GITS_TYPER_ITT_ENTRY_SIZE_SHIFT	4
>>  #define GITS_TYPER_IDBITS_SHIFT		8
>>  #define GITS_TYPER_DEVBITS_SHIFT	13
>>  #define GITS_TYPER_DEVBITS(r)		((((r) >> GITS_TYPER_DEVBITS_SHIFT) & 0x1f) + 1)
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index e174220..96378b8 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -33,6 +33,8 @@
>>  #include "vgic.h"
>>  #include "vgic-mmio.h"
>>  
>> +#define ITTE_SIZE 16
>> +
>>  /*
>>   * Creates a new (reference to a) struct vgic_irq for a given LPI.
>>   * If this LPI is already mapped on another ITS, we increase its refcount
>> @@ -399,6 +401,7 @@ static unsigned long vgic_mmio_read_its_typer(struct kvm *kvm,
>>  	 */
>>  	reg |= 0x0f << GITS_TYPER_DEVBITS_SHIFT;
>>  	reg |= 0x0f << GITS_TYPER_IDBITS_SHIFT;
>> +	reg |= ITTE_SIZE << GITS_TYPER_ITT_ENTRY_SIZE_SHIFT;
> 
> The field is defined as the "... number of bytes per translation table
> entry, minus one.". So it should be: (ITTE_SIZE - 1) << ...

You're perfectly right. I will fix that

Thanks

Eric
> 
> Cheers,
> Andre.
> 
>>  
>>  	return extract_bytes(reg, addr & 7, len);
>>  }
>>

[RFC 06/13] KVM: arm64: ITS: Expose ITT_Entry_Size in GITS_TYPER

[Auger Eric]

Hi Andre,

On 12/01/2017 18:06, Andre Przywara wrote:
> Hi Eric,
> 
> On 12/01/17 15:56, Eric Auger wrote:
>> An ITT_Entry_Size of 2x8Bytes is reported to the guest
>> to provision for ITTE state storage.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  include/linux/irqchip/arm-gic-v3.h | 1 +
>>  virt/kvm/arm/vgic/vgic-its.c       | 3 +++
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>> index 170e00a..8cfd81bc 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -233,6 +233,7 @@
>>  #define GITS_CTLR_QUIESCENT		(1U << 31)
>>  
>>  #define GITS_TYPER_PLPIS		(1UL << 0)
>> +#define GITS_TYPER_ITT_ENTRY_SIZE_SHIFT	4
>>  #define GITS_TYPER_IDBITS_SHIFT		8
>>  #define GITS_TYPER_DEVBITS_SHIFT	13
>>  #define GITS_TYPER_DEVBITS(r)		((((r) >> GITS_TYPER_DEVBITS_SHIFT) & 0x1f) + 1)
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index e174220..96378b8 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -33,6 +33,8 @@
>>  #include "vgic.h"
>>  #include "vgic-mmio.h"
>>  
>> +#define ITTE_SIZE 16
>> +
>>  /*
>>   * Creates a new (reference to a) struct vgic_irq for a given LPI.
>>   * If this LPI is already mapped on another ITS, we increase its refcount
>> @@ -399,6 +401,7 @@ static unsigned long vgic_mmio_read_its_typer(struct kvm *kvm,
>>  	 */
>>  	reg |= 0x0f << GITS_TYPER_DEVBITS_SHIFT;
>>  	reg |= 0x0f << GITS_TYPER_IDBITS_SHIFT;
>> +	reg |= ITTE_SIZE << GITS_TYPER_ITT_ENTRY_SIZE_SHIFT;
> 
> The field is defined as the "... number of bytes per translation table
> entry, minus one.". So it should be: (ITTE_SIZE - 1) << ...

You're perfectly right. I will fix that

Thanks

Eric
> 
> Cheers,
> Andre.
> 
>>  
>>  	return extract_bytes(reg, addr & 7, len);
>>  }
>>

Re: [RFC 07/13] KVM: arm64: ITS: Change entry_size and indirect bit in BASER

[Auger Eric]

Hi Marc,

On 12/01/2017 18:05, Marc Zyngier wrote:
> On 12/01/17 15:56, Eric Auger wrote:
>> Change the device table entry_size to 16 bytes instead of 8.
>> We also Store the device and collection device in the its
>> struct.
>>
>> The patch also clears the indirect bit for the device BASER.
>> The indirect bit is set as read-only.
> 
> Err... Why? We *really* want to continue supporting indirect tables, as
> this is a massive memory saver for the guest.
> 
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> TODO: investigate support of 2 level tables, ie. enabling
>> Indirect = 1. Support of 2 level tables is implementation
>> defined.
> 
> Clearly, that's a regression. What exactly is the issue that decided you
> to disable it?
Well no valuable reason besides I saw it was optional, lack of
time/knowledge and a bit of laziness. I will address this requirement in
my next respin.

For my curiosity why did we choose not allowing the feature for
collections. Is that just because we think their number if going
sufficiently small compared to devices?

Thanks

Eric
> 
> Thanks,
> 
> 	M.
> 

[RFC 07/13] KVM: arm64: ITS: Change entry_size and indirect bit in BASER

[Auger Eric]

Hi Marc,

On 12/01/2017 18:05, Marc Zyngier wrote:
> On 12/01/17 15:56, Eric Auger wrote:
>> Change the device table entry_size to 16 bytes instead of 8.
>> We also Store the device and collection device in the its
>> struct.
>>
>> The patch also clears the indirect bit for the device BASER.
>> The indirect bit is set as read-only.
> 
> Err... Why? We *really* want to continue supporting indirect tables, as
> this is a massive memory saver for the guest.
> 
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> TODO: investigate support of 2 level tables, ie. enabling
>> Indirect = 1. Support of 2 level tables is implementation
>> defined.
> 
> Clearly, that's a regression. What exactly is the issue that decided you
> to disable it?
Well no valuable reason besides I saw it was optional, lack of
time/knowledge and a bit of laziness. I will address this requirement in
my next respin.

For my curiosity why did we choose not allowing the feature for
collections. Is that just because we think their number if going
sufficiently small compared to devices?

Thanks

Eric
> 
> Thanks,
> 
> 	M.
> 

Re: [RFC 01/13] KVM: arm/arm64: Add vITS save/restore API documentation

[Auger Eric]

Hi Marc,

On 12/01/2017 17:52, Marc Zyngier wrote:
> Hi Eric,
> 
> On 12/01/17 15:56, Eric Auger wrote:
>> Add description for how to access vITS registers and how to flush/restore
>> vITS tables into/from memory
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 ++++++++++++++++++++++
>>  1 file changed, 70 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> index 6081a5b..bd74613 100644
>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> @@ -36,3 +36,73 @@ Groups:
>>      -ENXIO:  ITS not properly configured as required prior to setting
>>               this attribute
>>      -ENOMEM: Memory shortage when allocating ITS internal data
>> +
>> +  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
>> +  Attributes:
>> +      The attr field of kvm_device_attr encodes the offset of the
>> +      ITS register, relative to the ITS control frame base address
>> +      (ITS_base).
>> +
>> +      kvm_device_attr.addr points to a __u64 value whatever the width
>> +      of the addressed register (32/64 bits).
>> +
>> +      Writes to read-only registers are ignored by the kernel except
>> +      for a single register, GITS_READR. Normally this register is RO
>> +      but it needs to be restored otherwise commands in the queue will
>> +      be re-executed after CWRITER setting.
>> +
>> +      For other registers, Getting or setting a register has the same
>> +      effect as reading/writing the register on real hardware.
>> +  Errors:
>> +    -ENXIO: Offset does not correspond to any supported register
>> +    -EFAULT: Invalid user pointer for attr->addr
>> +    -EINVAL: Offset is not 64-bit aligned
>> +
>> +  KVM_DEV_ARM_VGIC_GRP_ITS_TABLES
>> +  Attributes
>> +       The attr field of kvm_device_attr is not used.
>> +
>> +       request the flush-save/restore of the ITS tables, namely
>> +       the device table, the collection table, all the ITT tables,
>> +       the LPI pending tables. On save, the tables are flushed
>> +       into guest memory at the location provisionned by the guest
> 
> 					    provisioned
> 
>> +       in GITS_BASER (device and collection tables), on MAPD command
>> +       (ITT_addr), GICR_PENDBASERs (pending tables).
>> +
>> +       This means the GIC should be restored before the ITS and all
>> +       ITS registers but the GITS_CTRL must be restored before
>> +       restoring the ITS tables.
>> +
>> +       Note the LPI configuration table is read-only for the
>> +       in-kernel ITS and its save/restore goes through the standard
>> +       RAM save/restore.
>> +
>> +       The layout of the tables in guest memory defines an ABI.
>> +       The entries are laid out in memory as follows;
>> +
>> +    Device Table Entry (DTE) layout: entry size = 16 bytes
>> +
>> +    bits:     | 63   ...  32  | 31 ... 6 | 5 | 4   ...   0 |
>> +    values:   |   DeviceID    |   Resv   | V |    Size     |
>> +
>> +    bits:     | 63 ... 44 | 43  ...  0  |
>> +    values:   |    Resv   |  ITT_addr   |
> 
> While I appreciate this layout represents the absolute maximum an ITS
> could implement, I'm a bit concerned about the amount of memory we may
> end-up requiring here. All the ITSs implementations I know of seem to
> get away with 8 bytes per entry. Maybe I'm just too worried.

OK so I would propose a 16b DeviceId and 16b eventid

bits:     | 63   ...  48  | 47 ... 4 | 3   ...   0 |
values:   |   DeviceID    | ITT_addr |    Size     |

I can use the size field as a validity indicator

> 
> Also, please mention that ITT_addr is actually ITT_addr[51:8], as we're
> guaranteed to have an ITT that is 256 byte aligned.
sure
> 
>> +
>> +    Collection Table Entry (CTE) layout: entry size = 8 bytes
>> +
>> +    bits:     | 63| 62 ..  52  | 51 ... 16 | 15  ...   0 |
>> +    values:   | V |    RES0    |  RDBase   |    ICID     |
>> +
>> +    Interrupt Translation Table Entry (ITTE) layout: entry size = 16 bytes
> 
> The actual name is Interrupt Translation Entry (ITE). I have a patch
> renaming this all over the vgic-its.c file...
ok
> 
>> +
>> +    bits:     | 63   ...  32  | 31 ... 17 | 16 | 15  ...  0 |
>> +    values:   |   DeviceID    |    RES0   | V  |    ICID    |
>> +
>> +    bits:     | 63 ...  32    | 31      ...        0 |
>> +    values:   |   pINTID      |        EventID       |
> 
> Same concern here. 32bit DevID, EventID and INTID seem a bit over the
> top. But maybe we shouldn't be concerned about memory... ;-)
So I would suggest encoding
16b DeviceId
16b eventid
16b collection ID
16b pINTID

bits:     | 63   ...  48  | 47 ... 32 | 31 ... 15 | 15  ...  0 |
values:   |   DeviceID    |   pINTID  |  EventId  |   ICID     |

a null pINTID would meen the ITE is invalid.

Does that make sense or should I instead reduce the number of bits
allocated to collections and keep the pINTID bit number larger?


> 
>> +
>> +    LPI Pending Table layout:
>> +
>> +    As specified in the ARM Generic Interrupt Controller Architecture
>> +    Specification GIC Architecture version 3.0 and version 4. The first
>> +    1kB contains only zeros.
>>
> 
> You definitely want to relax this. An ITS implementation is allowed (and
> actually encouraged) to maintain a coarse map in the first kB, and use
> this to quickly scan the table, which would be very useful on restore.
Maybe I miss something here. Currently I restore the ITEs before the
pending tables. So considering all the ITEs I know which LPI are defined
and which pending bits need to be restored. Why would I need to use a
coarse map for?

I understand the CPU cannot write the pending tables in our back, spec
says behavior would be unpredictable, right?

Thanks

Eric
> 
> Thanks,
> 
> 	M.
> 

[RFC 01/13] KVM: arm/arm64: Add vITS save/restore API documentation

[Auger Eric]

Hi Marc,

On 12/01/2017 17:52, Marc Zyngier wrote:
> Hi Eric,
> 
> On 12/01/17 15:56, Eric Auger wrote:
>> Add description for how to access vITS registers and how to flush/restore
>> vITS tables into/from memory
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 ++++++++++++++++++++++
>>  1 file changed, 70 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> index 6081a5b..bd74613 100644
>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> @@ -36,3 +36,73 @@ Groups:
>>      -ENXIO:  ITS not properly configured as required prior to setting
>>               this attribute
>>      -ENOMEM: Memory shortage when allocating ITS internal data
>> +
>> +  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
>> +  Attributes:
>> +      The attr field of kvm_device_attr encodes the offset of the
>> +      ITS register, relative to the ITS control frame base address
>> +      (ITS_base).
>> +
>> +      kvm_device_attr.addr points to a __u64 value whatever the width
>> +      of the addressed register (32/64 bits).
>> +
>> +      Writes to read-only registers are ignored by the kernel except
>> +      for a single register, GITS_READR. Normally this register is RO
>> +      but it needs to be restored otherwise commands in the queue will
>> +      be re-executed after CWRITER setting.
>> +
>> +      For other registers, Getting or setting a register has the same
>> +      effect as reading/writing the register on real hardware.
>> +  Errors:
>> +    -ENXIO: Offset does not correspond to any supported register
>> +    -EFAULT: Invalid user pointer for attr->addr
>> +    -EINVAL: Offset is not 64-bit aligned
>> +
>> +  KVM_DEV_ARM_VGIC_GRP_ITS_TABLES
>> +  Attributes
>> +       The attr field of kvm_device_attr is not used.
>> +
>> +       request the flush-save/restore of the ITS tables, namely
>> +       the device table, the collection table, all the ITT tables,
>> +       the LPI pending tables. On save, the tables are flushed
>> +       into guest memory at the location provisionned by the guest
> 
> 					    provisioned
> 
>> +       in GITS_BASER (device and collection tables), on MAPD command
>> +       (ITT_addr), GICR_PENDBASERs (pending tables).
>> +
>> +       This means the GIC should be restored before the ITS and all
>> +       ITS registers but the GITS_CTRL must be restored before
>> +       restoring the ITS tables.
>> +
>> +       Note the LPI configuration table is read-only for the
>> +       in-kernel ITS and its save/restore goes through the standard
>> +       RAM save/restore.
>> +
>> +       The layout of the tables in guest memory defines an ABI.
>> +       The entries are laid out in memory as follows;
>> +
>> +    Device Table Entry (DTE) layout: entry size = 16 bytes
>> +
>> +    bits:     | 63   ...  32  | 31 ... 6 | 5 | 4   ...   0 |
>> +    values:   |   DeviceID    |   Resv   | V |    Size     |
>> +
>> +    bits:     | 63 ... 44 | 43  ...  0  |
>> +    values:   |    Resv   |  ITT_addr   |
> 
> While I appreciate this layout represents the absolute maximum an ITS
> could implement, I'm a bit concerned about the amount of memory we may
> end-up requiring here. All the ITSs implementations I know of seem to
> get away with 8 bytes per entry. Maybe I'm just too worried.

OK so I would propose a 16b DeviceId and 16b eventid

bits:     | 63   ...  48  | 47 ... 4 | 3   ...   0 |
values:   |   DeviceID    | ITT_addr |    Size     |

I can use the size field as a validity indicator

> 
> Also, please mention that ITT_addr is actually ITT_addr[51:8], as we're
> guaranteed to have an ITT that is 256 byte aligned.
sure
> 
>> +
>> +    Collection Table Entry (CTE) layout: entry size = 8 bytes
>> +
>> +    bits:     | 63| 62 ..  52  | 51 ... 16 | 15  ...   0 |
>> +    values:   | V |    RES0    |  RDBase   |    ICID     |
>> +
>> +    Interrupt Translation Table Entry (ITTE) layout: entry size = 16 bytes
> 
> The actual name is Interrupt Translation Entry (ITE). I have a patch
> renaming this all over the vgic-its.c file...
ok
> 
>> +
>> +    bits:     | 63   ...  32  | 31 ... 17 | 16 | 15  ...  0 |
>> +    values:   |   DeviceID    |    RES0   | V  |    ICID    |
>> +
>> +    bits:     | 63 ...  32    | 31      ...        0 |
>> +    values:   |   pINTID      |        EventID       |
> 
> Same concern here. 32bit DevID, EventID and INTID seem a bit over the
> top. But maybe we shouldn't be concerned about memory... ;-)
So I would suggest encoding
16b DeviceId
16b eventid
16b collection ID
16b pINTID

bits:     | 63   ...  48  | 47 ... 32 | 31 ... 15 | 15  ...  0 |
values:   |   DeviceID    |   pINTID  |  EventId  |   ICID     |

a null pINTID would meen the ITE is invalid.

Does that make sense or should I instead reduce the number of bits
allocated to collections and keep the pINTID bit number larger?


> 
>> +
>> +    LPI Pending Table layout:
>> +
>> +    As specified in the ARM Generic Interrupt Controller Architecture
>> +    Specification GIC Architecture version 3.0 and version 4. The first
>> +    1kB contains only zeros.
>>
> 
> You definitely want to relax this. An ITS implementation is allowed (and
> actually encouraged) to maintain a coarse map in the first kB, and use
> this to quickly scan the table, which would be very useful on restore.
Maybe I miss something here. Currently I restore the ITEs before the
pending tables. So considering all the ITEs I know which LPI are defined
and which pending bits need to be restored. Why would I need to use a
coarse map for?

I understand the CPU cannot write the pending tables in our back, spec
says behavior would be unpredictable, right?

Thanks

Eric
> 
> Thanks,
> 
> 	M.
> 

Re: [RFC 07/13] KVM: arm64: ITS: Change entry_size and indirect bit in BASER

[Marc Zyngier]

On 13/01/17 08:57, Auger Eric wrote:
> Hi Marc,
> 
> On 12/01/2017 18:05, Marc Zyngier wrote:
>> On 12/01/17 15:56, Eric Auger wrote:
>>> Change the device table entry_size to 16 bytes instead of 8.
>>> We also Store the device and collection device in the its
>>> struct.
>>>
>>> The patch also clears the indirect bit for the device BASER.
>>> The indirect bit is set as read-only.
>>
>> Err... Why? We *really* want to continue supporting indirect tables, as
>> this is a massive memory saver for the guest.
>>
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>
>>> ---
>>>
>>> TODO: investigate support of 2 level tables, ie. enabling
>>> Indirect = 1. Support of 2 level tables is implementation
>>> defined.
>>
>> Clearly, that's a regression. What exactly is the issue that decided you
>> to disable it?
> Well no valuable reason besides I saw it was optional, lack of
> time/knowledge and a bit of laziness. I will address this requirement in
> my next respin.

Ah, I was worried about something much more fundamental! ;-)

> For my curiosity why did we choose not allowing the feature for
> collections. Is that just because we think their number if going
> sufficiently small compared to devices?

Collections are usually a much smaller number (directly related to the
number of CPUs in the system), and can be kept very compact. Devices, on
the other hand, can be extremely sparse (to the point where a guest can
fail to allocate enough memory to cover the required range).

TBH, we could allow it for collections as well. It is just not that useful.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[RFC 07/13] KVM: arm64: ITS: Change entry_size and indirect bit in BASER

[Marc Zyngier]

On 13/01/17 08:57, Auger Eric wrote:
> Hi Marc,
> 
> On 12/01/2017 18:05, Marc Zyngier wrote:
>> On 12/01/17 15:56, Eric Auger wrote:
>>> Change the device table entry_size to 16 bytes instead of 8.
>>> We also Store the device and collection device in the its
>>> struct.
>>>
>>> The patch also clears the indirect bit for the device BASER.
>>> The indirect bit is set as read-only.
>>
>> Err... Why? We *really* want to continue supporting indirect tables, as
>> this is a massive memory saver for the guest.
>>
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>
>>> ---
>>>
>>> TODO: investigate support of 2 level tables, ie. enabling
>>> Indirect = 1. Support of 2 level tables is implementation
>>> defined.
>>
>> Clearly, that's a regression. What exactly is the issue that decided you
>> to disable it?
> Well no valuable reason besides I saw it was optional, lack of
> time/knowledge and a bit of laziness. I will address this requirement in
> my next respin.

Ah, I was worried about something much more fundamental! ;-)

> For my curiosity why did we choose not allowing the feature for
> collections. Is that just because we think their number if going
> sufficiently small compared to devices?

Collections are usually a much smaller number (directly related to the
number of CPUs in the system), and can be kept very compact. Devices, on
the other hand, can be extremely sparse (to the point where a guest can
fail to allocate enough memory to cover the required range).

TBH, we could allow it for collections as well. It is just not that useful.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC 01/13] KVM: arm/arm64: Add vITS save/restore API documentation

[Marc Zyngier]

On 13/01/17 09:07, Auger Eric wrote:
> Hi Marc,
> 
> On 12/01/2017 17:52, Marc Zyngier wrote:
>> Hi Eric,
>>
>> On 12/01/17 15:56, Eric Auger wrote:
>>> Add description for how to access vITS registers and how to flush/restore
>>> vITS tables into/from memory
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 ++++++++++++++++++++++
>>>  1 file changed, 70 insertions(+)
>>>
>>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>> index 6081a5b..bd74613 100644
>>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>> @@ -36,3 +36,73 @@ Groups:
>>>      -ENXIO:  ITS not properly configured as required prior to setting
>>>               this attribute
>>>      -ENOMEM: Memory shortage when allocating ITS internal data
>>> +
>>> +  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
>>> +  Attributes:
>>> +      The attr field of kvm_device_attr encodes the offset of the
>>> +      ITS register, relative to the ITS control frame base address
>>> +      (ITS_base).
>>> +
>>> +      kvm_device_attr.addr points to a __u64 value whatever the width
>>> +      of the addressed register (32/64 bits).
>>> +
>>> +      Writes to read-only registers are ignored by the kernel except
>>> +      for a single register, GITS_READR. Normally this register is RO
>>> +      but it needs to be restored otherwise commands in the queue will
>>> +      be re-executed after CWRITER setting.
>>> +
>>> +      For other registers, Getting or setting a register has the same
>>> +      effect as reading/writing the register on real hardware.
>>> +  Errors:
>>> +    -ENXIO: Offset does not correspond to any supported register
>>> +    -EFAULT: Invalid user pointer for attr->addr
>>> +    -EINVAL: Offset is not 64-bit aligned
>>> +
>>> +  KVM_DEV_ARM_VGIC_GRP_ITS_TABLES
>>> +  Attributes
>>> +       The attr field of kvm_device_attr is not used.
>>> +
>>> +       request the flush-save/restore of the ITS tables, namely
>>> +       the device table, the collection table, all the ITT tables,
>>> +       the LPI pending tables. On save, the tables are flushed
>>> +       into guest memory at the location provisionned by the guest
>>
>> 					    provisioned
>>
>>> +       in GITS_BASER (device and collection tables), on MAPD command
>>> +       (ITT_addr), GICR_PENDBASERs (pending tables).
>>> +
>>> +       This means the GIC should be restored before the ITS and all
>>> +       ITS registers but the GITS_CTRL must be restored before
>>> +       restoring the ITS tables.
>>> +
>>> +       Note the LPI configuration table is read-only for the
>>> +       in-kernel ITS and its save/restore goes through the standard
>>> +       RAM save/restore.
>>> +
>>> +       The layout of the tables in guest memory defines an ABI.
>>> +       The entries are laid out in memory as follows;
>>> +
>>> +    Device Table Entry (DTE) layout: entry size = 16 bytes
>>> +
>>> +    bits:     | 63   ...  32  | 31 ... 6 | 5 | 4   ...   0 |
>>> +    values:   |   DeviceID    |   Resv   | V |    Size     |
>>> +
>>> +    bits:     | 63 ... 44 | 43  ...  0  |
>>> +    values:   |    Resv   |  ITT_addr   |
>>
>> While I appreciate this layout represents the absolute maximum an ITS
>> could implement, I'm a bit concerned about the amount of memory we may
>> end-up requiring here. All the ITSs implementations I know of seem to
>> get away with 8 bytes per entry. Maybe I'm just too worried.
> 
> OK so I would propose a 16b DeviceId and 16b eventid
> 
> bits:     | 63   ...  48  | 47 ... 4 | 3   ...   0 |
> values:   |   DeviceID    | ITT_addr |    Size     |
> 
> I can use the size field as a validity indicator

Note that you are allowed to use a 0 size field. It means 1 bit of
EventID (2 possible interrupts). So maybe using a particular address as
a valid flag?

> 
>>
>> Also, please mention that ITT_addr is actually ITT_addr[51:8], as we're
>> guaranteed to have an ITT that is 256 byte aligned.
> sure
>>
>>> +
>>> +    Collection Table Entry (CTE) layout: entry size = 8 bytes
>>> +
>>> +    bits:     | 63| 62 ..  52  | 51 ... 16 | 15  ...   0 |
>>> +    values:   | V |    RES0    |  RDBase   |    ICID     |
>>> +
>>> +    Interrupt Translation Table Entry (ITTE) layout: entry size = 16 bytes
>>
>> The actual name is Interrupt Translation Entry (ITE). I have a patch
>> renaming this all over the vgic-its.c file...
> ok
>>
>>> +
>>> +    bits:     | 63   ...  32  | 31 ... 17 | 16 | 15  ...  0 |
>>> +    values:   |   DeviceID    |    RES0   | V  |    ICID    |
>>> +
>>> +    bits:     | 63 ...  32    | 31      ...        0 |
>>> +    values:   |   pINTID      |        EventID       |
>>
>> Same concern here. 32bit DevID, EventID and INTID seem a bit over the
>> top. But maybe we shouldn't be concerned about memory... ;-)
> So I would suggest encoding
> 16b DeviceId
> 16b eventid
> 16b collection ID
> 16b pINTID
> 
> bits:     | 63   ...  48  | 47 ... 32 | 31 ... 15 | 15  ...  0 |
> values:   |   DeviceID    |   pINTID  |  EventId  |   ICID     |
> 
> a null pINTID would meen the ITE is invalid.
> 
> Does that make sense or should I instead reduce the number of bits
> allocated to collections and keep the pINTID bit number larger?

16bit worth of collections is quite a lot (64k CPUs?). I'd be perfectly
fine with a smaller number, but let's see what people think.

> 
> 
>>
>>> +
>>> +    LPI Pending Table layout:
>>> +
>>> +    As specified in the ARM Generic Interrupt Controller Architecture
>>> +    Specification GIC Architecture version 3.0 and version 4. The first
>>> +    1kB contains only zeros.
>>>
>>
>> You definitely want to relax this. An ITS implementation is allowed (and
>> actually encouraged) to maintain a coarse map in the first kB, and use
>> this to quickly scan the table, which would be very useful on restore.
> Maybe I miss something here. Currently I restore the ITEs before the
> pending tables. So considering all the ITEs I know which LPI are defined
> and which pending bits need to be restored. Why would I need to use a
> coarse map for?

You could, instead of testing all the bits for which you can generate an
LPI, look at the coarse map, which usually uses one bit to represent
something like 64 bits of pending table, and find out what is currently
pending. That's what HW does, but maybe there is no need to do this for
the SW implementation, specially if we have very few LPIs.

> I understand the CPU cannot write the pending tables in our back, spec
> says behavior would be unpredictable, right?

Absolutely. Only the ITS can touch that memory.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[RFC 01/13] KVM: arm/arm64: Add vITS save/restore API documentation

[Marc Zyngier]

On 13/01/17 09:07, Auger Eric wrote:
> Hi Marc,
> 
> On 12/01/2017 17:52, Marc Zyngier wrote:
>> Hi Eric,
>>
>> On 12/01/17 15:56, Eric Auger wrote:
>>> Add description for how to access vITS registers and how to flush/restore
>>> vITS tables into/from memory
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> ---
>>>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 ++++++++++++++++++++++
>>>  1 file changed, 70 insertions(+)
>>>
>>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>> index 6081a5b..bd74613 100644
>>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>> @@ -36,3 +36,73 @@ Groups:
>>>      -ENXIO:  ITS not properly configured as required prior to setting
>>>               this attribute
>>>      -ENOMEM: Memory shortage when allocating ITS internal data
>>> +
>>> +  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
>>> +  Attributes:
>>> +      The attr field of kvm_device_attr encodes the offset of the
>>> +      ITS register, relative to the ITS control frame base address
>>> +      (ITS_base).
>>> +
>>> +      kvm_device_attr.addr points to a __u64 value whatever the width
>>> +      of the addressed register (32/64 bits).
>>> +
>>> +      Writes to read-only registers are ignored by the kernel except
>>> +      for a single register, GITS_READR. Normally this register is RO
>>> +      but it needs to be restored otherwise commands in the queue will
>>> +      be re-executed after CWRITER setting.
>>> +
>>> +      For other registers, Getting or setting a register has the same
>>> +      effect as reading/writing the register on real hardware.
>>> +  Errors:
>>> +    -ENXIO: Offset does not correspond to any supported register
>>> +    -EFAULT: Invalid user pointer for attr->addr
>>> +    -EINVAL: Offset is not 64-bit aligned
>>> +
>>> +  KVM_DEV_ARM_VGIC_GRP_ITS_TABLES
>>> +  Attributes
>>> +       The attr field of kvm_device_attr is not used.
>>> +
>>> +       request the flush-save/restore of the ITS tables, namely
>>> +       the device table, the collection table, all the ITT tables,
>>> +       the LPI pending tables. On save, the tables are flushed
>>> +       into guest memory at the location provisionned by the guest
>>
>> 					    provisioned
>>
>>> +       in GITS_BASER (device and collection tables), on MAPD command
>>> +       (ITT_addr), GICR_PENDBASERs (pending tables).
>>> +
>>> +       This means the GIC should be restored before the ITS and all
>>> +       ITS registers but the GITS_CTRL must be restored before
>>> +       restoring the ITS tables.
>>> +
>>> +       Note the LPI configuration table is read-only for the
>>> +       in-kernel ITS and its save/restore goes through the standard
>>> +       RAM save/restore.
>>> +
>>> +       The layout of the tables in guest memory defines an ABI.
>>> +       The entries are laid out in memory as follows;
>>> +
>>> +    Device Table Entry (DTE) layout: entry size = 16 bytes
>>> +
>>> +    bits:     | 63   ...  32  | 31 ... 6 | 5 | 4   ...   0 |
>>> +    values:   |   DeviceID    |   Resv   | V |    Size     |
>>> +
>>> +    bits:     | 63 ... 44 | 43  ...  0  |
>>> +    values:   |    Resv   |  ITT_addr   |
>>
>> While I appreciate this layout represents the absolute maximum an ITS
>> could implement, I'm a bit concerned about the amount of memory we may
>> end-up requiring here. All the ITSs implementations I know of seem to
>> get away with 8 bytes per entry. Maybe I'm just too worried.
> 
> OK so I would propose a 16b DeviceId and 16b eventid
> 
> bits:     | 63   ...  48  | 47 ... 4 | 3   ...   0 |
> values:   |   DeviceID    | ITT_addr |    Size     |
> 
> I can use the size field as a validity indicator

Note that you are allowed to use a 0 size field. It means 1 bit of
EventID (2 possible interrupts). So maybe using a particular address as
a valid flag?

> 
>>
>> Also, please mention that ITT_addr is actually ITT_addr[51:8], as we're
>> guaranteed to have an ITT that is 256 byte aligned.
> sure
>>
>>> +
>>> +    Collection Table Entry (CTE) layout: entry size = 8 bytes
>>> +
>>> +    bits:     | 63| 62 ..  52  | 51 ... 16 | 15  ...   0 |
>>> +    values:   | V |    RES0    |  RDBase   |    ICID     |
>>> +
>>> +    Interrupt Translation Table Entry (ITTE) layout: entry size = 16 bytes
>>
>> The actual name is Interrupt Translation Entry (ITE). I have a patch
>> renaming this all over the vgic-its.c file...
> ok
>>
>>> +
>>> +    bits:     | 63   ...  32  | 31 ... 17 | 16 | 15  ...  0 |
>>> +    values:   |   DeviceID    |    RES0   | V  |    ICID    |
>>> +
>>> +    bits:     | 63 ...  32    | 31      ...        0 |
>>> +    values:   |   pINTID      |        EventID       |
>>
>> Same concern here. 32bit DevID, EventID and INTID seem a bit over the
>> top. But maybe we shouldn't be concerned about memory... ;-)
> So I would suggest encoding
> 16b DeviceId
> 16b eventid
> 16b collection ID
> 16b pINTID
> 
> bits:     | 63   ...  48  | 47 ... 32 | 31 ... 15 | 15  ...  0 |
> values:   |   DeviceID    |   pINTID  |  EventId  |   ICID     |
> 
> a null pINTID would meen the ITE is invalid.
> 
> Does that make sense or should I instead reduce the number of bits
> allocated to collections and keep the pINTID bit number larger?

16bit worth of collections is quite a lot (64k CPUs?). I'd be perfectly
fine with a smaller number, but let's see what people think.

> 
> 
>>
>>> +
>>> +    LPI Pending Table layout:
>>> +
>>> +    As specified in the ARM Generic Interrupt Controller Architecture
>>> +    Specification GIC Architecture version 3.0 and version 4. The first
>>> +    1kB contains only zeros.
>>>
>>
>> You definitely want to relax this. An ITS implementation is allowed (and
>> actually encouraged) to maintain a coarse map in the first kB, and use
>> this to quickly scan the table, which would be very useful on restore.
> Maybe I miss something here. Currently I restore the ITEs before the
> pending tables. So considering all the ITEs I know which LPI are defined
> and which pending bits need to be restored. Why would I need to use a
> coarse map for?

You could, instead of testing all the bits for which you can generate an
LPI, look at the coarse map, which usually uses one bit to represent
something like 64 bits of pending table, and find out what is currently
pending. That's what HW does, but maybe there is no need to do this for
the SW implementation, specially if we have very few LPIs.

> I understand the CPU cannot write the pending tables in our back, spec
> says behavior would be unpredictable, right?

Absolutely. Only the ITS can touch that memory.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC 01/13] KVM: arm/arm64: Add vITS save/restore API documentation

[Auger Eric]

Hi Marc,

On 13/01/2017 10:46, Marc Zyngier wrote:
> On 13/01/17 09:07, Auger Eric wrote:
>> Hi Marc,
>>
>> On 12/01/2017 17:52, Marc Zyngier wrote:
>>> Hi Eric,
>>>
>>> On 12/01/17 15:56, Eric Auger wrote:
>>>> Add description for how to access vITS registers and how to flush/restore
>>>> vITS tables into/from memory
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 ++++++++++++++++++++++
>>>>  1 file changed, 70 insertions(+)
>>>>
>>>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>> index 6081a5b..bd74613 100644
>>>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>> @@ -36,3 +36,73 @@ Groups:
>>>>      -ENXIO:  ITS not properly configured as required prior to setting
>>>>               this attribute
>>>>      -ENOMEM: Memory shortage when allocating ITS internal data
>>>> +
>>>> +  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
>>>> +  Attributes:
>>>> +      The attr field of kvm_device_attr encodes the offset of the
>>>> +      ITS register, relative to the ITS control frame base address
>>>> +      (ITS_base).
>>>> +
>>>> +      kvm_device_attr.addr points to a __u64 value whatever the width
>>>> +      of the addressed register (32/64 bits).
>>>> +
>>>> +      Writes to read-only registers are ignored by the kernel except
>>>> +      for a single register, GITS_READR. Normally this register is RO
>>>> +      but it needs to be restored otherwise commands in the queue will
>>>> +      be re-executed after CWRITER setting.
>>>> +
>>>> +      For other registers, Getting or setting a register has the same
>>>> +      effect as reading/writing the register on real hardware.
>>>> +  Errors:
>>>> +    -ENXIO: Offset does not correspond to any supported register
>>>> +    -EFAULT: Invalid user pointer for attr->addr
>>>> +    -EINVAL: Offset is not 64-bit aligned
>>>> +
>>>> +  KVM_DEV_ARM_VGIC_GRP_ITS_TABLES
>>>> +  Attributes
>>>> +       The attr field of kvm_device_attr is not used.
>>>> +
>>>> +       request the flush-save/restore of the ITS tables, namely
>>>> +       the device table, the collection table, all the ITT tables,
>>>> +       the LPI pending tables. On save, the tables are flushed
>>>> +       into guest memory at the location provisionned by the guest
>>>
>>> 					    provisioned
>>>
>>>> +       in GITS_BASER (device and collection tables), on MAPD command
>>>> +       (ITT_addr), GICR_PENDBASERs (pending tables).
>>>> +
>>>> +       This means the GIC should be restored before the ITS and all
>>>> +       ITS registers but the GITS_CTRL must be restored before
>>>> +       restoring the ITS tables.
>>>> +
>>>> +       Note the LPI configuration table is read-only for the
>>>> +       in-kernel ITS and its save/restore goes through the standard
>>>> +       RAM save/restore.
>>>> +
>>>> +       The layout of the tables in guest memory defines an ABI.
>>>> +       The entries are laid out in memory as follows;
>>>> +
>>>> +    Device Table Entry (DTE) layout: entry size = 16 bytes
>>>> +
>>>> +    bits:     | 63   ...  32  | 31 ... 6 | 5 | 4   ...   0 |
>>>> +    values:   |   DeviceID    |   Resv   | V |    Size     |
>>>> +
>>>> +    bits:     | 63 ... 44 | 43  ...  0  |
>>>> +    values:   |    Resv   |  ITT_addr   |
>>>
>>> While I appreciate this layout represents the absolute maximum an ITS
>>> could implement, I'm a bit concerned about the amount of memory we may
>>> end-up requiring here. All the ITSs implementations I know of seem to
>>> get away with 8 bytes per entry. Maybe I'm just too worried.
>>
>> OK so I would propose a 16b DeviceId and 16b eventid
>>
>> bits:     | 63   ...  48  | 47 ... 4 | 3   ...   0 |
>> values:   |   DeviceID    | ITT_addr |    Size     |
>>
>> I can use the size field as a validity indicator
> 
> Note that you are allowed to use a 0 size field. It means 1 bit of
> EventID (2 possible interrupts). So maybe using a particular address as
> a valid flag?
Is it really acceptable to encode the deviceId and eventid on 16 bits
instead of 32 bits max each?

Currently I do not use the deviceId indexing, ie. the device id is
directly encoded in the entry. The spec rather suggests device id
indexing in flat table and this is also stems from 2 stage table support.

So I have 2 strategies:
- ignore the device id indexing and store valid data at the beginning of
available buffers (pros: no sparsity, cons: shrinks device and eventid
to 16 bits). Natural in flat mode, less natural in 2 stage mode.
- implement device id indexing (pros: keep the full range for deviceid
and eventid, cons: sparsity). Then sparsity needs to be handled somehow.
Now I better understand your remark on first kB of the pending table...


> 
>>
>>>
>>> Also, please mention that ITT_addr is actually ITT_addr[51:8], as we're
>>> guaranteed to have an ITT that is 256 byte aligned.
>> sure
>>>
>>>> +
>>>> +    Collection Table Entry (CTE) layout: entry size = 8 bytes
>>>> +
>>>> +    bits:     | 63| 62 ..  52  | 51 ... 16 | 15  ...   0 |
>>>> +    values:   | V |    RES0    |  RDBase   |    ICID     |
>>>> +
>>>> +    Interrupt Translation Table Entry (ITTE) layout: entry size = 16 bytes
>>>
>>> The actual name is Interrupt Translation Entry (ITE). I have a patch
>>> renaming this all over the vgic-its.c file...
>> ok
>>>
>>>> +
>>>> +    bits:     | 63   ...  32  | 31 ... 17 | 16 | 15  ...  0 |
>>>> +    values:   |   DeviceID    |    RES0   | V  |    ICID    |
>>>> +
>>>> +    bits:     | 63 ...  32    | 31      ...        0 |
>>>> +    values:   |   pINTID      |        EventID       |
>>>
>>> Same concern here. 32bit DevID, EventID and INTID seem a bit over the
>>> top. But maybe we shouldn't be concerned about memory... ;-)
>> So I would suggest encoding
>> 16b DeviceId
>> 16b eventid
>> 16b collection ID
>> 16b pINTID
>>
>> bits:     | 63   ...  48  | 47 ... 32 | 31 ... 15 | 15  ...  0 |
>> values:   |   DeviceID    |   pINTID  |  EventId  |   ICID     |
>>
>> a null pINTID would meen the ITE is invalid.
>>
>> Does that make sense or should I instead reduce the number of bits
>> allocated to collections and keep the pINTID bit number larger?
> 
> 16bit worth of collections is quite a lot (64k CPUs?). I'd be perfectly
> fine with a smaller number, but let's see what people think.
This is useless to store the deviceId here since the deviceId is known
from the upper level device table. I will fix that in v2. But anyway if
I encode the ITE on 8 bytes I must shrink the pINTID/EventId compared to
their max size (32b). If EventId is encoded on 16b then I guess the
pINTID should be encoded on the same number of bits. ICID on 10 bits?

Thoughts?

Thanks

Eric
> 
>>
>>
>>>
>>>> +
>>>> +    LPI Pending Table layout:
>>>> +
>>>> +    As specified in the ARM Generic Interrupt Controller Architecture
>>>> +    Specification GIC Architecture version 3.0 and version 4. The first
>>>> +    1kB contains only zeros.
>>>>
>>>
>>> You definitely want to relax this. An ITS implementation is allowed (and
>>> actually encouraged) to maintain a coarse map in the first kB, and use
>>> this to quickly scan the table, which would be very useful on restore.
>> Maybe I miss something here. Currently I restore the ITEs before the
>> pending tables. So considering all the ITEs I know which LPI are defined
>> and which pending bits need to be restored. Why would I need to use a
>> coarse map for?
> 
> You could, instead of testing all the bits for which you can generate an
> LPI, look at the coarse map, which usually uses one bit to represent
> something like 64 bits of pending table, and find out what is currently
> pending. That's what HW does, but maybe there is no need to do this for
> the SW implementation, specially if we have very few LPIs.
> 
>> I understand the CPU cannot write the pending tables in our back, spec
>> says behavior would be unpredictable, right?
> 
> Absolutely. Only the ITS can touch that memory.
> 
> Thanks,
> 
> 	M.
> 

[RFC 01/13] KVM: arm/arm64: Add vITS save/restore API documentation

[Auger Eric]

Hi Marc,

On 13/01/2017 10:46, Marc Zyngier wrote:
> On 13/01/17 09:07, Auger Eric wrote:
>> Hi Marc,
>>
>> On 12/01/2017 17:52, Marc Zyngier wrote:
>>> Hi Eric,
>>>
>>> On 12/01/17 15:56, Eric Auger wrote:
>>>> Add description for how to access vITS registers and how to flush/restore
>>>> vITS tables into/from memory
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 ++++++++++++++++++++++
>>>>  1 file changed, 70 insertions(+)
>>>>
>>>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>> index 6081a5b..bd74613 100644
>>>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>> @@ -36,3 +36,73 @@ Groups:
>>>>      -ENXIO:  ITS not properly configured as required prior to setting
>>>>               this attribute
>>>>      -ENOMEM: Memory shortage when allocating ITS internal data
>>>> +
>>>> +  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
>>>> +  Attributes:
>>>> +      The attr field of kvm_device_attr encodes the offset of the
>>>> +      ITS register, relative to the ITS control frame base address
>>>> +      (ITS_base).
>>>> +
>>>> +      kvm_device_attr.addr points to a __u64 value whatever the width
>>>> +      of the addressed register (32/64 bits).
>>>> +
>>>> +      Writes to read-only registers are ignored by the kernel except
>>>> +      for a single register, GITS_READR. Normally this register is RO
>>>> +      but it needs to be restored otherwise commands in the queue will
>>>> +      be re-executed after CWRITER setting.
>>>> +
>>>> +      For other registers, Getting or setting a register has the same
>>>> +      effect as reading/writing the register on real hardware.
>>>> +  Errors:
>>>> +    -ENXIO: Offset does not correspond to any supported register
>>>> +    -EFAULT: Invalid user pointer for attr->addr
>>>> +    -EINVAL: Offset is not 64-bit aligned
>>>> +
>>>> +  KVM_DEV_ARM_VGIC_GRP_ITS_TABLES
>>>> +  Attributes
>>>> +       The attr field of kvm_device_attr is not used.
>>>> +
>>>> +       request the flush-save/restore of the ITS tables, namely
>>>> +       the device table, the collection table, all the ITT tables,
>>>> +       the LPI pending tables. On save, the tables are flushed
>>>> +       into guest memory at the location provisionned by the guest
>>>
>>> 					    provisioned
>>>
>>>> +       in GITS_BASER (device and collection tables), on MAPD command
>>>> +       (ITT_addr), GICR_PENDBASERs (pending tables).
>>>> +
>>>> +       This means the GIC should be restored before the ITS and all
>>>> +       ITS registers but the GITS_CTRL must be restored before
>>>> +       restoring the ITS tables.
>>>> +
>>>> +       Note the LPI configuration table is read-only for the
>>>> +       in-kernel ITS and its save/restore goes through the standard
>>>> +       RAM save/restore.
>>>> +
>>>> +       The layout of the tables in guest memory defines an ABI.
>>>> +       The entries are laid out in memory as follows;
>>>> +
>>>> +    Device Table Entry (DTE) layout: entry size = 16 bytes
>>>> +
>>>> +    bits:     | 63   ...  32  | 31 ... 6 | 5 | 4   ...   0 |
>>>> +    values:   |   DeviceID    |   Resv   | V |    Size     |
>>>> +
>>>> +    bits:     | 63 ... 44 | 43  ...  0  |
>>>> +    values:   |    Resv   |  ITT_addr   |
>>>
>>> While I appreciate this layout represents the absolute maximum an ITS
>>> could implement, I'm a bit concerned about the amount of memory we may
>>> end-up requiring here. All the ITSs implementations I know of seem to
>>> get away with 8 bytes per entry. Maybe I'm just too worried.
>>
>> OK so I would propose a 16b DeviceId and 16b eventid
>>
>> bits:     | 63   ...  48  | 47 ... 4 | 3   ...   0 |
>> values:   |   DeviceID    | ITT_addr |    Size     |
>>
>> I can use the size field as a validity indicator
> 
> Note that you are allowed to use a 0 size field. It means 1 bit of
> EventID (2 possible interrupts). So maybe using a particular address as
> a valid flag?
Is it really acceptable to encode the deviceId and eventid on 16 bits
instead of 32 bits max each?

Currently I do not use the deviceId indexing, ie. the device id is
directly encoded in the entry. The spec rather suggests device id
indexing in flat table and this is also stems from 2 stage table support.

So I have 2 strategies:
- ignore the device id indexing and store valid data at the beginning of
available buffers (pros: no sparsity, cons: shrinks device and eventid
to 16 bits). Natural in flat mode, less natural in 2 stage mode.
- implement device id indexing (pros: keep the full range for deviceid
and eventid, cons: sparsity). Then sparsity needs to be handled somehow.
Now I better understand your remark on first kB of the pending table...


> 
>>
>>>
>>> Also, please mention that ITT_addr is actually ITT_addr[51:8], as we're
>>> guaranteed to have an ITT that is 256 byte aligned.
>> sure
>>>
>>>> +
>>>> +    Collection Table Entry (CTE) layout: entry size = 8 bytes
>>>> +
>>>> +    bits:     | 63| 62 ..  52  | 51 ... 16 | 15  ...   0 |
>>>> +    values:   | V |    RES0    |  RDBase   |    ICID     |
>>>> +
>>>> +    Interrupt Translation Table Entry (ITTE) layout: entry size = 16 bytes
>>>
>>> The actual name is Interrupt Translation Entry (ITE). I have a patch
>>> renaming this all over the vgic-its.c file...
>> ok
>>>
>>>> +
>>>> +    bits:     | 63   ...  32  | 31 ... 17 | 16 | 15  ...  0 |
>>>> +    values:   |   DeviceID    |    RES0   | V  |    ICID    |
>>>> +
>>>> +    bits:     | 63 ...  32    | 31      ...        0 |
>>>> +    values:   |   pINTID      |        EventID       |
>>>
>>> Same concern here. 32bit DevID, EventID and INTID seem a bit over the
>>> top. But maybe we shouldn't be concerned about memory... ;-)
>> So I would suggest encoding
>> 16b DeviceId
>> 16b eventid
>> 16b collection ID
>> 16b pINTID
>>
>> bits:     | 63   ...  48  | 47 ... 32 | 31 ... 15 | 15  ...  0 |
>> values:   |   DeviceID    |   pINTID  |  EventId  |   ICID     |
>>
>> a null pINTID would meen the ITE is invalid.
>>
>> Does that make sense or should I instead reduce the number of bits
>> allocated to collections and keep the pINTID bit number larger?
> 
> 16bit worth of collections is quite a lot (64k CPUs?). I'd be perfectly
> fine with a smaller number, but let's see what people think.
This is useless to store the deviceId here since the deviceId is known
from the upper level device table. I will fix that in v2. But anyway if
I encode the ITE on 8 bytes I must shrink the pINTID/EventId compared to
their max size (32b). If EventId is encoded on 16b then I guess the
pINTID should be encoded on the same number of bits. ICID on 10 bits?

Thoughts?

Thanks

Eric
> 
>>
>>
>>>
>>>> +
>>>> +    LPI Pending Table layout:
>>>> +
>>>> +    As specified in the ARM Generic Interrupt Controller Architecture
>>>> +    Specification GIC Architecture version 3.0 and version 4. The first
>>>> +    1kB contains only zeros.
>>>>
>>>
>>> You definitely want to relax this. An ITS implementation is allowed (and
>>> actually encouraged) to maintain a coarse map in the first kB, and use
>>> this to quickly scan the table, which would be very useful on restore.
>> Maybe I miss something here. Currently I restore the ITEs before the
>> pending tables. So considering all the ITEs I know which LPI are defined
>> and which pending bits need to be restored. Why would I need to use a
>> coarse map for?
> 
> You could, instead of testing all the bits for which you can generate an
> LPI, look at the coarse map, which usually uses one bit to represent
> something like 64 bits of pending table, and find out what is currently
> pending. That's what HW does, but maybe there is no need to do this for
> the SW implementation, specially if we have very few LPIs.
> 
>> I understand the CPU cannot write the pending tables in our back, spec
>> says behavior would be unpredictable, right?
> 
> Absolutely. Only the ITS can touch that memory.
> 
> Thanks,
> 
> 	M.
> 

Re: [RFC 01/13] KVM: arm/arm64: Add vITS save/restore API documentation

[Peter Maydell]

On 12 January 2017 at 15:56, Eric Auger <eric.auger@redhat.com> wrote:
> Add description for how to access vITS registers and how to flush/restore
> vITS tables into/from memory
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 ++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> index 6081a5b..bd74613 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> @@ -36,3 +36,73 @@ Groups:
>      -ENXIO:  ITS not properly configured as required prior to setting
>               this attribute
>      -ENOMEM: Memory shortage when allocating ITS internal data
> +
> +  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
> +  Attributes:
> +      The attr field of kvm_device_attr encodes the offset of the
> +      ITS register, relative to the ITS control frame base address
> +      (ITS_base).
> +
> +      kvm_device_attr.addr points to a __u64 value whatever the width
> +      of the addressed register (32/64 bits).
> +
> +      Writes to read-only registers are ignored by the kernel except
> +      for a single register, GITS_READR. Normally this register is RO
> +      but it needs to be restored otherwise commands in the queue will
> +      be re-executed after CWRITER setting.

Dumb question -- is it possible/sensible to process the command
queue rather than migrating a list of outstanding commands?

thanks
-- PMM

[RFC 01/13] KVM: arm/arm64: Add vITS save/restore API documentation

[Peter Maydell]

On 12 January 2017 at 15:56, Eric Auger <eric.auger@redhat.com> wrote:
> Add description for how to access vITS registers and how to flush/restore
> vITS tables into/from memory
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 ++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> index 6081a5b..bd74613 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> @@ -36,3 +36,73 @@ Groups:
>      -ENXIO:  ITS not properly configured as required prior to setting
>               this attribute
>      -ENOMEM: Memory shortage when allocating ITS internal data
> +
> +  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
> +  Attributes:
> +      The attr field of kvm_device_attr encodes the offset of the
> +      ITS register, relative to the ITS control frame base address
> +      (ITS_base).
> +
> +      kvm_device_attr.addr points to a __u64 value whatever the width
> +      of the addressed register (32/64 bits).
> +
> +      Writes to read-only registers are ignored by the kernel except
> +      for a single register, GITS_READR. Normally this register is RO
> +      but it needs to be restored otherwise commands in the queue will
> +      be re-executed after CWRITER setting.

Dumb question -- is it possible/sensible to process the command
queue rather than migrating a list of outstanding commands?

thanks
-- PMM

Re: [RFC 01/13] KVM: arm/arm64: Add vITS save/restore API documentation

[Marc Zyngier]

On 03/02/17 14:00, Peter Maydell wrote:
> On 12 January 2017 at 15:56, Eric Auger <eric.auger@redhat.com> wrote:
>> Add description for how to access vITS registers and how to flush/restore
>> vITS tables into/from memory
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 ++++++++++++++++++++++
>>  1 file changed, 70 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> index 6081a5b..bd74613 100644
>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> @@ -36,3 +36,73 @@ Groups:
>>      -ENXIO:  ITS not properly configured as required prior to setting
>>               this attribute
>>      -ENOMEM: Memory shortage when allocating ITS internal data
>> +
>> +  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
>> +  Attributes:
>> +      The attr field of kvm_device_attr encodes the offset of the
>> +      ITS register, relative to the ITS control frame base address
>> +      (ITS_base).
>> +
>> +      kvm_device_attr.addr points to a __u64 value whatever the width
>> +      of the addressed register (32/64 bits).
>> +
>> +      Writes to read-only registers are ignored by the kernel except
>> +      for a single register, GITS_READR. Normally this register is RO
>> +      but it needs to be restored otherwise commands in the queue will
>> +      be re-executed after CWRITER setting.
> 
> Dumb question -- is it possible/sensible to process the command
> queue rather than migrating a list of outstanding commands?

It is not always possible, specially if the ITS implements the "stall"
mechanism, where it waits for SW acknowledgement in case of a command error.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[RFC 01/13] KVM: arm/arm64: Add vITS save/restore API documentation

[Marc Zyngier]

On 03/02/17 14:00, Peter Maydell wrote:
> On 12 January 2017 at 15:56, Eric Auger <eric.auger@redhat.com> wrote:
>> Add description for how to access vITS registers and how to flush/restore
>> vITS tables into/from memory
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 ++++++++++++++++++++++
>>  1 file changed, 70 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> index 6081a5b..bd74613 100644
>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> @@ -36,3 +36,73 @@ Groups:
>>      -ENXIO:  ITS not properly configured as required prior to setting
>>               this attribute
>>      -ENOMEM: Memory shortage when allocating ITS internal data
>> +
>> +  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
>> +  Attributes:
>> +      The attr field of kvm_device_attr encodes the offset of the
>> +      ITS register, relative to the ITS control frame base address
>> +      (ITS_base).
>> +
>> +      kvm_device_attr.addr points to a __u64 value whatever the width
>> +      of the addressed register (32/64 bits).
>> +
>> +      Writes to read-only registers are ignored by the kernel except
>> +      for a single register, GITS_READR. Normally this register is RO
>> +      but it needs to be restored otherwise commands in the queue will
>> +      be re-executed after CWRITER setting.
> 
> Dumb question -- is it possible/sensible to process the command
> queue rather than migrating a list of outstanding commands?

It is not always possible, specially if the ITS implements the "stall"
mechanism, where it waits for SW acknowledgement in case of a command error.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...