[PATCH v5 00/10] vITS Migration fixes and reset

[PATCH v5 00/10] 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-5] attempt to fix the migration issues without
implementing the reset IOCTL.
They are candidate for stable:
- 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
- save the collection table before device tables

Patches [6-10] 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-v5

* 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:
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 (8):
  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: Save the collection table before device
    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 |  21 +++
 arch/arm/include/uapi/asm/kvm.h                    |   1 +
 arch/arm64/include/uapi/asm/kvm.h                  |   1 +
 virt/kvm/arm/vgic/vgic-its.c                       | 205 ++++++++++++++-------
 4 files changed, 162 insertions(+), 66 deletions(-)

-- 
2.5.5

[PATCH v5 01/10] 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>

---

need to CC'ed stable

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 v5 02/10] 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 use the standard C convention for both vgic_its_restore_cte
and vgic_its_restore_collection_table. vgic_its_restore_cte now
returns whether we have reached the end of the table in the @last
output parameter. vgic_its_restore_collection_table aborts in case
of error or if the end is found. Otherwise, it now returns 0.

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

---

need to be CC'ed stable

v4 -> v5:
- added Christoffer R-b

v2 -> v3: creation
---
 virt/kvm/arm/vgic/vgic-its.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index d27a301..b0ba80f 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2169,7 +2169,19 @@ static int vgic_its_save_cte(struct vgic_its *its,
 	return kvm_write_guest(its->dev->kvm, gpa, &val, esz);
 }
 
