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

On 8/20/26 12:36 PM, Marc Zyngier wrote:
> Hi Eric,
> 
> On Thu, 20 Aug 2026 11:09:06 +0100,
> Eric Auger <[email protected]> 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.

OK thanks, this definitively clarifies the above sentence.
> 
>>> 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 <[email protected]>
>>> Link: https://lore.kernel.org/all/[email protected]/
>>> Signed-off-by: Fuad Tabba <[email protected]>
>>> ---
>>>  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.
OK
> 
> 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.
Agreed.

Thanks

Eric
> 
> Thanks,
> 
> 	M.
>