Date:   Wed, 15 Feb 2023 08:02:20 +0000
Message-ID: <87o7pvz78z.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Florian Fainelli <f.fainelli@gmail.com>
Cc:     linux-arm-kernel@lists.infradead.org,
        Thomas Gleixner <tglx@linutronix.de>,
        Oliver Upton <oliver.upton@linux.dev>,
        linux-kernel@vger.kernel.org (open list:IRQCHIP DRIVERS),
        Sudeep Holla <sudeep.holla@arm.com>,
        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: <20230214233426.2994501-4-f.fainelli@gmail.com>
References: <20230214233426.2994501-1-f.fainelli@gmail.com>     <20230214233426.2994501-4-f.fainelli@gmail.com>
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/27.1
 (x86_64-pc-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 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.

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.

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.

To conclude, this patch doesn't do what it advertises, because it
*cannot* do it, by definition. The secure firmware is the only place
where this can be done.

	M.

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