-static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
+/**
+ * vgic_its_restore_cte - Restore a collection table entry
+ *
+ * @its: its handle
+ * @gpa: guest physical address of the entry
+ * @esz: entry size
+ * @last: output boolean indicating whether we have reached the
+ *       end of the collection table (ie. an invalid entry was decoded)
+ *
+ * Return: 0 upon success, < 0 on error
+ */
+static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz,
+				bool *last)
 {
 	struct its_collection *collection;
 	struct kvm *kvm = its->dev->kvm;
@@ -2182,7 +2194,8 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
 	if (ret)
 		return ret;
 	val = le64_to_cpu(val);
-	if (!(val & KVM_ITS_CTE_VALID_MASK))
+	*last = !(val & KVM_ITS_CTE_VALID_MASK);
+	if (*last)
 		return 0;
 
 	target_addr = (u32)(val >> KVM_ITS_CTE_RDBASE_SHIFT);
@@ -2198,7 +2211,7 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
 	if (ret)
 		return ret;
 	collection->target_addr = target_addr;
-	return 1;
+	return 0;
 }
 
 /**
@@ -2243,8 +2256,13 @@ static int vgic_its_save_collection_table(struct vgic_its *its)
 
 /**
  * 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.
+ * in guest memory and restores the ITS collection cache.
+ *
+ * @its: its handle
+ *
+ * Requires the Collection BASER to be previously restored.
+ *
+ * Returns 0 on success or < 0 on error
  */
 static int vgic_its_restore_collection_table(struct vgic_its *its)
 {
@@ -2262,13 +2280,17 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
 	max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K;
 
 	while (read < max_size) {
-		ret = vgic_its_restore_cte(its, gpa, cte_esz);
-		if (ret <= 0)
-			break;
+		bool last;
+
+		ret = vgic_its_restore_cte(its, gpa, cte_esz, &last);
+		if (ret < 0 || last)
+			return ret;
 		gpa += cte_esz;
 		read += cte_esz;
 	}
-	return ret;
+
+	/* table was fully filled with valid entries, decoded without error */
+	return 0;
 }
 
 /**
-- 
2.5.5

[PATCH v5 03/10] 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>

---

need to be CC'ed stable

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 b0ba80f..1eb355e 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 v5 04/10] 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>

---

need to be CC'ed stable

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 1eb355e..b6650c2 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);
 
@@ -2232,17 +2233,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);
@@ -2278,17 +2279,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) {
 		bool last;
-- 
2.5.5

[PATCH v5 05/10] KVM: arm/arm64: vgic-its: Save the collection table before device tables

[Eric Auger]

Currently the ITS caches are not emptied on reset.

After a reset, in case we attempt to save the state before
the bound devices have registered their MSIs and after the
1st level table has been allocated by the ITS driver
(device BASER is valid), the first level entries are still
invalid. If the device cache is not empty (devices registered
before the reset), vgic_its_save_device_tables fails.

This failure has no consequence as those devices do not
deserve to be saved: they correspond to the state before
the reset.

However the ITS driver already sent MAPC for collections
and those need to be saved. With the current code, they
will not and the restored guest will not work properly.

So this patch saves collection tables  before device tables.

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

---

candidate to be CC'ed stable
---
 virt/kvm/arm/vgic/vgic-its.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index b6650c2..8472417 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2324,11 +2324,11 @@ static int vgic_its_save_tables_v0(struct vgic_its *its)
 		return -EBUSY;
 	}
 
-	ret = vgic_its_save_device_tables(its);
+	ret = vgic_its_save_collection_table(its);
 	if (ret)
 		goto out;
 
-	ret = vgic_its_save_collection_table(its);
+	ret = vgic_its_save_device_tables(its);
 
 out:
 	unlock_all_vcpus(kvm);
-- 
2.5.5

[PATCH v5 06/10] 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>

---

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 8472417..2f3c3a1 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 v5 07/10] 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>

---

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 | 51 ++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 2f3c3a1..8098f91 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -910,6 +910,30 @@ static void vgic_its_free_device(struct kvm *kvm, struct its_device *device)
 	kfree(device);
 }
 
+static void vgic_its_free_device_list(struct kvm *kvm, struct vgic_its *its)
+{
+	struct its_device *cur, *temp;
+
+	mutex_lock(&its->its_lock);
+
+	list_for_each_entry_safe(cur, temp, &its->device_list, dev_list)
+		vgic_its_free_device(kvm, cur);
+
+	mutex_unlock(&its->its_lock);
+}
+
+static void vgic_its_free_collection_list(struct kvm *kvm, struct vgic_its *its)
+{
+	struct its_collection *cur, *temp;
+
+	mutex_lock(&its->its_lock);
+
+	list_for_each_entry_safe(cur, temp, &its->collection_list, coll_list)
+		vgic_its_free_collection(its, cur->collection_id);
+
+	mutex_unlock(&its->its_lock);
+}
+
 /* 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 +1651,9 @@ 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;
-
-		coll = list_entry(cur, struct its_collection, coll_list);
-		list_del(cur);
-		kfree(coll);
-	}
-	mutex_unlock(&its->its_lock);
 
+	vgic_its_free_device_list(kvm, its);
+	vgic_its_free_collection_list(kvm, its);
 	kfree(its);
 }
 
-- 
2.5.5

[PATCH v5 08/10] 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>

---
v4 -> v5:
- add comment about locking

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

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 8098f91..bdfceb4 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1434,8 +1434,9 @@ 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;
 	u64 reg, *regptr, clearbits = 0;
+	int type;
 
 	/* When GITS_CTLR.Enable is 1, we ignore write accesses. */
 	if (its->enabled)
@@ -1445,12 +1446,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;
+		type = GITS_BASER_TYPE_DEVICE;
 		break;
 	case 1:
 		regptr = &its->baser_coll_table;
 		entry_size = abi->cte_esz;
-		device_type = GITS_BASER_TYPE_COLLECTION;
+		type = GITS_BASER_TYPE_COLLECTION;
 		clearbits = GITS_BASER_INDIRECT;
 		break;
 	default:
@@ -1462,10 +1463,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 |= (u64)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)) {
+		switch (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;
+		default:
+			break;
+		}
+	}
 }
 
 static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu,
-- 
2.5.5

[PATCH v5 09/10] 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>

---
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 | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
index eb06beb..d12d8e9 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,20 @@ 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
+- The command queue is not allocated:
+  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 v5 10/10] 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).

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

