Re: [PATCH v7 1/3] irqchip/gic-v3-its: add ability to save/restore ITS state

[dbasehore .]

On Wed, Mar 14, 2018 at 2:44 PM, dbasehore . <dbasehore@xxxxxxxxxxxx> wrote:
> On Wed, Mar 14, 2018 at 3:22 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
>> On 02/03/18 02:08, dbasehore . wrote:
>>> On Thu, Mar 1, 2018 at 4:29 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
>>>> Hi Mark,
>>>>
>>>> On 01/03/18 11:41, Mark Rutland wrote:
>>>>> On Wed, Feb 28, 2018 at 09:48:18PM -0800, Derek Basehore wrote:
>>>>>> Some platforms power off GIC logic in suspend, so we need to
>>>>>> save/restore state. The distributor and redistributor registers need
>>>>>> to be handled in platform code due to access permissions on those
>>>>>> registers, but the ITS registers can be restored in the kernel.
>>>>>>
>>>>>> Signed-off-by: Derek Basehore <dbasehore@xxxxxxxxxxxx>
>>>>>
>>>>> How much state do we have to save/restore?
>>>>>
>>>>> Given we can apparently read all this state, couldn't we *always* save
>>>>> the state, then upon resume detect if the state has been lost, restoring
>>>>> it if so?
>>>>>
>>>>> That way, we don't need a property in FW tables for DT or ACPI.
>>>>
>>>> That's a good point. I guess that we could just compare the saved
>>>> GITS_CTLR register and restore the full state only if the ITS comes back
>>>> as disabled.
>>>>
>>>> I'm just a bit worried that it makes it an implicit convention between
>>>> kernel an FW, which could change in funny ways. Importantly, the PSCI
>>>> spec says states FW should restore *the whole state*. Obviously, it
>>>> cannot to that on HW that doesn't allow you to read out the state, hence
>>>> the DT flag that outlines the departure from the expected behaviour.
>>>>
>>>> I'm happy to go either way, but then I have the feeling that we should
>>>> go back to quirking it on the actual implementation (GIC500 in this
>>>> case) if we're to from the property.
>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>> @@ -3261,6 +3363,9 @@ static int __init its_probe_one(struct resource *res,
>>>>>>              ctlr |= GITS_CTLR_ImDe;
>>>>>>      writel_relaxed(ctlr, its->base + GITS_CTLR);
>>>>>>
>>>>>> +    if (fwnode_property_present(handle, "reset-on-suspend"))
>>>>>> +            its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
>>>>>
>>>>> Does this allow this property on an ACPI system?
>>>>>
>>>>> If we need this on ACPI, we need a spec update to handle this properly,
>>>>> and shouldn't use device properties like this.
>>>>
>>>> Well spotted. I guess that dropping the property would fix that
>>>> altogether, assuming we feel that the above is safe enough.
>>>>
>>>> Thoughts?
>>>
>>> I'm fine changing it to get rid of the devicetree property.
>>>
>>> What's the reason for quirking the behavior though? It's not that much
>>> code + data and nothing else relies on the state of the ITS getting
>>> disabled across suspend/resume. Even if something did, we'd have to
>>> resolve it with this feature anyways.
>>
>> The reason we do this is to cope with GIC500 having the collection state
>> in registers instead of memory. If we didn't have this extraordinary
>> misfeature, FW could do a full save/restore of the ITS, and we wouldn't
>> have to do anything (which is what the driver currently expects).
>>
>> A middle ground approach is to limit the feature to systems where
>> GITS_TYPER.HCC is non-zero instead of limiting it to GIC500. Pretty easy
>> to fix. This should have the same effect, as GIC500 is the only
>> implementation I'm aware of with HCC!=0.
>>
>> Given that we're already at -rc5 and that I'd like to queue things for
>> 4.17, I've made this change myself and queued patches 1 and 3 here[1].
>>
>> Can you please have a look at let me know if that works for you?
>>
>
> Assuming that your fine with only having the GIC500 implementations
> that have HCC as non-zero getting ITS registers restored in the
> kernel. As far as I can tell, this can happen in firmware for all
> implementations. It's only the code to resend that MAPC on resume that
> needs to be in the kernel.

Actually, I guess we can't tell if we need to resend MAPC on resume if
the firmware restores the state. Assuming it's a problem to just
resend MAPC all the time, only restoring the ITS registers in the
kernel when HCC != 0 makes sense.

>
>> Thanks,
>>
>>         M.
>>
>> [1] git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
>> irq/irqchip-next
>> --
>> Jazz is not dead. It just smells funny...