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

[Samuel Mendoza-Jonas]

On Tue, 2019-05-07 at 16:51 +0530, Vasant Hegde wrote:
> On 05/07/2019 07:31 AM, 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.
> 
> Keep in mind that the way we do timer init is different in P8 and P9.
> In P9, p9_sbe_init() initiates timer while in P8, slw_init() initiates timer.
> 
> Your platform.seeprom_update() works fine on P8 systems today. We have to be 
> careful
> before enabling on P9 systems.

Forgot to mention in the last email; this is specifically only for P8
machines, there is no plan to perform this check on P9.

> 
> 
> > Signed-off-by: Samuel Mendoza-Jonas <sam at mendozajonas.com>
> > ---
> 
> .../...
> 
> > @@ -110,3 +134,78 @@ 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(5);
> 
> With 5ms we may not run poller (see time_wait_poll()).
> 
> > +
> > +	if (!bmc_boot_opt_valid)
> 
> Free ipmi_sys_boot_opt and return rc?
> 
> > +		return -1;
> > +
> > +	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]);
> 
> Free ipmi message here ?
> 
> > +		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);
> > +}
> > +
> 
> .../...
> 
> >   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..413613da 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,52 @@ int64_t astbmc_ipmi_reboot(void)
> >   	return ipmi_chassis_control(IPMI_CHASSIS_HARD_RESET);
> >   }
> > 
> > +void astbmc_seeprom_update(void)
> > +{
> > +	int flag_set, counter;
> > +
> > +	ipmi_get_chassis_boot_opt_request();
> > +
> > +	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) {
> 
> What is BMC never updates option flag properly (say buggy BMC code) ?
> Do we need some way to exit this loop ?
> 
> > +		time_wait_ms(10000);
> > +		if (++counter >= 3) {
> > +			/* Let the user know we're alive every 30s */
> > +			prlog(PR_WARNING, "waiting for completion..\n");
> > +			counter = 0;
> > +		}
> > +
> > +		ipmi_get_chassis_boot_opt_request();
> 
> May be its good to check return value to confirm we did queue IPMI message properly.
> 
> -Vasant
> 
> > +		flag_set = ipmi_chassis_check_sbe_validation();
> > +	}
> > +
> 
>