---

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      | 18 ++++++++++++++++++
 3 files changed, 20 insertions(+)

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 bdfceb4..64b6b04 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2395,6 +2395,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)
 {
@@ -2409,6 +2422,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:
@@ -2453,6 +2468,9 @@ static int vgic_its_set_attr(struct kvm_device *dev,
 		case KVM_DEV_ARM_VGIC_CTRL_INIT:
 			/* Nothing to do */
 			return 0;
+		case KVM_DEV_ARM_ITS_CTRL_RESET:
+			vgic_its_reset(dev->kvm, its);
+			return 0;
 		case KVM_DEV_ARM_ITS_SAVE_TABLES:
 			return abi->save_tables(its);
 		case KVM_DEV_ARM_ITS_RESTORE_TABLES:
-- 
2.5.5

Re: [PATCH v5 01/10] KVM: arm/arm64: vgic-its: Fix return value for device table restore

[Christoffer Dall]

On Mon, Oct 23, 2017 at 04:08:20PM +0200, Eric Auger wrote:
> 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>

> 
> ---
> 
> need to CC'ed stable
> 
> 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
> 

Re: [PATCH v5 02/10] KVM: arm/arm64: vgic-its: Fix vgic_its_restore_collection_table returned value

[Christoffer Dall]

On Mon, Oct 23, 2017 at 04:08:21PM +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 use the standard C convention for both vgic_its_restore_cte
> and vgic_its_restore_collection_table. vgic_its_restore_cte now
> returns whether we have reached the end of the table in the @last
> output parameter. vgic_its_restore_collection_table aborts in case
> of error or if the end is found. Otherwise, it now returns 0.
> 
> Fixes: ea1ad53e1e31a3 (KVM: arm64: vgic-its: Collection table save/restore)
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
> 
> ---
> 
> need to be CC'ed stable
> 
> v4 -> v5:
> - added Christoffer R-b
> 
> v2 -> v3: creation
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 40 +++++++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index d27a301..b0ba80f 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2169,7 +2169,19 @@ static int vgic_its_save_cte(struct vgic_its *its,
>  	return kvm_write_guest(its->dev->kvm, gpa, &val, esz);
>  }
>  
> -static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
> +/**
> + * vgic_its_restore_cte - Restore a collection table entry
> + *
> + * @its: its handle
> + * @gpa: guest physical address of the entry
> + * @esz: entry size
> + * @last: output boolean indicating whether we have reached the
> + *       end of the collection table (ie. an invalid entry was decoded)
> + *
> + * Return: 0 upon success, < 0 on error
> + */
> +static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz,
> +				bool *last)
>  {
>  	struct its_collection *collection;
>  	struct kvm *kvm = its->dev->kvm;
> @@ -2182,7 +2194,8 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
>  	if (ret)
>  		return ret;
>  	val = le64_to_cpu(val);
> -	if (!(val & KVM_ITS_CTE_VALID_MASK))
> +	*last = !(val & KVM_ITS_CTE_VALID_MASK);
> +	if (*last)
>  		return 0;
>  
>  	target_addr = (u32)(val >> KVM_ITS_CTE_RDBASE_SHIFT);
> @@ -2198,7 +2211,7 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
>  	if (ret)
>  		return ret;
>  	collection->target_addr = target_addr;
> -	return 1;
> +	return 0;
>  }
>  
>  /**
> @@ -2243,8 +2256,13 @@ static int vgic_its_save_collection_table(struct vgic_its *its)
>  
>  /**
>   * 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.
> + * in guest memory and restores the ITS collection cache.
> + *
> + * @its: its handle
> + *
> + * Requires the Collection BASER to be previously restored.
> + *
> + * Returns 0 on success or < 0 on error
>   */
>  static int vgic_its_restore_collection_table(struct vgic_its *its)
>  {
> @@ -2262,13 +2280,17 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
>  	max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K;
>  
>  	while (read < max_size) {
> -		ret = vgic_its_restore_cte(its, gpa, cte_esz);
> -		if (ret <= 0)
> -			break;
> +		bool last;
> +
> +		ret = vgic_its_restore_cte(its, gpa, cte_esz, &last);
> +		if (ret < 0 || last)
> +			return ret;
>  		gpa += cte_esz;
>  		read += cte_esz;
>  	}
> -	return ret;
> +
> +	/* table was fully filled with valid entries, decoded without error */
> +	return 0;

Why is this patch not replaced with:

	if (ret > 0)
		ret = 0;

Thanks,
-Christoffer

Re: [PATCH v5 03/10] KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS

[Marc Zyngier]

On Mon, Oct 23 2017 at  4:08:22 pm BST, Eric Auger <eric.auger@redhat.com> wrote:
> 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>
>
> ---
>
> need to be CC'ed stable
>
> 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 b0ba80f..1eb355e 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);
>  }

