Re: [PATCH v2 09/19] KVM: arm64: vgic-its: Maintain a translation cache per ITS

[Marc Zyngier <maz@kernel.org>]

On Fri, 19 Apr 2024 23:38:32 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Within the context of a single ITS, it is possible to use an xarray to
> cache the device ID & event ID translation to a particular irq
> descriptor. Take advantage of this to build a translation cache capable
> of fitting all valid translations for a given ITS.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/vgic/vgic-its.c | 26 +++++++++++++++++++++++++-
>  include/kvm/arm_vgic.h         |  6 ++++++
>  2 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index be17a53d16ef..fd576377c084 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -530,6 +530,11 @@ static struct vgic_its *__vgic_doorbell_to_its(struct kvm *kvm, gpa_t db)
>  	return iodev->its;
>  }
>  
> +static unsigned long vgic_its_cache_key(u32 devid, u32 eventid)
> +{
> +	return (((unsigned long)devid) << 32) | eventid;
> +}
> +
>  static struct vgic_irq *__vgic_its_check_cache(struct vgic_dist *dist,
>  					       phys_addr_t db,
>  					       u32 devid, u32 eventid)
> @@ -583,6 +588,7 @@ static void vgic_its_cache_translation(struct kvm *kvm, struct vgic_its *its,
>  				       u32 devid, u32 eventid,
>  				       struct vgic_irq *irq)
>  {
> +	unsigned long cache_key = vgic_its_cache_key(devid, eventid);
>  	struct vgic_dist *dist = &kvm->arch.vgic;
>  	struct vgic_translation_cache_entry *cte;
>  	unsigned long flags;
> @@ -592,6 +598,9 @@ static void vgic_its_cache_translation(struct kvm *kvm, struct vgic_its *its,
>  	if (irq->hw)
>  		return;
>  
> +	if (xa_reserve_irq(&its->translation_cache, cache_key, GFP_KERNEL_ACCOUNT))
> +		return;
> +

This thing tickles me a bit. What does it mean from the PoV of the
caller? That although we have missed in the cache initially, someone
else is populating it?

The final code reads like this:

	if (xa_reserve_irq(&its->translation_cache, cache_key, GFP_KERNEL_ACCOUNT))
		return;

	xa_lock_irqsave(&its->translation_cache, flags);
	rcu_read_lock();

	/*
	 * We could have raced with another CPU caching the same
	 * translation behind our back, so let's check it is not in
	 * already
	 */
	db = its->vgic_its_base + GITS_TRANSLATER;
	if (__vgic_its_check_cache(kvm, db, devid, eventid))
		goto out;

Does it mean we could drop this check? And even relax the locking?

Thanks,

	M.

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