Re: [PATCH 4/5] KVM: arm64: gic-v5: Support GICv3 compat

[Sascha Bischoff]

On Sun, 2025-06-22 at 05:37 -0700, Oliver Upton wrote:
> On Sun, Jun 22, 2025 at 01:19:13PM +0100, Marc Zyngier wrote:
> > On Fri, 20 Jun 2025 21:20:36 +0100,
> > Oliver Upton <oliver.upton@xxxxxxxxx> wrote:
> > > 
> > > Hi Sascha,
> > > 
> > > Thank you for posting this. Very excited to see the GICv5
> > > enablement get
> > > started.

Hi Oliver. Me too. Thanks for all of your comments!

> > > 
> > > On Fri, Jun 20, 2025 at 04:07:51PM +0000, Sascha Bischoff wrote:
> > > > Add support for GICv3 compat mode (FEAT_GCIE_LEGACY) which
> > > > allows a
> > > > GICv5 host to run GICv3-based VMs. This change enables the
> > > > VHE/nVHE/hVHE/protected modes, but does not support nested
> > > > virtualization.
> > > 
> > > Can't we just load the shadow state into the compat VGICv3? I'm
> > > worried
> > > this has sharp edges on the UAPI side as well as users wanting to
> > > migrate VMs to new hardware.
> > > 
> > > The guest hypervisor should only see GICv3-only or GICv5-only, we
> > > can
> > > pretend FEAT_GCIE_LEGACY never existed :)
> > 
> > That's exactly what this does. And the only reason NV isn't
> > supported
> > yet is the current BET0 spec makes ICC_SRE_EL2 UNDEF at EL1 with
> > NV,
> > which breaks NV in a spectacular way.
> 
> Gee, I wonder how... :)
> 
> > This will be addressed in a future revision of the architecture,
> > and
> > no HW will actually be built with this defect. As such, there is no
> > UAPI to break.
> 
> That's fine by me. TBH, when I left this comment I hadn't fully read
> the
> patch yet and was more curious about the intent.

As Marc said, this can't work right now. Once that's been relaxed it
should largely be a case of dropping the two compat mode checks in
__vgic_v3_activate_traps & __vgic_v3_deactivate_traps, which are
present to make sure we don't access ICC_SRE_EL2 and hit an UNDEF.

And yes, the idea is to expose a pure GICv3 or GICv5 system, and not
tell the guest hyp about FEAT_GCIE_LEGACY. :)

> 
> > > > +void __vgic_v3_compat_mode_disable(void)
> > > > +{
> > > > +	sysreg_clear_set_s(SYS_ICH_VCTLR_EL2,
> > > > ICH_VCTLR_EL2_V3, 0);
> > > > +	isb();
> > > > +}
> > > > +
> > > 
> > > It isn't clear to me what these ISBs are synchonizing against.
> > > AFAICT,
> > > the whole compat thing is always visible and we can restore the
> > > rest of
> > > the VGICv3 context before guaranteeing the enable bit has been
> > > observed.
> > 
> > No, some registers have a behaviour that is dependent on the status
> > of
> > the V3 bit (ICH_VMCR_EL2 being one), so that synchronisation is
> > absolutely needed before accessing this register.
> 
> Yeah, I had followed up on this after reading the spec, modal
> registers
> are great. Putting all the constituent registers together in the
> common
> load/put helpers will clear that up.
> 
> > The disabling is probably the wrong way around though, and I'd
> > expect
> > the clearing of V3 to have an ISB *before* the write to the sysreg,

Ah, that's a good spot. I'll check these and rework accordingly, taking
into account the explicit syncs from the ERET & ISBs already present.

> > 
> > > Can we consolidate this into a single hyp call along with
> > > __vgic_v3_*_vmcr_aprs()?
> > 
> > I agree that we should be able to move this to be driven by
> > load/put entirely.
> > 
> > But we first need to fix the whole WFI sequencing first, because
> > this
> > is a bit of a train wreck at the moment (entering the WFI emulation
> > results in *two* "put" sequences on the vgic, and exiting WFI
> > results
> > in two loads).
> 
> You're talking about the case where halt polling fails and we do a
> put/load on the whole vCPU to schedule right? i.e. in addition to the
> explicit put on the vgic for faithful emulation.

That's the one. When it comes to compat mode, it causes issues due to
sampling the ICH_VMCR_EL2 twice - once with the compat mode enabled
giving the correct VMCR representation, and a second time with compat
mode disabled clobbering the saved state.

Which brings me to...

> Do we need to eagerly disable compat mode or can we just configure
> the VGIC correctly for the intended vCPU at load()?

This isn't something I'd considered, actually. I think this could work,
and would coincidentally work around the issues caused by the double
vgic put.

My thinking was that whenever we're executing on the host, we put the
system back to a "clean" state where the default guest GIC matches the
host, and thereby we'd need to do nothing with compat mode in the
vgic_v5_load/put path when it is added. If we move to toggling compat
mode in the load path only, we need to enable compat in the v3 load and
disable in the v5 load.

Are we happy with this?


Thanks,
Sascha