linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] KVM: arm64: Assorted vgic-v3 fixes
@ 2021-10-10 15:09 Marc Zyngier
  2021-10-10 15:09 ` [PATCH v2 1/5] KVM: arm64: Force ID_AA64PFR0_EL1.GIC=1 when exposing a virtual GICv3 Marc Zyngier
                   ` (5 more replies)
  0 siblings, 6 replies; 8+ messages in thread
From: Marc Zyngier @ 2021-10-10 15:09 UTC (permalink / raw)
  To: kvmarm, kvm, linux-arm-kernel
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Joey Gouly, kernel-team

Here's a bunch of vgic-v3 fixes I have been sitting on for some
time. None of them are critical, though some are rather entertaining.

The first one is a leftover from the initial Apple-M1 enablement,
which doesn't advertise the GIC support via ID_AA64PFR0_EL1 (which is
expected, as it only has half a GIC...). We address it by forcefully
advertising the feature if the guest has a GICv3.

The second patch is really fun, and shows how things can go wrong when
they are badly specified. The gist of it is that on systems that
advertise ICH_VTR_EL2.SEIS, we need to fallback to the full GICv3
cpuif emulation. The third patch is an good optimisation on the
previous one, and the fourth a direct consequence of the whole thing.

The last patch are more of a harmless oddity: virtual LPIs happen to
have an active state buried into the pseudocode (and only there). Fun!
Nothing goes wrong with that, but we need to align the emulation to
match the pseudocode.

All of this is only targeting 5.16, and I don't plan to backport any
of it.

* From v1 [1]:

  - Dropped the patch that tried to optimise what to do with an active
    LPI. There is unfortunately a bad corner case in the pseudocode
    that prevents it. Oh well.
    
  - Added an extra patch to help in the case where ICH_HCR_EL2.TDS is
    supported, and that we can use that instead of ICH_HCR_EL2.TC to
    trap only ICC_DIR_EL1. Given the performance improvement, it was
    too hard to ignore it.

[1] https://lore.kernel.org/r/20210924082542.2766170-1-maz@kernel.org

Marc Zyngier (5):
  KVM: arm64: Force ID_AA64PFR0_EL1.GIC=1 when exposing a virtual GICv3
  KVM: arm64: vgic-v3: Work around GICv3 locally generated SErrors
  KVM: arm64: vgic-v3: Reduce common group trapping to ICV_DIR_EL1 when
    possible
  KVM: arm64: vgic-v3: Don't advertise ICC_CTLR_EL1.SEIS
  KVM: arm64: vgic-v3: Align emulated cpuif LPI state machine with the
    pseudocode

 arch/arm64/include/asm/sysreg.h |  3 +++
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 22 ++++++++--------------
 arch/arm64/kvm/sys_regs.c       |  5 +++++
 arch/arm64/kvm/vgic/vgic-v3.c   | 21 ++++++++++++++++++---
 4 files changed, 34 insertions(+), 17 deletions(-)

-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/5] KVM: arm64: Force ID_AA64PFR0_EL1.GIC=1 when exposing a virtual GICv3
  2021-10-10 15:09 [PATCH v2 0/5] KVM: arm64: Assorted vgic-v3 fixes Marc Zyngier
@ 2021-10-10 15:09 ` Marc Zyngier
  2021-10-10 15:09 ` [PATCH v2 2/5] KVM: arm64: vgic-v3: Work around GICv3 locally generated SErrors Marc Zyngier
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2021-10-10 15:09 UTC (permalink / raw)
  To: kvmarm, kvm, linux-arm-kernel
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Joey Gouly, kernel-team

Until now, we always let ID_AA64PFR0_EL1.GIC reflect the value
visible on the host, even if we were running a GICv2-enabled VM
on a GICv3+compat host.

That's fine, but we also now have the case of a host that does not
expose ID_AA64PFR0_EL1.GIC==1 despite having a vGIC. Yes, this is
confusing. Thank you M1.

