Date:   Thu, 16 Feb 2023 08:31:06 +0000
Message-ID: <86r0uqxb91.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: <20230215151048.xxmpvfre2xdngowr@bogus>
References: <20230214233426.2994501-1-f.fainelli@gmail.com>
        <20230214233426.2994501-4-f.fainelli@gmail.com>
        <87o7pvz78z.wl-maz@kernel.org>
        <20230215121050.d57tnfh7wzpyqzti@bogus>
        <86ttznxa9n.wl-maz@kernel.org>
        <20230215151048.xxmpvfre2xdngowr@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=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, 15 Feb 2023 15:10:48 +0000,
Sudeep Holla <sudeep.holla@arm.com> wrote:
>=20
> On Wed, Feb 15, 2023 at 02:40:04PM +0000, Marc Zyngier wrote:
> > On Wed, 15 Feb 2023 12:10:50 +0000,
> > Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >=20
> > > 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 powe=
r, we
> > > > > are not properly saving and restoring the GIC distributor and
> > > > > re-distributor registers thus leading to the system resuming with=
out any
> > > > > functional interrupts.
> > > >
> > > > The real question is *why* we need any of this. On any decent syste=
m,
> > > > this is the firmware's job.  It was *never* the OS GIC driver's job
> > > > the first place.
> > > >
> > >=20
> > > 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.
> > >=20
> > > > 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. H=
ow
> > > > do you restore the group configuration, for example? Oh wait, you
> > > > don't even save it.
> > > >
> > >=20
> > > Agreed, we can't manage secure side configurations. But one of the co=
ncern
> > > was about the large memory footprint to save the larger non-secure GIC
> > > context in the smaller secure memory.
> > >=20
> > > One of the suggestion at the time was to carve out a chunk of non-sec=
ure
> > > memory and let the secure side use the same for context save and rest=
ore.
> > > Not sure if this was tried out especially for the GIC. I may need to
> > > chase that with the concerned teams.
> >=20
> > 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.
> >
>=20
> Yes I meant NS memory for only NS state of GIC.
>=20
> > 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...
> >
>=20
> I understand your concern about late realisation =F0=9F=98=84.
>=20
> Another issue in general I see with reference firmware stack(like
> Trusted Firmware in this case) is that the requirements are driven from
> the reference platforms which may not have this GIC save/restore
> requirement as they are in always on domain and it is then made platform
> specific problem in that project which may not be ideal and may result
> in somewhat misleading indirectly other firmware developers using
> it.

Yeah, that's the usual state of affair. Unrealistic platforms, no
insight (and more generally no interest) in the actual usage model.

Still, most people got it right, so I guess they must be reading the
spec. How comes this was never picked from contributions to TF-A?
Surely duplication of platform code should be a massive hint to the
firmware maintainers?

Thanks,

	M.

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