On Tue, 2026-05-19 at 14:10 -0700, Oliver Upton wrote: > On Tue, May 19, 2026 at 03:13:30PM +0100, David Woodhouse wrote: > > On Tue, 2026-05-19 at 15:53 +0200, Paolo Bonzini wrote: > > > On Tue, May 19, 2026 at 3:00 PM David Woodhouse <[email protected]> > > > wrote: > > > > Or some guest configurations which have only ever been tested under KVM > > > > could have a bug where they *rely* on the registers not being writable, > > > > and write values which are inconsistent with the rest of their > > > > configuration. Which breaks the moment those registers become writable. > > > > > > Yeah, just having guests that worked by utter chance - but you still > > > don't want to break them - is the case that is most likely. Crappy > > > code that runs only under emulation/virtualization appears with > > > probability 1 over time. > > > > > > Is this likely in this specific case---probably not, honestly. > > > Christoffer's patch dates back to 2018 (commit d53c2c29ae0d); *back > > > then* KVM/Arm was a lot less mature, and people developing for Arm on > > > vanilla upstream kernels have moved on from Linux 4.19. > > > > It's not really 2018 though. EC2 is still running kernels with that > > older commit reverted because of the breaking change that it > > introduced. > > > > So the behaviour corresponding to GICD_IIDR.implementation_rev=1 is > > still current for *millions* of guests. > > > > I'm now finding that revert in our tree during a *later* kernel upgrade > > and trying to eliminate it. > > Still, as far as upstream is concerned this is damn near a decade old. > Decisions that you or your peers made in the downstream doesn't change > that. > > > And sure, I have given the engineers responsible for that a very hard > > stare and suggested that they should have fixed it 'properly' in the > > first place with a patch like the one we're discussing right now. > > > > And they're looking at this thread and saying "haha no, if fixing > > things properly for Arm is this hard then we'll stick with the crappy > > approach". > > The appropriate time to ask for accomodation was a *very* long time ago. > > And in the absence of clear evidence of a guest depending on the broken > IGROUPR behavior, I don't see how the guest-side changes of Christoffer's > series are any different from the multitude of bug fixes that we take > every single release cycle. It is an unfortunate bug and I concur with > Marc that it doesn't seem like the sort of thing a guest could rely > upon.
I find this concerning, because I've already explained this.
There is a very real possibility of guests simply not *noticing* that
they had bugs in this area, as it didn't *matter* what they wrote to
these registers since it never worked.
There is an even larger possibility of guests having worked around the
original issue by *detecting* whether the registers were actually
writable before choosing to use the alternative groups. And if such a
guest launches on a new kernel and then needs to be rolled back to an
older kernel, that will also break.
> Because it is very much a bug fix, it should've happened without a
> change to the revision number.
No. Changing the revision number in conjunction with the guest-visible
behaviour change is *absolutely* the right thing to do.
> Now, the handling of GICD_IIDR itself is a separate issue. By my count,
> the series broke UAPI on three separate occasions. Before b489edc36169
> IIDR was RAZ/WI from userspace. And of course dd6251e463d3 and d53c2c29ae0d
> changed the revision with no way of restoring the old value.
>
> And really, IIDR should've *never* been used as a buy in for new UAPI
> because it unnecessarily becomes guest visible. 49a1a2c70a7f ("KVM: arm64:
> vgic-v3: Advertise GICR_CTLR.{IR, CES} as a new GICD_IIDR revision") is
> a much better example for IIDR going forward as it gates *guest-side*
> behavior.
Yes, 49a1a2c70a7f is the exemplar. The guest-visible behaviour changes,
so we get a new IIDR revision and the ability to preserve the previous
behaviour by setting IIDR to the old value. That is exactly how it
should always be done.
> > I do not want them to be right. I don't think any of us want that.
> >
> > > I would still lean towards accepting the code considering the limited
> > > complexity of the addition (in fact I like it more now that it uses
> > > IIDR instead of v2_groups_user_writable, but that's taste).
> >
> > I'm absolutely prepared to have a separate technical discussion about
> > the v2_groups_user_writable thing for GICv2, which is the second part
> > of that series.
> >
> > It seems like the right thing to do, and as far as I can tell, this
> > code *never* worked with QEMU. But I'm not sure who even cares about
> > GICv2 any more. I couldn't find hardware and I had to test the whole
> > thing inside qemu-tcg.
> >
> > But the 'IIDR defaults to 3 but the *behaviour* doesn't match until you
> > explicitly *set* it to 3' thing seemed so *egregiously* wrong to me,
> > that I fixed it anyway.
>
> Wrong or not, this behavior is documented unambiguously. From the VGICv2
> UAPI documentation:
>
> """
> Userspace should set GICD_IIDR before setting any other registers (both
> KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_CPU_REGS) to ensure
> the expected behavior. Unless GICD_IIDR has been set from userspace, writes
> to the interrupt group registers (GICD_IGROUPR) are ignored.
> """
>
> I'm not inclined to change that.
That'll all very well... but as far as I can tell, QEMU *doesn't* set
GICD_IIDR, so it still gets the bizarre behaviour where the *guest* can
write the registers, but userspace can't. So it looks like it'll work
except migration will fail. Am I missing something?
But honestly, I don't care one iota about GICv2; I was only trying to
do the cleanup while I was there. Feel free to drop that part entirely.
> As a way out of this whole mess, can we
> instead:
>
> - Allow userspace to set IIDR.Revision to 1
>
> - Drop any bug emulation from the handling of IGROUPR registers
It doesn't make sense to allow setting IIDR.Revision to 1 *without* the
one-liner that actually implements the corresponding behaviour change
in the IGROUPR registers. And as explained at least twice now, it's the
behaviour change that's *important* here.
The fact that it's a long-standing bug in KVM which downstream has been
working around for a long time doesn't matter. The unconditional
behavioural change *is* a bug and we should fix it.
> - Special-case the stupid GICv2 UAPI where IGROUPR are only writable if
> the VMM has written to IIDR and the revision >= 2
That already *is* a special case, right? And you'd rather leave it as it is?
smime.p7s
Description: S/MIME cryptographic signature
