[Skiboot-stable] [PATCH v7 2/3] platforms/astbmc: Check for SBE validation step

Andrew Jeffery andrew at aj.id.au
Thu May 9 14:59:40 AEST 2019



On Thu, 9 May 2019, at 12:38, Samuel Mendoza-Jonas wrote:
> On some POWER8 astbmc systems an update to the SBE requires pausing at
> runtime to ensure integrity of the SBE. If this is required the BMC will
> set a chassis boot option IPMI flag using the OEM parameter 0x62. If
> Skiboot sees this flag is set it waits until the SBE update is complete
> and the flag is cleared.
> Unfortunately the mystery operation that validates the SBE also leaves
> it in a bad state and unable to be used for timer operations. To
> workaround this the flag is checked as soon as possible (ie. when IPMI
> and the console are set up), and once complete the system is rebooted.
> 
> Signed-off-by: Samuel Mendoza-Jonas <sam at mendozajonas.com>

Reviewed-by: Andrew Jeffery <andrew at aj.id.au>

> ---
> v7: Cleanup ipmi_sys_boot_opt in ipmi_get_chassis_boot_opt_request()
> v6: Improve some error paths in ipmi-info, check for ipmi errors in
> common.c and warn the user if things are taking too long
> v5: Check resp_size in ipmi-info and check the SBE flag less frequently
> v4: As explained above and in doc/bmc the operation breaks the SBE and
> gets the SLW timer stuck. Move the check as early as possible and
> reboot once complete to fix things.
> v3: Check location moved to just before kernel booting; otherwise the
> kernel image appears to be malformed somehow and we immediately reboot.
> v2: IPMI response format updated.
> This reflects functionality that will appear in new versions of AMI's
> and SMC's BMC implementations. The format of the IPMI response has been
> confirmed and testing is ongoing with both parties to verify the
> behaviour.
> 
>  core/init.c                 |   8 +++
>  hw/ipmi/ipmi-info.c         | 109 +++++++++++++++++++++++++++++++++++-
>  include/ipmi.h              |   7 +++
>  include/platform.h          |   5 ++
>  platforms/astbmc/astbmc.h   |   1 +
>  platforms/astbmc/common.c   |  64 +++++++++++++++++++++
>  platforms/astbmc/garrison.c |   1 +
>  platforms/astbmc/habanero.c |   1 +
>  platforms/astbmc/p8dnu.c    |   1 +
>  platforms/astbmc/p8dtu.c    |   2 +
>  10 files changed, 196 insertions(+), 3 deletions(-)
> 
> diff --git a/core/init.c b/core/init.c
> index 955d299d..bca12dfc 100644
> --- a/core/init.c
> +++ b/core/init.c
> @@ -1190,6 +1190,14 @@ void __noreturn __nomcount main_cpu_entry(const 
> void *fdt)
>  	/* Install the OPAL Console handlers */
>  	init_opal_console();
>  
> +	/*
> +	 * Some platforms set a flag to wait for SBE validation to be
> +	 * performed by the BMC. If this occurs it leaves the SBE in a
> +	 * bad state and the system will reboot at this point.
> +	 */
> +	if (platform.seeprom_update)
> +		platform.seeprom_update();
> +
>  	/* Init SLW related stuff, including fastsleep */
>  	slw_init();
>  
> diff --git a/hw/ipmi/ipmi-info.c b/hw/ipmi/ipmi-info.c
> index 56370037..be2bd442 100644
> --- a/hw/ipmi/ipmi-info.c
> +++ b/hw/ipmi/ipmi-info.c
> @@ -23,7 +23,7 @@
>  #include <timebase.h>
>  
>  /*
> - * Respones data from IPMI Get device ID command (As defined in
> + * Response data from IPMI Get device ID command (As defined in
>   * Section 20.1 Get Device ID Command - IPMI standard spec).
>   */
>  struct ipmi_dev_id {
> @@ -39,9 +39,27 @@ struct ipmi_dev_id {
>  };
>  static struct ipmi_dev_id *ipmi_dev_id;
>  
> +/*
> + * Response data from IPMI Chassis Get System Boot Option (As defined in
> + * Section 28.13 Get System Boot Options Command - IPMI standard spec).
> + */
> +struct ipmi_sys_boot_opt {
> +	uint8_t param_version;
> +	uint8_t param_valid;
> +	/*
> +	 * Fields for OEM parameter 0x62. This parameter does not follow
> +	 * the normal layout and just has a single byte to signal if it
> +	 * is active or not.
> +	 */
> +	uint8_t flag_set;
> +};
> +static struct ipmi_sys_boot_opt *ipmi_sys_boot_opt;
> +
>  /* Got response from BMC? */
>  static bool bmc_info_waiting = false;
>  static bool bmc_info_valid = false;
> +static bool bmc_boot_opt_waiting = false;
> +static bool bmc_boot_opt_valid = false;
>  
>  /* This will free ipmi_dev_id structure */
>  void ipmi_dt_add_bmc_info(void)
> @@ -79,8 +97,14 @@ static void ipmi_get_bmc_info_resp(struct ipmi_msg *msg)
>  		return;
>  	}
>  
> -	bmc_info_valid = true;
> -	memcpy(ipmi_dev_id, msg->data, msg->resp_size);
> +	/* ipmi_dev_id has optional fields */
> +	if (msg->resp_size <= sizeof(struct ipmi_dev_id)) {
> +		bmc_info_valid = true;
> +		memcpy(ipmi_dev_id, msg->data, msg->resp_size);
> +	} else {
> +		prlog(PR_WARNING, "IPMI: IPMI_BMC_GET_DEVICE_ID unexpected response size\n");
> +	}
> +
>  	ipmi_free_msg(msg);
>  }
>  
> @@ -110,3 +134,82 @@ int ipmi_get_bmc_info_request(void)
>  	bmc_info_waiting = true;
>  	return rc;
>  }
> +
> +/* This will free ipmi_sys_boot_opt structure */
> +int ipmi_chassis_check_sbe_validation(void)
> +{
> +	int rc = -1;
> +
> +	while (bmc_boot_opt_waiting)
> +		time_wait_ms(10);
> +
> +	if (!bmc_boot_opt_valid)
> +		goto out;
> +
> +	if ((ipmi_sys_boot_opt->param_valid & 0x8) != 0)
> +		goto out;
> +	if (ipmi_sys_boot_opt->param_valid != 0x62)
> +		goto out;
> +
> +	rc = ipmi_sys_boot_opt->flag_set;
> +
> +out:
> +	free(ipmi_sys_boot_opt);
> +	return rc;
> +}
> +
> +static void ipmi_get_chassis_boot_opt_resp(struct ipmi_msg *msg)
> +{
> +	bmc_boot_opt_waiting = false;
> +
> +	if (msg->cc != IPMI_CC_NO_ERROR) {
> +		prlog(PR_INFO, "IPMI: IPMI_CHASSIS_GET_BOOT_OPT cmd returned error"
> +		      " [rc : 0x%x]\n", msg->data[0]);
> +		ipmi_free_msg(msg);
> +		return;
> +	}
> +
> +	if (msg->resp_size == sizeof(struct ipmi_sys_boot_opt)) {
> +		bmc_boot_opt_valid = true;
> +		memcpy(ipmi_sys_boot_opt, msg->data, msg->resp_size);
> +	} else {
> +		prlog(PR_WARNING, "IPMI: IPMI_CHASSIS_GET_BOOT_OPT unexpected 
> response size\n");
> +	}
> +
> +	ipmi_free_msg(msg);
> +}
> +
> +int ipmi_get_chassis_boot_opt_request(void)
> +{
> +	int rc;
> +	struct ipmi_msg *msg;
> +	uint8_t req[] = {
> +		0x62, /* OEM parameter (SBE Validation on astbmc) */
> +		0x00, /* no set selector */
> +		0x00, /* no block selector */
> +	};
> +
> +	ipmi_sys_boot_opt = zalloc(sizeof(struct ipmi_sys_boot_opt));
> +	assert(ipmi_sys_boot_opt);
> +
> +	msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE, IPMI_CHASSIS_GET_BOOT_OPT,
> +			 ipmi_get_chassis_boot_opt_resp, NULL, req,
> +			 sizeof(req), sizeof(struct ipmi_sys_boot_opt));
> +	if (!msg) {
> +		free(ipmi_sys_boot_opt);
> +		return OPAL_NO_MEM;
> +	}
> +
> +	msg->error = ipmi_get_chassis_boot_opt_resp;
> +	prlog(PR_INFO, "IPMI: Requesting IPMI_CHASSIS_GET_BOOT_OPT\n");
> +	rc = ipmi_queue_msg(msg);
> +	if (rc) {
> +		prlog(PR_ERR, "IPMI: Failed to queue IPMI_CHASSIS_GET_BOOT_OPT\n");
> +		free(ipmi_sys_boot_opt);
> +		ipmi_free_msg(msg);
> +		return rc;
> +	}
> +
> +	bmc_boot_opt_waiting = true;
> +	return rc;
> +}
> diff --git a/include/ipmi.h b/include/ipmi.h
> index da85c4b0..ec9f3c49 100644
> --- a/include/ipmi.h
> +++ b/include/ipmi.h
> @@ -109,6 +109,7 @@
>  #define IPMI_GET_SEL_TIME		IPMI_CODE(IPMI_NETFN_STORAGE, 0x48)
>  #define IPMI_SET_SEL_TIME		IPMI_CODE(IPMI_NETFN_STORAGE, 0x49)
>  #define IPMI_CHASSIS_CONTROL		IPMI_CODE(IPMI_NETFN_CHASSIS, 0x02)
> +#define IPMI_CHASSIS_GET_BOOT_OPT	IPMI_CODE(IPMI_NETFN_CHASSIS, 0x09)
>  #define IPMI_BMC_GET_DEVICE_ID		IPMI_CODE(IPMI_NETFN_APP, 0x01)
>  #define IPMI_SET_POWER_STATE		IPMI_CODE(IPMI_NETFN_APP, 0x06)
>  #define IPMI_GET_POWER_STATE		IPMI_CODE(IPMI_NETFN_APP, 0x07)
> @@ -291,4 +292,10 @@ extern int ipmi_get_bmc_info_request(void);
>  /* Add BMC firmware info to device tree */
>  extern void ipmi_dt_add_bmc_info(void);
>  
> +/* Get BMC Boot Options info (specifically OEM param 0x62) */
> +int ipmi_get_chassis_boot_opt_request(void);
> +
> +/* Get OEM Boot Option 0x62 for SBE validation flag */
> +int ipmi_chassis_check_sbe_validation(void);
> +
>  #endif
> diff --git a/include/platform.h b/include/platform.h
> index f63c24a3..4f8627a3 100644
> --- a/include/platform.h
> +++ b/include/platform.h
> @@ -232,6 +232,11 @@ struct platform {
>  	 * OPAL terminate
>  	 */
>  	void __attribute__((noreturn)) (*terminate)(const char *msg);
> +
> +	/*
> +	 * SEEPROM update routine
> +	 */
> +	void		(*seeprom_update)(void);
>  };
>  
>  extern struct platform __platforms_start;
> diff --git a/platforms/astbmc/astbmc.h b/platforms/astbmc/astbmc.h
> index fe358b74..c302b607 100644
> --- a/platforms/astbmc/astbmc.h
> +++ b/platforms/astbmc/astbmc.h
> @@ -103,6 +103,7 @@ extern void astbmc_ext_irq_serirq_cpld(unsigned int 
> chip_id);
>  extern int pnor_init(void);
>  extern void check_all_slot_table(void);
>  extern void astbmc_exit(void);
> +extern void astbmc_seeprom_update(void);
>  
>  extern void slot_table_init(const struct slot_table_entry *top_table);
>  extern void slot_table_get_slot_info(struct phb *phb, struct pci_device * pd);
> diff --git a/platforms/astbmc/common.c b/platforms/astbmc/common.c
> index faa73e2f..76fa25f8 100644
> --- a/platforms/astbmc/common.c
> +++ b/platforms/astbmc/common.c
> @@ -26,6 +26,7 @@
>  #include <bt.h>
>  #include <errorlog.h>
>  #include <lpc.h>
> +#include <timebase.h>
>  
>  #include "astbmc.h"
>  
> @@ -168,6 +169,69 @@ int64_t astbmc_ipmi_reboot(void)
>  	return ipmi_chassis_control(IPMI_CHASSIS_HARD_RESET);
>  }
>  
> +void astbmc_seeprom_update(void)
> +{
> +	int flag_set, counter, rc;
> +
> +	rc = ipmi_get_chassis_boot_opt_request();
> +
> +	if (rc) {
> +		prlog(PR_WARNING, "Failed to check SBE validation flag\n");
> +		return;
> +	}
> +
> +	flag_set = ipmi_chassis_check_sbe_validation();
> +
> +	if (flag_set <= 0) {
> +		prlog(PR_DEBUG, "SBE validation flag unset or invalid\n");
> +		return;
> +	}
> +
> +	/*
> +	 * Flag is set, wait until SBE validation is complete and the flag
> +	 * has been reset.
> +	 */
> +	prlog(PR_WARNING, "SBE validation required, waiting for 
> completion\n");
> +	prlog(PR_WARNING, "System will be powered off if validation fails\n");
> +	counter = 0;
> +
> +	while (flag_set > 0) {
> +		time_wait_ms(10000);
> +		if (++counter % 3 == 0) {
> +			/* Let the user know we're alive every 30s */
> +			prlog(PR_WARNING, "waiting for completion...\n");
> +		}
> +		if (counter == 180) {
> +			/* This is longer than expected and we have no way of
> +			 * checking if it's still running. Apologies if you
> +			 * ever see this message.
> +			 */
> +			prlog(PR_WARNING, "30 minutes has elapsed, this is longer than 
> expected for verification\n");
> +			prlog(PR_WARNING, "If no progress is made a power reset of the BMC 
> and Host may be required\n");
> +			counter = 0;
> +		}
> +
> +		/* As above, loop anyway if we fail to check the flag */
> +		rc = ipmi_get_chassis_boot_opt_request();
> +		if (rc == 0)
> +			flag_set = ipmi_chassis_check_sbe_validation();
> +		else
> +			prlog(PR_WARNING, "Failed to check SBE validation flag\n");
> +	}
> +
> +	/*
> +	 * The SBE validation can (will) leave the SBE in a bad state,
> +	 * preventing timers from working properly. Reboot so that we
> +	 * can boot normally with everything intact.
> +	 */
> +	prlog(PR_WARNING, "SBE validation complete, rebooting\n");
> +	if (platform.cec_reboot)
> +		platform.cec_reboot();
> +	else
> +		abort();
> +	while(true);
> +}
> +
>  static void astbmc_fixup_dt_system_id(void)
>  {
>  	/* Make sure we don't already have one */
> diff --git a/platforms/astbmc/garrison.c b/platforms/astbmc/garrison.c
> index 5cbe64b5..ddd33721 100644
> --- a/platforms/astbmc/garrison.c
> +++ b/platforms/astbmc/garrison.c
> @@ -305,4 +305,5 @@ DECLARE_PLATFORM(garrison) = {
>  	.resource_loaded	= flash_resource_loaded,
>  	.exit			= ipmi_wdt_final_reset,
>  	.terminate		= ipmi_terminate,
> +	.seeprom_update		= astbmc_seeprom_update,
>  };
> diff --git a/platforms/astbmc/habanero.c b/platforms/astbmc/habanero.c
> index 8e11b81e..ab010278 100644
> --- a/platforms/astbmc/habanero.c
> +++ b/platforms/astbmc/habanero.c
> @@ -149,4 +149,5 @@ DECLARE_PLATFORM(habanero) = {
>  	.resource_loaded	= flash_resource_loaded,
>  	.exit			= ipmi_wdt_final_reset,
>  	.terminate		= ipmi_terminate,
> +	.seeprom_update		= astbmc_seeprom_update,
>  };
> diff --git a/platforms/astbmc/p8dnu.c b/platforms/astbmc/p8dnu.c
> index 9d42fc43..391aa7a8 100644
> --- a/platforms/astbmc/p8dnu.c
> +++ b/platforms/astbmc/p8dnu.c
> @@ -361,4 +361,5 @@ DECLARE_PLATFORM(p8dnu) = {
>  	.resource_loaded	= flash_resource_loaded,
>  	.exit			= ipmi_wdt_final_reset,
>  	.terminate		= ipmi_terminate,
> +	.seeprom_update		= astbmc_seeprom_update,
>  };
> diff --git a/platforms/astbmc/p8dtu.c b/platforms/astbmc/p8dtu.c
> index 69500ea2..6f66dc22 100644
> --- a/platforms/astbmc/p8dtu.c
> +++ b/platforms/astbmc/p8dtu.c
> @@ -262,6 +262,7 @@ DECLARE_PLATFORM(p8dtu1u) = {
>  	.resource_loaded	= flash_resource_loaded,
>  	.exit			= ipmi_wdt_final_reset,
>  	.terminate		= ipmi_terminate,
> +	.seeprom_update		= astbmc_seeprom_update,
>  };
>  
>  DECLARE_PLATFORM(p8dtu2u) = {
> @@ -279,5 +280,6 @@ DECLARE_PLATFORM(p8dtu2u) = {
>  	.resource_loaded	= flash_resource_loaded,
>  	.exit			= ipmi_wdt_final_reset,
>  	.terminate		= ipmi_terminate,
> +	.seeprom_update		= astbmc_seeprom_update,
>  };
>  
> -- 
> 2.21.0
> 
>


More information about the Skiboot-stable mailing list