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

From: Christoffer Dall <cdall@linaro.org>
To: Marc Zyngier <marc.zyngier@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org
Subject: Re: [PATCH v3 06/20] KVM: arm/arm64: Check that system supports split eoi/deactivate
Date: Wed, 18 Oct 2017 21:16:38 +0200	[thread overview]
Message-ID: <20171018191638.GJ8900@cbox> (raw)
In-Reply-To: <874lqwjvdv.fsf@on-the-bus.cambridge.arm.com>

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