[PATCH v4 00/11] vITS Migration fixes and reset

[PATCH v4 00/11] 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-7] attempt to fix the migration issues without
implementing the reset.
As such they may be candidate for stable:
- handle case where all collection, device and ITT entries are
  invalid on restore (which shouldn't be an error)
- limit the cases where we return -EINVAL in table save()
- never attempt to use GITS_BASER<n> and GITS_CBASER if they are
  not valid.
- Check GITS_BASER<n> and GITS_CBASER on ITS enable
- systematically do both device and collection save/restore even if
  one fails.

Patches [8-11] 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-v4

* 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:
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 (10):
  KVM: arm/arm64: vgic-its: fix return value for device table restore
  KVM: arm/arm64: vgic-its: fix vgic_its_restore_collection_table
    returned value
  KVM: arm/arm64: vgic-its: Improve error reporting on device table save
  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: Check GITS_CBASER validity before processing
    commands
  KVM: arm/arm64: vgic-its: Always attempt to save/restore device and
    collection tables
  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 (1):
  KVM: arm/arm64: vgic-its: new helper functions to free the caches

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

-- 
2.5.5

[PATCH v4 00/11] 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-7] attempt to fix the migration issues without
implementing the reset.
As such they may be candidate for stable:
- handle case where all collection, device and ITT entries are
  invalid on restore (which shouldn't be an error)
- limit the cases where we return -EINVAL in table save()
- never attempt to use GITS_BASER<n> and GITS_CBASER if they are
  not valid.
- Check GITS_BASER<n> and GITS_CBASER on ITS enable
- systematically do both device and collection save/restore even if
  one fails.

Patches [8-11] 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-v4

* 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:
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 (10):
  KVM: arm/arm64: vgic-its: fix return value for device table restore
  KVM: arm/arm64: vgic-its: fix vgic_its_restore_collection_table
    returned value
  KVM: arm/arm64: vgic-its: Improve error reporting on device table save
  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: Check GITS_CBASER validity before processing
    commands
  KVM: arm/arm64: vgic-its: Always attempt to save/restore device and
    collection tables
  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 (1):
  KVM: arm/arm64: vgic-its: new helper functions to free the caches

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

-- 
2.5.5

[PATCH v4 01/11] KVM: arm/arm64: vgic-its: fix return value for device table restore

[Eric Auger]

AT the moment 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 fully revisits the errror handling while fixing those
2 bugs.

- entry_fn_t now takes a valid output paraleter
- scan_its_table() now returns <= 0 values and output 2 booleans,
  valid and last.
- vgic_its_restore_itt() now returns <= 0 values.
- vgic_its_restore_device_tables() also returns <= 0 values.

With that patch we are able to properly handle the case where
all data are invalid but we still are able to detect the case
where a next entry was referenced by some valid entry and
never found.

Fixes: 57a9a117154c93 (KVM: arm64: vgic-its: Device table save/restore)
Fixes: eff484e0298da5 (KVM: arm64: vgic-its: ITT save and restore)
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reported-by: wanghaibin <wanghaibin.wang@huawei.com>

---

need to CC stable

v3 -> v4:
- set *valid at beginning of handle_l1_dte

v2 -> v3:
- add comments
- added valid parameter
- 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 | 95 ++++++++++++++++++++++++++++++++------------
 1 file changed, 70 insertions(+), 25 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index f51c1e1..eea14a1 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1772,16 +1772,20 @@ static u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
 
 /**
  * entry_fn_t - Callback called on a table entry restore path
+ *
  * @its: its handle
  * @id: id of the entry
  * @entry: pointer to the entry
  * @opaque: pointer to an opaque data
+ * @valid: indicates whether valid data is associated to this entry
+ * (the entry itself in case of linear table or entries in the next level,
+ * in case of hierachical tables)
  *
  * Return: < 0 on error, 0 if last element was identified, id offset to next
  * element otherwise
  */
 typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
-			  void *opaque);
+			  void *opaque, bool *valid);
 
 /**
  * scan_its_table - Scan a contiguous table in guest RAM and applies a function
@@ -1794,29 +1798,34 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
  * @start_id: the ID of the first entry in the table
  * (non zero for 2d level tables)
  * @fn: function to apply on each entry
+ * @opaque: opaque data passed to @fn
+ * @valid: indicates whether the table contains any valid data
+ * @last: returns whether the last valid entry was decoded
  *
- * Return: < 0 on error, 0 if last element was identified, 1 otherwise
- * (the last element may not be found on second level tables)
+ * Return: < 0 on error, 0 on success
  */
 static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz,
-			  int start_id, entry_fn_t fn, void *opaque)
+			  int start_id, entry_fn_t fn, void *opaque,
+			  bool *valid, bool *last)
 {
 	void *entry = kzalloc(esz, GFP_KERNEL);
 	struct kvm *kvm = its->dev->kvm;
 	unsigned long len = size;
 	int id = start_id;
 	gpa_t gpa = base;
+	int next_offset = 0;
 	int ret;
 
 	while (len > 0) {
-		int next_offset;
 		size_t byte_offset;
+		bool entry_valid;
 
 		ret = kvm_read_guest(kvm, gpa, entry, esz);
 		if (ret)
 			goto out;
 
-		next_offset = fn(its, id, entry, opaque);
+		next_offset = fn(its, id, entry, opaque, &entry_valid);
+		*valid |= entry_valid;
 		if (next_offset <= 0) {
 			ret = next_offset;
 			goto out;
@@ -1827,9 +1836,15 @@ static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz,
 		gpa += byte_offset;
 		len -= byte_offset;
 	}
-	ret =  1;
-
+	/*
+	 * the table lookup was completed without identifying the
+	 * last valid entry (ie. next_offset > 0).
+	 */
+	ret = 0;
 out:
+	if (!next_offset)
+		*last = true;
+
 	kfree(entry);
 	return ret;
 }
@@ -1854,12 +1869,14 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
 
 /**
  * vgic_its_restore_ite - restore an interrupt translation entry
+ *
  * @event_id: id used for indexing
  * @ptr: pointer to the ITE entry
  * @opaque: pointer to the its_device
+ * @valid: indicates whether the ite is valid
  */
 static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
-				void *ptr, void *opaque)
+				void *ptr, void *opaque, bool *valid)
 {
 	struct its_device *dev = (struct its_device *)opaque;
 	struct its_collection *collection;
@@ -1879,7 +1896,9 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
 	coll_id = val & KVM_ITS_ITE_ICID_MASK;
 	lpi_id = (val & KVM_ITS_ITE_PINTID_MASK) >> KVM_ITS_ITE_PINTID_SHIFT;
 
-	if (!lpi_id)
+	*valid = !!lpi_id;
+
+	if (!*valid)
 		return 1; /* invalid entry, no choice but to scan next entry */
 
 	if (lpi_id < VGIC_MIN_LPI)
@@ -1940,6 +1959,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);
@@ -1947,9 +1974,15 @@ static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
 	int ret;
 	int ite_esz = abi->ite_esz;
 	size_t max_size = BIT_ULL(dev->num_eventid_bits) * ite_esz;
+	bool valid = false, last = false;
 
 	ret = scan_its_table(its, base, max_size, ite_esz, 0,
-			     vgic_its_restore_ite, dev);
+			     vgic_its_restore_ite, dev, &valid, &last);
+
+	if (!ret && valid && !last) {
+		/* a next element was referenced but not found */
+		return -EINVAL;
+	}
 
 	return ret;
 }
@@ -1985,29 +2018,29 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev,
  * @id: device id the DTE corresponds to
  * @ptr: kernel VA where the 8 byte DTE is located
  * @opaque: unused
+ * @valid: indicates whether the dte is valid
  *
  * Return: < 0 on error, 0 if the dte is the last one, id offset to the
  * next dte otherwise
  */
 static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
-				void *ptr, void *opaque)
+				void *ptr, void *opaque, bool *valid)
 {
 	struct its_device *dev;
 	gpa_t itt_addr;
 	u8 num_eventid_bits;
 	u64 entry = *(u64 *)ptr;
-	bool valid;
 	u32 offset;
 	int ret;
 
 	entry = le64_to_cpu(entry);
 
-	valid = entry >> KVM_ITS_DTE_VALID_SHIFT;
+	*valid = entry >> KVM_ITS_DTE_VALID_SHIFT;
 	num_eventid_bits = (entry & KVM_ITS_DTE_SIZE_MASK) + 1;
 	itt_addr = ((entry & KVM_ITS_DTE_ITTADDR_MASK)
 			>> KVM_ITS_DTE_ITTADDR_SHIFT) << 8;
 
-	if (!valid)
+	if (!*valid)
 		return 1;
 
 	/* dte entry is valid */
@@ -2082,13 +2115,14 @@ static int vgic_its_save_device_tables(struct vgic_its *its)
  * @id: index of the entry in the L1 table
  * @addr: kernel VA
  * @opaque: unused
+ * @valid: indicates whether any dte entry was found
  *
  * L1 table entries are scanned by steps of 1 entry
  * Return < 0 if error, 0 if last dte was found when scanning the L2
  * table, +1 otherwise (meaning next L1 entry must be scanned)
  */
 static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
-			 void *opaque)
+			 void *opaque, bool *valid)
 {
 	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
 	int l2_start_id = id * (SZ_64K / abi->dte_esz);
@@ -2096,21 +2130,29 @@ static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
 	int dte_esz = abi->dte_esz;
 	gpa_t gpa;
 	int ret;
+	bool last;
 
 	entry = le64_to_cpu(entry);
 
-	if (!(entry & KVM_ITS_L1E_VALID_MASK))
+	*valid = entry & KVM_ITS_L1E_VALID_MASK;
+
+	if (!*valid)
 		return 1;
 
 	gpa = entry & KVM_ITS_L1E_ADDR_MASK;
 
 	ret = scan_its_table(its, gpa, SZ_64K, dte_esz,
-			     l2_start_id, vgic_its_restore_dte, NULL);
+			     l2_start_id, vgic_its_restore_dte, NULL,
+			     valid, &last);
 
-	if (ret <= 0)
-		return ret;
+	/*
+	 * if the last dte has not been found in this L2 table, we
+	 * need to scan the next L1 entry
+	 */
+	if (!ret && !last)
+		return 1;
 
-	return 1;
+	return ret;
 }
 
 /**
@@ -2124,6 +2166,7 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
 	int l1_esz, ret;
 	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
 	gpa_t l1_gpa;
+	bool valid = false, last = false;
 
 	if (!(baser & GITS_BASER_VALID))
 		return 0;
@@ -2133,15 +2176,17 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
 	if (baser & GITS_BASER_INDIRECT) {
 		l1_esz = GITS_LVL1_ENTRY_SIZE;
 		ret = scan_its_table(its, l1_gpa, l1_tbl_size, l1_esz, 0,
-				     handle_l1_dte, NULL);
+				     handle_l1_dte, NULL, &valid, &last);
 	} else {
 		l1_esz = abi->dte_esz;
 		ret = scan_its_table(its, l1_gpa, l1_tbl_size, l1_esz, 0,
-				     vgic_its_restore_dte, NULL);
+				     vgic_its_restore_dte, NULL, &valid, &last);
 	}
 
-	if (ret > 0)
-		ret = -EINVAL;
+	if (!ret && valid && !last) {
+		/* a next element was referenced but not found */
+		return -EINVAL;
+	}
 
 	return ret;
 }
-- 
2.5.5

[PATCH v4 01/11] KVM: arm/arm64: vgic-its: fix return value for device table restore

[Eric Auger]

AT the moment 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 fully revisits the errror handling while fixing those
2 bugs.

