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

Marc Zyngier
2017-10-18T16:03:40Z

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.

> 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!

M.
--
Jazz is not dead, it just smell funny.