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

[Marc Zyngier <maz@kernel.org>]

On 2021-01-08 17:59, Ard Biesheuvel wrote:
> On Fri, 8 Jan 2021 at 18:12, Marc Zyngier <maz@kernel.org> wrote:
>> 
>> 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);
>>  }
>> 
> 
> Is it necessary to perform this check unconditionally? We only care
> about this if the firmware claims v2 compat support.

Indeed. But this is done exactly once per boot, and I see it as
a way to extract the CPU configuration more than anything else.

Extracting it *only* when we have some v2 compat info would mean
sharing that information with EL2 (in the nVHE case), and it felt
more hassle than it is worth.

Do you foresee any issue with this, other than the whole thing
being disgusting (which I wilfully admit)?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

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