Re: [PATCH 04/35] irqchip/gic-v3: Detect GICv4.1 supporting RVPEID

[Andrew Murray <andrew.murray@arm.com>]

On Tue, Sep 24, 2019 at 11:49:24AM +0100, Marc Zyngier wrote:
> On 24/09/2019 11:24, Andrew Murray wrote:
> > On Mon, Sep 23, 2019 at 07:25:35PM +0100, Marc Zyngier wrote:
> >> GICv4.1 supports the RVPEID ("Residency per vPE ID"), which allows for
> >> a much efficient way of making virtual CPUs resident (to allow direct
> >> injection of interrupts).
> >>
> >> The functionnality needs to be discovered on each and every redistributor
> >> in the system, and disabled if the settings are inconsistent.
> >>
> >> Signed-off-by: Marc Zyngier <maz@kernel.org>
> >> ---
> >>  drivers/irqchip/irq-gic-v3.c       | 21 ++++++++++++++++++---
> >>  include/linux/irqchip/arm-gic-v3.h |  2 ++
> >>  2 files changed, 20 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> >> index 422664ac5f53..0b545e2c3498 100644
> >> --- a/drivers/irqchip/irq-gic-v3.c
> >> +++ b/drivers/irqchip/irq-gic-v3.c
> >> @@ -849,8 +849,21 @@ static int __gic_update_rdist_properties(struct redist_region *region,
> >>  					 void __iomem *ptr)
> >>  {
> >>  	u64 typer = gic_read_typer(ptr + GICR_TYPER);
> >> +
> >>  	gic_data.rdists.has_vlpis &= !!(typer & GICR_TYPER_VLPIS);
> >> -	gic_data.rdists.has_direct_lpi &= !!(typer & GICR_TYPER_DirectLPIS);
> >> +
> >> +	/* RVPEID implies some form of DirectLPI, no matter what the doc says... :-/ */
> > 
> > I think the doc says, RVPEID is *always* 1 for GICv4.1 (and presumably beyond)
> > and when RVPEID==1 then DirectLPI is *always* 0 - but that's OK because for
> > GICv4.1 support for direct LPIs is mandatory.
> 
> Well, v4.1 support for DirectLPI is pretty patchy. It has just enough
> features to make it useful.
> 
> > 
> >> +	gic_data.rdists.has_rvpeid &= !!(typer & GICR_TYPER_RVPEID);
> >> +	gic_data.rdists.has_direct_lpi &= (!!(typer & GICR_TYPER_DirectLPIS) |
> >> +					   gic_data.rdists.has_rvpeid);
> >> +
> >> +	/* Detect non-sensical configurations */
> >> +	if (WARN_ON_ONCE(gic_data.rdists.has_rvpeid && !gic_data.rdists.has_vlpis)) {
> > 
> > How feasible is the following suitation? All the redistributors in the system has
> > vlpis=0, and only the first redistributor has rvpeid=1 (with the remaining ones
> > rvpeid=0).If we evaluate this WARN_ON_ONCE on each call to
> > __gic_update_rdist_properties we end up without direct LPI support, however if we
> > evaluated this after iterating through all the redistributors then we'd end up
> > with direct LPI support and a non-essential WARN.
> > 
> > Should we do the WARN after iterating through all the redistributors once we
> > know what the final values of these flags will be, perhaps in
> > gic_update_rdist_properties?
> 
> What does it gains us?

It prevents an unnecessary WARN.

If the first redistributor has rvpeid=1, vlpis=0, direct_lpi=1, and the others
have rvpeid=0, vlpis=0, direct_lpi=0. At the end of iteration, without the
WARN if statement, you end up wth rvpeid=0, vlpis=0, direct_lpi=0. I.e. it's
done the right thing. In this use-case the WARN doesn't achieve anything other
than give the user a pointless WARN. If the WARN was moved to after iteration
then the WARN wouldn't fire.

I have no idea how likely this use-case is.

> The moment we've detected any inconsistency, any
> use of DirectLPI or VLPIs is a big nono, because the redistributors care
> not designed to communicate with each other, and we might as well do
> that early. Frankly, the HW should have stayed in someone's lab. The
> only reason I have that code in is to detect the FVP model being
> misconfigured, which is pretty easy to do

Thanks,

Andrew Murray

> 
> 	M.
> -- 
> Jazz is not dead, it just smells funny...