Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 19 May 2022 08:59:57 +0100
Message-ID: <87ee0q54w2.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Vivek Kumar <quic_vivekuma@quicinc.com>
Cc:     <corbet@lwn.net>, <catalin.marinas@arm.com>, <will@kernel.org>,
        <tglx@linutronix.de>, <axboe@kernel.dk>, <rafael@kernel.org>,
        <akpm@linux-foundation.org>, <linux-doc@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-block@vger.kernel.org>, <linux-pm@vger.kernel.org>,
        <linux-mm@kvack.org>, <len.brown@intel.com>, <pavel@ucw.cz>,
        <paulmck@kernel.org>, <bp@suse.de>, <keescook@chromium.org>,
        <songmuchun@bytedance.com>, <rdunlap@infradead.org>,
        <damien.lemoal@opensource.wdc.com>, <pasha.tatashin@soleen.com>,
        <tabba@google.com>, <ardb@kernel.org>, <tsoni@quicinc.com>,
        <quic_psodagud@quicinc.com>, <quic_svaddagi@quicinc.com>,
        Prasanna Kumar <quic_kprasan@quicinc.com>
Subject: Re: [RFC 6/6] irqchip/gic-v3: Re-init GIC hardware upon hibernation restore
In-Reply-To: <1652860121-24092-7-git-send-email-quic_vivekuma@quicinc.com>
References: <1652860121-24092-1-git-send-email-quic_vivekuma@quicinc.com>
        <1652860121-24092-7-git-send-email-quic_vivekuma@quicinc.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

Hi Vivek,

On Wed, 18 May 2022 08:48:41 +0100,
Vivek Kumar <quic_vivekuma@quicinc.com> wrote:
> 
> Code added in this patch takes backup of different set of
> registers during hibernation suspend. On receiving hibernation
> restore callback, it restores register values from backup. This
> ensures state of hardware to be same just before hibernation and
> after restore.
> 
> Signed-off-by: Vivek Kumar <quic_vivekuma@quicinc.com>
> Signed-off-by: Prasanna Kumar <quic_kprasan@quicinc.com>
> ---
>  drivers/irqchip/irq-gic-v3.c | 138 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 136 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 2be8dea..442d32f 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -29,6 +29,10 @@
>  #include <asm/smp_plat.h>
>  #include <asm/virt.h>
>  
> +#include <linux/syscore_ops.h>
> +#include <linux/suspend.h>
> +#include <linux/notifier.h>
> +
>  #include "irq-gic-common.h"
>  
>  #define GICD_INT_NMI_PRI	(GICD_INT_DEF_PRI & ~0x80)
> @@ -56,6 +60,14 @@ struct gic_chip_data {
>  	bool			has_rss;
>  	unsigned int		ppi_nr;
>  	struct partition_desc	**ppi_descs;
> +#ifdef CONFIG_HIBERNATION
> +	unsigned int enabled_irqs[32];
> +	unsigned int active_irqs[32];
> +	unsigned int irq_edg_lvl[64];
> +	unsigned int ppi_edg_lvl;
> +	unsigned int enabled_sgis;
> +	unsigned int pending_sgis;
> +#endif

This is either way too much, or way too little. Just restoring these
registers is nowhere near enough, as you are completely ignoring the
ITS, so this will leave the machine broken for anything that requires
LPIs.

But If the bootloader is supposed to replace the kernel to put the HW
in a state where the GIC is usable again, why do we need any of this?

Hibernation relies on a basic promise: the secondary kernel is entered
with the HW in a reasonable state, and the basic infrastructure
(specially for stuff that can be only programmed once per boot such as
the RD tables) is already available. If the bootloader is going to do
the work of the initial kernel, then it must do it fully, and not
require this sort of random sprinkling all over the shop.

Effectively, there is an ABI between the primary kernel and the
secondary, and I don't see why this interface should change to paper
over what I see as the deficiencies of the bootloader.

Am I missing anything?

Thanks,

	M.

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