Date:   Wed, 15 Feb 2023 12:10:50 +0000
From:   Sudeep Holla <sudeep.holla@arm.com>
To:     Marc Zyngier <maz@kernel.org>
Cc:     Florian Fainelli <f.fainelli@gmail.com>,
        Sudeep Holla <sudeep.holla@arm.com>,
        linux-arm-kernel@lists.infradead.org,
        Thomas Gleixner <tglx@linutronix.de>,
        Oliver Upton <oliver.upton@linux.dev>,
        "open list:IRQCHIP DRIVERS" <linux-kernel@vger.kernel.org>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@broadcom.com>
Subject: Re: [PATCH 3/3] irqchip/gic-v3: Save and restore distributor and
 re-distributor
Message-ID: <20230215121050.d57tnfh7wzpyqzti@bogus>
References: <20230214233426.2994501-1-f.fainelli@gmail.com>
 <20230214233426.2994501-4-f.fainelli@gmail.com>
 <87o7pvz78z.wl-maz@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87o7pvz78z.wl-maz@kernel.org>
Precedence: bulk

On Wed, Feb 15, 2023 at 08:02:20AM +0000, Marc Zyngier wrote:
> On Tue, 14 Feb 2023 23:34:26 +0000,
> Florian Fainelli <f.fainelli@gmail.com> wrote:
> >
> > On platforms implementing Suspend to RAM where the GIC loses power, we
> > are not properly saving and restoring the GIC distributor and
> > re-distributor registers thus leading to the system resuming without any
> > functional interrupts.
>
> The real question is *why* we need any of this. On any decent system,
> this is the firmware's job.  It was *never* the OS GIC driver's job
> the first place.
>

Completely agreed on the points you have made here, no disagreement.
However I would like to iterate some of the arguments/concerns the
firmware teams I have interacted in the past have made around this.
And this is while ago(couple of years) and they may have different
views. I am repeating them as I think it may be still valid on some
systems so that we can make some suggestions if we have here.

> Importantly, the OS cannot save the full state: a large part of it is
> only accessible via secure, and Linux doesn't run in secure mode. How
> do you restore the group configuration, for example? Oh wait, you
> don't even save it.
>

Agreed, we can't manage secure side configurations. But one of the concern
was about the large memory footprint to save the larger non-secure GIC
context in the smaller secure memory.

One of the suggestion at the time was to carve out a chunk of non-secure
memory and let the secure side use the same for context save and restore.
Not sure if this was tried out especially for the GIC. I may need to
chase that with the concerned teams.

Thanks Florian for starting this thread and sorry that I couldn't recollect
lots of the information when we chatted in the private about this. Marc
response triggered all the memory back.

> So unless you have a single security state system, this cannot
> work. And apart from VMs (which by the way do not need any of this),
> there is no GICv3-based system without EL3. If you know of one, please
> let me know. And if it existed, then all the save/restore should
> happen only when GICD_CTLR.DS==1.
>

Yes, now I remember the discussion we had probably almost 9-10 years
back when I first added the CPU PM notifiers for GICv3. I am sure we
would have discussed this at-least couple of times after that. Yet I
just got carried away by the fact that GICv2 does the save/restore and
this should also be possible. Sorry for that.

--
Regards,
Sudeep