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

[Sascha Bischoff <Sascha.Bischoff@arm.com>]

On Fri, 2025-06-20 at 16:02 -0700, Oliver Upton wrote:
> On Fri, Jun 20, 2025 at 01:20:36PM -0700, Oliver Upton wrote:
> > > +void __vgic_v3_compat_mode_enable(void)
> > > +{
> > > +	sysreg_clear_set_s(SYS_ICH_VCTLR_EL2, 0,
> > > ICH_VCTLR_EL2_V3);
> > > +	isb();
> > > +}
> > > +
> > > +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.
> > 
> > Can we consolidate this into a single hyp call along with
> > __vgic_v3_*_vmcr_aprs()?
> > 
> > Last bit as an FYI, kvm_call_hyp() has an implied context
> > synchronization upon
> > return, either because of ERET in nVHE or an explicit ISB on VHE.
> 
> Ah, reading the spec was a useful exercise. ICH_VMCR_EL2 is a modal
> register... I hear implementation folks *love* those :)
> 
> Please do the aforementioned consolidation, at which point the
> purpose
> of the ISB should be apparent.

If we are happy to move to only touching the ICH_VCTLR_EL2.V3 compat
mode control in the v3/v5 load path, I think this change can happen
now. However, we otherwise need to clean up the WFI load/put path first
to avoid the double calls.

> > > +	if
> > > (!static_branch_unlikely(&kvm_vgic_global_state.gicv5_cpuif)) {
> > 
> > Can we use the GCIE cpucap instead, possibly via a shared helper
> > with
> > the driver?

I'll look into it.

> > > -	if (kvm_vgic_global_state.type == VGIC_V3) {
> > > +	if (kvm_vgic_global_state.type == VGIC_V3 ||
> > > kvm_vgic_in_v3_compat_mode()) {
> > 
> > Can we do a helper for this too?

Will do.

> > 
> > >  		val &= ~ID_AA64PFR0_EL1_GIC_MASK;
> > >  		val |= SYS_FIELD_PREP_ENUM(ID_AA64PFR0_EL1, GIC,
> > > IMP);
> > >  	}
> > > 

> > > +	if
> > > (static_branch_unlikely(&kvm_vgic_global_state.gicv5_cpuif))
> > > +		kvm_call_hyp(__vgic_v3_compat_mode_disable);
> 
> Do we need to eagerly disable compat mode or can we just configure
> the
> VGIC correctly for the intended vCPU at load()?

I've responded to this in the other thread (I think we could do it
purely on load, and drop eager disable).

> 
> > >  }
> > > diff --git a/arch/arm64/kvm/vgic/vgic-v5.c
> > > b/arch/arm64/kvm/vgic/vgic-v5.c
> > > new file mode 100644
> > > index 000000000000..57199449ca0f
> > > --- /dev/null
> > > +++ b/arch/arm64/kvm/vgic/vgic-v5.c
> > > @@ -0,0 +1,14 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +
> > > +#include <kvm/arm_vgic.h>
> > > +
> > > +#include "vgic.h"
> > > +
> > > +inline bool kvm_vgic_in_v3_compat_mode(void)a
> > 
> > nit: we're generally trusting of the compiler to 'do the right
> > thing'
> > and avoid explicit inline specifiers unless necessary.

Dropped the explicit inline.

> > 
> > > +{
> > > +	if
> > > (static_branch_unlikely(&kvm_vgic_global_state.gicv5_cpuif) &&
> > > +	    kvm_vgic_global_state.has_gcie_v3_compat)
> > > +		return true;
> > > +
> > > +	return false;
> > > +}
> > 
> > This should be a per-VM thing once KVM support for GICv5 lands. Can
> > you
> > get ahead of that and take a KVM pointer that goes unused. Maybe
> > rename
> > it:
> > 
> > bool vgic_is_v3_compat(struct kvm *kvm)
> > 
> > Or something similar.

OK, will do. There's one case were we use this without access to a
struct kvm* in kvm_vgic_init_cpu_hardware, so that will need to be done
without this helper, but we could fall back to using cpucaps directly
there.

Thanks,
Sascha

>