| Message ID | 20230513121226.99723-4-npiggin@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | ipmi misc fixes | expand |
> On May 13, 2023, at 05:12, Nicholas Piggin <npiggin@gmail.com> wrote: > > Bring ipmi to a consistent state before booting a kernel by flushing > all outstanding messages. The OS may not start kicking the IPMI state > machine for some time. > > For example, without this change, when booting in QEMU, the IPMI command > issued by ipmi_wdt_final_reset() to disable the watchdog is not sent to > the BMC before the OS boots, effectively leaving the watchdog enabled > until the OS begins to drive OPAL pollers. Hah, that could be awkward and unpredictable for a payload to have to do a thing within a time. Nice catch :) Reviewed-by: Stewart Smith <stewart@flamingspork.com <mailto:stewart@flamingspork.com>> > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > core/ipmi.c | 11 ++++++++++- > hw/bt.c | 7 ++++++- > hw/fsp/fsp-ipmi.c | 12 ++++++++++-- > include/ipmi.h | 7 ++++++- > platforms/astbmc/common.c | 6 ++++++ > platforms/ibm-fsp/common.c | 6 ++++++ > 6 files changed, 44 insertions(+), 5 deletions(-) > > diff --git a/core/ipmi.c b/core/ipmi.c > index 59aa95fc..673aa0c9 100644 > --- a/core/ipmi.c > +++ b/core/ipmi.c > @@ -159,7 +159,7 @@ void ipmi_cmd_done(uint8_t cmd, uint8_t netfn, uint8_t cc, struct ipmi_msg *msg) > > void ipmi_queue_msg_sync(struct ipmi_msg *msg) > { > - void (*poll)(void) = msg->backend->poll; > + bool (*poll)(void) = msg->backend->poll; > > if (!ipmi_present()) > return; > @@ -192,6 +192,15 @@ void ipmi_queue_msg_sync(struct ipmi_msg *msg) > } > } > > +void ipmi_flush(void) > +{ > + if (!ipmi_present()) > + return; > + > + while (ipmi_backend->poll()) > + time_wait_ms(10); > +} > + > static void ipmi_read_event_complete(struct ipmi_msg *msg) > { > prlog(PR_DEBUG, "IPMI read event %02x complete: %d bytes. cc: %02x\n", > diff --git a/hw/bt.c b/hw/bt.c > index 5016feab..1912cd3a 100644 > --- a/hw/bt.c > +++ b/hw/bt.c > @@ -519,9 +519,14 @@ static void bt_poll(struct timer *t __unused, void *data __unused, > bt.irq_ok ? TIMER_POLL : msecs_to_tb(BT_DEFAULT_POLL_MS)); > } > > -static void bt_ipmi_poll(void) > +static bool bt_ipmi_poll(void) > { > + if (!lpc_ok()) > + return false; > + > bt_poll(NULL, NULL, mftb()); > + > + return bt.queue_len > 0; > } > > static void bt_add_msg(struct bt_msg *bt_msg) > diff --git a/hw/fsp/fsp-ipmi.c b/hw/fsp/fsp-ipmi.c > index e368c282..9e600f3f 100644 > --- a/hw/fsp/fsp-ipmi.c > +++ b/hw/fsp/fsp-ipmi.c > @@ -243,14 +243,22 @@ static int fsp_ipmi_dequeue_msg(struct ipmi_msg *ipmi_msg) > return 0; > } > > + > +static bool fsp_ipmi_poll(void) > +{ > + /* fsp_opal_poll poller checks command responses */ > + opal_run_pollers(); > + > + return !list_empty(&fsp_ipmi.msg_queue); > +} > + > static struct ipmi_backend fsp_ipmi_backend = { > .alloc_msg = fsp_ipmi_alloc_msg, > .free_msg = fsp_ipmi_free_msg, > .queue_msg = fsp_ipmi_queue_msg, > .queue_msg_head = fsp_ipmi_queue_msg_head, > .dequeue_msg = fsp_ipmi_dequeue_msg, > - /* FIXME if ever use ipmi_queue_msg_sync on FSP */ > - .poll = NULL, > + .poll = fsp_ipmi_poll, > }; > > static bool fsp_ipmi_rr_notify(uint32_t cmd_sub_mod, > diff --git a/include/ipmi.h b/include/ipmi.h > index 3e629ba4..5b7efd1e 100644 > --- a/include/ipmi.h > +++ b/include/ipmi.h > @@ -178,8 +178,10 @@ struct ipmi_backend { > * > * So, ensure we have a way to drive any state machines that an IPMI > * backend may neeed to crank to ensure forward progress. > + * > + * This returns true while there are any messages queued. > */ > - void (*poll)(void); > + bool (*poll)(void); > }; > > extern struct ipmi_backend *ipmi_backend; > @@ -220,6 +222,9 @@ void ipmi_queue_msg_sync(struct ipmi_msg *msg); > /* Removes the message from the list, queued previously */ > int ipmi_dequeue_msg(struct ipmi_msg *msg); > > +/* Polls the backend until all queued messages are completed */ > +void ipmi_flush(void); > + > /* Process a completed message */ > void ipmi_cmd_done(uint8_t cmd, uint8_t netfn, uint8_t cc, struct ipmi_msg *msg); > > diff --git a/platforms/astbmc/common.c b/platforms/astbmc/common.c > index 9ce22b38..bfbba2d5 100644 > --- a/platforms/astbmc/common.c > +++ b/platforms/astbmc/common.c > @@ -504,6 +504,12 @@ void astbmc_exit(void) > ipmi_wdt_final_reset(); > > ipmi_set_boot_count(); > + > + /* > + * Booting into an OS that may not call back into skiboot for > + * some time. Ensure all IPMI messages are processed first. > + */ > + ipmi_flush(); > } > > static const struct bmc_sw_config bmc_sw_ami = { > diff --git a/platforms/ibm-fsp/common.c b/platforms/ibm-fsp/common.c > index 4a723b25..f289d1f2 100644 > --- a/platforms/ibm-fsp/common.c > +++ b/platforms/ibm-fsp/common.c > @@ -186,6 +186,12 @@ void ibm_fsp_exit(void) > > /* Clear SRCs on the op-panel when Linux starts */ > op_panel_clear_src(); > + > + /* > + * Booting into an OS that may not call back into skiboot for > + * some time. Ensure all IPMI messages are processed first. > + */ > + ipmi_flush(); > } > > int64_t ibm_fsp_cec_reboot(void) > -- > 2.40.1 > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
On Mon May 15, 2023 at 6:44 AM AEST, Stewart Smith wrote: > > > > On May 13, 2023, at 05:12, Nicholas Piggin <npiggin@gmail.com> wrote: > > > > Bring ipmi to a consistent state before booting a kernel by flushing > > all outstanding messages. The OS may not start kicking the IPMI state > > machine for some time. > > > > For example, without this change, when booting in QEMU, the IPMI command > > issued by ipmi_wdt_final_reset() to disable the watchdog is not sent to > > the BMC before the OS boots, effectively leaving the watchdog enabled > > until the OS begins to drive OPAL pollers. > > Hah, that could be awkward and unpredictable for a payload to have to do a thing within a time. > > Nice catch :) > > Reviewed-by: Stewart Smith <stewart@flamingspork.com <mailto:stewart@flamingspork.com>> Thanks! It started out with adding kvm-unit-tests for the qemu powernv model. I use skiboot to boot that. When the test cases finish they would hang, which was my bug because the harness wasn't calling the poller after issuing OPAL shutdown (which is implemnted as IPMI command so that was never going through because the state machine was already backed up). Trying to debug that I got to the IPMI state machine and that it doesn't entirely process the message queue before booting the OS. So I only got lucky finding it because I'd added a different bug to the test harness. Still, I'll take it :) Thanks, Nick
diff --git a/core/ipmi.c b/core/ipmi.c index 59aa95fc..673aa0c9 100644 --- a/core/ipmi.c +++ b/core/ipmi.c @@ -159,7 +159,7 @@ void ipmi_cmd_done(uint8_t cmd, uint8_t netfn, uint8_t cc, struct ipmi_msg *msg) void ipmi_queue_msg_sync(struct ipmi_msg *msg) { - void (*poll)(void) = msg->backend->poll; + bool (*poll)(void) = msg->backend->poll; if (!ipmi_present()) return; @@ -192,6 +192,15 @@ void ipmi_queue_msg_sync(struct ipmi_msg *msg) } } +void ipmi_flush(void) +{ + if (!ipmi_present()) + return; + + while (ipmi_backend->poll()) + time_wait_ms(10); +} + static void ipmi_read_event_complete(struct ipmi_msg *msg) { prlog(PR_DEBUG, "IPMI read event %02x complete: %d bytes. cc: %02x\n", diff --git a/hw/bt.c b/hw/bt.c index 5016feab..1912cd3a 100644 --- a/hw/bt.c +++ b/hw/bt.c @@ -519,9 +519,14 @@ static void bt_poll(struct timer *t __unused, void *data __unused, bt.irq_ok ? TIMER_POLL : msecs_to_tb(BT_DEFAULT_POLL_MS)); } -static void bt_ipmi_poll(void) +static bool bt_ipmi_poll(void) { + if (!lpc_ok()) + return false; + bt_poll(NULL, NULL, mftb()); + + return bt.queue_len > 0; } static void bt_add_msg(struct bt_msg *bt_msg) diff --git a/hw/fsp/fsp-ipmi.c b/hw/fsp/fsp-ipmi.c index e368c282..9e600f3f 100644 --- a/hw/fsp/fsp-ipmi.c +++ b/hw/fsp/fsp-ipmi.c @@ -243,14 +243,22 @@ static int fsp_ipmi_dequeue_msg(struct ipmi_msg *ipmi_msg) return 0; } + +static bool fsp_ipmi_poll(void) +{ + /* fsp_opal_poll poller checks command responses */ + opal_run_pollers(); + + return !list_empty(&fsp_ipmi.msg_queue); +} + static struct ipmi_backend fsp_ipmi_backend = { .alloc_msg = fsp_ipmi_alloc_msg, .free_msg = fsp_ipmi_free_msg, .queue_msg = fsp_ipmi_queue_msg, .queue_msg_head = fsp_ipmi_queue_msg_head, .dequeue_msg = fsp_ipmi_dequeue_msg, - /* FIXME if ever use ipmi_queue_msg_sync on FSP */ - .poll = NULL, + .poll = fsp_ipmi_poll, }; static bool fsp_ipmi_rr_notify(uint32_t cmd_sub_mod, diff --git a/include/ipmi.h b/include/ipmi.h index 3e629ba4..5b7efd1e 100644 --- a/include/ipmi.h +++ b/include/ipmi.h @@ -178,8 +178,10 @@ struct ipmi_backend { * * So, ensure we have a way to drive any state machines that an IPMI * backend may neeed to crank to ensure forward progress. + * + * This returns true while there are any messages queued. */ - void (*poll)(void); + bool (*poll)(void); }; extern struct ipmi_backend *ipmi_backend; @@ -220,6 +222,9 @@ void ipmi_queue_msg_sync(struct ipmi_msg *msg); /* Removes the message from the list, queued previously */ int ipmi_dequeue_msg(struct ipmi_msg *msg); +/* Polls the backend until all queued messages are completed */ +void ipmi_flush(void); + /* Process a completed message */ void ipmi_cmd_done(uint8_t cmd, uint8_t netfn, uint8_t cc, struct ipmi_msg *msg); diff --git a/platforms/astbmc/common.c b/platforms/astbmc/common.c index 9ce22b38..bfbba2d5 100644 --- a/platforms/astbmc/common.c +++ b/platforms/astbmc/common.c @@ -504,6 +504,12 @@ void astbmc_exit(void) ipmi_wdt_final_reset(); ipmi_set_boot_count(); + + /* + * Booting into an OS that may not call back into skiboot for + * some time. Ensure all IPMI messages are processed first. + */ + ipmi_flush(); } static const struct bmc_sw_config bmc_sw_ami = { diff --git a/platforms/ibm-fsp/common.c b/platforms/ibm-fsp/common.c index 4a723b25..f289d1f2 100644 --- a/platforms/ibm-fsp/common.c +++ b/platforms/ibm-fsp/common.c @@ -186,6 +186,12 @@ void ibm_fsp_exit(void) /* Clear SRCs on the op-panel when Linux starts */ op_panel_clear_src(); + + /* + * Booting into an OS that may not call back into skiboot for + * some time. Ensure all IPMI messages are processed first. + */ + ipmi_flush(); } int64_t ibm_fsp_cec_reboot(void)
Bring ipmi to a consistent state before booting a kernel by flushing all outstanding messages. The OS may not start kicking the IPMI state machine for some time. For example, without this change, when booting in QEMU, the IPMI command issued by ipmi_wdt_final_reset() to disable the watchdog is not sent to the BMC before the OS boots, effectively leaving the watchdog enabled until the OS begins to drive OPAL pollers. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- core/ipmi.c | 11 ++++++++++- hw/bt.c | 7 ++++++- hw/fsp/fsp-ipmi.c | 12 ++++++++++-- include/ipmi.h | 7 ++++++- platforms/astbmc/common.c | 6 ++++++ platforms/ibm-fsp/common.c | 6 ++++++ 6 files changed, 44 insertions(+), 5 deletions(-)