While this is definitely a good hardening of the implementation, I don't
think it fixes anything for the guest which is already misbehaving and
would just not get anything out of this misconfigurarion (in line with
the UNPRED requirement).

So I don't think we need to Cc stable for this. Otherwise:

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

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

Re: [PATCH v5 06/10] KVM: arm/arm64: vgic-its: Remove kvm_its_unmap_device

[Christoffer Dall]

On Mon, Oct 23, 2017 at 04:08:25PM +0200, Eric Auger 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>

> 
> ---
> 
> 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 8472417..2f3c3a1 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
> 

Re: [PATCH v5 03/10] KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS

[Marc Zyngier]

On Wed, Oct 25 2017 at 10:38:13 am BST, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On Mon, Oct 23 2017 at  4:08:22 pm BST, Eric Auger <eric.auger@redhat.com> wrote:
>> 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>
>>
>> ---
>>
>> need to be CC'ed stable
>>
>> 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 b0ba80f..1eb355e 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);
>>  }
>
> While this is definitely a good hardening of the implementation, I don't
> think it fixes anything for the guest which is already misbehaving and
> would just not get anything out of this misconfigurarion (in line with
> the UNPRED requirement).
>
> So I don't think we need to Cc stable for this.

I'm having second thoughts. If the guest has written junk in one of the
BASER registers, enabled the ITS (which won't work), and is then
save/restored, userspace is going to get an -EFAULT as part of the
restore process. Not great.

So cc-stable is probably justified here.

Thanks,

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

Re: [PATCH v5 04/10] KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving tables

[Marc Zyngier]

On Mon, Oct 23 2017 at  4:08:23 pm BST, Eric Auger <eric.auger@redhat.com> wrote:
> 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>

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

Re: [PATCH v5 05/10] KVM: arm/arm64: vgic-its: Save the collection table before device tables

[Christoffer Dall]

On Mon, Oct 23, 2017 at 04:08:24PM +0200, Eric Auger wrote:
> Currently the ITS caches are not emptied on reset.
> 
> After a reset, in case we attempt to save the state before
> the bound devices have registered their MSIs and after the
> 1st level table has been allocated by the ITS driver
> (device BASER is valid), the first level entries are still
> invalid. If the device cache is not empty (devices registered
> before the reset), vgic_its_save_device_tables fails.
> 
> This failure has no consequence as those devices do not
> deserve to be saved: they correspond to the state before
> the reset.
> 
> However the ITS driver already sent MAPC for collections
> and those need to be saved. With the current code, they
> will not and the restored guest will not work properly.
> 
> So this patch saves collection tables  before device tables.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> candidate to be CC'ed stable
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index b6650c2..8472417 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2324,11 +2324,11 @@ static int vgic_its_save_tables_v0(struct vgic_its *its)
>  		return -EBUSY;
>  	}
>  
> -	ret = vgic_its_save_device_tables(its);
> +	ret = vgic_its_save_collection_table(its);
>  	if (ret)
>  		goto out;
>  
> -	ret = vgic_its_save_collection_table(its);
> +	ret = vgic_its_save_device_tables(its);
>  

I don't understand this.  It seems to indicate an ordering of device
tables vs. collection tables.  What is that?

I thought the point was that you'd want to save the valid table, and not
save the other one.

So, aren't we looking for something like this:

	ret = vgic_its_save_device_tables(its);
	if (ret < 0)
		goto out;
 
	ret = vgic_its_save_collection_table(its);


Thanks,
-Christoffer

Re: [PATCH v5 08/10] KVM: arm/arm64: vgic-its: Free caches when GITS_BASER Valid bit is cleared

[Marc Zyngier]

On Mon, Oct 23 2017 at  4:08:27 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>
>
> ---
> v4 -> v5:
> - add comment about locking
>
> v2 -> v3:
> - add a comment and clear cache in if block
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 8098f91..bdfceb4 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1434,8 +1434,9 @@ 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;
>  	u64 reg, *regptr, clearbits = 0;
> +	int type;
>  
>  	/* When GITS_CTLR.Enable is 1, we ignore write accesses. */
>  	if (its->enabled)
> @@ -1445,12 +1446,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;
> +		type = GITS_BASER_TYPE_DEVICE;
>  		break;
>  	case 1:
>  		regptr = &its->baser_coll_table;
>  		entry_size = abi->cte_esz;
> -		device_type = GITS_BASER_TYPE_COLLECTION;
> +		type = GITS_BASER_TYPE_COLLECTION;
>  		clearbits = GITS_BASER_INDIRECT;
>  		break;
>  	default:
> @@ -1462,10 +1463,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 |= (u64)type << GITS_BASER_TYPE_SHIFT;

