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

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

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.

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

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

-- 
Thanks,
Oliver