On 2021-01-11 12:21, Shameerali Kolothum Thodi wrote:
Hi Marc,
-----Original Message----- From: Marc Zyngier [mailto:maz@kernel.org] Sent: 08 January 2021 17:12 To: linux-arm-kernel@lists.infradead.org; kvmarm@lists.cs.columbia.edu Cc: Shameerali Kolothum Thodi shameerali.kolothum.thodi@huawei.com; James Morse james.morse@arm.com; Julien Thierry julien.thierry.kdev@gmail.com; Suzuki K Poulose suzuki.poulose@arm.com; Ard Biesheuvel ardb@kernel.org; kernel-team@android.com 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@huawei.com Signed-off-by: Marc Zyngier maz@kernel.org
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.