| Message ID | 20210510134824.1910399-5-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Initial host support for the Apple M1 | expand |
Hi Marc, On 5/10/21 2:48 PM, Marc Zyngier wrote: > The vGIC, as architected by ARM, allows a virtual interrupt to > trigger the deactivation of a physical interrupt. This allows > the following interrupt to be delivered without requiring an exit. If I got this right, the AIC doesn't implement/ignores the ICH_LR_EL2.HW bit. Does it mean that the CPU IF behaves as if HW = 0b0, meaning it asserts a maintenance interrupt on virtual interrupt deactivation when ICH_LR_EL2.EOI = 0b1? I assume that's the case, just double checking. I am wondering what would happen if we come across an interrupt controller where the CPU IF cannot assert a maintenance interrupt at all and we rely on the EOI bit to take us out of the guest to deactivate the HW interrupt. I have to say that it looks a bit strange to start relying on the maintenance interrupt to emulate interrupt deactivate for hardware interrupts, but at the same timer allowing an interrupt controller without a maintenance interrupt. Other than that, this idea sounds like the best thing to do considering the circumstances, I certainly can't think of anything better. > > However, some implementations have choosen not to implement this, > meaning that we will need some unsavoury workarounds to deal with this. > > On detecting such a case, taint the kernel and spit a nastygram. > We'll deal with this in later patches. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/vgic/vgic-init.c | 10 ++++++++++ > include/kvm/arm_vgic.h | 3 +++ > include/linux/irqchip/arm-vgic-info.h | 2 ++ > 3 files changed, 15 insertions(+) > > diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c > index 9fd23f32aa54..5b06a9970a57 100644 > --- a/arch/arm64/kvm/vgic/vgic-init.c > +++ b/arch/arm64/kvm/vgic/vgic-init.c > @@ -524,6 +524,16 @@ int kvm_vgic_hyp_init(void) > if (!gic_kvm_info) > return -ENODEV; > > + /* > + * If we get one of these oddball non-GICs, taint the kernel, > + * as we have no idea of how they *really* behave. > + */ > + if (gic_kvm_info->no_hw_deactivation) { > + kvm_info("Non-architectural vgic, tainting kernel\n"); > + add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK); > + kvm_vgic_global_state.no_hw_deactivation = true; > + } IMO, since this means we're going to rely even more on the maintenance interrupt (not just for software emulation of level sensitive interrupts), I think there should be some sort of dependency on having something that resembles a working maintenance interrupt. Thanks, Alex > + > switch (gic_kvm_info->type) { > case GIC_V2: > ret = vgic_v2_probe(gic_kvm_info); > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h > index ec621180ef09..e45b26e8d479 100644 > --- a/include/kvm/arm_vgic.h > +++ b/include/kvm/arm_vgic.h > @@ -72,6 +72,9 @@ struct vgic_global { > bool has_gicv4; > bool has_gicv4_1; > > + /* Pseudo GICv3 from outer space */ > + bool no_hw_deactivation; > + > /* GIC system register CPU interface */ > struct static_key_false gicv3_cpuif; > > diff --git a/include/linux/irqchip/arm-vgic-info.h b/include/linux/irqchip/arm-vgic-info.h > index 0319636be928..d39d0b591c5a 100644 > --- a/include/linux/irqchip/arm-vgic-info.h > +++ b/include/linux/irqchip/arm-vgic-info.h > @@ -30,6 +30,8 @@ struct gic_kvm_info { > bool has_v4; > /* rvpeid support */ > bool has_v4_1; > + /* Deactivation impared, subpar stuff */ > + bool no_hw_deactivation; > }; > > #ifdef CONFIG_KVM
Hi Alex, On Fri, 21 May 2021 18:01:05 +0100, Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > Hi Marc, > > On 5/10/21 2:48 PM, Marc Zyngier wrote: > > The vGIC, as architected by ARM, allows a virtual interrupt to > > trigger the deactivation of a physical interrupt. This allows > > the following interrupt to be delivered without requiring an exit. > > If I got this right, the AIC doesn't implement/ignores the s/AIC/M1 CPU/ > ICH_LR_EL2.HW bit. Does it mean that the CPU IF behaves as if HW = > 0b0, meaning it asserts a maintenance interrupt on virtual interrupt > deactivation when ICH_LR_EL2.EOI = 0b1? I assume that's the case, > just double checking. Yes, that's what it looks like. > I am wondering what would happen if we come across an interrupt > controller where the CPU IF cannot assert a maintenance interrupt at > all and we rely on the EOI bit to take us out of the guest to > deactivate the HW interrupt. That'd be broken, and we wouldn't be able to support such an implementation, at least not in configuration such as CPU isolation. > I have to say that it looks a bit strange to start relying on the > maintenance interrupt to emulate interrupt deactivate for hardware > interrupts, but at the same timer allowing an interrupt controller > without a maintenance interrupt. We are not allowing a vGIC without a maintenance interrupt. We are actively mandating it. The M1 does have a working per-CPU maintenance interrupt. It just isn't wired into an interrupt controller, which means we can't mask it. But it is otherwise perfectly functional. > Other than that, this idea sounds like the best thing to do > considering the circumstances, I certainly can't think of anything > better. > > > > > However, some implementations have choosen not to implement this, > > meaning that we will need some unsavoury workarounds to deal with this. > > > > On detecting such a case, taint the kernel and spit a nastygram. > > We'll deal with this in later patches. > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > arch/arm64/kvm/vgic/vgic-init.c | 10 ++++++++++ > > include/kvm/arm_vgic.h | 3 +++ > > include/linux/irqchip/arm-vgic-info.h | 2 ++ > > 3 files changed, 15 insertions(+) > > > > diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c > > index 9fd23f32aa54..5b06a9970a57 100644 > > --- a/arch/arm64/kvm/vgic/vgic-init.c > > +++ b/arch/arm64/kvm/vgic/vgic-init.c > > @@ -524,6 +524,16 @@ int kvm_vgic_hyp_init(void) > > if (!gic_kvm_info) > > return -ENODEV; > > > > + /* > > + * If we get one of these oddball non-GICs, taint the kernel, > > + * as we have no idea of how they *really* behave. > > + */ > > + if (gic_kvm_info->no_hw_deactivation) { > > + kvm_info("Non-architectural vgic, tainting kernel\n"); > > + add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK); > > + kvm_vgic_global_state.no_hw_deactivation = true; > > + } > > IMO, since this means we're going to rely even more on the > maintenance interrupt (not just for software emulation of level > sensitive interrupts), I think there should be some sort of > dependency on having something that resembles a working maintenance > interrupt. But the timer interrupt is exactly a SW emulation of a level sensitive interrupt in this context. And the maintenance interrupt is still required to use the vGIC. Thanks, M.
diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c index 9fd23f32aa54..5b06a9970a57 100644 --- a/arch/arm64/kvm/vgic/vgic-init.c +++ b/arch/arm64/kvm/vgic/vgic-init.c @@ -524,6 +524,16 @@ int kvm_vgic_hyp_init(void) if (!gic_kvm_info) return -ENODEV; + /* + * If we get one of these oddball non-GICs, taint the kernel, + * as we have no idea of how they *really* behave. + */ + if (gic_kvm_info->no_hw_deactivation) { + kvm_info("Non-architectural vgic, tainting kernel\n"); + add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK); + kvm_vgic_global_state.no_hw_deactivation = true; + } + switch (gic_kvm_info->type) { case GIC_V2: ret = vgic_v2_probe(gic_kvm_info); diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index ec621180ef09..e45b26e8d479 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -72,6 +72,9 @@ struct vgic_global { bool has_gicv4; bool has_gicv4_1; + /* Pseudo GICv3 from outer space */ + bool no_hw_deactivation; + /* GIC system register CPU interface */ struct static_key_false gicv3_cpuif; diff --git a/include/linux/irqchip/arm-vgic-info.h b/include/linux/irqchip/arm-vgic-info.h index 0319636be928..d39d0b591c5a 100644 --- a/include/linux/irqchip/arm-vgic-info.h +++ b/include/linux/irqchip/arm-vgic-info.h @@ -30,6 +30,8 @@ struct gic_kvm_info { bool has_v4; /* rvpeid support */ bool has_v4_1; + /* Deactivation impared, subpar stuff */ + bool no_hw_deactivation; }; #ifdef CONFIG_KVM
The vGIC, as architected by ARM, allows a virtual interrupt to trigger the deactivation of a physical interrupt. This allows the following interrupt to be delivered without requiring an exit. However, some implementations have choosen not to implement this, meaning that we will need some unsavoury workarounds to deal with this. On detecting such a case, taint the kernel and spit a nastygram. We'll deal with this in later patches. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/vgic/vgic-init.c | 10 ++++++++++ include/kvm/arm_vgic.h | 3 +++ include/linux/irqchip/arm-vgic-info.h | 2 ++ 3 files changed, 15 insertions(+)