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.au...@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.d...@linaro.org>
