[Skiboot] [PATCH RESEND v6 2/2] SBE: Add timer support
Benjamin Herrenschmidt
benh at kernel.crashing.org
Fri Apr 13 19:35:33 AEST 2018
On Tue, 2018-04-03 at 17:27 +0530, Vasant Hegde wrote:
> SBE on P9 provides one shot programmable timer facility. We can use this
> to implement OPAL timers and hence limit the reliance on the Linux
> heartbeat (similar to HW timer facility provided by SLW on P8).
>
> Design:
> - We will continue to run Linux heartbeat.
> - Each chip has SBE. This patch always schedules timer on SBE on master chip.
> - Start timer option starts new timer or restarts an active timer for the
> specified timeout.
> - SBE expects timeout value in microseconds. We track timeout value in TB.
> Hence we convert tb to microseconds before sending request to SBE.
> - We are requesting ack from SBE for timer message. It gaurantees that
> SBE has scheduled timer.
> - Disabling SBE timer
> We expect SBE to send timer expiry interrupt whenever timer expires. We
> wait for 1 more ms before disabling timer.
> In future we can consider below alternative approaches:
> - Presently SBE timer disable is permanent (until we reboot system).
> SBE sends "I'm back" interrupt after reset. We can consider restarting
> timer after SBE reset.
> - Reset SBE and start timer again.
> - Each chip has SBE. On multi chip system we can try to schedule timer
> on different chip.
>
> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
> CC: Benjamin Herrenschmidt <benh at kernel.crashing.org>
> ---
> core/interrupts.c | 3 +-
> core/test/run-timer.c | 8 +++
> core/timer.c | 17 +++++-
> hw/sbe-p9.c | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++
> include/sbe-p9.h | 6 ++
> 5 files changed, 189 insertions(+), 4 deletions(-)
>
> diff --git a/core/interrupts.c b/core/interrupts.c
> index b7f597c84..b82a9afd0 100644
> --- a/core/interrupts.c
> +++ b/core/interrupts.c
> @@ -26,6 +26,7 @@
> #include <ccan/str/str.h>
> #include <timer.h>
> #include <sbe-p8.h>
> +#include <sbe-p9.h>
>
> /* ICP registers */
> #define ICP_XIRR 0x4 /* 32-bit access */
> @@ -470,7 +471,7 @@ static int64_t opal_handle_interrupt(uint32_t isn, __be64 *outstanding_event_mas
> is->ops->interrupt(is, isn);
>
> /* Check timers if SBE timer isn't working */
> - if (!p8_sbe_timer_ok())
> + if (!p8_sbe_timer_ok() && !p9_sbe_timer_ok())
> check_timers(true);
>
> /* Update output events */
> diff --git a/core/test/run-timer.c b/core/test/run-timer.c
> index 986af28d8..159e007ac 100644
> --- a/core/test/run-timer.c
> +++ b/core/test/run-timer.c
> @@ -4,12 +4,15 @@
>
> #define __TEST__
> #include <timer.h>
> +#include <skiboot.h>
>
> #define mftb() (stamp)
> #define sync()
> #define smt_lowest()
> #define smt_medium()
>
> +enum proc_gen proc_gen = proc_gen_p9;
> +
> static uint64_t stamp, last;
> struct lock;
> static inline void lock_caller(struct lock *l, const char *caller)
> @@ -53,6 +56,11 @@ void p8_sbe_update_timer_expiry(uint64_t new_target)
> /* FIXME: do intersting SLW timer sim */
> }
>
> +void p9_sbe_update_timer_expiry(uint64_t new_target)
> +{
> + (void)new_target;
> +}
> +
> int main(void)
> {
> unsigned int i;
> diff --git a/core/timer.c b/core/timer.c
> index 21f62a492..8eefb74be 100644
> --- a/core/timer.c
> +++ b/core/timer.c
> @@ -5,6 +5,7 @@
> #include <device.h>
> #include <opal.h>
> #include <sbe-p8.h>
> +#include <sbe-p9.h>
>
> #ifdef __TEST__
> #define this_cpu() ((void *)-1)
> @@ -109,8 +110,12 @@ static void __schedule_timer_at(struct timer *t, uint64_t when)
> bail:
> /* Pick up the next timer and upddate the SBE HW timer */
> lt = list_top(&timer_list, struct timer, link);
> - if (lt)
> - p8_sbe_update_timer_expiry(lt->target);
> + if (lt) {
> + if (proc_gen < proc_gen_p9)
> + p8_sbe_update_timer_expiry(lt->target);
> + else
> + p9_sbe_update_timer_expiry(lt->target);
> + }
> }
>
> void schedule_timer_at(struct timer *t, uint64_t when)
> @@ -167,7 +172,11 @@ static void __check_poll_timers(uint64_t now)
> * arbitrarily 1us.
> */
> if (t->running) {
> - p8_sbe_update_timer_expiry(now + usecs_to_tb(1));
> + if (proc_gen < proc_gen_p9)
> + p8_sbe_update_timer_expiry(now + usecs_to_tb(1));
> + else
> + p9_sbe_update_timer_expiry(now + usecs_to_tb(1));
> +
> break;
> }
>
> @@ -266,6 +275,8 @@ void late_init_timers(void)
> */
> if (platform.heartbeat_time) {
> heartbeat = platform.heartbeat_time();
> + } else if (p9_sbe_timer_ok()) {
> + heartbeat = HEARTBEAT_DEFAULT_MS * 10;
> } else if (p8_sbe_timer_ok() || fsp_present()) {
> heartbeat = HEARTBEAT_DEFAULT_MS * 10;
> }
> diff --git a/hw/sbe-p9.c b/hw/sbe-p9.c
> index 059bbf757..0da9516f8 100644
> --- a/hw/sbe-p9.c
> +++ b/hw/sbe-p9.c
> @@ -53,6 +53,7 @@
> #include <sbe-p9.h>
> #include <skiboot.h>
> #include <timebase.h>
> +#include <timer.h>
> #include <trace.h>
> #include <xscom.h>
>
> @@ -86,6 +87,24 @@ struct p9_sbe {
> /* Default SBE chip ID */
> static int sbe_default_chip_id = -1;
>
> +/* Is SBE timer running? */
> +static bool sbe_has_timer = false;
> +static bool sbe_timer_in_progress = false;
> +
> +/* Inflight and next timer in TB */
> +static uint64_t sbe_last_gen_stamp;
> +static uint64_t sbe_timer_target;
> +
> +/*
> + * Minimum timeout value for P9 is 100 microseconds. After that
> + * SBE timer can handle granularity of 1 microsecond.
> + */
> +#define SBE_TIMER_DEFAULT_US 100
> +static uint64_t sbe_timer_def_tb;
> +
> +/* Timer control message */
> +static struct p9_sbe_msg *timer_ctrl_msg;
> +
> #define SBE_STATUS_PRI_SHIFT 0x30
> #define SBE_STATUS_SEC_SHIFT 0x20
> #define SBE_FFDC_PRESENT PPC_BIT(1)
> @@ -469,8 +488,18 @@ static void p9_sbe_handle_response(struct p9_sbe *sbe, struct p9_sbe_msg *msg)
> }
> }
>
> +/* Check whether we got ACK from SBE for timer message */
> +static inline bool p9_sbe_timer_got_ack(void)
> +{
> + if (p9_sbe_msg_busy(timer_ctrl_msg))
> + return false;
> +
> + return true;
> +}
> +
> void p9_sbe_interrupt(uint32_t chip_id)
> {
> + bool timer_interrupt = false;
> int rc;
> u64 data = 0, val;
> struct proc_chip *chip;
> @@ -504,6 +533,16 @@ void p9_sbe_interrupt(uint32_t chip_id)
> prd_sbe_passthrough(chip_id);
> }
>
> + /* Timer expired */
> + if (data & SBE_HOST_TIMER_EXPIRY) {
> + if (!p9_sbe_timer_got_ack()) {
> + data &= ~(SBE_HOST_TIMER_EXPIRY);
> + } else {
> + sbe_timer_in_progress = false;
> + timer_interrupt = true;
> + }
> + }
So not sure I understand the above. It's the same bit in the doorbell
that is the "ack" and the timer expiry ? That means you basically need
"2" interrupts, one for the ack, one for the expiry, when you set the
timer ?
That's racy no ? IE what if you poll a bit slowly bcs you're busy
elsewhere and get both ack and timer interrupt ? They coalesce into a
single event and your timer is gone, no ?
> /* SBE came back from reset */
> if (data & SBE_HOST_RESET) {
> prlog(PR_NOTICE, "Back from reset [chip id = %x]\n", chip_id);
> @@ -547,6 +586,9 @@ clr_interrupt:
>
> p9_sbe_process_queue(sbe);
> unlock(&sbe->lock);
> + /* Drop lock and call timers */
> + if (timer_interrupt)
> + check_timers(true);
If for some reason the timer fired early, are you confident it will be
re-armed ?
> }
>
> static void p9_sbe_timeout_poll(void *user_data __unused)
> @@ -598,6 +640,120 @@ static void p9_sbe_timeout_poll(void *user_data __unused)
> p9_sbe_process_queue(sbe);
> unlock(&sbe->lock);
> }
> +
> + /*
> + * Check if the timer is working. If at least 1ms elapsed since
> + * last scheduled timer expiry.
> + */
> + if (sbe_has_timer && sbe_timer_in_progress) {
> + chip = get_chip(sbe_default_chip_id);
> + sbe = chip->sbe;
> + if (tb_compare(mftb(), sbe_last_gen_stamp + msecs_to_tb(1))
> + != TB_AAFTERB)
> + return;
So I would have put that inside the above lock section, basically, if
the chip is the default chip, add the timeout logic (and move it to a
separate function).
> + /*
> + * In some cases there will be a delay in calling OPAL interrupt
> + * handler routine (opal_handle_interrupt). In such cases its
> + * possible that SBE has responded, but OPAL didn't act on that.
> + * Hence check for SBE response before disabling timer.
> + */
> + p9_sbe_interrupt(sbe_default_chip_id);
Otherwise you call the above twice in the same function and the
function becomes a mess.
Also you keep taking & releasing the lock, it's inefficient. Follow my
advice in the other patch, have the lock taken once in a per-chip thing
and everything else underneath is done with that one lock instance.
> +
> + lock(&sbe->lock);
> + if (!sbe_timer_in_progress) {
> + unlock(&sbe->lock);
> + return;
> + }
> +
> + if (tb_compare(mftb(), sbe_last_gen_stamp + msecs_to_tb(1))
> + != TB_AAFTERB) {
> + unlock(&sbe->lock);
> + return;
> + }
> +
> + prlog(PR_ERR, "Timer stuck, falling back to OPAL pollers.\n");
> + prlog(PR_ERR, "You will likely have slower I2C and may have "
> + "experienced increased jitter.\n");
> + p9_sbe_reg_dump(sbe_default_chip_id);
> + sbe_has_timer = false;
> + sbe_timer_in_progress = false;
> + unlock(&sbe->lock);
> + }
> +}
> +
> +/*
> + * This is called with the timer lock held, so there is no
> + * issue with re-entrancy or concurrence
> + */
> +void p9_sbe_update_timer_expiry(uint64_t new_target)
> +{
> + int rc;
> + u32 tick_us = SBE_TIMER_DEFAULT_US;
> + u64 tb_cnt, now = mftb();
> +
> + if (!sbe_has_timer || new_target == sbe_timer_target)
> + return;
> +
> + sbe_timer_target = new_target;
> +
> + /* Modify timer */
> + if (sbe_timer_in_progress) {
> + if (sbe_timer_target < now)
> + return;
> +
> + /* remaining time <= sbe_timer_def_tb */
> + if ((sbe_last_gen_stamp - now) <= sbe_timer_def_tb)
> + return;
> +
> + if (sbe_timer_target > sbe_last_gen_stamp)
> + return;
> +
> + sbe_timer_in_progress = false;
So aren't you potentially re-using or modifying a message that is in
the middle of being sent ?
IE a previous call to p9_sbe_update_timer_expiry has queued the
message, it's now in the process of being sent, while this gets called,
cleares sbe_timer_in_progress, ad move on with hacking the "active"
message ... that isn't right.
You need to flag that a new target is needed and prep a new message
from the completion of the previous one.
> + }
> +
> + if (now < sbe_timer_target) {
> + /* Calculate how many microseconds from now, rounded up */
> + if ((sbe_timer_target - now) > sbe_timer_def_tb) {
> + tb_cnt = sbe_timer_target - now + usecs_to_tb(1) - 1;
> + tick_us = tb_to_usecs(tb_cnt);
> + }
> + }
> +
> + /* Clear sequence number */
> + timer_ctrl_msg->reg[0] &= ~(PPC_BITMASK(32, 47));
Why ?
> + /* Update timeout value */
> + timer_ctrl_msg->reg[1] = tick_us;
> +
> + rc = p9_sbe_sync_msg(sbe_default_chip_id, timer_ctrl_msg, false);
> + if (rc != OPAL_SUCCESS) {
> + prlog(PR_ERR, "Failed to start timer [chip id = %x]\n",
> + sbe_default_chip_id);
> + return;
> + }
> +
> + /* Update last scheduled timer value */
> + sbe_last_gen_stamp = mftb() + usecs_to_tb(tick_us);
> + sbe_timer_in_progress = true;
> +}
> +
> +/* Initialize SBE timer */
> +static void p9_sbe_timer_init(void)
> +{
> + timer_ctrl_msg = p9_sbe_mkmsg(SBE_CMD_CONTROL_TIMER,
> + CONTROL_TIMER_START, 0, 0, 0);
> + assert(timer_ctrl_msg);
> +
> + sbe_has_timer = true;
> + sbe_timer_target = mftb();
> + sbe_last_gen_stamp = ~0ull;
> + sbe_timer_def_tb = usecs_to_tb(SBE_TIMER_DEFAULT_US);
> + prlog(PR_INFO, "Timer facility on chip %x\n", sbe_default_chip_id);
> +}
> +
> +bool p9_sbe_timer_ok(void)
> +{
> + return sbe_has_timer;
> }
>
> void p9_sbe_init(void)
> @@ -635,6 +791,9 @@ void p9_sbe_init(void)
> return;
> }
>
> + /* Initiate SBE timer */
> + p9_sbe_timer_init();
> +
> /* Initiate SBE timeout poller */
> opal_add_poller(p9_sbe_timeout_poll, NULL);
> }
> diff --git a/include/sbe-p9.h b/include/sbe-p9.h
> index 892be4b85..c68894f11 100644
> --- a/include/sbe-p9.h
> +++ b/include/sbe-p9.h
> @@ -230,4 +230,10 @@ extern void p9_sbe_init(void);
> /* SBE interrupt */
> extern void p9_sbe_interrupt(uint32_t chip_id);
>
> +/* Is SBE timer available ? */
> +extern bool p9_sbe_timer_ok(void);
> +
> +/* Update SBE timer expiry */
> +extern void p9_sbe_update_timer_expiry(uint64_t new_target);
> +
> #endif /* __SBE_P9_H */
More information about the Skiboot
mailing list