- entry_fn_t now takes a valid output paraleter
- scan_its_table() now returns <= 0 values and output 2 booleans,
  valid and last.
- vgic_its_restore_itt() now returns <= 0 values.
- vgic_its_restore_device_tables() also returns <= 0 values.

With that patch we are able to properly handle the case where
all data are invalid but we still are able to detect the case
where a next entry was referenced by some valid entry and
never found.

Fixes: 57a9a117154c93 (KVM: arm64: vgic-its: Device table save/restore)
Fixes: eff484e0298da5 (KVM: arm64: vgic-its: ITT save and restore)
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reported-by: wanghaibin <wanghaibin.wang@huawei.com>

---

need to CC stable

v3 -> v4:
- set *valid at beginning of handle_l1_dte

v2 -> v3:
- add comments
- added valid parameter
- 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 | 95 ++++++++++++++++++++++++++++++++------------
 1 file changed, 70 insertions(+), 25 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index f51c1e1..eea14a1 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1772,16 +1772,20 @@ static u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
 
 /**
  * entry_fn_t - Callback called on a table entry restore path
+ *
  * @its: its handle
  * @id: id of the entry
  * @entry: pointer to the entry
  * @opaque: pointer to an opaque data
+ * @valid: indicates whether valid data is associated to this entry
+ * (the entry itself in case of linear table or entries in the next level,
+ * in case of hierachical tables)
  *
  * Return: < 0 on error, 0 if last element was identified, id offset to next
  * element otherwise
  */
 typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
-			  void *opaque);
+			  void *opaque, bool *valid);
 
 /**
  * scan_its_table - Scan a contiguous table in guest RAM and applies a function
@@ -1794,29 +1798,34 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
  * @start_id: the ID of the first entry in the table
  * (non zero for 2d level tables)
  * @fn: function to apply on each entry
+ * @opaque: opaque data passed to @fn
+ * @valid: indicates whether the table contains any valid data
+ * @last: returns whether the last valid entry was decoded
  *
- * Return: < 0 on error, 0 if last element was identified, 1 otherwise
- * (the last element may not be found on second level tables)
+ * Return: < 0 on error, 0 on success
  */
 static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz,
-			  int start_id, entry_fn_t fn, void *opaque)
+			  int start_id, entry_fn_t fn, void *opaque,
+			  bool *valid, bool *last)
 {
 	void *entry = kzalloc(esz, GFP_KERNEL);
 	struct kvm *kvm = its->dev->kvm;
 	unsigned long len = size;
 	int id = start_id;
 	gpa_t gpa = base;
+	int next_offset = 0;
 	int ret;
 
 	while (len > 0) {
-		int next_offset;
 		size_t byte_offset;
+		bool entry_valid;
 
 		ret = kvm_read_guest(kvm, gpa, entry, esz);
 		if (ret)
 			goto out;
 
-		next_offset = fn(its, id, entry, opaque);
+		next_offset = fn(its, id, entry, opaque, &entry_valid);
+		*valid |= entry_valid;
 		if (next_offset <= 0) {
 			ret = next_offset;
 			goto out;
@@ -1827,9 +1836,15 @@ static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz,
 		gpa += byte_offset;
 		len -= byte_offset;
 	}
-	ret =  1;
-
+	/*
+	 * the table lookup was completed without identifying the
+	 * last valid entry (ie. next_offset > 0).
+	 */
+	ret = 0;
 out:
+	if (!next_offset)
+		*last = true;
+
 	kfree(entry);
 	return ret;
 }
@@ -1854,12 +1869,14 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
 
 /**
  * vgic_its_restore_ite - restore an interrupt translation entry
+ *
  * @event_id: id used for indexing
  * @ptr: pointer to the ITE entry
  * @opaque: pointer to the its_device
+ * @valid: indicates whether the ite is valid
  */
 static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
-				void *ptr, void *opaque)
+				void *ptr, void *opaque, bool *valid)
 {
 	struct its_device *dev = (struct its_device *)opaque;
 	struct its_collection *collection;
@@ -1879,7 +1896,9 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
 	coll_id = val & KVM_ITS_ITE_ICID_MASK;
 	lpi_id = (val & KVM_ITS_ITE_PINTID_MASK) >> KVM_ITS_ITE_PINTID_SHIFT;
 
-	if (!lpi_id)
+	*valid = !!lpi_id;
+
+	if (!*valid)
 		return 1; /* invalid entry, no choice but to scan next entry */
 
 	if (lpi_id < VGIC_MIN_LPI)
@@ -1940,6 +1959,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);
@@ -1947,9 +1974,15 @@ static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
 	int ret;
 	int ite_esz = abi->ite_esz;
 	size_t max_size = BIT_ULL(dev->num_eventid_bits) * ite_esz;
+	bool valid = false, last = false;
 
 	ret = scan_its_table(its, base, max_size, ite_esz, 0,
-			     vgic_its_restore_ite, dev);
+			     vgic_its_restore_ite, dev, &valid, &last);
+
+	if (!ret && valid && !last) {
+		/* a next element was referenced but not found */
+		return -EINVAL;
+	}
 
 	return ret;
 }
@@ -1985,29 +2018,29 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev,
  * @id: device id the DTE corresponds to
  * @ptr: kernel VA where the 8 byte DTE is located
  * @opaque: unused
+ * @valid: indicates whether the dte is valid
  *
  * Return: < 0 on error, 0 if the dte is the last one, id offset to the
  * next dte otherwise
  */
 static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
-				void *ptr, void *opaque)
+				void *ptr, void *opaque, bool *valid)
 {
 	struct its_device *dev;
 	gpa_t itt_addr;
 	u8 num_eventid_bits;
 	u64 entry = *(u64 *)ptr;
-	bool valid;
 	u32 offset;
 	int ret;
 
 	entry = le64_to_cpu(entry);
 
-	valid = entry >> KVM_ITS_DTE_VALID_SHIFT;
+	*valid = entry >> KVM_ITS_DTE_VALID_SHIFT;
 	num_eventid_bits = (entry & KVM_ITS_DTE_SIZE_MASK) + 1;
 	itt_addr = ((entry & KVM_ITS_DTE_ITTADDR_MASK)
 			>> KVM_ITS_DTE_ITTADDR_SHIFT) << 8;
 
-	if (!valid)
+	if (!*valid)
 		return 1;
 
 	/* dte entry is valid */
@@ -2082,13 +2115,14 @@ static int vgic_its_save_device_tables(struct vgic_its *its)
  * @id: index of the entry in the L1 table
  * @addr: kernel VA
  * @opaque: unused
+ * @valid: indicates whether any dte entry was found
  *
  * L1 table entries are scanned by steps of 1 entry
  * Return < 0 if error, 0 if last dte was found when scanning the L2
  * table, +1 otherwise (meaning next L1 entry must be scanned)
  */
 static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
-			 void *opaque)
+			 void *opaque, bool *valid)
 {
 	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
 	int l2_start_id = id * (SZ_64K / abi->dte_esz);
@@ -2096,21 +2130,29 @@ static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
 	int dte_esz = abi->dte_esz;
 	gpa_t gpa;
 	int ret;
+	bool last;
 
 	entry = le64_to_cpu(entry);
 
-	if (!(entry & KVM_ITS_L1E_VALID_MASK))
+	*valid = entry & KVM_ITS_L1E_VALID_MASK;
+
+	if (!*valid)
 		return 1;
 
 	gpa = entry & KVM_ITS_L1E_ADDR_MASK;
 
 	ret = scan_its_table(its, gpa, SZ_64K, dte_esz,
-			     l2_start_id, vgic_its_restore_dte, NULL);
+			     l2_start_id, vgic_its_restore_dte, NULL,
+			     valid, &last);
 
-	if (ret <= 0)
-		return ret;
+	/*
+	 * if the last dte has not been found in this L2 table, we
+	 * need to scan the next L1 entry
+	 */
+	if (!ret && !last)
+		return 1;
 
-	return 1;
+	return ret;
 }
 
 /**
@@ -2124,6 +2166,7 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
 	int l1_esz, ret;
 	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
 	gpa_t l1_gpa;
+	bool valid = false, last = false;
 
 	if (!(baser & GITS_BASER_VALID))
 		return 0;
@@ -2133,15 +2176,17 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
 	if (baser & GITS_BASER_INDIRECT) {
 		l1_esz = GITS_LVL1_ENTRY_SIZE;
 		ret = scan_its_table(its, l1_gpa, l1_tbl_size, l1_esz, 0,
-				     handle_l1_dte, NULL);
+				     handle_l1_dte, NULL, &valid, &last);
 	} else {
 		l1_esz = abi->dte_esz;
 		ret = scan_its_table(its, l1_gpa, l1_tbl_size, l1_esz, 0,
-				     vgic_its_restore_dte, NULL);
+				     vgic_its_restore_dte, NULL, &valid, &last);
 	}
 
-	if (ret > 0)
-		ret = -EINVAL;
+	if (!ret && valid && !last) {
+		/* a next element was referenced but not found */
+		return -EINVAL;
+	}
 
 	return ret;
 }
-- 
2.5.5

[PATCH v4 02/11] 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>

---

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 eea14a1..a4ff8f7 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2204,7 +2204,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;
@@ -2217,7 +2229,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);
@@ -2233,7 +2246,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;
 }
 
 /**
@@ -2278,8 +2291,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)
 {
@@ -2297,13 +2315,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 v4 03/11] 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

v2 -> v3:
- correct kerneldoc comment
---
 virt/kvm/arm/vgic/vgic-its.c | 55 +++++++++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 18 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index a4ff8f7..e59363e 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -687,15 +687,25 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
 	return 0;
 }
 
-/*
- * 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.
+ *
+ * @its: its handle
+ * @baser: GITS_BASER<n> register
+ * @id: id of the device/collection
+ * @eaddr: output gpa of the corresponding table entry
+ *
  * 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.
+ *
+ * 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)
@@ -2093,9 +2112,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 v4 03/11] 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

v2 -> v3:
- correct kerneldoc comment
---
 virt/kvm/arm/vgic/vgic-its.c | 55 +++++++++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 18 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index a4ff8f7..e59363e 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -687,15 +687,25 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
 	return 0;
 }
 
-/*
- * 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.
+ *
+ * @its: its handle
+ * @baser: GITS_BASER<n> register
+ * @id: id of the device/collection
+ * @eaddr: output gpa of the corresponding table entry
+ *
  * 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.
+ *
+ * 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)
@@ -2093,9 +2112,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 v4 04/11] 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>

---

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

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index e59363e..e61736b 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1488,6 +1488,16 @@ static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its,
 	its->enabled = !!(val & GITS_CTLR_ENABLE);
 
 	/*
+	 * It is UNPREDICTABLE to enable the ITS if any of the CBASER or
+	 * device/collection BASER are invalid
+	 */
+	if (its->enabled &&
+		(!(its->baser_device_table & GITS_BASER_VALID) ||
+		 !(its->baser_coll_table & GITS_BASER_VALID) ||
+		 !(its->cbaser && GITS_CBASER_VALID)))
+		its->enabled = false;
+
+	/*
 	 * Try to process any pending commands. This function bails out early
 	 * if the ITS is disabled or no commands have been queued.
 	 */
-- 
2.5.5

[PATCH v4 04/11] 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>

---

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

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index e59363e..e61736b 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1488,6 +1488,16 @@ static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its,
 	its->enabled = !!(val & GITS_CTLR_ENABLE);
 
 	/*
+	 * It is UNPREDICTABLE to enable the ITS if any of the CBASER or
+	 * device/collection BASER are invalid
+	 */
+	if (its->enabled &&
+		(!(its->baser_device_table & GITS_BASER_VALID) ||
+		 !(its->baser_coll_table & GITS_BASER_VALID) ||
+		 !(its->cbaser && GITS_CBASER_VALID)))
+		its->enabled = false;
+
+	/*
 	 * Try to process any pending commands. This function bails out early
 	 * if the ITS is disabled or no commands have been queued.
 	 */
