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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Ricardo,

On 4/27/22 20:48, Ricardo Koller wrote:
> Restoring a corrupted collection entry is being ignored and treated as
maybe precise what is a corrupted ITE (out of range id or not matching
guest RAM)
> success. More specifically, 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.
Not fully correct as vgic_its_restore_cte() also returns a bunch of
generic negative error codes. vgic_its_alloc_collection() only returns
one positive ITS error code.
> 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.
Isn't what we want. If I remember correctly an invalid entry corresponds
to the end of the collection table, hence the break.
see vgic_its_save_collection_table() and "add a last dummy element with
valid bit unset".
>
> 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@xxxxxxxxxx>
> ---
>  arch/arm64/kvm/vgic/vgic-its.c | 35 ++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index fb2d26a73880..86c26aaa8275 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -999,15 +999,16 @@ static bool vgic_its_check_event_id(struct vgic_its *its, struct its_device *dev
>  	return __is_visible_gfn_locked(its, gpa);
>  }
>  
> +/*
> + * Adds a new collection into the ITS collection table.
nit: s/Adds/Add here and below
> + * 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)
>  {
>  	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;
> @@ -1101,7 +1102,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;
> @@ -1256,6 +1262,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)
> @@ -2497,6 +2507,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;
> @@ -2511,7 +2525,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;
> @@ -2523,11 +2537,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;
>  }
>  
>  /**
> @@ -2593,15 +2611,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;
>  }
>  
Thanks

Eric




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux