[PATCH v6 0/9] vITS Migration fixes and reset

[PATCH v6 0/9] vITS Migration fixes and reset

[Eric Auger]

This series fixes various bugs observed when saving/restoring the
ITS state before the guest writes the ITS registers (on first boot or
after reset/reboot).

This is a follow up of Wanghaibin's series [1] plus additional
patches following additional code review. It also proposes one
ITS reset implementation.

Currently, the in-kernel emulated ITS is not reset. After a
reset/reboot, the ITS register values and caches are left
unchanged. Registers may point to some tables in guest memory
which do not exist anymore. If an ITS state backup is initiated
before the guest re-writes the registers, the save fails
because inconsistencies are detected. Also restore of data saved
as such moment is failing.

Patches [1-4] are fixes of bugs observed during migration at
early guets boot stage.
- handle case where all collection, device and ITT entries are
  invalid on restore (which is not an error)
- Check the GITS_BASER<n> valid bit before attempting the save
  any table
- Check the GITS_BASER<n> and GITS_CBASER are valid before enabling
  the ITS

Patches [5-9] allow to empty the caches on reset and implement a
new ITS reset IOCTL

Best Regards

Eric

Git: complete series available at
https://github.com/eauger/linux/tree/v4.14-rc5-its-reset-v6

* Testing:
- on Cavium using a virtio-net-pci guest and various sequences of
  guest shutdown -r now, virsh reset, virsh suspend/resume,
  virsh reboot, virsh save.restore, virsh shutdown

References:
[1] [RFC PATCH 0/3] fix migrate failed when vm is in booting
https://www.spinics.net/lists/kvm-arm/msg27121.html

History:
v5 -> v6:
as per the discussions we had in the KVM forum, :
- don't try to fix everything without reset IOCTL
- removed "KVM: arm/arm64: vgic-its: Save the collection table
  before device tables"
- remove "The command queue is not allocated:" in API doc
- rework the locking in last patch
- vgic_its_free_device_list and vgic_its_free_collection_list
  do not take the its->lock anymore. The caller does.
- in vgic_its_restore_collection_table(), return 0 if last
  vgic_its_restore_cte returned +1

v4 -> v5:
- came back to the original version of
  KVM: arm/arm64: vgic-its: Fix return value for device table restore
  Rework of error handling will come later
- remove [PATCH v4 03/11] KVM: arm/arm64: vgic-its: Improve error reporting
  on device table save as of now
- remove KVM: arm/arm64: vgic-its: Always attempt to save/restore device
  and collection tables
  inversing the save order of device/collection tables fixes the same issue
- reword ITS IOCTL doc
- add mutex lock in vgic_its_free_collection_list
- remove vgic_its_unmap_device

v3 -> v4:
- fixes a bug in indirect mode: in handle_l1_dte, set
  *valid at the beginning of the function

v2 -> v3:
- Revisited error handling in restore functions
- Added "KVM: arm/arm64: vgic-its: fix
        vgic_its_restore_collection_table returned value"
- Added "KVM: arm/arm64: vgic-its: Check CBASER/BASER validity
  before enabling the ITS"
- Removed KVM: arm/arm64: vgic-its: Always allow clearing
  GITS_CREADR/CWRITER
- Reworded documentation according to Christoffer's comments

v1 -> v2:
- added KVM: arm/arm64: vgic-its: Always attempt to save/restore
  device and collection tables

PATCH v1
- series including 2 modified patches of Wanghaibin


Eric Auger (7):
  KVM: arm/arm64: vgic-its: Fix vgic_its_restore_collection_table
    returned value
  KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling
    the ITS
  KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving
    tables
  KVM: arm/arm64: vgic-its: Remove kvm_its_unmap_device
  KVM: arm/arm64: vgic-its: Free caches when GITS_BASER Valid bit is
    cleared
  KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL_RESET
  KVM: arm/arm64: vgic-its: Implement KVM_DEV_ARM_ITS_CTRL_RESET

wanghaibin (2):
  KVM: arm/arm64: vgic-its: Fix return value for device table restore
  KVM: arm/arm64: vgic-its: New helper functions to free the caches

 Documentation/virtual/kvm/devices/arm-vgic-its.txt |  20 ++
 arch/arm/include/uapi/asm/kvm.h                    |   1 +
 arch/arm64/include/uapi/asm/kvm.h                  |   1 +
 virt/kvm/arm/vgic/vgic-its.c                       | 241 ++++++++++++---------
 4 files changed, 163 insertions(+), 100 deletions(-)

-- 
2.5.5

[PATCH v6 1/9] KVM: arm/arm64: vgic-its: Fix return value for device table restore

[Eric Auger]

From: wanghaibin <wanghaibin.wang@huawei.com>

If ITT only contains invalid entries, vgic_its_restore_itt
returns 1 and this is considered as an an error in
vgic_its_restore_dte.

Also in case the device table only contains invalid entries,
the table restore fails and this is not correct.

This patch fixes those 2 issues:
- vgic_its_restore_itt now returns <= 0 values. If all
  ITEs are invalid, this is considered as successful.
- vgic_its_restore_device_tables also returns <= 0 values.

We also simplify the returned value computation in
handle_l1_dte.

Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

---

v5 -> v6:
- added Christoffer's R-b

v2 -> v3:
- add comments
- vgic_its_restore_itt don't return +1 anymore
- reword the commit message

v1 -> v2:
- if (ret > 0) ret = 0
---
 virt/kvm/arm/vgic/vgic-its.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index f51c1e1..d27a301 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1940,6 +1940,14 @@ static int vgic_its_save_itt(struct vgic_its *its, struct its_device *device)
 	return 0;
 }
 
+/**
+ * vgic_its_restore_itt - restore the ITT of a device
+ *
+ * @its: its handle
+ * @dev: device handle
+ *
+ * Return 0 on success, < 0 on error
+ */
 static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
 {
 	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
@@ -1951,6 +1959,10 @@ static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
 	ret = scan_its_table(its, base, max_size, ite_esz, 0,
 			     vgic_its_restore_ite, dev);
 
+	/* scan_its_table returns +1 if all ITEs are invalid */
+	if (ret > 0)
+		ret = 0;
+
 	return ret;
 }
 
@@ -2107,10 +2119,7 @@ static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
 	ret = scan_its_table(its, gpa, SZ_64K, dte_esz,
 			     l2_start_id, vgic_its_restore_dte, NULL);
 
-	if (ret <= 0)
-		return ret;
-
-	return 1;
+	return ret;
 }
 
 /**
@@ -2140,8 +2149,9 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
 				     vgic_its_restore_dte, NULL);
 	}
 
+	/* scan_its_table returns +1 if all entries are invalid */
 	if (ret > 0)
-		ret = -EINVAL;
+		ret = 0;
 
 	return ret;
 }
-- 
2.5.5

[PATCH v6 2/9] KVM: arm/arm64: vgic-its: Fix vgic_its_restore_collection_table returned value

[Eric Auger]

vgic_its_restore_cte returns +1 if the collection table entry
is valid and properly decoded. As a consequence, if the
collection table is fully filled with valid data that are
decoded without error, vgic_its_restore_collection_table()
returns +1. This is wrong.

Let's return 0 in that case.

Fixes: ea1ad53e1e31a3 (KVM: arm64: vgic-its: Collection table save/restore)
Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v5 -> v6:
- use the same trick as vgic_its_restore_itt and
  vgic_its_restore_device_tables

v4 -> v5:
- added Christoffer R-b

