| Message ID | 20190501070556.12913-1-stewart@linux.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | ipmi: ensure forward progress on ipmi_queue_msg_sync() | expand |
On Wed, 1 May 2019, at 16:36, Stewart Smith wrote: > BT responses are handled using a timer doing the polling. To hope to > get an answer to an IPMI synchronous message, the timer needs to run. > > We can't just check all timers though as there may be a timer that > wants a lock that's held by a code path calling ipmi_queue_msg_sync(), > and if we did enforce that as a requirement, it's a pretty subtle > API that is asking to be broken. > > So, if we just run a poll function to crank anything that the IPMI > backend needs, then we should be fine. > > This issue shows up very quickly under QEMU when loading the first > flash resource with the IPMI HIOMAP backend. > > Reported-by: Cédric Le Goater <clg@kaod.org> > Signed-off-by: Stewart Smith <stewart@linux.ibm.com> Reviewed-by: Andrew Jeffery <andrew@aj.id.au> > --- > core/ipmi.c | 12 +++++++++++- > hw/bt.c | 6 ++++++ > hw/fsp/fsp-ipmi.c | 2 ++ > include/ipmi.h | 9 +++++++++ > 4 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/core/ipmi.c b/core/ipmi.c > index 2bf3f4dabe19..9cf5aa626b02 100644 > --- a/core/ipmi.c > +++ b/core/ipmi.c > @@ -182,8 +182,18 @@ void ipmi_queue_msg_sync(struct ipmi_msg *msg) > ipmi_queue_msg_head(msg); > unlock(&sync_lock); > > - while (sync_msg == msg) > + /* > + * BT response handling relies on a timer. We can't just run all > + * timers because we may have been called with a lock that a timer > + * wants, and they're generally not written to cope with that. > + * So, just run whatever the IPMI backend needs to make forward > + * progress. > + */ > + while (sync_msg == msg) { > + if (msg->backend->poll) > + msg->backend->poll(); > time_wait_ms(10); > + } > } > > static void ipmi_read_event_complete(struct ipmi_msg *msg) > diff --git a/hw/bt.c b/hw/bt.c > index 9febe8e5ce08..a0ff0db4c479 100644 > --- a/hw/bt.c > +++ b/hw/bt.c > @@ -526,6 +526,11 @@ 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) > +{ > + bt_poll(NULL, NULL, mftb()); > +} > + > static void bt_add_msg(struct bt_msg *bt_msg) > { > bt_msg->tb = 0; > @@ -647,6 +652,7 @@ static struct ipmi_backend bt_backend = { > .queue_msg_head = bt_add_ipmi_msg_head, > .dequeue_msg = bt_del_ipmi_msg, > .disable_retry = bt_disable_ipmi_msg_retry, > + .poll = bt_ipmi_poll, > }; > > static struct lpc_client bt_lpc_client = { > diff --git a/hw/fsp/fsp-ipmi.c b/hw/fsp/fsp-ipmi.c > index d262cee6591d..8c65e6c77f88 100644 > --- a/hw/fsp/fsp-ipmi.c > +++ b/hw/fsp/fsp-ipmi.c > @@ -254,6 +254,8 @@ static struct ipmi_backend fsp_ipmi_backend = { > .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, > }; > > static bool fsp_ipmi_send_response(uint32_t cmd) > diff --git a/include/ipmi.h b/include/ipmi.h > index 4999bb5a3f4c..ea5a0a971daf 100644 > --- a/include/ipmi.h > +++ b/include/ipmi.h > @@ -182,6 +182,15 @@ struct ipmi_backend { > int (*queue_msg_head)(struct ipmi_msg *); > int (*dequeue_msg)(struct ipmi_msg *); > void (*disable_retry)(struct ipmi_msg *); > + /* > + * When processing a synchronous IPMI message, pollers may not run, and > + * neither may timers (as the synchronous IPMI message may be being > + * done with locks held, which a timer may then try to also take). > + * > + * So, ensure we have a way to drive any state machines that an IPMI > + * backend may neeed to crank to ensure forward progress. > + */ > + void (*poll)(void); > }; > > extern struct ipmi_backend *ipmi_backend; > -- > 2.20.1 > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot >
On 5/1/19 9:05 AM, Stewart Smith wrote: > BT responses are handled using a timer doing the polling. To hope to > get an answer to an IPMI synchronous message, the timer needs to run. > > We can't just check all timers though as there may be a timer that > wants a lock that's held by a code path calling ipmi_queue_msg_sync(), > and if we did enforce that as a requirement, it's a pretty subtle > API that is asking to be broken. > > So, if we just run a poll function to crank anything that the IPMI > backend needs, then we should be fine. > > This issue shows up very quickly under QEMU when loading the first > flash resource with the IPMI HIOMAP backend. > > Reported-by: Cédric Le Goater <clg@kaod.org> > Signed-off-by: Stewart Smith <stewart@linux.ibm.com> Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > core/ipmi.c | 12 +++++++++++- > hw/bt.c | 6 ++++++ > hw/fsp/fsp-ipmi.c | 2 ++ > include/ipmi.h | 9 +++++++++ > 4 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/core/ipmi.c b/core/ipmi.c > index 2bf3f4dabe19..9cf5aa626b02 100644 > --- a/core/ipmi.c > +++ b/core/ipmi.c > @@ -182,8 +182,18 @@ void ipmi_queue_msg_sync(struct ipmi_msg *msg) > ipmi_queue_msg_head(msg); > unlock(&sync_lock); > > - while (sync_msg == msg) > + /* > + * BT response handling relies on a timer. We can't just run all > + * timers because we may have been called with a lock that a timer > + * wants, and they're generally not written to cope with that. > + * So, just run whatever the IPMI backend needs to make forward > + * progress. > + */ > + while (sync_msg == msg) { > + if (msg->backend->poll) > + msg->backend->poll(); > time_wait_ms(10); > + } > } > > static void ipmi_read_event_complete(struct ipmi_msg *msg) > diff --git a/hw/bt.c b/hw/bt.c > index 9febe8e5ce08..a0ff0db4c479 100644 > --- a/hw/bt.c > +++ b/hw/bt.c > @@ -526,6 +526,11 @@ 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) > +{ > + bt_poll(NULL, NULL, mftb()); > +} > + > static void bt_add_msg(struct bt_msg *bt_msg) > { > bt_msg->tb = 0; > @@ -647,6 +652,7 @@ static struct ipmi_backend bt_backend = { > .queue_msg_head = bt_add_ipmi_msg_head, > .dequeue_msg = bt_del_ipmi_msg, > .disable_retry = bt_disable_ipmi_msg_retry, > + .poll = bt_ipmi_poll, > }; > > static struct lpc_client bt_lpc_client = { > diff --git a/hw/fsp/fsp-ipmi.c b/hw/fsp/fsp-ipmi.c > index d262cee6591d..8c65e6c77f88 100644 > --- a/hw/fsp/fsp-ipmi.c > +++ b/hw/fsp/fsp-ipmi.c > @@ -254,6 +254,8 @@ static struct ipmi_backend fsp_ipmi_backend = { > .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, > }; > > static bool fsp_ipmi_send_response(uint32_t cmd) > diff --git a/include/ipmi.h b/include/ipmi.h > index 4999bb5a3f4c..ea5a0a971daf 100644 > --- a/include/ipmi.h > +++ b/include/ipmi.h > @@ -182,6 +182,15 @@ struct ipmi_backend { > int (*queue_msg_head)(struct ipmi_msg *); > int (*dequeue_msg)(struct ipmi_msg *); > void (*disable_retry)(struct ipmi_msg *); > + /* > + * When processing a synchronous IPMI message, pollers may not run, and > + * neither may timers (as the synchronous IPMI message may be being > + * done with locks held, which a timer may then try to also take). > + * > + * So, ensure we have a way to drive any state machines that an IPMI > + * backend may neeed to crank to ensure forward progress. > + */ > + void (*poll)(void); > }; > > extern struct ipmi_backend *ipmi_backend; >
On 05/01/2019 12:35 PM, Stewart Smith wrote: > BT responses are handled using a timer doing the polling. To hope to > get an answer to an IPMI synchronous message, the timer needs to run. > > We can't just check all timers though as there may be a timer that > wants a lock that's held by a code path calling ipmi_queue_msg_sync(), > and if we did enforce that as a requirement, it's a pretty subtle > API that is asking to be broken. > > So, if we just run a poll function to crank anything that the IPMI > backend needs, then we should be fine. > > This issue shows up very quickly under QEMU when loading the first > flash resource with the IPMI HIOMAP backend. > > Reported-by: Cédric Le Goater <clg@kaod.org> > Signed-off-by: Stewart Smith <stewart@linux.ibm.com> Reviewed-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> -Vasant
On Wed, May 1, 2019 at 5:06 PM Stewart Smith <stewart@linux.ibm.com> wrote: > > BT responses are handled using a timer doing the polling. To hope to > get an answer to an IPMI synchronous message, the timer needs to run. > > We can't just check all timers though as there may be a timer that > wants a lock that's held by a code path calling ipmi_queue_msg_sync(), > and if we did enforce that as a requirement, it's a pretty subtle > API that is asking to be broken. > > So, if we just run a poll function to crank anything that the IPMI > backend needs, then we should be fine. > > This issue shows up very quickly under QEMU when loading the first > flash resource with the IPMI HIOMAP backend. > > Reported-by: Cédric Le Goater <clg@kaod.org> > Signed-off-by: Stewart Smith <stewart@linux.ibm.com> Merged to master as of v6.3-2-gf01cd777adb1 I'm a little bit iffy on this since we adding hacks for specific pollers seems kind of jank, but I don't have any better ideas right now. > --- > core/ipmi.c | 12 +++++++++++- > hw/bt.c | 6 ++++++ > hw/fsp/fsp-ipmi.c | 2 ++ > include/ipmi.h | 9 +++++++++ > 4 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/core/ipmi.c b/core/ipmi.c > index 2bf3f4dabe19..9cf5aa626b02 100644 > --- a/core/ipmi.c > +++ b/core/ipmi.c > @@ -182,8 +182,18 @@ void ipmi_queue_msg_sync(struct ipmi_msg *msg) > ipmi_queue_msg_head(msg); > unlock(&sync_lock); > > - while (sync_msg == msg) > + /* > + * BT response handling relies on a timer. We can't just run all > + * timers because we may have been called with a lock that a timer > + * wants, and they're generally not written to cope with that. > + * So, just run whatever the IPMI backend needs to make forward > + * progress. > + */ > + while (sync_msg == msg) { > + if (msg->backend->poll) > + msg->backend->poll(); > time_wait_ms(10); > + } > } > > static void ipmi_read_event_complete(struct ipmi_msg *msg) > diff --git a/hw/bt.c b/hw/bt.c > index 9febe8e5ce08..a0ff0db4c479 100644 > --- a/hw/bt.c > +++ b/hw/bt.c > @@ -526,6 +526,11 @@ 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) > +{ > + bt_poll(NULL, NULL, mftb()); > +} > + > static void bt_add_msg(struct bt_msg *bt_msg) > { > bt_msg->tb = 0; > @@ -647,6 +652,7 @@ static struct ipmi_backend bt_backend = { > .queue_msg_head = bt_add_ipmi_msg_head, > .dequeue_msg = bt_del_ipmi_msg, > .disable_retry = bt_disable_ipmi_msg_retry, > + .poll = bt_ipmi_poll, > }; > > static struct lpc_client bt_lpc_client = { > diff --git a/hw/fsp/fsp-ipmi.c b/hw/fsp/fsp-ipmi.c > index d262cee6591d..8c65e6c77f88 100644 > --- a/hw/fsp/fsp-ipmi.c > +++ b/hw/fsp/fsp-ipmi.c > @@ -254,6 +254,8 @@ static struct ipmi_backend fsp_ipmi_backend = { > .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, > }; > > static bool fsp_ipmi_send_response(uint32_t cmd) > diff --git a/include/ipmi.h b/include/ipmi.h > index 4999bb5a3f4c..ea5a0a971daf 100644 > --- a/include/ipmi.h > +++ b/include/ipmi.h > @@ -182,6 +182,15 @@ struct ipmi_backend { > int (*queue_msg_head)(struct ipmi_msg *); > int (*dequeue_msg)(struct ipmi_msg *); > void (*disable_retry)(struct ipmi_msg *); > + /* > + * When processing a synchronous IPMI message, pollers may not run, and > + * neither may timers (as the synchronous IPMI message may be being > + * done with locks held, which a timer may then try to also take). > + * > + * So, ensure we have a way to drive any state machines that an IPMI > + * backend may neeed to crank to ensure forward progress. > + */ > + void (*poll)(void); > }; > > extern struct ipmi_backend *ipmi_backend; > -- > 2.20.1 > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
Oliver <oohall@gmail.com> writes: > On Wed, May 1, 2019 at 5:06 PM Stewart Smith <stewart@linux.ibm.com> wrote: >> >> BT responses are handled using a timer doing the polling. To hope to >> get an answer to an IPMI synchronous message, the timer needs to run. >> >> We can't just check all timers though as there may be a timer that >> wants a lock that's held by a code path calling ipmi_queue_msg_sync(), >> and if we did enforce that as a requirement, it's a pretty subtle >> API that is asking to be broken. >> >> So, if we just run a poll function to crank anything that the IPMI >> backend needs, then we should be fine. >> >> This issue shows up very quickly under QEMU when loading the first >> flash resource with the IPMI HIOMAP backend. >> >> Reported-by: Cédric Le Goater <clg@kaod.org> >> Signed-off-by: Stewart Smith <stewart@linux.ibm.com> > > Merged to master as of v6.3-2-gf01cd777adb1 Thanks. > I'm a little bit iffy on this since we adding hacks for specific > pollers seems kind of jank, but I don't have any better ideas right > now. Yeah, I was a bit the same. I came down on this design being a bit less likely to cause unintented consequences at least. In an ideal world, nothing would do a sync call except the abort() code path where we have really no option but to kill the world.
diff --git a/core/ipmi.c b/core/ipmi.c index 2bf3f4dabe19..9cf5aa626b02 100644 --- a/core/ipmi.c +++ b/core/ipmi.c @@ -182,8 +182,18 @@ void ipmi_queue_msg_sync(struct ipmi_msg *msg) ipmi_queue_msg_head(msg); unlock(&sync_lock); - while (sync_msg == msg) + /* + * BT response handling relies on a timer. We can't just run all + * timers because we may have been called with a lock that a timer + * wants, and they're generally not written to cope with that. + * So, just run whatever the IPMI backend needs to make forward + * progress. + */ + while (sync_msg == msg) { + if (msg->backend->poll) + msg->backend->poll(); time_wait_ms(10); + } } static void ipmi_read_event_complete(struct ipmi_msg *msg) diff --git a/hw/bt.c b/hw/bt.c index 9febe8e5ce08..a0ff0db4c479 100644 --- a/hw/bt.c +++ b/hw/bt.c @@ -526,6 +526,11 @@ 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) +{ + bt_poll(NULL, NULL, mftb()); +} + static void bt_add_msg(struct bt_msg *bt_msg) { bt_msg->tb = 0; @@ -647,6 +652,7 @@ static struct ipmi_backend bt_backend = { .queue_msg_head = bt_add_ipmi_msg_head, .dequeue_msg = bt_del_ipmi_msg, .disable_retry = bt_disable_ipmi_msg_retry, + .poll = bt_ipmi_poll, }; static struct lpc_client bt_lpc_client = { diff --git a/hw/fsp/fsp-ipmi.c b/hw/fsp/fsp-ipmi.c index d262cee6591d..8c65e6c77f88 100644 --- a/hw/fsp/fsp-ipmi.c +++ b/hw/fsp/fsp-ipmi.c @@ -254,6 +254,8 @@ static struct ipmi_backend fsp_ipmi_backend = { .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, }; static bool fsp_ipmi_send_response(uint32_t cmd) diff --git a/include/ipmi.h b/include/ipmi.h index 4999bb5a3f4c..ea5a0a971daf 100644 --- a/include/ipmi.h +++ b/include/ipmi.h @@ -182,6 +182,15 @@ struct ipmi_backend { int (*queue_msg_head)(struct ipmi_msg *); int (*dequeue_msg)(struct ipmi_msg *); void (*disable_retry)(struct ipmi_msg *); + /* + * When processing a synchronous IPMI message, pollers may not run, and + * neither may timers (as the synchronous IPMI message may be being + * done with locks held, which a timer may then try to also take). + * + * So, ensure we have a way to drive any state machines that an IPMI + * backend may neeed to crank to ensure forward progress. + */ + void (*poll)(void); }; extern struct ipmi_backend *ipmi_backend;
BT responses are handled using a timer doing the polling. To hope to get an answer to an IPMI synchronous message, the timer needs to run. We can't just check all timers though as there may be a timer that wants a lock that's held by a code path calling ipmi_queue_msg_sync(), and if we did enforce that as a requirement, it's a pretty subtle API that is asking to be broken. So, if we just run a poll function to crank anything that the IPMI backend needs, then we should be fine. This issue shows up very quickly under QEMU when loading the first flash resource with the IPMI HIOMAP backend. Reported-by: Cédric Le Goater <clg@kaod.org> Signed-off-by: Stewart Smith <stewart@linux.ibm.com> --- core/ipmi.c | 12 +++++++++++- hw/bt.c | 6 ++++++ hw/fsp/fsp-ipmi.c | 2 ++ include/ipmi.h | 9 +++++++++ 4 files changed, 28 insertions(+), 1 deletion(-)