KVM: arm64: Fix handling of ID_PFR1_EL1.GIC

Peter reported[0] 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.

* From v1 [1]:

- Make all 32bit ID regs writable

- Use official accessors to manipulate ID regs

- Rebased on 6.18-rc3

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

Marc Zyngier (3):
KVM: arm64: Make all 32bit ID registers fully 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       | 71 ++++++++++++++++++---------------
 arch/arm64/kvm/vgic/vgic-init.c | 14 ++++++-
 2 files changed, 50 insertions(+), 35 deletions(-)

2.47.3

32bit ID registers aren't getting much love these days, and are often missed in updates. One of these updates broke restoring a GICv2 guest on a GICv3 machine.

Instead of performing a piecemeal fix, just bite the bullet and make all 32bit ID regs fully writable. KVM itself never relies on them for anything, and if the VMM wants to mess up the guest, so be it.

---
 arch/arm64/kvm/sys_regs.c | 59 ++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

index e67eb39ddc118..ad82264c6cbe1 100644
@@ -2595,19 +2595,23 @@ static bool bad_redir_trap(struct kvm_vcpu *vcpu,
 	.val = 0,				\
 }
 
-/* sys_reg_desc initialiser for known cpufeature ID registers */
-#define AA32_ID_SANITISED(name) {		\
-	ID_DESC(name),				\
-	.visibility = aa32_id_visibility,	\
-	.val = 0,				\
-}
-
 /* sys_reg_desc initialiser for writable ID registers */
 #define ID_WRITABLE(name, mask) {		\
 	ID_DESC(name),				\
 	.val = mask,				\
 }
 
+/*
+ * 32bit ID regs are fully writable when the guest is 32bit
+ * capable. Nothing in the KVM code should rely on 32bit features
+ * anyway, only 64bit, so let the VMM do its worse.
+ */
+#define AA32_ID_WRITABLE(name) {		\
+	ID_DESC(name),				\
+	.visibility = aa32_id_visibility,	\
+	.val = GENMASK(31, 0),			\
+}
+
 /* sys_reg_desc initialiser for cpufeature ID registers that need filtering */
 #define ID_FILTERED(sysreg, name, mask) {	\
 	ID_DESC(sysreg),				\

@@ -3128,40 +3132,39 @@ 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_PFR0_EL1),
+	AA32_ID_WRITABLE(ID_PFR1_EL1),
 	{ SYS_DESC(SYS_ID_DFR0_EL1),
 	  .access = access_id_reg,
 	  .get_user = get_id_reg,
 	  .set_user = set_id_dfr0_el1,
 	  .visibility = aa32_id_visibility,
 	  .reset = read_sanitised_id_dfr0_el1,
-	  .val = ID_DFR0_EL1_PerfMon_MASK |
-		 ID_DFR0_EL1_CopDbg_MASK, },
+	  .val = GENMASK(31, 0) },
 	ID_HIDDEN(ID_AFR0_EL1),
-	AA32_ID_SANITISED(ID_MMFR0_EL1),
-	AA32_ID_SANITISED(ID_MMFR1_EL1),
-	AA32_ID_SANITISED(ID_MMFR2_EL1),
-	AA32_ID_SANITISED(ID_MMFR3_EL1),
+	AA32_ID_WRITABLE(ID_MMFR0_EL1),
+	AA32_ID_WRITABLE(ID_MMFR1_EL1),
+	AA32_ID_WRITABLE(ID_MMFR2_EL1),
+	AA32_ID_WRITABLE(ID_MMFR3_EL1),
 
 	/* CRm=2 */
