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

[Marc Zyngier <maz@kernel.org>]

On Fri, 20 Jun 2025 21:20:36 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Hi Sascha,
> 
> Thank you for posting this. Very excited to see the GICv5 enablement get
> started.
> 
> 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.

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.

> 
> > Co-authored-by: Timothy Hayes <timothy.hayes@arm.com>
> > Signed-off-by: Timothy Hayes <timothy.hayes@arm.com>
> > Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
> > ---
> >  arch/arm64/include/asm/kvm_asm.h   |  2 ++
> >  arch/arm64/include/asm/kvm_hyp.h   |  2 ++
> >  arch/arm64/kvm/Makefile            |  3 +-
> >  arch/arm64/kvm/hyp/nvhe/hyp-main.c | 12 +++++++
> >  arch/arm64/kvm/hyp/vgic-v3-sr.c    | 51 +++++++++++++++++++++++++-----
> >  arch/arm64/kvm/sys_regs.c          | 10 +++++-
> >  arch/arm64/kvm/vgic/vgic-init.c    |  6 ++--
> >  arch/arm64/kvm/vgic/vgic-v3.c      |  6 ++++
> >  arch/arm64/kvm/vgic/vgic-v5.c      | 14 ++++++++
> >  arch/arm64/kvm/vgic/vgic.h         |  2 ++
> >  include/kvm/arm_vgic.h             |  9 +++++-
> >  11 files changed, 104 insertions(+), 13 deletions(-)
> >  create mode 100644 arch/arm64/kvm/vgic/vgic-v5.c
> > 
> > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> > index bec227f9500a..ad1ef0460fd6 100644
> > --- a/arch/arm64/include/asm/kvm_asm.h
> > +++ b/arch/arm64/include/asm/kvm_asm.h
> > @@ -81,6 +81,8 @@ enum __kvm_host_smccc_func {
> >  	__KVM_HOST_SMCCC_FUNC___kvm_timer_set_cntvoff,
> >  	__KVM_HOST_SMCCC_FUNC___vgic_v3_save_vmcr_aprs,
> >  	__KVM_HOST_SMCCC_FUNC___vgic_v3_restore_vmcr_aprs,
> > +	__KVM_HOST_SMCCC_FUNC___vgic_v3_compat_mode_enable,
> > +	__KVM_HOST_SMCCC_FUNC___vgic_v3_compat_mode_disable,
> >  	__KVM_HOST_SMCCC_FUNC___pkvm_init_vm,
> >  	__KVM_HOST_SMCCC_FUNC___pkvm_init_vcpu,
> >  	__KVM_HOST_SMCCC_FUNC___pkvm_teardown_vm,
> > diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> > index e6be1f5d0967..9c8adc5186ec 100644
> > --- a/arch/arm64/include/asm/kvm_hyp.h
> > +++ b/arch/arm64/include/asm/kvm_hyp.h
> > @@ -85,6 +85,8 @@ void __vgic_v3_deactivate_traps(struct vgic_v3_cpu_if *cpu_if);
> >  void __vgic_v3_save_vmcr_aprs(struct vgic_v3_cpu_if *cpu_if);
> >  void __vgic_v3_restore_vmcr_aprs(struct vgic_v3_cpu_if *cpu_if);
> >  int __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu);
> > +void __vgic_v3_compat_mode_enable(void);
> > +void __vgic_v3_compat_mode_disable(void);
> >  
> >  #ifdef __KVM_NVHE_HYPERVISOR__
> >  void __timer_enable_traps(struct kvm_vcpu *vcpu);
> > diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> > index 7c329e01c557..3ebc0570345c 100644
> > --- a/arch/arm64/kvm/Makefile
> > +++ b/arch/arm64/kvm/Makefile
> > @@ -23,7 +23,8 @@ kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \
> >  	 vgic/vgic-v3.o vgic/vgic-v4.o \
> >  	 vgic/vgic-mmio.o vgic/vgic-mmio-v2.o \
> >  	 vgic/vgic-mmio-v3.o vgic/vgic-kvm-device.o \
> > -	 vgic/vgic-its.o vgic/vgic-debug.o vgic/vgic-v3-nested.o
> > +	 vgic/vgic-its.o vgic/vgic-debug.o vgic/vgic-v3-nested.o \
> > +	 vgic/vgic-v5.o
> >  
> >  kvm-$(CONFIG_HW_PERF_EVENTS)  += pmu-emul.o pmu.o
> >  kvm-$(CONFIG_ARM64_PTR_AUTH)  += pauth.o
> > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > index e9198e56e784..61af55df60a9 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> > @@ -475,6 +475,16 @@ static void handle___vgic_v3_restore_vmcr_aprs(struct kvm_cpu_context *host_ctxt
> >  	__vgic_v3_restore_vmcr_aprs(kern_hyp_va(cpu_if));
> >  }
> >  
> > +static void handle___vgic_v3_compat_mode_enable(struct kvm_cpu_context *host_ctxt)
> > +{
> > +	__vgic_v3_compat_mode_enable();
> > +}
> > +
> > +static void handle___vgic_v3_compat_mode_disable(struct kvm_cpu_context *host_ctxt)
> > +{
> > +	__vgic_v3_compat_mode_disable();
> > +}
> > +
> >  static void handle___pkvm_init(struct kvm_cpu_context *host_ctxt)
> >  {
> >  	DECLARE_REG(phys_addr_t, phys, host_ctxt, 1);
> > @@ -603,6 +613,8 @@ static const hcall_t host_hcall[] = {
> >  	HANDLE_FUNC(__kvm_timer_set_cntvoff),
> >  	HANDLE_FUNC(__vgic_v3_save_vmcr_aprs),
> >  	HANDLE_FUNC(__vgic_v3_restore_vmcr_aprs),
> > +	HANDLE_FUNC(__vgic_v3_compat_mode_enable),
> > +	HANDLE_FUNC(__vgic_v3_compat_mode_disable),
> >  	HANDLE_FUNC(__pkvm_init_vm),
> >  	HANDLE_FUNC(__pkvm_init_vcpu),
> >  	HANDLE_FUNC(__pkvm_teardown_vm),
> > diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> > index f162b0df5cae..b03b5f012226 100644
> > --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
> > +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
> > @@ -257,6 +257,18 @@ void __vgic_v3_restore_state(struct vgic_v3_cpu_if *cpu_if)
> >  	}
> >  }
> >  
> > +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.

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.

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,

> 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).

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.