[Skiboot] [PATCH v3 2/3] platforms/astbmc: Check for SBE validation step
Andrew Jeffery
andrew at aj.id.au
Mon May 6 17:43:25 AEST 2019
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.
> + 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)?
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?
> + 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.
> + prlog(PR_WARNING, "waiting for completion..\n");
> + counter = 0;
> + }
> +
Because we're still waiting 30s to get here:
> + 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