linux-kernel - Re: [PATCH] drivers: firmware: psci: add support for warm reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 3 Apr 2019 21:12:06 +0300
From:   Aaro Koskinen <aaro.koskinen@....fi>
To:     Mark Rutland <mark.rutland@....com>
Cc:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Aaro Koskinen <aaro.koskinen@...ia.com>
Subject: Re: [PATCH] drivers: firmware: psci: add support for warm reset

Hi,

On Wed, Apr 03, 2019 at 04:33:15AM +0100, Mark Rutland wrote:
> On Mon, Apr 01, 2019 at 09:14:43PM +0300, Aaro Koskinen wrote:
> > From: Aaro Koskinen <aaro.koskinen@...ia.com>
> > 
> > Add support for warm reset using SYSTEM_RESET2 introduced in PSCI
> > 1.1 specification.
> 
> For reference, how do you intend to use this?

The reboot mode is set using the kernel command line. One use case for warm
reset is pstore/ramoops.

> > @@ -256,6 +257,14 @@ static void psci_sys_reset(enum reboot_mode reboot_mode, const char *cmd)
> >  	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
> >  }
> >  
> > +static void psci_sys_reset2(enum reboot_mode reboot_mode, const char *cmd)
> > +{
> > +	if (reboot_mode == REBOOT_WARM)
> > +		invoke_psci_fn(psci_function_id[PSCI_FN_SYSTEM_RESET2], 0, 0, 0);
> > +	else
> > +		psci_sys_reset(reboot_mode, cmd);
> > +}
> 
> What happens when RESET2 isn't available, but the user requests s REBOOT_WARM
> reset? It looks like we'd invoke a bogus SMC.

This function is only called through arm_pm_restart. If the feature is
not available, then arm_pm_restart points to the default psci_sys_reset.

> Could we please add a preprocessor definition for SYSTEM_RESET_WARM so that
> it's clear that we're passing a parameter here?

OK, will add.

> >  /*
> >   * Probe function for PSCI firmware versions >= 0.2
> >   */
> > @@ -588,6 +611,8 @@ static int __init psci_probe(void)
> >  		psci_init_smccc();
> >  		psci_init_cpu_suspend();
> >  		psci_init_system_suspend();
> > +		if (PSCI_VERSION_MINOR(ver) >= 1)
> > +			psci_1_1_set_functions();
> >  	}
> 
> If we're going to add more things to this chain, can we please refactor this as:
> 
> 	if (ver < PSCI_VERSION(1, 0))
> 		return;
> 
> 	/* PSCI 1.0 setup here */
> 
> 	if (ver < PSCI_VERSION(1,1))
> 		return;
> 
> 	/* PSCI 1.1 setup here */
> 
> ... as that saves on indentation and is easier to extend in future.

OK, makes sense, I will do that.

Thanks,

A.