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

[Eric Auger]

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?
> 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@xxxxxxxxxx>
> Link: https://lore.kernel.org/all/87ecg9owwa.wl-maz@xxxxxxxxxx/
> Signed-off-by: Fuad Tabba <fuad.tabba@xxxxxxxxx>
> ---
>  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?

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

Thanks

Eric