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

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

On Sat, Apr 20, 2024 at 08:08:36PM +0100, Marc Zyngier wrote:
> 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).

Right, that'd definitely be useful. I think we'll need an early,
explicit check that EID + DID are within range if we go with this
option, as we currently rely on find_ite() failing in the slow path to
do that for us.

> >  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.

Sounds good, I'll just spell it out completely.

> > @@ -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).

Sure, I can just say that instead.

-- 
Thanks,
Oliver