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 
> 
> ---
> 
> 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, , 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, );
> + 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 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[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 

---

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, , 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, );
+   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

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