KVM: arm64: Fix handling of ID_PFR1_EL1.GIC

Peter reported[1] that restoring a GICv2 VM fails badly, and correctly points out that ID_PFR1_EL1.GIC isn't writable, while its 64bit equivalent is. I broke that in 6.12.

The other thing is that fixing the ID regs at runtime isn't great. specially when we could adjust them at the point where the GIC gets created.

This small series aims at fixing these issues. I've only tagged the first one as a stable candidate. With these fixes, I can happily save/restore a GICv2 VM (both 32 and 64bit) on my trusty Synquacer.

[1] https://lore.kernel.org/r/CAFEAcA8TpQduexT=8rdRYC=yxm_073COjzgWJAvc26_T+-F5vA@mail.gmail.com

Marc Zyngier (3):
KVM: arm64: Make ID_PFR1_EL1.GIC writable
KVM: arm64: Set ID_{AA64PFR0,PFR1}_EL1.GIC when GICv3 is configured
KVM: arm64: Limit clearing of ID_{AA64PFR0,PFR1}_EL1.GIC to userspace
irqchip

 arch/arm64/kvm/sys_regs.c       | 12 ++++++++----
 arch/arm64/kvm/vgic/vgic-init.c | 10 ++++++++--
 2 files changed, 16 insertions(+), 6 deletions(-)

2.47.3

Similarly to ID_AA64PFR0_EL1.GIC, relax ID_PFR1_EL1.GIC to be writable.

---
 arch/arm64/kvm/sys_regs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

index b29f72478a50d..73dcefe51a3e7 100644
@@ -2528,6 +2528,12 @@ static bool bad_redir_trap(struct kvm_vcpu *vcpu,
 	.val = mask,				\
 }
 
+#define AA32_ID_WRITABLE(name, mask) {		\
+	ID_DESC(name),				\
+	.visibility = aa32_id_visibility,	\
+	.val = mask,				\
+}
+
 /* sys_reg_desc initialiser for cpufeature ID registers that need filtering */
 #define ID_FILTERED(sysreg, name, mask) {	\
 	ID_DESC(sysreg),				\

@@ -3040,7 +3046,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	/* AArch64 mappings of the AArch32 ID registers */
 	/* CRm=1 */
 	AA32_ID_SANITISED(ID_PFR0_EL1),
-	AA32_ID_SANITISED(ID_PFR1_EL1),
+	AA32_ID_WRITABLE(ID_PFR1_EL1, ID_PFR1_EL1_GIC),
 	{ SYS_DESC(SYS_ID_DFR0_EL1),
 	  .access = access_id_reg,
 	  .get_user = get_id_reg,

2.47.3

Hey,

This looks fine to me, although I do wonder if we should just allow userspace to write whatever value it wants to the 32-bit ID registers and be done with it.

Nowhere do we use a 32-bit ID register value as a condition for trap configuration / emulation, so even if the VMM lies to the guest it shouldn't trip up KVM.

Thanks,
Oliver

That's a good point. Nobody really cares about 32bit anyway, and I'd be happy to just let the VMM write whatever it wants. Might be a bit harder to backport, but whoever is interested in AArch32 will be able to do it.

Thanks,

	M.

Without deviation from the norm, progress is not possible.

Drive the idreg fields indicating the presence of GICv3 directly from the vgic code. This avoids having to do any sort of runtime clearing of the idreg.

---
 arch/arm64/kvm/vgic/vgic-init.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

index 4c3c0d82e4760..2c518b0a4d81b 100644
@@ -161,10 +161,16 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
 
 	kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
 
-	if (type == KVM_DEV_TYPE_ARM_VGIC_V2)
+	*__vm_id_reg(&kvm->arch, SYS_ID_AA64PFR0_EL1) &= ~ID_AA64PFR0_EL1_GIC;
+	*__vm_id_reg(&kvm->arch, SYS_ID_PFR1_EL1) &= ~ID_PFR1_EL1_GIC;
+
+	if (type == KVM_DEV_TYPE_ARM_VGIC_V2) {
 		kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
-	else
+	} else {
 		INIT_LIST_HEAD(&kvm->arch.vgic.rd_regions);
+		*__vm_id_reg(&kvm->arch, SYS_ID_AA64PFR0_EL1) |= SYS_FIELD_VALUE(ID_AA64PFR0_EL1, GIC, IMP);
+		*__vm_id_reg(&kvm->arch, SYS_ID_PFR1_EL1) |= SYS_FIELD_VALUE(ID_PFR1_EL1, GIC, GICv3);
+	}
 
 	if (type == KVM_DEV_TYPE_ARM_VGIC_V3)
 		kvm->arch.vgic.nassgicap = system_supports_direct_sgis();

2.47.3

Now that the idreg's GIC field is in sync with the irqchip, limit the runtime clearing of these fields to the pathological case where we do not have an in-kernel GIC.

---
 arch/arm64/kvm/sys_regs.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index 73dcefe51a3e7..25cfd0f9541f5 100644
@@ -5494,9 +5494,7 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)
 
 	guard(mutex)(&kvm->arch.config_lock);
 
-	if (!(static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif) &&
-	      irqchip_in_kernel(kvm) &&
-	      kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)) {
+	if (!irqchip_in_kernel(kvm)) {
 		kvm->arch.id_regs[IDREG_IDX(SYS_ID_AA64PFR0_EL1)] &= ~ID_AA64PFR0_EL1_GIC_MASK;
 		kvm->arch.id_regs[IDREG_IDX(SYS_ID_PFR1_EL1)] &= ~ID_PFR1_EL1_GIC_MASK;
 	}

2.47.3