-- 
2.5.5

[PATCH v4 05/11] 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>

---

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 e61736b..3b539d4 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2110,11 +2110,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);
 
@@ -2285,17 +2286,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);
@@ -2331,17 +2332,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 v4 06/11] 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 3b539d4..e18f1e4 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 v4 06/11] 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 3b539d4..e18f1e4 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 v4 07/11] KVM: arm/arm64: vgic-its: Always attempt to save/restore device and collection tables

[Eric Auger]

In case the device table save fails, we currently do not
attempt to save the collection table. However it may
happen that the device table fails because the structures
in memory are inconsistent with device GITS_BASER however
this does not mean collection backup can't and shouldn't
be performed. Same must happen on restore path.

Without this patch, after a reset and early state backup,
the device table restore may fail due to L1 entry not valid.
If we don't restore the collection table the guest does
not reboot properly.

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

---

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

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index e18f1e4..1c3e83f 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2381,12 +2381,9 @@ static int vgic_its_save_tables_v0(struct vgic_its *its)
 	}
 
 	ret = vgic_its_save_device_tables(its);
-	if (ret)
-		goto out;
 
-	ret = vgic_its_save_collection_table(its);
+	ret |= vgic_its_save_collection_table(its);
 
-out:
 	unlock_all_vcpus(kvm);
 	mutex_unlock(&its->its_lock);
 	mutex_unlock(&kvm->lock);
@@ -2413,11 +2410,9 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
 	}
 
 	ret = vgic_its_restore_collection_table(its);
-	if (ret)
-		goto out;
 
-	ret = vgic_its_restore_device_tables(its);
-out:
+	ret |= vgic_its_restore_device_tables(its);
+
 	unlock_all_vcpus(kvm);
 	mutex_unlock(&its->its_lock);
 	mutex_unlock(&kvm->lock);
-- 
2.5.5

[PATCH v4 07/11] KVM: arm/arm64: vgic-its: Always attempt to save/restore device and collection tables

[Eric Auger]

In case the device table save fails, we currently do not
attempt to save the collection table. However it may
happen that the device table fails because the structures
in memory are inconsistent with device GITS_BASER however
this does not mean collection backup can't and shouldn't
be performed. Same must happen on restore path.

Without this patch, after a reset and early state backup,
the device table restore may fail due to L1 entry not valid.
If we don't restore the collection table the guest does
not reboot properly.

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

---

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

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index e18f1e4..1c3e83f 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2381,12 +2381,9 @@ static int vgic_its_save_tables_v0(struct vgic_its *its)
 	}
 
 	ret = vgic_its_save_device_tables(its);
-	if (ret)
-		goto out;
 
-	ret = vgic_its_save_collection_table(its);
+	ret |= vgic_its_save_collection_table(its);
 
-out:
 	unlock_all_vcpus(kvm);
 	mutex_unlock(&its->its_lock);
 	mutex_unlock(&kvm->lock);
@@ -2413,11 +2410,9 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
 	}
 
 	ret = vgic_its_restore_collection_table(its);
-	if (ret)
-		goto out;
 
-	ret = vgic_its_restore_device_tables(its);
-out:
+	ret |= vgic_its_restore_device_tables(its);
+
 	unlock_all_vcpus(kvm);
 	mutex_unlock(&its->its_lock);
 	mutex_unlock(&kvm->lock);
-- 
2.5.5

[PATCH v4 08/11] 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. Indeed, the device list always is initialized
when vgic_its_destroy gets called: 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 1c3e83f..f3f0026f 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);
@@ -1644,46 +1683,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 v4 08/11] 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. Indeed, the device list always is initialized
when vgic_its_destroy gets called: 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 1c3e83f..f3f0026f 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);
@@ -1644,46 +1683,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 v4 09/11] 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>

---

v2 -> v3:
- add a comment and clear cache in if block
---
 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 f3f0026f..084239c 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;
+
+	/* Table no longer valid: clear cached data */
+	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 v4 09/11] 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>

---

v2 -> v3:
- add a comment and clear cache in if block
---
 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 f3f0026f..084239c 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;
+
+	/* Table no longer valid: clear cached data */
+	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 v4 10/11] 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>

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

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

diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
index eb06beb..4e9bb6f 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
@@ -33,6 +33,10 @@ Groups:
       request the initialization of the ITS, no additional parameter in
       kvm_device_attr.addr.
 
+    KVM_DEV_ARM_ITS_CTRL_RESET
+      reset the ITS, no additional parameter in kvm_device_attr.addr.
+      See "ITS Reset State" section.
+
     KVM_DEV_ARM_ITS_SAVE_TABLES
       save the ITS table data into guest RAM, at the location provisioned
       by the guest in corresponding registers/table entries.
@@ -157,3 +161,19 @@ Then vcpus can be started.
  - pINTID is the physical LPI ID; if zero, it means the entry is not valid
    and other fields are not meaningful.
  - ICID is the collection ID
+
+ ITS Reset State:
+ ----------------
+
+RESET returns the ITS to the same state that it was when first created and
+inited:
+
+- The ITS is not enabled and quiescent
+  GITS_CTLR.Enabled = 0 .Quiescent=1
+- There is no internally cache 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 v4 11/11] 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 084239c..9ceb348 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2457,6 +2457,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)
 {
@@ -2471,6 +2484,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:
@@ -2515,6 +2530,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

[PATCH v4 11/11] 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 084239c..9ceb348 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2457,6 +2457,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)
 {
@@ -2471,6 +2484,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:
@@ -2515,6 +2530,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 v4 01/11] KVM: arm/arm64: vgic-its: fix return value for device table restore

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:09:59AM +0200, Eric Auger wrote:
> AT the moment 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 fully revisits the errror handling while fixing those
> 2 bugs.
> 
> - entry_fn_t now takes a valid output paraleter

                                        parameter

> - scan_its_table() now returns <= 0 values and output 2 booleans,
                                                 outputs
>   valid and last.
> - vgic_its_restore_itt() now returns <= 0 values.
> - vgic_its_restore_device_tables() also returns <= 0 values.
> 
> With that patch we are able to properly handle the case where
> all data are invalid but we still are able to detect the case
> where a next entry was referenced by some valid entry and
> never found.
> 
> Fixes: 57a9a117154c93 (KVM: arm64: vgic-its: Device table save/restore)
> Fixes: eff484e0298da5 (KVM: arm64: vgic-its: ITT save and restore)
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
> 
> ---
> 
> need to CC stable
> 
> v3 -> v4:
> - set *valid at beginning of handle_l1_dte
> 
> v2 -> v3:
> - add comments
> - added valid parameter
> - 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 | 95 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 70 insertions(+), 25 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index f51c1e1..eea14a1 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1772,16 +1772,20 @@ static u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
>  
>  /**
>   * entry_fn_t - Callback called on a table entry restore path
> + *
>   * @its: its handle
>   * @id: id of the entry
>   * @entry: pointer to the entry
>   * @opaque: pointer to an opaque data
> + * @valid: indicates whether valid data is associated to this entry
> + * (the entry itself in case of linear table or entries in the next level,
> + * in case of hierachical tables)
>   *
>   * Return: < 0 on error, 0 if last element was identified, id offset to next
>   * element otherwise
>   */
>  typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
> -			  void *opaque);
> +			  void *opaque, bool *valid);
>  
>  /**
>   * scan_its_table - Scan a contiguous table in guest RAM and applies a function
> @@ -1794,29 +1798,34 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
>   * @start_id: the ID of the first entry in the table
>   * (non zero for 2d level tables)
>   * @fn: function to apply on each entry
> + * @opaque: opaque data passed to @fn
> + * @valid: indicates whether the table contains any valid data
> + * @last: returns whether the last valid entry was decoded
>   *
> - * Return: < 0 on error, 0 if last element was identified, 1 otherwise
> - * (the last element may not be found on second level tables)
> + * Return: < 0 on error, 0 on success
>   */
>  static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz,
> -			  int start_id, entry_fn_t fn, void *opaque)
> +			  int start_id, entry_fn_t fn, void *opaque,
> +			  bool *valid, bool *last)
>  {
>  	void *entry = kzalloc(esz, GFP_KERNEL);
>  	struct kvm *kvm = its->dev->kvm;
>  	unsigned long len = size;
>  	int id = start_id;
>  	gpa_t gpa = base;
> +	int next_offset = 0;
>  	int ret;
>  
>  	while (len > 0) {
> -		int next_offset;
>  		size_t byte_offset;
> +		bool entry_valid;
>  
>  		ret = kvm_read_guest(kvm, gpa, entry, esz);
>  		if (ret)
>  			goto out;
>  
> -		next_offset = fn(its, id, entry, opaque);
> +		next_offset = fn(its, id, entry, opaque, &entry_valid);
> +		*valid |= entry_valid;
>  		if (next_offset <= 0) {
>  			ret = next_offset;
>  			goto out;
> @@ -1827,9 +1836,15 @@ static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz,
>  		gpa += byte_offset;
>  		len -= byte_offset;
>  	}
> -	ret =  1;
> -
> +	/*
> +	 * the table lookup was completed without identifying the
> +	 * last valid entry (ie. next_offset > 0).
> +	 */

but you never set last to false?  If you require the caller to set the
variable to false, that should be documented, but it's weird.

> +	ret = 0;
>  out:
> +	if (!next_offset)
> +		*last = true;
> +

so if we scan the entire table to the end we won't set last?  Isn't that
a bit strange?

Also, if we can get id of the valid out parameter and instead handle
that within this function, I don't think you'll need the 'last' return
value in vgic_its_restore_device_tables, and you could make this:

	if (!next_offset && last)
		*last = true;
	else if (last)
		*last = false;

>  	kfree(entry);
>  	return ret;
>  }
> @@ -1854,12 +1869,14 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
>  
>  /**
>   * vgic_its_restore_ite - restore an interrupt translation entry
> + *
>   * @event_id: id used for indexing
>   * @ptr: pointer to the ITE entry
>   * @opaque: pointer to the its_device
> + * @valid: indicates whether the ite is valid
>   */
>  static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
> -				void *ptr, void *opaque)
> +				void *ptr, void *opaque, bool *valid)
>  {
>  	struct its_device *dev = (struct its_device *)opaque;
>  	struct its_collection *collection;
> @@ -1879,7 +1896,9 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
>  	coll_id = val & KVM_ITS_ITE_ICID_MASK;
>  	lpi_id = (val & KVM_ITS_ITE_PINTID_MASK) >> KVM_ITS_ITE_PINTID_SHIFT;
>  
> -	if (!lpi_id)
> +	*valid = !!lpi_id;
> +
> +	if (!*valid)
>  		return 1; /* invalid entry, no choice but to scan next entry */
>  
>  	if (lpi_id < VGIC_MIN_LPI)
> @@ -1940,6 +1959,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);
> @@ -1947,9 +1974,15 @@ static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
>  	int ret;
>  	int ite_esz = abi->ite_esz;
>  	size_t max_size = BIT_ULL(dev->num_eventid_bits) * ite_esz;
> +	bool valid = false, last = false;
>  
>  	ret = scan_its_table(its, base, max_size, ite_esz, 0,
> -			     vgic_its_restore_ite, dev);
> +			     vgic_its_restore_ite, dev, &valid, &last);
> +
> +	if (!ret && valid && !last) {
> +		/* a next element was referenced but not found */
> +		return -EINVAL;

So this is if we ever found a valid entry, but somehow it didn't lead us
to the last entry, right?  Can't you handle that within the
scan_its_table?

As I understand it, scan_its_table is in one of two modes, either it's
linearly scanning, or it found a valid entry, and it's jumping from one
entry to the next, given the offsets.  If it's in the second mode, and
finds an invalid entry, it should return an error.

I think you can also get rid of the '*valid = false; return 1;' thing,
which looks a bit strange.

> +	}
>  
>  	return ret;
>  }
> @@ -1985,29 +2018,29 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev,
>   * @id: device id the DTE corresponds to
>   * @ptr: kernel VA where the 8 byte DTE is located
>   * @opaque: unused
> + * @valid: indicates whether the dte is valid
>   *
>   * Return: < 0 on error, 0 if the dte is the last one, id offset to the
>   * next dte otherwise
>   */
>  static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
> -				void *ptr, void *opaque)
> +				void *ptr, void *opaque, bool *valid)
>  {
>  	struct its_device *dev;
>  	gpa_t itt_addr;
>  	u8 num_eventid_bits;
>  	u64 entry = *(u64 *)ptr;
> -	bool valid;
>  	u32 offset;
>  	int ret;
>  
>  	entry = le64_to_cpu(entry);
>  
> -	valid = entry >> KVM_ITS_DTE_VALID_SHIFT;
> +	*valid = entry >> KVM_ITS_DTE_VALID_SHIFT;
>  	num_eventid_bits = (entry & KVM_ITS_DTE_SIZE_MASK) + 1;
>  	itt_addr = ((entry & KVM_ITS_DTE_ITTADDR_MASK)
>  			>> KVM_ITS_DTE_ITTADDR_SHIFT) << 8;
>  
> -	if (!valid)
> +	if (!*valid)
>  		return 1;
>  
>  	/* dte entry is valid */
> @@ -2082,13 +2115,14 @@ static int vgic_its_save_device_tables(struct vgic_its *its)
>   * @id: index of the entry in the L1 table
>   * @addr: kernel VA
>   * @opaque: unused
> + * @valid: indicates whether any dte entry was found
>   *
>   * L1 table entries are scanned by steps of 1 entry
>   * Return < 0 if error, 0 if last dte was found when scanning the L2
>   * table, +1 otherwise (meaning next L1 entry must be scanned)
>   */
>  static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
> -			 void *opaque)
> +			 void *opaque, bool *valid)
>  {
>  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
>  	int l2_start_id = id * (SZ_64K / abi->dte_esz);
> @@ -2096,21 +2130,29 @@ static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
>  	int dte_esz = abi->dte_esz;
>  	gpa_t gpa;
>  	int ret;
> +	bool last;
>  
>  	entry = le64_to_cpu(entry);
>  
> -	if (!(entry & KVM_ITS_L1E_VALID_MASK))
> +	*valid = entry & KVM_ITS_L1E_VALID_MASK;
> +
> +	if (!*valid)
>  		return 1;
>  
>  	gpa = entry & KVM_ITS_L1E_ADDR_MASK;
>  
>  	ret = scan_its_table(its, gpa, SZ_64K, dte_esz,
> -			     l2_start_id, vgic_its_restore_dte, NULL);
> +			     l2_start_id, vgic_its_restore_dte, NULL,
> +			     valid, &last);
>  
> -	if (ret <= 0)
> -		return ret;
> +	/*
> +	 * if the last dte has not been found in this L2 table, we
> +	 * need to scan the next L1 entry
> +	 */
> +	if (!ret && !last)
> +		return 1;
>  
> -	return 1;
> +	return ret;
>  }
>  
>  /**
> @@ -2124,6 +2166,7 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
>  	int l1_esz, ret;
>  	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
>  	gpa_t l1_gpa;
> +	bool valid = false, last = false;
>  
>  	if (!(baser & GITS_BASER_VALID))
>  		return 0;
> @@ -2133,15 +2176,17 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
>  	if (baser & GITS_BASER_INDIRECT) {
>  		l1_esz = GITS_LVL1_ENTRY_SIZE;
>  		ret = scan_its_table(its, l1_gpa, l1_tbl_size, l1_esz, 0,
> -				     handle_l1_dte, NULL);
> +				     handle_l1_dte, NULL, &valid, &last);
>  	} else {
>  		l1_esz = abi->dte_esz;
>  		ret = scan_its_table(its, l1_gpa, l1_tbl_size, l1_esz, 0,
> -				     vgic_its_restore_dte, NULL);
> +				     vgic_its_restore_dte, NULL, &valid, &last);
>  	}
>  
> -	if (ret > 0)
> -		ret = -EINVAL;
> +	if (!ret && valid && !last) {
> +		/* a next element was referenced but not found */
> +		return -EINVAL;
> +	}

