Date:   Wed, 15 Feb 2023 14:40:04 +0000
Message-ID: <86ttznxa9n.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Sudeep Holla <sudeep.holla@arm.com>
Cc:     Florian Fainelli <f.fainelli@gmail.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
In-Reply-To: <20230215121050.d57tnfh7wzpyqzti@bogus>
References: <20230214233426.2994501-1-f.fainelli@gmail.com>
        <20230214233426.2994501-4-f.fainelli@gmail.com>
        <87o7pvz78z.wl-maz@kernel.org>
        <20230215121050.d57tnfh7wzpyqzti@bogus>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 15 Feb 2023 12:10:50 +0000,
Sudeep Holla <sudeep.holla@arm.com> wrote:
> 
> 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.

The main issue is that you still need secure memory to save the secure
state, as leaving it in NS memory would be an interesting attack
vector! Other than that, I see no issue with FW carving out the memory
it needs to save/restore the NS state of the GIC.

Note that this isn't only the (re-)distributor(s) PPI/SPI registers.
The LPI setup must also be saved, and that includes all the ITS
registers. I'm surprised the FW folks are, all of a sudden,
discovering this requirements. It isn't like the GIC architecture is a
novelty, and they have had ample time to review the spec...

> 
> 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.

GICv2 is just as fsck'd. It is just that we pretend it works for the
sake of 32bit that may run in secure mode. On a 64bit machine, or in a
NS setup, it is doomed for the same reasons. There really isn't any
substitute for secure firmware here.

	M.

-- 
Without deviation from the norm, progress is not possible.