Skip to contentSkip to conversation

KVM: arm64: Handle the lack of GICv3 exposed to a guest

PATCH Applied Marc Zyngier 3 participants 27 messages Aug 20, 2024 – Aug 22, 2024

Subsystems (5)
Stable branch KVM Kselftest arm64 +1 more subsystems

v1 of 2 Applied Latest is v2 →

lore
Share & tools
lore

Marc Zyngier

author maintainer
#0 lore raw

cover KVM: arm64: Handle the lack of GICv3 exposed to a guest

It recently appeared that, when running on a GICv3-equipped platform (which is what non-ancient arm64 HW has), *not* configuring a GICv3 for the guest could result in less than desirable outcomes.

We have multiple issues to fix:

- for registers that *always* trap (the SGI registers) or that *may* trap (the SRE register), we need to check whether a GICv3 has been instantiated before acting upon the trap.

- for registers that only conditionally trap, we must actively trap them even in the absence of a GICv3 being instantiated, and handle those traps accordingly.

- finally, ID registers must reflect the absence of a GICv3, so that we are consistent.

This series goes through all these requirements. The main complexity here is to apply a GICv3 configuration on the host in the absence of a GICv3 in the guest. This is pretty hackish, but I don't have a much better solution so far.

As part of making wider use of of the trap bits, we fully define the trap routing as per the architecture, something that we eventually need for NV anyway.

Note that patch #1 is a candidate for immediate merge in 6.11 as a fix, to be backported to all stable versions. We can live without the rest.

Finally, I have added two additional changes:

- a file-wide cleanup of sys_regs.c, unifying the way we inject an UNDEF from the trap handling array

- a selftest that checks for the implemented trapping behaviour (yes, I actually wrote a test -- hated every minute of it).

Note that the effects of this series when a GICv2 is configured on a GICv3 host capable of emulation are imperfect: For some of the registers, the guest may take a system register trap at EL1 (EC=0x18), and there is nothing that KVM can do about it (this is a consequence of ICC_SRE_EL1.SRE being 0, which GICv2 requires). But at least that's a guest problem, not the host's.

PAtches on top of v6.11-rc4, tested on the usual lot of terrible HW:
Synquacer, TX1 and M1.

Marc Zyngier (12):
KVM: arm64: Make ICC_*SGI*_EL1 undef in the absence of a vGICv3
KVM: arm64: Move GICv3 trap configuration to kvm_calculate_traps()
KVM: arm64: Force SRE traps when SRE access is not enabled
KVM: arm64: Force GICv3 traps activa when no irqchip is configured on
VHE
KVM: arm64: Add helper for last ditch idreg adjustments
KVM: arm64: Zero ID_AA64PFR0_EL1.GIC when no GICv3 is presented to the
guest
KVM: arm64: Add ICH_HCR_EL2 to the vcpu state
KVM: arm64: Add trap routing information for ICH_HCR_EL2
KVM: arm64: Honor guest requested traps in GICv3 emulation
KVM: arm64: Make most GICv3 accesses UNDEF if they trap
KVM: arm64: Unify UNDEF injection helpers
KVM: arm64: Add selftest checking how the absence of GICv3 is handled

 arch/arm64/include/asm/kvm_host.h             |   2 +
 arch/arm64/kvm/arm.c                          |  10 +-
 arch/arm64/kvm/emulate-nested.c               |  77 +++++-
 arch/arm64/kvm/hyp/vgic-v3-sr.c               |  97 ++++++-
 arch/arm64/kvm/nested.c                       |  15 +-
 arch/arm64/kvm/sys_regs.c                     | 236 +++++++++++-------
 arch/arm64/kvm/sys_regs.h                     |   9 +
 arch/arm64/kvm/vgic/vgic-v3.c                 |  12 +
 arch/arm64/kvm/vgic/vgic.c                    |  14 +-
 arch/arm64/kvm/vgic/vgic.h                    |   9 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/no-vgic-v3.c        | 170 +++++++++++++
 12 files changed, 526 insertions(+), 126 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/no-vgic-v3.c
Signature · 1 line
2.39.2
author maintainer
#1 lore raw

PATCH 1/12 KVM: arm64: Make ICC_*SGI*_EL1 undef in the absence of a vGICv3

On a system with a GICv3, if a guest hasn't been configured with GICv3 and that the host is not capable of GICv2 emulation, a write to any of the ICC_*SGI*_EL1 registers is trapped to EL2.

We therefore try to emulate the SGI access, only to hit a NULL pointer as no private interrupt is allocated (no GIC, remember?).

The obvious fix is to give the guest what it deserves, in the shape of a UNDEF exception.

Reported-by Alexander Potapenko <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
Cc [email protected]
---
 arch/arm64/kvm/sys_regs.c  | 6 ++++++
 arch/arm64/kvm/vgic/vgic.h | 7 +++++++
 2 files changed, 13 insertions(+)
arch/arm64/kvm/sys_regs.c +6 -0
index c90324060436..31e49da867ff 100644
@@ -33,6 +33,7 @@
 #include <trace/events/kvm.h>
 
 #include "sys_regs.h"
+#include "vgic/vgic.h"
 
 #include "trace.h"
 

@@ -435,6 +436,11 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 {
 	bool g1;
 
+	if (!kvm_has_gicv3(vcpu->kvm)) {
+		kvm_inject_undefined(vcpu);
+		return false;
+	}
+
 	if (!p->is_write)
 		return read_from_write_only(vcpu, p, r);
arch/arm64/kvm/vgic/vgic.h +7 -0
index ba8f790431bd..8532bfe3fed4 100644
@@ -346,4 +346,11 @@ void vgic_v4_configure_vsgis(struct kvm *kvm);
 void vgic_v4_get_vlpi_state(struct vgic_irq *irq, bool *val);
 int vgic_v4_request_vpe_irq(struct kvm_vcpu *vcpu, int irq);
 
+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);
+}
+
 #endif
Signature · 1 line
2.39.2
#2 lore raw

Re: [PATCH 01/12] KVM: arm64: Make ICC_*SGI*_EL1 undef in the absence of a vGICv3

Quoted 7 lines · Marc Zyngier

On a system with a GICv3, if a guest hasn't been configured with GICv3 and that the host is not capable of GICv2 emulation, a write to any of the ICC_*SGI*_EL1 registers is trapped to EL2.

We therefore try to emulate the SGI access, only to hit a NULL pointer as no private interrupt is allocated (no GIC, remember?).

Quoted 6 lines

The obvious fix is to give the guest what it deserves, in the shape of a UNDEF exception.

Reported-by Alexander Potapenko <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
Cc [email protected]

LGTM, and just as an FYI I do plan on grabbing this for 6.11

Signature · 2 lines
Thanks,
Oliver
author maintainer
#3 lore raw

Re: [PATCH 01/12] KVM: arm64: Make ICC_*SGI*_EL1 undef in the absence of a vGICv3

Quoted 9 lines · Oliver Upton
Quoted 7 lines · Marc Zyngier

On a system with a GICv3, if a guest hasn't been configured with GICv3 and that the host is not capable of GICv2 emulation, a write to any of the ICC_*SGI*_EL1 registers is trapped to EL2.

We therefore try to emulate the SGI access, only to hit a NULL pointer as no private interrupt is allocated (no GIC, remember?).

Quoted 8 lines
Quoted 6 lines

The obvious fix is to give the guest what it deserves, in the shape of a UNDEF exception.

Reported-by Alexander Potapenko <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
Cc [email protected]

LGTM, and just as an FYI I do plan on grabbing this for 6.11

Great, thanks. Are you planning to route this via arm64, given that Paolo is away for a bit?

	M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
#4 lore raw

Re: [PATCH 01/12] KVM: arm64: Make ICC_*SGI*_EL1 undef in the absence of a vGICv3

Quoted 14 lines · Marc Zyngier
Quoted 12 lines · Oliver Upton
Quoted 10 lines · Marc Zyngier

On a system with a GICv3, if a guest hasn't been configured with GICv3 and that the host is not capable of GICv2 emulation, a write to any of the ICC_*SGI*_EL1 registers is trapped to EL2.

We therefore try to emulate the SGI access, only to hit a NULL pointer as no private interrupt is allocated (no GIC, remember?).

The obvious fix is to give the guest what it deserves, in the shape of a UNDEF exception.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Reported-by Alexander Potapenko <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
Cc [email protected]

LGTM, and just as an FYI I do plan on grabbing this for 6.11

Great, thanks. Are you planning to route this via arm64, given that Paolo is away for a bit?

Yup, exactly that. I'll send the PR in the next day or two when I have some time to kick the tires on everything.

Signature · 2 lines
Thanks,
Oliver
Applied
#5 lore raw

Re: (subset) [PATCH 01/12] KVM: arm64: Make ICC_*SGI*_EL1 undef in the absence of a vGICv3

In reply to Marc Zyngier
Quoted 8 lines · Marc Zyngier

On a system with a GICv3, if a guest hasn't been configured with GICv3 and that the host is not capable of GICv2 emulation, a write to any of the ICC_*SGI*_EL1 registers is trapped to EL2.

We therefore try to emulate the SGI access, only to hit a NULL pointer as no private interrupt is allocated (no GIC, remember?).

[...]

Applied to kvmarm/fixes, thanks!

[01/12] KVM: arm64: Make ICC_*SGI*_EL1 undef in the absence of a vGICv3

        https://git.kernel.org/kvmarm/kvmarm/c/3e6245ebe7ef

--
Best,
Oliver

author maintainer
#6 lore raw

PATCH 2/12 KVM: arm64: Move GICv3 trap configuration to kvm_calculate_traps()

In reply to Marc Zyngier

Follow the pattern introduced with vcpu_set_hcr(), and introduce vcpu_set_ich_hcr(), which configures the GICv3 traps at the same point.

This will allow future changes to introduce trap configuration on a per-VM basis.

Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/sys_regs.c     | 1 +
 arch/arm64/kvm/vgic/vgic-v3.c | 9 +++++++++
 arch/arm64/kvm/vgic/vgic.h    | 2 ++
 3 files changed, 12 insertions(+)
arch/arm64/kvm/sys_regs.c +1 -0
index 31e49da867ff..257c8da23a4e 100644
@@ -4551,6 +4551,7 @@ void kvm_calculate_traps(struct kvm_vcpu *vcpu)
 
 	mutex_lock(&kvm->arch.config_lock);
 	vcpu_set_hcr(vcpu);
+	vcpu_set_ich_hcr(vcpu);
 
 	if (cpus_have_final_cap(ARM64_HAS_HCX)) {
 		/*
arch/arm64/kvm/vgic/vgic-v3.c +9 -0
index 3eecdd2f4b8f..11718412921f 100644
@@ -292,6 +292,15 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu)
 
 	/* Get the show on the road... */
 	vgic_v3->vgic_hcr = ICH_HCR_EN;
+}
+
+void vcpu_set_ich_hcr(struct kvm_vcpu *vcpu)
+{
+	struct vgic_v3_cpu_if *vgic_v3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (!kvm_has_gicv3(vcpu->kvm))
+		return;
+
 	if (group0_trap)
 		vgic_v3->vgic_hcr |= ICH_HCR_TALL0;
 	if (group1_trap)
arch/arm64/kvm/vgic/vgic.h +2 -0
index 8532bfe3fed4..c72c38b44234 100644
@@ -346,6 +346,8 @@ void vgic_v4_configure_vsgis(struct kvm *kvm);
 void vgic_v4_get_vlpi_state(struct vgic_irq *irq, bool *val);
 int vgic_v4_request_vpe_irq(struct kvm_vcpu *vcpu, int irq);
 
+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) &&
Signature · 1 line
2.39.2
author maintainer
#7 lore raw

PATCH 3/12 KVM: arm64: Force SRE traps when SRE access is not enabled

In reply to Marc Zyngier

We so far only write the ICH_HCR_EL2 config in two situations:

- when we need to emulate the GICv3 CPU interface due to HW bugs

- when we do direct injection, as the virtual CPU interface needs to be enabled

This is all good. But it also means that we don't do anything special when we emulate a GICv2, or that there is no GIC at all.

What happens in this case when the guest uses the GICv3 system registers? The *guest* gets a trap for a sysreg access (EC=0x18) while we'd really like it to get an UNDEF.

Fixing this is a bit involved:

- we need to set all the required trap bits (TC, TALL0, TALL1, TDIR)

- for these traps to take effect, we need to (counter-intuitively) set ICC_SRE_EL1.SRE to 1 so that the above traps take priority.

Note that doesn't fully work when GICv2 emulation is enabled, as we cannot set ICC_SRE_EL1.SRE to 1 (it breaks Group0 delivery as IRQ).

Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 22 ++++++++++++++++------
 arch/arm64/kvm/vgic/vgic-v3.c   |  5 ++++-
 2 files changed, 20 insertions(+), 7 deletions(-)
arch/arm64/kvm/hyp/vgic-v3-sr.c +16 -6
index 7b397fad26f2..a184def8f5ad 100644
@@ -268,8 +268,16 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if)
 	 * starting to mess with the rest of the GIC, and VMCR_EL2 in
 	 * particular.  This logic must be called before
 	 * __vgic_v3_restore_state().
+	 *
+	 * However, if the vgic is disabled (ICH_HCR_EL2.EN==0), no GIC is
+	 * provisionned at all. In order to prevent illegal accesses to the
+	 * system registers to trap to EL1 (duh), force ICC_SRE_EL1.SRE to 1
+	 * so that the trap bits can take effect. Yes, we *loves* the GIC.
 	 */
-	if (!cpu_if->vgic_sre) {
+	if (!(cpu_if->vgic_hcr & ICH_HCR_EN)) {
+		write_gicreg(ICC_SRE_EL1_SRE, ICC_SRE_EL1);
+		isb();
+	} else if (!cpu_if->vgic_sre) {
 		write_gicreg(0, ICC_SRE_EL1);
 		isb();
 		write_gicreg(cpu_if->vgic_vmcr, ICH_VMCR_EL2);

@@ -288,8 +296,9 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if)
 	}
 
 	/*
-	 * Prevent the guest from touching the GIC system registers if
-	 * SRE isn't enabled for GICv3 emulation.
+	 * Prevent the guest from touching the ICC_SRE_EL1 system
+	 * register. Note that this may not have any effect, as
+	 * ICC_SRE_EL2.Enable being RAO/WI is a valid implementation.
 	 */
 	write_gicreg(read_gicreg(ICC_SRE_EL2) & ~ICC_SRE_EL2_ENABLE,
 		     ICC_SRE_EL2);

@@ -297,10 +306,11 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if)
 	/*
 	 * If we need to trap system registers, we must write
 	 * ICH_HCR_EL2 anyway, even if no interrupts are being
-	 * injected,
+	 * injected. Note that this also applies if we don't expect
+	 * any system register access (GICv2 or no vgic at all).
 	 */
 	if (static_branch_unlikely(&vgic_v3_cpuif_trap) ||
-	    cpu_if->its_vpe.its_vm)
+	    cpu_if->its_vpe.its_vm || !cpu_if->vgic_sre)
 		write_gicreg(cpu_if->vgic_hcr, ICH_HCR_EL2);
 }
 

@@ -326,7 +336,7 @@ void __vgic_v3_deactivate_traps(struct vgic_v3_cpu_if *cpu_if)
 	 * no interrupts were being injected, and we disable it again here.
 	 */
 	if (static_branch_unlikely(&vgic_v3_cpuif_trap) ||
-	    cpu_if->its_vpe.its_vm)
+	    cpu_if->its_vpe.its_vm || !cpu_if->vgic_sre)
 		write_gicreg(0, ICH_HCR_EL2);
 }
arch/arm64/kvm/vgic/vgic-v3.c +4 -1
index 11718412921f..b217b256853c 100644
@@ -298,8 +298,11 @@ void vcpu_set_ich_hcr(struct kvm_vcpu *vcpu)
 {
 	struct vgic_v3_cpu_if *vgic_v3 = &vcpu->arch.vgic_cpu.vgic_v3;
 
-	if (!kvm_has_gicv3(vcpu->kvm))
+	/* Hide GICv3 sysreg if necessary */
+	if (!kvm_has_gicv3(vcpu->kvm)) {
+		vgic_v3->vgic_hcr |= ICH_HCR_TALL0 | ICH_HCR_TALL1 | ICH_HCR_TC;
 		return;
+	}
 
 	if (group0_trap)
 		vgic_v3->vgic_hcr |= ICH_HCR_TALL0;
Signature · 1 line
2.39.2
#8 lore raw

Re: [PATCH 03/12] KVM: arm64: Force SRE traps when SRE access is not enabled

Hey,

Quoted 16 lines · Marc Zyngier

We so far only write the ICH_HCR_EL2 config in two situations:

- when we need to emulate the GICv3 CPU interface due to HW bugs

- when we do direct injection, as the virtual CPU interface needs to be enabled

This is all good. But it also means that we don't do anything special when we emulate a GICv2, or that there is no GIC at all.

What happens in this case when the guest uses the GICv3 system registers? The *guest* gets a trap for a sysreg access (EC=0x18) while we'd really like it to get an UNDEF.

Fixing this is a bit involved:

Quoted 8 lines

- we need to set all the required trap bits (TC, TALL0, TALL1, TDIR)

- for these traps to take effect, we need to (counter-intuitively) set ICC_SRE_EL1.SRE to 1 so that the above traps take priority.

Note that doesn't fully work when GICv2 emulation is enabled, as we cannot set ICC_SRE_EL1.SRE to 1 (it breaks Group0 delivery as IRQ).

Just to make sure I'm following completely, GICv2-on-GICv3 guest sees the (barf) architected behavior of sysreg traps going to EL1.

Quoted 11 lines
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 22 ++++++++++++++++------
 arch/arm64/kvm/vgic/vgic-v3.c   |  5 ++++-
 2 files changed, 20 insertions(+), 7 deletions(-)
arch/arm64/kvm/hyp/vgic-v3-sr.c +0 -0
index 7b397fad26f2..a184def8f5ad 100644

arch/arm64/kvm/hyp/vgic-v3-sr.c void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if) · line 273

Quoted 6 lines
arch/arm64/kvm/hyp/vgic-v3-sr.c +3 -0
@@ -268,8 +268,16 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if)
 	 * starting to mess with the rest of the GIC, and VMCR_EL2 in
 	 * particular.  This logic must be called before
 	 * __vgic_v3_restore_state().
+	 *
+	 * However, if the vgic is disabled (ICH_HCR_EL2.EN==0), no GIC is
+	 * provisionned at all. In order to prevent illegal accesses to the

typo: provisioned

Quoted 12 lines
arch/arm64/kvm/hyp/vgic-v3-sr.c +6 -1
+	 * system registers to trap to EL1 (duh), force ICC_SRE_EL1.SRE to 1
+	 * so that the trap bits can take effect. Yes, we *loves* the GIC.
 	 */
-	if (!cpu_if->vgic_sre) {
+	if (!(cpu_if->vgic_hcr & ICH_HCR_EN)) {
+		write_gicreg(ICC_SRE_EL1_SRE, ICC_SRE_EL1);
+		isb();
+	} else if (!cpu_if->vgic_sre) {
 		write_gicreg(0, ICC_SRE_EL1);
 		isb();
 		write_gicreg(cpu_if->vgic_vmcr, ICH_VMCR_EL2);

arch/arm64/kvm/hyp/vgic-v3-sr.c void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if) · line 301

Quoted 8 lines
arch/arm64/kvm/hyp/vgic-v3-sr.c +3 -2
@@ -288,8 +296,9 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if)
 	}
 
 	/*
-	 * Prevent the guest from touching the GIC system registers if
-	 * SRE isn't enabled for GICv3 emulation.
+	 * Prevent the guest from touching the ICC_SRE_EL1 system
+	 * register. Note that this may not have any effect, as
+	 * ICC_SRE_EL2.Enable being RAO/WI is a valid implementation.

So this behavior is weird but still 'safe' as, ICC_SRE_EL1.SRE is also RAO/WI and the HCR traps are still effective. Right?

arch/arm64/kvm/hyp/vgic-v3-sr.c void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if) · line 310

Quoted 10 lines
arch/arm64/kvm/hyp/vgic-v3-sr.c +2 -1
 	 */
 	write_gicreg(read_gicreg(ICC_SRE_EL2) & ~ICC_SRE_EL2_ENABLE,
 		     ICC_SRE_EL2);

@@ -297,10 +306,11 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if)
 	/*
 	 * If we need to trap system registers, we must write
 	 * ICH_HCR_EL2 anyway, even if no interrupts are being
-	 * injected,
+	 * injected. Note that this also applies if we don't expect
+	 * any system register access (GICv2 or no vgic at all).

We don't expect the traps to come in the GICv2 case, though, right?

Looks alright to me otherwise, but blech!

Signature · 2 lines
Thanks,
Oliver
author maintainer
#9 lore raw

Re: [PATCH 03/12] KVM: arm64: Force SRE traps when SRE access is not enabled

Quoted 25 lines · Oliver Upton

Hey,

Quoted 21 lines · Marc Zyngier

We so far only write the ICH_HCR_EL2 config in two situations:

- when we need to emulate the GICv3 CPU interface due to HW bugs

- when we do direct injection, as the virtual CPU interface needs to be enabled

This is all good. But it also means that we don't do anything special when we emulate a GICv2, or that there is no GIC at all.

What happens in this case when the guest uses the GICv3 system registers? The *guest* gets a trap for a sysreg access (EC=0x18) while we'd really like it to get an UNDEF.

Fixing this is a bit involved:

- we need to set all the required trap bits (TC, TALL0, TALL1, TDIR)

- for these traps to take effect, we need to (counter-intuitively) set ICC_SRE_EL1.SRE to 1 so that the above traps take priority.

Quoted 6 lines
Quoted 3 lines

Note that doesn't fully work when GICv2 emulation is enabled, as we cannot set ICC_SRE_EL1.SRE to 1 (it breaks Group0 delivery as IRQ).

Just to make sure I'm following completely, GICv2-on-GICv3 guest sees the (barf) architected behavior of sysreg traps going to EL1.

Indeed. There isn't anything we can do about that, short of changing the behaviour of GICv2 emulation. I'll add something to that effect in the commit message.

Quoted 35 lines
Quoted 12 lines
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 22 ++++++++++++++++------
 arch/arm64/kvm/vgic/vgic-v3.c   |  5 ++++-
 2 files changed, 20 insertions(+), 7 deletions(-)
arch/arm64/kvm/hyp/vgic-v3-sr.c +0 -0
index 7b397fad26f2..a184def8f5ad 100644

arch/arm64/kvm/hyp/vgic-v3-sr.c void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if) · line 273

Quoted 6 lines
arch/arm64/kvm/hyp/vgic-v3-sr.c +3 -0
@@ -268,8 +268,16 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if)
 	 * starting to mess with the rest of the GIC, and VMCR_EL2 in
 	 * particular.  This logic must be called before
 	 * __vgic_v3_restore_state().
+	 *
+	 * However, if the vgic is disabled (ICH_HCR_EL2.EN==0), no GIC is
+	 * provisionned at all. In order to prevent illegal accesses to the

typo: provisioned

Quoted 14 lines
arch/arm64/kvm/hyp/vgic-v3-sr.c +6 -1
+	 * system registers to trap to EL1 (duh), force ICC_SRE_EL1.SRE to 1
+	 * so that the trap bits can take effect. Yes, we *loves* the GIC.
 	 */
-	if (!cpu_if->vgic_sre) {
+	if (!(cpu_if->vgic_hcr & ICH_HCR_EN)) {
+		write_gicreg(ICC_SRE_EL1_SRE, ICC_SRE_EL1);
+		isb();
+	} else if (!cpu_if->vgic_sre) {
 		write_gicreg(0, ICC_SRE_EL1);
 		isb();
 		write_gicreg(cpu_if->vgic_vmcr, ICH_VMCR_EL2);

@@ -288,8 +296,9 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if)
 	}
Quoted 9 lines

arch/arm64/kvm/hyp/vgic-v3-sr.c void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if) · line 301

Quoted 6 lines
arch/arm64/kvm/hyp/vgic-v3-sr.c +3 -2
 	/*
-	 * Prevent the guest from touching the GIC system registers if
-	 * SRE isn't enabled for GICv3 emulation.
+	 * Prevent the guest from touching the ICC_SRE_EL1 system
+	 * register. Note that this may not have any effect, as
+	 * ICC_SRE_EL2.Enable being RAO/WI is a valid implementation.

So this behavior is weird but still 'safe' as, ICC_SRE_EL1.SRE is also RAO/WI and the HCR traps are still effective. Right?

Exactly. All the traps are working, *except* for ICC_SRE_EL1 itself. A guest can then infer that it runs under a hypervisor, but not much more.

Quoted 13 lines

arch/arm64/kvm/hyp/vgic-v3-sr.c void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if) · line 310

Quoted 11 lines
arch/arm64/kvm/hyp/vgic-v3-sr.c +2 -1
 	 */
 	write_gicreg(read_gicreg(ICC_SRE_EL2) & ~ICC_SRE_EL2_ENABLE,
 		     ICC_SRE_EL2);

@@ -297,10 +306,11 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if)
 	/*
 	 * If we need to trap system registers, we must write
 	 * ICH_HCR_EL2 anyway, even if no interrupts are being
-	 * injected,
+	 * injected. Note that this also applies if we don't expect
+	 * any system register access (GICv2 or no vgic at all).

We don't expect the traps to come in the GICv2 case, though, right?

Yup. I'll fix the comment, as it is misleading.

Quoted 1 line

Looks alright to me otherwise, but blech!

Yeah. It's the sort of code that makes you feel dirty just looking at it.

Thanks,

	M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
author maintainer
#10 lore raw

PATCH 4/12 KVM: arm64: Force GICv3 traps activa when no irqchip is configured on VHE

In reply to Marc Zyngier

On a VHE system, no GICv3 traps get configured when no irqchip is present. This is not quite matching the "no GICv3" semantics that we want to present.

Force such traps to be configured in this case.

Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/vgic/vgic.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
arch/arm64/kvm/vgic/vgic.c +10 -4
index 974849ea7101..2caa64415ff3 100644
@@ -917,10 +917,13 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 
 void kvm_vgic_load(struct kvm_vcpu *vcpu)
 {
-	if (unlikely(!vgic_initialized(vcpu->kvm)))
+	if (unlikely(!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))) {
+		if (has_vhe() && static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif))
+			__vgic_v3_activate_traps(&vcpu->arch.vgic_cpu.vgic_v3);
 		return;
+	}
 
-	if (kvm_vgic_global_state.type == VGIC_V2)
+	if (!static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif))
 		vgic_v2_load(vcpu);
 	else
 		vgic_v3_load(vcpu);

@@ -928,10 +931,13 @@ void kvm_vgic_load(struct kvm_vcpu *vcpu)
 
 void kvm_vgic_put(struct kvm_vcpu *vcpu)
 {
-	if (unlikely(!vgic_initialized(vcpu->kvm)))
+	if (unlikely(!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))) {
+		if (has_vhe() && static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif))
+			__vgic_v3_deactivate_traps(&vcpu->arch.vgic_cpu.vgic_v3);
 		return;
+	}
 
-	if (kvm_vgic_global_state.type == VGIC_V2)
+	if (!static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif))
 		vgic_v2_put(vcpu);
 	else
 		vgic_v3_put(vcpu);
Signature · 1 line
2.39.2
#11 lore raw

Re: [PATCH 04/12] KVM: arm64: Force GICv3 traps activa when no irqchip is configured on VHE

s/activa/active/

Quoted 16 lines · Marc Zyngier

On a VHE system, no GICv3 traps get configured when no irqchip is present. This is not quite matching the "no GICv3" semantics that we want to present.

Force such traps to be configured in this case.

Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/vgic/vgic.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
arch/arm64/kvm/vgic/vgic.c +0 -0
index 974849ea7101..2caa64415ff3 100644

arch/arm64/kvm/vgic/vgic.c void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu) · line 920

Quoted 5 lines
arch/arm64/kvm/vgic/vgic.c +1 -1
@@ -917,10 +917,13 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 
 void kvm_vgic_load(struct kvm_vcpu *vcpu)
 {
-	if (unlikely(!vgic_initialized(vcpu->kvm)))
+	if (unlikely(!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))) {

Doesn't !vgic_initialized(vcpu->kvm) also cover the case of no irqchip in kernel?

Signature · 2 lines
Thanks,
Oliver
author maintainer
#12 lore raw

Re: [PATCH 04/12] KVM: arm64: Force GICv3 traps activa when no irqchip is configured on VHE

Quoted 21 lines · Oliver Upton

s/activa/active/

Quoted 17 lines · Marc Zyngier

On a VHE system, no GICv3 traps get configured when no irqchip is present. This is not quite matching the "no GICv3" semantics that we want to present.

Force such traps to be configured in this case.

Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/vgic/vgic.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
arch/arm64/kvm/vgic/vgic.c +0 -0
index 974849ea7101..2caa64415ff3 100644
@@ -917,10 +917,13 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
Quoted 7 lines

arch/arm64/kvm/vgic/vgic.c void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu) · line 920

Quoted 4 lines
arch/arm64/kvm/vgic/vgic.c +1 -1
 void kvm_vgic_load(struct kvm_vcpu *vcpu)
 {
-	if (unlikely(!vgic_initialized(vcpu->kvm)))
+	if (unlikely(!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))) {

Doesn't !vgic_initialized(vcpu->kvm) also cover the case of no irqchip in kernel?

It does, but that's purely accidental. I can drop that, but it is really fragile.

Thanks,

	M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
#13 lore raw

Re: [PATCH 04/12] KVM: arm64: Force GICv3 traps activa when no irqchip is configured on VHE

Quoted 31 lines · Marc Zyngier
Quoted 21 lines · Oliver Upton

s/activa/active/

Quoted 17 lines · Marc Zyngier

On a VHE system, no GICv3 traps get configured when no irqchip is present. This is not quite matching the "no GICv3" semantics that we want to present.

Force such traps to be configured in this case.

Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/vgic/vgic.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
arch/arm64/kvm/vgic/vgic.c +0 -0
index 974849ea7101..2caa64415ff3 100644
@@ -917,10 +917,13 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
Quoted 7 lines

arch/arm64/kvm/vgic/vgic.c void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu) · line 920

Quoted 4 lines
arch/arm64/kvm/vgic/vgic.c +1 -1
 void kvm_vgic_load(struct kvm_vcpu *vcpu)
 {
-	if (unlikely(!vgic_initialized(vcpu->kvm)))
+	if (unlikely(!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))) {

Doesn't !vgic_initialized(vcpu->kvm) also cover the case of no irqchip in kernel?

Quoted 2 lines

It does, but that's purely accidental. I can drop that, but it is really fragile.

Oh, definitely not, I was just wondering if this was meant as a functional change or a readability change to make the relation explicit.

Signature · 2 lines
Thanks,
Oliver
author maintainer
#14 lore raw

PATCH 5/12 KVM: arm64: Add helper for last ditch idreg adjustments

In reply to Marc Zyngier

We already have to perform a set of last-chance adjustments for NV purposes. We will soon have to do the same for the GIC, so introduce a helper for that exact purpose.

Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/arm.c      | 10 +++++-----
 arch/arm64/kvm/nested.c   | 15 +++++----------
 arch/arm64/kvm/sys_regs.c | 23 +++++++++++++++++++++++
 arch/arm64/kvm/sys_regs.h |  2 ++
 4 files changed, 35 insertions(+), 15 deletions(-)
arch/arm64/kvm/arm.c +5 -5
index 9bef7638342e..fc46457d38d6 100644
@@ -46,6 +46,8 @@
 #include <kvm/arm_pmu.h>
 #include <kvm/arm_psci.h>
 
+#include "sys_regs.h"
+
 static enum kvm_mode kvm_mode = KVM_MODE_DEFAULT;
 
 enum kvm_wfx_trap_policy {

@@ -821,11 +823,9 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
 			return ret;
 	}
 
-	if (vcpu_has_nv(vcpu)) {
-		ret = kvm_init_nv_sysregs(vcpu->kvm);
-		if (ret)
-			return ret;
-	}
+	ret = kvm_finalize_sys_regs(vcpu);
+	if (ret)
+		return ret;
 
 	/*
 	 * This needs to happen after NV has imposed its own restrictions on
arch/arm64/kvm/nested.c +5 -10
index bab27f9d8cc6..e2067c594e4a 100644
@@ -954,19 +954,16 @@ static void set_sysreg_masks(struct kvm *kvm, int sr, u64 res0, u64 res1)
 int kvm_init_nv_sysregs(struct kvm *kvm)
 {
 	u64 res0, res1;
-	int ret = 0;
 
-	mutex_lock(&kvm->arch.config_lock);
+	lockdep_assert_held(&kvm->arch.config_lock);
 
 	if (kvm->arch.sysreg_masks)
-		goto out;
+		return 0;
 
 	kvm->arch.sysreg_masks = kzalloc(sizeof(*(kvm->arch.sysreg_masks)),
 					 GFP_KERNEL_ACCOUNT);
-	if (!kvm->arch.sysreg_masks) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!kvm->arch.sysreg_masks)
+		return -ENOMEM;
 
 	limit_nv_id_regs(kvm);
 

@@ -1195,8 +1192,6 @@ int kvm_init_nv_sysregs(struct kvm *kvm)
 	if (!kvm_has_feat(kvm, ID_AA64PFR0_EL1, AMU, V1P1))
 		res0 |= ~(res0 | res1);
 	set_sysreg_masks(kvm, HAFGRTR_EL2, res0, res1);
-out:
-	mutex_unlock(&kvm->arch.config_lock);
 
-	return ret;
+	return 0;
 }
arch/arm64/kvm/sys_regs.c +23 -0
index 257c8da23a4e..bc2d54da3827 100644
@@ -4620,6 +4620,29 @@ void kvm_calculate_traps(struct kvm_vcpu *vcpu)
 	mutex_unlock(&kvm->arch.config_lock);
 }
 
+/*
+ * Perform last adjustments to the ID registers that are implied by the
+ * configuration outside of the ID regs themselves, as well as any
+ * initialisation that directly depend on these ID registers (such as
+ * RES0/RES1 behaviours). This is not the place to configure traps though.
+ *
+ * Because this can be called once per CPU, changes must be idempotent.
+ */
+int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)
+{
+	struct kvm *kvm = vcpu->kvm;
+
+	guard(mutex)(&kvm->arch.config_lock);
+
+	if (vcpu_has_nv(vcpu)) {
+		int ret = kvm_init_nv_sysregs(kvm);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 int __init kvm_sys_reg_table_init(void)
 {
 	bool valid = true;
arch/arm64/kvm/sys_regs.h +2 -0
index 997eea21ba2a..7c9b4eb0baa6 100644
@@ -235,6 +235,8 @@ int kvm_sys_reg_set_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
 
 bool triage_sysreg_trap(struct kvm_vcpu *vcpu, int *sr_index);
 
+int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu);
+
 #define AA32(_x)	.aarch32_map = AA32_##_x
 #define Op0(_x) 	.Op0 = _x
 #define Op1(_x) 	.Op1 = _x
Signature · 1 line
2.39.2
author maintainer
#15 lore raw

PATCH 6/12 KVM: arm64: Zero ID_AA64PFR0_EL1.GIC when no GICv3 is presented to the guest

In reply to Marc Zyngier

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.

Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/sys_regs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
arch/arm64/kvm/sys_regs.c +5 -1
index bc2d54da3827..7d00d7e359e1 100644
@@ -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,11 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)
 
 	guard(mutex)(&kvm->arch.config_lock);
 
+	if (!kvm_has_gicv3(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;
+	}
+
 	if (vcpu_has_nv(vcpu)) {
 		int ret = kvm_init_nv_sysregs(kvm);
 		if (ret)
Signature · 1 line
2.39.2
#16 lore raw

Re: [PATCH 06/12] KVM: arm64: Zero ID_AA64PFR0_EL1.GIC when no GICv3 is presented to the guest

Quoted 24 lines · Marc Zyngier

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.

Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/sys_regs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
arch/arm64/kvm/sys_regs.c +0 -1
index bc2d54da3827..7d00d7e359e1 100644
@@ -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),

arch/arm64/kvm/sys_regs.c int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu) · line 4640

Quoted 8 lines
arch/arm64/kvm/sys_regs.c +5 -0
@@ -4634,6 +4633,11 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)
 
 	guard(mutex)(&kvm->arch.config_lock);
 
+	if (!kvm_has_gicv3(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;
+	}
+

Hmm, should we use the ID register field as the source of truth for kvm_has_gicv3() at this point?

I think what you have in patch #1 makes good sense for a stable backport. Using the ID register from this point forward would make the behavior consistent for a stupid userspace instantiated GICv3 for the VM but clobbered it from the ID register.

AFAICT all other usage of kvm_has_gicv3() happens after kvm_finalize_sys_regs(), so it should take this last-minute fixup into account.

Signature · 2 lines
Thanks,
Oliver
author maintainer
#17 lore raw

Re: [PATCH 06/12] KVM: arm64: Zero ID_AA64PFR0_EL1.GIC when no GICv3 is presented to the guest

Quoted 38 lines · Oliver Upton
Quoted 24 lines · Marc Zyngier

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.

Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/sys_regs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
arch/arm64/kvm/sys_regs.c +0 -1
index bc2d54da3827..7d00d7e359e1 100644
@@ -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),