Same comment as above.

>  
>  	return ret;
>  }
> -- 
> 2.5.5
> 
Thanks,
-Christoffer

Re: [PATCH v4 02/11] KVM: arm/arm64: vgic-its: fix vgic_its_restore_collection_table returned value

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:00AM +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>
> 
> ---
> 
> 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 eea14a1..a4ff8f7 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2204,7 +2204,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;
> @@ -2217,7 +2229,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);
> @@ -2233,7 +2246,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;
>  }
>  
>  /**
> @@ -2278,8 +2291,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)
>  {
> @@ -2297,13 +2315,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
> 

It's not that I find this looking particularly elegant, but I'm not sure
I have a better alternative, perhaps it's just the natur of the beast:

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

Re: [PATCH v4 03/11] KVM: arm/arm64: vgic-its: Improve error reporting on device table save

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:01AM +0200, Eric Auger wrote:
> 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
> 

I think this commit message fails to explain the crucial bit, which is
that a guest can somehow create a situation that brings down the VM when
trying to migrate the VM, which we shouldn't allow.

Can you explain what case that is, and how that is then solved?

(I can't tell from looking at this patch if the EINVAL or the EFAULT are
the things that userspace can just ignore with a warning or, and which
is the one that really should crash the VM and why).

> 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

I'm still not convinced that this is a candidate for stable:

Documentation/process/stable-kernel-rules.rst

It's about error codes and is pretty large, has documentation changes
etc.

Thanks,
-Christoffer

> 
> v2 -> v3:
> - correct kerneldoc comment
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 55 +++++++++++++++++++++++++++++---------------
>  1 file changed, 37 insertions(+), 18 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index a4ff8f7..e59363e 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -687,15 +687,25 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
>  	return 0;
>  }
>  
> -/*
> - * 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.
> + *
> + * @its: its handle
> + * @baser: GITS_BASER<n> register
> + * @id: id of the device/collection
> + * @eaddr: output gpa of the corresponding table entry
> + *
>   * 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.
> + *
> + * 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)
> @@ -2093,9 +2112,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
> 

Re: [PATCH v4 03/11] KVM: arm/arm64: vgic-its: Improve error reporting on device table save

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:01AM +0200, Eric Auger wrote:
> 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
> 

I think this commit message fails to explain the crucial bit, which is
that a guest can somehow create a situation that brings down the VM when
trying to migrate the VM, which we shouldn't allow.

Can you explain what case that is, and how that is then solved?

(I can't tell from looking at this patch if the EINVAL or the EFAULT are
the things that userspace can just ignore with a warning or, and which
is the one that really should crash the VM and why).

> 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

I'm still not convinced that this is a candidate for stable:

Documentation/process/stable-kernel-rules.rst

It's about error codes and is pretty large, has documentation changes
etc.

Thanks,
-Christoffer

> 
> v2 -> v3:
> - correct kerneldoc comment
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 55 +++++++++++++++++++++++++++++---------------
>  1 file changed, 37 insertions(+), 18 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index a4ff8f7..e59363e 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -687,15 +687,25 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
>  	return 0;
>  }
>  
> -/*
> - * 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.
> + *
> + * @its: its handle
> + * @baser: GITS_BASER<n> register
> + * @id: id of the device/collection
> + * @eaddr: output gpa of the corresponding table entry
> + *
>   * 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.
> + *
> + * 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)
> @@ -2093,9 +2112,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
> 

Re: [PATCH v4 04/11] KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:02AM +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>
> 
> ---
> 
> v3: creation
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index e59363e..e61736b 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1488,6 +1488,16 @@ static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its,
>  	its->enabled = !!(val & GITS_CTLR_ENABLE);
>  
>  	/*
> +	 * It is UNPREDICTABLE to enable the ITS if any of the CBASER or
> +	 * device/collection BASER are invalid
> +	 */
> +	if (its->enabled &&
> +		(!(its->baser_device_table & GITS_BASER_VALID) ||
> +		 !(its->baser_coll_table & GITS_BASER_VALID) ||
> +		 !(its->cbaser && GITS_CBASER_VALID)))
> +		its->enabled = false;
> +

uh, why don't we check these conditions before we enable the ITS instead
of enabling, checking, and then disabling?


Thanks,
-Christoffer

> +	/*
>  	 * Try to process any pending commands. This function bails out early
>  	 * if the ITS is disabled or no commands have been queued.
>  	 */
> -- 
> 2.5.5
> 

Re: [PATCH v4 04/11] KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:02AM +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>
> 
> ---
> 
> v3: creation
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index e59363e..e61736b 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1488,6 +1488,16 @@ static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its,
>  	its->enabled = !!(val & GITS_CTLR_ENABLE);
>  
>  	/*
> +	 * It is UNPREDICTABLE to enable the ITS if any of the CBASER or
> +	 * device/collection BASER are invalid
> +	 */
> +	if (its->enabled &&
> +		(!(its->baser_device_table & GITS_BASER_VALID) ||
> +		 !(its->baser_coll_table & GITS_BASER_VALID) ||
> +		 !(its->cbaser && GITS_CBASER_VALID)))

whoops, you wanted &, not && in the last one I think.

> +		its->enabled = false;
> +
> +	/*
>  	 * Try to process any pending commands. This function bails out early
>  	 * if the ITS is disabled or no commands have been queued.
>  	 */
> -- 
> 2.5.5
> 

Re: [PATCH v4 04/11] KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:02AM +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>
> 
> ---
> 
> v3: creation
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index e59363e..e61736b 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -1488,6 +1488,16 @@ static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its,
>  	its->enabled = !!(val & GITS_CTLR_ENABLE);
>  
>  	/*
> +	 * It is UNPREDICTABLE to enable the ITS if any of the CBASER or
> +	 * device/collection BASER are invalid
> +	 */
> +	if (its->enabled &&
> +		(!(its->baser_device_table & GITS_BASER_VALID) ||
> +		 !(its->baser_coll_table & GITS_BASER_VALID) ||
> +		 !(its->cbaser && GITS_CBASER_VALID)))

whoops, you wanted &, not && in the last one I think.

> +		its->enabled = false;
> +
> +	/*
>  	 * Try to process any pending commands. This function bails out early
>  	 * if the ITS is disabled or no commands have been queued.
>  	 */
> -- 
> 2.5.5
> 

Re: [PATCH v4 06/11] KVM: arm/arm64: vgic-its: Check GITS_CBASER validity before processing commands

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:04AM +0200, Eric Auger wrote:
> 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 3b539d4..e18f1e4 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)))

Is it valid to clear the CBASER valid bit after having enabled the ITS?
If not, I think you should check changes to CBASER and then this
shouldn't be necessary.



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

Otherwise, this looks fine.

Thanks,
-Christoffer

Re: [PATCH v4 06/11] KVM: arm/arm64: vgic-its: Check GITS_CBASER validity before processing commands

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:04AM +0200, Eric Auger wrote:
> 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 3b539d4..e18f1e4 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)))

