diff mbox series

[v2,05/11] KVM: arm64: Zero ID_AA64PFR0_EL1.GIC when no GICv3 is presented to the guest

Message ID 20240827152517.3909653-6-maz@kernel.org (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: Handle the lack of GICv3 exposed to a guest | expand

Commit Message

Marc Zyngier Aug. 27, 2024, 3:25 p.m. UTC
In order to be consistent, we shouldn't advertise a GICv3 when none
is actually usable by the guest.

Wipe the feature when these conditions apply, and allow the field
to be written from userspace.

This now allows us to rewrite the kvm_has_gicv3 helper() in terms
of kvm_has_feat(), given that it is always evaluated at runtime.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c  | 8 +++++++-
 arch/arm64/kvm/vgic/vgic.h | 4 +---
 2 files changed, 8 insertions(+), 4 deletions(-)

Comments

Mark Brown Aug. 28, 2024, 11:22 p.m. UTC | #1
On Tue, Aug 27, 2024 at 04:25:11PM +0100, Marc Zyngier wrote:
> In order to be consistent, we shouldn't advertise a GICv3 when none
> is actually usable by the guest.
> 
> Wipe the feature when these conditions apply, and allow the field
> to be written from userspace.
> 
> This now allows us to rewrite the kvm_has_gicv3 helper() in terms
> of kvm_has_feat(), given that it is always evaluated at runtime.

This patch, which is in -next, is causing the set_id_regs tests to fail
on a variety of platforms including synquacer (it looks to be everything
with GICv3 which wouldn't be surprising but I didn't confirm):

# selftests: kvm: set_id_regs
# Random seed: 0x6b8b4567
# TAP version 13
# 1..81
# ok 1 ID_AA64DFR0_EL1_PMUVer
# ok 2 ID_AA64DFR0_EL1_DebugVer

...

# ok 79 ID_AA64ZFR0_EL1_SVEver
# ok 80 test_vcpu_ftr_id_regs
# ==== Test Assertion Failure ====
#   aarch64/set_id_regs.c:449: test_reg_vals[encoding_to_range_idx(uc.args[2])] == uc.args[3]
#   pid=1716 tid=1716 errno=4 - Interrupted system call
#      1	0x000000000040249f: test_guest_reg_read at set_id_regs.c:449
#      2	0x0000000000401cc7: main at set_id_regs.c:580
#      3	0x0000ffff9a737543: ?? ??:0
#      4	0x0000ffff9a737617: ?? ??:0
#      5	0x0000000000401e2f: _start at ??:?
#   0x1001111 != 0x1111 (test_reg_vals[encoding_to_range_idx(uc.args[2])] != uc.args[3])
not ok 6 selftests: kvm: set_id_regs # exit=254

That's running test_reset_preserves_id_regs.

Full log at:

   https://lava.sirena.org.uk/scheduler/job/661438

The bisection converges fairly smoothly:

git bisect start
# status: waiting for both good and bad commits
# bad: [195a402a75791e6e0d96d9da27ca77671bc656a8] Add linux-next specific files for 20240828
git bisect bad 195a402a75791e6e0d96d9da27ca77671bc656a8
# status: waiting for good commit(s), bad commit known
# good: [0c78c247d6e9dddf53ea0ac009ccc3399f9203ae] Merge branch 'for-linux-next-fixes' of https://gitlab.freedesktop.org/drm/misc/kernel.git
git bisect good 0c78c247d6e9dddf53ea0ac009ccc3399f9203ae
# good: [319121b3e57ddefccb36ca4af417ae602c9f97bc] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
git bisect good 319121b3e57ddefccb36ca4af417ae602c9f97bc
# good: [ed51c1e6d8adb0fc6ec023d7473627e03c6b0a2e] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux-dt.git
git bisect good ed51c1e6d8adb0fc6ec023d7473627e03c6b0a2e
# bad: [125bc49fbc0e1333b9602c1200153f2763cb0a3c] Merge branch 'char-misc-next' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
git bisect bad 125bc49fbc0e1333b9602c1200153f2763cb0a3c
# good: [c55ba156ad53e2b1f56b3cc53174b0860aef1b10] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/rcu/linux.git
git bisect good c55ba156ad53e2b1f56b3cc53174b0860aef1b10
# bad: [8765b2fe7b05721f45e4320c2290c6a7e4f2ccd3] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git
git bisect bad 8765b2fe7b05721f45e4320c2290c6a7e4f2ccd3
# bad: [bf8600945fa1536cea05731775d5f7d62e8632c8] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git
git bisect bad bf8600945fa1536cea05731775d5f7d62e8632c8
# bad: [be43d82250a5d125e578065615ca805359dc58fe] Merge branch 'topic/ppc-kvm' of git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git
git bisect bad be43d82250a5d125e578065615ca805359dc58fe
# bad: [1093a3758142ce8613341ae39fb4591383d5df0a] Merge branch kvm-arm64/vgic-sre-traps into kvmarm-master/next
git bisect bad 1093a3758142ce8613341ae39fb4591383d5df0a
# good: [d17317a13fd0498cfa2b4bf9c4eebbe5adf929ab] Merge branch kvm-arm64/fpmr into kvmarm-master/next
git bisect good d17317a13fd0498cfa2b4bf9c4eebbe5adf929ab
# bad: [9f5deace58da737d67ec9c2d23534a475be68481] KVM: arm64: Add ICH_HCR_EL2 to the vcpu state
git bisect bad 9f5deace58da737d67ec9c2d23534a475be68481
# good: [8d917e0a8651377321c06513f42e2ab9a86161f4] KVM: arm64: Force GICv3 trap activation when no irqchip is configured on VHE
git bisect good 8d917e0a8651377321c06513f42e2ab9a86161f4
# bad: [5cb57a1aff7551bcb3b800d33141b06ef0ac178b] KVM: arm64: Zero ID_AA64PFR0_EL1.GIC when no GICv3 is presented to the guest
git bisect bad 5cb57a1aff7551bcb3b800d33141b06ef0ac178b
# good: [795a0bbaeee2aa993338166bc063fe3c89373d2a] KVM: arm64: Add helper for last ditch idreg adjustments
git bisect good 795a0bbaeee2aa993338166bc063fe3c89373d2a
# first bad commit: [5cb57a1aff7551bcb3b800d33141b06ef0ac178b] KVM: arm64: Zero ID_AA64PFR0_EL1.GIC when no GICv3 is presented to the guest
Oliver Upton Aug. 29, 2024, 12:48 a.m. UTC | #2
On Thu, Aug 29, 2024 at 12:22:52AM +0100, Mark Brown wrote:
> On Tue, Aug 27, 2024 at 04:25:11PM +0100, Marc Zyngier wrote:
> > In order to be consistent, we shouldn't advertise a GICv3 when none
> > is actually usable by the guest.
> > 
> > Wipe the feature when these conditions apply, and allow the field
> > to be written from userspace.
> > 
> > This now allows us to rewrite the kvm_has_gicv3 helper() in terms
> > of kvm_has_feat(), given that it is always evaluated at runtime.
> 
> This patch, which is in -next, is causing the set_id_regs tests to fail
> on a variety of platforms including synquacer (it looks to be everything
> with GICv3 which wouldn't be surprising but I didn't confirm):

Mind taking this for a spin? Seems to do the trick on my end.

https://lore.kernel.org/kvmarm/20240829004622.3058639-1-oliver.upton@linux.dev/
diff mbox series

Patch

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index bc2d54da3827..e9d8e916e3af 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2365,7 +2365,6 @@  static const struct sys_reg_desc sys_reg_descs[] = {
 		   ID_AA64PFR0_EL1_MPAM |
 		   ID_AA64PFR0_EL1_SVE |
 		   ID_AA64PFR0_EL1_RAS |
-		   ID_AA64PFR0_EL1_GIC |
 		   ID_AA64PFR0_EL1_AdvSIMD |
 		   ID_AA64PFR0_EL1_FP), },
 	ID_SANITISED(ID_AA64PFR1_EL1),
@@ -4634,6 +4633,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 (vcpu_has_nv(vcpu)) {
 		int ret = kvm_init_nv_sysregs(kvm);
 		if (ret)
diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h
index c72c38b44234..f2486b4d9f95 100644
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -350,9 +350,7 @@  void vcpu_set_ich_hcr(struct kvm_vcpu *vcpu);
 
 static inline bool kvm_has_gicv3(struct kvm *kvm)
 {
-	return (static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif) &&
-		irqchip_in_kernel(kvm) &&
-		kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3);
+	return kvm_has_feat(kvm, ID_AA64PFR0_EL1, GIC, IMP);
 }
 
 #endif