Let's go back to first principles and expose ID_AA64PFR0_EL1.GIC=1
when a GICv3 is exposed to the guest. This also hides a GICv4.1
CPU interface from the guest which has no business knowing about
the v4.1 extension.

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 1d46e185f31e..0e8fc29df19c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1075,6 +1075,11 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 		val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), (u64)vcpu->kvm->arch.pfr0_csv2);
 		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3);
 		val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), (u64)vcpu->kvm->arch.pfr0_csv3);
+		if (irqchip_in_kernel(vcpu->kvm) &&
+		    vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
+			val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC);
+			val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1);
+		}
 		break;
 	case SYS_ID_AA64PFR1_EL1:
 		val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_MTE);
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/5] KVM: arm64: vgic-v3: Work around GICv3 locally generated SErrors
  2021-10-10 15:09 [PATCH v2 0/5] KVM: arm64: Assorted vgic-v3 fixes Marc Zyngier
  2021-10-10 15:09 ` [PATCH v2 1/5] KVM: arm64: Force ID_AA64PFR0_EL1.GIC=1 when exposing a virtual GICv3 Marc Zyngier
@ 2021-10-10 15:09 ` Marc Zyngier
  2021-10-10 15:09 ` [PATCH v2 3/5] KVM: arm64: vgic-v3: Reduce common group trapping to ICV_DIR_EL1 when possible Marc Zyngier
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2021-10-10 15:09 UTC (permalink / raw)
  To: kvmarm, kvm, linux-arm-kernel
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Joey Gouly, kernel-team

The infamous M1 has a feature nobody else ever implemented,
in the form of the "GIC locally generated SError interrupts",
also known as SEIS for short.

These SErrors are generated when a guest does something that violates
the GIC state machine. It would have been simpler to just *ignore*
the damned thing, but that's not what this HW does. Oh well.

This part of of the architecture is also amazingly under-specified.
There is a whole 10 lines that describe the feature in a spec that
is 930 pages long, and some of these lines are factually wrong.
Oh, and it is deprecated, so the insentive to clarify it is low.

Now, the spec says that this should be a *virtual* SError when
HCR_EL2.AMO is set. As it turns out, that's not always the case
on this CPU, and the SError sometimes fires on the host as a
physical SError. Goodbye, cruel world. This clearly is a HW bug,
and it means that a guest can easily take the host down, on demand.

Thankfully, we have seen systems that were just as broken in the
past, and we have the perfect vaccine for it.

Apple M1, please meet the Cavium ThunderX workaround. All your
GIC accesses will be trapped, sanitised, and emulated. Only the
signalling aspect of the HW will be used. It won't be super speedy,
but it will at least be safe. You're most welcome.

Given that this has only ever been seen on this single implementation,
that the spec is unclear at best and that we cannot trust it to ever
be implemented correctly, gate the workaround solely on ICH_VTR_EL2.SEIS
being set.

Tested-by: Joey Gouly <joey.gouly@arm.com>
Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/vgic/vgic-v3.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 21a6207fb2ee..ae59e2580bf5 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -671,6 +671,14 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
 		group1_trap = true;
 	}
 
+	if (kvm_vgic_global_state.ich_vtr_el2 & ICH_VTR_SEIS_MASK) {
+		kvm_info("GICv3 with locally generated SEI\n");
+
+		group0_trap = true;
+		group1_trap = true;
+		common_trap = true;
+	}
+
 	if (group0_trap || group1_trap || common_trap) {
 		kvm_info("GICv3 sysreg trapping enabled ([%s%s%s], reduced performance)\n",
 			 group0_trap ? "G0" : "",
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 3/5] KVM: arm64: vgic-v3: Reduce common group trapping to ICV_DIR_EL1 when possible
  2021-10-10 15:09 [PATCH v2 0/5] KVM: arm64: Assorted vgic-v3 fixes Marc Zyngier
  2021-10-10 15:09 ` [PATCH v2 1/5] KVM: arm64: Force ID_AA64PFR0_EL1.GIC=1 when exposing a virtual GICv3 Marc Zyngier
  2021-10-10 15:09 ` [PATCH v2 2/5] KVM: arm64: vgic-v3: Work around GICv3 locally generated SErrors Marc Zyngier
@ 2021-10-10 15:09 ` Marc Zyngier
  2021-10-12 16:05   ` Alexandru Elisei
  2021-10-10 15:09 ` [PATCH v2 4/5] KVM: arm64: vgic-v3: Don't advertise ICC_CTLR_EL1.SEIS Marc Zyngier
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 8+ messages in thread
From: Marc Zyngier @ 2021-10-10 15:09 UTC (permalink / raw)
  To: kvmarm, kvm, linux-arm-kernel
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Joey Gouly, kernel-team

