[RFC v2 00/19] vITS save/restore

[RFC v2 00/19] 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 latter
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 configation table and the command queues do not need extra
syncs.

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 series applies on top of Vijaya's series:
- [PATCH v11 0/8] arm/arm64: vgic: Implement API for vGICv3 live
  migration
  https://www.spinics.net/lists/arm-kernel/msg558046.html

Best Regards

Eric

Git: complete series available at
https://github.com/eauger/linux/tree/kvmarm-queue-vgic-mig-v11-its-rfcv2

applied on kvmarm queue.

* Testing:
- on Cavium ThunderX using a virtio-net-pci guest (x3 vNIC),
  virsh save/restore commands and virt-manager live migration.
  Tested with 1 and 2 stage device table.

History:

v1 -> v2:
- rebased on Vijaya's v11
- all entries now are 8 byte large
- devid/eventid indexing for device table and ITT
- support 2 stage device table
- common infra to read indexed tables
- add cpu <-> le64 conversions
- itte renamed into ite
- do not care anymore about pending table 1st KB
  (not needed at the moment for coarse mapping)

RFC v1
- creation


Eric Auger (19):
  KVM: arm/arm64: Add vITS save/restore API documentation
  KVM: arm/arm64: rename itte into ite
  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: Report the ITE size in GITS_TYPER
  KVM: arm64: ITS: Interpret MAPD Size field and check related errors
  KVM: arm64: ITS: Interpret MAPD ITT_addr field
  KVM: arm64: ITS: Check the device id matches TYPER DEVBITS range
  KVM: arm64: ITS: KVM_DEV_ARM_VGIC_GRP_ITS_TABLES group
  KVM: arm64: ITS: vgic_its_alloc_ite/device
  KVM: arm64: ITS: Sort the device and ITE lists
  KVM: arm64: ITS: Add infrastructure for table lookup
  KVM: arm64: ITS: Collection table save/restore
  KVM: arm64: ITS: vgic_its_check_id returns the entry's GPA
  KVM: arm64: ITS: ITT flush and restore
  KVM: arm64: ITS: Device table save/restore
  KVM: arm64: ITS: Pending table save/restore

 Documentation/virtual/kvm/devices/arm-vgic-its.txt |   78 ++
 arch/arm/include/uapi/asm/kvm.h                    |    2 +
 arch/arm64/include/uapi/asm/kvm.h                  |    2 +
 include/linux/irqchip/arm-gic-v3.h                 |    3 +
 virt/kvm/arm/vgic/vgic-its.c                       | 1031 ++++++++++++++++++--
 virt/kvm/arm/vgic/vgic-mmio.c                      |    3 +-
 virt/kvm/arm/vgic/vgic-mmio.h                      |   14 +-
 7 files changed, 1022 insertions(+), 111 deletions(-)

-- 
2.5.5

[RFC v2 00/19] 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 latter
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 configation table and the command queues do not need extra
syncs.

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 series applies on top of Vijaya's series:
- [PATCH v11 0/8] arm/arm64: vgic: Implement API for vGICv3 live
  migration
  https://www.spinics.net/lists/arm-kernel/msg558046.html

Best Regards

Eric

Git: complete series available at
https://github.com/eauger/linux/tree/kvmarm-queue-vgic-mig-v11-its-rfcv2

applied on kvmarm queue.

* Testing:
- on Cavium ThunderX using a virtio-net-pci guest (x3 vNIC),
  virsh save/restore commands and virt-manager live migration.
  Tested with 1 and 2 stage device table.

History:

v1 -> v2:
- rebased on Vijaya's v11
- all entries now are 8 byte large
- devid/eventid indexing for device table and ITT
- support 2 stage device table
- common infra to read indexed tables
- add cpu <-> le64 conversions
- itte renamed into ite
- do not care anymore about pending table 1st KB
  (not needed at the moment for coarse mapping)

RFC v1
- creation


Eric Auger (19):
  KVM: arm/arm64: Add vITS save/restore API documentation
  KVM: arm/arm64: rename itte into ite
  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: Report the ITE size in GITS_TYPER
  KVM: arm64: ITS: Interpret MAPD Size field and check related errors
  KVM: arm64: ITS: Interpret MAPD ITT_addr field
  KVM: arm64: ITS: Check the device id matches TYPER DEVBITS range
  KVM: arm64: ITS: KVM_DEV_ARM_VGIC_GRP_ITS_TABLES group
  KVM: arm64: ITS: vgic_its_alloc_ite/device
  KVM: arm64: ITS: Sort the device and ITE lists
  KVM: arm64: ITS: Add infrastructure for table lookup
  KVM: arm64: ITS: Collection table save/restore
  KVM: arm64: ITS: vgic_its_check_id returns the entry's GPA
  KVM: arm64: ITS: ITT flush and restore
  KVM: arm64: ITS: Device table save/restore
  KVM: arm64: ITS: Pending table save/restore

 Documentation/virtual/kvm/devices/arm-vgic-its.txt |   78 ++
 arch/arm/include/uapi/asm/kvm.h                    |    2 +
 arch/arm64/include/uapi/asm/kvm.h                  |    2 +
 include/linux/irqchip/arm-gic-v3.h                 |    3 +
 virt/kvm/arm/vgic/vgic-its.c                       | 1031 ++++++++++++++++++--
 virt/kvm/arm/vgic/vgic-mmio.c                      |    3 +-
 virt/kvm/arm/vgic/vgic-mmio.h                      |   14 +-
 7 files changed, 1022 insertions(+), 111 deletions(-)

-- 
2.5.5

[RFC v2 01/19] 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>

---

v1 -> v2:
- DTE and ITE now are 8 bytes
- DTE and ITE now indexed by deviceid/eventid
- use ITE name instead of ITTE
- mentions ITT_addr matches bits [51:8] of the actual address
- mentions LE layout
---
 Documentation/virtual/kvm/devices/arm-vgic-its.txt | 78 ++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
index 6081a5b..49ade0c 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
@@ -36,3 +36,81 @@ 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 provisioned 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 in little endian format as follows;
+
+    The device table and ITE are respectively indexed by device id and
+    eventid. The collection table however is not indexed by collection id:
+    CTE are written at the beginning of the buffer.
+
+    Device Table Entry (DTE) layout: entry size = 8 bytes
+
+    bits:     | 63 ... 45 | 44 ... 5 | 4 ... 0 |
+    values:   |   next    | ITT_addr |  Size   |
+
+    where
+    - ITT_addr matches bits [48:8] of the ITT address (256B aligned).
+    - next field is meaningful only if the entry is valid (ITT_addr != NULL).
+    It equals to 0 if this entry is the last one; otherwise it corresponds
+    to the minimum between the offset to the next device id and 2^19 -1.
+
+    Collection Table Entry (CTE) layout: entry size = 8 bytes
+
+    bits:     | 63| 62 ..  52  | 51 ... 16 | 15  ...   0 |
+    values:   | V |    RES0    |  RDBase   |    ICID     |
+
+    Interrupt Translation Entry (ITE) layout: entry size = 8 bytes
+
+    bits:     | 63 ... 48 | 47 ... 16 | 15 ... 0 |
+    values:   |    next   |   pINTID  |  ICID    |
+
+    - next field is meaningful only if the entry is valid (pINTID != NULL).
+    It equals to 0 if this entry is the last one; otherwise it corresponds
+    to the minimum between the eventid offset to the next ITE and 2^16 -1.
+
+    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 is not modified and therefore should contain zeroes.
-- 
2.5.5

[RFC v2 02/19] KVM: arm/arm64: rename itte into ite

[Eric Auger]

The actual abbreviation for the interrupt translation table entry
is ITE. Let's rename all itte instances by ite.

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

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 571b64a..a7b5a50 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -114,8 +114,8 @@ struct its_collection {
 #define its_is_collection_mapped(coll) ((coll) && \
 				((coll)->target_addr != COLLECTION_NOT_MAPPED))
 
-struct its_itte {
-	struct list_head itte_list;
+struct its_ite {
+	struct list_head ite_list;
 
 	struct vgic_irq *irq;
 	struct its_collection *collection;
@@ -143,27 +143,27 @@ static struct its_device *find_its_device(struct vgic_its *its, u32 device_id)
  * Device ID/Event ID pair on an ITS.
  * Must be called with the its_lock mutex held.
  */
-static struct its_itte *find_itte(struct vgic_its *its, u32 device_id,
+static struct its_ite *find_ite(struct vgic_its *its, u32 device_id,
 				  u32 event_id)
 {
 	struct its_device *device;
-	struct its_itte *itte;
+	struct its_ite *ite;
 
 	device = find_its_device(its, device_id);
 	if (device == NULL)
 		return NULL;
 
-	list_for_each_entry(itte, &device->itt_head, itte_list)
-		if (itte->event_id == event_id)
-			return itte;
+	list_for_each_entry(ite, &device->itt_head, ite_list)
+		if (ite->event_id == event_id)
+			return ite;
 
 	return NULL;
 }
 
 /* To be used as an iterator this macro misses the enclosing parentheses */
-#define for_each_lpi_its(dev, itte, its) \
+#define for_each_lpi_its(dev, ite, its) \
 	list_for_each_entry(dev, &(its)->device_list, dev_list) \
-		list_for_each_entry(itte, &(dev)->itt_head, itte_list)
+		list_for_each_entry(ite, &(dev)->itt_head, ite_list)
 
 /*
  * We only implement 48 bits of PA at the moment, although the ITS
@@ -270,18 +270,18 @@ static int vgic_copy_lpi_list(struct kvm *kvm, u32 **intid_ptr)
  * Needs to be called whenever either the collection for a LPIs has
  * changed or the collection itself got retargeted.
  */
-static void update_affinity_itte(struct kvm *kvm, struct its_itte *itte)
+static void update_affinity_ite(struct kvm *kvm, struct its_ite *ite)
 {
 	struct kvm_vcpu *vcpu;
 
-	if (!its_is_collection_mapped(itte->collection))
+	if (!its_is_collection_mapped(ite->collection))
 		return;
 
-	vcpu = kvm_get_vcpu(kvm, itte->collection->target_addr);
+	vcpu = kvm_get_vcpu(kvm, ite->collection->target_addr);
 
-	spin_lock(&itte->irq->irq_lock);
-	itte->irq->target_vcpu = vcpu;
-	spin_unlock(&itte->irq->irq_lock);
+	spin_lock(&ite->irq->irq_lock);
+	ite->irq->target_vcpu = vcpu;
+	spin_unlock(&ite->irq->irq_lock);
 }
 
 /*
@@ -292,13 +292,13 @@ static void update_affinity_collection(struct kvm *kvm, struct vgic_its *its,
 				       struct its_collection *coll)
 {
 	struct its_device *device;
-	struct its_itte *itte;
+	struct its_ite *ite;
 
-	for_each_lpi_its(device, itte, its) {
-		if (!itte->collection || coll != itte->collection)
+	for_each_lpi_its(device, ite, its) {
+		if (!ite->collection || coll != ite->collection)
 			continue;
 
-		update_affinity_itte(kvm, itte);
+		update_affinity_ite(kvm, ite);
 	}
 }
 
@@ -448,25 +448,25 @@ static int vgic_its_trigger_msi(struct kvm *kvm, struct vgic_its *its,
 				u32 devid, u32 eventid)
 {
 	struct kvm_vcpu *vcpu;
-	struct its_itte *itte;
+	struct its_ite *ite;
 
 	if (!its->enabled)
 		return -EBUSY;
 
-	itte = find_itte(its, devid, eventid);
-	if (!itte || !its_is_collection_mapped(itte->collection))
+	ite = find_ite(its, devid, eventid);
+	if (!ite || !its_is_collection_mapped(ite->collection))
 		return E_ITS_INT_UNMAPPED_INTERRUPT;
 
-	vcpu = kvm_get_vcpu(kvm, itte->collection->target_addr);
+	vcpu = kvm_get_vcpu(kvm, ite->collection->target_addr);
 	if (!vcpu)
 		return E_ITS_INT_UNMAPPED_INTERRUPT;
 
 	if (!vcpu->arch.vgic_cpu.lpis_enabled)
 		return -EBUSY;
 
-	spin_lock(&itte->irq->irq_lock);
-	itte->irq->pending_latch = true;
-	vgic_queue_irq_unlock(kvm, itte->irq);
+	spin_lock(&ite->irq->irq_lock);
+	ite->irq->pending_latch = true;
+	vgic_queue_irq_unlock(kvm, ite->irq);
 
 	return 0;
 }
@@ -534,15 +534,15 @@ int vgic_its_inject_msi(struct kvm *kvm, struct kvm_msi *msi)
 }
 
 /* Requires the its_lock to be held. */
-static void its_free_itte(struct kvm *kvm, struct its_itte *itte)
+static void its_free_ite(struct kvm *kvm, struct its_ite *ite)
 {
-	list_del(&itte->itte_list);
+	list_del(&ite->ite_list);
 
 	/* This put matches the get in vgic_add_lpi. */
-	if (itte->irq)
-		vgic_put_irq(kvm, itte->irq);
+	if (ite->irq)
+		vgic_put_irq(kvm, ite->irq);
 
-	kfree(itte);
+	kfree(ite);
 }
 
 static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
@@ -567,17 +567,17 @@ static int vgic_its_cmd_handle_discard(struct kvm *kvm, struct vgic_its *its,
 {
 	u32 device_id = its_cmd_get_deviceid(its_cmd);
 	u32 event_id = its_cmd_get_id(its_cmd);
-	struct its_itte *itte;
+	struct its_ite *ite;
 
 
-	itte = find_itte(its, device_id, event_id);
-	if (itte && itte->collection) {
+	ite = find_ite(its, device_id, event_id);
+	if (ite && ite->collection) {
 		/*
 		 * Though the spec talks about removing the pending state, we
 		 * don't bother here since we clear the ITTE anyway and the
 		 * pending state is a property of the ITTE struct.
 		 */
-		its_free_itte(kvm, itte);
+		its_free_ite(kvm, ite);
 		return 0;
 	}
 
@@ -595,26 +595,26 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
 	u32 event_id = its_cmd_get_id(its_cmd);
 	u32 coll_id = its_cmd_get_collection(its_cmd);
 	struct kvm_vcpu *vcpu;
-	struct its_itte *itte;
+	struct its_ite *ite;
 	struct its_collection *collection;
 
-	itte = find_itte(its, device_id, event_id);
-	if (!itte)
+	ite = find_ite(its, device_id, event_id);
+	if (!ite)
 		return E_ITS_MOVI_UNMAPPED_INTERRUPT;
 
-	if (!its_is_collection_mapped(itte->collection))
+	if (!its_is_collection_mapped(ite->collection))
 		return E_ITS_MOVI_UNMAPPED_COLLECTION;
 
 	collection = find_collection(its, coll_id);
 	if (!its_is_collection_mapped(collection))
 		return E_ITS_MOVI_UNMAPPED_COLLECTION;
 
-	itte->collection = collection;
+	ite->collection = collection;
 	vcpu = kvm_get_vcpu(kvm, collection->target_addr);
 
-	spin_lock(&itte->irq->irq_lock);
-	itte->irq->target_vcpu = vcpu;
-	spin_unlock(&itte->irq->irq_lock);
+	spin_lock(&ite->irq->irq_lock);
+	ite->irq->target_vcpu = vcpu;
+	spin_unlock(&ite->irq->irq_lock);
 
 	return 0;
 }
@@ -702,7 +702,7 @@ static void vgic_its_free_collection(struct vgic_its *its, u32 coll_id)
 {
 	struct its_collection *collection;
 	struct its_device *device;
-	struct its_itte *itte;
+	struct its_ite *ite;
 
 	/*
 	 * Clearing the mapping for that collection ID removes the
@@ -713,10 +713,10 @@ static void vgic_its_free_collection(struct vgic_its *its, u32 coll_id)
 	if (!collection)
 		return;
 
-	for_each_lpi_its(device, itte, its)
-		if (itte->collection &&
-		    itte->collection->collection_id == coll_id)
-			itte->collection = NULL;
+	for_each_lpi_its(device, ite, its)
+		if (ite->collection &&
+		    ite->collection->collection_id == coll_id)
+			ite->collection = NULL;
 
 	list_del(&collection->coll_list);
 	kfree(collection);
@@ -732,7 +732,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	u32 device_id = its_cmd_get_deviceid(its_cmd);
 	u32 event_id = its_cmd_get_id(its_cmd);
 	u32 coll_id = its_cmd_get_collection(its_cmd);
-	struct its_itte *itte;
+	struct its_ite *ite;
 	struct its_device *device;
 	struct its_collection *collection, *new_coll = NULL;
 	int lpi_nr;
@@ -751,7 +751,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 		return E_ITS_MAPTI_PHYSICALID_OOR;
 
 	/* If there is an existing mapping, behavior is UNPREDICTABLE. */
-	if (find_itte(its, device_id, event_id))
+	if (find_ite(its, device_id, event_id))
 		return 0;
 
 	collection = find_collection(its, coll_id);
@@ -762,36 +762,36 @@ 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) {
+	ite = kzalloc(sizeof(struct its_ite), GFP_KERNEL);
+	if (!ite) {
 		if (new_coll)
 			vgic_its_free_collection(its, coll_id);
 		return -ENOMEM;
 	}
 
-	itte->event_id	= event_id;
-	list_add_tail(&itte->itte_list, &device->itt_head);
+	ite->event_id	= event_id;
+	list_add_tail(&ite->ite_list, &device->itt_head);
 
-	itte->collection = collection;
-	itte->lpi = lpi_nr;
+	ite->collection = collection;
+	ite->lpi = lpi_nr;
 
 	irq = vgic_add_lpi(kvm, lpi_nr);
 	if (IS_ERR(irq)) {
 		if (new_coll)
 			vgic_its_free_collection(its, coll_id);
-		its_free_itte(kvm, itte);
+		its_free_ite(kvm, ite);
 		return PTR_ERR(irq);
 	}
-	itte->irq = irq;
+	ite->irq = irq;
 
-	update_affinity_itte(kvm, itte);
+	update_affinity_ite(kvm, ite);
 
 	/*
 	 * We "cache" the configuration table entries in out struct vgic_irq's.
 	 * However we only have those structs for mapped IRQs, so we read in
 	 * the respective config data from memory here upon mapping the LPI.
 	 */
-	update_lpi_config(kvm, itte->irq, NULL);
+	update_lpi_config(kvm, ite->irq, NULL);
 
 	return 0;
 }
@@ -799,15 +799,15 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 /* Requires the its_lock to be held. */
 static void vgic_its_unmap_device(struct kvm *kvm, struct its_device *device)
 {
-	struct its_itte *itte, *temp;
+	struct its_ite *ite, *temp;
 
 	/*
 	 * The spec says that unmapping a device with still valid
 	 * ITTEs associated is UNPREDICTABLE. We remove all ITTEs,
 	 * since we cannot leave the memory unreferenced.
 	 */
-	list_for_each_entry_safe(itte, temp, &device->itt_head, itte_list)
-		its_free_itte(kvm, itte);
+	list_for_each_entry_safe(ite, temp, &device->itt_head, ite_list)
+		its_free_ite(kvm, ite);
 
 	list_del(&device->dev_list);
 	kfree(device);
@@ -906,14 +906,14 @@ static int vgic_its_cmd_handle_clear(struct kvm *kvm, struct vgic_its *its,
 {
 	u32 device_id = its_cmd_get_deviceid(its_cmd);
 	u32 event_id = its_cmd_get_id(its_cmd);
-	struct its_itte *itte;
+	struct its_ite *ite;
 
 
-	itte = find_itte(its, device_id, event_id);
-	if (!itte)
+	ite = find_ite(its, device_id, event_id);
+	if (!ite)
 		return E_ITS_CLEAR_UNMAPPED_INTERRUPT;
 
-	itte->irq->pending_latch = false;
+	ite->irq->pending_latch = false;
 
 	return 0;
 }
@@ -927,14 +927,14 @@ static int vgic_its_cmd_handle_inv(struct kvm *kvm, struct vgic_its *its,
 {
 	u32 device_id = its_cmd_get_deviceid(its_cmd);
 	u32 event_id = its_cmd_get_id(its_cmd);
-	struct its_itte *itte;
+	struct its_ite *ite;
 
 
-	itte = find_itte(its, device_id, event_id);
-	if (!itte)
+	ite = find_ite(its, device_id, event_id);
+	if (!ite)
 		return E_ITS_INV_UNMAPPED_INTERRUPT;
 
-	return update_lpi_config(kvm, itte->irq, NULL);
+	return update_lpi_config(kvm, ite->irq, NULL);
 }
 
 /*
@@ -1414,7 +1414,7 @@ static void vgic_its_destroy(struct kvm_device *kvm_dev)
 	struct kvm *kvm = kvm_dev->kvm;
 	struct vgic_its *its = kvm_dev->private;
 	struct its_device *dev;
-	struct its_itte *itte;
+	struct its_ite *ite;
 	struct list_head *dev_cur, *dev_temp;
 	struct list_head *cur, *temp;
 
@@ -1429,8 +1429,8 @@ static void vgic_its_destroy(struct kvm_device *kvm_dev)
 	list_for_each_safe(dev_cur, dev_temp, &its->device_list) {
 		dev = container_of(dev_cur, struct its_device, dev_list);
 		list_for_each_safe(cur, temp, &dev->itt_head) {
-			itte = (container_of(cur, struct its_itte, itte_list));
-			its_free_itte(kvm, itte);
+			ite = (container_of(cur, struct its_ite, ite_list));
+			its_free_ite(kvm, ite);
 		}
 		list_del(dev_cur);
 		kfree(dev);
-- 
2.5.5

[RFC v2 03/19] 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 3654b4c..0427ddb 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -430,8 +430,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 98bb566..055ad42 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -192,4 +192,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 v2 04/19] 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 6ebd3e6..4beb83b 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_GRP_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 c286035..7e8dd69 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_GRP_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 a7b5a50..43bb17e 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 v2 05/19] 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 43bb17e..e9c8f9f 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 055ad42..ad8a585 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 v2 06/19] 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 e9c8f9f..6120c6e 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 v2 07/19] KVM: arm64: ITS: Report the ITE size in GITS_TYPER

[Eric Auger]

An ITE size of 8 Bytes is reported to the guest. Combining this
information with the number of event IDs the guest wants to support,
this latter will be able to allocate each device's ITT with the
right size.

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

---

v1 -> v2:
- correct ITT_ENTRY_SIZE field
- remove ITE_SIZE since all entries become 8 bytes
---
 include/linux/irqchip/arm-gic-v3.h | 1 +
 virt/kvm/arm/vgic/vgic-its.c       | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

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 6120c6e..6d84508 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -176,6 +176,8 @@ static struct its_ite *find_ite(struct vgic_its *its, u32 device_id,
 
 #define GIC_LPI_OFFSET 8192
 
+#define VITS_ESZ 8
+
 /*
  * Finds and returns a collection in the ITS collection table.
  * Must be called with the its_lock mutex held.
@@ -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 |= (VITS_ESZ - 1) << GITS_TYPER_ITT_ENTRY_SIZE_SHIFT;
 
 	return extract_bytes(reg, addr & 7, len);
 }
@@ -1387,7 +1390,7 @@ 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)			| \
+	 ((u64)(VITS_ESZ - 1) << GITS_BASER_ENTRY_SIZE_SHIFT)		| \
 	 GITS_BASER_PAGE_SIZE_64K)
 
 #define INITIAL_PROPBASER_VALUE						  \
-- 
2.5.5

[RFC v2 08/19] KVM: arm64: ITS: Interpret MAPD Size field and check related errors

[Eric Auger]

Up to now the MAPD's ITT size field has been ignored. It encodes
the number of eventid bit minus 1. It should be used to check
the eventid when a MAPTI command is issued on a device. Let's
store the nb_eventid_bits in the its_device and do the check
on MAPTI. Also make sure the ITT size field does not exceed the
GITS_TYPER IDBITS field.

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

---
---
 include/linux/irqchip/arm-gic-v3.h |  2 ++
 virt/kvm/arm/vgic/vgic-its.c       | 14 +++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 8cfd81bc..11bbbf3 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -341,9 +341,11 @@
 #define E_ITS_INT_UNMAPPED_INTERRUPT		0x010307
 #define E_ITS_CLEAR_UNMAPPED_INTERRUPT		0x010507
 #define E_ITS_MAPD_DEVICE_OOR			0x010801
+#define E_ITS_MAPD_ITTSIZE_OOR			0x010802
 #define E_ITS_MAPC_PROCNUM_OOR			0x010902
 #define E_ITS_MAPC_COLLECTION_OOR		0x010903
 #define E_ITS_MAPTI_UNMAPPED_DEVICE		0x010a04
+#define E_ITS_MAPTI_ID_OOR			0x010a05
 #define E_ITS_MAPTI_PHYSICALID_OOR		0x010a06
 #define E_ITS_INV_UNMAPPED_INTERRUPT		0x010c07
 #define E_ITS_INVALL_UNMAPPED_COLLECTION	0x010d09
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 6d84508..56bcd92 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -99,6 +99,7 @@ struct its_device {
 
 	/* the head for the list of ITTEs */
 	struct list_head itt_head;
+	u32 nb_eventid_bits;
 	u32 device_id;
 };
 
@@ -177,6 +178,7 @@ static struct its_ite *find_ite(struct vgic_its *its, u32 device_id,
 #define GIC_LPI_OFFSET 8192
 
 #define VITS_ESZ 8
+#define VITS_TYPER_IDBITS 0xF
 
 /*
  * Finds and returns a collection in the ITS collection table.
@@ -400,7 +402,7 @@ static unsigned long vgic_mmio_read_its_typer(struct kvm *kvm,
 	 * DevBits low - as least for the time being.
 	 */
 	reg |= 0x0f << GITS_TYPER_DEVBITS_SHIFT;
-	reg |= 0x0f << GITS_TYPER_IDBITS_SHIFT;
+	reg |= VITS_TYPER_IDBITS << GITS_TYPER_IDBITS_SHIFT;
 	reg |= (VITS_ESZ - 1) << GITS_TYPER_ITT_ENTRY_SIZE_SHIFT;
 
 	return extract_bytes(reg, addr & 7, len);
@@ -560,6 +562,7 @@ 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_size(cmd)		its_cmd_mask_field(cmd, 1, 0,  5)
 
 /*
  * The DISCARD command frees an Interrupt Translation Table Entry (ITTE).
@@ -745,6 +748,9 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	if (!device)
 		return E_ITS_MAPTI_UNMAPPED_DEVICE;
 
+	if (event_id >= (2 << device->nb_eventid_bits))
+		return E_ITS_MAPTI_ID_OOR;
+
 	if (its_cmd_get_command(its_cmd) == GITS_CMD_MAPTI)
 		lpi_nr = its_cmd_get_physical_id(its_cmd);
 	else
@@ -825,11 +831,15 @@ 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);
 	struct its_device *device;
 
 	if (!vgic_its_check_id(its, its->baser_device_table, device_id))
 		return E_ITS_MAPD_DEVICE_OOR;
 
+	if (valid && size > VITS_TYPER_IDBITS)
+		return E_ITS_MAPD_ITTSIZE_OOR;
+
 	device = find_its_device(its, device_id);
 
 	/*
@@ -852,6 +862,8 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
 		return -ENOMEM;
 
 	device->device_id = device_id;
+	device->nb_eventid_bits = size + 1;
+
 	INIT_LIST_HEAD(&device->itt_head);
 
 	list_add_tail(&device->dev_list, &its->device_list);
-- 
2.5.5

[RFC v2 09/19] KVM: arm64: ITS: Interpret MAPD ITT_addr field

[Eric Auger]

Up to now the MAPD ITT_addr had been ignored. We will need it
for save/restore. Let's record it in the its_device struct.

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

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 56bcd92..694023f 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -99,6 +99,7 @@ struct its_device {
 
 	/* the head for the list of ITTEs */
 	struct list_head itt_head;
+	gpa_t itt_addr;
 	u32 nb_eventid_bits;
 	u32 device_id;
 };
@@ -562,6 +563,7 @@ 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)
 
 /*
@@ -832,6 +834,7 @@ 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))
@@ -862,6 +865,7 @@ 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;
 
 	INIT_LIST_HEAD(&device->itt_head);
-- 
2.5.5

[RFC v2 10/19] KVM: arm64: ITS: Check the device id matches TYPER DEVBITS range

[Eric Auger]

On MAPD we currently check the device id can be stored in the device table.
Let's first check it can be encoded within the range defined by TYPER
DEVBITS.

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

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 694023f..322e370 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -180,6 +180,7 @@ static struct its_ite *find_ite(struct vgic_its *its, u32 device_id,
 
 #define VITS_ESZ 8
 #define VITS_TYPER_IDBITS 0xF
+#define VITS_TYPER_DEVBITS 0xF
 
 /*
  * Finds and returns a collection in the ITS collection table.
@@ -402,7 +403,7 @@ static unsigned long vgic_mmio_read_its_typer(struct kvm *kvm,
 	 * To avoid memory waste in the guest, we keep the number of IDBits and
 	 * DevBits low - as least for the time being.
 	 */
-	reg |= 0x0f << GITS_TYPER_DEVBITS_SHIFT;
+	reg |= VITS_TYPER_DEVBITS << GITS_TYPER_DEVBITS_SHIFT;
 	reg |= VITS_TYPER_IDBITS << GITS_TYPER_IDBITS_SHIFT;
 	reg |= (VITS_ESZ - 1) << GITS_TYPER_ITT_ENTRY_SIZE_SHIFT;
 
@@ -631,7 +632,7 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
  * Check whether an ID can be stored into the corresponding guest table.
  * For a direct table this is pretty easy, but gets a bit nasty for
  * indirect tables. We check whether the resulting guest physical address
- * is actually valid (covered by a memslot and guest accessbible).
+ * is actually valid (covered by a memslot and guest accessible).
  * For this we have to read the respective first level entry.
  */
 static bool vgic_its_check_id(struct vgic_its *its, u64 baser, int id)
@@ -642,6 +643,9 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, int id)
 	gfn_t gfn;
 	int esz = GITS_BASER_ENTRY_SIZE(baser);
 
+	if (id >= (2 << (VITS_TYPER_DEVBITS + 1)))
+		return false;
+
 	if (!(baser & GITS_BASER_INDIRECT)) {
 		phys_addr_t addr;
 
-- 
2.5.5

[RFC v2 11/19] 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>

---

v1 -> v2:
- remove useless kvm parameter

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      | 115 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 116 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index 4beb83b..7b165e9 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_GRP_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 7e8dd69..166df68 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_GRP_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 322e370..dd7545a 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1551,6 +1551,112 @@ 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 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 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 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 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 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 vgic_its *its)
+{
+	return -ENXIO;
+}
+
+/**
+ * vgic_its_table_flush - Flush all the tables into guest RAM
+ */
+static int vgic_its_table_flush(struct vgic_its *its)
+{
+	int ret;
+
+	mutex_lock(&its->its_lock);
+
+	ret = vgic_its_flush_pending_tables(its);
+	if (ret)
+		goto out;
+	ret = vgic_its_flush_device_tables(its);
+	if (ret)
+		goto out;
+
+	ret = vgic_its_flush_collection_table(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 vgic_its *its)
+{
+	int ret;
+
+	mutex_lock(&its->its_lock);
+	ret = vgic_its_restore_collection_table(its);
+	if (ret)
+		goto out;
+
+	ret = vgic_its_restore_device_tables(its);
+	if (ret)
+		goto out;
+
+	ret = vgic_its_restore_pending_tables(its);
+out:
+	mutex_unlock(&its->its_lock);
+
+	/**
+	 *  In real use case we observe MSI injection before
+	 *  the first CPU run. This is likely to stall virtio-net-pci
+	 *  for instance
+	 */
+	ret = kvm_vgic_map_resources(its->dev->kvm);
+	return ret;
+}
+
 static int vgic_its_has_attr(struct kvm_device *dev,
 			     struct kvm_device_attr *attr)
 {
@@ -1569,6 +1675,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;
 }
@@ -1617,6 +1725,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(its);
 	}
 	return -ENXIO;
 }
@@ -1624,9 +1734,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;
@@ -1647,6 +1758,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(its);
 	}
 	default:
 		return -ENXIO;
-- 
2.5.5

[RFC v2 12/19] KVM: arm64: ITS: vgic_its_alloc_ite/device

[Eric Auger]

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

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

---

v1 -> v2:
- report itt_size fix and remove ITE_SIZE
- s/itte/ite/g
---
 virt/kvm/arm/vgic/vgic-its.c | 70 +++++++++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 23 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index dd7545a..9792110 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -734,6 +734,26 @@ static void vgic_its_free_collection(struct vgic_its *its, u32 coll_id)
 	kfree(collection);
 }
 
+static int vgic_its_alloc_ite(struct its_device *device,
+			       struct its_ite **itep,
+			       struct its_collection *collection,
+			       u32 lpi_id, u32 event_id)
+{
+	struct its_ite *ite;
+
+	ite = kzalloc(sizeof(ite), GFP_KERNEL);
+	if (!ite)
+		return -ENOMEM;
+
+	ite->event_id	= event_id;
+	ite->collection = collection;
+	ite->lpi = lpi_id;
+
+	list_add_tail(&ite->ite_list, &device->itt_head);
+	*itep = ite;
+	return 0;
+}
+
 /*
  * The MAPTI and MAPI commands map LPIs to ITTEs.
  * Must be called with its_lock mutex held.
@@ -747,7 +767,7 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	struct its_ite *ite;
 	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);
@@ -777,19 +797,13 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 		new_coll = collection;
 	}
 
-	ite = kzalloc(sizeof(struct its_ite), GFP_KERNEL);
-	if (!ite) {
+	ret = vgic_its_alloc_ite(device, &ite, collection, lpi_nr, event_id);
+	if (ret) {
 		if (new_coll)
 			vgic_its_free_collection(its, coll_id);
-		return -ENOMEM;
+		return ret;
 	}
 
-	ite->event_id	= event_id;
-	list_add_tail(&ite->ite_list, &device->itt_head);
-
-	ite->collection = collection;
-	ite->lpi = lpi_nr;
-
 	irq = vgic_add_lpi(kvm, lpi_nr);
 	if (IS_ERR(irq)) {
 		if (new_coll)
@@ -828,6 +842,28 @@ 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;
+	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.
@@ -864,19 +900,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;
-
-	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 v2 13/19] KVM: arm64: ITS: Sort the device and ITE lists

[Eric Auger]

Natively sort the device and ITE lists in ascending
deviceId/eventid order. This paves the way to optimized
DTE and ITE scan in guest RAM table where entries are chained
together using a next ID offset.

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

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 9792110..1cd6ae6 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -734,6 +734,21 @@ static void vgic_its_free_collection(struct vgic_its *its, u32 coll_id)
 	kfree(collection);
 }
 
+static void ite_list_insert_sorted(struct list_head *h, struct its_ite *ite)
+{
+	struct list_head *pos = h->next;
+	u32 id = ite->event_id;
+
+	while (pos != h) {
+		struct its_ite *iter =
+			list_entry(pos, struct its_ite, ite_list);
+		if (id < iter->event_id)
+			break;
+		pos = pos->next;
+	}
+	list_add_tail(&ite->ite_list, pos);
+}
+
 static int vgic_its_alloc_ite(struct its_device *device,
 			       struct its_ite **itep,
 			       struct its_collection *collection,
@@ -749,7 +764,7 @@ static int vgic_its_alloc_ite(struct its_device *device,
 	ite->collection = collection;
 	ite->lpi = lpi_id;
 
-	list_add_tail(&ite->ite_list, &device->itt_head);
+	ite_list_insert_sorted(&device->itt_head, ite);
 	*itep = ite;
 	return 0;
 }
@@ -842,6 +857,22 @@ static void vgic_its_unmap_device(struct kvm *kvm, struct its_device *device)
 	kfree(device);
 }
 
+static void device_list_insert_sorted(struct list_head *h,
+				      struct its_device *dev)
+{
+	struct list_head *pos = h->next;
+	u32 id = dev->device_id;
+
+	while (pos != h) {
+		struct its_device *iter =
+			list_entry(pos, struct its_device, dev_list);
+		if (id < iter->device_id)
+			break;
+		pos = pos->next;
+	}
+	list_add_tail(&dev->dev_list, pos);
+}
+
 static int vgic_its_alloc_device(struct vgic_its *its,
 				 struct its_device **devp,
 				 u32 device_id, gpa_t itt_addr_field,
@@ -858,7 +889,8 @@ static int vgic_its_alloc_device(struct vgic_its *its,
 	device->nb_eventid_bits = size_field + 1;
 	INIT_LIST_HEAD(&device->itt_head);
 
-	list_add_tail(&device->dev_list, &its->device_list);
+	device_list_insert_sorted(&its->device_list, device);
+
 	*devp = device;
 
 	return 0;
-- 
2.5.5

[RFC v2 14/19] KVM: arm64: ITS: Add infrastructure for table lookup

[Eric Auger]

Add a generic lookup_table() helper whose role consists in
scanning a contiguous table located in guest RAM and applying
a callback on each entry. Entries can be handled as linked lists
since the callback may return an offset to the next entry and
also tell that an entry is the last one.

Helper functions also are added to compute the device/event ID
offset to the next DTE/ITE.

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

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 1cd6ae6..cf04776 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -181,6 +181,8 @@ static struct its_ite *find_ite(struct vgic_its *its, u32 device_id,
 #define VITS_ESZ 8
 #define VITS_TYPER_IDBITS 0xF
 #define VITS_TYPER_DEVBITS 0xF
+#define VITS_DTE_MAX_DEVID_OFFSET GENMASK(19, 0)
+#define VITS_ITE_MAX_EVENTID_OFFSET GENMASK(16, 0)
 
 /*
  * Finds and returns a collection in the ITS collection table.
@@ -1607,6 +1609,123 @@ int vgic_its_attr_regs_access(struct kvm_device *dev,
 	return 0;
 }
 
+static u32 compute_next_devid_offset(struct list_head *h,
+				     struct its_device *dev)
+{
+	struct list_head *e = &dev->dev_list;
+	struct its_device *next;
+	u32 next_offset;
+
+	if (e->next == h)
+		return 0;
+	next = list_entry(e->next, struct its_device, dev_list);
+	next_offset = next->device_id - dev->device_id;
+
+	return next_offset < VITS_DTE_MAX_DEVID_OFFSET ?
+			next_offset : VITS_DTE_MAX_DEVID_OFFSET;
+}
+
+static u32 compute_next_eventid_offset(struct list_head *h,
+				       struct its_ite *ite)
+{
+	struct list_head *e = &ite->ite_list;
+	struct its_ite *next;
+	u32 next_offset;
+
+	if (e->next == h)
+		return 0;
+	next = list_entry(e->next, struct its_ite, ite_list);
+	next_offset = next->event_id - ite->event_id;
+
+	return next_offset < VITS_ITE_MAX_EVENTID_OFFSET ?
+			next_offset : VITS_ITE_MAX_EVENTID_OFFSET;
+}
+
+static int next_segment(unsigned long len, int offset)
+{
+	if (len > PAGE_SIZE - offset)
+		return PAGE_SIZE - offset;
+	else
+		return len;
+}
+
+/**
+ * entry_fn_t - Callback called on a table entry restore path
+ * @its: its handle
+ * @id: id of the entry
+ * @addr: kernel VA of the entry
+ * @opaque: pointer to an opaque data
+ * @next_offset: minimal ID offset to the next entry. 0 if this
+ * entry is the last one, 1 if the entry is invalid, >= 1 if an
+ * entry's next_offset field was truly decoded
+ *
+ * Return: < 0 on error, 0 otherwise
+ */
+typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *addr,
+			  void *opaque, u32 *next_offset);
+
+/**
+ * lookup_table - scans a contiguous table in guest RAM and applies a function
+ * to each entry
+ *
+ * @its: its handle
+ * @base: base gpa of the table
+ * @size: size of the table in bytes
+ * @esz: entry size in bytes
+ * @start_id: first entry's ID
+ * @fn: function to apply on each entry
+ *
+ * Return: < 0 on error, 1 if last element identified, 0 otherwise
+ */
+static int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
+			int start_id, entry_fn_t fn, void *opaque)
+{
+	gpa_t gpa = base, top = base + size - 1;
+	unsigned long len = top - gpa;
+	int ret, seg, offset = offset_in_page(gpa);
+	gfn_t gfn = gpa >> PAGE_SHIFT;
+	u32 id = start_id + (gpa - base)/esz;
+	struct kvm *kvm = its->dev->kvm;
+	kvm_pfn_t pfn;
+
+	while ((seg = next_segment(len, offset)) != 0) {
+		u32 next_offset;
+		bool writeable;
+		void *addr;
+
+		pfn = gfn_to_pfn_prot(kvm, gfn, true, &writeable);
+		if (is_error_noslot_pfn(pfn))
+			return -EINVAL;
+		addr = phys_to_virt((u64)pfn << PAGE_SHIFT);
+		addr += offset;
+
+		while (seg > 0) {
+			size_t byte_offset;
+
+			ret = fn(its, id, addr, opaque, &next_offset);
+			if (ret < 0 || (!ret && !next_offset))
+				goto out;
+
+			byte_offset = next_offset * esz;
+
+			id += next_offset;
+			gpa += byte_offset;
+			addr += byte_offset;
+			seg -= byte_offset;
+		}
+		kvm_release_pfn_clean(pfn);
+
+		len = top - gpa;
+		offset = offset_in_page(gpa);
+		gfn = gpa >> PAGE_SHIFT;
+		id = start_id + (gpa - base)/esz;
+	}
+	return 0;
+out:
+	kvm_release_pfn_clean(pfn);
+	return (ret < 0 ? ret : 1);
+}
+
 /**
  * vgic_its_flush_pending_tables - Flush the pending tables into guest RAM
  */
-- 
2.5.5

[RFC v2 15/19] 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.

We do not index by collection ID as the collection entry
can fit into 8 bytes while containing the collection ID.

On restore path we re-allocate the collection objects.

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

---

v1 -> v2:
- reword commit message and directly use 8 as entry size
- no kvm parameter anymore
- add helper for flush/restore cte
- table size computed here
- add le64/cpu conversions
---
 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 cf04776..ad67759 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1761,13 +1761,84 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
 	return -ENXIO;
 }
 
+static int vgic_its_flush_cte(struct vgic_its *its,
+			      struct its_collection *collection, u64 *ptr)
+{
+	u64 val;
+	int ret;
+
+	val = ((u64)1 << 63 | ((u64)collection->target_addr << 16) |
+	       collection->collection_id);
+	val = cpu_to_le64(val);
+	ret = kvm_write_guest(its->dev->kvm, (gpa_t)ptr, &val, 8);
+	return ret;
+}
+
+static int vgic_its_restore_cte(struct vgic_its *its, u64 *ptr, bool *valid)
+{
+	struct its_collection *collection;
+	u32 target_addr;
+	u32 coll_id;
+	u64 val;
+	int ret;
+
+	*valid = false;
+
+	ret = kvm_read_guest(its->dev->kvm, (gpa_t)ptr, &val, 8);
+	if (ret)
+		return ret;
+	val = le64_to_cpu(val);
+	*valid = val & BIT_ULL(63);
+
+	if (!*valid)
+		return 0;
+
+	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;
+	return 0;
+}
+
 /**
  * vgic_its_flush_collection_table - flush the collection table into
  * guest RAM
  */
 static int vgic_its_flush_collection_table(struct vgic_its *its)
 {
-	return -ENXIO;
+	struct its_collection *collection;
+	u64 val, *ptr;
+	size_t max_size, filled = 0;
+	int ret;
+
+	ptr = (u64 *)(BASER_ADDRESS(its->baser_coll_table));
+	if (!ptr)
+		return 0;
+
+	max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K;
+
+	list_for_each_entry(collection, &its->collection_list, coll_list) {
+		if (filled == max_size)
+			return -ENOSPC;
+		ret = vgic_its_flush_cte(its, collection, ptr);
+		if (ret)
+			return ret;
+		ptr++;
+		filled += 8;
+	}
+
+	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(its->dev->kvm, (gpa_t)ptr, &val, 8);
+	return ret;
 }
 
 /**
@@ -1777,7 +1848,26 @@ static int vgic_its_flush_collection_table(struct vgic_its *its)
  */
 static int vgic_its_restore_collection_table(struct vgic_its *its)
 {
-	return -ENXIO;
+	size_t max_size, read = 0;
+	u64 *ptr;
+	int ret;
+
+	ptr = (u64 *)(BASER_ADDRESS(its->baser_coll_table));
+	if (!ptr)
+		return 0;
+
+	max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K;
+
+	while (read < max_size) {
+		bool valid;
+
+		ret = vgic_its_restore_cte(its, ptr, &valid);
+		if (!valid || ret)
+			break;
+		ptr++;
+		read += 8;
+	}
+	return ret;
 }
 
 /**
-- 
2.5.5

[RFC v2 16/19] KVM: arm64: ITS: vgic_its_check_id returns the entry's GPA

[Eric Auger]

As vgic_its_check_id() computes the device/collection entry's
GPA, let's return it so that new callers can retrieve it easily.

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

---

v2: new
---
 virt/kvm/arm/vgic/vgic-its.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index ad67759..1060125 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -637,7 +637,8 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
  * is actually valid (covered by a memslot and guest accessible).
  * For this we have to read the respective first level entry.
  */
