[Skiboot] [PATCH v3 2/3] platforms/astbmc: Check for SBE validation step
Andrew Jeffery
andrew at aj.id.au
Tue May 7 11:04:08 AEST 2019
On Tue, 7 May 2019, at 09:50, Samuel Mendoza-Jonas wrote:
> On Mon, 2019-05-06 at 03:43 -0400, Andrew Jeffery wrote:
> >
> > On Tue, 30 Apr 2019, at 14:51, 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.
> > >
> > > Signed-off-by: Samuel Mendoza-Jonas <sam at mendozajonas.com>
> > > ---
> > > 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 | 4 ++
> > > hw/ipmi/ipmi-info.c | 90 ++++++++++++++++++++++++++++++++++++-
> > > include/ipmi.h | 7 +++
> > > include/platform.h | 5 +++
> > > platforms/astbmc/astbmc.h | 1 +
> > > platforms/astbmc/common.c | 37 +++++++++++++++
> > > platforms/astbmc/garrison.c | 1 +
> > > platforms/astbmc/habanero.c | 1 +
> > > platforms/astbmc/p8dnu.c | 1 +
> > > platforms/astbmc/p8dtu.c | 2 +
> > > 10 files changed, 148 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/core/init.c b/core/init.c
> > > index 0fe6c168..8d87854b 100644
> > > --- a/core/init.c
> > > +++ b/core/init.c
> > > @@ -590,6 +590,10 @@ void __noreturn load_and_boot_kernel(bool is_reboot)
> > > cmdline);
> > > }
> > >
> > > + /* Perform last-minute SBE checks if needed */
> > > + if (platform.seeprom_update)
> > > + platform.seeprom_update();
> > > +
> > > op_display(OP_LOG, OP_MOD_INIT, 0x000B);
> > >
> > > add_fast_reboot_dt_entries();
> > > diff --git a/hw/ipmi/ipmi-info.c b/hw/ipmi/ipmi-info.c
> > > index 56370037..3d514f95 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)
> > > @@ -110,3 +128,73 @@ 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);
> > > +
> > > + if (!bmc_boot_opt_valid)
> > > + 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]);
> > > + return;
> > > + }
> > > +
> > > + bmc_boot_opt_valid = true;
> > > + memcpy(ipmi_sys_boot_opt, msg->data, msg->resp_size);
> >
> > Should validate that msg->resp_size <= sizeof(*ipmi_sys_boot_opt) here, otherwise
> > Bad Things might happen.
>
> Oh yes, I'll fix up the other instance of this here too.
>
> >
> > > + 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)
> > > + 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");
> > > + 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 de4638f3..a67845f8 100644
> > > --- a/include/platform.h
> > > +++ b/include/platform.h
> > > @@ -231,6 +231,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..32eded14 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,42 @@ 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_WARNING, "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) {
> >
> > Do we want a bail-out condition? Do we have any insight into how long validation
> > might take (i.e. is there a sensible upper-bound)?
>
> Ha, you said sensible.
>
> >
> > Maybe not given the assumption if we were to bail out would be that the
> > validation failed, at which point we'd just shut down anyway?
>
> The rough guide is apparently this can take from 5 minutes to up to 20(!)
> minutes depending on the machine. Perhaps it's worth warning after 30
> minutes that something could be wrong but practically there's nothing we
> can do except for turn off.
Yep.
>
> >
> > > + time_wait_ms(500);
> > > + if (++counter >= 60) {
> > > + /* Let the user know we're alive every 30s */
> >
> > Why the 500ms/60 split rather than e.g. 1000ms/30 ? Just out of curiosity.
>
> I think that split just came from me debugging the SBE issues :)
> However..
>
> >
> > > + prlog(PR_WARNING, "waiting for completion..\n");
> > > + counter = 0;
> > > + }
> > > +
> >
> > Because we're still waiting 30s to get here:
>
> This happens every 500ms. Given the expected time though we *could* bump
> it up to every 10-30 seconds.
Hah, yes, disregard that.
>
> >
> > > + ipmi_get_chassis_boot_opt_request();
> > > + flag_set = ipmi_chassis_check_sbe_validation();
> > > + }
> > > +
> > > + prlog(PR_WARNING, "SBE validation complete, continuing boot\n");
> > > +}
> > > +
> > > 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
> > >
> > > _______________________________________________
> > > Skiboot mailing list
> > > Skiboot at lists.ozlabs.org
> > > https://lists.ozlabs.org/listinfo/skiboot
> > >
> >
> > Cheers,
> >
> > Andrew
>
>
>
More information about the Skiboot
mailing list