-	AA32_ID_SANITISED(ID_ISAR0_EL1),
-	AA32_ID_SANITISED(ID_ISAR1_EL1),
-	AA32_ID_SANITISED(ID_ISAR2_EL1),
-	AA32_ID_SANITISED(ID_ISAR3_EL1),
-	AA32_ID_SANITISED(ID_ISAR4_EL1),
-	AA32_ID_SANITISED(ID_ISAR5_EL1),
-	AA32_ID_SANITISED(ID_MMFR4_EL1),
-	AA32_ID_SANITISED(ID_ISAR6_EL1),
+	AA32_ID_WRITABLE(ID_ISAR0_EL1),
+	AA32_ID_WRITABLE(ID_ISAR1_EL1),
+	AA32_ID_WRITABLE(ID_ISAR2_EL1),
+	AA32_ID_WRITABLE(ID_ISAR3_EL1),
+	AA32_ID_WRITABLE(ID_ISAR4_EL1),
+	AA32_ID_WRITABLE(ID_ISAR5_EL1),
+	AA32_ID_WRITABLE(ID_MMFR4_EL1),
+	AA32_ID_WRITABLE(ID_ISAR6_EL1),
 
 	/* CRm=3 */
-	AA32_ID_SANITISED(MVFR0_EL1),
-	AA32_ID_SANITISED(MVFR1_EL1),
-	AA32_ID_SANITISED(MVFR2_EL1),
+	AA32_ID_WRITABLE(MVFR0_EL1),
+	AA32_ID_WRITABLE(MVFR1_EL1),
+	AA32_ID_WRITABLE(MVFR2_EL1),
 	ID_UNALLOCATED(3,3),
-	AA32_ID_SANITISED(ID_PFR2_EL1),
+	AA32_ID_WRITABLE(ID_PFR2_EL1),
 	ID_HIDDEN(ID_DFR1_EL1),
-	AA32_ID_SANITISED(ID_MMFR5_EL1),
+	AA32_ID_WRITABLE(ID_MMFR5_EL1),
 	ID_UNALLOCATED(3,7),
 
 	/* AArch64 ID registers */

2.47.3

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 | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

index 1796b1a22a72a..ca411cce41409 100644
@@ -71,6 +71,7 @@ static int vgic_allocate_private_irqs_locked(struct kvm_vcpu *vcpu, u32 type);
 int kvm_vgic_create(struct kvm *kvm, u32 type)
 {
 	struct kvm_vcpu *vcpu;
+	u64 aa64pfr0, pfr1;
 	unsigned long i;
 	int ret;
 

@@ -161,10 +162,19 @@ 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)
+	aa64pfr0 = kvm_read_vm_id_reg(kvm, SYS_ID_AA64PFR0_EL1) & ~ID_AA64PFR0_EL1_GIC;
+	pfr1 = kvm_read_vm_id_reg(kvm, 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);
+		aa64pfr0 |= SYS_FIELD_PREP_ENUM(ID_AA64PFR0_EL1, GIC, IMP);
+		pfr1 |= SYS_FIELD_PREP_ENUM(ID_PFR1_EL1, GIC, GICv3);
+	}
+
+	kvm_set_vm_id_reg(kvm, SYS_ID_AA64PFR0_EL1, aa64pfr0);
+	kvm_set_vm_id_reg(kvm, SYS_ID_PFR1_EL1, pfr1);
 
 	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.

While we're at it, use the existing API instead of open-coded accessors to access the ID regs.

---
 arch/arm64/kvm/sys_regs.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

