[Skiboot] [PATCH] ipmi: ensure forward progress on ipmi_queue_msg_sync()

[Oliver]

On Wed, May 1, 2019 at 5:06 PM Stewart Smith <stewart at 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 at kaod.org>
> Signed-off-by: Stewart Smith <stewart at 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 at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot