Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220920151202.180057-1-chenzhongjin@huawei.com> <20220920151202.180057-4-chenzhongjin@huawei.com>
In-Reply-To: <20220920151202.180057-4-chenzhongjin@huawei.com>
From:   Guo Ren <guoren@kernel.org>
Date:   Wed, 21 Sep 2022 10:13:57 +0800
Message-ID: <CAJF2gTS1=xSrsaABLzbRwq2V=Wj3_SyHtooXosfY7L9zAwf=NA@mail.gmail.com>
Subject: Re: [PATCH -next 3/7] riscv: stacktrace: Save pt_regs in encoded fp
 on irq entry
To:     Chen Zhongjin <chenzhongjin@huawei.com>
Cc:     linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org,
        linux-perf-users@vger.kernel.org, paul.walmsley@sifive.com,
        palmer@dabbelt.com, aou@eecs.berkeley.edu, peterz@infradead.org,
        mingo@redhat.com, acme@kernel.org, mark.rutland@arm.com,
        alexander.shishkin@linux.intel.com, namhyung@kernel.org,
        jolsa@kernel.org, nsaenzju@redhat.com, frederic@kernel.org,
        changbin.du@intel.com, vincent.chen@sifive.com, ardb@kernel.org,
        mhiramat@kernel.org, rostedt@goodmis.org, keescook@chromium.org,
        catalin.marinas@arm.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Reviewed-by: Guo Ren <guoren@kernel.org>

ENCODE fp with BIT[0] is okay.

On Tue, Sep 20, 2022 at 11:15 PM Chen Zhongjin <chenzhongjin@huawei.com> wrote:
>
> To support stack unwinding at irq entry, the position of pt_regs saved
> on stack is nessesary. Because for some functions, compiler only push
> s0/fp on stack without ra.
>
> As the situation described in
> commit f766f77a74f5("riscv/stacktrace: Fix stack output without ra on the stack top")
>
> When irq happens there, the the function frame looks like:
>
> prev function   |       ...       |
>                 |                 |
> normal function +-----------------+
>                 | ra to prev      |
>                 | s0 of prev      |
>                 |       ...       |<-+
> leaf function   +-----------------+  |
>                 | s0 of normal    |  |
>                 | empty slot      |  |
> irq pt_regs     +-----------------+  |
>                 | epc (ra to leaf)|  |
>                 | ra  (ra to norm)|  |
>                 | ...             |  |
>                 | s0 of leaf      |--+
>                 | ...             |
>                 +-----------------+
>
> If the position of register in pt_regs is {epc, s0}, we can easily
> unwind from irq frame to leaf function, as normal functions do.
>
> However when unwinding from unwinding from leaf to normal, beacause
> (ra to norm) is saved in pt_regs, but not stackframe of leaf, we
> have to get pt_regs for that.
>
> To get pt_regs position on stack, we can save the encoded *pt_regs
> in s0, as x86 architecture did. Then we can get s0, epc and ra
> easily.
>
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> ---
>  arch/riscv/include/asm/frame.h | 45 ++++++++++++++++++++++++++++++++++
>  arch/riscv/kernel/entry.S      |  3 +++
>  2 files changed, 48 insertions(+)
>  create mode 100644 arch/riscv/include/asm/frame.h
>
> diff --git a/arch/riscv/include/asm/frame.h b/arch/riscv/include/asm/frame.h
> new file mode 100644
> index 000000000000..2a1f45cf3a4e
> --- /dev/null
> +++ b/arch/riscv/include/asm/frame.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_RISCV_FRAME_H
> +#define _ASM_RISCV_FRAME_H
> +
> +#include <asm/asm.h>
> +
> +#ifdef CONFIG_FRAME_POINTER
> +
> +#ifdef __ASSEMBLY__
> +
> +/*
> + * This is a sneaky trick to help the unwinder find pt_regs on the stack.  The
> + * frame pointer is replaced with an encoded pointer to pt_regs.  The encoding
> + * is just setting the LSB, which makes it an invalid stack address and is also
> + * a signal to the unwinder that it's a pt_regs pointer in disguise.
> + *
> + * This macro must be used when sp point to pt_regs
> + */
> +.macro ENCODE_FRAME_POINTER
> +       add s0, sp, 0x1
> +.endm
> +
> +#else /* !__ASSEMBLY__ */
> +
> +#define ENCODE_FRAME_POINTER                   \
> +       "add s0, sp, 0x1\n\t"
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#else /* !CONFIG_FRAME_POINTER */
> +
> +#ifdef __ASSEMBLY__
> +
> +.macro ENCODE_FRAME_POINTER ptregs_offset=0
> +.endm
> +
> +#else /* !__ASSEMBLY */
> +
> +#define ENCODE_FRAME_POINTER
> +
> +#endif /* !__ASSEMBLY */
> +
> +#endif /* CONFIG_FRAME_POINTER */
> +
> +#endif /* _ASM_RISCV_FRAME_H */
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index b9eda3fcbd6d..ecb15c7430b4 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -13,6 +13,7 @@
>  #include <asm/thread_info.h>
>  #include <asm/asm-offsets.h>
>  #include <asm/errata_list.h>
> +#include <asm/frame.h>
>
>  #if !IS_ENABLED(CONFIG_PREEMPTION)
>  .set resume_kernel, restore_all
> @@ -95,6 +96,8 @@ _save_context:
>         REG_S s4, PT_CAUSE(sp)
>         REG_S s5, PT_TP(sp)
>
> +       ENCODE_FRAME_POINTER
> +
>         /*
>          * Set the scratch register to 0, so that if a recursive exception
>          * occurs, the exception vector knows it came from the kernel
> --
> 2.17.1
>


-- 
Best Regards
 Guo Ren