Is it valid to clear the CBASER valid bit after having enabled the ITS?
If not, I think you should check changes to CBASER and then this
shouldn't be necessary.



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

Otherwise, this looks fine.

Thanks,
-Christoffer

Re: [PATCH v4 07/11] KVM: arm/arm64: vgic-its: Always attempt to save/restore device and collection tables

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:05AM +0200, Eric Auger wrote:
> In case the device table save fails, we currently do not
> attempt to save the collection table. However it may
> happen that the device table fails because the structures
> in memory are inconsistent with device GITS_BASER however
> this does not mean collection backup can't and shouldn't
> be performed. Same must happen on restore path.
> 
> Without this patch, after a reset and early state backup,
> the device table restore may fail due to L1 entry not valid.
> If we don't restore the collection table the guest does
> not reboot properly.

I don't really understand.  After the previous patches, why would a
properly configured ITS return an error in its_save_device_tables?

If that's not possible, are we not trying to support partially migrating
half-way broken state, and is that something we care about?
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> candidate to be CC'ed stable
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index e18f1e4..1c3e83f 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2381,12 +2381,9 @@ static int vgic_its_save_tables_v0(struct vgic_its *its)
>  	}
>  
>  	ret = vgic_its_save_device_tables(its);
> -	if (ret)
> -		goto out;
>  
> -	ret = vgic_its_save_collection_table(its);
> +	ret |= vgic_its_save_collection_table(its);

What if the two functions return two different error codes, is the
binary OR of these error codes going to tell userspace anything
meaningful?


>  
> -out:
>  	unlock_all_vcpus(kvm);
>  	mutex_unlock(&its->its_lock);
>  	mutex_unlock(&kvm->lock);
> @@ -2413,11 +2410,9 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
>  	}
>  
>  	ret = vgic_its_restore_collection_table(its);
> -	if (ret)
> -		goto out;
>  
> -	ret = vgic_its_restore_device_tables(its);
> -out:
> +	ret |= vgic_its_restore_device_tables(its);
> +
>  	unlock_all_vcpus(kvm);
>  	mutex_unlock(&its->its_lock);
>  	mutex_unlock(&kvm->lock);
> -- 
> 2.5.5
> 

Thanks,
-Christoffer

Re: [PATCH v4 07/11] KVM: arm/arm64: vgic-its: Always attempt to save/restore device and collection tables

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:05AM +0200, Eric Auger wrote:
> In case the device table save fails, we currently do not
> attempt to save the collection table. However it may
> happen that the device table fails because the structures
> in memory are inconsistent with device GITS_BASER however
> this does not mean collection backup can't and shouldn't
> be performed. Same must happen on restore path.
> 
> Without this patch, after a reset and early state backup,
> the device table restore may fail due to L1 entry not valid.
> If we don't restore the collection table the guest does
> not reboot properly.

I don't really understand.  After the previous patches, why would a
properly configured ITS return an error in its_save_device_tables?

If that's not possible, are we not trying to support partially migrating
half-way broken state, and is that something we care about?
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> candidate to be CC'ed stable
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index e18f1e4..1c3e83f 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -2381,12 +2381,9 @@ static int vgic_its_save_tables_v0(struct vgic_its *its)
>  	}
>  
>  	ret = vgic_its_save_device_tables(its);
> -	if (ret)
> -		goto out;
>  
> -	ret = vgic_its_save_collection_table(its);
> +	ret |= vgic_its_save_collection_table(its);

What if the two functions return two different error codes, is the
binary OR of these error codes going to tell userspace anything
meaningful?


>  
> -out:
>  	unlock_all_vcpus(kvm);
>  	mutex_unlock(&its->its_lock);
>  	mutex_unlock(&kvm->lock);
> @@ -2413,11 +2410,9 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
>  	}
>  
>  	ret = vgic_its_restore_collection_table(its);
> -	if (ret)
> -		goto out;
>  
> -	ret = vgic_its_restore_device_tables(its);
> -out:
> +	ret |= vgic_its_restore_device_tables(its);
> +
>  	unlock_all_vcpus(kvm);
>  	mutex_unlock(&its->its_lock);
>  	mutex_unlock(&kvm->lock);
> -- 
> 2.5.5
> 

Thanks,
-Christoffer

Re: [PATCH v4 08/11] KVM: arm/arm64: vgic-its: new helper functions to free the caches

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:06AM +0200, Eric Auger wrote:
> 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.

See my previous comments about language issues in this paragraph.

> 
> We also remove the check on its->device_list.next as it looks
> unnecessary. Indeed, the device list always is initialized
> when vgic_its_destroy gets called: 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 1c3e83f..f3f0026f 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);
> @@ -1644,46 +1683,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;

Hmm, I feel like we managed to convince ourselves this was needed
before.

Andre, can you remember what your original rationale was here?

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

If we're really sure the original check was just a misunderstanding,
then this patch looks ok, given the fixes to the commit message.

Thanks,
-Christoffer

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

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:07AM +0200, Eric Auger 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>
> 
> ---
> 
> v2 -> v3:
> - add a comment and clear cache in if block
> ---
>  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 f3f0026f..084239c 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;
> +
> +	/* Table no longer valid: clear cached data */
> +	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;
> +		}
> +	}

So we do this after setting the *regptr, which makes we worried about
races.

How are guest writes to these registers synchronized with, for example
trying to save the tables.  Perhaps we don't care because userspace
should have stopped the VM before trying to save the ITS state?



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

Otherwise looks good to me.
-Christoffer

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

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:07AM +0200, Eric Auger 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>
> 
> ---
> 
> v2 -> v3:
> - add a comment and clear cache in if block
> ---
>  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 f3f0026f..084239c 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;
> +
> +	/* Table no longer valid: clear cached data */
> +	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;
> +		}
> +	}

So we do this after setting the *regptr, which makes we worried about
races.

How are guest writes to these registers synchronized with, for example
trying to save the tables.  Perhaps we don't care because userspace
should have stopped the VM before trying to save the ITS state?



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

Otherwise looks good to me.
-Christoffer

Re: [PATCH v4 10/11] KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL_RESET

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:08AM +0200, Eric Auger wrote:
> At the moment, the in-kernel emulated ITS is not properly reset.
> On guest restart/reset some registers keep their old values and
> internal structures like device, ITE, and collection lists are not
> freed.
> 
> This may lead to various bugs. Among them, we can have incorrect state
> backup or failure when saving the ITS state at early guest boot stage.
> 
> This patch documents a new attribute, KVM_DEV_ARM_ITS_CTRL_RESET in
> the KVM_DEV_ARM_VGIC_GRP_CTRL group.
> 
> Upon this action, we can reset registers and especially those
> pointing to tables previously allocated by the guest and free
> the internal data structures storing the list of devices, collections
> and lpis.
> 
> The usual approach for device reset of having userspace write
> the reset values of the registers to the kernel via the register
> read/write APIs doesn't work for the ITS because it has some
> internal state (caches) which is not exposed as registers,
> and there is no register interface for "drop cached data without
> writing it back to RAM". So we need a KVM API which mimics the
> hardware's reset line, to provide the equivalent behaviour to
> a "pull the power cord out of the back of the machine" reset.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
> 
> ---
> v2 -> v3:
> - reword commit message, credit to Peter Maydell.
> - take into account Christoffer rewording comments but still
>   kept details. Added Peter's comment but still kept details.
>   Peter may disagree.
> 
> v1 -> v2:
> - Describe architecturally-defined reset values
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> index eb06beb..4e9bb6f 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> @@ -33,6 +33,10 @@ Groups:
>        request the initialization of the ITS, no additional parameter in
>        kvm_device_attr.addr.
>  
> +    KVM_DEV_ARM_ITS_CTRL_RESET
> +      reset the ITS, no additional parameter in kvm_device_attr.addr.
> +      See "ITS Reset State" section.
> +
>      KVM_DEV_ARM_ITS_SAVE_TABLES
>        save the ITS table data into guest RAM, at the location provisioned
>        by the guest in corresponding registers/table entries.
> @@ -157,3 +161,19 @@ Then vcpus can be started.
>   - pINTID is the physical LPI ID; if zero, it means the entry is not valid
>     and other fields are not meaningful.
>   - ICID is the collection ID
> +
> + ITS Reset State:
> + ----------------
> +
> +RESET returns the ITS to the same state that it was when first created and
> +inited:

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 cache state

internally cached,  or internal cache

> +- 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
> 
With those changes, this looks ok to me.

Thanks,
-Christoffer

Re: [PATCH v4 10/11] KVM: arm/arm64: Document KVM_DEV_ARM_ITS_CTRL_RESET

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:08AM +0200, Eric Auger wrote:
> At the moment, the in-kernel emulated ITS is not properly reset.
> On guest restart/reset some registers keep their old values and
> internal structures like device, ITE, and collection lists are not
> freed.
> 
> This may lead to various bugs. Among them, we can have incorrect state
> backup or failure when saving the ITS state at early guest boot stage.
> 
> This patch documents a new attribute, KVM_DEV_ARM_ITS_CTRL_RESET in
> the KVM_DEV_ARM_VGIC_GRP_CTRL group.
> 
> Upon this action, we can reset registers and especially those
> pointing to tables previously allocated by the guest and free
> the internal data structures storing the list of devices, collections
> and lpis.
> 
> The usual approach for device reset of having userspace write
> the reset values of the registers to the kernel via the register
> read/write APIs doesn't work for the ITS because it has some
> internal state (caches) which is not exposed as registers,
> and there is no register interface for "drop cached data without
> writing it back to RAM". So we need a KVM API which mimics the
> hardware's reset line, to provide the equivalent behaviour to
> a "pull the power cord out of the back of the machine" reset.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
> 
> ---
> v2 -> v3:
> - reword commit message, credit to Peter Maydell.
> - take into account Christoffer rewording comments but still
>   kept details. Added Peter's comment but still kept details.
>   Peter may disagree.
> 
> v1 -> v2:
> - Describe architecturally-defined reset values
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> index eb06beb..4e9bb6f 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> @@ -33,6 +33,10 @@ Groups:
>        request the initialization of the ITS, no additional parameter in
>        kvm_device_attr.addr.
>  
> +    KVM_DEV_ARM_ITS_CTRL_RESET
> +      reset the ITS, no additional parameter in kvm_device_attr.addr.
> +      See "ITS Reset State" section.
> +
>      KVM_DEV_ARM_ITS_SAVE_TABLES
>        save the ITS table data into guest RAM, at the location provisioned
>        by the guest in corresponding registers/table entries.
> @@ -157,3 +161,19 @@ Then vcpus can be started.
>   - pINTID is the physical LPI ID; if zero, it means the entry is not valid
>     and other fields are not meaningful.
>   - ICID is the collection ID
> +
> + ITS Reset State:
> + ----------------
> +
> +RESET returns the ITS to the same state that it was when first created and
> +inited:

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 cache state

internally cached,  or internal cache

> +- 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
> 
With those changes, this looks ok to me.

Thanks,
-Christoffer

Re: [PATCH v4 06/11] KVM: arm/arm64: vgic-its: Check GITS_CBASER validity before processing commands

[Auger Eric]

Hi Christoffer,

On 18/10/2017 00:10, Christoffer Dall wrote:
> On Tue, Oct 17, 2017 at 09:10:04AM +0200, Eric Auger wrote:
>> 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 3b539d4..e18f1e4 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)))
> 
> Is it valid to clear the CBASER valid bit after having enabled the ITS?
> If not, I think you should check changes to CBASER and then this
> shouldn't be necessary.