Nit: Why having changed the type to being an int? The existing type (u64) was
perfectly sane, and avoids the ugly cast. Also, table_type would be more
explicit than the pretty generic "type".

>  	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)) {
> +		switch (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;
> +		default:
> +			break;

Nit: this default clause is useless.

> +		}
> +	}
>  }
>  
>  static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu,

Otherwise:

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

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

Re: [PATCH v5 07/10] KVM: arm/arm64: vgic-its: New helper functions to free the caches

[Christoffer Dall]

On Mon, Oct 23, 2017 at 04:08:26PM +0200, Eric Auger 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: Christoffer Dall <christoffer.dall@linaro.org>

> ---
> 
> 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 | 51 ++++++++++++++++++++++----------------------
>  1 file changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 2f3c3a1..8098f91 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -910,6 +910,30 @@ static void vgic_its_free_device(struct kvm *kvm, struct its_device *device)
>  	kfree(device);
>  }
>  
> +static void vgic_its_free_device_list(struct kvm *kvm, struct vgic_its *its)
> +{
> +	struct its_device *cur, *temp;
> +
> +	mutex_lock(&its->its_lock);
> +
> +	list_for_each_entry_safe(cur, temp, &its->device_list, dev_list)
> +		vgic_its_free_device(kvm, cur);
> +
> +	mutex_unlock(&its->its_lock);
> +}
> +
> +static void vgic_its_free_collection_list(struct kvm *kvm, struct vgic_its *its)
> +{
> +	struct its_collection *cur, *temp;
> +
> +	mutex_lock(&its->its_lock);
> +
> +	list_for_each_entry_safe(cur, temp, &its->collection_list, coll_list)
> +		vgic_its_free_collection(its, cur->collection_id);
> +
> +	mutex_unlock(&its->its_lock);
> +}
> +
>  /* 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 +1651,9 @@ 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;
> -
> -		coll = list_entry(cur, struct its_collection, coll_list);
> -		list_del(cur);
> -		kfree(coll);
> -	}
> -	mutex_unlock(&its->its_lock);
>  
> +	vgic_its_free_device_list(kvm, its);
> +	vgic_its_free_collection_list(kvm, its);
>  	kfree(its);
>  }
>  
> -- 
> 2.5.5
> 

Re: [PATCH v5 07/10] KVM: arm/arm64: vgic-its: New helper functions to free the caches

[Marc Zyngier]

On Mon, Oct 23 2017 at  4:08:26 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 v5 09/10] KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL_RESET

[Marc Zyngier]

On Mon, Oct 23 2017 at  4:08:28 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>
>
> ---
> 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 | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> index eb06beb..d12d8e9 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,20 @@ 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
> +- The command queue is not allocated:

I don't think we should say anything like that. Allocation is a guest
thing, and hasn't much to do with the ITS itself. Specifying the state
of the various registers should be enough.

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

Thanks,

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

Re: [PATCH v5 10/10] KVM: arm/arm64: vgic-its: Implement KVM_DEV_ARM_ITS_CTRL_RESET

[Marc Zyngier]

On Mon, Oct 23 2017 at  4:08:29 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).
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
>
> ---
>
> 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      | 18 ++++++++++++++++++
>  3 files changed, 20 insertions(+)
>
> 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 bdfceb4..64b6b04 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2395,6 +2395,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);

I sense a problem here. There is no locking when resetting the fields,
and there is no guarantee that no vcpus are running at this stage (we
rely on a well behaved userspace).

How do we ensure this? We should move the checks we have in the
save/restore functions to a common location vgic_its_set_attr and
protect all the call sites.

Thanks,

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

Re: [PATCH v5 03/10] KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS

[Christoffer Dall]

On Mon, Oct 23, 2017 at 04:08:22PM +0200, Eric Auger wrote:
> 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>

> 
> ---
> 
> need to be CC'ed stable
> 
> 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 b0ba80f..1eb355e 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
>