Re: [PATCH 3/3] irqchip/gic-v3: Save and restore distributor and re-distributor

[Marc Zyngier]

On Wed, 15 Feb 2023 15:10:48 +0000,
Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
> 
> 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@xxxxxxx> 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@xxxxxxxxx> 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.
> >
> 
> Yes I meant NS memory for only NS state of 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...
> >
> 
> I understand your concern about late realisation 😄.
> 
> 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.

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