Yes you're right. the spec says:
When GITS_CTLR.Enable == 1 or GITS_CTLR.Quiescent == 0, writing this
register (CBASER) is UNPREDICTABLE .

Given we already check its->enabled in vgic_mmio_write_its_cbaser, I
think it is safe to remove this patch.

Thanks

Eric
> 
> 
> 
>>  		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
>>
> 
> Otherwise, this looks fine.
> 
> Thanks,
> -Christoffer
> 

Re: [PATCH v4 06/11] KVM: arm/arm64: vgic-its: Check GITS_CBASER validity before processing commands

[Auger Eric]

Hi Christoffer,

On 18/10/2017 00:10, Christoffer Dall wrote:
> On Tue, Oct 17, 2017 at 09:10:04AM +0200, Eric Auger wrote:
>> 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 3b539d4..e18f1e4 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)))
> 
> Is it valid to clear the CBASER valid bit after having enabled the ITS?
> If not, I think you should check changes to CBASER and then this
> shouldn't be necessary.

Yes you're right. the spec says:
When GITS_CTLR.Enable == 1 or GITS_CTLR.Quiescent == 0, writing this
register (CBASER) is UNPREDICTABLE .

Given we already check its->enabled in vgic_mmio_write_its_cbaser, I
think it is safe to remove this patch.

Thanks

Eric
> 
> 
> 
>>  		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
>>
> 
> Otherwise, this looks fine.
> 
> Thanks,
> -Christoffer
> 

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

[Auger Eric]

Hi Christoffer,

On 18/10/2017 00:34, Christoffer Dall wrote:
> On Tue, Oct 17, 2017 at 09:10:07AM +0200, Eric Auger 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>
>>
>> ---
>>
>> v2 -> v3:
>> - add a comment and clear cache in if block
>> ---
>>  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 f3f0026f..084239c 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;
>> +
>> +	/* Table no longer valid: clear cached data */
>> +	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;
>> +		}
>> +	}
> 
> So we do this after setting the *regptr, which makes we worried about
> races.
> 
> How are guest writes to these registers synchronized with, for example
> trying to save the tables.  Perhaps we don't care because userspace
> should have stopped the VM before trying to save the ITS state?

Yes save & restore can happen only if all vcpus are locked. Same for
user space accesses.

Thanks

Eric

> 
> 
> 
>>  }
>>  
>>  static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu,
>> -- 
>> 2.5.5
>>
> 
> Otherwise looks good to me.
> -Christoffer
> 

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

[Auger Eric]

Hi Christoffer,

On 18/10/2017 00:34, Christoffer Dall wrote:
> On Tue, Oct 17, 2017 at 09:10:07AM +0200, Eric Auger 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>
>>
>> ---
>>
>> v2 -> v3:
>> - add a comment and clear cache in if block
>> ---
>>  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 f3f0026f..084239c 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;
>> +
>> +	/* Table no longer valid: clear cached data */
>> +	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;
>> +		}
>> +	}
> 
> So we do this after setting the *regptr, which makes we worried about
> races.
> 
> How are guest writes to these registers synchronized with, for example
> trying to save the tables.  Perhaps we don't care because userspace
> should have stopped the VM before trying to save the ITS state?

Yes save & restore can happen only if all vcpus are locked. Same for
user space accesses.

Thanks

Eric

> 
> 
> 
>>  }
>>  
>>  static unsigned long vgic_mmio_read_its_ctlr(struct kvm *vcpu,
>> -- 
>> 2.5.5
>>
> 
> Otherwise looks good to me.
> -Christoffer
> 

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

[Christoffer Dall]

On Sat, Oct 21, 2017 at 12:13:21PM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 18/10/2017 00:34, Christoffer Dall wrote:
> > On Tue, Oct 17, 2017 at 09:10:07AM +0200, Eric Auger 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>
> >>
> >> ---
> >>
> >> v2 -> v3:
> >> - add a comment and clear cache in if block
> >> ---
> >>  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 f3f0026f..084239c 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;
> >> +
> >> +	/* Table no longer valid: clear cached data */
> >> +	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;
> >> +		}
> >> +	}
> > 
> > So we do this after setting the *regptr, which makes we worried about
> > races.
> > 
> > How are guest writes to these registers synchronized with, for example
> > trying to save the tables.  Perhaps we don't care because userspace
> > should have stopped the VM before trying to save the ITS state?
> 
> Yes save & restore can happen only if all vcpus are locked. Same for
> user space accesses.
> 

So this cannot happens, because the save/restore operation will fail to
get the lock of an executing VCPU which is modifying this function?

Thanks,
-Christoffer

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

[Christoffer Dall]

On Sat, Oct 21, 2017 at 12:13:21PM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 18/10/2017 00:34, Christoffer Dall wrote:
> > On Tue, Oct 17, 2017 at 09:10:07AM +0200, Eric Auger 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>
> >>
> >> ---
> >>
> >> v2 -> v3:
> >> - add a comment and clear cache in if block
> >> ---
> >>  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 f3f0026f..084239c 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;
> >> +
> >> +	/* Table no longer valid: clear cached data */
> >> +	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;
> >> +		}
> >> +	}
> > 
> > So we do this after setting the *regptr, which makes we worried about
> > races.
> > 
> > How are guest writes to these registers synchronized with, for example
> > trying to save the tables.  Perhaps we don't care because userspace
> > should have stopped the VM before trying to save the ITS state?
> 
> Yes save & restore can happen only if all vcpus are locked. Same for
> user space accesses.
> 

So this cannot happens, because the save/restore operation will fail to
get the lock of an executing VCPU which is modifying this function?

Thanks,
-Christoffer

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

[Auger Eric]

Hi Christoffer,

On 21/10/2017 16:31, Christoffer Dall wrote:
> On Sat, Oct 21, 2017 at 12:13:21PM +0200, Auger Eric wrote:
>> Hi Christoffer,
>>
>> On 18/10/2017 00:34, Christoffer Dall wrote:
>>> On Tue, Oct 17, 2017 at 09:10:07AM +0200, Eric Auger 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>
>>>>
>>>> ---
>>>>
>>>> v2 -> v3:
>>>> - add a comment and clear cache in if block
>>>> ---
>>>>  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 f3f0026f..084239c 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;
>>>> +
>>>> +	/* Table no longer valid: clear cached data */
>>>> +	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;
>>>> +		}
>>>> +	}
>>>
>>> So we do this after setting the *regptr, which makes we worried about
>>> races.
>>>
>>> How are guest writes to these registers synchronized with, for example
>>> trying to save the tables.  Perhaps we don't care because userspace
>>> should have stopped the VM before trying to save the ITS state?
>>
>> Yes save & restore can happen only if all vcpus are locked. Same for
>> user space accesses.
>>
> 
> So this cannot happens, because the save/restore operation will fail to
> get the lock of an executing VCPU which is modifying this function?

Yes that's my understanding.

Thanks

Eric
> 
> Thanks,
> -Christoffer
> 

Re: [PATCH v4 01/11] KVM: arm/arm64: vgic-its: fix return value for device table restore

[Auger Eric]

Hi Christoffer,