On systems that advertise ICH_VTR_EL2.SEIS, we trap all GICv3 sysreg
accesses from the guest. From a performance perspective, this is OK
as long as the guest doesn't hammer the GICv3 CPU interface.

In most cases, this is fine, unless the guest actively uses
priorities and switches PMR_EL1 very often. Which is exactly what
happens when a Linux guest runs with irqchip.gicv3_pseudo_nmi=1.
In these condition, the performance plumets as we hit PMR each time
we mask/unmask interrupts. Not good.

There is however an opportunity for improvement. Careful reading
of the architecture specification indicates that the only GICv3
sysreg belonging to the common group (which contains the SGI
registers, PMR, DIR, CTLR and RPR) that is allowed to generate
a SError is DIR. Everything else is safe.

It is thus possible to substitute the trapping of all the common
group with just that of DIR if it supported by the implementation.
Yes, that's yet another optional bit of the architecture.
So let's just do that, as it leads to some impressive result on
the M1:

Without this change:
	bash-5.1# /host/home/maz/hackbench 100 process 1000
	Running with 100*40 (== 4000) tasks.
	Time: 56.596

With this change:
	bash-5.1# /host/home/maz/hackbench 100 process 1000
	Running with 100*40 (== 4000) tasks.
	Time: 8.649

which is a pretty convincing result.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/sysreg.h |  3 +++
 arch/arm64/kvm/vgic/vgic-v3.c   | 15 +++++++++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index b268082d67ed..9412a645a1c0 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -1152,6 +1152,7 @@
 #define ICH_HCR_TC		(1 << 10)
 #define ICH_HCR_TALL0		(1 << 11)
 #define ICH_HCR_TALL1		(1 << 12)
+#define ICH_HCR_TDIR		(1 << 14)
 #define ICH_HCR_EOIcount_SHIFT	27
 #define ICH_HCR_EOIcount_MASK	(0x1f << ICH_HCR_EOIcount_SHIFT)
 
@@ -1184,6 +1185,8 @@
 #define ICH_VTR_SEIS_MASK	(1 << ICH_VTR_SEIS_SHIFT)
 #define ICH_VTR_A3V_SHIFT	21
 #define ICH_VTR_A3V_MASK	(1 << ICH_VTR_A3V_SHIFT)
+#define ICH_VTR_TDS_SHIFT	19
+#define ICH_VTR_TDS_MASK	(1 << ICH_VTR_TDS_SHIFT)
 
 #define ARM64_FEATURE_FIELD_BITS	4
 
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index ae59e2580bf5..467c22bbade6 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -15,6 +15,7 @@
 static bool group0_trap;
 static bool group1_trap;
 static bool common_trap;
+static bool dir_trap;
 static bool gicv4_enable;
 
 void vgic_v3_set_underflow(struct kvm_vcpu *vcpu)
