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

[Marc Zyngier <maz@kernel.org>]

On 24/09/2019 12:00, Andrew Murray wrote:
> 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.

But it definitely *should* fire. RVPEID+!VLPI is terminally broken.
What's the use of RVPEID if you cannot directly inject anything? To me
blunt: any difference of HW configuration for any redistributor is an
error. They should all be identical (no, I'm not planning to deal with
the GIC equivalent OF BL).

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

There is no use case. Such a configuration shouldn't exist. I'm
considering calling panic instead.

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