On 17/10/2017 23:40, Christoffer Dall wrote:
> On Tue, Oct 17, 2017 at 09:09:59AM +0200, Eric Auger wrote:
>> AT the moment 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 fully revisits the errror handling while fixing those
>> 2 bugs.
>>
>> - entry_fn_t now takes a valid output paraleter
> 
>                                         parameter
> 
>> - scan_its_table() now returns <= 0 values and output 2 booleans,
>                                                  outputs
>>   valid and last.
>> - vgic_its_restore_itt() now returns <= 0 values.
>> - vgic_its_restore_device_tables() also returns <= 0 values.
>>
>> With that patch we are able to properly handle the case where
>> all data are invalid but we still are able to detect the case
>> where a next entry was referenced by some valid entry and
>> never found.
>>
>> Fixes: 57a9a117154c93 (KVM: arm64: vgic-its: Device table save/restore)
>> Fixes: eff484e0298da5 (KVM: arm64: vgic-its: ITT save and restore)
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
>>
>> ---
>>
>> need to CC stable
>>
>> v3 -> v4:
>> - set *valid at beginning of handle_l1_dte
>>
>> v2 -> v3:
>> - add comments
>> - added valid parameter
>> - 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 | 95 ++++++++++++++++++++++++++++++++------------
>>  1 file changed, 70 insertions(+), 25 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index f51c1e1..eea14a1 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -1772,16 +1772,20 @@ static u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
>>  
>>  /**
>>   * entry_fn_t - Callback called on a table entry restore path
>> + *
>>   * @its: its handle
>>   * @id: id of the entry
>>   * @entry: pointer to the entry
>>   * @opaque: pointer to an opaque data
>> + * @valid: indicates whether valid data is associated to this entry
>> + * (the entry itself in case of linear table or entries in the next level,
>> + * in case of hierachical tables)
>>   *
>>   * Return: < 0 on error, 0 if last element was identified, id offset to next
>>   * element otherwise
>>   */
>>  typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
>> -			  void *opaque);
>> +			  void *opaque, bool *valid);
>>  
>>  /**
>>   * scan_its_table - Scan a contiguous table in guest RAM and applies a function
>> @@ -1794,29 +1798,34 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
>>   * @start_id: the ID of the first entry in the table
>>   * (non zero for 2d level tables)
>>   * @fn: function to apply on each entry
>> + * @opaque: opaque data passed to @fn
>> + * @valid: indicates whether the table contains any valid data
>> + * @last: returns whether the last valid entry was decoded
>>   *
>> - * Return: < 0 on error, 0 if last element was identified, 1 otherwise
>> - * (the last element may not be found on second level tables)
>> + * Return: < 0 on error, 0 on success
>>   */
>>  static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz,
>> -			  int start_id, entry_fn_t fn, void *opaque)
>> +			  int start_id, entry_fn_t fn, void *opaque,
>> +			  bool *valid, bool *last)
>>  {
>>  	void *entry = kzalloc(esz, GFP_KERNEL);
>>  	struct kvm *kvm = its->dev->kvm;
>>  	unsigned long len = size;
>>  	int id = start_id;
>>  	gpa_t gpa = base;
>> +	int next_offset = 0;
>>  	int ret;
>>  
>>  	while (len > 0) {
>> -		int next_offset;
>>  		size_t byte_offset;
>> +		bool entry_valid;
>>  
>>  		ret = kvm_read_guest(kvm, gpa, entry, esz);
>>  		if (ret)
>>  			goto out;
>>  
>> -		next_offset = fn(its, id, entry, opaque);
>> +		next_offset = fn(its, id, entry, opaque, &entry_valid);
>> +		*valid |= entry_valid;
>>  		if (next_offset <= 0) {
>>  			ret = next_offset;
>>  			goto out;
>> @@ -1827,9 +1836,15 @@ static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz,
>>  		gpa += byte_offset;
>>  		len -= byte_offset;
>>  	}
>> -	ret =  1;
>> -
>> +	/*
>> +	 * the table lookup was completed without identifying the
>> +	 * last valid entry (ie. next_offset > 0).
>> +	 */
> 
> but you never set last to false?  If you require the caller to set the
> variable to false, that should be documented, but it's weird.
> 
>> +	ret = 0;
>>  out:
>> +	if (!next_offset)
>> +		*last = true;
>> +
> 
> so if we scan the entire table to the end we won't set last?  Isn't that
> a bit strange?
> 
> Also, if we can get id of the valid out parameter and instead handle
> that within this function, I don't think you'll need the 'last' return
> value in vgic_its_restore_device_tables, and you could make this:
> 
> 	if (!next_offset && last)
> 		*last = true;
> 	else if (last)
> 		*last = false;
> 
>>  	kfree(entry);
>>  	return ret;
>>  }
>> @@ -1854,12 +1869,14 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
>>  
>>  /**
>>   * vgic_its_restore_ite - restore an interrupt translation entry
>> + *
>>   * @event_id: id used for indexing
>>   * @ptr: pointer to the ITE entry
>>   * @opaque: pointer to the its_device
>> + * @valid: indicates whether the ite is valid
>>   */
>>  static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
>> -				void *ptr, void *opaque)
>> +				void *ptr, void *opaque, bool *valid)
>>  {
>>  	struct its_device *dev = (struct its_device *)opaque;
>>  	struct its_collection *collection;
>> @@ -1879,7 +1896,9 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
>>  	coll_id = val & KVM_ITS_ITE_ICID_MASK;
>>  	lpi_id = (val & KVM_ITS_ITE_PINTID_MASK) >> KVM_ITS_ITE_PINTID_SHIFT;
>>  
>> -	if (!lpi_id)
>> +	*valid = !!lpi_id;
>> +
>> +	if (!*valid)
>>  		return 1; /* invalid entry, no choice but to scan next entry */
>>  
>>  	if (lpi_id < VGIC_MIN_LPI)
>> @@ -1940,6 +1959,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);
>> @@ -1947,9 +1974,15 @@ static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
>>  	int ret;
>>  	int ite_esz = abi->ite_esz;
>>  	size_t max_size = BIT_ULL(dev->num_eventid_bits) * ite_esz;
>> +	bool valid = false, last = false;
>>  
>>  	ret = scan_its_table(its, base, max_size, ite_esz, 0,
>> -			     vgic_its_restore_ite, dev);
>> +			     vgic_its_restore_ite, dev, &valid, &last);
>> +
>> +	if (!ret && valid && !last) {
>> +		/* a next element was referenced but not found */
>> +		return -EINVAL;
> 
> So this is if we ever found a valid entry, but somehow it didn't lead us
> to the last entry, right?  Can't you handle that within the
> scan_its_table?
> 
> As I understand it, scan_its_table is in one of two modes, either it's
> linearly scanning, or it found a valid entry, and it's jumping from one
> entry to the next, given the offsets.  If it's in the second mode, and
> finds an invalid entry, it should return an error.
> 
> I think you can also get rid of the '*valid = false; return 1;' thing,
> which looks a bit strange.

Given the number of changes this rework will produce I guess this patch
wouldn't be candidate for cc'ed stable. Then shouldn't we consider to
first apply the fix proposed by Wanghaibin (cc'ed stable) and then apply
the rework in a second and subsequent patch?

Thanks

Eric
> 
>> +	}
>>  
>>  	return ret;
>>  }
>> @@ -1985,29 +2018,29 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev,
>>   * @id: device id the DTE corresponds to
>>   * @ptr: kernel VA where the 8 byte DTE is located
>>   * @opaque: unused
>> + * @valid: indicates whether the dte is valid
>>   *
>>   * Return: < 0 on error, 0 if the dte is the last one, id offset to the
>>   * next dte otherwise
>>   */
>>  static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
>> -				void *ptr, void *opaque)
>> +				void *ptr, void *opaque, bool *valid)
>>  {
>>  	struct its_device *dev;
>>  	gpa_t itt_addr;
>>  	u8 num_eventid_bits;
>>  	u64 entry = *(u64 *)ptr;
>> -	bool valid;
>>  	u32 offset;
>>  	int ret;
>>  
>>  	entry = le64_to_cpu(entry);
>>  
>> -	valid = entry >> KVM_ITS_DTE_VALID_SHIFT;
>> +	*valid = entry >> KVM_ITS_DTE_VALID_SHIFT;
>>  	num_eventid_bits = (entry & KVM_ITS_DTE_SIZE_MASK) + 1;
>>  	itt_addr = ((entry & KVM_ITS_DTE_ITTADDR_MASK)
>>  			>> KVM_ITS_DTE_ITTADDR_SHIFT) << 8;
>>  
>> -	if (!valid)
>> +	if (!*valid)
>>  		return 1;
>>  
>>  	/* dte entry is valid */
>> @@ -2082,13 +2115,14 @@ static int vgic_its_save_device_tables(struct vgic_its *its)
>>   * @id: index of the entry in the L1 table
>>   * @addr: kernel VA
>>   * @opaque: unused
>> + * @valid: indicates whether any dte entry was found
>>   *
>>   * L1 table entries are scanned by steps of 1 entry
>>   * Return < 0 if error, 0 if last dte was found when scanning the L2
>>   * table, +1 otherwise (meaning next L1 entry must be scanned)
>>   */
>>  static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
>> -			 void *opaque)
>> +			 void *opaque, bool *valid)
>>  {
>>  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
>>  	int l2_start_id = id * (SZ_64K / abi->dte_esz);
>> @@ -2096,21 +2130,29 @@ static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
>>  	int dte_esz = abi->dte_esz;
>>  	gpa_t gpa;
>>  	int ret;
>> +	bool last;
>>  
>>  	entry = le64_to_cpu(entry);
>>  
>> -	if (!(entry & KVM_ITS_L1E_VALID_MASK))
>> +	*valid = entry & KVM_ITS_L1E_VALID_MASK;
>> +
>> +	if (!*valid)
>>  		return 1;
>>  
>>  	gpa = entry & KVM_ITS_L1E_ADDR_MASK;
>>  
>>  	ret = scan_its_table(its, gpa, SZ_64K, dte_esz,
>> -			     l2_start_id, vgic_its_restore_dte, NULL);
>> +			     l2_start_id, vgic_its_restore_dte, NULL,
>> +			     valid, &last);
>>  
>> -	if (ret <= 0)
>> -		return ret;
>> +	/*
>> +	 * if the last dte has not been found in this L2 table, we
>> +	 * need to scan the next L1 entry
>> +	 */
>> +	if (!ret && !last)
>> +		return 1;
>>  
>> -	return 1;
>> +	return ret;
>>  }
>>  
>>  /**
>> @@ -2124,6 +2166,7 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
>>  	int l1_esz, ret;
>>  	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
>>  	gpa_t l1_gpa;
>> +	bool valid = false, last = false;
>>  
>>  	if (!(baser & GITS_BASER_VALID))
>>  		return 0;
>> @@ -2133,15 +2176,17 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
>>  	if (baser & GITS_BASER_INDIRECT) {
>>  		l1_esz = GITS_LVL1_ENTRY_SIZE;
>>  		ret = scan_its_table(its, l1_gpa, l1_tbl_size, l1_esz, 0,
>> -				     handle_l1_dte, NULL);
>> +				     handle_l1_dte, NULL, &valid, &last);
>>  	} else {
>>  		l1_esz = abi->dte_esz;
>>  		ret = scan_its_table(its, l1_gpa, l1_tbl_size, l1_esz, 0,
>> -				     vgic_its_restore_dte, NULL);
>> +				     vgic_its_restore_dte, NULL, &valid, &last);
>>  	}
>>  
>> -	if (ret > 0)
>> -		ret = -EINVAL;
>> +	if (!ret && valid && !last) {
>> +		/* a next element was referenced but not found */
>> +		return -EINVAL;
>> +	}
> 
> Same comment as above.
> 
>>  
>>  	return ret;
>>  }
>> -- 
>> 2.5.5
>>
> Thanks,
> -Christoffer
> 

Re: [PATCH v4 01/11] KVM: arm/arm64: vgic-its: fix return value for device table restore

[Auger Eric]

Hi Christoffer,