@@ -296,6 +297,8 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu)
 		vgic_v3->vgic_hcr |= ICH_HCR_TALL1;
 	if (common_trap)
 		vgic_v3->vgic_hcr |= ICH_HCR_TC;
+	if (dir_trap)
+		vgic_v3->vgic_hcr |= ICH_HCR_TDIR;
 }
 
 int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
@@ -676,14 +679,18 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
 
 		group0_trap = true;
 		group1_trap = true;
-		common_trap = true;
+		if (ich_vtr_el2 & ICH_VTR_TDS_MASK)
+			dir_trap = true;
+		else
+			common_trap = true;
 	}
 
-	if (group0_trap || group1_trap || common_trap) {
-		kvm_info("GICv3 sysreg trapping enabled ([%s%s%s], reduced performance)\n",
+	if (group0_trap || group1_trap || common_trap | dir_trap) {
+		kvm_info("GICv3 sysreg trapping enabled ([%s%s%s%s], reduced performance)\n",
 			 group0_trap ? "G0" : "",
 			 group1_trap ? "G1" : "",
-			 common_trap ? "C"  : "");
+			 common_trap ? "C"  : "",
+			 dir_trap    ? "D"  : "");
 		static_branch_enable(&vgic_v3_cpuif_trap);
 	}
 
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 4/5] KVM: arm64: vgic-v3: Don't advertise ICC_CTLR_EL1.SEIS
  2021-10-10 15:09 [PATCH v2 0/5] KVM: arm64: Assorted vgic-v3 fixes Marc Zyngier
                   ` (2 preceding siblings ...)
  2021-10-10 15:09 ` [PATCH v2 3/5] KVM: arm64: vgic-v3: Reduce common group trapping to ICV_DIR_EL1 when possible Marc Zyngier
