Re: [PATCH v2 3/4] KVM: arm64: vgic-its: Don't save collections the table cannot hold

[Marc Zyngier]

On Mon, 17 Aug 2026 19:21:24 +0100,
Fuad Tabba <fuad.tabba@xxxxxxxxx> wrote:
> 
> Hi Marc,
> 
> Sorry for the slow reply, I was away.

No worries.

> 
> On Sat, 8 Aug 2026 at 09:09, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> ...
> > > A guest that disables the ITS and rewrites GITS_BASER with fewer pages,
> > > VALID still set, keeps every collection it mapped against the larger
> > > table: KVM stores the new BASER unconditionally and frees the list only
> > > when VALID is cleared.
> >
> > But isn't that the *real* problem? Shouldn't we instead nuke the
> > collections entirely and go through a reload sequence?
> 
> Agreed, that is the problem. The fix belongs in vgic_mmio_write_its_baser().
> 
> Should it drop everything, or only the collections that no longer fit?
> On a shrink in place the low ICIDs are still in the table. Or is no
> realistic guest doing this, so the simpler rule wins? I can do the
> unconditional drop if you prefer.

Whatever is the simplest thing. I don't think it is common to update
the BASER registers (Linux certainly does it exactly once), and if
that means there will be a small hickup on things like kexec handover,
so be it.

Thinking of it a bit more: the format of the table is not architected.
So a write to the register with a different value is allowed to result
in data loss (for all the guest knows, this could be a linked list
starting from the last page...). So purging all collections (and
therefore all mappings) would be acceptable, as there no guarantee
this is now fit for purpose.

This would make both save and restore succeed, and be really quick!

> > > Validate each collection against the current table with
> > > vgic_its_check_id() and return -EINVAL, as vgic_its_save_device_tables()
> > > does for devices. Collection IDs are unique and the collection table is
> > > never indirect, so the check also bounds the walk.
> >
> > I think returning -EINVAL here was a mistake, as it aborts the save
> > procedure that userspace should be able to issue reliably, even if
> > that means the state is crap. I don't think we should expand that
> > behaviour any further.
> 
> Fair enough. Either way the check goes with the fix rather than being
> expanded: MAPC, MAPTI and the restore path already validate against
> the current table.
> 
> vgic_its_save_device_tables() is harder: with an indirect table the
> guest can invalidate an L1 entry without touching GITS_BASER, so that
> check can still fire. Skip the device rather than fail the save?

Skipping the devices that fall into an unmapped page looks fine. After
all, the guest is messing with itself, and I have no sympathy for it.

Thanks,

	M.

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