[Skiboot] [PATCH] HBRT: fix clobbered r16 when host services handlers are called
Frederic Barrat
fbarrat at linux.ibm.com
Wed Oct 13 19:18:53 AEDT 2021
On 12/10/2021 12:55, Nicholas Piggin wrote:
> Skiboot is using r16 as a fixed register containing this CPU pointer,
> but we can be called back into from hostboot via the host services
> interface, where r16 may have been set by hostboot. Switch this back to
> skiboot's CPU pointer before running host services handlers, and then
> restore it to the hostboot value before returning.
>
> Fixes: 11ce9612b3aa ("move the __this_cpu register to r16, reserve r13-r15")
> Reported-by: Frederic Barrat <fbarrat at linux.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin at gmail.com>
> ---
> Frederic tested an earlier version of this patch successfully but I
> made a change since then so I've left of the Tested-by tag.
So compared to the previous version I had tried, you removed the restore
of r16 when leaving the runtime services and leaving HBRT for good. r16
is non volatile, so it makes sense, HBRT should have restored it.
And it boots :-)
Reviewed-by: Frederic Barrat <fbarrat at linux.ibm.com>
Tested-by: Frederic Barrat <fbarrat at linux.ibm.com>
Thanks!
Fred
> Thanks,
> Nick
>
> asm/head.S | 10 +++
> include/skiboot.h | 5 ++
> platforms/ibm-fsp/hostservices.c | 125 +++++++++++++++++++++++++++++--
> 3 files changed, 134 insertions(+), 6 deletions(-)
>
> diff --git a/asm/head.S b/asm/head.S
> index fa8933b14..e968ac6b2 100644
> --- a/asm/head.S
> +++ b/asm/head.S
> @@ -1124,3 +1124,13 @@ start_kernel_secondary:
> mtspr SPR_HSRR1,%r10
> mfspr %r3,SPR_PIR
> hrfid
> +
> +.global restore_cpu_ptr_r16
> +restore_cpu_ptr_r16:
> + GET_CPU()
> + blr
> +
> +.global set_cpu_ptr_r16
> +set_cpu_ptr_r16:
> + mr %r16,%r3
> + blr
> diff --git a/include/skiboot.h b/include/skiboot.h
> index df11934f6..4f4a00574 100644
> --- a/include/skiboot.h
> +++ b/include/skiboot.h
> @@ -163,6 +163,11 @@ extern void start_kernel32(uint64_t entry, void* fdt,
> uint64_t mem_top) __noreturn;
> extern void start_kernel_secondary(uint64_t entry) __noreturn;
>
> +/* Re-set r16 register with CPU pointer, based on stack (r1) value */
> +extern void restore_cpu_ptr_r16(void);
> +/* Set r16 register with value in 'r16' parameter */
> +extern void set_cpu_ptr_r16(uint64_t r16);
> +
> /* Get description of machine from HDAT and create device-tree */
> extern int parse_hdat(bool is_opal);
>
> diff --git a/platforms/ibm-fsp/hostservices.c b/platforms/ibm-fsp/hostservices.c
> index accc0989a..1aab668d0 100644
> --- a/platforms/ibm-fsp/hostservices.c
> +++ b/platforms/ibm-fsp/hostservices.c
> @@ -168,6 +168,27 @@ struct runtime_interfaces {
> void (*reserved[32])(void);
> };
>
> +/*
> + * The host interfaces are called by hostboot, which considers r16 a regular
> + * non-volatile register that it may have used at the time of the call.
> + * Skiboot requires r16 to be set to the CPU pointer while it runs. So the
> + * host interface handlers must save HBRT's r16, switch r16 to the CPU pointer,
> + * do their thing, and then restore HBRT's r16 value before returning.
> + */
> +static uint64_t host_interface_pre_fixup(void)
> +{
> + uint64_t r16 = (uint64_t)__this_cpu;
> +
> + restore_cpu_ptr_r16();
> +
> + return r16;
> +}
> +
> +static void host_interface_post_fixup(uint64_t r16)
> +{
> + set_cpu_ptr_r16(r16);
> +}
> +
> static struct runtime_interfaces *hservice_runtime;
>
> static char *hbrt_con_buf = (char *)HBRT_CON_START;
> @@ -187,6 +208,7 @@ static struct memcons hbrt_memcons __section(".data.memcons") = {
>
> static void hservice_putc(char c)
> {
> + uint64_t r16 = host_interface_pre_fixup();
> uint32_t opos;
>
> hbrt_con_buf[hbrt_con_pos++] = c;
> @@ -207,15 +229,20 @@ static void hservice_putc(char c)
> opos |= MEMCONS_OUT_POS_WRAP;
> lwsync();
> hbrt_memcons.out_pos = cpu_to_be32(opos);
> +
> + host_interface_post_fixup(r16);
> }
>
> static void hservice_puts(const char *str)
> {
> + uint64_t r16 = host_interface_pre_fixup();
> char c;
>
> while((c = *(str++)) != 0)
> hservice_putc(c);
> hservice_putc(10);
> +
> + host_interface_post_fixup(r16);
> }
>
> static void hservice_mark(void)
> @@ -226,28 +253,48 @@ static void hservice_mark(void)
>
> static void hservice_assert(void)
> {
> + uint64_t r16 = host_interface_pre_fixup();
> +
> /**
> * @fwts-label HBRTassert
> * @fwts-advice HBRT triggered assert: you need to debug HBRT
> */
> prlog(PR_EMERG, "HBRT: Assertion from hostservices\n");
> abort();
> + /* Should not be reached... */
> + host_interface_post_fixup(r16);
> }
>
> static void *hservice_malloc(size_t size)
> {
> - return malloc(size);
> + uint64_t r16 = host_interface_pre_fixup();
> + void *mem;
> +
> + mem = malloc(size);
> +
> + host_interface_post_fixup(r16);
> +
> + return mem;
> }
>
> static void hservice_free(void *ptr)
> {
> + uint64_t r16 = host_interface_pre_fixup();
> free(ptr);
> + host_interface_post_fixup(r16);
> }
>
>
> static void *hservice_realloc(void *ptr, size_t size)
> {
> - return realloc(ptr, size);
> + uint64_t r16 = host_interface_pre_fixup();
> + void *mem;
> +
> + mem = realloc(ptr, size);
> +
> + host_interface_post_fixup(r16);
> +
> + return mem;
> }
>
> struct hbrt_elog_ent {
> @@ -319,6 +366,7 @@ static void hservice_start_elog_send(void)
>
> int hservice_send_error_log(uint32_t plid, uint32_t dsize, void *data)
> {
> + uint64_t r16 = host_interface_pre_fixup();
> struct hbrt_elog_ent *ent;
> void *abuf;
>
> @@ -327,6 +375,9 @@ int hservice_send_error_log(uint32_t plid, uint32_t dsize, void *data)
> /* We only know how to send error logs to FSP */
> if (!fsp_present()) {
> prerror("HBRT: Warning, error log from HBRT discarded !\n");
> +
> + host_interface_post_fixup(r16);
> +
> return OPAL_UNSUPPORTED;
> }
> if (dsize > PSI_DMA_HBRT_LOG_WRITE_BUF_SZ) {
> @@ -341,6 +392,9 @@ int hservice_send_error_log(uint32_t plid, uint32_t dsize, void *data)
> ent = zalloc(sizeof(struct hbrt_elog_ent));
> if (!ent) {
> unlock(&hbrt_elog_lock);
> +
> + host_interface_post_fixup(r16);
> +
> return OPAL_NO_MEM;
> }
>
> @@ -349,6 +403,9 @@ int hservice_send_error_log(uint32_t plid, uint32_t dsize, void *data)
> if (!abuf) {
> free(ent);
> unlock(&hbrt_elog_lock);
> +
> + host_interface_post_fixup(r16);
> +
> return OPAL_NO_MEM;
> }
> memset(abuf, 0, PSI_DMA_HBRT_LOG_WRITE_BUF_SZ);
> @@ -361,21 +418,36 @@ int hservice_send_error_log(uint32_t plid, uint32_t dsize, void *data)
> hservice_start_elog_send();
> unlock(&hbrt_elog_lock);
>
> + host_interface_post_fixup(r16);
> +
> return 0;
> }
>
> static int hservice_scom_read(uint64_t chip_id, uint64_t addr, void *buf)
> {
> - return xscom_read(chip_id, addr, buf);
> + uint64_t r16 = host_interface_pre_fixup();
> + int ret;
> +
> + ret = xscom_read(chip_id, addr, buf);
> +
> + host_interface_post_fixup(r16);
> +
> + return ret;
> }
>
> static int hservice_scom_write(uint64_t chip_id, uint64_t addr,
> const void *buf)
> {
> + uint64_t r16 = host_interface_pre_fixup();
> uint64_t val;
> + int ret;
>
> memcpy(&val, buf, sizeof(val));
> - return xscom_write(chip_id, addr, val);
> + ret = xscom_write(chip_id, addr, val);
> +
> + host_interface_post_fixup(r16);
> +
> + return ret;
> }
>
> struct hbrt_lid {
> @@ -475,6 +547,7 @@ void hservices_lid_preload(void)
>
> static int hservice_lid_load(uint32_t lid, void **buf, size_t *len)
> {
> + uint64_t r16 = host_interface_pre_fixup();
> struct hbrt_lid *hlid;
>
> prlog(PR_INFO, "HBRT: Lid load request for 0x%08x\n", lid);
> @@ -494,14 +567,24 @@ static int hservice_lid_load(uint32_t lid, void **buf, size_t *len)
> *len = hlid->len;
> prlog(PR_DEBUG, "HBRT: LID Serviced from cache,"
> " %x, len=0x%lx\n", hlid->id, hlid->len);
> +
> + host_interface_post_fixup(r16);
> +
> return 0;
> }
> }
> +
> + host_interface_post_fixup(r16);
> +
> return -ENOENT;
> }
>
> static int hservice_lid_unload(void *buf __unused)
> {
> + uint64_t r16 = host_interface_pre_fixup();
> +
> + host_interface_post_fixup(r16);
> +
> /* We do nothing as the LID is held in cache */
> return 0;
> }
> @@ -526,15 +609,19 @@ static uint64_t hservice_get_reserved_mem(const char *name)
>
> static void hservice_nanosleep(uint64_t i_seconds, uint64_t i_nano_seconds)
> {
> + uint64_t r16 = host_interface_pre_fixup();
> struct timespec ts;
>
> ts.tv_sec = i_seconds;
> ts.tv_nsec = i_nano_seconds;
> nanosleep_nopoll(&ts, NULL);
> +
> + host_interface_post_fixup(r16);
> }
>
> int hservice_wakeup(uint32_t i_core, uint32_t i_mode)
> {
> + uint64_t r16 = host_interface_pre_fixup();
> struct cpu_thread *cpu;
> int rc = OPAL_SUCCESS;
>
> @@ -556,6 +643,8 @@ int hservice_wakeup(uint32_t i_core, uint32_t i_mode)
> i_core <<= 2;
> break;
> default:
> + host_interface_post_fixup(r16);
> +
> return OPAL_UNSUPPORTED;
> }
>
> @@ -563,32 +652,49 @@ int hservice_wakeup(uint32_t i_core, uint32_t i_mode)
> switch(i_mode) {
> case 0: /* Assert special wakeup */
> cpu = find_cpu_by_pir(i_core);
> - if (!cpu)
> + if (!cpu) {
> + host_interface_post_fixup(r16);
> +
> return OPAL_PARAMETER;
> + }
> prlog(PR_TRACE, "HBRT: Special wakeup assert for core 0x%x,"
> " count=%d\n", i_core, cpu->hbrt_spec_wakeup);
> if (cpu->hbrt_spec_wakeup == 0)
> rc = dctl_set_special_wakeup(cpu);
> if (rc == 0)
> cpu->hbrt_spec_wakeup++;
> +
> + host_interface_post_fixup(r16);
> +
> return rc;
> +
> case 1: /* Deassert special wakeup */
> cpu = find_cpu_by_pir(i_core);
> - if (!cpu)
> + if (!cpu) {
> + host_interface_post_fixup(r16);
> +
> return OPAL_PARAMETER;
> + }
> prlog(PR_TRACE, "HBRT: Special wakeup release for core"
> " 0x%x, count=%d\n", i_core, cpu->hbrt_spec_wakeup);
> if (cpu->hbrt_spec_wakeup == 0) {
> prerror("HBRT: Special wakeup clear"
> " on core 0x%x with count=0\n",
> i_core);
> +
> + host_interface_post_fixup(r16);
> +
> return OPAL_WRONG_STATE;
> }
> /* What to do with count on errors ? */
> cpu->hbrt_spec_wakeup--;
> if (cpu->hbrt_spec_wakeup == 0)
> rc = dctl_clear_special_wakeup(cpu);
> +
> + host_interface_post_fixup(r16);
> +
> return rc;
> +
> case 2: /* Clear all special wakeups */
> prlog(PR_DEBUG, "HBRT: Special wakeup release for all cores\n");
> for_each_cpu(cpu) {
> @@ -598,8 +704,15 @@ int hservice_wakeup(uint32_t i_core, uint32_t i_mode)
> dctl_clear_special_wakeup(cpu);
> }
> }
> +
> + host_interface_post_fixup(r16);
> +
> return OPAL_SUCCESS;
> +
> default:
> +
> + host_interface_post_fixup(r16);
> +
> return OPAL_PARAMETER;
> }
> }
>
More information about the Skiboot
mailing list