@ 2021-10-10 15:09 ` Marc Zyngier
  2021-10-10 15:09 ` [PATCH v2 5/5] KVM: arm64: vgic-v3: Align emulated cpuif LPI state machine with the pseudocode Marc Zyngier
  2021-10-17 10:11 ` [PATCH v2 0/5] KVM: arm64: Assorted vgic-v3 fixes Marc Zyngier
  5 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2021-10-10 15:09 UTC (permalink / raw)
  To: kvmarm, kvm, linux-arm-kernel
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Joey Gouly, kernel-team

Since we are trapping all sysreg accesses when ICH_VTR_EL2.SEIS
is set, and that we never deliver an SError when emulating
any of the GICv3 sysregs, don't advertise ICC_CTLR_EL1.SEIS.

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
index 39f8f7f9227c..b3b50de496a3 100644
--- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
+++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
@@ -987,8 +987,6 @@ static void __vgic_v3_read_ctlr(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 	val = ((vtr >> 29) & 7) << ICC_CTLR_EL1_PRI_BITS_SHIFT;
 	/* IDbits */
 	val |= ((vtr >> 23) & 7) << ICC_CTLR_EL1_ID_BITS_SHIFT;
-	/* SEIS */
-	val |= ((vtr >> 22) & 1) << ICC_CTLR_EL1_SEIS_SHIFT;
 	/* A3V */
 	val |= ((vtr >> 21) & 1) << ICC_CTLR_EL1_A3V_SHIFT;
 	/* EOImode */
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 5/5] KVM: arm64: vgic-v3: Align emulated cpuif LPI state machine with the pseudocode
  2021-10-10 15:09 [PATCH v2 0/5] KVM: arm64: Assorted vgic-v3 fixes Marc Zyngier
                   ` (3 preceding siblings ...)
  2021-10-10 15:09 ` [PATCH v2 4/5] KVM: arm64: vgic-v3: Don't advertise ICC_CTLR_EL1.SEIS Marc Zyngier
@ 2021-10-10 15:09 ` Marc Zyngier
  2021-10-17 10:11 ` [PATCH v2 0/5] KVM: arm64: Assorted vgic-v3 fixes Marc Zyngier
  5 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2021-10-10 15:09 UTC (permalink / raw)
  To: kvmarm, kvm, linux-arm-kernel
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Joey Gouly, kernel-team

Having realised that a virtual LPI does transition through an active
state that does not exist on bare metal, align the CPU interface
emulation with the behaviour specified in the architecture pseudocode.

The LPIs now transition to active on IAR read, and to inactive on
EOI write. Special care is taken not to increment the EOIcount for
an LPI that isn't present in the LRs.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
index b3b50de496a3..20db2f281cf2 100644
--- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
+++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
@@ -695,9 +695,7 @@ static void __vgic_v3_read_iar(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 		goto spurious;
 
 	lr_val &= ~ICH_LR_STATE;
-	/* No active state for LPIs */
-	if ((lr_val & ICH_LR_VIRTUAL_ID_MASK) <= VGIC_MAX_SPI)
-		lr_val |= ICH_LR_ACTIVE_BIT;
+	lr_val |= ICH_LR_ACTIVE_BIT;
 	__gic_v3_set_lr(lr_val, lr);
 	__vgic_v3_set_active_priority(lr_prio, vmcr, grp);
 	vcpu_set_reg(vcpu, rt, lr_val & ICH_LR_VIRTUAL_ID_MASK);
@@ -764,20 +762,18 @@ static void __vgic_v3_write_eoir(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
 	/* Drop priority in any case */
 	act_prio = __vgic_v3_clear_highest_active_priority();
 
-	/* If EOIing an LPI, no deactivate to be performed */
-	if (vid >= VGIC_MIN_LPI)
-		return;
-
-	/* EOImode == 1, nothing to be done here */
-	if (vmcr & ICH_VMCR_EOIM_MASK)
-		return;
-
 	lr = __vgic_v3_find_active_lr(vcpu, vid, &lr_val);
 	if (lr == -1) {
-		__vgic_v3_bump_eoicount();
+		/* Do not bump EOIcount for LPIs that aren't in the LRs */
+		if (!(vid >= VGIC_MIN_LPI))
+			__vgic_v3_bump_eoicount();
 		return;
 	}
 
+	/* EOImode == 1 and not an LPI, nothing to be done here */
+	if ((vmcr & ICH_VMCR_EOIM_MASK) && !(vid >= VGIC_MIN_LPI))
+		return;
+
 	lr_prio = (lr_val & ICH_LR_PRIORITY_MASK) >> ICH_LR_PRIORITY_SHIFT;
 
 	/* If priorities or group do not match, the guest has fscked-up. */
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 3/5] KVM: arm64: vgic-v3: Reduce common group trapping to ICV_DIR_EL1 when possible
  2021-10-10 15:09 ` [PATCH v2 3/5] KVM: arm64: vgic-v3: Reduce common group trapping to ICV_DIR_EL1 when possible Marc Zyngier
