Re: [PATCH v3 06/20] KVM: arm/arm64: Check that system supports split eoi/deactivate

Christoffer Dall
2017-10-18T19:15:36Z

On Wed, Oct 18, 2017 at 05:03:40PM +0100, Marc Zyngier wrote:
> On Wed, Oct 18 2017 at 3:41:45 pm BST, Christoffer Dall <cdall@linaro.org> wrote:
> > On Mon, Oct 09, 2017 at 05:47:18PM +0100, Marc Zyngier wrote:
> >> On 23/09/17 01:41, Christoffer Dall wrote:
> >> > Some systems without proper firmware and/or hardware description data
> >> > don't support the split EOI and deactivate operation.
> >> >
> >> > On such systems, we cannot leave the physical interrupt active after the
> >> > timer handler on the host has run, so we cannot support KVM with an
> >> > in-kernel GIC with the timer changes we are about to introduce.
> >> >
> >> > This patch makes sure that trying to initialize the KVM GIC code will
> >> > fail on such systems.
> >> >
> >> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> >> > Signed-off-by: Christoffer Dall <cdall@linaro.org>
> >> > ---
> >> > drivers/irqchip/irq-gic.c | 3 ++-
> >> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> >> > index f641e8e..ab12bf4 100644
> >> > --- a/drivers/irqchip/irq-gic.c
> >> > +++ b/drivers/irqchip/irq-gic.c
> >> > @@ -1420,7 +1420,8 @@ static void __init gic_of_setup_kvm_info(struct device_node *node)
> >> > if (ret)
> >> > return;
> >> >
> >> > - gic_set_kvm_info(&gic_v2_kvm_info);
> >> > + if (static_key_true(&supports_deactivate))
> >> > + gic_set_kvm_info(&gic_v2_kvm_info);
> >> > }
> >> >
> >> > int __init
> >> >
> >>
> >> Should we add the same level of checking on the ACPI path, just for the
> >> sake symmetry?
> >
> > Yes, we should, if anyone is crazy enough to use ACPI :)
>
> Sadly, the madness is becoming commonplace.
>
> >>
> >> Also, do we need to add the same thing for GICv3?
> >>
> >
> > Why would split EOI/deactivate not be available on GICv3, actually? It
> > looks like this is not supported unless you have EL2, but I can't seem
> > to find anything in the spec for this, and KVM should support
> > EOI/deactivate for GICv3 guests I think. Am I missing something?
>
> No, you're not. This is just a Linux choice (or rather mine) not to use
> EOImode=1 in guests (or anything booted at EL1), as we don't really need
> the two-stage deactivate in that situation (it is pure overhead).
>
> I'm just worried of potentially broken HW, and would like to make sure
> that when we force EOImode=0 on these systems, we truly tell KVM about
> it.
>

Yes, makes sense, it's also more cosistent that way.

> > Assuming I'm wrong about GICv3, which I probably am, how does this look
> > (on top of the posted patch):
> >
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 519149e..aed524c 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -1228,7 +1228,9 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
> > goto out_unmap_rdist;
> >
> > gic_populate_ppi_partitions(node);
> > - gic_of_setup_kvm_info(node);
> > +
> > + if (static_key_true(&supports_deactivate))
> > + gic_of_setup_kvm_info(node);
> > return 0;
> >
> > out_unmap_rdist:
> > @@ -1517,7 +1519,9 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
> > goto out_fwhandle_free;
> >
> > acpi_set_irq_model(ACPI_IRQ_MODEL_GIC, domain_handle);
> > - gic_acpi_setup_kvm_info();
> > +
> > + if (static_key_true(&supports_deactivate))
> > + gic_acpi_setup_kvm_info();
> >
> > return 0;
> >
> > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> > index ab12bf4..121af5c 100644
> > --- a/drivers/irqchip/irq-gic.c
> > +++ b/drivers/irqchip/irq-gic.c
> > @@ -1653,7 +1653,8 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
> > if (IS_ENABLED(CONFIG_ARM_GIC_V2M))
> > gicv2m_init(NULL, gic_data[0].domain);
> >
> > - gic_acpi_setup_kvm_info();
> > + if (static_key_true(&supports_deactivate))
> > + gic_acpi_setup_kvm_info();
> >
> > return 0;
> > }
>
> Yup, looks good to me!
>

Thanks,
-Christoffer