index ad82264c6cbe1..8ae2bca816148 100644
@@ -5609,11 +5609,13 @@ 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)) {
-		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;
+	if (!irqchip_in_kernel(kvm)) {
+		u64 val;
+
+		val = kvm_read_vm_id_reg(kvm, SYS_ID_AA64PFR0_EL1) & ~ID_AA64PFR0_EL1_GIC;
+		kvm_set_vm_id_reg(kvm, SYS_ID_AA64PFR0_EL1, val);
+		val = kvm_read_vm_id_reg(kvm, SYS_ID_PFR1_EL1) & ~ID_PFR1_EL1_GIC;
+		kvm_set_vm_id_reg(kvm, SYS_ID_PFR1_EL1, val);
 	}
 
 	if (vcpu_has_nv(vcpu)) {

2.47.3

Today's next/pending-fixes is showing regressions on a range of physical arm64 platforms (including at least a bunch of A53 systems, an A55 one and an A72 one) in the steal_time selftest which bisect to this patch. We get asserts in the kernel on ID register sets:

[  150.872407] WARNING: CPU: 0 PID: 2865 at arch/arm64/kvm/sys_regs.c:2353 kvm_set_vm_id_reg+0x9c/0xf4

...

[  151.045312] Call trace:
[  151.047780]  kvm_set_vm_id_reg+0x9c/0xf4 (P)
[  151.052098]  kvm_finalize_sys_regs+0x88/0x240
[  151.056504]  kvm_arch_vcpu_run_pid_change+0xb4/0x438
[  151.061527]  kvm_vcpu_ioctl+0x92c/0x9d0

which generate errors to userspace, causing the test to fail. Full log from one of the failing runs:

https://lava.sirena.org.uk/scheduler/job/2065669#L2863

Bisect log:

# bad: [55f97faf872612ac604ae72eb1968e6619cc41be] Merge branch 'for-linux-next-fixes' of https://gitlab.freedesktop.org/drm/misc/kernel.git
# good: [f850568efe3a7a9ec4df357cfad1f997f0058924] Merge tag 'i2c-for-6.18-rc5' of git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux
# good: [636f4618b1cd96f6b5a2b8c7c4f665c8533ecf13] regulator: fixed: fix GPIO descriptor leak on register failure
# good: [86d57d9c07d54e8cb385ffe800930816ccdba0c1] spi: imx: keep dma request disabled before dma transfer setup
# good: [939edfaa10f1d22e6af6a84bf4bd96dc49c67302] spi: xilinx: increase number of retries before declaring stall
# good: [3cd2018e15b3d66d2187d92867e265f45ad79e6f] spi: Try to get ACPI GPIO IRQ earlier
# good: [29528c8e643bb0c54da01237a35010c6438423d2] ASoC: tas2781: fix getting the wrong device number
# good: [3dc8c73365d3ca25c99e7e1a0f493039d7291df5] ASoC: codecs: va-macro: fix resource leak in probe error path
# good: [84f5526e4dce0a44d050ceb1b1bf21d43016d91b] ASoC: tas2783A: Fix issues in firmware parsing
# good: [8da0efc3da9312b65f5cbf06e57d284f69222b2e] ASoC: doc: cs35l56: Update firmware filename description for B0 silicon
# good: [249d96b492efb7a773296ab2c62179918301c146] ASoC: da7213: Use component driver suspend/resume
git bisect start '55f97faf872612ac604ae72eb1968e6619cc41be' 'f850568efe3a7a9ec4df357cfad1f997f0058924' '636f4618b1cd96f6b5a2b8c7c4f665c8533ecf13' '86d57d9c07d54e8cb385ffe800930816ccdba0c1' '939edfaa10f1d22e6af6a84bf4bd96dc49c67302' '3cd2018e15b3d66d2187d92867e265f45ad79e6f' '29528c8e643bb0c54da01237a35010c6438423d2' '3dc8c73365d3ca25c99e7e1a0f493039d7291df5' '84f5526e4dce0a44d050ceb1b1bf21d43016d91b' '8da0efc3da9312b65f5cbf06e57d284f69222b2e' '249d96b492efb7a773296ab2c62179918301c146'

# test job: [636f4618b1cd96f6b5a2b8c7c4f665c8533ecf13] https://lava.sirena.org.uk/scheduler/job/2048731
# test job: [86d57d9c07d54e8cb385ffe800930816ccdba0c1] https://lava.sirena.org.uk/scheduler/job/2053555
# test job: [939edfaa10f1d22e6af6a84bf4bd96dc49c67302] https://lava.sirena.org.uk/scheduler/job/2057909
# test job: [3cd2018e15b3d66d2187d92867e265f45ad79e6f] https://lava.sirena.org.uk/scheduler/job/2049140
# test job: [29528c8e643bb0c54da01237a35010c6438423d2] https://lava.sirena.org.uk/scheduler/job/2057927
# test job: [3dc8c73365d3ca25c99e7e1a0f493039d7291df5] https://lava.sirena.org.uk/scheduler/job/2054782
# test job: [84f5526e4dce0a44d050ceb1b1bf21d43016d91b] https://lava.sirena.org.uk/scheduler/job/2053642
# test job: [8da0efc3da9312b65f5cbf06e57d284f69222b2e] https://lava.sirena.org.uk/scheduler/job/2038272
# test job: [249d96b492efb7a773296ab2c62179918301c146] https://lava.sirena.org.uk/scheduler/job/2043898
# test job: [55f97faf872612ac604ae72eb1968e6619cc41be] https://lava.sirena.org.uk/scheduler/job/2065669
# bad: [55f97faf872612ac604ae72eb1968e6619cc41be] Merge branch 'for-linux-next-fixes' of https://gitlab.freedesktop.org/drm/misc/kernel.git
git bisect bad 55f97faf872612ac604ae72eb1968e6619cc41be
# test job: [1f007059d445f5a1904328b3d34ad462329ba314] https://lava.sirena.org.uk/scheduler/job/2065857
# good: [1f007059d445f5a1904328b3d34ad462329ba314] Merge branch 'usb-linus' of https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
git bisect good 1f007059d445f5a1904328b3d34ad462329ba314
# test job: [0319e334a50bdc96486bca1f44828c55ea9d3008] https://lava.sirena.org.uk/scheduler/job/2066320
# good: [0319e334a50bdc96486bca1f44828c55ea9d3008] Merge branch 'reset/fixes' of https://git.pengutronix.de/git/pza/linux
git bisect good 0319e334a50bdc96486bca1f44828c55ea9d3008
# test job: [4fc9ec35871028dc4db558d1bb74f18075544a61] https://lava.sirena.org.uk/scheduler/job/2066361
# bad: [4fc9ec35871028dc4db558d1bb74f18075544a61] Merge branch 'dma-mapping-fixes' of https://git.kernel.org/pub/scm/linux/kernel/git/mszyprowski/linux.git
git bisect bad 4fc9ec35871028dc4db558d1bb74f18075544a61
# test job: [0e5ba55750c1f7fb194a0022b8c887e6413da9b1] https://lava.sirena.org.uk/scheduler/job/2066403
# good: [0e5ba55750c1f7fb194a0022b8c887e6413da9b1] Merge tag 'kvm-x86-fixes-6.18-rc5' of https://github.com/kvm-x86/linux into HEAD
git bisect good 0e5ba55750c1f7fb194a0022b8c887e6413da9b1
# test job: [ca00c3af8ede65d16097d322be330146d9231bd2] https://lava.sirena.org.uk/scheduler/job/2066412
# bad: [ca00c3af8ede65d16097d322be330146d9231bd2] Merge tag 'kvmarm-fixes-6.18-2' of git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm into HEAD
git bisect bad ca00c3af8ede65d16097d322be330146d9231bd2
# test job: [103e17aac09cdd358133f9e00998b75d6c1f1518] https://lava.sirena.org.uk/scheduler/job/2066443
# good: [103e17aac09cdd358133f9e00998b75d6c1f1518] KVM: arm64: Check the untrusted offset in FF-A memory share
git bisect good 103e17aac09cdd358133f9e00998b75d6c1f1518
# test job: [50e7cce81b9b2fbd6f0104c1698959d45ce3cf58] https://lava.sirena.org.uk/scheduler/job/2066695
# bad: [50e7cce81b9b2fbd6f0104c1698959d45ce3cf58] KVM: arm64: Limit clearing of ID_{AA64PFR0,PFR1}_EL1.GIC to userspace irqchip
git bisect bad 50e7cce81b9b2fbd6f0104c1698959d45ce3cf58
# test job: [8a9866ff860052efc5f9766f3f87fae30c983156] https://lava.sirena.org.uk/scheduler/job/2067273
# good: [8a9866ff860052efc5f9766f3f87fae30c983156] KVM: arm64: Set ID_{AA64PFR0,PFR1}_EL1.GIC when GICv3 is configured
git bisect good 8a9866ff860052efc5f9766f3f87fae30c983156
# first bad commit: [50e7cce81b9b2fbd6f0104c1698959d45ce3cf58] KVM: arm64: Limit clearing of ID_{AA64PFR0,PFR1}_EL1.GIC to userspace irqchip

Please name the platforms this fails on. Here, on a sample of one A72 box, I don't see the issue:

	maz@sy-borg:~$ ./steal_time
	Random seed: 0x6b8b4567
	TAP version 13
	1..4
	ok 1 vcpu0
	ok 2 vcpu1
	ok 3 vcpu2
	ok 4 vcpu3
	# Totals: pass:4 fail:0 xfail:0 xpass:0 skip:0 error:0

Thanks,

	M.

Without deviation from the norm, progress is not possible.

It looks like it's GICv2 that's affected - I'm seeing this on at least Raspberry Pi 3B+ and 4, Pine 64 Plus and Libretech Potato, Solitude and Tritum. The platforms with GICv3 that I have results for (eg, the Toradex Verdin i.MX8MP and Mallow AM625) all seem fine.

Yeah, I just found out by exhuming the dusty dregs. As it turns out, this catches a pre-existing bug that wasn't noticed until we moved over to the standard accessors rather than bypassing them.

The hack below fixes it for me on XGene.

	M.

index 3bf7005258f07..19afcd833d6fa 100644
@@ -5624,7 +5624,11 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)
 
 	guard(mutex)(&kvm->arch.config_lock);
 
-	if (!irqchip_in_kernel(kvm)) {
+	/*
+	 * This hacks into the ID registers, so only perform it when the
+	 * first vcpu runs, or the kvm_set_vm_id_reg() helper will scream.
+	 */
+	if (!irqchip_in_kernel(kvm) && !kvm_vm_has_ran_once(kvm)) {
 		u64 val;
 
 		val = kvm_read_vm_id_reg(kvm, SYS_ID_AA64PFR0_EL1) & ~ID_AA64PFR0_EL1_GIC;

Without deviation from the norm, progress is not possible.

Yes, that seems to work for me too on the boards I tried. Thanks!

Thanks,
Oliver

Hi Marc

But, all 3 patches have the same Fixes tag, was that intentional ?

Otherwise looks good.

Suzuki

With these fixes, I can happily

Yes. Fixes don't necessarily need backporting, and the latter ones, while making things less awful, are not absolutely necessary to fix 6.12.

However, I have no doubt that the Artificial Idiots that automatically backport stuff to stable will pick whatever seems to apply without any discrimination.

I've stopped fighting that battle.

	M.

Without deviation from the norm, progress is not possible.

Applied to fixes, thanks!

[1/3] KVM: arm64: Make all 32bit ID registers fully writable

      commit: 3f9eacf4f0705876a5d6526d7d320ca91d7d7a16
[2/3] KVM: arm64: Set ID_{AA64PFR0,PFR1}_EL1.GIC when GICv3 is configured
      commit: 8a9866ff860052efc5f9766f3f87fae30c983156
[3/3] KVM: arm64: Limit clearing of ID_{AA64PFR0,PFR1}_EL1.GIC to userspace irqchip
      commit: 50e7cce81b9b2fbd6f0104c1698959d45ce3cf58

Cheers,

	M.

Without deviation from the norm, progress is not possible.