[PATCH 0/9] vITS Migration fixes and reset

[PATCH 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. I dared to include 2 Wanghaibin patches,
taking into account the discussions held on the ML but obviously
they can live separately.

On guest reset or when shutdown -r is initiated from guest, the ITS
caches are not saved into guest RAM. However currently the ITS is not
resettable, meaning the ITS will contain previously set values in
its registers. If we initiate a state backup before the guest
re-writes the ITS registers, we use the old register values to restore
the ITS tables. And since they may have never been written they
may contain invalid entries. This leads to inconsistencies detected
by the save/restore code and causes save/restore failure.

Patches [1-5] should be cc'ed stable I think:
- KVM: arm/arm64: vgic-its: fix return value for restore (Wanghaibin)
  happens on restore whenever the device or ITT tables contain
  only invalid data
- Always allow clearing GITS_CREADR/CWRITER
  On first boot, if GITS_CBASER queue size is 0 and if we save at
  this moment, restore fails on CREAD write.
- if GITS_BASER<n> point to invalid tables we currently return -EINVAL
  on state save. QEMU aborts on this error whereas it forgives -EFAULT
  as we considered we should not abort on guest bad programming.
- Then we should never attempt to use GITS_BASER<n> and GITS_CBASER
  if they are not valid. Patches 4 and 5 fix that.

Patches [6-9] implement a new ITS reset IOCTL
  - maybe patch "free caches when GITS_BASER Valid bit is cleared" can
    be omitted if we consider the caches should not be cleared when
    GITS_BASER<n>.Valid is cleared.

Best Regards

Eric

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

* 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:
PATCH v1
- series including 2 modified patches of Wanghaibin


Eric Auger (7):
  KVM: arm/arm64: vgic-its: Always allow clearing GITS_CREADR/CWRITER
  KVM: arm/arm64: vgic-its: Improve error reporting on device table save
  KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving
    tables
  KVM: arm/arm64: vgic-its: Check GITS_CBASER validity before processing
    commands
  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 restore
  KVM: arm/arm64: vgic-its: new helper functions to free the caches

 Documentation/virtual/kvm/devices/arm-vgic-its.txt |  16 ++
 arch/arm/include/uapi/asm/kvm.h                    |   1 +
 arch/arm64/include/uapi/asm/kvm.h                  |   1 +
 virt/kvm/arm/vgic/vgic-its.c                       | 211 +++++++++++++--------
 4 files changed, 155 insertions(+), 74 deletions(-)

-- 
2.5.5

[PATCH 1/9] KVM: arm/arm64: vgic-its: fix return value for restore

[Eric Auger]

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

This patch fix the migrate restore tables failure.

The same scene, at the destination, the restore tables
interface traversal guest memory, and check the dte/ite
is valid or not.  If all dtes/ites are invalid, we will do
try next one, and the last it will take the 1 return value,
but currently, it be treated as error. That's not correct.

This patch try to fix this problem.

Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>

---

need to CC stable

v1 -> v2:
- if (ret > 0) ret = 0
---
 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 f51c1e1..fbbc97b 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2018,7 +2018,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
 		return PTR_ERR(dev);
 
 	ret = vgic_its_restore_itt(its, dev);
-	if (ret) {
+	if (ret < 0) {
 		vgic_its_free_device(its->dev->kvm, dev);
 		return ret;
 	}
@@ -2141,7 +2141,7 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
 	}
 
 	if (ret > 0)
-		ret = -EINVAL;
+		ret = 0;
 
 	return ret;
 }
-- 
2.5.5

[PATCH 2/9] KVM: arm/arm64: vgic-its: Always allow clearing GITS_CREADR/CWRITER

[Eric Auger]

If the GITS_CBASER Size field is 0, which can correspond to a
reset value, the userspace fails to set the GITS_CREADR/CWRITER
offsets to 0. This failure is not justified.

Let's allow this setting which can also correspond to a reset value.

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

---

