linux-kernel - RE: [PATCH 2/7] tboot: Add return values for tboot

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9F57BF860713DF4BA3EFA4F8C6DFEDAC2D6870EE@ORSMSX101.amr.corp.intel.com>
Date:	Thu, 26 Jan 2012 00:16:39 +0000
From:	"Cihula, Joseph" <joseph.cihula@...el.com>
To:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"rjw@...k.pl" <rjw@...k.pl>, "x86@...nel.org" <x86@...nel.org>,
	"Brown, Len" <len.brown@...el.com>,
	"linux-pm@...ts.linux-foundation.org" 
	<linux-pm@...ts.linux-foundation.org>,
	"tboot-devel@...ts.sourceforge.net" 
	<tboot-devel@...ts.sourceforge.net>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"liang.tang@...cle.com" <liang.tang@...cle.com>,
	"hpa@...or.com" <hpa@...or.com>
Subject: RE: [PATCH 2/7] tboot: Add return values for tboot_sleep

> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@...cle.com]
> Sent: Thursday, January 12, 2012 12:23 PM
eturn values for tboot_sleep
> 
> On Thu, Jan 12, 2012 at 09:49:58AM -0800, Konrad Rzeszutek Wilk wrote:
> > On Tue, Jan 10, 2012 at 08:10:53PM +0000, Cihula, Joseph wrote:
> > > ACK, but tboot_sleep() calls tboot_shutdown() and there are error conditions in that which you
> are not reflecting upward--i.e. you should make tboot_shutdown() return an 'int' and propagate its
> errors through tboot_sleep().
> >
> > Hey Joe,
> >
> > Thanks for looking at the patches.
> >
> > Right now [with this patch applied] the code looks as so:
> >
> > 297
> > 298         tboot_shutdown(acpi_shutdown_map[sleep_state]);
> > 299         return 0;
> > 300 }
> >
> >
> > If we do make tboot_shutdown() return an int, there will be a need to
> > modify other callers: native_machine_emergency_restart,
> > native_machine_halt, native_machine_power_off, and native_play_dead as well.
> >
> > Perhaps that could be done in another patch and leave this one as is
> > where the 'tboot_sleep' would just return 0 instead of 'return
> > tboot_shutdown(...)' ?
> >
> > Perhaps another way to do this is to extract the common code of
> > tboot_sleep and tboot_shutdown?

I'd be happier to see the callers above changed to handle a tboot_shutdown() that returned an error than splitting this up and only checking the error in one path.

Joe

> 
> Like this (not yet tested):
> 
> diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c index 6410744..2f93a50 100644
> --- a/arch/x86/kernel/tboot.c
> +++ b/arch/x86/kernel/tboot.c
> @@ -212,17 +212,16 @@ static int tboot_setup_sleep(void)  {
>  	/* S3 shutdown requested, but S3 not supported by the kernel... */
>  	BUG();
> -	return -1;
> +	return -ENOSYS;
>  }
> 
>  #endif
> 
> -void tboot_shutdown(u32 shutdown_type)
> -{
> -	void (*shutdown)(void);
> +static int tboot_check(u32 shutdown_type) {
> +
> 
>  	if (!tboot_enabled())
> -		return;
> +		return -ENODEV;
> 
>  	/*
>  	 * if we're being called before the 1:1 mapping is set up then just @@ -230,12 +229,21 @@
> void tboot_shutdown(u32 shutdown_type)
>  	 * due to very early panic()
>  	 */
>  	if (!tboot_pg_dir)
> -		return;
> +		return -EBUSY;
> 
>  	/* if this is S3 then set regions to MAC */
> -	if (shutdown_type == TB_SHUTDOWN_S3)
> -		if (tboot_setup_sleep())
> -			return;
> +	if (shutdown_type == TB_SHUTDOWN_S3) {
> +		int err = tboot_setup_sleep();
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static void __tboot_shutdown(u32 shutdown_type) {
> +	void (*shutdown)(void);
> 
>  	tboot->shutdown_type = shutdown_type;
> 
> @@ -248,6 +256,13 @@ void tboot_shutdown(u32 shutdown_type)
>  	while (1)
>  		halt();
>  }
> +void tboot_shutdown(u32 shutdown_type)
> +{
> +	if (!tboot_check(shutdown_type))
> +		return;
> +
> +	__tboot_shutdown(shutdown_type);
> +}
> 
>  static void tboot_copy_fadt(const struct acpi_table_fadt *fadt)  { @@ -279,9 +294,17 @@ static
> int tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
>  		/* S3: */ TB_SHUTDOWN_S3,
>  		/* S4: */ TB_SHUTDOWN_S4,
>  		/* S5: */ TB_SHUTDOWN_S5 };
> +	int err;
> 
> -	if (!tboot_enabled())
> -		return 0;
> +	if (sleep_state >= ACPI_S_STATE_COUNT ||
> +	    acpi_shutdown_map[sleep_state] == -1) {
> +		pr_warning("unsupported sleep state 0x%x\n", sleep_state);
> +		return -ENOSYS;
> +	}
> +
> +	err = tboot_check(acpi_shutdown_map[sleep_state]);
> +	if (!err && err != EBUSY)
> +		return err;
> 
>  	tboot_copy_fadt(&acpi_gbl_FADT);
>  	tboot->acpi_sinfo.pm1a_cnt_val = pm1a_control; @@ -289,13 +312,7 @@ static int
> tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
>  	/* we always use the 32b wakeup vector */
>  	tboot->acpi_sinfo.vector_width = 32;
> 
> -	if (sleep_state >= ACPI_S_STATE_COUNT ||
> -	    acpi_shutdown_map[sleep_state] == -1) {
> -		pr_warning("unsupported sleep state 0x%x\n", sleep_state);
> -		return -1;
> -	}
> -
> -	tboot_shutdown(acpi_shutdown_map[sleep_state]);
> +	__tboot_shutdown((acpi_shutdown_map[sleep_state]));
>  	return 0;
>  }
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/