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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 1 Mar 2018 18:08:31 -0800
From:   "dbasehore ." <dbasehore@...omium.org>
To:     Marc Zyngier <marc.zyngier@....com>
Cc:     Mark Rutland <mark.rutland@....com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Soby Mathew <Soby.Mathew@....com>,
        Sudeep Holla <sudeep.holla@....com>,
        devicetree@...r.kernel.org, robh+dt@...nel.org,
        Linux-pm mailing list <linux-pm@...r.kernel.org>,
        "Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Brian Norris <briannorris@...omium.org>
Subject: Re: [PATCH v7 1/3] irqchip/gic-v3-its: add ability to save/restore
 ITS state

On Thu, Mar 1, 2018 at 4:29 AM, Marc Zyngier <marc.zyngier@....com> 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@...omium.org>
>>
>> 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.

>
>         M.
> --
> Jazz is not dead. It just smells funny...