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;
> +}
> +

Although this is correct, you may want to use the fact that our
EIDs/DIDs are limited to 16 bits, as advertised in GITS_TYPER. By
having a more compact index, you'd get better performance from the
xarray itself (this is alluded to in the documentation).

>  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;
> +
>  	raw_spin_lock_irqsave(&dist->lpi_list_lock, flags);
>  
>  	if (unlikely(list_empty(&dist->lpi_translation_cache)))
> @@ -624,6 +633,11 @@ static void vgic_its_cache_translation(struct kvm *kvm, struct vgic_its *its,
>  	 */
>  	lockdep_assert_held(&its->its_lock);
>  	vgic_get_irq_kref(irq);
> +	/*
> +	 * Get a second ref for the ITS' translation cache. This will
> +	 * disappear.

Is that for the *global* translation cache? Or this one? I guess there
is one for each, but a clearer comment would help, even if this gets
removed three patches down the line.

> +	 */
> +	vgic_get_irq_kref(irq);
>  
>  	cte->db		= db;
>  	cte->devid	= devid;
> @@ -633,6 +647,7 @@ static void vgic_its_cache_translation(struct kvm *kvm, struct vgic_its *its,
>  	/* Move the new translation to the head of the list */
>  	list_move(&cte->entry, &dist->lpi_translation_cache);
>  
> +	xa_store(&its->translation_cache, cache_key, irq, 0);
>  out:
>  	raw_spin_unlock_irqrestore(&dist->lpi_list_lock, flags);
>  }
> @@ -641,7 +656,8 @@ static void vgic_its_invalidate_cache(struct vgic_its *its)
>  {
>  	struct vgic_dist *dist = &kvm->arch.vgic;
>  	struct vgic_translation_cache_entry *cte;
> -	unsigned long flags;
> +	unsigned long flags, idx;
> +	struct vgic_irq *irq;
>  
>  	raw_spin_lock_irqsave(&dist->lpi_list_lock, flags);
>  
> @@ -658,6 +674,11 @@ static void vgic_its_invalidate_cache(struct vgic_its *its)
>  	}
>  
>  	raw_spin_unlock_irqrestore(&dist->lpi_list_lock, flags);
> +
> +	xa_for_each(&its->translation_cache, idx, irq) {
> +		xa_erase(&its->translation_cache, idx);
> +		vgic_put_irq(its->dev->kvm, irq);
> +	}
>  }
>  
>  void vgic_its_invalidate_all_caches(struct kvm *kvm)
> @@ -1967,6 +1988,7 @@ static int vgic_its_create(struct kvm_device *dev, u32 type)
>  
>  	INIT_LIST_HEAD(&its->device_list);
>  	INIT_LIST_HEAD(&its->collection_list);
> +	xa_init_flags(&its->translation_cache, XA_FLAGS_LOCK_IRQ);
>  
>  	dev->kvm->arch.vgic.msis_require_devid = true;
>  	dev->kvm->arch.vgic.has_its = true;
> @@ -1997,6 +2019,8 @@ static void vgic_its_destroy(struct kvm_device *kvm_dev)
>  
>  	vgic_its_free_device_list(kvm, its);
>  	vgic_its_free_collection_list(kvm, its);
> +	vgic_its_invalidate_cache(its);
> +	xa_destroy(&its->translation_cache);
>  
>  	mutex_unlock(&its->its_lock);
>  	kfree(its);
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index ac7f15ec1586..0b027ed71dac 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -210,6 +210,12 @@ struct vgic_its {
>  	struct mutex		its_lock;
>  	struct list_head	device_list;
>  	struct list_head	collection_list;
> +
> +	/*
> +	 * Caches the (device_id, event_id) -> vgic_irq translation for
> +	 * *deliverable* LPIs.

Can you clarify what "deliverable" LPIs are? That's not an
architectural term, and I'd rather you describe the conditions that
allow the LPI to be cached (my guess is: mapped and enabled).

> +	 */
> +	struct xarray		translation_cache;
>  };
>  
>  struct vgic_state_iter;

Thanks,

	M.

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