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

[Marc Zyngier <maz@kernel.org>]

On Sun, 21 Apr 2024 18:03:24 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Sun, Apr 21, 2024 at 11:30:09AM +0100, Marc Zyngier wrote:
> > On Fri, 19 Apr 2024 23:38:32 +0100,
> > Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> [...]
> 
> > > +	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?
> 
> Sorry, I'm a touch confused. This call is to preallocate space in the
> xarray to store something at index @cache_key. Even if there already was
> a valid entry (meaning no need for memory allocation), xa_reserve()
> ought to return 0.

Yup, I understood that. My question is about its actual effect.  From
a cache "client" perspective, it means that the translation isn't
getting cached because the same translation (in another context) is
being in the process of being cached at the same time.

> 
> > 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?
> 
> I don't think so, a race still exists. For example, Userspace could issue
> concurrent calls to KVM_SIGNAL_MSI for the same device / event.

Really? When injecting an MSI, either you hit in the cache or you
don't. If you don't, you translate the hard way, then try to fit the
translation in the cache. If if you have a concurrent MSI being
signalled, whoever wins the "reserve" game wins, and it is "their"
translation that will make it into the cache.

At this stage, the locking becomes irrelevant for the purpose of
avoiding concurrent filling of the cache, because reserving serves as
a proxy for the store.

Or am I missing the point completely? Wouldn't be surprising... ;-)

> We could do away with the *explicit* locking if this code were
> restructured to drop references on old entries returned from xa_store(),
> like:
> 
> 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);
> 	phys_addr_t db = its->vgic_its_base + GITS_TRANSLATER;
> 	struct vgic_irq *old;
> 
> 	/* Do not cache a directly injected interrupt */
> 	if (irq->hw)
> 		return;
> 
> 	/*
> 	 * The irq refcount is guaranteed to be nonzero while holding the
> 	 * its_lock, as the ITE (and the reference it holds) cannot be freed.
> 	 */
> 	lockdep_assert_held(&its->its_lock);
> 	vgic_get_irq_kref(irq);
> 
> 	/*
> 	 * We could have raced with another CPU caching the same translation
> 	 * behind our back. Ensure we don't leak a reference if that is the case.
> 	 */
> 	old = xa_store_irq(&its->translation_cache, cache_key, irq, GFP_KERNEL_ACCOUNT);
> 	if (old)
> 		vgic_put_irq(old);
> }

Exactly. By relying on the xarray for synchronisation, we can avoid
holding any extra lock and playing the reserve game. I'm pretty keen
on this version.

Thanks,

	M.

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