@ 2021-10-12 16:05   ` Alexandru Elisei
  0 siblings, 0 replies; 8+ messages in thread
From: Alexandru Elisei @ 2021-10-12 16:05 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: kvmarm, kvm, linux-arm-kernel, James Morse, Suzuki K Poulose,
	Eric Auger, Joey Gouly, kernel-team

Hi Marc,

On Sun, Oct 10, 2021 at 04:09:08PM +0100, Marc Zyngier wrote:
> On systems that advertise ICH_VTR_EL2.SEIS, we trap all GICv3 sysreg
> accesses from the guest. From a performance perspective, this is OK
> as long as the guest doesn't hammer the GICv3 CPU interface.
> 
> In most cases, this is fine, unless the guest actively uses
> priorities and switches PMR_EL1 very often. Which is exactly what
> happens when a Linux guest runs with irqchip.gicv3_pseudo_nmi=1.
> In these condition, the performance plumets as we hit PMR each time
> we mask/unmask interrupts. Not good.
> 
> There is however an opportunity for improvement. Careful reading
> of the architecture specification indicates that the only GICv3
> sysreg belonging to the common group (which contains the SGI
> registers, PMR, DIR, CTLR and RPR) that is allowed to generate
> a SError is DIR. Everything else is safe.
> 
> It is thus possible to substitute the trapping of all the common
> group with just that of DIR if it supported by the implementation.
> Yes, that's yet another optional bit of the architecture.
> So let's just do that, as it leads to some impressive result on
> the M1:
> 
> Without this change:
> 	bash-5.1# /host/home/maz/hackbench 100 process 1000
> 	Running with 100*40 (== 4000) tasks.
> 	Time: 56.596
> 
> With this change:
> 	bash-5.1# /host/home/maz/hackbench 100 process 1000
> 	Running with 100*40 (== 4000) tasks.
> 	Time: 8.649
> 
> which is a pretty convincing result.

This is a very good idea. Checked when I reviewed the latest iteration that only
ICC_DIR_EL1/ICV_DIR_EL1 can cause SErrors, so this approach looks sensible to
me.

Also checked the bit field positions:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,
Alex

> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/include/asm/sysreg.h |  3 +++
>  arch/arm64/kvm/vgic/vgic-v3.c   | 15 +++++++++++----
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index b268082d67ed..9412a645a1c0 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -1152,6 +1152,7 @@
>  #define ICH_HCR_TC		(1 << 10)
>  #define ICH_HCR_TALL0		(1 << 11)
>  #define ICH_HCR_TALL1		(1 << 12)
> +#define ICH_HCR_TDIR		(1 << 14)
>  #define ICH_HCR_EOIcount_SHIFT	27
>  #define ICH_HCR_EOIcount_MASK	(0x1f << ICH_HCR_EOIcount_SHIFT)
>  
> @@ -1184,6 +1185,8 @@
>  #define ICH_VTR_SEIS_MASK	(1 << ICH_VTR_SEIS_SHIFT)
>  #define ICH_VTR_A3V_SHIFT	21
>  #define ICH_VTR_A3V_MASK	(1 << ICH_VTR_A3V_SHIFT)
> +#define ICH_VTR_TDS_SHIFT	19
> +#define ICH_VTR_TDS_MASK	(1 << ICH_VTR_TDS_SHIFT)
>  
>  #define ARM64_FEATURE_FIELD_BITS	4
>  
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index ae59e2580bf5..467c22bbade6 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -15,6 +15,7 @@
>  static bool group0_trap;
>  static bool group1_trap;
>  static bool common_trap;
> +static bool dir_trap;
>  static bool gicv4_enable;
>  
>  void vgic_v3_set_underflow(struct kvm_vcpu *vcpu)
> @@ -296,6 +297,8 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu)
>  		vgic_v3->vgic_hcr |= ICH_HCR_TALL1;
>  	if (common_trap)
>  		vgic_v3->vgic_hcr |= ICH_HCR_TC;
> +	if (dir_trap)
> +		vgic_v3->vgic_hcr |= ICH_HCR_TDIR;
>  }
>  
>  int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
> @@ -676,14 +679,18 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
>  
>  		group0_trap = true;
>  		group1_trap = true;
> -		common_trap = true;
> +		if (ich_vtr_el2 & ICH_VTR_TDS_MASK)
> +			dir_trap = true;
> +		else
> +			common_trap = true;
>  	}
>  
> -	if (group0_trap || group1_trap || common_trap) {
> -		kvm_info("GICv3 sysreg trapping enabled ([%s%s%s], reduced performance)\n",
> +	if (group0_trap || group1_trap || common_trap | dir_trap) {
> +		kvm_info("GICv3 sysreg trapping enabled ([%s%s%s%s], reduced performance)\n",
>  			 group0_trap ? "G0" : "",
>  			 group1_trap ? "G1" : "",
> -			 common_trap ? "C"  : "");
> +			 common_trap ? "C"  : "",
> +			 dir_trap    ? "D"  : "");
>  		static_branch_enable(&vgic_v3_cpuif_trap);
>  	}
>  
> -- 
> 2.30.2
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/5] KVM: arm64: Assorted vgic-v3 fixes
  2021-10-10 15:09 [PATCH v2 0/5] KVM: arm64: Assorted vgic-v3 fixes Marc Zyngier
                   ` (4 preceding siblings ...)
  2021-10-10 15:09 ` [PATCH v2 5/5] KVM: arm64: vgic-v3: Align emulated cpuif LPI state machine with the pseudocode Marc Zyngier
@ 2021-10-17 10:11 ` Marc Zyngier
  5 siblings, 0 replies; 8+ messages in thread
