Re: [PATCH v2 3/4] KVM: arm64: vgic-its: Don't save collections the table cannot hold
Fuad Tabba <[email protected]>
| Newsgroups | dev.linux.lists.kvmarm,org.infradead.lists.linux-arm-kernel,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <CA+EHjTy90YP8dzUy724VQ7vzVusr+mGocFxyHoQZ2c4OJnpBcQ@mail.gmail.com> |
Hi Marc, Sorry for the slow reply, I was away. On Sat, 8 Aug 2026 at 09:09, Marc Zyngier <[email protected]> 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. > > 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? Cheers, /fuad