On 17/10/2017 23:40, Christoffer Dall wrote:
> On Tue, Oct 17, 2017 at 09:09:59AM +0200, Eric Auger wrote:
>> AT the moment 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 fully revisits the errror handling while fixing those
>> 2 bugs.
>>
>> - entry_fn_t now takes a valid output paraleter
> 
>                                         parameter
> 
>> - scan_its_table() now returns <= 0 values and output 2 booleans,
>                                                  outputs
>>   valid and last.
>> - vgic_its_restore_itt() now returns <= 0 values.
>> - vgic_its_restore_device_tables() also returns <= 0 values.
>>
>> With that patch we are able to properly handle the case where
>> all data are invalid but we still are able to detect the case
>> where a next entry was referenced by some valid entry and
>> never found.
>>
>> Fixes: 57a9a117154c93 (KVM: arm64: vgic-its: Device table save/restore)
>> Fixes: eff484e0298da5 (KVM: arm64: vgic-its: ITT save and restore)
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
>>
>> ---
>>
>> need to CC stable
>>
>> v3 -> v4:
>> - set *valid at beginning of handle_l1_dte
>>
>> v2 -> v3:
>> - add comments
>> - added valid parameter
>> - 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 | 95 ++++++++++++++++++++++++++++++++------------
>>  1 file changed, 70 insertions(+), 25 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index f51c1e1..eea14a1 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -1772,16 +1772,20 @@ static u32 compute_next_eventid_offset(struct list_head *h, struct its_ite *ite)
>>  
>>  /**
>>   * entry_fn_t - Callback called on a table entry restore path
>> + *
>>   * @its: its handle
>>   * @id: id of the entry
>>   * @entry: pointer to the entry
>>   * @opaque: pointer to an opaque data
>> + * @valid: indicates whether valid data is associated to this entry
>> + * (the entry itself in case of linear table or entries in the next level,
>> + * in case of hierachical tables)
>>   *
>>   * Return: < 0 on error, 0 if last element was identified, id offset to next
>>   * element otherwise
>>   */
>>  typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
>> -			  void *opaque);
>> +			  void *opaque, bool *valid);
>>  
>>  /**
>>   * scan_its_table - Scan a contiguous table in guest RAM and applies a function
>> @@ -1794,29 +1798,34 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
>>   * @start_id: the ID of the first entry in the table
>>   * (non zero for 2d level tables)
>>   * @fn: function to apply on each entry
>> + * @opaque: opaque data passed to @fn
>> + * @valid: indicates whether the table contains any valid data
>> + * @last: returns whether the last valid entry was decoded
>>   *
>> - * Return: < 0 on error, 0 if last element was identified, 1 otherwise
>> - * (the last element may not be found on second level tables)
>> + * Return: < 0 on error, 0 on success
>>   */
>>  static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz,
>> -			  int start_id, entry_fn_t fn, void *opaque)
>> +			  int start_id, entry_fn_t fn, void *opaque,
>> +			  bool *valid, bool *last)
>>  {
>>  	void *entry = kzalloc(esz, GFP_KERNEL);
>>  	struct kvm *kvm = its->dev->kvm;
>>  	unsigned long len = size;
>>  	int id = start_id;
>>  	gpa_t gpa = base;
>> +	int next_offset = 0;
>>  	int ret;
>>  
>>  	while (len > 0) {
>> -		int next_offset;
>>  		size_t byte_offset;
>> +		bool entry_valid;
>>  
>>  		ret = kvm_read_guest(kvm, gpa, entry, esz);
>>  		if (ret)
>>  			goto out;
>>  
>> -		next_offset = fn(its, id, entry, opaque);
>> +		next_offset = fn(its, id, entry, opaque, &entry_valid);
>> +		*valid |= entry_valid;
>>  		if (next_offset <= 0) {
>>  			ret = next_offset;
>>  			goto out;
>> @@ -1827,9 +1836,15 @@ static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz,
>>  		gpa += byte_offset;
>>  		len -= byte_offset;
>>  	}
>> -	ret =  1;
>> -
>> +	/*
>> +	 * the table lookup was completed without identifying the
>> +	 * last valid entry (ie. next_offset > 0).
>> +	 */
> 
> but you never set last to false?  If you require the caller to set the
> variable to false, that should be documented, but it's weird.
> 
>> +	ret = 0;
>>  out:
>> +	if (!next_offset)
>> +		*last = true;
>> +
> 
> so if we scan the entire table to the end we won't set last?  Isn't that
> a bit strange?
> 
> Also, if we can get id of the valid out parameter and instead handle
> that within this function, I don't think you'll need the 'last' return
> value in vgic_its_restore_device_tables, and you could make this:
> 
> 	if (!next_offset && last)
> 		*last = true;
> 	else if (last)
> 		*last = false;
> 
>>  	kfree(entry);
>>  	return ret;
>>  }
>> @@ -1854,12 +1869,14 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
>>  
>>  /**
>>   * vgic_its_restore_ite - restore an interrupt translation entry
>> + *
>>   * @event_id: id used for indexing
>>   * @ptr: pointer to the ITE entry
>>   * @opaque: pointer to the its_device
>> + * @valid: indicates whether the ite is valid
>>   */
>>  static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
>> -				void *ptr, void *opaque)
>> +				void *ptr, void *opaque, bool *valid)
>>  {
>>  	struct its_device *dev = (struct its_device *)opaque;
>>  	struct its_collection *collection;
>> @@ -1879,7 +1896,9 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
>>  	coll_id = val & KVM_ITS_ITE_ICID_MASK;
>>  	lpi_id = (val & KVM_ITS_ITE_PINTID_MASK) >> KVM_ITS_ITE_PINTID_SHIFT;
>>  
>> -	if (!lpi_id)
>> +	*valid = !!lpi_id;
>> +
>> +	if (!*valid)
>>  		return 1; /* invalid entry, no choice but to scan next entry */
>>  
>>  	if (lpi_id < VGIC_MIN_LPI)
>> @@ -1940,6 +1959,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);
>> @@ -1947,9 +1974,15 @@ static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
>>  	int ret;
>>  	int ite_esz = abi->ite_esz;
>>  	size_t max_size = BIT_ULL(dev->num_eventid_bits) * ite_esz;
>> +	bool valid = false, last = false;
>>  
>>  	ret = scan_its_table(its, base, max_size, ite_esz, 0,
>> -			     vgic_its_restore_ite, dev);
>> +			     vgic_its_restore_ite, dev, &valid, &last);
>> +
>> +	if (!ret && valid && !last) {
>> +		/* a next element was referenced but not found */
>> +		return -EINVAL;
> 
> So this is if we ever found a valid entry, but somehow it didn't lead us
> to the last entry, right?  Can't you handle that within the
> scan_its_table?
> 
> As I understand it, scan_its_table is in one of two modes, either it's
> linearly scanning, or it found a valid entry, and it's jumping from one
> entry to the next, given the offsets.  If it's in the second mode, and
> finds an invalid entry, it should return an error.
> 
> I think you can also get rid of the '*valid = false; return 1;' thing,
> which looks a bit strange.

Given the number of changes this rework will produce I guess this patch
wouldn't be candidate for cc'ed stable. Then shouldn't we consider to
first apply the fix proposed by Wanghaibin (cc'ed stable) and then apply
the rework in a second and subsequent patch?

Thanks

Eric
> 
>> +	}
>>  
>>  	return ret;
>>  }
>> @@ -1985,29 +2018,29 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev,
>>   * @id: device id the DTE corresponds to
>>   * @ptr: kernel VA where the 8 byte DTE is located
>>   * @opaque: unused
>> + * @valid: indicates whether the dte is valid
>>   *
>>   * Return: < 0 on error, 0 if the dte is the last one, id offset to the
>>   * next dte otherwise
>>   */
>>  static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
>> -				void *ptr, void *opaque)
>> +				void *ptr, void *opaque, bool *valid)
>>  {
>>  	struct its_device *dev;
>>  	gpa_t itt_addr;
>>  	u8 num_eventid_bits;
>>  	u64 entry = *(u64 *)ptr;
>> -	bool valid;
>>  	u32 offset;
>>  	int ret;
>>  
>>  	entry = le64_to_cpu(entry);
>>  
>> -	valid = entry >> KVM_ITS_DTE_VALID_SHIFT;
>> +	*valid = entry >> KVM_ITS_DTE_VALID_SHIFT;
>>  	num_eventid_bits = (entry & KVM_ITS_DTE_SIZE_MASK) + 1;
>>  	itt_addr = ((entry & KVM_ITS_DTE_ITTADDR_MASK)
>>  			>> KVM_ITS_DTE_ITTADDR_SHIFT) << 8;
>>  
>> -	if (!valid)
>> +	if (!*valid)
>>  		return 1;
>>  
>>  	/* dte entry is valid */
>> @@ -2082,13 +2115,14 @@ static int vgic_its_save_device_tables(struct vgic_its *its)
>>   * @id: index of the entry in the L1 table
>>   * @addr: kernel VA
>>   * @opaque: unused
>> + * @valid: indicates whether any dte entry was found
>>   *
>>   * L1 table entries are scanned by steps of 1 entry
>>   * Return < 0 if error, 0 if last dte was found when scanning the L2
>>   * table, +1 otherwise (meaning next L1 entry must be scanned)
>>   */
>>  static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
>> -			 void *opaque)
>> +			 void *opaque, bool *valid)
>>  {
>>  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
>>  	int l2_start_id = id * (SZ_64K / abi->dte_esz);
>> @@ -2096,21 +2130,29 @@ static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
>>  	int dte_esz = abi->dte_esz;
>>  	gpa_t gpa;
>>  	int ret;
>> +	bool last;
>>  
>>  	entry = le64_to_cpu(entry);
>>  
>> -	if (!(entry & KVM_ITS_L1E_VALID_MASK))
>> +	*valid = entry & KVM_ITS_L1E_VALID_MASK;
>> +
>> +	if (!*valid)
>>  		return 1;
>>  
>>  	gpa = entry & KVM_ITS_L1E_ADDR_MASK;
>>  
>>  	ret = scan_its_table(its, gpa, SZ_64K, dte_esz,
>> -			     l2_start_id, vgic_its_restore_dte, NULL);
>> +			     l2_start_id, vgic_its_restore_dte, NULL,
>> +			     valid, &last);
>>  
>> -	if (ret <= 0)
>> -		return ret;
>> +	/*
>> +	 * if the last dte has not been found in this L2 table, we
>> +	 * need to scan the next L1 entry
>> +	 */
>> +	if (!ret && !last)
>> +		return 1;
>>  
>> -	return 1;
>> +	return ret;
>>  }
>>  
>>  /**
>> @@ -2124,6 +2166,7 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
>>  	int l1_esz, ret;
>>  	int l1_tbl_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
>>  	gpa_t l1_gpa;
>> +	bool valid = false, last = false;
>>  
>>  	if (!(baser & GITS_BASER_VALID))
>>  		return 0;
>> @@ -2133,15 +2176,17 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
>>  	if (baser & GITS_BASER_INDIRECT) {
>>  		l1_esz = GITS_LVL1_ENTRY_SIZE;
>>  		ret = scan_its_table(its, l1_gpa, l1_tbl_size, l1_esz, 0,
>> -				     handle_l1_dte, NULL);
>> +				     handle_l1_dte, NULL, &valid, &last);
>>  	} else {
>>  		l1_esz = abi->dte_esz;
>>  		ret = scan_its_table(its, l1_gpa, l1_tbl_size, l1_esz, 0,
>> -				     vgic_its_restore_dte, NULL);
>> +				     vgic_its_restore_dte, NULL, &valid, &last);
>>  	}
>>  
>> -	if (ret > 0)
>> -		ret = -EINVAL;
>> +	if (!ret && valid && !last) {
>> +		/* a next element was referenced but not found */
>> +		return -EINVAL;
>> +	}
> 
> Same comment as above.
> 
>>  
>>  	return ret;
>>  }
>> -- 
>> 2.5.5
>>
> Thanks,
> -Christoffer
> 

Re: [PATCH v4 01/11] KVM: arm/arm64: vgic-its: fix return value for device table restore

[Christoffer Dall]

On Sat, Oct 21, 2017 at 04:40:00PM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 17/10/2017 23:40, Christoffer Dall wrote:
> > On Tue, Oct 17, 2017 at 09:09:59AM +0200, Eric Auger wrote:
> >> AT the moment 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 fully revisits the errror handling while fixing those
> >> 2 bugs.
> >>
> >> - entry_fn_t now takes a valid output paraleter
> > 
> >                                         parameter
> > 
> >> - scan_its_table() now returns <= 0 values and output 2 booleans,
> >                                                  outputs
> >>   valid and last.
> >> - vgic_its_restore_itt() now returns <= 0 values.
> >> - vgic_its_restore_device_tables() also returns <= 0 values.
> >>
> >> With that patch we are able to properly handle the case where
> >> all data are invalid but we still are able to detect the case
> >> where a next entry was referenced by some valid entry and
> >> never found.
> >>
> >> Fixes: 57a9a117154c93 (KVM: arm64: vgic-its: Device table save/restore)
> >> Fixes: eff484e0298da5 (KVM: arm64: vgic-its: ITT save and restore)
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >> Reported-by: wanghaibin <wanghaibin.wang@huawei.com>
> >>
> >> ---
> >>
> >> need to CC stable
> >>
> >> v3 -> v4:
> >> - set *valid at beginning of handle_l1_dte
> >>
> >> v2 -> v3:
> >> - add comments
> >> - added valid parameter
> >> - vgic_its_restore_itt don't return +1 anymore
> >> - reword the commit message
> >>
> >> v1 -> v2:
> >> - if (ret > 0) ret = 0
> >> ---

[...]

> 
> Given the number of changes this rework will produce I guess this patch
> wouldn't be candidate for cc'ed stable. Then shouldn't we consider to
> first apply the fix proposed by Wanghaibin (cc'ed stable) and then apply
> the rework in a second and subsequent patch?
> 

Yes, probably, let's keep the fix small and obviously correct and get
that in for v4.14 ASAP.

Then we can do rework for v4.15 or later, to clean things up.

Thanks,
-Christoffer

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

[Christoffer Dall]

On Sat, Oct 21, 2017 at 04:36:22PM +0200, Auger Eric wrote:
> Hi Christoffer,
> 
> On 21/10/2017 16:31, Christoffer Dall wrote:
> > On Sat, Oct 21, 2017 at 12:13:21PM +0200, Auger Eric wrote:
> >> Hi Christoffer,
> >>
> >> On 18/10/2017 00:34, Christoffer Dall wrote:
> >>> On Tue, Oct 17, 2017 at 09:10:07AM +0200, Eric Auger 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>
> >>>>
> >>>> ---
> >>>>
> >>>> v2 -> v3:
> >>>> - add a comment and clear cache in if block
> >>>> ---
> >>>>  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 f3f0026f..084239c 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;
> >>>> +
> >>>> +	/* Table no longer valid: clear cached data */
> >>>> +	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;
> >>>> +		}
> >>>> +	}
> >>>
> >>> So we do this after setting the *regptr, which makes we worried about
> >>> races.
> >>>
> >>> How are guest writes to these registers synchronized with, for example
> >>> trying to save the tables.  Perhaps we don't care because userspace
> >>> should have stopped the VM before trying to save the ITS state?
> >>
> >> Yes save & restore can happen only if all vcpus are locked. Same for
> >> user space accesses.
> >>
> > 
> > So this cannot happens, because the save/restore operation will fail to
> > get the lock of an executing VCPU which is modifying this function?
> 
> Yes that's my understanding.
> 

ok.  If you respin, putting a comment here may be worth it.

Thanks,
-Christoffer