From: Marc Zyngier @ 2021-10-17 10:11 UTC (permalink / raw)
  To: kvmarm, kvm, Marc Zyngier, linux-arm-kernel
  Cc: James Morse, Eric Auger, Alexandru Elisei, Suzuki K Poulose,
	Joey Gouly, kernel-team

On Sun, 10 Oct 2021 16:09:05 +0100, Marc Zyngier wrote:
> Here's a bunch of vgic-v3 fixes I have been sitting on for some
> time. None of them are critical, though some are rather entertaining.
> 
> The first one is a leftover from the initial Apple-M1 enablement,
> which doesn't advertise the GIC support via ID_AA64PFR0_EL1 (which is
> expected, as it only has half a GIC...). We address it by forcefully
> advertising the feature if the guest has a GICv3.
> 
> [...]

Applied to next, thanks!

[1/5] KVM: arm64: Force ID_AA64PFR0_EL1.GIC=1 when exposing a virtual GICv3
      commit: 562e530fd7707aad7fed953692d1835612238966
[2/5] KVM: arm64: vgic-v3: Work around GICv3 locally generated SErrors
      commit: df652bcf1136db7f16e486a204ba4b4fc4181759
[3/5] KVM: arm64: vgic-v3: Reduce common group trapping to ICV_DIR_EL1 when possible
      commit: 0924729b21bffdd0e13f29ea6256d299fc807cff
[4/5] KVM: arm64: vgic-v3: Don't advertise ICC_CTLR_EL1.SEIS
      commit: f87ab682722299cddf8cf5f7bc17053d70300ee0
[5/5] KVM: arm64: vgic-v3: Align emulated cpuif LPI state machine with the pseudocode
      commit: 9d449c71bd8f74282e84213c8f0b8328293ab0a7

Cheers,

	M.
-- 
Without deviation from the norm, progress is not possible.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-10-17 10:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-10 15:09 [PATCH v2 0/5] KVM: arm64: Assorted vgic-v3 fixes Marc Zyngier
2021-10-10 15:09 ` [PATCH v2 1/5] KVM: arm64: Force ID_AA64PFR0_EL1.GIC=1 when exposing a virtual GICv3 Marc Zyngier
2021-10-10 15:09 ` [PATCH v2 2/5] KVM: arm64: vgic-v3: Work around GICv3 locally generated SErrors Marc Zyngier
2021-10-10 15:09 ` [PATCH v2 3/5] KVM: arm64: vgic-v3: Reduce common group trapping to ICV_DIR_EL1 when possible Marc Zyngier
2021-10-12 16:05   ` Alexandru Elisei
2021-10-10 15:09 ` [PATCH v2 4/5] KVM: arm64: vgic-v3: Don't advertise ICC_CTLR_EL1.SEIS Marc Zyngier
2021-10-10 15:09 ` [PATCH v2 5/5] KVM: arm64: vgic-v3: Align emulated cpuif LPI state machine with the pseudocode Marc Zyngier
2021-10-17 10:11 ` [PATCH v2 0/5] KVM: arm64: Assorted vgic-v3 fixes Marc Zyngier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).