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

[Ard Biesheuvel]

On Fri, 8 Jan 2021 at 19:13, Marc Zyngier <maz at kernel.org> wrote:
>
> On 2021-01-08 17:59, Ard Biesheuvel wrote:
> > On Fri, 8 Jan 2021 at 18:12, Marc Zyngier <maz at 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 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);
> >>  }
> >>
> >
> > 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)?
>

No I don't think it's a problem per se. Just a bit disappointing that
every system will be burdened with this for as long as the last v2
compat capable system is still being supported.