Re: [PATCH 1/4] KVM: arm64: vgic-its: Free the caches when GITS_BASER changes

[Marc Zyngier <maz@kernel.org>]

Hi Eric,

On Thu, 20 Aug 2026 11:09:06 +0100,
Eric Auger <eauger@redhat.com> wrote:
> 
> Hi Fuad,
> 
> On 8/19/26 12:28 PM, Fuad Tabba wrote:
> > A guest that disables the ITS and re-points or shrinks GITS_BASER<n>
> > with VALID still set keeps the devices and collections it mapped
> > against the old table, as KVM frees them only when VALID is cleared.
> > The table format is not architected, so a write with a different value
> > is allowed to lose what it describes. Free the list whenever the
> I don't really get "a write with a different value is allowed to lose
> what it describes". A write at which place, in the collection table?

A write to the GITS_BASERn register describing the pointer to the
collection table.

The additional clarification is that because the *content* of the
table is IMPDEF, if you point the ITS to a different location or size
in memory, then there is no guarantee that the caches (the KVM
internal data structures) are up to date. In this case, the proposed
course of action is to invalidate the caches and start afresh.

> > stored value changes.
> > 
> > Test for a change rather than a write: its_restore_enable() rewrites
> > GITS_BASER<n> from its probe-time cache on resume, and KVM reports
> > GITS_TYPER.HCC as 0, so nothing re-maps the boot CPU's collection
> > afterwards.
> > 
> > Fixes: 36d6961c2b481 ("KVM: arm/arm64: vgic-its: Free caches when GITS_BASER Valid bit is cleared")
> > Suggested-by: Marc Zyngier <maz@kernel.org>
> > Link: https://lore.kernel.org/all/87ecg9owwa.wl-maz@kernel.org/
> > Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
> > ---
> >  arch/arm64/kvm/vgic/vgic-its.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> > index f6538b1976f9b..3339d9977af27 100644
> > --- a/arch/arm64/kvm/vgic/vgic-its.c
> > +++ b/arch/arm64/kvm/vgic/vgic-its.c
> > @@ -1649,7 +1649,7 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
> >  				      unsigned long val)
> >  {
> >  	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> > -	u64 entry_size, table_type;
> > +	u64 old, entry_size, table_type;
> >  	u64 reg, *regptr, clearbits = 0;
> >  
> >  	/* When GITS_CTLR.Enable is 1, we ignore write accesses. */
> > @@ -1672,7 +1672,9 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
> >  		return;
> >  	}
> >  
> > -	reg = update_64bit_reg(*regptr, addr & 7, len, val);
> > +	old = *regptr;
> > +
> > +	reg = update_64bit_reg(old, addr & 7, len, val);
> >  	reg &= ~GITS_BASER_RO_MASK;
> >  	reg &= ~clearbits;
> >  
> > @@ -1682,7 +1684,8 @@ static void vgic_mmio_write_its_baser(struct kvm *kvm,
> >  
> >  	*regptr = reg;
> >  
> > -	if (!(reg & GITS_BASER_VALID)) {
> > +	/* The ITS driver rewrites an unchanged GITS_BASER<n> on resume. */
> > +	if (reg != old) {
> >  		/* Take the its_lock to prevent a race with a save/restore */
> >  		mutex_lock(&its->its_lock);
> >  		switch (table_type) {
> One question: There is no vgic_its_invalidate_cache() in the function.
> Is it OK?

Probably not. We should make sure that the translation cache is gone
as well so that we retranslate and avoid signalling LPIs that have
undergone such invalidation. Thanks for spotting this.

> Besides out of curiosity, why don't we go further and remove ite entries
> that refer to removed collections in vgic_its_free_collection()?

The current policy is to keep the LPI alive as long as it is
mapped. The only thing is that we can't signal it, obviously. But it
would be legal to drop them altogether, only more work.

An additional question is whether we should consider doing a reload of
the collection table or not. I'm not keen on it, but I can also see
how a guest could want to do this. Feels a bit over the top though.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.