On 2021-01-11 12:21, Shameerali Kolothum Thodi wrote:
Hi Marc,
-----Original Message-----
From: Marc Zyngier [mailto:maz@xxxxxxxxxx]
Sent: 08 January 2021 17:12
To: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; kvmarm@xxxxxxxxxxxxxxxxxxxxx
Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>;
James Morse <james.morse@xxxxxxx>; Julien Thierry
<julien.thierry.kdev@xxxxxxxxx>; Suzuki K Poulose
<suzuki.poulose@xxxxxxx>; Ard Biesheuvel <ardb@xxxxxxxxxx>;
kernel-team@xxxxxxxxxxx
Subject: [PATCH 2/2] KVM: arm64: Workaround firmware wrongly
advertising
GICv2-on-v3 compatibility
It looks like we have broken firmware out there that wrongly
advertises
a GICv2 compatibility interface, despite the CPUs not being able to
deal
with it.
To work around this, check that the CPU initialising KVM is actually
able
to switch to MMIO instead of system registers, and use that as a
precondition to enable GICv2 compatibility in KVM.
Note that the detection happens on a single CPU. If the firmware is
lying *and* that the CPUs are asymetric, all hope is lost anyway.
Reported-by: Shameerali Kolothum Thodi
<shameerali.kolothum.thodi@xxxxxxxxxx>
Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
---
arch/arm64/kvm/hyp/vgic-v3-sr.c | 34
+++++++++++++++++++++++++++++++--
arch/arm64/kvm/vgic/vgic-v3.c | 8 ++++++--
2 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c
b/arch/arm64/kvm/hyp/vgic-v3-sr.c
index 005daa0c9dd7..d504499ab917 100644
--- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
+++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c
@@ -408,11 +408,41 @@ void __vgic_v3_init_lrs(void)
/*
* Return the GIC CPU configuration:
* - [31:0] ICH_VTR_EL2
- * - [63:32] RES0
+ * - [62:32] RES0
+ * - [63] MMIO (GICv2) capable
*/
u64 __vgic_v3_get_gic_config(void)
{
- return read_gicreg(ICH_VTR_EL2);
+ u64 sre = read_gicreg(ICC_SRE_EL1);
+ unsigned long flags = 0;
+ bool v2_capable;
+
+ /*
+ * To check whether we have a MMIO-based (GICv2 compatible)
+ * CPU interface, we need to disable the system register
+ * view. To do that safely, we have to prevent any interrupt
+ * from firing (which would be deadly).
+ *
+ * Note that this only makes sense on VHE, as interrupts are
+ * already masked for nVHE as part of the exception entry to
+ * EL2.
+ */
+ if (has_vhe())
+ flags = local_daif_save();
+
+ write_gicreg(0, ICC_SRE_EL1);
+ isb();
+
+ v2_capable = !(read_gicreg(ICC_SRE_EL1) & ICC_SRE_EL1_SRE);
+
+ write_gicreg(sre, ICC_SRE_EL1);
+ isb();
+
+ if (has_vhe())
+ local_daif_restore(flags);
+
+ return (read_gicreg(ICH_VTR_EL2) |
+ v2_capable ? (1ULL << 63) : 0);
}
Thanks for sending this out. I had a go with this series and
unfortunately
it didn't work on a system with faulty BIOS. It looks like the culprit
here is
the ?: operator. There seems to be an operator precedence at play here
and it returns,
vgic_v3_probe: ich_vtr_el2 0x8000000000000000
And with the below change,
return (read_gicreg(ICH_VTR_EL2) |
- v2_capable ? (1ULL << 63) : 0);
+ (v2_capable ? (1ULL << 63) : 0));
Gaahh. Well caught! Each time I use this operator, I end-up screwing
up one way or another. Thanks for the heads up, and for testing.
I'll respin the series shortly.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
