[PATCH 0/4] KVM: arm64: vgic: Misc ITS fixes

[PATCH 0/4] KVM: arm64: vgic: Misc ITS fixes

[Ricardo Koller]

The purpose of this series is to help debugging failed ITS saves and
restores.  Failures can be due to misconfiguration on the guest side:
tables with bogus base addresses, or the guest overwriting L1 indirect
tables. KVM can't do much in these cases, but one thing it can do to help
is raising errors as soon as possible. Here are a couple of cases where
KVM could do more:

- A command that adds an entry into an ITS table that is not in guest
  memory should fail, as any command should be treated as if it was
  actually saving entries in guest memory (KVM doesn't until saving).  KVM
  does this check for collections and devices (using vgic_its_check_id),
  but it doesn't for the ITT (Interrupt Translation Table). Commit #1 adds
  the missing check.

- Restoring the ITS tables does some checks for corrupted tables, but not
  as many as in a save.  For example, a device ID overflowing the table
  will be detected on save but not on restore.  The consequence is that
  restoring a corrupted table won't be detected until the next save;
  including the ITS not working as expected after the restore. As an
  example, if the guest sets tables overlapping each other, this would most
  likely result in some corrupted table; and this is what we would see from
  the host point of view:

	guest sets bogus baser addresses
	save ioctl
	restore ioctl
	save ioctl (fails)

  This failed save could happen many days after the first operation, so it
  would be hard to track down. Commit #2 adds some checks into restore:
  like checking that the ITE entries are not repeated.

- Restoring a corrupted collection entry is being ignored. Commit #3 fixes
  this while trying to organize the code so to make the bug more obvious
  next time.

Finally, failed restores should clean up all intermediate state. Commit #4
takes care of cleaning up everything created until the restore was deemed a
failure.

Tested with kvm-unit-tests ITS tests.

Ricardo Koller (4):
  KVM: arm64: vgic: Check that new ITEs could be saved in guest memory
  KVM: arm64: vgic: Add more checks when restoring ITS tables
  KVM: arm64: vgic: Do not ignore vgic_its_restore_cte failures
  KVM: arm64: vgic: Undo work in failed ITS restores

 arch/arm64/kvm/vgic/vgic-its.c | 91 ++++++++++++++++++++++++++++++----
 1 file changed, 80 insertions(+), 11 deletions(-)

-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH 0/4] KVM: arm64: vgic: Misc ITS fixes

[Ricardo Koller]

The purpose of this series is to help debugging failed ITS saves and
restores.  Failures can be due to misconfiguration on the guest side:
tables with bogus base addresses, or the guest overwriting L1 indirect
tables. KVM can't do much in these cases, but one thing it can do to help
is raising errors as soon as possible. Here are a couple of cases where
KVM could do more:

- A command that adds an entry into an ITS table that is not in guest
  memory should fail, as any command should be treated as if it was
  actually saving entries in guest memory (KVM doesn't until saving).  KVM
  does this check for collections and devices (using vgic_its_check_id),
  but it doesn't for the ITT (Interrupt Translation Table). Commit #1 adds
  the missing check.

- Restoring the ITS tables does some checks for corrupted tables, but not
  as many as in a save.  For example, a device ID overflowing the table
  will be detected on save but not on restore.  The consequence is that
  restoring a corrupted table won't be detected until the next save;
  including the ITS not working as expected after the restore. As an
  example, if the guest sets tables overlapping each other, this would most
  likely result in some corrupted table; and this is what we would see from
  the host point of view:

	guest sets bogus baser addresses
	save ioctl
	restore ioctl
	save ioctl (fails)

  This failed save could happen many days after the first operation, so it
  would be hard to track down. Commit #2 adds some checks into restore:
  like checking that the ITE entries are not repeated.

- Restoring a corrupted collection entry is being ignored. Commit #3 fixes
  this while trying to organize the code so to make the bug more obvious
  next time.

Finally, failed restores should clean up all intermediate state. Commit #4
takes care of cleaning up everything created until the restore was deemed a
failure.

Tested with kvm-unit-tests ITS tests.

Ricardo Koller (4):
  KVM: arm64: vgic: Check that new ITEs could be saved in guest memory
  KVM: arm64: vgic: Add more checks when restoring ITS tables
  KVM: arm64: vgic: Do not ignore vgic_its_restore_cte failures
  KVM: arm64: vgic: Undo work in failed ITS restores

 arch/arm64/kvm/vgic/vgic-its.c | 91 ++++++++++++++++++++++++++++++----
 1 file changed, 80 insertions(+), 11 deletions(-)

-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 1/4] KVM: arm64: vgic: Check that new ITEs could be saved in guest memory

[Ricardo Koller]

A command that adds an entry into an ITS table that is not in guest
memory should fail, as any command should be treated as if it was
actually saving entries in guest memory (KVM doesn't until saving).
Add the corresponding check for the ITT when adding ITEs.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/vgic/vgic-its.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 2e13402be3bd..d7c1a3a01af4 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -976,6 +976,37 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
 	return ret;
 }
 
+/*
+ * Check whether an event ID can be stored in the corresponding Interrupt
+ * Translation Table, which starts at device->itt_addr.
+ */
+static bool vgic_its_check_ite(struct vgic_its *its, struct its_device *device,
+		u32 event_id)
+{
+	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
+	int ite_esz = abi->ite_esz;
+	gpa_t gpa;
+	gfn_t gfn;
+	int idx;
+	bool ret;
+
+	/* max table size is: BIT_ULL(device->num_eventid_bits) * ite_esz */
+	if (event_id >= BIT_ULL(device->num_eventid_bits))
+		return false;
+
+	gpa = device->itt_addr + event_id * ite_esz;
+	gfn = gpa >> PAGE_SHIFT;
+
+	idx = srcu_read_lock(&its->dev->kvm->srcu);
+	ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
+	srcu_read_unlock(&its->dev->kvm->srcu, idx);
+	return ret;
+}
+
+/*
+ * Adds a new collection into the ITS collection table.
+ * Returns 0 on success, and a negative error value for generic errors.
+ */
 static int vgic_its_alloc_collection(struct vgic_its *its,
 				     struct its_collection **colp,
 				     u32 coll_id)
@@ -1076,6 +1107,9 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	if (find_ite(its, device_id, event_id))
 		return 0;
 
+	if (!vgic_its_check_ite(its, device, event_id))
+		return E_ITS_MAPTI_ID_OOR;
+
 	collection = find_collection(its, coll_id);
 	if (!collection) {
 		int ret = vgic_its_alloc_collection(its, &collection, coll_id);
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH 1/4] KVM: arm64: vgic: Check that new ITEs could be saved in guest memory

[Ricardo Koller]

A command that adds an entry into an ITS table that is not in guest
memory should fail, as any command should be treated as if it was
actually saving entries in guest memory (KVM doesn't until saving).
Add the corresponding check for the ITT when adding ITEs.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/vgic/vgic-its.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 2e13402be3bd..d7c1a3a01af4 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -976,6 +976,37 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
 	return ret;
 }
 
+/*
+ * Check whether an event ID can be stored in the corresponding Interrupt
+ * Translation Table, which starts at device->itt_addr.
+ */
+static bool vgic_its_check_ite(struct vgic_its *its, struct its_device *device,
+		u32 event_id)
+{
+	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
+	int ite_esz = abi->ite_esz;
+	gpa_t gpa;
+	gfn_t gfn;
+	int idx;
+	bool ret;
+
+	/* max table size is: BIT_ULL(device->num_eventid_bits) * ite_esz */
+	if (event_id >= BIT_ULL(device->num_eventid_bits))
+		return false;
+
+	gpa = device->itt_addr + event_id * ite_esz;
+	gfn = gpa >> PAGE_SHIFT;
+
+	idx = srcu_read_lock(&its->dev->kvm->srcu);
+	ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
+	srcu_read_unlock(&its->dev->kvm->srcu, idx);
+	return ret;
+}
+
+/*
+ * Adds a new collection into the ITS collection table.
+ * Returns 0 on success, and a negative error value for generic errors.
+ */
 static int vgic_its_alloc_collection(struct vgic_its *its,
 				     struct its_collection **colp,
 				     u32 coll_id)
@@ -1076,6 +1107,9 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 	if (find_ite(its, device_id, event_id))
 		return 0;
 
+	if (!vgic_its_check_ite(its, device, event_id))
+		return E_ITS_MAPTI_ID_OOR;
+
 	collection = find_collection(its, coll_id);
 	if (!collection) {
 		int ret = vgic_its_alloc_collection(its, &collection, coll_id);
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 2/4] KVM: arm64: vgic: Add more checks when restoring ITS tables

[Ricardo Koller]

Restoring corrupted ITS tables could lead to a misbehaving ITS, and
possibly a failed ITS save as the save performs more checks than the
restore. Add sanity checks when restoring DTEs and ITEs.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/vgic/vgic-its.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index d7c1a3a01af4..dfd73fa1ed43 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -2209,6 +2209,12 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
 	if (!collection)
 		return -EINVAL;
 
+	if (find_ite(its, dev->device_id, event_id))
+		return -EINVAL;
+
+	if (!vgic_its_check_ite(its, dev, event_id))
+		return -EINVAL;
+
 	ite = vgic_its_alloc_ite(dev, collection, event_id);
 	if (IS_ERR(ite))
 		return PTR_ERR(ite);
@@ -2330,6 +2336,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
 				void *ptr, void *opaque)
 {
 	struct its_device *dev;
+	u64 baser = its->baser_device_table;
 	gpa_t itt_addr;
 	u8 num_eventid_bits;
 	u64 entry = *(u64 *)ptr;
@@ -2350,6 +2357,12 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
 	/* dte entry is valid */
 	offset = (entry & KVM_ITS_DTE_NEXT_MASK) >> KVM_ITS_DTE_NEXT_SHIFT;
 
+	if (find_its_device(its, id))
+		return -EINVAL;
+
+	if (!vgic_its_check_id(its, baser, id, NULL))
+		return -EINVAL;
+
 	dev = vgic_its_alloc_device(its, id, itt_addr, num_eventid_bits);
 	if (IS_ERR(dev))
 		return PTR_ERR(dev);
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH 2/4] KVM: arm64: vgic: Add more checks when restoring ITS tables

[Ricardo Koller]

Restoring corrupted ITS tables could lead to a misbehaving ITS, and
possibly a failed ITS save as the save performs more checks than the
restore. Add sanity checks when restoring DTEs and ITEs.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/vgic/vgic-its.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index d7c1a3a01af4..dfd73fa1ed43 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -2209,6 +2209,12 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
 	if (!collection)
 		return -EINVAL;
 
+	if (find_ite(its, dev->device_id, event_id))
+		return -EINVAL;
+
+	if (!vgic_its_check_ite(its, dev, event_id))
+		return -EINVAL;
+
 	ite = vgic_its_alloc_ite(dev, collection, event_id);
 	if (IS_ERR(ite))
 		return PTR_ERR(ite);
@@ -2330,6 +2336,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
 				void *ptr, void *opaque)
 {
 	struct its_device *dev;
+	u64 baser = its->baser_device_table;
 	gpa_t itt_addr;
 	u8 num_eventid_bits;
 	u64 entry = *(u64 *)ptr;
@@ -2350,6 +2357,12 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
 	/* dte entry is valid */
 	offset = (entry & KVM_ITS_DTE_NEXT_MASK) >> KVM_ITS_DTE_NEXT_SHIFT;
 
+	if (find_its_device(its, id))
+		return -EINVAL;
+
+	if (!vgic_its_check_id(its, baser, id, NULL))
+		return -EINVAL;
+
 	dev = vgic_its_alloc_device(its, id, itt_addr, num_eventid_bits);
 	if (IS_ERR(dev))
 		return PTR_ERR(dev);
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 3/4] KVM: arm64: vgic: Do not ignore vgic_its_restore_cte failures