arch/arm64/kvm/sys_regs.c int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu) · line 4640

Quoted 8 lines
arch/arm64/kvm/sys_regs.c +5 -0
@@ -4634,6 +4633,11 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu)
 
 	guard(mutex)(&kvm->arch.config_lock);
 
+	if (!kvm_has_gicv3(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;
+	}
+

Hmm, should we use the ID register field as the source of truth for kvm_has_gicv3() at this point?

Quoted 8 lines

I think what you have in patch #1 makes good sense for a stable backport. Using the ID register from this point forward would make the behavior consistent for a stupid userspace instantiated GICv3 for the VM but clobbered it from the ID register.

AFAICT all other usage of kvm_has_gicv3() happens after kvm_finalize_sys_regs(), so it should take this last-minute fixup into account.

Right, so changing the definition of kvm_has_gicv3() in this patch, and using the "old" definition here only. Yup, this seems like a reasonable thing to do.

Thanks,

	M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
author maintainer
#18 lore raw

PATCH 7/12 KVM: arm64: Add ICH_HCR_EL2 to the vcpu state

In reply to Marc Zyngier

As we are about to describe the trap routing for ICH_HCR_EL2, add the register to the vcpu state in its VNCR form, as well as reset

Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/include/asm/kvm_host.h | 2 ++
 arch/arm64/kvm/sys_regs.c         | 2 ++
 2 files changed, 4 insertions(+)
arch/arm64/include/asm/kvm_host.h +2 -0
index a33f5996ca9f..16cd59362b3d 100644
@@ -530,6 +530,8 @@ enum vcpu_sysreg {
 	VNCR(CNTP_CVAL_EL0),
 	VNCR(CNTP_CTL_EL0),
 
+	VNCR(ICH_HCR_EL2),
+
 	NR_SYS_REGS	/* Nothing after this line! */
 };
arch/arm64/kvm/sys_regs.c +2 -0
index 7d00d7e359e1..c14fea3abc1b 100644
@@ -2797,6 +2797,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	EL2_REG(RVBAR_EL2, access_rw, reset_val, 0),
 	{ SYS_DESC(SYS_RMR_EL2), trap_undef },
 
+	EL2_REG_VNCR(ICH_HCR_EL2, reset_val, 0),
+
 	EL2_REG(CONTEXTIDR_EL2, access_rw, reset_val, 0),
 	EL2_REG(TPIDR_EL2, access_rw, reset_val, 0),
Signature · 1 line
2.39.2
author maintainer
#19 lore raw

PATCH 8/12 KVM: arm64: Add trap routing information for ICH_HCR_EL2

In reply to Marc Zyngier

The usual song and dance. Anything that is a trap, any register it traps. Note that we don't handle the registers added by FEAT_NMI for now.

Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/emulate-nested.c | 77 ++++++++++++++++++++++++++++++---
 1 file changed, 72 insertions(+), 5 deletions(-)
arch/arm64/kvm/emulate-nested.c +72 -5
index 05166eccea0a..63a2ce76619f 100644
@@ -85,12 +85,17 @@ enum cgt_group_id {
 
 	CGT_HCRX_TCR2En,
 
+	CGT_ICH_HCR_TC,
+	CGT_ICH_HCR_TALL0,
+	CGT_ICH_HCR_TALL1,
+	CGT_ICH_HCR_TDIR,
+
 	/*
 	 * Anything after this point is a combination of coarse trap
 	 * controls, which must all be evaluated to decide what to do.
 	 */
 	__MULTIPLE_CONTROL_BITS__,
-	CGT_HCR_IMO_FMO = __MULTIPLE_CONTROL_BITS__,
+	CGT_HCR_IMO_FMO_ICH_HCR_TC = __MULTIPLE_CONTROL_BITS__,
 	CGT_HCR_TID2_TID4,
 	CGT_HCR_TTLB_TTLBIS,
 	CGT_HCR_TTLB_TTLBOS,

@@ -105,6 +110,8 @@ enum cgt_group_id {
 	CGT_MDCR_TDE_TDRA,
 	CGT_MDCR_TDCC_TDE_TDA,
 
+	CGT_ICH_HCR_TC_TDIR,
+
 	/*
 	 * Anything after this point requires a callback evaluating a
 	 * complex trap condition. Ugly stuff.

@@ -378,6 +385,36 @@ static const struct trap_bits coarse_trap_bits[] = {
 		.mask		= HCRX_EL2_TCR2En,
 		.behaviour	= BEHAVE_FORWARD_ANY,
 	},
+	[CGT_ICH_HCR_TC] = {
+		.index		= ICH_HCR_EL2,
+		.value		= ICH_HCR_TC,
+		.mask		= ICH_HCR_TC,
+		.behaviour	= BEHAVE_FORWARD_ANY,
+	},
+	[CGT_ICH_HCR_TALL0] = {
+		.index		= ICH_HCR_EL2,
+		.value		= ICH_HCR_TALL0,
+		.mask		= ICH_HCR_TALL0,
+		.behaviour	= BEHAVE_FORWARD_ANY,
+	},
+	[CGT_ICH_HCR_TALL1] = {
+		.index		= ICH_HCR_EL2,
+		.value		= ICH_HCR_TALL1,
+		.mask		= ICH_HCR_TALL1,
+		.behaviour	= BEHAVE_FORWARD_ANY,
+	},
+	[CGT_ICH_HCR_TALL0] = {
+		.index		= ICH_HCR_EL2,
+		.value		= ICH_HCR_TALL0,
+		.mask		= ICH_HCR_TALL0,
+		.behaviour	= BEHAVE_FORWARD_ANY,
+	},
+	[CGT_ICH_HCR_TDIR] = {
+		.index		= ICH_HCR_EL2,
+		.value		= ICH_HCR_TDIR,
+		.mask		= ICH_HCR_TDIR,
+		.behaviour	= BEHAVE_FORWARD_ANY,
+	},
 };
 
 #define MCB(id, ...)						\

@@ -387,7 +424,6 @@ static const struct trap_bits coarse_trap_bits[] = {
 		}
 
 static const enum cgt_group_id *coarse_control_combo[] = {
-	MCB(CGT_HCR_IMO_FMO,		CGT_HCR_IMO, CGT_HCR_FMO),
 	MCB(CGT_HCR_TID2_TID4,		CGT_HCR_TID2, CGT_HCR_TID4),
 	MCB(CGT_HCR_TTLB_TTLBIS,	CGT_HCR_TTLB, CGT_HCR_TTLBIS),
 	MCB(CGT_HCR_TTLB_TTLBOS,	CGT_HCR_TTLB, CGT_HCR_TTLBOS),

@@ -402,6 +438,9 @@ static const enum cgt_group_id *coarse_control_combo[] = {
 	MCB(CGT_MDCR_TDE_TDOSA,		CGT_MDCR_TDE, CGT_MDCR_TDOSA),
 	MCB(CGT_MDCR_TDE_TDRA,		CGT_MDCR_TDE, CGT_MDCR_TDRA),
 	MCB(CGT_MDCR_TDCC_TDE_TDA,	CGT_MDCR_TDCC, CGT_MDCR_TDE, CGT_MDCR_TDA),
+
+	MCB(CGT_HCR_IMO_FMO_ICH_HCR_TC,	CGT_HCR_IMO, CGT_HCR_FMO, CGT_ICH_HCR_TC),
+	MCB(CGT_ICH_HCR_TC_TDIR,	CGT_ICH_HCR_TC, CGT_ICH_HCR_TDIR),
 };
 
 typedef enum trap_behaviour (*complex_condition_check)(struct kvm_vcpu *);

@@ -536,9 +575,9 @@ static const struct encoding_to_trap_config encoding_to_cgt[] __initconst = {
 	SR_TRAP(SYS_CSSELR_EL1,		CGT_HCR_TID2_TID4),
 	SR_RANGE_TRAP(SYS_ID_PFR0_EL1,
 		      sys_reg(3, 0, 0, 7, 7), CGT_HCR_TID3),
-	SR_TRAP(SYS_ICC_SGI0R_EL1,	CGT_HCR_IMO_FMO),
-	SR_TRAP(SYS_ICC_ASGI1R_EL1,	CGT_HCR_IMO_FMO),
-	SR_TRAP(SYS_ICC_SGI1R_EL1,	CGT_HCR_IMO_FMO),
+	SR_TRAP(SYS_ICC_SGI0R_EL1,	CGT_HCR_IMO_FMO_ICH_HCR_TC),
+	SR_TRAP(SYS_ICC_ASGI1R_EL1,	CGT_HCR_IMO_FMO_ICH_HCR_TC),
+	SR_TRAP(SYS_ICC_SGI1R_EL1,	CGT_HCR_IMO_FMO_ICH_HCR_TC),
 	SR_RANGE_TRAP(sys_reg(3, 0, 11, 0, 0),
 		      sys_reg(3, 0, 11, 15, 7), CGT_HCR_TIDCP),
 	SR_RANGE_TRAP(sys_reg(3, 1, 11, 0, 0),

@@ -1108,6 +1147,34 @@ static const struct encoding_to_trap_config encoding_to_cgt[] __initconst = {
 	SR_TRAP(SYS_CNTP_CTL_EL0,	CGT_CNTHCTL_EL1PTEN),
 	SR_TRAP(SYS_CNTPCT_EL0,		CGT_CNTHCTL_EL1PCTEN),
 	SR_TRAP(SYS_CNTPCTSS_EL0,	CGT_CNTHCTL_EL1PCTEN),
+	/*
+	 * IMPDEF choice:
+	 * We treat ICC_SRE_EL2.{SRE,Enable) and ICV_SRE_EL1.SRE as
+	 * RAO/WI. We therefore never consider ICC_SRE_EL2.Enable for
+	 * ICC_SRE_EL1 access, and always handle it locally.
+	 */
+	SR_TRAP(SYS_ICC_AP0R0_EL1,	CGT_ICH_HCR_TALL0),
+	SR_TRAP(SYS_ICC_AP0R1_EL1,	CGT_ICH_HCR_TALL0),
+	SR_TRAP(SYS_ICC_AP0R2_EL1,	CGT_ICH_HCR_TALL0),
+	SR_TRAP(SYS_ICC_AP0R3_EL1,	CGT_ICH_HCR_TALL0),
+	SR_TRAP(SYS_ICC_AP1R0_EL1,	CGT_ICH_HCR_TALL1),
+	SR_TRAP(SYS_ICC_AP1R1_EL1,	CGT_ICH_HCR_TALL1),
+	SR_TRAP(SYS_ICC_AP1R2_EL1,	CGT_ICH_HCR_TALL1),
+	SR_TRAP(SYS_ICC_AP1R3_EL1,	CGT_ICH_HCR_TALL1),
+	SR_TRAP(SYS_ICC_BPR0_EL1,	CGT_ICH_HCR_TALL0),
+	SR_TRAP(SYS_ICC_BPR1_EL1,	CGT_ICH_HCR_TALL1),
+	SR_TRAP(SYS_ICC_CTLR_EL1,	CGT_ICH_HCR_TC),
+	SR_TRAP(SYS_ICC_DIR_EL1,	CGT_ICH_HCR_TC_TDIR),
+	SR_TRAP(SYS_ICC_EOIR0_EL1,	CGT_ICH_HCR_TALL0),
+	SR_TRAP(SYS_ICC_EOIR1_EL1,	CGT_ICH_HCR_TALL1),
+	SR_TRAP(SYS_ICC_HPPIR0_EL1,	CGT_ICH_HCR_TALL0),
+	SR_TRAP(SYS_ICC_HPPIR1_EL1,	CGT_ICH_HCR_TALL1),
+	SR_TRAP(SYS_ICC_IAR0_EL1,	CGT_ICH_HCR_TALL0),
+	SR_TRAP(SYS_ICC_IAR1_EL1,	CGT_ICH_HCR_TALL1),
+	SR_TRAP(SYS_ICC_IGRPEN0_EL1,	CGT_ICH_HCR_TALL0),
+	SR_TRAP(SYS_ICC_IGRPEN1_EL1,	CGT_ICH_HCR_TALL1),
+	SR_TRAP(SYS_ICC_PMR_EL1,	CGT_ICH_HCR_TC),
+	SR_TRAP(SYS_ICC_RPR_EL1,	CGT_ICH_HCR_TC),
 };
 
 static DEFINE_XARRAY(sr_forward_xa);
Signature · 1 line
2.39.2
build bot
#20 lore raw

Re: [PATCH 08/12] KVM: arm64: Add trap routing information for ICH_HCR_EL2

kernel test robot noticed the following build warnings:

Hi Marc,

kernel test robot noticed the following build warnings:

[auto build test WARNING on kvmarm/next]
[also build test WARNING on kvm/queue arm64/for-next/core soc/for-next linus/master v6.11-rc4 next-20240821] [cannot apply to kvm/linux-next arm/for-next arm/fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Marc-Zyngier/KVM-arm64-Make-ICC_-SGI-_EL1-undef-in-the-absence-of-a-vGICv3/20240820-180937
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
patch link:    https://lore.kernel.org/r/20240820100349.3544850-9-maz%40kernel.org
patch subject: [PATCH 08/12] KVM: arm64: Add trap routing information for ICH_HCR_EL2
config: arm64-randconfig-r113-20240821 (https://download.01.org/0day-ci/archive/20240822/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 14.1.0
reproduce: (https://download.01.org/0day-ci/archive/20240822/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

sparse warnings: (new ones prefixed by >>)

>> arch/arm64/kvm/emulate-nested.c:394:10: sparse: sparse: Initializer entry defined twice

arch/arm64/kvm/emulate-nested.c:406:10: sparse: also defined here

vim +394 arch/arm64/kvm/emulate-nested.c

128
129 static const struct trap_bits coarse_trap_bits[] = {
130 [CGT_HCR_TID1] = {
131 .index = HCR_EL2,
132 .value = HCR_TID1,
133 .mask = HCR_TID1,
134 .behaviour = BEHAVE_FORWARD_READ,
135 },
136 [CGT_HCR_TID2] = {
137 .index = HCR_EL2,
138 .value = HCR_TID2,
139 .mask = HCR_TID2,
140 .behaviour = BEHAVE_FORWARD_ANY,
141 },
142 [CGT_HCR_TID3] = {
143 .index = HCR_EL2,
144 .value = HCR_TID3,
145 .mask = HCR_TID3,
146 .behaviour = BEHAVE_FORWARD_READ,
147 },
148 [CGT_HCR_IMO] = {
149 .index = HCR_EL2,
150 .value = HCR_IMO,
151 .mask = HCR_IMO,
152 .behaviour = BEHAVE_FORWARD_WRITE,
153 },
154 [CGT_HCR_FMO] = {
155 .index = HCR_EL2,
156 .value = HCR_FMO,
157 .mask = HCR_FMO,
158 .behaviour = BEHAVE_FORWARD_WRITE,
159 },
160 [CGT_HCR_TIDCP] = {
161 .index = HCR_EL2,
162 .value = HCR_TIDCP,
163 .mask = HCR_TIDCP,
164 .behaviour = BEHAVE_FORWARD_ANY,
165 },
166 [CGT_HCR_TACR] = {
167 .index = HCR_EL2,
168 .value = HCR_TACR,
169 .mask = HCR_TACR,
170 .behaviour = BEHAVE_FORWARD_ANY,
171 },
172 [CGT_HCR_TSW] = {
173 .index = HCR_EL2,
174 .value = HCR_TSW,
175 .mask = HCR_TSW,
176 .behaviour = BEHAVE_FORWARD_ANY,
177 },
178 [CGT_HCR_TPC] = { /* Also called TCPC when FEAT_DPB is implemented */
179 .index = HCR_EL2,
180 .value = HCR_TPC,
181 .mask = HCR_TPC,
182 .behaviour = BEHAVE_FORWARD_ANY,
183 },
184 [CGT_HCR_TPU] = {
185 .index = HCR_EL2,
186 .value = HCR_TPU,
187 .mask = HCR_TPU,
188 .behaviour = BEHAVE_FORWARD_ANY,
189 },
190 [CGT_HCR_TTLB] = {
191 .index = HCR_EL2,
192 .value = HCR_TTLB,
193 .mask = HCR_TTLB,
194 .behaviour = BEHAVE_FORWARD_ANY,
195 },
196 [CGT_HCR_TVM] = {
197 .index = HCR_EL2,
198 .value = HCR_TVM,
199 .mask = HCR_TVM,
200 .behaviour = BEHAVE_FORWARD_WRITE,
201 },
202 [CGT_HCR_TDZ] = {
203 .index = HCR_EL2,
204 .value = HCR_TDZ,
205 .mask = HCR_TDZ,
206 .behaviour = BEHAVE_FORWARD_ANY,
207 },
208 [CGT_HCR_TRVM] = {
209 .index = HCR_EL2,
210 .value = HCR_TRVM,
211 .mask = HCR_TRVM,
212 .behaviour = BEHAVE_FORWARD_READ,
213 },
214 [CGT_HCR_TLOR] = {
215 .index = HCR_EL2,
216 .value = HCR_TLOR,
217 .mask = HCR_TLOR,
218 .behaviour = BEHAVE_FORWARD_ANY,
219 },
220 [CGT_HCR_TERR] = {
221 .index = HCR_EL2,
222 .value = HCR_TERR,
223 .mask = HCR_TERR,
224 .behaviour = BEHAVE_FORWARD_ANY,
225 },
226 [CGT_HCR_APK] = {
227 .index = HCR_EL2,
228 .value = 0,
229 .mask = HCR_APK,
230 .behaviour = BEHAVE_FORWARD_ANY,
231 },
232 [CGT_HCR_NV] = {
233 .index = HCR_EL2,
234 .value = HCR_NV,
235 .mask = HCR_NV,
236 .behaviour = BEHAVE_FORWARD_ANY,
237 },
238 [CGT_HCR_NV_nNV2] = {
239 .index = HCR_EL2,
240 .value = HCR_NV,
241 .mask = HCR_NV | HCR_NV2,
242 .behaviour = BEHAVE_FORWARD_ANY,
243 },
244 [CGT_HCR_NV1_nNV2] = {
245 .index = HCR_EL2,
246 .value = HCR_NV | HCR_NV1,
247 .mask = HCR_NV | HCR_NV1 | HCR_NV2,
248 .behaviour = BEHAVE_FORWARD_ANY,
249 },
250 [CGT_HCR_AT] = {
251 .index = HCR_EL2,
252 .value = HCR_AT,
253 .mask = HCR_AT,
254 .behaviour = BEHAVE_FORWARD_ANY,
255 },
256 [CGT_HCR_nFIEN] = {
257 .index = HCR_EL2,
258 .value = 0,
259 .mask = HCR_FIEN,
260 .behaviour = BEHAVE_FORWARD_ANY,
261 },
262 [CGT_HCR_TID4] = {
263 .index = HCR_EL2,
264 .value = HCR_TID4,
265 .mask = HCR_TID4,
266 .behaviour = BEHAVE_FORWARD_ANY,
267 },
268 [CGT_HCR_TICAB] = {
269 .index = HCR_EL2,
270 .value = HCR_TICAB,
271 .mask = HCR_TICAB,
272 .behaviour = BEHAVE_FORWARD_ANY,
273 },
274 [CGT_HCR_TOCU] = {
275 .index = HCR_EL2,
276 .value = HCR_TOCU,
277 .mask = HCR_TOCU,
278 .behaviour = BEHAVE_FORWARD_ANY,
279 },
280 [CGT_HCR_ENSCXT] = {
281 .index = HCR_EL2,
282 .value = 0,
283 .mask = HCR_ENSCXT,
284 .behaviour = BEHAVE_FORWARD_ANY,
285 },
286 [CGT_HCR_TTLBIS] = {
287 .index = HCR_EL2,
288 .value = HCR_TTLBIS,
289 .mask = HCR_TTLBIS,
290 .behaviour = BEHAVE_FORWARD_ANY,
291 },
292 [CGT_HCR_TTLBOS] = {
293 .index = HCR_EL2,
294 .value = HCR_TTLBOS,
295 .mask = HCR_TTLBOS,
296 .behaviour = BEHAVE_FORWARD_ANY,
297 },
298 [CGT_MDCR_TPMCR] = {
299 .index = MDCR_EL2,
300 .value = MDCR_EL2_TPMCR,
301 .mask = MDCR_EL2_TPMCR,
302 .behaviour = BEHAVE_FORWARD_ANY,
303 },
304 [CGT_MDCR_TPM] = {
305 .index = MDCR_EL2,
306 .value = MDCR_EL2_TPM,
307 .mask = MDCR_EL2_TPM,
308 .behaviour = BEHAVE_FORWARD_ANY,
309 },
310 [CGT_MDCR_TDE] = {
311 .index = MDCR_EL2,
312 .value = MDCR_EL2_TDE,
313 .mask = MDCR_EL2_TDE,
314 .behaviour = BEHAVE_FORWARD_ANY,
315 },
316 [CGT_MDCR_TDA] = {
317 .index = MDCR_EL2,
318 .value = MDCR_EL2_TDA,
319 .mask = MDCR_EL2_TDA,
320 .behaviour = BEHAVE_FORWARD_ANY,
321 },
322 [CGT_MDCR_TDOSA] = {
323 .index = MDCR_EL2,
324 .value = MDCR_EL2_TDOSA,
325 .mask = MDCR_EL2_TDOSA,
326 .behaviour = BEHAVE_FORWARD_ANY,
327 },
328 [CGT_MDCR_TDRA] = {
329 .index = MDCR_EL2,
330 .value = MDCR_EL2_TDRA,
331 .mask = MDCR_EL2_TDRA,
332 .behaviour = BEHAVE_FORWARD_ANY,
333 },
334 [CGT_MDCR_E2PB] = {
335 .index = MDCR_EL2,
336 .value = 0,
337 .mask = BIT(MDCR_EL2_E2PB_SHIFT),
338 .behaviour = BEHAVE_FORWARD_ANY,
339 },
340 [CGT_MDCR_TPMS] = {
341 .index = MDCR_EL2,
342 .value = MDCR_EL2_TPMS,
343 .mask = MDCR_EL2_TPMS,
344 .behaviour = BEHAVE_FORWARD_ANY,
345 },
346 [CGT_MDCR_TTRF] = {
347 .index = MDCR_EL2,
348 .value = MDCR_EL2_TTRF,
349 .mask = MDCR_EL2_TTRF,
350 .behaviour = BEHAVE_FORWARD_ANY,
351 },
352 [CGT_MDCR_E2TB] = {
353 .index = MDCR_EL2,
354 .value = 0,
355 .mask = BIT(MDCR_EL2_E2TB_SHIFT),
356 .behaviour = BEHAVE_FORWARD_ANY,
357 },
358 [CGT_MDCR_TDCC] = {
359 .index = MDCR_EL2,
360 .value = MDCR_EL2_TDCC,
361 .mask = MDCR_EL2_TDCC,
362 .behaviour = BEHAVE_FORWARD_ANY,
363 },
364 [CGT_CPACR_E0POE] = {
365 .index = CPTR_EL2,
366 .value = CPACR_ELx_E0POE,
367 .mask = CPACR_ELx_E0POE,
368 .behaviour = BEHAVE_FORWARD_ANY,
369 },
370 [CGT_CPTR_TAM] = {
371 .index = CPTR_EL2,
372 .value = CPTR_EL2_TAM,
373 .mask = CPTR_EL2_TAM,
374 .behaviour = BEHAVE_FORWARD_ANY,
375 },
376 [CGT_CPTR_TCPAC] = {
377 .index = CPTR_EL2,
378 .value = CPTR_EL2_TCPAC,
379 .mask = CPTR_EL2_TCPAC,
380 .behaviour = BEHAVE_FORWARD_ANY,
381 },
382 [CGT_HCRX_TCR2En] = {
383 .index = HCRX_EL2,
384 .value = 0,
385 .mask = HCRX_EL2_TCR2En,
386 .behaviour = BEHAVE_FORWARD_ANY,
387 },
388 [CGT_ICH_HCR_TC] = {
389 .index = ICH_HCR_EL2,
390 .value = ICH_HCR_TC,
391 .mask = ICH_HCR_TC,
392 .behaviour = BEHAVE_FORWARD_ANY,
393 },

 > 394		[CGT_ICH_HCR_TALL0] = {

395 .index = ICH_HCR_EL2,
396 .value = ICH_HCR_TALL0,
397 .mask = ICH_HCR_TALL0,
398 .behaviour = BEHAVE_FORWARD_ANY,
399 },
400 [CGT_ICH_HCR_TALL1] = {
401 .index = ICH_HCR_EL2,
402 .value = ICH_HCR_TALL1,
403 .mask = ICH_HCR_TALL1,
404 .behaviour = BEHAVE_FORWARD_ANY,
405 },
406 [CGT_ICH_HCR_TALL0] = {
407 .index = ICH_HCR_EL2,
408 .value = ICH_HCR_TALL0,
409 .mask = ICH_HCR_TALL0,
410 .behaviour = BEHAVE_FORWARD_ANY,
411 },
412 [CGT_ICH_HCR_TDIR] = {
413 .index = ICH_HCR_EL2,
414 .value = ICH_HCR_TDIR,
415 .mask = ICH_HCR_TDIR,
416 .behaviour = BEHAVE_FORWARD_ANY,
417 },
418 };
419

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
author maintainer
#21 lore raw

PATCH 9/12 KVM: arm64: Honor guest requested traps in GICv3 emulation

In reply to Marc Zyngier

On platforms that require emulation of the CPU interface, we still need to honor the traps requested by the guest (ICH_HCR_EL2 as well as the FGTs for ICC_IGRPEN{0,1}_EL1.

Check for these bits early and lail out if any trap applies.

Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 72 +++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)
arch/arm64/kvm/hyp/vgic-v3-sr.c +72 -0
index a184def8f5ad..a01138a663ae 100644
@@ -1042,6 +1042,75 @@ static void __vgic_v3_write_ctlr(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 	write_gicreg(vmcr, ICH_VMCR_EL2);
 }
 
+static bool __vgic_v3_check_trap_forwarding(struct kvm_vcpu *vcpu,
+					    u32 sysreg, bool is_read)
+{
+	u64 ich_hcr;
+
+	if (!vcpu_has_nv(vcpu) || is_hyp_ctxt(vcpu))
+		return false;
+
+	ich_hcr = __vcpu_sys_reg(vcpu, ICH_HCR_EL2);
+
+	switch (sysreg) {
+	case SYS_ICC_IGRPEN0_EL1:
+		if (is_read &&
+		    (__vcpu_sys_reg(vcpu, HFGRTR_EL2) & HFGxTR_EL2_ICC_IGRPENn_EL1))
+			return true;
+
+		if (!is_read &&
+		    (__vcpu_sys_reg(vcpu, HFGWTR_EL2) & HFGxTR_EL2_ICC_IGRPENn_EL1))
+			return true;
+
+		fallthrough;
+
+	case SYS_ICC_AP0Rn_EL1(0):
+	case SYS_ICC_AP0Rn_EL1(1):
+	case SYS_ICC_AP0Rn_EL1(2):
+	case SYS_ICC_AP0Rn_EL1(3):
+	case SYS_ICC_BPR0_EL1:
+	case SYS_ICC_EOIR0_EL1:
+	case SYS_ICC_HPPIR0_EL1:
+	case SYS_ICC_IAR0_EL1:
+		return ich_hcr & ICH_HCR_TALL0;
+
+	case SYS_ICC_IGRPEN1_EL1:
+		if (is_read &&
+		    (__vcpu_sys_reg(vcpu, HFGRTR_EL2) & HFGxTR_EL2_ICC_IGRPENn_EL1))
+			return true;
+
+		if (!is_read &&
+		    (__vcpu_sys_reg(vcpu, HFGWTR_EL2) & HFGxTR_EL2_ICC_IGRPENn_EL1))
+			return true;
+
+		fallthrough;
+
+	case SYS_ICC_AP1Rn_EL1(0):
+	case SYS_ICC_AP1Rn_EL1(1):
+	case SYS_ICC_AP1Rn_EL1(2):
+	case SYS_ICC_AP1Rn_EL1(3):
+	case SYS_ICC_BPR1_EL1:
+	case SYS_ICC_EOIR1_EL1:
+	case SYS_ICC_HPPIR1_EL1:
+	case SYS_ICC_IAR1_EL1:
+		return ich_hcr & ICH_HCR_TALL1;
+
+	case SYS_ICC_DIR_EL1:
+		if (ich_hcr & ICH_HCR_TDIR)
+			return true;
+
+		fallthrough;
+
+	case SYS_ICC_RPR_EL1:
+	case SYS_ICC_CTLR_EL1:
+	case SYS_ICC_PMR_EL1:
+		return ich_hcr & ICH_HCR_TC;
+
+	default:
+		return false;
+	}
+}
+
 int __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu)
 {
 	int rt;

@@ -1065,6 +1134,9 @@ int __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu)
 
 	is_read = (esr & ESR_ELx_SYS64_ISS_DIR_MASK) == ESR_ELx_SYS64_ISS_DIR_READ;
 
+	if (__vgic_v3_check_trap_forwarding(vcpu, sysreg, is_read))
+		return 0;
+
 	switch (sysreg) {
 	case SYS_ICC_IAR0_EL1:
 	case SYS_ICC_IAR1_EL1:
Signature · 1 line
2.39.2
author maintainer
#22 lore raw

PATCH 10/12 KVM: arm64: Make most GICv3 accesses UNDEF if they trap

In reply to Marc Zyngier

We don't expect to trap any GICv3 register for host handling, apart from ICC_SRE_EL1 and the SGI registers. If they trap, that's because the guest is playing with us despite being told it doesn't have a GICv3.

If it does, UNDEF is what it will get.

Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/hyp/vgic-v3-sr.c |  3 ++
 arch/arm64/kvm/sys_regs.c       | 74 +++++++++++++++++++++++++--------
 arch/arm64/kvm/sys_regs.h       |  7 ++++
 3 files changed, 66 insertions(+), 18 deletions(-)
arch/arm64/kvm/hyp/vgic-v3-sr.c +3 -0
index a01138a663ae..a0291f4b0eb9 100644
@@ -1120,6 +1120,9 @@ int __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu)
 	bool is_read;
 	u32 sysreg;
 
+	if (kern_hyp_va(vcpu->kvm)->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return 0;
+
 	esr = kvm_vcpu_get_esr(vcpu);
 	if (vcpu_mode_is_32bit(vcpu)) {
 		if (!kvm_condition_valid(vcpu)) {
arch/arm64/kvm/sys_regs.c +56 -18

120 lines not on this page · raw message

arch/arm64/kvm/sys_regs.h +7 -0
index 7c9b4eb0baa6..dfb2ec83b284 100644
@@ -250,4 +250,11 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu);
 	CRn(sys_reg_CRn(reg)), CRm(sys_reg_CRm(reg)),	\
 	Op2(sys_reg_Op2(reg))
 
+#define CP15_SYS_DESC(reg)				\
+	.name = #reg,					\
+	.aarch32_map = AA32_DIRECT,			\
+	Op0(0), Op1(sys_reg_Op1(reg)),			\
+	CRn(sys_reg_CRn(reg)), CRm(sys_reg_CRm(reg)),	\
+	Op2(sys_reg_Op2(reg))
+
 #endif /* __ARM64_KVM_SYS_REGS_LOCAL_H__ */
Signature · 1 line
2.39.2
author maintainer
#23 lore raw

PATCH 11/12 KVM: arm64: Unify UNDEF injection helpers

In reply to Marc Zyngier

We currently have two helpers (undef_access() and trap_undef()) that do exactly the same thing: inject an UNDEF and return 'false' (as an indication that PC should not be incremented).

We definitely could do with one less. Given that undef_access() is used 80ish times, while trap_undef() is only used 30 times, the latter loses the battle and is immediately sacrificed.

We also have a large number of instances where undef_access() is open-coded. Let's also convert those.

Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/sys_regs.c | 132 +++++++++++++++-----------------------
 1 file changed, 51 insertions(+), 81 deletions(-)
arch/arm64/kvm/sys_regs.c +51 -81
index ba6075973a36..0976df0008cc 100644
@@ -61,8 +61,7 @@ static bool bad_trap(struct kvm_vcpu *vcpu,
 {
 	WARN_ONCE(1, "Unexpected %s\n", msg);
 	print_sys_reg_instr(params);
-	kvm_inject_undefined(vcpu);
-	return false;
+	return undef_access(vcpu, params, r);
 }
 
 static bool read_from_write_only(struct kvm_vcpu *vcpu,

@@ -353,10 +352,8 @@ static bool access_dcgsw(struct kvm_vcpu *vcpu,
 			 struct sys_reg_params *p,
 			 const struct sys_reg_desc *r)
 {
-	if (!kvm_has_mte(vcpu->kvm)) {
-		kvm_inject_undefined(vcpu);
-		return false;
-	}
+	if (!kvm_has_mte(vcpu->kvm))
+		return undef_access(vcpu, p, r);
 
 	/* Treat MTE S/W ops as we treat the classic ones: with contempt */
 	return access_dcsw(vcpu, p, r);

@@ -393,10 +390,8 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
 	u64 val, mask, shift;
 
 	if (reg_to_encoding(r) == SYS_TCR2_EL1 &&
-	    !kvm_has_feat(vcpu->kvm, ID_AA64MMFR3_EL1, TCRX, IMP)) {
-		kvm_inject_undefined(vcpu);
-		return false;
-	}
+	    !kvm_has_feat(vcpu->kvm, ID_AA64MMFR3_EL1, TCRX, IMP))
+		return undef_access(vcpu, p, r);
 
 	BUG_ON(!p->is_write);
 

@@ -443,10 +438,8 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 {
 	bool g1;
 
-	if (!kvm_has_gicv3(vcpu->kvm)) {
-		kvm_inject_undefined(vcpu);
-		return false;
-	}
+	if (!kvm_has_gicv3(vcpu->kvm))
+		return undef_access(vcpu, p, r);
 
 	if (!p->is_write)
 		return read_from_write_only(vcpu, p, r);

@@ -511,14 +504,6 @@ static bool trap_raz_wi(struct kvm_vcpu *vcpu,
 		return read_zero(vcpu, p);
 }
 
-static bool trap_undef(struct kvm_vcpu *vcpu,
-		       struct sys_reg_params *p,
-		       const struct sys_reg_desc *r)
-{
-	kvm_inject_undefined(vcpu);
-	return false;
-}
-
 /*
  * ARMv8.1 mandates at least a trivial LORegion implementation, where all the
  * RW registers are RES0 (which we can implement as RAZ/WI). On an ARMv8.0

@@ -531,10 +516,8 @@ static bool trap_loregion(struct kvm_vcpu *vcpu,
 {
 	u32 sr = reg_to_encoding(r);
 
-	if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR1_EL1, LO, IMP)) {
-		kvm_inject_undefined(vcpu);
-		return false;
-	}
+	if (!kvm_has_feat(vcpu->kvm, ID_AA64MMFR1_EL1, LO, IMP))
+		return undef_access(vcpu, p, r);
 
 	if (p->is_write && sr == SYS_LORID_EL1)
 		return write_to_read_only(vcpu, p, r);

@@ -1267,10 +1250,8 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 			     const struct sys_reg_desc *r)
 {
 	if (p->is_write) {
-		if (!vcpu_mode_priv(vcpu)) {
-			kvm_inject_undefined(vcpu);
-			return false;
-		}
+		if (!vcpu_mode_priv(vcpu))
+			return undef_access(vcpu, p, r);
 
 		__vcpu_sys_reg(vcpu, PMUSERENR_EL0) =
 			       p->regval & ARMV8_PMU_USERENR_MASK;

@@ -1412,8 +1393,7 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
 		break;
 	default:
 		print_sys_reg_msg(p, "%s", "Unhandled trapped timer register");
-		kvm_inject_undefined(vcpu);
-		return false;
+		return undef_access(vcpu, p, r);
 	}
 
 	if (p->is_write)

@@ -2309,7 +2289,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	// DBGDTR[TR]X_EL0 share the same encoding
 	{ SYS_DESC(SYS_DBGDTRTX_EL0), trap_raz_wi },
 
-	{ SYS_DESC(SYS_DBGVCR32_EL2), trap_undef, reset_val, DBGVCR32_EL2, 0 },
+	{ SYS_DESC(SYS_DBGVCR32_EL2), undef_access, reset_val, DBGVCR32_EL2, 0 },
 
 	{ SYS_DESC(SYS_MPIDR_EL1), NULL, reset_mpidr, MPIDR_EL1 },
 

@@ -2780,7 +2760,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	EL2_REG_VNCR(VTTBR_EL2, reset_val, 0),
 	EL2_REG_VNCR(VTCR_EL2, reset_val, 0),
 
-	{ SYS_DESC(SYS_DACR32_EL2), trap_undef, reset_unknown, DACR32_EL2 },
+	{ SYS_DESC(SYS_DACR32_EL2), undef_access, reset_unknown, DACR32_EL2 },
 	EL2_REG_VNCR(HDFGRTR_EL2, reset_val, 0),
 	EL2_REG_VNCR(HDFGWTR_EL2, reset_val, 0),
 	EL2_REG_VNCR(HAFGRTR_EL2, reset_val, 0),

@@ -2798,11 +2778,11 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_SPSR_fiq), .access = trap_raz_wi,
 	  .visibility = hidden_user_visibility },
 
-	{ SYS_DESC(SYS_IFSR32_EL2), trap_undef, reset_unknown, IFSR32_EL2 },
+	{ SYS_DESC(SYS_IFSR32_EL2), undef_access, reset_unknown, IFSR32_EL2 },
 	EL2_REG(AFSR0_EL2, access_rw, reset_val, 0),
 	EL2_REG(AFSR1_EL2, access_rw, reset_val, 0),
 	EL2_REG_REDIR(ESR_EL2, reset_val, 0),
-	{ SYS_DESC(SYS_FPEXC32_EL2), trap_undef, reset_val, FPEXC32_EL2, 0x700 },
+	{ SYS_DESC(SYS_FPEXC32_EL2), undef_access, reset_val, FPEXC32_EL2, 0x700 },
 
 	EL2_REG_REDIR(FAR_EL2, reset_val, 0),
 	EL2_REG(HPFAR_EL2, access_rw, reset_val, 0),

@@ -2812,7 +2792,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	EL2_REG(VBAR_EL2, access_rw, reset_val, 0),
 	EL2_REG(RVBAR_EL2, access_rw, reset_val, 0),
-	{ SYS_DESC(SYS_RMR_EL2), trap_undef },
+	{ SYS_DESC(SYS_RMR_EL2), undef_access },
 
 	EL2_REG_VNCR(ICH_HCR_EL2, reset_val, 0),
 

@@ -2848,10 +2828,8 @@ static bool handle_alle1is(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 {
 	u32 sys_encoding = sys_insn(p->Op0, p->Op1, p->CRn, p->CRm, p->Op2);
 
-	if (!kvm_supported_tlbi_s12_op(vcpu, sys_encoding)) {
-		kvm_inject_undefined(vcpu);
-		return false;
-	}
+	if (!kvm_supported_tlbi_s12_op(vcpu, sys_encoding))
+		return undef_access(vcpu, p, r);
 
 	write_lock(&vcpu->kvm->mmu_lock);
 

@@ -2920,10 +2898,8 @@ static bool handle_vmalls12e1is(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	u32 sys_encoding = sys_insn(p->Op0, p->Op1, p->CRn, p->CRm, p->Op2);
 	u64 limit, vttbr;
 
-	if (!kvm_supported_tlbi_s12_op(vcpu, sys_encoding)) {
-		kvm_inject_undefined(vcpu);
-		return false;
-	}
+	if (!kvm_supported_tlbi_s12_op(vcpu, sys_encoding))
+		return undef_access(vcpu, p, r);
 
 	vttbr = vcpu_read_sys_reg(vcpu, VTTBR_EL2);
 	limit = BIT_ULL(kvm_get_pa_bits(vcpu->kvm));

@@ -2948,10 +2924,8 @@ static bool handle_ripas2e1is(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	u64 base, range, tg, num, scale;
 	int shift;
 
-	if (!kvm_supported_tlbi_ipas2_op(vcpu, sys_encoding)) {
-		kvm_inject_undefined(vcpu);
-		return false;
-	}
+	if (!kvm_supported_tlbi_ipas2_op(vcpu, sys_encoding))
+		return undef_access(vcpu, p, r);
 
 	/*
 	 * Because the shadow S2 structure doesn't necessarily reflect that

@@ -3019,10 +2993,8 @@ static bool handle_ipas2e1is(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	u32 sys_encoding = sys_insn(p->Op0, p->Op1, p->CRn, p->CRm, p->Op2);
 	u64 vttbr = vcpu_read_sys_reg(vcpu, VTTBR_EL2);
 
-	if (!kvm_supported_tlbi_ipas2_op(vcpu, sys_encoding)) {
-		kvm_inject_undefined(vcpu);
-		return false;
-	}
+	if (!kvm_supported_tlbi_ipas2_op(vcpu, sys_encoding))
+		return undef_access(vcpu, p, r);
 
 	kvm_s2_mmu_iterate_by_vmid(vcpu->kvm, get_vmid(vttbr),
 				   &(union tlbi_info) {

@@ -3062,10 +3034,8 @@ static bool handle_tlbi_el1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 
 	WARN_ON(!vcpu_is_el2(vcpu));
 
-	if (!kvm_supported_tlbi_s1e1_op(vcpu, sys_encoding)) {
-		kvm_inject_undefined(vcpu);
-		return false;
-	}
+	if (!kvm_supported_tlbi_s1e1_op(vcpu, sys_encoding))
+		return undef_access(vcpu, p, r);
 
 	kvm_s2_mmu_iterate_by_vmid(vcpu->kvm, get_vmid(vttbr),
 				   &(union tlbi_info) {

@@ -3173,14 +3143,14 @@ static struct sys_reg_desc sys_insn_descs[] = {
 	SYS_INSN(TLBI_IPAS2LE1IS, handle_ipas2e1is),
 	SYS_INSN(TLBI_RIPAS2LE1IS, handle_ripas2e1is),
 
-	SYS_INSN(TLBI_ALLE2OS, trap_undef),
-	SYS_INSN(TLBI_VAE2OS, trap_undef),
+	SYS_INSN(TLBI_ALLE2OS, undef_access),
+	SYS_INSN(TLBI_VAE2OS, undef_access),
 	SYS_INSN(TLBI_ALLE1OS, handle_alle1is),
-	SYS_INSN(TLBI_VALE2OS, trap_undef),
+	SYS_INSN(TLBI_VALE2OS, undef_access),
 	SYS_INSN(TLBI_VMALLS12E1OS, handle_vmalls12e1is),
 
-	SYS_INSN(TLBI_RVAE2IS, trap_undef),
-	SYS_INSN(TLBI_RVALE2IS, trap_undef),
+	SYS_INSN(TLBI_RVAE2IS, undef_access),
+	SYS_INSN(TLBI_RVALE2IS, undef_access),
 
 	SYS_INSN(TLBI_ALLE1IS, handle_alle1is),
 	SYS_INSN(TLBI_VMALLS12E1IS, handle_vmalls12e1is),

@@ -3192,10 +3162,10 @@ static struct sys_reg_desc sys_insn_descs[] = {
 	SYS_INSN(TLBI_IPAS2LE1, handle_ipas2e1is),
 	SYS_INSN(TLBI_RIPAS2LE1, handle_ripas2e1is),
 	SYS_INSN(TLBI_RIPAS2LE1OS, handle_ripas2e1is),
-	SYS_INSN(TLBI_RVAE2OS, trap_undef),
-	SYS_INSN(TLBI_RVALE2OS, trap_undef),
-	SYS_INSN(TLBI_RVAE2, trap_undef),
-	SYS_INSN(TLBI_RVALE2, trap_undef),
+	SYS_INSN(TLBI_RVAE2OS, undef_access),
+	SYS_INSN(TLBI_RVALE2OS, undef_access),
+	SYS_INSN(TLBI_RVAE2, undef_access),
+	SYS_INSN(TLBI_RVALE2, undef_access),
 	SYS_INSN(TLBI_ALLE1, handle_alle1is),
 	SYS_INSN(TLBI_VMALLS12E1, handle_vmalls12e1is),
 

@@ -3204,19 +3174,19 @@ static struct sys_reg_desc sys_insn_descs[] = {
 	SYS_INSN(TLBI_IPAS2LE1ISNXS, handle_ipas2e1is),
 	SYS_INSN(TLBI_RIPAS2LE1ISNXS, handle_ripas2e1is),
 
-	SYS_INSN(TLBI_ALLE2OSNXS, trap_undef),
-	SYS_INSN(TLBI_VAE2OSNXS, trap_undef),
+	SYS_INSN(TLBI_ALLE2OSNXS, undef_access),
+	SYS_INSN(TLBI_VAE2OSNXS, undef_access),
 	SYS_INSN(TLBI_ALLE1OSNXS, handle_alle1is),
-	SYS_INSN(TLBI_VALE2OSNXS, trap_undef),
+	SYS_INSN(TLBI_VALE2OSNXS, undef_access),
 	SYS_INSN(TLBI_VMALLS12E1OSNXS, handle_vmalls12e1is),
 
-	SYS_INSN(TLBI_RVAE2ISNXS, trap_undef),
-	SYS_INSN(TLBI_RVALE2ISNXS, trap_undef),
-	SYS_INSN(TLBI_ALLE2ISNXS, trap_undef),
-	SYS_INSN(TLBI_VAE2ISNXS, trap_undef),
+	SYS_INSN(TLBI_RVAE2ISNXS, undef_access),
+	SYS_INSN(TLBI_RVALE2ISNXS, undef_access),
+	SYS_INSN(TLBI_ALLE2ISNXS, undef_access),
+	SYS_INSN(TLBI_VAE2ISNXS, undef_access),
 
 	SYS_INSN(TLBI_ALLE1ISNXS, handle_alle1is),
-	SYS_INSN(TLBI_VALE2ISNXS, trap_undef),
+	SYS_INSN(TLBI_VALE2ISNXS, undef_access),
 	SYS_INSN(TLBI_VMALLS12E1ISNXS, handle_vmalls12e1is),
 	SYS_INSN(TLBI_IPAS2E1OSNXS, handle_ipas2e1is),
 	SYS_INSN(TLBI_IPAS2E1NXS, handle_ipas2e1is),

@@ -3226,14 +3196,14 @@ static struct sys_reg_desc sys_insn_descs[] = {
 	SYS_INSN(TLBI_IPAS2LE1NXS, handle_ipas2e1is),
 	SYS_INSN(TLBI_RIPAS2LE1NXS, handle_ripas2e1is),
 	SYS_INSN(TLBI_RIPAS2LE1OSNXS, handle_ripas2e1is),
-	SYS_INSN(TLBI_RVAE2OSNXS, trap_undef),
-	SYS_INSN(TLBI_RVALE2OSNXS, trap_undef),
-	SYS_INSN(TLBI_RVAE2NXS, trap_undef),
-	SYS_INSN(TLBI_RVALE2NXS, trap_undef),
-	SYS_INSN(TLBI_ALLE2NXS, trap_undef),
-	SYS_INSN(TLBI_VAE2NXS, trap_undef),
+	SYS_INSN(TLBI_RVAE2OSNXS, undef_access),
+	SYS_INSN(TLBI_RVALE2OSNXS, undef_access),
+	SYS_INSN(TLBI_RVAE2NXS, undef_access),
+	SYS_INSN(TLBI_RVALE2NXS, undef_access),
+	SYS_INSN(TLBI_ALLE2NXS, undef_access),
+	SYS_INSN(TLBI_VAE2NXS, undef_access),
 	SYS_INSN(TLBI_ALLE1NXS, handle_alle1is),
-	SYS_INSN(TLBI_VALE2NXS, trap_undef),
+	SYS_INSN(TLBI_VALE2NXS, undef_access),
 	SYS_INSN(TLBI_VMALLS12E1NXS, handle_vmalls12e1is),
 };
Signature · 1 line
2.39.2
author maintainer
#24 lore raw

PATCH 12/12 KVM: arm64: Add selftest checking how the absence of GICv3 is handled

In reply to Marc Zyngier

Given how tortuous and fragile the whole lack-of-GICv3 story is, add a selftest checking that we don't regress it.

Signed-off-by Marc Zyngier <[email protected]>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/no-vgic-v3.c        | 170 ++++++++++++++++++
 2 files changed, 171 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/no-vgic-v3.c
tools/testing/selftests/kvm/Makefile +1 -0
index 48d32c5aa3eb..f66b37acc0b0 100644
@@ -163,6 +163,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
 TEST_GEN_PROGS_aarch64 += aarch64/vgic_irq
 TEST_GEN_PROGS_aarch64 += aarch64/vgic_lpi_stress
 TEST_GEN_PROGS_aarch64 += aarch64/vpmu_counter_access
+TEST_GEN_PROGS_aarch64 += aarch64/no-vgic-v3
 TEST_GEN_PROGS_aarch64 += access_tracking_perf_test
 TEST_GEN_PROGS_aarch64 += arch_timer
 TEST_GEN_PROGS_aarch64 += demand_paging_test
tools/testing/selftests/kvm/aarch64/no-vgic-v3.c new file +170 -0

170 lines not on this page · raw message

Signature · 1 line
2.39.2
#25 lore raw

Re: [PATCH 12/12] KVM: arm64: Add selftest checking how the absence of GICv3 is handled

Quoted 128 lines · Marc Zyngier

Given how tortuous and fragile the whole lack-of-GICv3 story is, add a selftest checking that we don't regress it.

Signed-off-by Marc Zyngier <[email protected]>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/no-vgic-v3.c        | 170 ++++++++++++++++++
 2 files changed, 171 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/no-vgic-v3.c
tools/testing/selftests/kvm/Makefile +1 -0
index 48d32c5aa3eb..f66b37acc0b0 100644
@@ -163,6 +163,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
 TEST_GEN_PROGS_aarch64 += aarch64/vgic_irq
 TEST_GEN_PROGS_aarch64 += aarch64/vgic_lpi_stress
 TEST_GEN_PROGS_aarch64 += aarch64/vpmu_counter_access
+TEST_GEN_PROGS_aarch64 += aarch64/no-vgic-v3
 TEST_GEN_PROGS_aarch64 += access_tracking_perf_test
 TEST_GEN_PROGS_aarch64 += arch_timer
 TEST_GEN_PROGS_aarch64 += demand_paging_test
tools/testing/selftests/kvm/aarch64/no-vgic-v3.c new file +100 -0
new file mode 100644
index 000000000000..27169afc94c6
@@ -0,0 +1,170 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Check that, on a GICv3 system, not configuring GICv3 correctly
+// results in all of the sysregs generating an UNDEF exception.
+
+#include <test_util.h>
+#include <kvm_util.h>
+#include <processor.h>
+
+static volatile bool handled;
+
+#define __check_sr_read(r)					\
+	do {							\
+		uint64_t val;					\
+								\
+		handled = false;				\
+		dsb(sy);					\
+		val = read_sysreg_s(SYS_ ## r);			\
+		(void)val;					\
+	} while(0)
+
+#define __check_sr_write(r)					\
+	do {							\
+		handled = false;				\
+		dsb(sy);					\
+		write_sysreg_s(0, SYS_ ## r);			\
+		isb();						\
+	} while(0)
+
+/* Fatal checks */
+#define check_sr_read(r)					\
+	do {							\
+		__check_sr_read(r);				\
+		__GUEST_ASSERT(handled, #r " no read trap");	\
+	} while(0)
+
+#define check_sr_write(r)					\
+	do {							\
+		__check_sr_write(r);				\
+		__GUEST_ASSERT(handled, #r " no write trap");	\
+	} while(0)
+
+#define check_sr_rw(r)				\
+	do {					\
+		check_sr_read(r);		\
+		check_sr_write(r);		\
+	} while(0)
+
+/* Non-fatal checks */
+#define check_sr_read_maybe(r)						\
+	do {								\
+		__check_sr_read(r);					\
+		if (!handled)						\
+			GUEST_PRINTF(#r " read not trapping (OK)\n");	\
+	} while(0)
+
+#define check_sr_write_maybe(r)						\
+	do {								\
+		__check_sr_write(r);					\
+		if (!handled)						\
+			GUEST_PRINTF(#r " write not trapping (OK)\n");	\
+	} while(0)
+
+static void guest_code(void)
+{
+	/*
+	 * Check that we advertise that ID_AA64PFR0_EL1.GIC == 0, having
+	 * hidden the feature at runtime without any other userspace action.
+	 */
+	__GUEST_ASSERT(FIELD_GET(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC),
+				 read_sysreg(id_aa64pfr0_el1)) == 0,
+		       "GICv3 wrongly advertised");
+
+	/*
+	 * Access all GICv3 registers, and fail if we don't get an UNDEF.
+	 * Note that we happily access all the APxRn registers without
+	 * checking their existance, as all we want to see is a failure.
+	 */
+	check_sr_rw(ICC_PMR_EL1);
+	check_sr_read(ICC_IAR0_EL1);
+	check_sr_write(ICC_EOIR0_EL1);
+	check_sr_rw(ICC_HPPIR0_EL1);
+	check_sr_rw(ICC_BPR0_EL1);
+	check_sr_rw(ICC_AP0R0_EL1);
+	check_sr_rw(ICC_AP0R1_EL1);
+	check_sr_rw(ICC_AP0R2_EL1);
+	check_sr_rw(ICC_AP0R3_EL1);
+	check_sr_rw(ICC_AP1R0_EL1);
+	check_sr_rw(ICC_AP1R1_EL1);
+	check_sr_rw(ICC_AP1R2_EL1);
+	check_sr_rw(ICC_AP1R3_EL1);
+	check_sr_write(ICC_DIR_EL1);
+	check_sr_read(ICC_RPR_EL1);
+	check_sr_write(ICC_SGI1R_EL1);
+	check_sr_write(ICC_ASGI1R_EL1);
+	check_sr_write(ICC_SGI0R_EL1);
+	check_sr_read(ICC_IAR1_EL1);
+	check_sr_write(ICC_EOIR1_EL1);
+	check_sr_rw(ICC_HPPIR1_EL1);
+	check_sr_rw(ICC_BPR1_EL1);

tools/testing/selftests/kvm/aarch64/no-vgic-v3.c line 110

Quoted 10 lines
tools/testing/selftests/kvm/aarch64/no-vgic-v3.c new file +10 -0
+	check_sr_rw(ICC_CTLR_EL1);
+	check_sr_rw(ICC_IGRPEN0_EL1);
+	check_sr_rw(ICC_IGRPEN1_EL1);
+
+	/*
+	 * ICC_SRE_EL1 may not be trappable, as ICC_SRE_EL2.Enable can
+	 * be RAO/WI
+	 */
+	check_sr_read_maybe(ICC_SRE_EL1);
+	check_sr_write_maybe(ICC_SRE_EL1);

In the case that a write does not UNDEF, should we check that ICC_SRE_EL1.SRE is also RAO/WI?

Signature · 2 lines
Thanks,
Oliver
author maintainer
#26 lore raw

Re: [PATCH 12/12] KVM: arm64: Add selftest checking how the absence of GICv3 is handled

Quoted 135 lines · Oliver Upton
Quoted 133 lines · Marc Zyngier

Given how tortuous and fragile the whole lack-of-GICv3 story is, add a selftest checking that we don't regress it.

Signed-off-by Marc Zyngier <[email protected]>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/no-vgic-v3.c        | 170 ++++++++++++++++++
 2 files changed, 171 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/no-vgic-v3.c
tools/testing/selftests/kvm/Makefile +1 -0
index 48d32c5aa3eb..f66b37acc0b0 100644
@@ -163,6 +163,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/vgic_init
 TEST_GEN_PROGS_aarch64 += aarch64/vgic_irq
 TEST_GEN_PROGS_aarch64 += aarch64/vgic_lpi_stress
 TEST_GEN_PROGS_aarch64 += aarch64/vpmu_counter_access
+TEST_GEN_PROGS_aarch64 += aarch64/no-vgic-v3
 TEST_GEN_PROGS_aarch64 += access_tracking_perf_test
 TEST_GEN_PROGS_aarch64 += arch_timer
 TEST_GEN_PROGS_aarch64 += demand_paging_test
tools/testing/selftests/kvm/aarch64/no-vgic-v3.c new file +105 -0
new file mode 100644
index 000000000000..27169afc94c6
@@ -0,0 +1,170 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Check that, on a GICv3 system, not configuring GICv3 correctly
+// results in all of the sysregs generating an UNDEF exception.
+
+#include <test_util.h>
+#include <kvm_util.h>
+#include <processor.h>
+
+static volatile bool handled;
+
+#define __check_sr_read(r)					\
+	do {							\
+		uint64_t val;					\
+								\
+		handled = false;				\
+		dsb(sy);					\
+		val = read_sysreg_s(SYS_ ## r);			\
+		(void)val;					\
+	} while(0)
+
+#define __check_sr_write(r)					\
+	do {							\
+		handled = false;				\
+		dsb(sy);					\
+		write_sysreg_s(0, SYS_ ## r);			\
+		isb();						\
+	} while(0)
+
+/* Fatal checks */
+#define check_sr_read(r)					\
+	do {							\
+		__check_sr_read(r);				\
+		__GUEST_ASSERT(handled, #r " no read trap");	\
+	} while(0)
+
+#define check_sr_write(r)					\
+	do {							\
+		__check_sr_write(r);				\
+		__GUEST_ASSERT(handled, #r " no write trap");	\
+	} while(0)
+
+#define check_sr_rw(r)				\
+	do {					\
+		check_sr_read(r);		\
+		check_sr_write(r);		\
+	} while(0)
+
+/* Non-fatal checks */
+#define check_sr_read_maybe(r)						\
+	do {								\
+		__check_sr_read(r);					\
+		if (!handled)						\
+			GUEST_PRINTF(#r " read not trapping (OK)\n");	\
+	} while(0)
+
+#define check_sr_write_maybe(r)						\
+	do {								\
+		__check_sr_write(r);					\
+		if (!handled)						\
+			GUEST_PRINTF(#r " write not trapping (OK)\n");	\
+	} while(0)
+
+static void guest_code(void)
+{
+	/*
+	 * Check that we advertise that ID_AA64PFR0_EL1.GIC == 0, having
+	 * hidden the feature at runtime without any other userspace action.
+	 */
+	__GUEST_ASSERT(FIELD_GET(ARM64_FEATURE_MASK(ID_AA64PFR0_EL1_GIC),
+				 read_sysreg(id_aa64pfr0_el1)) == 0,
+		       "GICv3 wrongly advertised");
+
+	/*
+	 * Access all GICv3 registers, and fail if we don't get an UNDEF.
+	 * Note that we happily access all the APxRn registers without
+	 * checking their existance, as all we want to see is a failure.
+	 */
+	check_sr_rw(ICC_PMR_EL1);
+	check_sr_read(ICC_IAR0_EL1);
+	check_sr_write(ICC_EOIR0_EL1);
+	check_sr_rw(ICC_HPPIR0_EL1);
+	check_sr_rw(ICC_BPR0_EL1);
+	check_sr_rw(ICC_AP0R0_EL1);
+	check_sr_rw(ICC_AP0R1_EL1);
+	check_sr_rw(ICC_AP0R2_EL1);
+	check_sr_rw(ICC_AP0R3_EL1);
+	check_sr_rw(ICC_AP1R0_EL1);
+	check_sr_rw(ICC_AP1R1_EL1);
+	check_sr_rw(ICC_AP1R2_EL1);
+	check_sr_rw(ICC_AP1R3_EL1);
+	check_sr_write(ICC_DIR_EL1);
+	check_sr_read(ICC_RPR_EL1);
+	check_sr_write(ICC_SGI1R_EL1);
+	check_sr_write(ICC_ASGI1R_EL1);
+	check_sr_write(ICC_SGI0R_EL1);
+	check_sr_read(ICC_IAR1_EL1);
+	check_sr_write(ICC_EOIR1_EL1);
+	check_sr_rw(ICC_HPPIR1_EL1);
+	check_sr_rw(ICC_BPR1_EL1);
+	check_sr_rw(ICC_CTLR_EL1);
+	check_sr_rw(ICC_IGRPEN0_EL1);
+	check_sr_rw(ICC_IGRPEN1_EL1);
+
+	/*
Quoted 8 lines

tools/testing/selftests/kvm/aarch64/no-vgic-v3.c line 110

Quoted 5 lines
tools/testing/selftests/kvm/aarch64/no-vgic-v3.c new file +5 -0
+	 * ICC_SRE_EL1 may not be trappable, as ICC_SRE_EL2.Enable can
+	 * be RAO/WI
+	 */
+	check_sr_read_maybe(ICC_SRE_EL1);
+	check_sr_write_maybe(ICC_SRE_EL1);

In the case that a write does not UNDEF, should we check that ICC_SRE_EL1.SRE is also RAO/WI?

Ah, not a bad idea. I'll add that.

Thanks,

	M.
Signature · 1 line
Without deviation from the norm, progress is not possible.