Re: [PATCH] KVM: arm/arm64: vgic-its: Add error handling in vgic_its_cache_translation

[Oliver Upton <oliver.upton@linux.dev>]

On Thu, Nov 28, 2024 at 02:45:34PM +0100, Keisuke Nishimura wrote:
> The xa_store() may fail because there is no guarantee that the cache_key
> index is already used in its->translation_cache. This fix (1) resolves
> the kref inconsistency on failure and (2) returns the error code.

xa_store() doesn't fail if an entry is already present at the specified
index. It returns the old entry, which is why we have a vgic_put_irq()
on the "error" path.

Genuine error handling definitely is missing here, but that would only
happen if the xarray library failed to allocate (-ENOMEM) or if the
xarray itself is broken beyond repair (-EINVAL).

> Fixes: 8201d1028caa ("KVM: arm64: vgic-its: Maintain a translation cache per ITS")
> Signed-off-by: Keisuke Nishimura <keisuke.nishimura@inria.fr>
> ---
>  arch/arm64/kvm/vgic/vgic-its.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 198296933e7e..8f423857b7d2 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -555,7 +555,7 @@ static struct vgic_irq *vgic_its_check_cache(struct kvm *kvm, phys_addr_t db,
>  	return irq;
>  }
>  
> -static void vgic_its_cache_translation(struct kvm *kvm, struct vgic_its *its,
> +static int vgic_its_cache_translation(struct kvm *kvm, struct vgic_its *its,
>  				       u32 devid, u32 eventid,
>  				       struct vgic_irq *irq)

This was deliberately made a void return. The entire translation cache
is opportunistic and not required for functional correctness. Nothing
breaks if we fail to insert an entry for, say, a failed memory
allocation.

It would be extremely helpful if you could share the steps to reproduce
the error you observe.

-- 
Thanks,
Oliver