v2 -> v3: creation
---
 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 d27a301..8230ffe 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2268,6 +2268,10 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
 		gpa += cte_esz;
 		read += cte_esz;
 	}
+
+	if (ret > 0)
+		return 0;
+
 	return ret;
 }
 
-- 
2.5.5

[PATCH v6 3/9] KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS

[Eric Auger]

The spec says it is UNPREDICTABLE to enable the ITS
if any of the following conditions are true:

- GITS_CBASER.Valid == 0.
- GITS_BASER<n>.Valid == 0, for any GITS_BASER<n> register
  where the Type field indicates Device.
- GITS_BASER<n>.Valid == 0, for any GITS_BASER<n> register
  where the Type field indicates Interrupt Collection and
  GITS_TYPER.HCC == 0.

In that case, let's keep the ITS disabled.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reported-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

---

v5 -> v6:
- added Christoffer's R-b

v4 -> v5:
- check the condition before updating its->enabled and
  fix its->cbaser && GITS_CBASER_VALID

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

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 8230ffe..c4758e6 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1466,6 +1466,16 @@ static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its,
 {
 	mutex_lock(&its->cmd_lock);
 
+	/*
+	 * It is UNPREDICTABLE to enable the ITS if any of the CBASER or
+	 * device/collection BASER are invalid
+	 */
+	if (!its->enabled && (val & GITS_CTLR_ENABLE) &&
+		(!(its->baser_device_table & GITS_BASER_VALID) ||
+		 !(its->baser_coll_table & GITS_BASER_VALID) ||
+		 !(its->cbaser & GITS_CBASER_VALID)))
+		goto out;
+
 	its->enabled = !!(val & GITS_CTLR_ENABLE);
 
 	/*
@@ -1474,6 +1484,7 @@ static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its,
 	 */
 	vgic_its_process_commands(kvm, its);
 
+out:
 	mutex_unlock(&its->cmd_lock);
 }
 
-- 
2.5.5

[PATCH v6 4/9] KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving tables

[Eric Auger]

At the moment we don't properly check the GITS_BASER<n>.Valid
bit before saving the collection and device tables.

On vgic_its_save_collection_table() we use the GITS_BASER gpa
field whereas the Valid bit should be used.

On vgic_its_save_device_tables() there is no check. This can
cause various bugs, among which a subsequent fault when accessing
the table in guest memory.

Let's systematically check the Valid bit before doing anything.

We also uniformize the code between save and restore.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

---
v5 -> v6:
- added Marc's R-b

v2 -> v3:
- slight rewording of the commit message
- added Andre's and Christoffer's R-b
---
 virt/kvm/arm/vgic/vgic-its.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index c4758e6..9ba6f23 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2071,11 +2071,12 @@ static int vgic_its_device_cmp(void *priv, struct list_head *a,
 static int vgic_its_save_device_tables(struct vgic_its *its)
 {
 	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
+	u64 baser = its->baser_device_table;
 	struct its_device *dev;
 	int dte_esz = abi->dte_esz;
-	u64 baser;
 
-	baser = its->baser_device_table;
+	if (!(baser & GITS_BASER_VALID))
+		return 0;
 
 	list_sort(NULL, &its->device_list, vgic_its_device_cmp);
 
@@ -2219,17 +2220,17 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
 static int vgic_its_save_collection_table(struct vgic_its *its)
 {
 	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
+	u64 baser = its->baser_coll_table;
+	gpa_t gpa = BASER_ADDRESS(baser);
 	struct its_collection *collection;
 	u64 val;
-	gpa_t gpa;
 	size_t max_size, filled = 0;
 	int ret, cte_esz = abi->cte_esz;
 
-	gpa = BASER_ADDRESS(its->baser_coll_table);
-	if (!gpa)
+	if (!(baser & GITS_BASER_VALID))
 		return 0;
 
-	max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K;
+	max_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
 
 	list_for_each_entry(collection, &its->collection_list, coll_list) {
 		ret = vgic_its_save_cte(its, collection, gpa, cte_esz);
@@ -2260,17 +2261,18 @@ static int vgic_its_save_collection_table(struct vgic_its *its)
 static int vgic_its_restore_collection_table(struct vgic_its *its)
 {
 	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
+	u64 baser = its->baser_coll_table;
 	int cte_esz = abi->cte_esz;
 	size_t max_size, read = 0;
 	gpa_t gpa;
 	int ret;
 
-	if (!(its->baser_coll_table & GITS_BASER_VALID))
+	if (!(baser & GITS_BASER_VALID))
 		return 0;
 
-	gpa = BASER_ADDRESS(its->baser_coll_table);
+	gpa = BASER_ADDRESS(baser);
 
-	max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K;
+	max_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
 
 	while (read < max_size) {
 		ret = vgic_its_restore_cte(its, gpa, cte_esz);
-- 
2.5.5

[PATCH v6 5/9] KVM: arm/arm64: vgic-its: Remove kvm_its_unmap_device

[Eric Auger]

Let's remove kvm_its_unmap_device and use kvm_its_free_device
as both functions are identical.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

---
v5 -> v6:
- Added Christoffer's A-b

v5: creation
---
 virt/kvm/arm/vgic/vgic-its.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 9ba6f23..d21c960 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -894,7 +894,7 @@ 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)
+static void vgic_its_free_device(struct kvm *kvm, struct its_device *device)
 {
 	struct its_ite *ite, *temp;
 
@@ -957,7 +957,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
 	 * by removing the mapping and re-establishing it.
 	 */
 	if (device)
-		vgic_its_unmap_device(kvm, device);
+		vgic_its_free_device(kvm, device);
 
 	/*
 	 * The spec does not say whether unmapping a not-mapped device
@@ -1623,16 +1623,6 @@ static int vgic_its_create(struct kvm_device *dev, u32 type)
 	return vgic_its_set_abi(its, NR_ITS_ABIS - 1);
 }
 
-static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
-{
-	struct its_ite *ite, *tmp;
-
-	list_for_each_entry_safe(ite, tmp, &dev->itt_head, ite_list)
-		its_free_ite(kvm, ite);
-	list_del(&dev->dev_list);
-	kfree(dev);
-}
-
 static void vgic_its_destroy(struct kvm_device *kvm_dev)
 {
 	struct kvm *kvm = kvm_dev->kvm;
-- 
2.5.5

[PATCH v6 6/9] KVM: arm/arm64: vgic-its: New helper functions to free the caches

[Eric Auger]

From: wanghaibin <wanghaibin.wang@huawei.com>

We create two new functions that free the device and
collection lists. They are currently called by vgic_its_destroy()
and other callers will be added in subsequent patches.

We also remove the check on its->device_list.next.
Lists are initialized in vgic_create_its() and the device
is added to the device list only if this latter succeeds.

vgic_its_destroy is the device destroy ops. This latter is called
by kvm_destroy_devices() which loops on all created devices. So
at this point the list is initialized.

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

---
v5 -> v6:
- take the its lock in vgic_its_destroy() instead of in
  vgic_its_free_device_list and vgic_its_free_collection_list

v4 -> v5:
- add mutex_lock in vgic_its_free_collection_list
- use list_for_each_entry_safe
- reword commit message
---
 virt/kvm/arm/vgic/vgic-its.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index d21c960..5b7be85 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -910,6 +910,24 @@ static void vgic_its_free_device(struct kvm *kvm, struct its_device *device)
 	kfree(device);
 }
 
+/* its lock must be held */
+static void vgic_its_free_device_list(struct kvm *kvm, struct vgic_its *its)
+{
+	struct its_device *cur, *temp;
+
+	list_for_each_entry_safe(cur, temp, &its->device_list, dev_list)
+		vgic_its_free_device(kvm, cur);
+}
+
+/* its lock must be held */
+static void vgic_its_free_collection_list(struct kvm *kvm, struct vgic_its *its)
+{
+	struct its_collection *cur, *temp;
+
+	list_for_each_entry_safe(cur, temp, &its->collection_list, coll_list)
+		vgic_its_free_collection(its, cur->collection_id);
+}
+
 /* Must be called with its_lock mutex held */
 static struct its_device *vgic_its_alloc_device(struct vgic_its *its,
 						u32 device_id, gpa_t itt_addr,
@@ -1627,32 +1645,13 @@ static void vgic_its_destroy(struct kvm_device *kvm_dev)
 {
 	struct kvm *kvm = kvm_dev->kvm;
 	struct vgic_its *its = kvm_dev->private;
-	struct list_head *cur, *temp;
-
-	/*
-	 * We may end up here without the lists ever having been initialized.
-	 * Check this and bail out early to avoid dereferencing a NULL pointer.
-	 */
-	if (!its->device_list.next)
-		return;
 
 	mutex_lock(&its->its_lock);
-	list_for_each_safe(cur, temp, &its->device_list) {
-		struct its_device *dev;
 
-		dev = list_entry(cur, struct its_device, dev_list);
-		vgic_its_free_device(kvm, dev);
-	}
-
-	list_for_each_safe(cur, temp, &its->collection_list) {
-		struct its_collection *coll;
+	vgic_its_free_device_list(kvm, its);
+	vgic_its_free_collection_list(kvm, its);
 
-		coll = list_entry(cur, struct its_collection, coll_list);
-		list_del(cur);
-		kfree(coll);
-	}
 	mutex_unlock(&its->its_lock);
-
 	kfree(its);
 }
 
-- 
2.5.5

[PATCH v6 7/9] KVM: arm/arm64: vgic-its: Free caches when GITS_BASER Valid bit is cleared

[Eric Auger]

When the GITS_BASER<n>.Valid gets cleared, the data structures in
guest RAM are not valid anymore. The device, collection
and LPI lists stored in the in-kernel ITS represent the same
information in some form of cache. So let's void the cache.

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

---
v5 -> v6:
- rename type into device_type and revert tthe u64 -> int change
- remove the default clause
- take the its mutex lock around vgic_its_free_device/collection_list

v4 -> v5:
- add comment about locking

v2 -> v3:
- add a comment and clear cache in if block
---
 virt/kvm/arm/vgic/vgic-its.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 5b7be85..2a92d4d 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1428,7 +1428,7 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
 				      unsigned long val)
 {
 	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
-	u64 entry_size, device_type;
+	u64 entry_size, table_type;
 	u64 reg, *regptr, clearbits = 0;
 
 	/* When GITS_CTLR.Enable is 1, we ignore write accesses. */
@@ -1439,12 +1439,12 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
 	case 0:
 		regptr = &its->baser_device_table;
 		entry_size = abi->dte_esz;
-		device_type = GITS_BASER_TYPE_DEVICE;
+		table_type = GITS_BASER_TYPE_DEVICE;
 		break;
 	case 1:
 		regptr = &its->baser_coll_table;
 		entry_size = abi->cte_esz;
-		device_type = GITS_BASER_TYPE_COLLECTION;
+		table_type = GITS_BASER_TYPE_COLLECTION;
 		clearbits = GITS_BASER_INDIRECT;
 		break;
 	default:
@@ -1456,10 +1456,28 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
 	reg &= ~clearbits;
 
 	reg |= (entry_size - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
-	reg |= device_type << GITS_BASER_TYPE_SHIFT;
+	reg |= table_type << GITS_BASER_TYPE_SHIFT;
 	reg = vgic_sanitise_its_baser(reg);
 
 	*regptr = reg;
+
+	/*
+	 * If the table is no longer valid, we clear the associated cached data.
+	 * Note: there cannot be any race with save/restore code which locks
+	 * all vcpus.
+	 */
+	if (!(reg & GITS_BASER_VALID)) {
+		mutex_lock(&its->its_lock);
+		switch (table_type) {
+		case GITS_BASER_TYPE_DEVICE:
+			vgic_its_free_device_list(kvm, its);
+			break;
+		case GITS_BASER_TYPE_COLLECTION:
+			vgic_its_free_collection_list(kvm, its);
+			break;
+		}
+		mutex_unlock(&its->its_lock);
+	}
 }
 
 static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu,
-- 
2.5.5

[PATCH v6 8/9] KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL_RESET

[Eric Auger]

At the moment, the in-kernel emulated ITS is not properly reset.
On guest restart/reset some registers keep their old values and
internal structures like device, ITE, and collection lists are not
freed.

This may lead to various bugs. Among them, we can have incorrect state
backup or failure when saving the ITS state at early guest boot stage.

This patch documents a new attribute, KVM_DEV_ARM_ITS_CTRL_RESET in
the KVM_DEV_ARM_VGIC_GRP_CTRL group.

Upon this action, we can reset registers and especially those
pointing to tables previously allocated by the guest and free
the internal data structures storing the list of devices, collections
and lpis.

The usual approach for device reset of having userspace write
the reset values of the registers to the kernel via the register
read/write APIs doesn't work for the ITS because it has some
internal state (caches) which is not exposed as registers,
and there is no register interface for "drop cached data without
writing it back to RAM". So we need a KVM API which mimics the
hardware's reset line, to provide the equivalent behaviour to
a "pull the power cord out of the back of the machine" reset.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reported-by: wanghaibin <wanghaibin.wang@huawei.com>

---
v5 -> v6:
- remove "The command queue is not allocated:"

v4 -> v5:
- some rewording according to Christoffer's comments

v2 -> v3:
- reword commit message, credit to Peter Maydell.
- take into account Christoffer rewording comments but still
  kept details. Added Peter's comment but still kept details.
  Peter may disagree.

v1 -> v2:
- Describe architecturally-defined reset values
---
 Documentation/virtual/kvm/devices/arm-vgic-its.txt | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
index eb06beb..8d5830e 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
@@ -33,6 +33,10 @@ Groups:
       request the initialization of the ITS, no additional parameter in
       kvm_device_attr.addr.
 
+    KVM_DEV_ARM_ITS_CTRL_RESET
+      reset the ITS, no additional parameter in kvm_device_attr.addr.
+      See "ITS Reset State" section.
+
     KVM_DEV_ARM_ITS_SAVE_TABLES
       save the ITS table data into guest RAM, at the location provisioned
       by the guest in corresponding registers/table entries.
@@ -157,3 +161,19 @@ Then vcpus can be started.
  - pINTID is the physical LPI ID; if zero, it means the entry is not valid
    and other fields are not meaningful.
  - ICID is the collection ID
+
+ ITS Reset State:
+ ----------------
+
+RESET returns the ITS to the same state that it was when first created and
+initialized. When the RESET command returns, the following things are
+guaranteed:
+
+- The ITS is not enabled and quiescent
+  GITS_CTLR.Enabled = 0 .Quiescent=1
+- There is no internally cached state
+- No collection or device table are used
+  GITS_BASER<n>.Valid = 0
+- GITS_CBASER = 0, GITS_CREADR = 0, GITS_CWRITER = 0
+- The ABI version is unchanged and remains the one set when the ITS
+  device was first created.
-- 
2.5.5

[PATCH v6 9/9] KVM: arm/arm64: vgic-its: Implement KVM_DEV_ARM_ITS_CTRL_RESET

[Eric Auger]

On reset we clear the valid bits of GITS_CBASER and GITS_BASER<n>.
We also clear command queue registers and free the cache (device,
collection, and lpi lists).

As we need to take the same locks as save/restore functions, we
create a vgic_its_ctrl() wrapper that handles KVM_DEV_ARM_VGIC_GRP_CTRL
group functions.

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

---

v5 -> v6:
- Rework the locking and create vgic_its_ctrl

v2 -> v3:
- added Christoffer's R-b
---
 arch/arm/include/uapi/asm/kvm.h   |   1 +
 arch/arm64/include/uapi/asm/kvm.h |   1 +
 virt/kvm/arm/vgic/vgic-its.c      | 105 ++++++++++++++++++++------------------
 3 files changed, 58 insertions(+), 49 deletions(-)

diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index 5db2d4c..7ef0c06 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -215,6 +215,7 @@ struct kvm_arch_memory_slot {
 #define   KVM_DEV_ARM_ITS_SAVE_TABLES		1
 #define   KVM_DEV_ARM_ITS_RESTORE_TABLES	2
 #define   KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES	3
+#define   KVM_DEV_ARM_ITS_CTRL_RESET		4
 
 /* KVM_IRQ_LINE irq field index values */
 #define KVM_ARM_IRQ_TYPE_SHIFT		24
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 9f3ca24..b5306ce 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -227,6 +227,7 @@ struct kvm_arch_memory_slot {
 #define   KVM_DEV_ARM_ITS_SAVE_TABLES           1
 #define   KVM_DEV_ARM_ITS_RESTORE_TABLES        2
 #define   KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES	3
+#define   KVM_DEV_ARM_ITS_CTRL_RESET		4
 
 /* Device Control API on vcpu fd */
 #define KVM_ARM_VCPU_PMU_V3_CTRL	0
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 2a92d4d..09a459b 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2301,29 +2301,13 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
  */
 static int vgic_its_save_tables_v0(struct vgic_its *its)
 {
-	struct kvm *kvm = its->dev->kvm;
 	int ret;
 
-	mutex_lock(&kvm->lock);
-	mutex_lock(&its->its_lock);
-
-	if (!lock_all_vcpus(kvm)) {
-		mutex_unlock(&its->its_lock);
-		mutex_unlock(&kvm->lock);
-		return -EBUSY;
-	}
-
 	ret = vgic_its_save_device_tables(its);
 	if (ret)
-		goto out;
-
-	ret = vgic_its_save_collection_table(its);
+		return ret;
 
-out:
-	unlock_all_vcpus(kvm);
-	mutex_unlock(&its->its_lock);
-	mutex_unlock(&kvm->lock);
-	return ret;
+	return vgic_its_save_collection_table(its);
 }
 
 /**
@@ -2333,29 +2317,13 @@ static int vgic_its_save_tables_v0(struct vgic_its *its)
  */
 static int vgic_its_restore_tables_v0(struct vgic_its *its)
 {
-	struct kvm *kvm = its->dev->kvm;
 	int ret;
 
-	mutex_lock(&kvm->lock);
-	mutex_lock(&its->its_lock);
-
-	if (!lock_all_vcpus(kvm)) {
-		mutex_unlock(&its->its_lock);
-		mutex_unlock(&kvm->lock);
-		return -EBUSY;
-	}
-
 	ret = vgic_its_restore_collection_table(its);
 	if (ret)
-		goto out;
-
-	ret = vgic_its_restore_device_tables(its);
-out:
-	unlock_all_vcpus(kvm);
-	mutex_unlock(&its->its_lock);
-	mutex_unlock(&kvm->lock);
+		return ret;
 
-	return ret;
+	return vgic_its_restore_device_tables(its);
 }
 
 static int vgic_its_commit_v0(struct vgic_its *its)
@@ -2374,6 +2342,19 @@ static int vgic_its_commit_v0(struct vgic_its *its)
 	return 0;
 }
 
+static void vgic_its_reset(struct kvm *kvm, struct vgic_its *its)
+{
+	/* We need to keep the ABI specific field values */
+	its->baser_coll_table &= ~GITS_BASER_VALID;
+	its->baser_device_table &= ~GITS_BASER_VALID;
+	its->cbaser = 0;
+	its->creadr = 0;
+	its->cwriter = 0;
+	its->enabled = 0;
+	vgic_its_free_device_list(kvm, its);
+	vgic_its_free_collection_list(kvm, its);
+}
+
 static int vgic_its_has_attr(struct kvm_device *dev,
 			     struct kvm_device_attr *attr)
 {
@@ -2388,6 +2369,8 @@ static int vgic_its_has_attr(struct kvm_device *dev,
 		switch (attr->attr) {
 		case KVM_DEV_ARM_VGIC_CTRL_INIT:
 			return 0;
+		case KVM_DEV_ARM_ITS_CTRL_RESET:
+			return 0;
 		case KVM_DEV_ARM_ITS_SAVE_TABLES:
 			return 0;
 		case KVM_DEV_ARM_ITS_RESTORE_TABLES:
@@ -2400,6 +2383,41 @@ static int vgic_its_has_attr(struct kvm_device *dev,
 	return -ENXIO;
 }
 
+static int vgic_its_ctrl(struct kvm *kvm, struct vgic_its *its, u64 attr)
+{
+	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
+	int ret = 0;
+
+	if (attr == KVM_DEV_ARM_VGIC_CTRL_INIT) /* Nothing to do */
+		return 0;
+
+	mutex_lock(&kvm->lock);
+	mutex_lock(&its->its_lock);
+
+	if (!lock_all_vcpus(kvm)) {
+		mutex_unlock(&its->its_lock);
+		mutex_unlock(&kvm->lock);
+		return -EBUSY;
+	}
+
+	switch (attr) {
+	case KVM_DEV_ARM_ITS_CTRL_RESET:
+		vgic_its_reset(kvm, its);
+		break;
+	case KVM_DEV_ARM_ITS_SAVE_TABLES:
+		ret = abi->save_tables(its);
+		break;
+	case KVM_DEV_ARM_ITS_RESTORE_TABLES:
+		ret = abi->restore_tables(its);
+		break;
+	}
+
+	unlock_all_vcpus(kvm);
+	mutex_unlock(&its->its_lock);
+	mutex_unlock(&kvm->lock);
+	return ret;
+}
+
 static int vgic_its_set_attr(struct kvm_device *dev,
 			     struct kvm_device_attr *attr)
 {
@@ -2425,19 +2443,8 @@ static int vgic_its_set_attr(struct kvm_device *dev,
 
 		return vgic_register_its_iodev(dev->kvm, its, addr);
 	}
-	case KVM_DEV_ARM_VGIC_GRP_CTRL: {
-		const struct vgic_its_abi *abi = vgic_its_get_abi(its);
-
-		switch (attr->attr) {
-		case KVM_DEV_ARM_VGIC_CTRL_INIT:
-			/* Nothing to do */
-			return 0;
-		case KVM_DEV_ARM_ITS_SAVE_TABLES:
-			return abi->save_tables(its);
-		case KVM_DEV_ARM_ITS_RESTORE_TABLES:
-			return abi->restore_tables(its);
-		}
-	}
+	case KVM_DEV_ARM_VGIC_GRP_CTRL:
+		return vgic_its_ctrl(dev->kvm, its, attr->attr);
 	case KVM_DEV_ARM_VGIC_GRP_ITS_REGS: {
 		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
 		u64 reg;
-- 
2.5.5

Re: [PATCH v6 5/9] KVM: arm/arm64: vgic-its: Remove kvm_its_unmap_device

[Marc Zyngier]

On Thu, Oct 26 2017 at  6:23:07 pm BST, Eric Auger <eric.auger@redhat.com> wrote:
> Let's remove kvm_its_unmap_device and use kvm_its_free_device
> as both functions are identical.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

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

Re: [PATCH v6 7/9] KVM: arm/arm64: vgic-its: Free caches when GITS_BASER Valid bit is cleared

[Marc Zyngier]

On Thu, Oct 26 2017 at  6:23:09 pm BST, Eric Auger <eric.auger@redhat.com> wrote:
> When the GITS_BASER<n>.Valid gets cleared, the data structures in
> guest RAM are not valid anymore. The device, collection
> and LPI lists stored in the in-kernel ITS represent the same
> information in some form of cache. So let's void the cache.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
> v5 -> v6:
> - rename type into device_type and revert tthe u64 -> int change
> - remove the default clause
> - take the its mutex lock around vgic_its_free_device/collection_list
>
> v4 -> v5:
> - add comment about locking
>
> v2 -> v3:
> - add a comment and clear cache in if block
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 5b7be85..2a92d4d 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1428,7 +1428,7 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>  				      unsigned long val)
>  {
>  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> -	u64 entry_size, device_type;
> +	u64 entry_size, table_type;
>  	u64 reg, *regptr, clearbits = 0;
>  
>  	/* When GITS_CTLR.Enable is 1, we ignore write accesses. */
> @@ -1439,12 +1439,12 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>  	case 0:
>  		regptr = &its->baser_device_table;
>  		entry_size = abi->dte_esz;
> -		device_type = GITS_BASER_TYPE_DEVICE;
> +		table_type = GITS_BASER_TYPE_DEVICE;
>  		break;
>  	case 1:
>  		regptr = &its->baser_coll_table;
>  		entry_size = abi->cte_esz;
> -		device_type = GITS_BASER_TYPE_COLLECTION;
> +		table_type = GITS_BASER_TYPE_COLLECTION;
>  		clearbits = GITS_BASER_INDIRECT;
>  		break;
>  	default:
> @@ -1456,10 +1456,28 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
>  	reg &= ~clearbits;
>  
>  	reg |= (entry_size - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
> -	reg |= device_type << GITS_BASER_TYPE_SHIFT;
> +	reg |= table_type << GITS_BASER_TYPE_SHIFT;
>  	reg = vgic_sanitise_its_baser(reg);
>  
>  	*regptr = reg;
> +
> +	/*
> +	 * If the table is no longer valid, we clear the associated cached data.
> +	 * Note: there cannot be any race with save/restore code which locks
> +	 * all vcpus.
> +	 */

nit: I found this comment to be pretty confusing, as it talks about
locks that we don't try to take here. The actual mutual exclusion is
done by taking the its_lock, which is also taken on the save/restore
path.

Christoffer: can you fix that when applying this patch? I don't think
there is a need for a respin of the series just for this.

> +	if (!(reg & GITS_BASER_VALID)) {
> +		mutex_lock(&its->its_lock);
> +		switch (table_type) {
> +		case GITS_BASER_TYPE_DEVICE:
> +			vgic_its_free_device_list(kvm, its);
> +			break;
> +		case GITS_BASER_TYPE_COLLECTION:
> +			vgic_its_free_collection_list(kvm, its);
> +			break;
> +		}
> +		mutex_unlock(&its->its_lock);
> +	}
>  }
>  
>  static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu,

Otherwise:

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

Thanks,

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

Re: [PATCH v6 6/9] KVM: arm/arm64: vgic-its: New helper functions to free the caches

[Marc Zyngier]

On Thu, Oct 26 2017 at  6:23:08 pm BST, Eric Auger <eric.auger@redhat.com> wrote:
> From: wanghaibin <wanghaibin.wang@huawei.com>
>
> We create two new functions that free the device and
> collection lists. They are currently called by vgic_its_destroy()
> and other callers will be added in subsequent patches.
>
> We also remove the check on its->device_list.next.
> Lists are initialized in vgic_create_its() and the device
> is added to the device list only if this latter succeeds.
>
> vgic_its_destroy is the device destroy ops. This latter is called
> by kvm_destroy_devices() which loops on all created devices. So
> at this point the list is initialized.
>
> Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

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

Re: [PATCH v6 8/9] KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL_RESET

[Marc Zyngier]

On Thu, Oct 26 2017 at  6:23:10 pm BST, Eric Auger <eric.auger@redhat.com> wrote:
> At the moment, the in-kernel emulated ITS is not properly reset.
> On guest restart/reset some registers keep their old values and
> internal structures like device, ITE, and collection lists are not
> freed.
>
> This may lead to various bugs. Among them, we can have incorrect state
> backup or failure when saving the ITS state at early guest boot stage.
>
> This patch documents a new attribute, KVM_DEV_ARM_ITS_CTRL_RESET in
> the KVM_DEV_ARM_VGIC_GRP_CTRL group.
>
> Upon this action, we can reset registers and especially those
> pointing to tables previously allocated by the guest and free
> the internal data structures storing the list of devices, collections
> and lpis.
>
> The usual approach for device reset of having userspace write
> the reset values of the registers to the kernel via the register
> read/write APIs doesn't work for the ITS because it has some
> internal state (caches) which is not exposed as registers,
> and there is no register interface for "drop cached data without
> writing it back to RAM". So we need a KVM API which mimics the
> hardware's reset line, to provide the equivalent behaviour to
> a "pull the power cord out of the back of the machine" reset.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

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

Re: [PATCH v6 9/9] KVM: arm/arm64: vgic-its: Implement KVM_DEV_ARM_ITS_CTRL_RESET

[Marc Zyngier]

On Thu, Oct 26 2017 at  6:23:11 pm BST, Eric Auger <eric.auger@redhat.com> wrote:
> On reset we clear the valid bits of GITS_CBASER and GITS_BASER<n>.
> We also clear command queue registers and free the cache (device,
> collection, and lpi lists).
>
> As we need to take the same locks as save/restore functions, we
> create a vgic_its_ctrl() wrapper that handles KVM_DEV_ARM_VGIC_GRP_CTRL
> group functions.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>

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

Re: [PATCH v6 7/9] KVM: arm/arm64: vgic-its: Free caches when GITS_BASER Valid bit is cleared

[Christoffer Dall]

On Mon, Oct 30, 2017 at 03:19:54AM +0000, Marc Zyngier wrote:
> On Thu, Oct 26 2017 at  6:23:09 pm BST, Eric Auger <eric.auger@redhat.com> wrote:
> > When the GITS_BASER<n>.Valid gets cleared, the data structures in
> > guest RAM are not valid anymore. The device, collection
> > and LPI lists stored in the in-kernel ITS represent the same
> > information in some form of cache. So let's void the cache.
> >
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >
> > ---
> > v5 -> v6:
> > - rename type into device_type and revert tthe u64 -> int change
> > - remove the default clause
> > - take the its mutex lock around vgic_its_free_device/collection_list
> >
> > v4 -> v5:
> > - add comment about locking
> >
> > v2 -> v3:
> > - add a comment and clear cache in if block
> > ---
> >  virt/kvm/arm/vgic/vgic-its.c | 26 ++++++++++++++++++++++----
> >  1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> > index 5b7be85..2a92d4d 100644
> > --- a/virt/kvm/arm/vgic/vgic-its.c
> > +++ b/virt/kvm/arm/vgic/vgic-its.c
> > @@ -1428,7 +1428,7 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
> >  				      unsigned long val)
> >  {
> >  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> > -	u64 entry_size, device_type;
> > +	u64 entry_size, table_type;
> >  	u64 reg, *regptr, clearbits = 0;
> >  
> >  	/* When GITS_CTLR.Enable is 1, we ignore write accesses. */
> > @@ -1439,12 +1439,12 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
> >  	case 0:
> >  		regptr = &its->baser_device_table;
> >  		entry_size = abi->dte_esz;
> > -		device_type = GITS_BASER_TYPE_DEVICE;
> > +		table_type = GITS_BASER_TYPE_DEVICE;
> >  		break;
> >  	case 1:
> >  		regptr = &its->baser_coll_table;
> >  		entry_size = abi->cte_esz;
> > -		device_type = GITS_BASER_TYPE_COLLECTION;
> > +		table_type = GITS_BASER_TYPE_COLLECTION;
> >  		clearbits = GITS_BASER_INDIRECT;
> >  		break;
> >  	default:
> > @@ -1456,10 +1456,28 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
> >  	reg &= ~clearbits;
> >  
> >  	reg |= (entry_size - 1) << GITS_BASER_ENTRY_SIZE_SHIFT;
> > -	reg |= device_type << GITS_BASER_TYPE_SHIFT;
> > +	reg |= table_type << GITS_BASER_TYPE_SHIFT;
> >  	reg = vgic_sanitise_its_baser(reg);
> >  
> >  	*regptr = reg;
> > +
> > +	/*
> > +	 * If the table is no longer valid, we clear the associated cached data.
> > +	 * Note: there cannot be any race with save/restore code which locks
> > +	 * all vcpus.
> > +	 */
> 
> nit: I found this comment to be pretty confusing, as it talks about
> locks that we don't try to take here. The actual mutual exclusion is
> done by taking the its_lock, which is also taken on the save/restore
> path.
> 
> Christoffer: can you fix that when applying this patch? I don't think
> there is a need for a respin of the series just for this.
> 

Yes, no problem.

> > +	if (!(reg & GITS_BASER_VALID)) {
> > +		mutex_lock(&its->its_lock);
> > +		switch (table_type) {
> > +		case GITS_BASER_TYPE_DEVICE:
> > +			vgic_its_free_device_list(kvm, its);
> > +			break;
> > +		case GITS_BASER_TYPE_COLLECTION:
> > +			vgic_its_free_collection_list(kvm, its);
> > +			break;
> > +		}
> > +		mutex_unlock(&its->its_lock);
> > +	}
> >  }
> >  
> >  static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu,
> 
> Otherwise:
> 
> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
> 

Also,

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

Re: [PATCH v6 8/9] KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL_RESET

[Christoffer Dall]

On Thu, Oct 26, 2017 at 05:23:10PM +0200, Eric Auger wrote:
> At the moment, the in-kernel emulated ITS is not properly reset.
> On guest restart/reset some registers keep their old values and
> internal structures like device, ITE, and collection lists are not
> freed.
> 
> This may lead to various bugs. Among them, we can have incorrect state
> backup or failure when saving the ITS state at early guest boot stage.
> 
> This patch documents a new attribute, KVM_DEV_ARM_ITS_CTRL_RESET in
> the KVM_DEV_ARM_VGIC_GRP_CTRL group.
> 
> Upon this action, we can reset registers and especially those
> pointing to tables previously allocated by the guest and free
> the internal data structures storing the list of devices, collections
> and lpis.
> 
> The usual approach for device reset of having userspace write
> the reset values of the registers to the kernel via the register
> read/write APIs doesn't work for the ITS because it has some
> internal state (caches) which is not exposed as registers,
> and there is no register interface for "drop cached data without
> writing it back to RAM". So we need a KVM API which mimics the
> hardware's reset line, to provide the equivalent behaviour to
> a "pull the power cord out of the back of the machine" reset.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>

Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

> 
> ---
> v5 -> v6:
> - remove "The command queue is not allocated:"
> 
> v4 -> v5:
> - some rewording according to Christoffer's comments
> 
> v2 -> v3:
> - reword commit message, credit to Peter Maydell.
> - take into account Christoffer rewording comments but still
>   kept details. Added Peter's comment but still kept details.
>   Peter may disagree.
> 
> v1 -> v2:
> - Describe architecturally-defined reset values
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> index eb06beb..8d5830e 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> @@ -33,6 +33,10 @@ Groups:
>        request the initialization of the ITS, no additional parameter in
>        kvm_device_attr.addr.
>  
> +    KVM_DEV_ARM_ITS_CTRL_RESET
> +      reset the ITS, no additional parameter in kvm_device_attr.addr.
> +      See "ITS Reset State" section.
> +
>      KVM_DEV_ARM_ITS_SAVE_TABLES
>        save the ITS table data into guest RAM, at the location provisioned
>        by the guest in corresponding registers/table entries.
> @@ -157,3 +161,19 @@ Then vcpus can be started.
>   - pINTID is the physical LPI ID; if zero, it means the entry is not valid
>     and other fields are not meaningful.
>   - ICID is the collection ID
> +
> + ITS Reset State:
> + ----------------
> +
> +RESET returns the ITS to the same state that it was when first created and
> +initialized. When the RESET command returns, the following things are
> +guaranteed:
> +
> +- The ITS is not enabled and quiescent
> +  GITS_CTLR.Enabled = 0 .Quiescent=1
> +- There is no internally cached state
> +- No collection or device table are used
> +  GITS_BASER<n>.Valid = 0
> +- GITS_CBASER = 0, GITS_CREADR = 0, GITS_CWRITER = 0
> +- The ABI version is unchanged and remains the one set when the ITS
> +  device was first created.
> -- 
> 2.5.5
> 

Re: [PATCH v6 9/9] KVM: arm/arm64: vgic-its: Implement KVM_DEV_ARM_ITS_CTRL_RESET

[Christoffer Dall]

On Thu, Oct 26, 2017 at 05:23:11PM +0200, Eric Auger wrote:
> On reset we clear the valid bits of GITS_CBASER and GITS_BASER<n>.
> We also clear command queue registers and free the cache (device,
> collection, and lpi lists).
> 
> As we need to take the same locks as save/restore functions, we
> create a vgic_its_ctrl() wrapper that handles KVM_DEV_ARM_VGIC_GRP_CTRL
> group functions.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

> ---
> 
> v5 -> v6:
> - Rework the locking and create vgic_its_ctrl
> 
> v2 -> v3:
> - added Christoffer's R-b
> ---
>  arch/arm/include/uapi/asm/kvm.h   |   1 +
>  arch/arm64/include/uapi/asm/kvm.h |   1 +
>  virt/kvm/arm/vgic/vgic-its.c      | 105 ++++++++++++++++++++------------------
>  3 files changed, 58 insertions(+), 49 deletions(-)
> 
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index 5db2d4c..7ef0c06 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -215,6 +215,7 @@ struct kvm_arch_memory_slot {
>  #define   KVM_DEV_ARM_ITS_SAVE_TABLES		1
>  #define   KVM_DEV_ARM_ITS_RESTORE_TABLES	2
>  #define   KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES	3
> +#define   KVM_DEV_ARM_ITS_CTRL_RESET		4
>  
>  /* KVM_IRQ_LINE irq field index values */
>  #define KVM_ARM_IRQ_TYPE_SHIFT		24
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 9f3ca24..b5306ce 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -227,6 +227,7 @@ struct kvm_arch_memory_slot {
>  #define   KVM_DEV_ARM_ITS_SAVE_TABLES           1
>  #define   KVM_DEV_ARM_ITS_RESTORE_TABLES        2
>  #define   KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES	3
> +#define   KVM_DEV_ARM_ITS_CTRL_RESET		4
>  
>  /* Device Control API on vcpu fd */
>  #define KVM_ARM_VCPU_PMU_V3_CTRL	0
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 2a92d4d..09a459b 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2301,29 +2301,13 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
>   */
>  static int vgic_its_save_tables_v0(struct vgic_its *its)
>  {
> -	struct kvm *kvm = its->dev->kvm;
>  	int ret;
>  
> -	mutex_lock(&kvm->lock);
> -	mutex_lock(&its->its_lock);
> -
> -	if (!lock_all_vcpus(kvm)) {
> -		mutex_unlock(&its->its_lock);
> -		mutex_unlock(&kvm->lock);
> -		return -EBUSY;
> -	}
> -
>  	ret = vgic_its_save_device_tables(its);
>  	if (ret)
> -		goto out;
> -
> -	ret = vgic_its_save_collection_table(its);
> +		return ret;
>  
> -out:
> -	unlock_all_vcpus(kvm);
> -	mutex_unlock(&its->its_lock);
> -	mutex_unlock(&kvm->lock);
> -	return ret;
> +	return vgic_its_save_collection_table(its);
>  }
>  
>  /**
> @@ -2333,29 +2317,13 @@ static int vgic_its_save_tables_v0(struct vgic_its *its)
>   */
>  static int vgic_its_restore_tables_v0(struct vgic_its *its)
>  {
> -	struct kvm *kvm = its->dev->kvm;
>  	int ret;
>  
> -	mutex_lock(&kvm->lock);
> -	mutex_lock(&its->its_lock);
> -
> -	if (!lock_all_vcpus(kvm)) {
> -		mutex_unlock(&its->its_lock);
> -		mutex_unlock(&kvm->lock);
> -		return -EBUSY;
> -	}
> -
>  	ret = vgic_its_restore_collection_table(its);
>  	if (ret)
> -		goto out;
> -
> -	ret = vgic_its_restore_device_tables(its);
> -out:
> -	unlock_all_vcpus(kvm);
> -	mutex_unlock(&its->its_lock);
> -	mutex_unlock(&kvm->lock);
> +		return ret;
>  
> -	return ret;
> +	return vgic_its_restore_device_tables(its);
>  }
>  
>  static int vgic_its_commit_v0(struct vgic_its *its)
> @@ -2374,6 +2342,19 @@ static int vgic_its_commit_v0(struct vgic_its *its)
>  	return 0;
>  }
>  
> +static void vgic_its_reset(struct kvm *kvm, struct vgic_its *its)
> +{
> +	/* We need to keep the ABI specific field values */
> +	its->baser_coll_table &= ~GITS_BASER_VALID;
> +	its->baser_device_table &= ~GITS_BASER_VALID;
> +	its->cbaser = 0;
> +	its->creadr = 0;
> +	its->cwriter = 0;
> +	its->enabled = 0;
> +	vgic_its_free_device_list(kvm, its);
> +	vgic_its_free_collection_list(kvm, its);
> +}
> +
>  static int vgic_its_has_attr(struct kvm_device *dev,
>  			     struct kvm_device_attr *attr)
>  {
> @@ -2388,6 +2369,8 @@ static int vgic_its_has_attr(struct kvm_device *dev,
>  		switch (attr->attr) {
>  		case KVM_DEV_ARM_VGIC_CTRL_INIT:
>  			return 0;
> +		case KVM_DEV_ARM_ITS_CTRL_RESET:
> +			return 0;
>  		case KVM_DEV_ARM_ITS_SAVE_TABLES:
>  			return 0;
>  		case KVM_DEV_ARM_ITS_RESTORE_TABLES:
> @@ -2400,6 +2383,41 @@ static int vgic_its_has_attr(struct kvm_device *dev,
>  	return -ENXIO;
>  }
>  
> +static int vgic_its_ctrl(struct kvm *kvm, struct vgic_its *its, u64 attr)
> +{
> +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> +	int ret = 0;
> +
> +	if (attr == KVM_DEV_ARM_VGIC_CTRL_INIT) /* Nothing to do */
> +		return 0;
> +
> +	mutex_lock(&kvm->lock);
> +	mutex_lock(&its->its_lock);
> +
> +	if (!lock_all_vcpus(kvm)) {
> +		mutex_unlock(&its->its_lock);
> +		mutex_unlock(&kvm->lock);
> +		return -EBUSY;
> +	}
> +
> +	switch (attr) {
> +	case KVM_DEV_ARM_ITS_CTRL_RESET:
> +		vgic_its_reset(kvm, its);
> +		break;
> +	case KVM_DEV_ARM_ITS_SAVE_TABLES:
> +		ret = abi->save_tables(its);
> +		break;
> +	case KVM_DEV_ARM_ITS_RESTORE_TABLES:
> +		ret = abi->restore_tables(its);
> +		break;
> +	}
> +
> +	unlock_all_vcpus(kvm);
> +	mutex_unlock(&its->its_lock);
> +	mutex_unlock(&kvm->lock);
> +	return ret;
> +}
> +
>  static int vgic_its_set_attr(struct kvm_device *dev,
>  			     struct kvm_device_attr *attr)
>  {
> @@ -2425,19 +2443,8 @@ static int vgic_its_set_attr(struct kvm_device *dev,
>  
>  		return vgic_register_its_iodev(dev->kvm, its, addr);
>  	}
> -	case KVM_DEV_ARM_VGIC_GRP_CTRL: {
> -		const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> -
> -		switch (attr->attr) {
> -		case KVM_DEV_ARM_VGIC_CTRL_INIT:
> -			/* Nothing to do */
> -			return 0;
> -		case KVM_DEV_ARM_ITS_SAVE_TABLES:
> -			return abi->save_tables(its);
> -		case KVM_DEV_ARM_ITS_RESTORE_TABLES:
> -			return abi->restore_tables(its);
> -		}
> -	}
> +	case KVM_DEV_ARM_VGIC_GRP_CTRL:
> +		return vgic_its_ctrl(dev->kvm, its, attr->attr);
>  	case KVM_DEV_ARM_VGIC_GRP_ITS_REGS: {
>  		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
>  		u64 reg;
> -- 
> 2.5.5
> 

Re: [PATCH v6 0/9] vITS Migration fixes and reset

[Christoffer Dall]

Hi Eric,

On Thu, Oct 26, 2017 at 05:23:02PM +0200, Eric Auger wrote:
> This series fixes various bugs observed when saving/restoring the
> ITS state before the guest writes the ITS registers (on first boot or
> after reset/reboot).
> 
> This is a follow up of Wanghaibin's series [1] plus additional
> patches following additional code review. It also proposes one
> ITS reset implementation.
> 
> Currently, the in-kernel emulated ITS is not reset. After a
> reset/reboot, the ITS register values and caches are left
> unchanged. Registers may point to some tables in guest memory
> which do not exist anymore. If an ITS state backup is initiated
> before the guest re-writes the registers, the save fails
> because inconsistencies are detected. Also restore of data saved
> as such moment is failing.
> 
> Patches [1-4] are fixes of bugs observed during migration at
> early guets boot stage.
> - handle case where all collection, device and ITT entries are
>   invalid on restore (which is not an error)
> - Check the GITS_BASER<n> valid bit before attempting the save
>   any table
> - Check the GITS_BASER<n> and GITS_CBASER are valid before enabling
>   the ITS
> 
> Patches [5-9] allow to empty the caches on reset and implement a
> new ITS reset IOCTL

I applied patches 1-4 to kvmarm/master and included them in a late pull
request to kvm.

I also took the remaining patches with the adjusted comment in
kvmarm/next.

One question:  Don't we have a remaining issue to support saving the
collection table even if the device table is inconsistent and vice
versa?  Are you planning on picking up that work?

Thanks,
-Christoffer

Re: [PATCH v6 0/9] vITS Migration fixes and reset

[Auger Eric]

Hi Christoffer,

On 30/10/2017 07:20, Christoffer Dall wrote:
> Hi Eric,
> 
> On Thu, Oct 26, 2017 at 05:23:02PM +0200, Eric Auger wrote:
>> This series fixes various bugs observed when saving/restoring the
>> ITS state before the guest writes the ITS registers (on first boot or
>> after reset/reboot).
>>
>> This is a follow up of Wanghaibin's series [1] plus additional
>> patches following additional code review. It also proposes one
>> ITS reset implementation.
>>
>> Currently, the in-kernel emulated ITS is not reset. After a
>> reset/reboot, the ITS register values and caches are left
>> unchanged. Registers may point to some tables in guest memory
>> which do not exist anymore. If an ITS state backup is initiated
>> before the guest re-writes the registers, the save fails
>> because inconsistencies are detected. Also restore of data saved
>> as such moment is failing.
>>
>> Patches [1-4] are fixes of bugs observed during migration at
>> early guets boot stage.
>> - handle case where all collection, device and ITT entries are
>>   invalid on restore (which is not an error)
>> - Check the GITS_BASER<n> valid bit before attempting the save
>>   any table
>> - Check the GITS_BASER<n> and GITS_CBASER are valid before enabling
>>   the ITS
>>
>> Patches [5-9] allow to empty the caches on reset and implement a
>> new ITS reset IOCTL
> 
> I applied patches 1-4 to kvmarm/master and included them in a late pull
> request to kvm.
> 
> I also took the remaining patches with the adjusted comment in
> kvmarm/next.

OK Thanks.
> 
> One question:  Don't we have a remaining issue to support saving the
> collection table even if the device table is inconsistent and vice
> versa?  Are you planning on picking up that work?

Actually to make it clear, patches 1-4 don't fix all failures but we
discussed at KVM forum that we shouldn't try to fix all of them without
a proper ITS reset implementation. So indeed even with patches 1-4 you
can get the migration failing as the save can happen after a reset,
in-between the collection creation and the PCIe device MSI registration.
This happens because the caches are not void and the L1 device table
entries are not valid. In that case the device table save fails and we
get no chance to save the collection as we abort the save immediately.
So on restore, guest will not work properly.

But on top of kvmarm/next, caches are void and we shouldn't face this
issue anymore. So in case the device table save fails, I think it still
makes sense to return an error.

But this means that the migration of ITS at early guest boot stage only
is fixed on kvmarm/next.

Thanks

Eric
> 
> Thanks,
> -Christoffer
> 

Re: [PATCH v6 0/9] vITS Migration fixes and reset

[Christoffer Dall]

On Mon, Oct 30, 2017 at 08:59:36AM +0100, Auger Eric wrote:
> Hi Christoffer,
> 
> On 30/10/2017 07:20, Christoffer Dall wrote:
> > Hi Eric,
> > 
> > On Thu, Oct 26, 2017 at 05:23:02PM +0200, Eric Auger wrote:
> >> This series fixes various bugs observed when saving/restoring the
> >> ITS state before the guest writes the ITS registers (on first boot or
> >> after reset/reboot).
> >>
> >> This is a follow up of Wanghaibin's series [1] plus additional
> >> patches following additional code review. It also proposes one
> >> ITS reset implementation.
> >>
> >> Currently, the in-kernel emulated ITS is not reset. After a
> >> reset/reboot, the ITS register values and caches are left
> >> unchanged. Registers may point to some tables in guest memory
> >> which do not exist anymore. If an ITS state backup is initiated
> >> before the guest re-writes the registers, the save fails
> >> because inconsistencies are detected. Also restore of data saved
> >> as such moment is failing.
> >>
> >> Patches [1-4] are fixes of bugs observed during migration at
> >> early guets boot stage.
> >> - handle case where all collection, device and ITT entries are
> >>   invalid on restore (which is not an error)
> >> - Check the GITS_BASER<n> valid bit before attempting the save
> >>   any table
> >> - Check the GITS_BASER<n> and GITS_CBASER are valid before enabling
> >>   the ITS
> >>
> >> Patches [5-9] allow to empty the caches on reset and implement a
> >> new ITS reset IOCTL
> > 
> > I applied patches 1-4 to kvmarm/master and included them in a late pull
> > request to kvm.
> > 
> > I also took the remaining patches with the adjusted comment in
> > kvmarm/next.
> 
> OK Thanks.
> > 
> > One question:  Don't we have a remaining issue to support saving the
> > collection table even if the device table is inconsistent and vice
> > versa?  Are you planning on picking up that work?
> 
> Actually to make it clear, patches 1-4 don't fix all failures but we
> discussed at KVM forum that we shouldn't try to fix all of them without
> a proper ITS reset implementation. So indeed even with patches 1-4 you
> can get the migration failing as the save can happen after a reset,
> in-between the collection creation and the PCIe device MSI registration.
> This happens because the caches are not void and the L1 device table
> entries are not valid. In that case the device table save fails and we
> get no chance to save the collection as we abort the save immediately.
> So on restore, guest will not work properly.
> 
> But on top of kvmarm/next, caches are void and we shouldn't face this
> issue anymore. So in case the device table save fails, I think it still
> makes sense to return an error.
> 
> But this means that the migration of ITS at early guest boot stage only
> is fixed on kvmarm/next.
> 

I think you're right.

Thanks,
-Christoffer

Re: [PATCH v6 2/9] KVM: arm/arm64: vgic-its: Fix vgic_its_restore_collection_table returned value

[Christoffer Dall]

On Thu, Oct 26, 2017 at 05:23:04PM +0200, Eric Auger wrote:
> vgic_its_restore_cte returns +1 if the collection table entry
> is valid and properly decoded. As a consequence, if the
> collection table is fully filled with valid data that are
> decoded without error, vgic_its_restore_collection_table()
> returns +1. This is wrong.
>
> Let's return 0 in that case.
>

Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

> Fixes: ea1ad53e1e31a3 (KVM: arm64: vgic-its: Collection table save/restore)
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> v5 -> v6:
> - use the same trick as vgic_its_restore_itt and
>   vgic_its_restore_device_tables
>
> v4 -> v5:
> - added Christoffer R-b
>
> v2 -> v3: creation
> ---
>  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 d27a301..8230ffe 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2268,6 +2268,10 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
>   gpa += cte_esz;
>   read += cte_esz;
>   }
> +
> + if (ret > 0)
> + return 0;
> +
>   return ret;
>  }
>
> --
> 2.5.5
>