need to CC 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 fbbc97b..76bed2d 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1329,7 +1329,7 @@ static void vgic_mmio_write_its_cwriter(struct kvm *kvm, struct vgic_its *its,
 
 	reg = update_64bit_reg(its->cwriter, addr & 7, len, val);
 	reg = ITS_CMD_OFFSET(reg);
-	if (reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
+	if (reg && reg >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
 		mutex_unlock(&its->cmd_lock);
 		return;
 	}
@@ -1370,7 +1370,7 @@ static int vgic_mmio_uaccess_write_its_creadr(struct kvm *kvm,
 	}
 
 	cmd_offset = ITS_CMD_OFFSET(val);
-	if (cmd_offset >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
+	if (cmd_offset && cmd_offset >= ITS_CMD_BUFFER_SIZE(its->cbaser)) {
 		ret = -EINVAL;
 		goto out;
 	}
-- 
2.5.5

[PATCH 3/9] KVM: arm/arm64: vgic-its: Improve error reporting on device table save

[Eric Auger]

At the moment the device table save() returns -EINVAL if
vgic_its_check_id() fails to return the gpa of the entry
associated to the device/collection id. Let vgic_its_check_id()
return an int instead of a bool and return a more precised
error value:
- EINVAL in case the id is out of range
- EFAULT if the gpa is not provisionned or is not valid

We also check first the GITS_BASER<n> Valid bit is set.

This allows the userspace to discriminate failure reasons.

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

---

need to CC stable
---
 virt/kvm/arm/vgic/vgic-its.c | 53 ++++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 17 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 76bed2d..c1f7972 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -688,14 +688,24 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
 }
 
 /*
- * Check whether an ID can be stored into the corresponding guest table.
+ * vgic_its_check_id - Check whether an ID can be stored into
+ * the corresponding guest table.
+ *
  * For a direct table this is pretty easy, but gets a bit nasty for
  * indirect tables. We check whether the resulting guest physical address
  * is actually valid (covered by a memslot and guest accessible).
  * For this we have to read the respective first level entry.
+ *
+ * @its: its handle
+ * @baser: GITS_BASER<n> register
+ * @id: id of the device/collection
+ * @eaddr: output gpa of the corresponding table entry
+ *
+ * Return: 0 on success, -EINVAL if @id is out of range, -EFAULT if
+ * the address cannot be computed or is not valid
  */
-static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
-			      gpa_t *eaddr)
+static int vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
+			     gpa_t *eaddr)
 {
 	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
 	u64 indirect_ptr, type = GITS_BASER_TYPE(baser);
@@ -703,50 +713,56 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
 	int index;
 	gfn_t gfn;
 
+	if (!(baser & GITS_BASER_VALID))
+		return -EFAULT;
+
 	switch (type) {
 	case GITS_BASER_TYPE_DEVICE:
 		if (id >= BIT_ULL(VITS_TYPER_DEVBITS))
-			return false;
+			return -EINVAL;
 		break;
 	case GITS_BASER_TYPE_COLLECTION:
 		/* as GITS_TYPER.CIL == 0, ITS supports 16-bit collection ID */
 		if (id >= BIT_ULL(16))
-			return false;
+			return -EINVAL;
 		break;
 	default:
-		return false;
+		return -EINVAL;
 	}
 
 	if (!(baser & GITS_BASER_INDIRECT)) {
 		phys_addr_t addr;
 
 		if (id >= (l1_tbl_size / esz))
-			return false;
+			return -EINVAL;
 
 		addr = BASER_ADDRESS(baser) + id * esz;
 		gfn = addr >> PAGE_SHIFT;
 
 		if (eaddr)
 			*eaddr = addr;
-		return kvm_is_visible_gfn(its->dev->kvm, gfn);
+		if (kvm_is_visible_gfn(its->dev->kvm, gfn))
+			return 0;
+		else
+			return -EFAULT;
 	}
 
 	/* calculate and check the index into the 1st level */
 	index = id / (SZ_64K / esz);
 	if (index >= (l1_tbl_size / sizeof(u64)))
-		return false;
+		return -EINVAL;
 
 	/* Each 1st level entry is represented by a 64-bit value. */
 	if (kvm_read_guest(its->dev->kvm,
 			   BASER_ADDRESS(baser) + index * sizeof(indirect_ptr),
 			   &indirect_ptr, sizeof(indirect_ptr)))
-		return false;
+		return -EFAULT;
 
 	indirect_ptr = le64_to_cpu(indirect_ptr);
 
 	/* check the valid bit of the first level entry */
 	if (!(indirect_ptr & BIT_ULL(63)))
-		return false;
+		return -EFAULT;
 
 	/*
 	 * Mask the guest physical address and calculate the frame number.
@@ -762,7 +778,10 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
 
 	if (eaddr)
 		*eaddr = indirect_ptr;
-	return kvm_is_visible_gfn(its->dev->kvm, gfn);
+	if (kvm_is_visible_gfn(its->dev->kvm, gfn))
+		return 0;
+	else
+		return -EFAULT;
 }
 
 static int vgic_its_alloc_collection(struct vgic_its *its,
@@ -771,7 +790,7 @@ static int vgic_its_alloc_collection(struct vgic_its *its,
 {
 	struct its_collection *collection;
 
-	if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
+	if (vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
 		return E_ITS_MAPC_COLLECTION_OOR;
 
 	collection = kzalloc(sizeof(*collection), GFP_KERNEL);
@@ -943,7 +962,7 @@ static int vgic_its_cmd_handle_mapd(struct kvm *kvm, struct vgic_its *its,
 	gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
 	struct its_device *device;
 
-	if (!vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
+	if (vgic_its_check_id(its, its->baser_device_table, device_id, NULL))
 		return E_ITS_MAPD_DEVICE_OOR;
 
 	if (valid && num_eventid_bits > VITS_TYPER_IDBITS)
@@ -2060,9 +2079,9 @@ static int vgic_its_save_device_tables(struct vgic_its *its)
 		int ret;
 		gpa_t eaddr;
 
-		if (!vgic_its_check_id(its, baser,
-				       dev->device_id, &eaddr))
-			return -EINVAL;
+		ret = vgic_its_check_id(its, baser, dev->device_id, &eaddr);
+		if (ret)
+			return ret;
 
 		ret = vgic_its_save_itt(its, dev);
 		if (ret)
-- 
2.5.5

[PATCH 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 Collection table save() we use the gpa field whereas the Valid bit
should be used. On device table save() 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 unifomize the code between save and restore.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 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 c1f7972..60ecf91 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2067,11 +2067,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);
 
@@ -2217,17 +2218,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);
@@ -2258,17 +2259,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 5/9] KVM: arm/arm64: vgic-its: Check GITS_CBASER validity before processing commands

[Eric Auger]

At the moment vgic_its_process_commands() does not
check the CBASER is valid before processing any command.
Let's fix that.

Also rename cbaser local variable into cbaser_pa to avoid
any confusion with the full register.

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

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 60ecf91..720552c 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1301,17 +1301,20 @@ static void vgic_mmio_write_its_cbaser(struct kvm *kvm, struct vgic_its *its,
 /* Must be called with the cmd_lock held. */
 static void vgic_its_process_commands(struct kvm *kvm, struct vgic_its *its)
 {
-	gpa_t cbaser;
+	gpa_t cbaser_pa;
 	u64 cmd_buf[4];
 
-	/* Commands are only processed when the ITS is enabled. */
-	if (!its->enabled)
+	/*
+	 * Commands are only processed when the ITS is enabled and
+	 * CBASER is valid
+	 */
+	if (!its->enabled || (!(its->cbaser & GITS_CBASER_VALID)))
 		return;
 
-	cbaser = CBASER_ADDRESS(its->cbaser);
+	cbaser_pa = CBASER_ADDRESS(its->cbaser);
 
 	while (its->cwriter != its->creadr) {
-		int ret = kvm_read_guest(kvm, cbaser + its->creadr,
+		int ret = kvm_read_guest(kvm, cbaser_pa + its->creadr,
 					 cmd_buf, ITS_CMD_SIZE);
 		/*
 		 * If kvm_read_guest() fails, this could be due to the guest
-- 
2.5.5

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

[Eric Auger]

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

We create 2 new functions that frees the device and
collection lists. this is currently called by vgic_its_destroy()
and we will add other callers in subsequent patches.

We also remove the check on its->device_list.next as it looks
unnecessary:

The kvm device is removed by kvm_destroy_devices which loops on
all the devices added to kvm->devices. kvm_ioctl_create_device
only adds the device to kvm_devices once the lists have been
initialized (in vgic_create_its).

We also move vgic_its_free_device to prepare for new callers.

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

---
[Eric] removed its->device_list.next which is not needed as
pointed out by Wanghaibin. Reword the commit message
---
 virt/kvm/arm/vgic/vgic-its.c | 76 ++++++++++++++++++++++++--------------------
 1 file changed, 41 insertions(+), 35 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 720552c..3f4deb4 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -611,6 +611,45 @@ static void its_free_ite(struct kvm *kvm, struct its_ite *ite)
 	kfree(ite);
 }
 
+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_free_device_list(struct kvm *kvm, struct vgic_its *its)
+{
+	struct list_head *cur, *temp;
+
+	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);
+	}
+	mutex_unlock(&its->its_lock);
+}
+
+static void vgic_its_free_collection_list(struct kvm *kvm, struct vgic_its *its)
+{
+	struct list_head *cur, *temp;
+
+	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);
+}
+
+
 static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
 {
 	return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT_ULL(size) - 1);
@@ -1634,46 +1673,13 @@ 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;
 	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 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 provisionned 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>
---
 virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 3f4deb4..e280e62 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1471,8 +1471,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)
@@ -1482,12 +1483,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:
@@ -1499,10 +1500,24 @@ 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 (reg & GITS_BASER_VALID)
+		return;
+
+	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 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, 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.

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

---

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

diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
index eb06beb..047358c 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,15 @@ 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:
+ ----------------
+
+- the ITS is not enabled and quiescent:
+  GITS_CTLR.Enabled = 0 .Quiescent=1
+- caches are empty
+- No collection or device table is provisionned
+  GITS_BASER<n>.Valid = 0
+- the command queue is not allocated:
+  GITS_CBASER = 0, GITS_CREADR = 0, GITS_CWRITER = 0
+- The ABI version corresponds to the one set before reset
-- 
2.5.5

[PATCH 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 device,
collection and lpi lists.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arch/arm/include/uapi/asm/kvm.h   |  1 +
 arch/arm64/include/uapi/asm/kvm.h |  1 +
 virt/kvm/arm/vgic/vgic-its.c      | 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 e280e62..ed96269 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2385,6 +2385,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)
 {
@@ -2399,6 +2412,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:
@@ -2443,6 +2458,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 7/9] KVM: arm/arm64: vgic-its: free caches when GITS_BASER Valid bit is cleared

[Christoffer Dall]

Hi Eric,

On Mon, Sep 25, 2017 at 03:34:36PM +0200, Eric Auger wrote:
> When the GITS_BASER<n>.Valid gets cleared, the data structures in
> guest RAM are not provisionned 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.

Just a thought.  What about the opposite case, if the BASERs were
previously not valid, and then become valid, is the ITS expected restore
the state from memory?

Thanks,
-Christoffer

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

[Auger Eric]

Hi Christoffer,
On 16/10/2017 11:26, Christoffer Dall wrote:
> Hi Eric,
> 
> On Mon, Sep 25, 2017 at 03:34:36PM +0200, Eric Auger wrote:
>> When the GITS_BASER<n>.Valid gets cleared, the data structures in
>> guest RAM are not provisionned 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.
> 
> Just a thought.  What about the opposite case, if the BASERs were
> previously not valid, and then become valid, is the ITS expected restore
> the state from memory?
> 
> Thanks,
> -Christoffer
> 
No the spec does not mandate that as far as I understand. Also the spec
does not mandate clearing the cache when BASER moves to invalid (which
this patch does), although this would have madee sense to me. So maybe
we should drop that patch and only clean the cache when the IOCTL is used.

Thanks

Eric

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

[Peter Maydell]

On 16 October 2017 at 10:26, Christoffer Dall <cdall@linaro.org> wrote:
> Hi Eric,
>
> On Mon, Sep 25, 2017 at 03:34:36PM +0200, Eric Auger wrote:
>> When the GITS_BASER<n>.Valid gets cleared, the data structures in
>> guest RAM are not provisionned 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.
>
> Just a thought.  What about the opposite case, if the BASERs were
> previously not valid, and then become valid, is the ITS expected restore
> the state from memory?

Architecturally speaking, it's a cache. As soon as the guest
turns the GITS_CTLR.Enabled bit on, the ITS is permitted to
start reading from the tables. It's an implementation choice
whether it wants to preload a bunch of stuff, only load it
up when it becomes necessary or even leave it all in memory
and cache nothing.

Eric wrote:
> Also the spec does not mandate clearing the cache when BASER
> moves to invalid (which this patch does), although this would
> have made sense to me.

The spec says that messing with GITS_BASER while GITS_CTRL.Enabled
is set or GITS_CTLR.Quiescent is 0 is UNPREDICTABLE. And it
says that once the OS has done the Enabled/Quiescent handshake
then the ITS must have written out any dirty data to memory
and stopped doing anything. So the effect is that BASER
can't ever move to invalid while the caches are dirty.

thanks
-- PMM

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

[Christoffer Dall]

On Mon, Oct 16, 2017 at 11:44:05AM +0200, Auger Eric wrote:
> Hi Christoffer,
> On 16/10/2017 11:26, Christoffer Dall wrote:
> > Hi Eric,
> > 
> > On Mon, Sep 25, 2017 at 03:34:36PM +0200, Eric Auger wrote:
> >> When the GITS_BASER<n>.Valid gets cleared, the data structures in
> >> guest RAM are not provisionned 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.
> > 
> > Just a thought.  What about the opposite case, if the BASERs were
> > previously not valid, and then become valid, is the ITS expected restore
> > the state from memory?
> > 
> > Thanks,
> > -Christoffer
> > 
> No the spec does not mandate that as far as I understand. Also the spec
> does not mandate clearing the cache when BASER moves to invalid (which
> this patch does), although this would have madee sense to me. So maybe
> we should drop that patch and only clean the cache when the IOCTL is used.
> 
I thought Marc pointed out that there is at least one hardware
implementation which does clean its cache when the BASER moves to
invalid, so it's not unlikely that software could rely on this behavior,
and I therefore think we should implement it, because it's not in
violation of the spec either.

Thanks,
-Christoffer

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

[Christoffer Dall]

On Mon, Oct 16, 2017 at 10:47:35AM +0100, Peter Maydell wrote:
> On 16 October 2017 at 10:26, Christoffer Dall <cdall@linaro.org> wrote:
> > Hi Eric,
> >
> > On Mon, Sep 25, 2017 at 03:34:36PM +0200, Eric Auger wrote:
> >> When the GITS_BASER<n>.Valid gets cleared, the data structures in
> >> guest RAM are not provisionned 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.
> >
> > Just a thought.  What about the opposite case, if the BASERs were
> > previously not valid, and then become valid, is the ITS expected restore
> > the state from memory?
> 
> Architecturally speaking, it's a cache. As soon as the guest
> turns the GITS_CTLR.Enabled bit on, the ITS is permitted to
> start reading from the tables. It's an implementation choice
> whether it wants to preload a bunch of stuff, only load it
> up when it becomes necessary or even leave it all in memory
> and cache nothing.
> 
> Eric wrote:
> > Also the spec does not mandate clearing the cache when BASER
> > moves to invalid (which this patch does), although this would
> > have made sense to me.
> 
> The spec says that messing with GITS_BASER while GITS_CTRL.Enabled
> is set or GITS_CTLR.Quiescent is 0 is UNPREDICTABLE. And it
> says that once the OS has done the Enabled/Quiescent handshake
> then the ITS must have written out any dirty data to memory
> and stopped doing anything. So the effect is that BASER
> can't ever move to invalid while the caches are dirty.
> 
Right, ok, so if we in fact clear the caches when the ITS is turned off
and the BASER becomes invalid, and we load any data from memory as the
ITS is turned on again (for the OS power management sequence) we should
be good.

And that means that software can't just 'move' a table location in
memory by making the BASER invalid and then valid again, at least not
without turning off the ITS.

That makes sense to me, and so we don't need to do anything when the
BASER becomes valid.

Thanks,
-Christoffer