-static bool vgic_its_check_id(struct vgic_its *its, u64 baser, int id)
+static bool vgic_its_check_id(struct vgic_its *its, u64 baser, int id,
+			      gpa_t *eaddr)
 {
 	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
 	int index;
@@ -657,6 +658,7 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, int id)
 		addr = BASER_ADDRESS(baser) + id * esz;
 		gfn = addr >> PAGE_SHIFT;
 
+		*eaddr = addr;
 		return kvm_is_visible_gfn(its->dev->kvm, gfn);
 	}
 
@@ -689,6 +691,7 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, int id)
 	indirect_ptr += index * esz;
 	gfn = indirect_ptr >> PAGE_SHIFT;
 
+	*eaddr = indirect_ptr;
 	return kvm_is_visible_gfn(its->dev->kvm, gfn);
 }
 
@@ -697,8 +700,9 @@ static int vgic_its_alloc_collection(struct vgic_its *its,
 				     u32 coll_id)
 {
 	struct its_collection *collection;
+	gpa_t eaddr;
 
-	if (!vgic_its_check_id(its, its->baser_coll_table, coll_id))
+	if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, &eaddr))
 		return E_ITS_MAPC_COLLECTION_OOR;
 
 	collection = kzalloc(sizeof(*collection), GFP_KERNEL);
