[PATCH 2/2] KVM: arm64: Workaround firmware wrongly advertising GICv2-on-v3 compatibility

[Marc Zyngier]

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