[Ricardo Koller]

Restoring a corrupted collection entry is being ignored: a
vgic_its_restore_cte failure is treated as success by
vgic_its_restore_collection_table.  vgic_its_restore_cte uses a positive
number to return ITS error codes, and +1 to return success.  The caller
then uses "ret > 0" to check for success. An additional issue is that
invalid entries return 0 and although that doesn't fail the restore, it
leads to skipping all the next entries.

Fix this by having vgic_its_restore_cte return negative numbers on
error, and 0 on success (which includes skipping an invalid entry).
While doing that, also fix alloc_collection return codes to not mix ITS
error codes (positive numbers) and generic error codes (negative
numbers).

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/vgic/vgic-its.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index dfd73fa1ed43..4ece649e2493 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -1013,9 +1013,6 @@ 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))
-		return E_ITS_MAPC_COLLECTION_OOR;
-
 	collection = kzalloc(sizeof(*collection), GFP_KERNEL_ACCOUNT);
 	if (!collection)
 		return -ENOMEM;
@@ -1112,7 +1109,12 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 
 	collection = find_collection(its, coll_id);
 	if (!collection) {
-		int ret = vgic_its_alloc_collection(its, &collection, coll_id);
+		int ret;
+
+		if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
+			return E_ITS_MAPC_COLLECTION_OOR;
+
+		ret = vgic_its_alloc_collection(its, &collection, coll_id);
 		if (ret)
 			return ret;
 		new_coll = collection;
@@ -1267,6 +1269,10 @@ static int vgic_its_cmd_handle_mapc(struct kvm *kvm, struct vgic_its *its,
 		if (!collection) {
 			int ret;
 
+			if (!vgic_its_check_id(its, its->baser_coll_table,
+						coll_id, NULL))
+				return E_ITS_MAPC_COLLECTION_OOR;
+
 			ret = vgic_its_alloc_collection(its, &collection,
 							coll_id);
 			if (ret)
@@ -2508,6 +2514,10 @@ static int vgic_its_save_cte(struct vgic_its *its,
 	return kvm_write_guest_lock(its->dev->kvm, gpa, &val, esz);
 }
 
+/*
+ * Restores a collection entry into the ITS collection table.
+ * Returns 0 on success, and a negative error value for generic errors.
+ */
 static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
 {
 	struct its_collection *collection;
@@ -2522,7 +2532,7 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
 		return ret;
 	val = le64_to_cpu(val);
 	if (!(val & KVM_ITS_CTE_VALID_MASK))
-		return 0;
+		return 0; /* invalid entry, skip it */
 
 	target_addr = (u32)(val >> KVM_ITS_CTE_RDBASE_SHIFT);
 	coll_id = val & KVM_ITS_CTE_ICID_MASK;
@@ -2534,11 +2544,15 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
 	collection = find_collection(its, coll_id);
 	if (collection)
 		return -EEXIST;
+
+	if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
+		return -EINVAL;
+
 	ret = vgic_its_alloc_collection(its, &collection, coll_id);
 	if (ret)
 		return ret;
 	collection->target_addr = target_addr;
-	return 1;
+	return 0;
 }
 
 /**
@@ -2604,15 +2618,12 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
 
 	while (read < max_size) {
 		ret = vgic_its_restore_cte(its, gpa, cte_esz);
-		if (ret <= 0)
+		if (ret < 0)
 			break;
 		gpa += cte_esz;
 		read += cte_esz;
 	}
 
-	if (ret > 0)
-		return 0;
-
 	return ret;
 }
 
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH 3/4] KVM: arm64: vgic: Do not ignore vgic_its_restore_cte failures

[Ricardo Koller]

Restoring a corrupted collection entry is being ignored: a
vgic_its_restore_cte failure is treated as success by
vgic_its_restore_collection_table.  vgic_its_restore_cte uses a positive
number to return ITS error codes, and +1 to return success.  The caller
then uses "ret > 0" to check for success. An additional issue is that
invalid entries return 0 and although that doesn't fail the restore, it
leads to skipping all the next entries.

Fix this by having vgic_its_restore_cte return negative numbers on
error, and 0 on success (which includes skipping an invalid entry).
While doing that, also fix alloc_collection return codes to not mix ITS
error codes (positive numbers) and generic error codes (negative
numbers).

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/vgic/vgic-its.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index dfd73fa1ed43..4ece649e2493 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -1013,9 +1013,6 @@ 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))
-		return E_ITS_MAPC_COLLECTION_OOR;
-
 	collection = kzalloc(sizeof(*collection), GFP_KERNEL_ACCOUNT);
 	if (!collection)
 		return -ENOMEM;
@@ -1112,7 +1109,12 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
 
 	collection = find_collection(its, coll_id);
 	if (!collection) {
-		int ret = vgic_its_alloc_collection(its, &collection, coll_id);
+		int ret;
+
+		if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
+			return E_ITS_MAPC_COLLECTION_OOR;
+
+		ret = vgic_its_alloc_collection(its, &collection, coll_id);
 		if (ret)
 			return ret;
 		new_coll = collection;
@@ -1267,6 +1269,10 @@ static int vgic_its_cmd_handle_mapc(struct kvm *kvm, struct vgic_its *its,
 		if (!collection) {
 			int ret;
 
+			if (!vgic_its_check_id(its, its->baser_coll_table,
+						coll_id, NULL))
+				return E_ITS_MAPC_COLLECTION_OOR;
+
 			ret = vgic_its_alloc_collection(its, &collection,
 							coll_id);
 			if (ret)
@@ -2508,6 +2514,10 @@ static int vgic_its_save_cte(struct vgic_its *its,
 	return kvm_write_guest_lock(its->dev->kvm, gpa, &val, esz);
 }
 
+/*
+ * Restores a collection entry into the ITS collection table.
+ * Returns 0 on success, and a negative error value for generic errors.
+ */
 static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
 {
 	struct its_collection *collection;
@@ -2522,7 +2532,7 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
 		return ret;
 	val = le64_to_cpu(val);
 	if (!(val & KVM_ITS_CTE_VALID_MASK))
-		return 0;
+		return 0; /* invalid entry, skip it */
 
 	target_addr = (u32)(val >> KVM_ITS_CTE_RDBASE_SHIFT);
 	coll_id = val & KVM_ITS_CTE_ICID_MASK;
@@ -2534,11 +2544,15 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
 	collection = find_collection(its, coll_id);
 	if (collection)
 		return -EEXIST;
+
+	if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
+		return -EINVAL;
+
 	ret = vgic_its_alloc_collection(its, &collection, coll_id);
 	if (ret)
 		return ret;
 	collection->target_addr = target_addr;
-	return 1;
+	return 0;
 }
 
 /**
@@ -2604,15 +2618,12 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
 
 	while (read < max_size) {
 		ret = vgic_its_restore_cte(its, gpa, cte_esz);
-		if (ret <= 0)
+		if (ret < 0)
 			break;
 		gpa += cte_esz;
 		read += cte_esz;
 	}
 
-	if (ret > 0)
-		return 0;
-
 	return ret;
 }
 
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 4/4] KVM: arm64: vgic: Undo work in failed ITS restores

[Ricardo Koller]

Failed ITS restores should clean up all state restored until the
failure. There is some cleanup present for this situation, but it's not
complete. Add the missing free's.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/vgic/vgic-its.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 4ece649e2493..200ac59edaf9 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -2229,8 +2229,10 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
 		vcpu = kvm_get_vcpu(kvm, collection->target_addr);
 
 	irq = vgic_add_lpi(kvm, lpi_id, vcpu);
-	if (IS_ERR(irq))
+	if (IS_ERR(irq)) {
+		its_free_ite(kvm, ite);
 		return PTR_ERR(irq);
+	}
 	ite->irq = irq;
 
 	return offset;
@@ -2498,6 +2500,9 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
 	if (ret > 0)
 		ret = 0;
 
+	if (ret < 0)
+		vgic_its_free_device_list(its->dev->kvm, its);
+
 	return ret;
 }
 
@@ -2624,6 +2629,9 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
 		read += cte_esz;
 	}
 
+	if (ret < 0)
+		vgic_its_free_collection_list(its->dev->kvm, its);
+
 	return ret;
 }
 
@@ -2655,7 +2663,10 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
 	if (ret)
 		return ret;
 
-	return vgic_its_restore_device_tables(its);
+	ret = vgic_its_restore_device_tables(its);
+	if (ret)
+		vgic_its_free_collection_list(its->dev->kvm, its);
+	return ret;
 }
 
 static int vgic_its_commit_v0(struct vgic_its *its)
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH 4/4] KVM: arm64: vgic: Undo work in failed ITS restores

[Ricardo Koller]

Failed ITS restores should clean up all state restored until the
failure. There is some cleanup present for this situation, but it's not
complete. Add the missing free's.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/vgic/vgic-its.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 4ece649e2493..200ac59edaf9 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -2229,8 +2229,10 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
 		vcpu = kvm_get_vcpu(kvm, collection->target_addr);
 
 	irq = vgic_add_lpi(kvm, lpi_id, vcpu);
-	if (IS_ERR(irq))
+	if (IS_ERR(irq)) {
+		its_free_ite(kvm, ite);
 		return PTR_ERR(irq);
+	}
 	ite->irq = irq;
 
 	return offset;
@@ -2498,6 +2500,9 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
 	if (ret > 0)
 		ret = 0;
 
+	if (ret < 0)
+		vgic_its_free_device_list(its->dev->kvm, its);
+
 	return ret;
 }
 
@@ -2624,6 +2629,9 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
 		read += cte_esz;
 	}
 
+	if (ret < 0)
+		vgic_its_free_collection_list(its->dev->kvm, its);
+
 	return ret;
 }
 
@@ -2655,7 +2663,10 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
 	if (ret)
 		return ret;
 
-	return vgic_its_restore_device_tables(its);
+	ret = vgic_its_restore_device_tables(its);
+	if (ret)
+		vgic_its_free_collection_list(its->dev->kvm, its);
+	return ret;
 }
 
 static int vgic_its_commit_v0(struct vgic_its *its)
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/4] KVM: arm64: vgic: Check that new ITEs could be saved in guest memory

[Marc Zyngier]

On Mon, 25 Apr 2022 19:55:31 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> A command that adds an entry into an ITS table that is not in guest
> memory should fail, as any command should be treated as if it was
> actually saving entries in guest memory (KVM doesn't until saving).
> Add the corresponding check for the ITT when adding ITEs.
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arch/arm64/kvm/vgic/vgic-its.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 2e13402be3bd..d7c1a3a01af4 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -976,6 +976,37 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>  	return ret;
>  }
>  
> +/*
> + * Check whether an event ID can be stored in the corresponding Interrupt
> + * Translation Table, which starts at device->itt_addr.
> + */
> +static bool vgic_its_check_ite(struct vgic_its *its, struct its_device *device,
> +		u32 event_id)
> +{
> +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> +	int ite_esz = abi->ite_esz;
> +	gpa_t gpa;
> +	gfn_t gfn;
> +	int idx;
> +	bool ret;
> +
> +	/* max table size is: BIT_ULL(device->num_eventid_bits) * ite_esz */
> +	if (event_id >= BIT_ULL(device->num_eventid_bits))
> +		return false;

We have already checked this condition, it seems.

> +
> +	gpa = device->itt_addr + event_id * ite_esz;
> +	gfn = gpa >> PAGE_SHIFT;
> +
> +	idx = srcu_read_lock(&its->dev->kvm->srcu);
> +	ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
> +	srcu_read_unlock(&its->dev->kvm->srcu, idx);
> +	return ret;

Why should we care? If the guest doesn't give us the memory that is
required, that's its problem. The only architectural requirement is
that the EID fits into the device table. There is no guarantee that
the ITS will actually write to the memory.

And if you feel that there is a strong need to have this, surely you
can reuse some of the vgic_its_check_id() code.

> +}
> +
> +/*
> + * Adds a new collection into the ITS collection table.
> + * Returns 0 on success, and a negative error value for generic errors.

Not only. A positive error can also be returned for an out of range
condition.

> + */
>  static int vgic_its_alloc_collection(struct vgic_its *its,
>  				     struct its_collection **colp,
>  				     u32 coll_id)
> @@ -1076,6 +1107,9 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>  	if (find_ite(its, device_id, event_id))
>  		return 0;
>  
> +	if (!vgic_its_check_ite(its, device, event_id))
> +		return E_ITS_MAPTI_ID_OOR;
> +
>  	collection = find_collection(its, coll_id);
>  	if (!collection) {
>  		int ret = vgic_its_alloc_collection(its, &collection, coll_id);
> -- 
> 2.36.0.rc2.479.g8af0fa9b8e-goog
> 
> 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/4] KVM: arm64: vgic: Check that new ITEs could be saved in guest memory

[Marc Zyngier]

On Mon, 25 Apr 2022 19:55:31 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> A command that adds an entry into an ITS table that is not in guest
> memory should fail, as any command should be treated as if it was
> actually saving entries in guest memory (KVM doesn't until saving).
> Add the corresponding check for the ITT when adding ITEs.
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arch/arm64/kvm/vgic/vgic-its.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 2e13402be3bd..d7c1a3a01af4 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -976,6 +976,37 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>  	return ret;
>  }
>  
> +/*
> + * Check whether an event ID can be stored in the corresponding Interrupt
> + * Translation Table, which starts at device->itt_addr.
> + */
> +static bool vgic_its_check_ite(struct vgic_its *its, struct its_device *device,
> +		u32 event_id)
> +{
> +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> +	int ite_esz = abi->ite_esz;
> +	gpa_t gpa;
> +	gfn_t gfn;
> +	int idx;
> +	bool ret;
> +
> +	/* max table size is: BIT_ULL(device->num_eventid_bits) * ite_esz */
> +	if (event_id >= BIT_ULL(device->num_eventid_bits))
> +		return false;

We have already checked this condition, it seems.

> +
> +	gpa = device->itt_addr + event_id * ite_esz;
> +	gfn = gpa >> PAGE_SHIFT;
> +
> +	idx = srcu_read_lock(&its->dev->kvm->srcu);
> +	ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
> +	srcu_read_unlock(&its->dev->kvm->srcu, idx);
> +	return ret;

Why should we care? If the guest doesn't give us the memory that is
required, that's its problem. The only architectural requirement is
that the EID fits into the device table. There is no guarantee that
the ITS will actually write to the memory.

And if you feel that there is a strong need to have this, surely you
can reuse some of the vgic_its_check_id() code.

> +}
> +
> +/*
> + * Adds a new collection into the ITS collection table.
> + * Returns 0 on success, and a negative error value for generic errors.

Not only. A positive error can also be returned for an out of range
condition.

> + */
>  static int vgic_its_alloc_collection(struct vgic_its *its,
>  				     struct its_collection **colp,
>  				     u32 coll_id)
> @@ -1076,6 +1107,9 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>  	if (find_ite(its, device_id, event_id))
>  		return 0;
>  
> +	if (!vgic_its_check_ite(its, device, event_id))
> +		return E_ITS_MAPTI_ID_OOR;
> +
>  	collection = find_collection(its, coll_id);
>  	if (!collection) {
>  		int ret = vgic_its_alloc_collection(its, &collection, coll_id);
> -- 
> 2.36.0.rc2.479.g8af0fa9b8e-goog
> 
> 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/4] KVM: arm64: vgic: Check that new ITEs could be saved in guest memory

[Ricardo Koller]

On Tue, Apr 26, 2022 at 05:07:40AM +0100, Marc Zyngier wrote:
> On Mon, 25 Apr 2022 19:55:31 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > A command that adds an entry into an ITS table that is not in guest
> > memory should fail, as any command should be treated as if it was
> > actually saving entries in guest memory (KVM doesn't until saving).
> > Add the corresponding check for the ITT when adding ITEs.
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arch/arm64/kvm/vgic/vgic-its.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > index 2e13402be3bd..d7c1a3a01af4 100644
> > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > @@ -976,6 +976,37 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> >  	return ret;
> >  }
> >  
> > +/*
> > + * Check whether an event ID can be stored in the corresponding Interrupt
> > + * Translation Table, which starts at device->itt_addr.
> > + */
> > +static bool vgic_its_check_ite(struct vgic_its *its, struct its_device *device,
> > +		u32 event_id)
> > +{
> > +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> > +	int ite_esz = abi->ite_esz;
> > +	gpa_t gpa;
> > +	gfn_t gfn;
> > +	int idx;
> > +	bool ret;
> > +
> > +	/* max table size is: BIT_ULL(device->num_eventid_bits) * ite_esz */
> > +	if (event_id >= BIT_ULL(device->num_eventid_bits))
> > +		return false;
> 
> We have already checked this condition, it seems.
> 
> > +
> > +	gpa = device->itt_addr + event_id * ite_esz;
> > +	gfn = gpa >> PAGE_SHIFT;
> > +
> > +	idx = srcu_read_lock(&its->dev->kvm->srcu);
> > +	ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
> > +	srcu_read_unlock(&its->dev->kvm->srcu, idx);
> > +	return ret;
> 
> Why should we care? If the guest doesn't give us the memory that is
> required, that's its problem.

The issue is that if the guest does that, then the pause will fail and
we won't be able to migrate the VM. This series objective is to help
with failed migrations due to the ITS. This commit tries to do it by
avoiding them.

> The only architectural requirement is
> that the EID fits into the device table. There is no guarantee that
> the ITS will actually write to the memory.

If I understand it correctly, failing the command in this case would
also be architectural (right?). If the ITS tries to write the new
entry into memory immediately, then the command would fail. This
proposed check is just making this assumption.

> 
> And if you feel that there is a strong need to have this, surely you
> can reuse some of the vgic_its_check_id() code.
> 

Sure, will do that.

> > +}
> > +
> > +/*
> > + * Adds a new collection into the ITS collection table.
> > + * Returns 0 on success, and a negative error value for generic errors.
> 
> Not only. A positive error can also be returned for an out of range
> condition.
> 

This is coming from a subsequent commit. It made it here by mistake
(most likely when I was reordering the commits). Will put it where it
belongs, sorry for that.

Thanks for the review,
Ricardo

> > + */
> >  static int vgic_its_alloc_collection(struct vgic_its *its,
> >  				     struct its_collection **colp,
> >  				     u32 coll_id)
> > @@ -1076,6 +1107,9 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
> >  	if (find_ite(its, device_id, event_id))
> >  		return 0;
> >  
> > +	if (!vgic_its_check_ite(its, device, event_id))
> > +		return E_ITS_MAPTI_ID_OOR;
> > +
> >  	collection = find_collection(its, coll_id);
> >  	if (!collection) {
> >  		int ret = vgic_its_alloc_collection(its, &collection, coll_id);
> > -- 
> > 2.36.0.rc2.479.g8af0fa9b8e-goog
> > 
> > 
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.

Re: [PATCH 1/4] KVM: arm64: vgic: Check that new ITEs could be saved in guest memory

[Ricardo Koller]

On Tue, Apr 26, 2022 at 05:07:40AM +0100, Marc Zyngier wrote:
> On Mon, 25 Apr 2022 19:55:31 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > A command that adds an entry into an ITS table that is not in guest
> > memory should fail, as any command should be treated as if it was
> > actually saving entries in guest memory (KVM doesn't until saving).
> > Add the corresponding check for the ITT when adding ITEs.
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arch/arm64/kvm/vgic/vgic-its.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > index 2e13402be3bd..d7c1a3a01af4 100644
> > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > @@ -976,6 +976,37 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> >  	return ret;
> >  }
> >  
> > +/*
> > + * Check whether an event ID can be stored in the corresponding Interrupt
> > + * Translation Table, which starts at device->itt_addr.
> > + */
> > +static bool vgic_its_check_ite(struct vgic_its *its, struct its_device *device,
> > +		u32 event_id)
> > +{
> > +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> > +	int ite_esz = abi->ite_esz;
> > +	gpa_t gpa;
> > +	gfn_t gfn;
> > +	int idx;
> > +	bool ret;
> > +
> > +	/* max table size is: BIT_ULL(device->num_eventid_bits) * ite_esz */
> > +	if (event_id >= BIT_ULL(device->num_eventid_bits))
> > +		return false;
> 
> We have already checked this condition, it seems.
> 
> > +
> > +	gpa = device->itt_addr + event_id * ite_esz;
> > +	gfn = gpa >> PAGE_SHIFT;
> > +
> > +	idx = srcu_read_lock(&its->dev->kvm->srcu);
> > +	ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
> > +	srcu_read_unlock(&its->dev->kvm->srcu, idx);
> > +	return ret;
> 
> Why should we care? If the guest doesn't give us the memory that is
> required, that's its problem.

The issue is that if the guest does that, then the pause will fail and
we won't be able to migrate the VM. This series objective is to help
with failed migrations due to the ITS. This commit tries to do it by
avoiding them.

> The only architectural requirement is
> that the EID fits into the device table. There is no guarantee that
> the ITS will actually write to the memory.

If I understand it correctly, failing the command in this case would
also be architectural (right?). If the ITS tries to write the new
entry into memory immediately, then the command would fail. This
proposed check is just making this assumption.

> 
> And if you feel that there is a strong need to have this, surely you
> can reuse some of the vgic_its_check_id() code.
> 

Sure, will do that.

> > +}
> > +
> > +/*
> > + * Adds a new collection into the ITS collection table.
> > + * Returns 0 on success, and a negative error value for generic errors.
> 
> Not only. A positive error can also be returned for an out of range
> condition.
> 

This is coming from a subsequent commit. It made it here by mistake
(most likely when I was reordering the commits). Will put it where it
belongs, sorry for that.

Thanks for the review,
Ricardo

> > + */
> >  static int vgic_its_alloc_collection(struct vgic_its *its,
> >  				     struct its_collection **colp,
> >  				     u32 coll_id)
> > @@ -1076,6 +1107,9 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
> >  	if (find_ite(its, device_id, event_id))
> >  		return 0;
> >  
> > +	if (!vgic_its_check_ite(its, device, event_id))
> > +		return E_ITS_MAPTI_ID_OOR;
> > +
> >  	collection = find_collection(its, coll_id);
> >  	if (!collection) {
> >  		int ret = vgic_its_alloc_collection(its, &collection, coll_id);
> > -- 
> > 2.36.0.rc2.479.g8af0fa9b8e-goog
> > 
> > 
> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/4] KVM: arm64: vgic: Check that new ITEs could be saved in guest memory

[Marc Zyngier]

On Tue, 26 Apr 2022 17:21:07 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Tue, Apr 26, 2022 at 05:07:40AM +0100, Marc Zyngier wrote:
> > On Mon, 25 Apr 2022 19:55:31 +0100,
> > Ricardo Koller <ricarkol@google.com> wrote:
> > > 
> > > A command that adds an entry into an ITS table that is not in guest
> > > memory should fail, as any command should be treated as if it was
> > > actually saving entries in guest memory (KVM doesn't until saving).
> > > Add the corresponding check for the ITT when adding ITEs.
> > > 
> > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > ---
> > >  arch/arm64/kvm/vgic/vgic-its.c | 34 ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > > 
> > > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > > index 2e13402be3bd..d7c1a3a01af4 100644
> > > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > > @@ -976,6 +976,37 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> > >  	return ret;
> > >  }
> > >  
> > > +/*
> > > + * Check whether an event ID can be stored in the corresponding Interrupt
> > > + * Translation Table, which starts at device->itt_addr.
> > > + */
> > > +static bool vgic_its_check_ite(struct vgic_its *its, struct its_device *device,
> > > +		u32 event_id)
> > > +{
> > > +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> > > +	int ite_esz = abi->ite_esz;
> > > +	gpa_t gpa;
> > > +	gfn_t gfn;
> > > +	int idx;
> > > +	bool ret;
> > > +
> > > +	/* max table size is: BIT_ULL(device->num_eventid_bits) * ite_esz */
> > > +	if (event_id >= BIT_ULL(device->num_eventid_bits))
> > > +		return false;
> > 
> > We have already checked this condition, it seems.
> > 
> > > +
> > > +	gpa = device->itt_addr + event_id * ite_esz;
> > > +	gfn = gpa >> PAGE_SHIFT;
> > > +
> > > +	idx = srcu_read_lock(&its->dev->kvm->srcu);
> > > +	ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
> > > +	srcu_read_unlock(&its->dev->kvm->srcu, idx);
> > > +	return ret;
> > 
> > Why should we care? If the guest doesn't give us the memory that is
> > required, that's its problem.
> 
> The issue is that if the guest does that, then the pause will fail and
> we won't be able to migrate the VM. This series objective is to help
> with failed migrations due to the ITS. This commit tries to do it by
> avoiding them.

But the guest is buggy, isn't it? No memory, no cookie! ;-)

I understand that you want save/restore to be predictable even when
the guest is too crap for words. I think clarifying this in your
commit message would help.

> > The only architectural requirement is
> > that the EID fits into the device table. There is no guarantee that
> > the ITS will actually write to the memory.
> 
> If I understand it correctly, failing the command in this case would
> also be architectural (right?). If the ITS tries to write the new
> entry into memory immediately, then the command would fail. This
> proposed check is just making this assumption.

Neither behaviour is architectural (they are both allowed, but none
is required). Not providing the memory, however, is unpredictable.

I'm OK with your approach because it makes things consistent (we fail
early rather than late). But the commit message doesn't really reflect
that (it sort of hints to it, but not in a clear way).

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/4] KVM: arm64: vgic: Check that new ITEs could be saved in guest memory

[Marc Zyngier]

On Tue, 26 Apr 2022 17:21:07 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Tue, Apr 26, 2022 at 05:07:40AM +0100, Marc Zyngier wrote:
> > On Mon, 25 Apr 2022 19:55:31 +0100,
> > Ricardo Koller <ricarkol@google.com> wrote:
> > > 
> > > A command that adds an entry into an ITS table that is not in guest
> > > memory should fail, as any command should be treated as if it was
> > > actually saving entries in guest memory (KVM doesn't until saving).
> > > Add the corresponding check for the ITT when adding ITEs.
> > > 
> > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > ---
> > >  arch/arm64/kvm/vgic/vgic-its.c | 34 ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > > 
> > > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > > index 2e13402be3bd..d7c1a3a01af4 100644
> > > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > > @@ -976,6 +976,37 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> > >  	return ret;
> > >  }
> > >  
> > > +/*
> > > + * Check whether an event ID can be stored in the corresponding Interrupt
> > > + * Translation Table, which starts at device->itt_addr.
> > > + */
> > > +static bool vgic_its_check_ite(struct vgic_its *its, struct its_device *device,
> > > +		u32 event_id)
> > > +{
> > > +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> > > +	int ite_esz = abi->ite_esz;
> > > +	gpa_t gpa;
> > > +	gfn_t gfn;
> > > +	int idx;
> > > +	bool ret;
> > > +
> > > +	/* max table size is: BIT_ULL(device->num_eventid_bits) * ite_esz */
> > > +	if (event_id >= BIT_ULL(device->num_eventid_bits))
> > > +		return false;
> > 
> > We have already checked this condition, it seems.
> > 
> > > +
> > > +	gpa = device->itt_addr + event_id * ite_esz;
> > > +	gfn = gpa >> PAGE_SHIFT;
> > > +
> > > +	idx = srcu_read_lock(&its->dev->kvm->srcu);
> > > +	ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
> > > +	srcu_read_unlock(&its->dev->kvm->srcu, idx);
> > > +	return ret;
> > 
> > Why should we care? If the guest doesn't give us the memory that is
> > required, that's its problem.
> 
> The issue is that if the guest does that, then the pause will fail and
> we won't be able to migrate the VM. This series objective is to help
> with failed migrations due to the ITS. This commit tries to do it by
> avoiding them.

But the guest is buggy, isn't it? No memory, no cookie! ;-)

I understand that you want save/restore to be predictable even when
the guest is too crap for words. I think clarifying this in your
commit message would help.

> > The only architectural requirement is
> > that the EID fits into the device table. There is no guarantee that
> > the ITS will actually write to the memory.
> 
> If I understand it correctly, failing the command in this case would
> also be architectural (right?). If the ITS tries to write the new
> entry into memory immediately, then the command would fail. This
> proposed check is just making this assumption.

Neither behaviour is architectural (they are both allowed, but none
is required). Not providing the memory, however, is unpredictable.

I'm OK with your approach because it makes things consistent (we fail
early rather than late). But the commit message doesn't really reflect
that (it sort of hints to it, but not in a clear way).

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/4] KVM: arm64: vgic: Check that new ITEs could be saved in guest memory

[Ricardo Koller]

On Tue, Apr 26, 2022 at 06:34:49PM +0100, Marc Zyngier wrote:
> On Tue, 26 Apr 2022 17:21:07 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > On Tue, Apr 26, 2022 at 05:07:40AM +0100, Marc Zyngier wrote:
> > > On Mon, 25 Apr 2022 19:55:31 +0100,
> > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > 
> > > > A command that adds an entry into an ITS table that is not in guest
> > > > memory should fail, as any command should be treated as if it was
> > > > actually saving entries in guest memory (KVM doesn't until saving).
> > > > Add the corresponding check for the ITT when adding ITEs.
> > > > 
> > > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > > ---
> > > >  arch/arm64/kvm/vgic/vgic-its.c | 34 ++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 34 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > > > index 2e13402be3bd..d7c1a3a01af4 100644
> > > > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > > > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > > > @@ -976,6 +976,37 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +/*
> > > > + * Check whether an event ID can be stored in the corresponding Interrupt
> > > > + * Translation Table, which starts at device->itt_addr.
> > > > + */
> > > > +static bool vgic_its_check_ite(struct vgic_its *its, struct its_device *device,
> > > > +		u32 event_id)
> > > > +{
> > > > +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> > > > +	int ite_esz = abi->ite_esz;
> > > > +	gpa_t gpa;
> > > > +	gfn_t gfn;
> > > > +	int idx;
> > > > +	bool ret;
> > > > +
> > > > +	/* max table size is: BIT_ULL(device->num_eventid_bits) * ite_esz */
> > > > +	if (event_id >= BIT_ULL(device->num_eventid_bits))
> > > > +		return false;
> > > 
> > > We have already checked this condition, it seems.
> > > 
> > > > +
> > > > +	gpa = device->itt_addr + event_id * ite_esz;
> > > > +	gfn = gpa >> PAGE_SHIFT;
> > > > +
> > > > +	idx = srcu_read_lock(&its->dev->kvm->srcu);
> > > > +	ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
> > > > +	srcu_read_unlock(&its->dev->kvm->srcu, idx);
> > > > +	return ret;
> > > 
> > > Why should we care? If the guest doesn't give us the memory that is
> > > required, that's its problem.
> > 
> > The issue is that if the guest does that, then the pause will fail and
> > we won't be able to migrate the VM. This series objective is to help
> > with failed migrations due to the ITS. This commit tries to do it by
> > avoiding them.
> 
> But the guest is buggy, isn't it? No memory, no cookie! ;-)
> 
> I understand that you want save/restore to be predictable even when
> the guest is too crap for words. I think clarifying this in your
> commit message would help.
> 
> > > The only architectural requirement is
> > > that the EID fits into the device table. There is no guarantee that
> > > the ITS will actually write to the memory.
> > 
> > If I understand it correctly, failing the command in this case would
> > also be architectural (right?). If the ITS tries to write the new
> > entry into memory immediately, then the command would fail. This
> > proposed check is just making this assumption.
> 
> Neither behaviour is architectural (they are both allowed, but none
> is required). Not providing the memory, however, is unpredictable.
> 
> I'm OK with your approach because it makes things consistent (we fail
> early rather than late). But the commit message doesn't really reflect
> that (it sort of hints to it, but not in a clear way).
> 

Sounds good, will do that, thanks.

> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.

Re: [PATCH 1/4] KVM: arm64: vgic: Check that new ITEs could be saved in guest memory

[Ricardo Koller]

On Tue, Apr 26, 2022 at 06:34:49PM +0100, Marc Zyngier wrote:
> On Tue, 26 Apr 2022 17:21:07 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > On Tue, Apr 26, 2022 at 05:07:40AM +0100, Marc Zyngier wrote:
> > > On Mon, 25 Apr 2022 19:55:31 +0100,
> > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > 
> > > > A command that adds an entry into an ITS table that is not in guest
> > > > memory should fail, as any command should be treated as if it was
> > > > actually saving entries in guest memory (KVM doesn't until saving).
> > > > Add the corresponding check for the ITT when adding ITEs.
> > > > 
> > > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > > ---
> > > >  arch/arm64/kvm/vgic/vgic-its.c | 34 ++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 34 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > > > index 2e13402be3bd..d7c1a3a01af4 100644
> > > > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > > > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > > > @@ -976,6 +976,37 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +/*
> > > > + * Check whether an event ID can be stored in the corresponding Interrupt
> > > > + * Translation Table, which starts at device->itt_addr.
> > > > + */
> > > > +static bool vgic_its_check_ite(struct vgic_its *its, struct its_device *device,
> > > > +		u32 event_id)
> > > > +{
> > > > +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> > > > +	int ite_esz = abi->ite_esz;
> > > > +	gpa_t gpa;
> > > > +	gfn_t gfn;
> > > > +	int idx;
> > > > +	bool ret;
> > > > +
> > > > +	/* max table size is: BIT_ULL(device->num_eventid_bits) * ite_esz */
> > > > +	if (event_id >= BIT_ULL(device->num_eventid_bits))
> > > > +		return false;
> > > 
> > > We have already checked this condition, it seems.
> > > 
> > > > +
> > > > +	gpa = device->itt_addr + event_id * ite_esz;
> > > > +	gfn = gpa >> PAGE_SHIFT;
> > > > +
> > > > +	idx = srcu_read_lock(&its->dev->kvm->srcu);
> > > > +	ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
> > > > +	srcu_read_unlock(&its->dev->kvm->srcu, idx);
> > > > +	return ret;
> > > 
> > > Why should we care? If the guest doesn't give us the memory that is
> > > required, that's its problem.
> > 
> > The issue is that if the guest does that, then the pause will fail and
> > we won't be able to migrate the VM. This series objective is to help
> > with failed migrations due to the ITS. This commit tries to do it by
> > avoiding them.
> 
> But the guest is buggy, isn't it? No memory, no cookie! ;-)
> 
> I understand that you want save/restore to be predictable even when
> the guest is too crap for words. I think clarifying this in your
> commit message would help.
> 
> > > The only architectural requirement is
> > > that the EID fits into the device table. There is no guarantee that
> > > the ITS will actually write to the memory.
> > 
> > If I understand it correctly, failing the command in this case would
> > also be architectural (right?). If the ITS tries to write the new
> > entry into memory immediately, then the command would fail. This
> > proposed check is just making this assumption.
> 
> Neither behaviour is architectural (they are both allowed, but none
> is required). Not providing the memory, however, is unpredictable.
> 
> I'm OK with your approach because it makes things consistent (we fail
> early rather than late). But the commit message doesn't really reflect
> that (it sort of hints to it, but not in a clear way).
> 

Sounds good, will do that, thanks.

> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm