On Fri, 8 Jan 2021 at 19:13, Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On 2021-01-08 17:59, Ard Biesheuvel wrote:
> > On Fri, 8 Jan 2021 at 18:12, Marc Zyngier <maz@xxxxxxxxxx> 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@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);
> >> }
> >>
> >
> > 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.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