@@ -910,8 +914,9 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
 	size_t size = its_cmd_get_size(its_cmd);
 	gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
 	struct its_device *device;
+	gpa_t eaddr;
 
-	if (!vgic_its_check_id(its, its->baser_device_table, device_id))
+	if (!vgic_its_check_id(its, its->baser_device_table, device_id, &eaddr))
 		return E_ITS_MAPD_DEVICE_OOR;
 
 	if (valid && size > VITS_TYPER_IDBITS)
-- 
2.5.5

[RFC v2 17/19] KVM: arm64: ITS: ITT flush and restore

[Eric Auger]

Introduce routines to flush and restore device ITT and their
interrupt table entries (ITE).

The routines will be called on device table flush and
restore.

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

---

v2: creation
---
 virt/kvm/arm/vgic/vgic-its.c | 99 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 1060125..be9e8ed 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1748,6 +1748,105 @@ static int vgic_its_restore_pending_tables(struct vgic_its *its)
 	return -ENXIO;
 }
 
+static int vgic_its_flush_ite(struct vgic_its *its, struct its_device *dev,
+			      struct its_ite *ite, gpa_t gpa)
+{
+	struct kvm *kvm = its->dev->kvm;
+	u32 next_offset;
+	u64 val;
+
+	next_offset = compute_next_eventid_offset(&dev->itt_head, ite);
+	val = ((u64)next_offset << 48) | ((u64)ite->lpi << 16) |
+		ite->collection->collection_id;
+	val = cpu_to_le64(val);
+	return kvm_write_guest(kvm, gpa, &val, VITS_ESZ);
+}
+
+/**
+ * vgic_its_restore_ite - restore an interrupt translation entry
+ * @event_id: id used for indexing
+ * @ptr: kernel VA where the 8 byte ITE is located
+ * @opaque: pointer to the its_device
+ * @next: id offset to the next entry
+ */
+static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
+				void *ptr, void *opaque, u32 *next)
+{
+	struct its_device *dev = (struct its_device *)opaque;
+	struct its_collection *collection;
+	struct kvm *kvm = its->dev->kvm;
+	u64 val, *p = (u64 *)ptr;
+	struct vgic_irq *irq;
+	u32 coll_id, lpi_id;
+	struct its_ite *ite;
+	int ret;
+
+	val = *p;
+	*next = 1;
+
+	val = le64_to_cpu(val);
+
+	coll_id = val & GENMASK_ULL(15, 0);
+	lpi_id = (val & GENMASK_ULL(47, 16)) >> 16;
+
+	if (!lpi_id)
+		return 0;
+
+	*next = (val & GENMASK_ULL(63, 48)) >> 48;
+
+	collection = find_collection(its, coll_id);
+	if (!collection)
+		return -EINVAL;
+
+	ret = vgic_its_alloc_ite(dev, &ite, collection,
+				  lpi_id, event_id);
+	if (ret)
+		return ret;
+
+	irq = vgic_add_lpi(kvm, lpi_id);
+	if (IS_ERR(irq))
+		return PTR_ERR(irq);
+	ite->irq = irq;
+
+	/* restore the configuration of the LPI */
+	ret = update_lpi_config(kvm, irq, NULL);
+	if (ret)
+		return ret;
+
+	update_affinity_ite(kvm, ite);
+}
+
+static int vgic_its_flush_itt(struct vgic_its *its, struct its_device *device)
+{
+	gpa_t base = device->itt_addr;
+	struct its_ite *ite;
+	int ret;
+
+	list_for_each_entry(ite, &device->itt_head, ite_list) {
+		gpa_t gpa = base + ite->event_id * VITS_ESZ;
+
+		ret = vgic_its_flush_ite(its, device, ite, gpa);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+static int vgic_its_restore_itt(struct vgic_its *its,
+				struct its_device *dev)
+{
+	size_t max_size = (2 << dev->nb_eventid_bits) * VITS_ESZ;
+	gpa_t base = dev->itt_addr;
+	int ret;
+
+	ret =  lookup_table(its, base, max_size, VITS_ESZ, 0,
+			    vgic_its_restore_ite, dev);
+
+	if (ret < 0)
+		return ret;
+	return 0;
+}
+
 /**
  * vgic_its_flush_device_tables - flush the device table and all ITT
  * into guest RAM
-- 
2.5.5

[RFC v2 18/19] KVM: arm64: ITS: Device table save/restore

[Eric Auger]

This patch flushes the device table entries into guest RAM.
Both flat table and 2 stage tables are supported.  DeviceId
indexing is used.

For each device listed in the device table, we also flush
the translation table using the vgic_its_flush/restore_itt
routines.

On restore, devices are re-allocated and their itte are
re-built.

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

---

v1 -> v2:
- use 8 byte format for DTE and ITE
- support 2 stage format
- remove kvm parameter
- ITT flush/restore moved in a separate patch
- use deviceid indexing
---
 virt/kvm/arm/vgic/vgic-its.c | 145 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 143 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index be9e8ed..c1ae85b 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1814,6 +1814,7 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
 		return ret;
 
 	update_affinity_ite(kvm, ite);
+	return 0;
 }
 
 static int vgic_its_flush_itt(struct vgic_its *its, struct its_device *device)
@@ -1848,12 +1849,137 @@ static int vgic_its_restore_itt(struct vgic_its *its,
 }
 
 /**
+ * vgic_its_flush_dte - Flush a device table entry at a given GPA
+ *
+ * @its: ITS handle
+ * @dev: ITS device
+ * @ptr: GPA
+ */
+static int vgic_its_flush_dte(struct vgic_its *its,
+			      struct its_device *dev, gpa_t ptr)
+{
+	struct kvm *kvm = its->dev->kvm;
+	u64 val, itt_addr_field;
+	int ret;
+	u32 next_offset;
+
+	itt_addr_field = dev->itt_addr >> 8;
+	next_offset = compute_next_devid_offset(&its->device_list, dev);
+	val = (((u64)next_offset << 45) | (itt_addr_field << 5) |
+		(dev->nb_eventid_bits - 1));
+	val = cpu_to_le64(val);
+	ret = kvm_write_guest(kvm, ptr, &val, 8);
+	return ret;
+}
+
+/**
+ * vgic_its_restore_dte - restore a device table entry
+ *
+ * @its: its handle
+ * @id: device id the DTE corresponds to
+ * @ptr: kernel VA where the 8 byte DTE is located
+ * @opaque: unused
+ * @next: offset to the next valid device id
+ *
+ * Return: < 0 on error, 0 otherwise
+ */
+static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
+				void *ptr, void *opaque, u32 *next)
+{
+	struct its_device *dev;
+	gpa_t itt_addr;
+	size_t size;
+	u64 val, *p = (u64 *)ptr;
+	int ret;
+
+	val = *p;
+	val = le64_to_cpu(val);
+
+	size = val & GENMASK_ULL(4, 0);
+	itt_addr = (val & GENMASK_ULL(48, 5)) >> 5;
+	*next = 1;
+
+	if (!itt_addr)
+		return 0;
+
+	/* dte entry is valid */
+	*next = (val & GENMASK_ULL(63, 45)) >> 45;
+
+	ret = vgic_its_alloc_device(its, &dev, id,
+				    itt_addr, size);
+	if (ret)
+		return ret;
+	ret = vgic_its_restore_itt(its, dev);
+
+	return ret;
+}
+
+/**
  * vgic_its_flush_device_tables - flush the device table and all ITT
  * into guest RAM
  */
 static int vgic_its_flush_device_tables(struct vgic_its *its)
 {
-	return -ENXIO;
+	struct its_device *dev;
+	u64 baser;
+
+	baser = its->baser_device_table;
+
+	list_for_each_entry(dev, &its->device_list, dev_list) {
+		int ret;
+		gpa_t eaddr;
+
+		if (!vgic_its_check_id(its, baser,
+				       dev->device_id, &eaddr))
+			return -EINVAL;
+
+		ret = vgic_its_flush_itt(its, dev);
+		if (ret)
+			return ret;
+
+		ret = vgic_its_flush_dte(its, dev, eaddr);
+		if (ret)
+			return ret;
+		}
+	return 0;
+}
+
+/**
+ * handle_l1_entry - callback used for L1 entries (2 stage case)
+ *
+ * @its: its handle
+ * @id: id
+ * @addr: kernel VA
+ * @opaque: unused
+ * @next_offset: offset to the next L1 entry: 0 if the last element
+ * was found, 1 otherwise
+ */
+static int handle_l1_entry(struct vgic_its *its, u32 id, void *addr,
+			   void *opaque, u32 *next_offset)
+{
+	u64 *pe = addr;
+	gpa_t gpa;
+	int l2_start_id = id * (SZ_64K / 8);
+	int ret;
+
+	*pe = le64_to_cpu(*pe);
+	*next_offset = 1;
+
+	if (!(*pe & BIT_ULL(63)))
+		return 0;
+
+	gpa = *pe & GENMASK_ULL(51, 16);
+
+	ret = lookup_table(its, gpa, SZ_64K, 8,
+			    l2_start_id, vgic_its_restore_dte, NULL);
+
+	if (ret == 1) {
+		/* last entry was found in this L2 table */
+		*next_offset = 0;
+		ret = 0;
+	}
+
+	return ret;
 }
 
 /**
@@ -1862,7 +1988,22 @@ static int vgic_its_flush_device_tables(struct vgic_its *its)
  */
 static int vgic_its_restore_device_tables(struct vgic_its *its)
 {
-	return -ENXIO;
+	u64 baser = its->baser_device_table;
+	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
+	int l1_esz = GITS_BASER_ENTRY_SIZE(baser);
+	gpa_t l1_gpa;
+
+	l1_gpa = BASER_ADDRESS(baser);
+	if (!l1_gpa)
+		return 0;
+
+	if (!(baser & GITS_BASER_INDIRECT))
+		return lookup_table(its, l1_gpa, l1_tbl_size, l1_esz,
+				    0, vgic_its_restore_dte, NULL);
+
+	/* two stage table */
+	return lookup_table(its, l1_gpa, l1_tbl_size, 8, 0,
+			    handle_l1_entry, NULL);
 }
 
 static int vgic_its_flush_cte(struct vgic_its *its,
-- 
2.5.5

[RFC v2 19/19] 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.

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

---

v1 -> v2:
- do not care about the 1st KB which should be zeroed according to
  the spec.
---
 virt/kvm/arm/vgic/vgic-its.c | 71 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 69 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index c1ae85b..67db680 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1736,7 +1736,48 @@ static int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz,
  */
 static int vgic_its_flush_pending_tables(struct vgic_its *its)
 {
-	return -ENXIO;
+	struct kvm *kvm = its->dev->kvm;
+	struct vgic_dist *dist = &kvm->arch.vgic;
+	struct vgic_irq *irq;
+	int ret;
+
+	/**
+	 * we do not take the dist->lpi_list_lock since we have a garantee
+	 * the LPI list is not touched while the its lock is 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_latch)
+			continue;
+
+		if (irq->pending_latch)
+			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;
 }
 
 /**
@@ -1745,7 +1786,33 @@ static int vgic_its_flush_pending_tables(struct vgic_its *its)
  */
 static int vgic_its_restore_pending_tables(struct vgic_its *its)
 {
-	return -ENXIO;
+	struct vgic_irq *irq;
+	struct kvm *kvm = its->dev->kvm;
+	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_latch = val & (1 << (irq->intid % BITS_PER_BYTE));
+	}
+	return 0;
 }
 
 static int vgic_its_flush_ite(struct vgic_its *its, struct its_device *dev,
-- 
2.5.5

Re: [RFC v2 05/19] KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access

[Andre Przywara]

On 08/02/17 11:43, Eric Auger wrote:

Salut Eric,

one minor thing below, but first a general question:
I take it that the state of the ITS (enabled/disabled) shouldn't matter
when it comes to reading/writing registers, right? Because this is
totally under guest control and userland shouldn't mess with it?

Maybe this is handled somewhere in the next patches, but how are we
supposed to write CBASER and the BASERs, for instance, if the handler
bails out early when the ITS is already enabled?

> 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 43bb17e..e9c8f9f 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;

Isn't GITS_IIDR only 32-bit aligned?
Is it expected to just avoid reading this from userland?
If yes, it deserves a comment here, I guess.

Cheers,
Andre.

> +
> +	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 055ad42..ad8a585 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;
> 

[RFC v2 05/19] KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access

[Andre Przywara]

On 08/02/17 11:43, Eric Auger wrote:

Salut Eric,

one minor thing below, but first a general question:
I take it that the state of the ITS (enabled/disabled) shouldn't matter
when it comes to reading/writing registers, right? Because this is
totally under guest control and userland shouldn't mess with it?

Maybe this is handled somewhere in the next patches, but how are we
supposed to write CBASER and the BASERs, for instance, if the handler
bails out early when the ITS is already enabled?

> 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 43bb17e..e9c8f9f 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;

Isn't GITS_IIDR only 32-bit aligned?
Is it expected to just avoid reading this from userland?
If yes, it deserves a comment here, I guess.

Cheers,
Andre.

> +
> +	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 055ad42..ad8a585 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;
> 

Re: [RFC v2 05/19] KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access

[Auger Eric]

Hi Andre,

On 10/02/2017 12:57, Andre Przywara wrote:
> On 08/02/17 11:43, Eric Auger wrote:
> 
> Salut Eric,
> 
> one minor thing below, but first a general question:
> I take it that the state of the ITS (enabled/disabled) shouldn't matter
> when it comes to reading/writing registers, right? Because this is
> totally under guest control and userland shouldn't mess with it?
> 
> Maybe this is handled somewhere in the next patches, but how are we
> supposed to write CBASER and the BASERs, for instance, if the handler
> bails out early when the ITS is already enabled?
Well I mentioned in the KVM ITS device documentation that userspace
accesses to those registers have the same behavior as guest accesses. As
such it is not possible to write into CBASER and BASERs when the ITS is
already enabled. But isn't it correct to forbid such userspace accesses
at this moment? What is the use case?
> 
>> 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 43bb17e..e9c8f9f 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;
> 
> Isn't GITS_IIDR only 32-bit aligned?
> Is it expected to just avoid reading this from userland?
> If yes, it deserves a comment here, I guess.
No it was not made on purpose :-( I will fix that.

Thanks!

Eric
> 
> Cheers,
> Andre.
> 
>> +
>> +	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 055ad42..ad8a585 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;
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[RFC v2 05/19] KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access

[Auger Eric]

Hi Andre,

On 10/02/2017 12:57, Andre Przywara wrote:
> On 08/02/17 11:43, Eric Auger wrote:
> 
> Salut Eric,
> 
> one minor thing below, but first a general question:
> I take it that the state of the ITS (enabled/disabled) shouldn't matter
> when it comes to reading/writing registers, right? Because this is
> totally under guest control and userland shouldn't mess with it?
> 
> Maybe this is handled somewhere in the next patches, but how are we
> supposed to write CBASER and the BASERs, for instance, if the handler
> bails out early when the ITS is already enabled?
Well I mentioned in the KVM ITS device documentation that userspace
accesses to those registers have the same behavior as guest accesses. As
such it is not possible to write into CBASER and BASERs when the ITS is
already enabled. But isn't it correct to forbid such userspace accesses
at this moment? What is the use case?
> 
>> 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 43bb17e..e9c8f9f 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;
> 
> Isn't GITS_IIDR only 32-bit aligned?
> Is it expected to just avoid reading this from userland?
> If yes, it deserves a comment here, I guess.
No it was not made on purpose :-( I will fix that.

Thanks!

Eric
> 
> Cheers,
> Andre.
> 
>> +
>> +	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 055ad42..ad8a585 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;
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [RFC v2 05/19] KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access

[Andre Przywara]

Hi,

On 10/02/17 12:26, Auger Eric wrote:
> Hi Andre,
> 
> On 10/02/2017 12:57, Andre Przywara wrote:
>> On 08/02/17 11:43, Eric Auger wrote:
>>
>> Salut Eric,
>>
>> one minor thing below, but first a general question:
>> I take it that the state of the ITS (enabled/disabled) shouldn't matter
>> when it comes to reading/writing registers, right? Because this is
>> totally under guest control and userland shouldn't mess with it?
>>
>> Maybe this is handled somewhere in the next patches, but how are we
>> supposed to write CBASER and the BASERs, for instance, if the handler
>> bails out early when the ITS is already enabled?
> Well I mentioned in the KVM ITS device documentation 

Oh, I am one of those guys who read the documentation at the very end
;-) Sorry, my bad.

> that userspace
> accesses to those registers have the same behavior as guest accesses. As
> such it is not possible to write into CBASER and BASERs when the ITS is
> already enabled. But isn't it correct to forbid such userspace accesses
> at this moment? What is the use case?

So the idea is to observe an order when restoring the registers? And
relying on the ITS being disabled upon creation, so that we can write to
those registers? Then restoring CTLR as the very last to get things going?

I think we need some special handling for CWRITER as well, but I just
see that we have some bug in our ITS emulation (processing commands
while the ITS being disabled and not triggering command processing upon
ITS enablement). Waiting for Marc's verdict on this.
I think the patch I came up with should fix this particular issue here.

But apart from that it should work, I think.

Cheers,
Andre.

>>> 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 43bb17e..e9c8f9f 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;
>>
>> Isn't GITS_IIDR only 32-bit aligned?
>> Is it expected to just avoid reading this from userland?
>> If yes, it deserves a comment here, I guess.
> No it was not made on purpose :-( I will fix that.
> 
> Thanks!
> 
> Eric
>>
>> Cheers,
>> Andre.
>>
>>> +
>>> +	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 055ad42..ad8a585 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;
>>>
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>

[RFC v2 05/19] KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access

[Andre Przywara]

Hi,

On 10/02/17 12:26, Auger Eric wrote:
> Hi Andre,
> 
> On 10/02/2017 12:57, Andre Przywara wrote:
>> On 08/02/17 11:43, Eric Auger wrote:
>>
>> Salut Eric,
>>
>> one minor thing below, but first a general question:
>> I take it that the state of the ITS (enabled/disabled) shouldn't matter
>> when it comes to reading/writing registers, right? Because this is
>> totally under guest control and userland shouldn't mess with it?
>>
>> Maybe this is handled somewhere in the next patches, but how are we
>> supposed to write CBASER and the BASERs, for instance, if the handler
>> bails out early when the ITS is already enabled?
> Well I mentioned in the KVM ITS device documentation 

Oh, I am one of those guys who read the documentation at the very end
;-) Sorry, my bad.

> that userspace
> accesses to those registers have the same behavior as guest accesses. As
> such it is not possible to write into CBASER and BASERs when the ITS is
> already enabled. But isn't it correct to forbid such userspace accesses
> at this moment? What is the use case?

So the idea is to observe an order when restoring the registers? And
relying on the ITS being disabled upon creation, so that we can write to
those registers? Then restoring CTLR as the very last to get things going?

I think we need some special handling for CWRITER as well, but I just
see that we have some bug in our ITS emulation (processing commands
while the ITS being disabled and not triggering command processing upon
ITS enablement). Waiting for Marc's verdict on this.
I think the patch I came up with should fix this particular issue here.

But apart from that it should work, I think.

Cheers,
Andre.

>>> 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 43bb17e..e9c8f9f 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;
>>
>> Isn't GITS_IIDR only 32-bit aligned?
>> Is it expected to just avoid reading this from userland?
>> If yes, it deserves a comment here, I guess.
> No it was not made on purpose :-( I will fix that.
> 
> Thanks!
> 
> Eric
>>
>> Cheers,
>> Andre.
>>
>>> +
>>> +	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 055ad42..ad8a585 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;
>>>
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>

Re: [RFC v2 05/19] KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access

[Auger Eric]

Hi Andre,

On 10/02/2017 18:06, Andre Przywara wrote:
> Hi,
> 
> On 10/02/17 12:26, Auger Eric wrote:
>> Hi Andre,
>>
>> On 10/02/2017 12:57, Andre Przywara wrote:
>>> On 08/02/17 11:43, Eric Auger wrote:
>>>
>>> Salut Eric,
>>>
>>> one minor thing below, but first a general question:
>>> I take it that the state of the ITS (enabled/disabled) shouldn't matter
>>> when it comes to reading/writing registers, right? Because this is
>>> totally under guest control and userland shouldn't mess with it?
>>>
>>> Maybe this is handled somewhere in the next patches, but how are we
>>> supposed to write CBASER and the BASERs, for instance, if the handler
>>> bails out early when the ITS is already enabled?
>> Well I mentioned in the KVM ITS device documentation 
> 
> Oh, I am one of those guys who read the documentation at the very end
> ;-) Sorry, my bad.
> 
>> that userspace
>> accesses to those registers have the same behavior as guest accesses. As
>> such it is not possible to write into CBASER and BASERs when the ITS is
>> already enabled. But isn't it correct to forbid such userspace accesses
>> at this moment? What is the use case?
> 
> So the idea is to observe an order when restoring the registers? And
> relying on the ITS being disabled upon creation, so that we can write to
> those registers? Then restoring CTLR as the very last to get things going?
Yes that's what I currently do on QEMU side.
> 
> I think we need some special handling for CWRITER as well, but I just
> see that we have some bug in our ITS emulation (processing commands
> while the ITS being disabled and not triggering command processing upon
> ITS enablement). Waiting for Marc's verdict on this.
> I think the patch I came up with should fix this particular issue here.
OK Looking forward to reviewing it.

Thanks

Eric
> 
> But apart from that it should work, I think.
> 
> Cheers,
> Andre.
> 
>>>> 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 43bb17e..e9c8f9f 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;
>>>
>>> Isn't GITS_IIDR only 32-bit aligned?
>>> Is it expected to just avoid reading this from userland?
>>> If yes, it deserves a comment here, I guess.
>> No it was not made on purpose :-( I will fix that.
>>
>> Thanks!
>>
>> Eric
>>>
>>> Cheers,
>>> Andre.
>>>
>>>> +
>>>> +	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 055ad42..ad8a585 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;
>>>>
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[RFC v2 05/19] KVM: arm64: ITS: Implement vgic_its_has_attr_regs and attr_regs_access

[Auger Eric]

Hi Andre,

On 10/02/2017 18:06, Andre Przywara wrote:
> Hi,
> 
> On 10/02/17 12:26, Auger Eric wrote:
>> Hi Andre,
>>
>> On 10/02/2017 12:57, Andre Przywara wrote:
>>> On 08/02/17 11:43, Eric Auger wrote:
>>>
>>> Salut Eric,
>>>
>>> one minor thing below, but first a general question:
>>> I take it that the state of the ITS (enabled/disabled) shouldn't matter
>>> when it comes to reading/writing registers, right? Because this is
>>> totally under guest control and userland shouldn't mess with it?
>>>
>>> Maybe this is handled somewhere in the next patches, but how are we
>>> supposed to write CBASER and the BASERs, for instance, if the handler
>>> bails out early when the ITS is already enabled?
>> Well I mentioned in the KVM ITS device documentation 
> 
> Oh, I am one of those guys who read the documentation at the very end
> ;-) Sorry, my bad.
> 
>> that userspace
>> accesses to those registers have the same behavior as guest accesses. As
>> such it is not possible to write into CBASER and BASERs when the ITS is
>> already enabled. But isn't it correct to forbid such userspace accesses
>> at this moment? What is the use case?
> 
> So the idea is to observe an order when restoring the registers? And
> relying on the ITS being disabled upon creation, so that we can write to
> those registers? Then restoring CTLR as the very last to get things going?
Yes that's what I currently do on QEMU side.
> 
> I think we need some special handling for CWRITER as well, but I just
> see that we have some bug in our ITS emulation (processing commands
> while the ITS being disabled and not triggering command processing upon
> ITS enablement). Waiting for Marc's verdict on this.
> I think the patch I came up with should fix this particular issue here.
OK Looking forward to reviewing it.

Thanks

Eric
> 
> But apart from that it should work, I think.
> 
> Cheers,
> Andre.
> 
>>>> 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 43bb17e..e9c8f9f 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;
>>>
>>> Isn't GITS_IIDR only 32-bit aligned?
>>> Is it expected to just avoid reading this from userland?
>>> If yes, it deserves a comment here, I guess.
>> No it was not made on purpose :-( I will fix that.
>>
>> Thanks!
>>
>> Eric
>>>
>>> Cheers,
>>> Andre.
>>>
>>>> +
>>>> +	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 055ad42..ad8a585 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;
>>>>
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [RFC v2 18/19] KVM: arm64: ITS: Device table save/restore

[Auger Eric]

Hi all,
On 08/02/2017 12:43, Eric Auger wrote:
> This patch flushes the device table entries into guest RAM.
> Both flat table and 2 stage tables are supported.  DeviceId
> indexing is used.
> 
> For each device listed in the device table, we also flush
> the translation table using the vgic_its_flush/restore_itt
> routines.
> 
> On restore, devices are re-allocated and their itte are
> re-built.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v1 -> v2:
> - use 8 byte format for DTE and ITE
> - support 2 stage format
> - remove kvm parameter
> - ITT flush/restore moved in a separate patch
> - use deviceid indexing
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 145 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 143 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index be9e8ed..c1ae85b 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1814,6 +1814,7 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
>  		return ret;
>  
>  	update_affinity_ite(kvm, ite);
> +	return 0;
>  }
>  
>  static int vgic_its_flush_itt(struct vgic_its *its, struct its_device *device)
> @@ -1848,12 +1849,137 @@ static int vgic_its_restore_itt(struct vgic_its *its,
>  }
>  
>  /**
> + * vgic_its_flush_dte - Flush a device table entry at a given GPA
> + *
> + * @its: ITS handle
> + * @dev: ITS device
> + * @ptr: GPA
> + */
> +static int vgic_its_flush_dte(struct vgic_its *its,
> +			      struct its_device *dev, gpa_t ptr)
> +{
> +	struct kvm *kvm = its->dev->kvm;
> +	u64 val, itt_addr_field;
> +	int ret;
> +	u32 next_offset;
> +
> +	itt_addr_field = dev->itt_addr >> 8;
> +	next_offset = compute_next_devid_offset(&its->device_list, dev);
> +	val = (((u64)next_offset << 45) | (itt_addr_field << 5) |
> +		(dev->nb_eventid_bits - 1));
> +	val = cpu_to_le64(val);
> +	ret = kvm_write_guest(kvm, ptr, &val, 8);
> +	return ret;
> +}
> +
> +/**
> + * vgic_its_restore_dte - restore a device table entry
> + *
> + * @its: its handle
> + * @id: device id the DTE corresponds to
> + * @ptr: kernel VA where the 8 byte DTE is located
> + * @opaque: unused
> + * @next: offset to the next valid device id
> + *
> + * Return: < 0 on error, 0 otherwise
> + */
> +static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
> +				void *ptr, void *opaque, u32 *next)
> +{
> +	struct its_device *dev;
> +	gpa_t itt_addr;
> +	size_t size;
> +	u64 val, *p = (u64 *)ptr;
> +	int ret;
> +
> +	val = *p;
> +	val = le64_to_cpu(val);
> +
> +	size = val & GENMASK_ULL(4, 0);
> +	itt_addr = (val & GENMASK_ULL(48, 5)) >> 5;
I Just discovered a bug above. 48 should be replaced by 44. With my LPI
setup the restore phase was failing.

I will send a formal fix soon. Please apologize for the inconvenience.

Best Regards

Eric
> +	*next = 1;
> +
> +	if (!itt_addr)
> +		return 0;
> +
> +	/* dte entry is valid */
> +	*next = (val & GENMASK_ULL(63, 45)) >> 45;
> +
> +	ret = vgic_its_alloc_device(its, &dev, id,
> +				    itt_addr, size);
> +	if (ret)
> +		return ret;
> +	ret = vgic_its_restore_itt(its, dev);
> +
> +	return ret;
> +}
> +
> +/**
>   * vgic_its_flush_device_tables - flush the device table and all ITT
>   * into guest RAM
>   */
>  static int vgic_its_flush_device_tables(struct vgic_its *its)
>  {
> -	return -ENXIO;
> +	struct its_device *dev;
> +	u64 baser;
> +
> +	baser = its->baser_device_table;
> +
> +	list_for_each_entry(dev, &its->device_list, dev_list) {
> +		int ret;
> +		gpa_t eaddr;
> +
> +		if (!vgic_its_check_id(its, baser,
> +				       dev->device_id, &eaddr))
> +			return -EINVAL;
> +
> +		ret = vgic_its_flush_itt(its, dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = vgic_its_flush_dte(its, dev, eaddr);
> +		if (ret)
> +			return ret;
> +		}
> +	return 0;
> +}
> +
> +/**
> + * handle_l1_entry - callback used for L1 entries (2 stage case)
> + *
> + * @its: its handle
> + * @id: id
> + * @addr: kernel VA
> + * @opaque: unused
> + * @next_offset: offset to the next L1 entry: 0 if the last element
> + * was found, 1 otherwise
> + */
> +static int handle_l1_entry(struct vgic_its *its, u32 id, void *addr,
> +			   void *opaque, u32 *next_offset)
> +{
> +	u64 *pe = addr;
> +	gpa_t gpa;
> +	int l2_start_id = id * (SZ_64K / 8);
> +	int ret;
> +
> +	*pe = le64_to_cpu(*pe);
> +	*next_offset = 1;
> +
> +	if (!(*pe & BIT_ULL(63)))
> +		return 0;
> +
> +	gpa = *pe & GENMASK_ULL(51, 16);
> +
> +	ret = lookup_table(its, gpa, SZ_64K, 8,
> +			    l2_start_id, vgic_its_restore_dte, NULL);
> +
> +	if (ret == 1) {
> +		/* last entry was found in this L2 table */
> +		*next_offset = 0;
> +		ret = 0;
> +	}
> +
> +	return ret;
>  }
>  
>  /**
> @@ -1862,7 +1988,22 @@ static int vgic_its_flush_device_tables(struct vgic_its *its)
>   */
>  static int vgic_its_restore_device_tables(struct vgic_its *its)
>  {
> -	return -ENXIO;
> +	u64 baser = its->baser_device_table;
> +	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
> +	int l1_esz = GITS_BASER_ENTRY_SIZE(baser);
> +	gpa_t l1_gpa;
> +
> +	l1_gpa = BASER_ADDRESS(baser);
> +	if (!l1_gpa)
> +		return 0;
> +
> +	if (!(baser & GITS_BASER_INDIRECT))
> +		return lookup_table(its, l1_gpa, l1_tbl_size, l1_esz,
> +				    0, vgic_its_restore_dte, NULL);
> +
> +	/* two stage table */
> +	return lookup_table(its, l1_gpa, l1_tbl_size, 8, 0,
> +			    handle_l1_entry, NULL);
>  }
>  
>  static int vgic_its_flush_cte(struct vgic_its *its,
> 

[RFC v2 18/19] KVM: arm64: ITS: Device table save/restore

[Auger Eric]

Hi all,
On 08/02/2017 12:43, Eric Auger wrote:
> This patch flushes the device table entries into guest RAM.
> Both flat table and 2 stage tables are supported.  DeviceId
> indexing is used.
> 
> For each device listed in the device table, we also flush
> the translation table using the vgic_its_flush/restore_itt
> routines.
> 
> On restore, devices are re-allocated and their itte are
> re-built.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v1 -> v2:
> - use 8 byte format for DTE and ITE
> - support 2 stage format
> - remove kvm parameter
> - ITT flush/restore moved in a separate patch
> - use deviceid indexing
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 145 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 143 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index be9e8ed..c1ae85b 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1814,6 +1814,7 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
>  		return ret;
>  
>  	update_affinity_ite(kvm, ite);
> +	return 0;
>  }
>  
>  static int vgic_its_flush_itt(struct vgic_its *its, struct its_device *device)
> @@ -1848,12 +1849,137 @@ static int vgic_its_restore_itt(struct vgic_its *its,
>  }
>  
>  /**
> + * vgic_its_flush_dte - Flush a device table entry at a given GPA
> + *
> + * @its: ITS handle
> + * @dev: ITS device
> + * @ptr: GPA
> + */
> +static int vgic_its_flush_dte(struct vgic_its *its,
> +			      struct its_device *dev, gpa_t ptr)
> +{
> +	struct kvm *kvm = its->dev->kvm;
> +	u64 val, itt_addr_field;
> +	int ret;
> +	u32 next_offset;
> +
> +	itt_addr_field = dev->itt_addr >> 8;
> +	next_offset = compute_next_devid_offset(&its->device_list, dev);
> +	val = (((u64)next_offset << 45) | (itt_addr_field << 5) |
> +		(dev->nb_eventid_bits - 1));
> +	val = cpu_to_le64(val);
> +	ret = kvm_write_guest(kvm, ptr, &val, 8);
> +	return ret;
> +}
> +
> +/**
> + * vgic_its_restore_dte - restore a device table entry
> + *
> + * @its: its handle
> + * @id: device id the DTE corresponds to
> + * @ptr: kernel VA where the 8 byte DTE is located
> + * @opaque: unused
> + * @next: offset to the next valid device id
> + *
> + * Return: < 0 on error, 0 otherwise
> + */
> +static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
> +				void *ptr, void *opaque, u32 *next)
> +{
> +	struct its_device *dev;
> +	gpa_t itt_addr;
> +	size_t size;
> +	u64 val, *p = (u64 *)ptr;
> +	int ret;
> +
> +	val = *p;
> +	val = le64_to_cpu(val);
> +
> +	size = val & GENMASK_ULL(4, 0);
> +	itt_addr = (val & GENMASK_ULL(48, 5)) >> 5;
I Just discovered a bug above. 48 should be replaced by 44. With my LPI
setup the restore phase was failing.

I will send a formal fix soon. Please apologize for the inconvenience.

Best Regards

Eric
> +	*next = 1;
> +
> +	if (!itt_addr)
> +		return 0;
> +
> +	/* dte entry is valid */
> +	*next = (val & GENMASK_ULL(63, 45)) >> 45;
> +
> +	ret = vgic_its_alloc_device(its, &dev, id,
> +				    itt_addr, size);
> +	if (ret)
> +		return ret;
> +	ret = vgic_its_restore_itt(its, dev);
> +
> +	return ret;
> +}
> +
> +/**
>   * vgic_its_flush_device_tables - flush the device table and all ITT
>   * into guest RAM
>   */
>  static int vgic_its_flush_device_tables(struct vgic_its *its)
>  {
> -	return -ENXIO;
> +	struct its_device *dev;
> +	u64 baser;
> +
> +	baser = its->baser_device_table;
> +
> +	list_for_each_entry(dev, &its->device_list, dev_list) {
> +		int ret;
> +		gpa_t eaddr;
> +
> +		if (!vgic_its_check_id(its, baser,
> +				       dev->device_id, &eaddr))
> +			return -EINVAL;
> +
> +		ret = vgic_its_flush_itt(its, dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = vgic_its_flush_dte(its, dev, eaddr);
> +		if (ret)
> +			return ret;
> +		}
> +	return 0;
> +}
> +
> +/**
> + * handle_l1_entry - callback used for L1 entries (2 stage case)
> + *
> + * @its: its handle
> + * @id: id
> + * @addr: kernel VA
> + * @opaque: unused
> + * @next_offset: offset to the next L1 entry: 0 if the last element
> + * was found, 1 otherwise
> + */
> +static int handle_l1_entry(struct vgic_its *its, u32 id, void *addr,
> +			   void *opaque, u32 *next_offset)
> +{
> +	u64 *pe = addr;
> +	gpa_t gpa;
> +	int l2_start_id = id * (SZ_64K / 8);
> +	int ret;
> +
> +	*pe = le64_to_cpu(*pe);
> +	*next_offset = 1;
> +
> +	if (!(*pe & BIT_ULL(63)))
> +		return 0;
> +
> +	gpa = *pe & GENMASK_ULL(51, 16);
> +
> +	ret = lookup_table(its, gpa, SZ_64K, 8,
> +			    l2_start_id, vgic_its_restore_dte, NULL);
> +
> +	if (ret == 1) {
> +		/* last entry was found in this L2 table */
> +		*next_offset = 0;
> +		ret = 0;
> +	}
> +
> +	return ret;
>  }
>  
>  /**
> @@ -1862,7 +1988,22 @@ static int vgic_its_flush_device_tables(struct vgic_its *its)
>   */
>  static int vgic_its_restore_device_tables(struct vgic_its *its)
>  {
> -	return -ENXIO;
> +	u64 baser = its->baser_device_table;
> +	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
> +	int l1_esz = GITS_BASER_ENTRY_SIZE(baser);
> +	gpa_t l1_gpa;
> +
> +	l1_gpa = BASER_ADDRESS(baser);
> +	if (!l1_gpa)
> +		return 0;
> +
> +	if (!(baser & GITS_BASER_INDIRECT))
> +		return lookup_table(its, l1_gpa, l1_tbl_size, l1_esz,
> +				    0, vgic_its_restore_dte, NULL);
> +
> +	/* two stage table */
> +	return lookup_table(its, l1_gpa, l1_tbl_size, 8, 0,
> +			    handle_l1_entry, NULL);
>  }
>  
>  static int vgic_its_flush_cte(struct vgic_its *its,
>