On 1/6/22 10:31 AM, Mark Rutland wrote:
> On Mon, Jan 03, 2022 at 10:52:06AM -0600, madvenka@xxxxxxxxxxxxxxxxxxx wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@xxxxxxxxxxxxxxxxxxx>
>>
>> unwind_init() is currently a single function that initializes all of the
>> unwind state. Split it into the following functions and call them
>> appropriately:
>>
>> - unwind_init_regs() - initialize from regs passed by caller.
>>
>> - unwind_init_current() - initialize for the current task from the
>> caller of arch_stack_walk().
>>
>> - unwind_init_from_task() - initialize from the saved state of a
>> task other than the current task. In this case, the other
>> task must not be running.
>>
>> - unwind_init_common() - initialize fields that are common across
>> the above 3 cases.
>>
>> This is done so that specialized initialization can be added to each case
>> in the future.
>>
>> Signed-off-by: Madhavan T. Venkataraman <madvenka@xxxxxxxxxxxxxxxxxxx>
>> ---
>> arch/arm64/kernel/stacktrace.c | 50 +++++++++++++++++++++++++++-------
>> 1 file changed, 40 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index a1a7ff93b84f..bd797e3f7789 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -33,11 +33,8 @@
>> */
>>
>>
>> -static void unwind_init(struct unwind_state *state, unsigned long fp,
>> - unsigned long pc)
>> +static void unwind_init_common(struct unwind_state *state)
>> {
>> - state->fp = fp;
>> - state->pc = pc;
>> #ifdef CONFIG_KRETPROBES
>> state->kr_cur = NULL;
>> #endif
>> @@ -56,6 +53,40 @@ static void unwind_init(struct unwind_state *state, unsigned long fp,
>> state->prev_type = STACK_TYPE_UNKNOWN;
>> }
>>
>> +/*
>> + * TODO: document requirements here.
>> + */
>> +static inline void unwind_init_regs(struct unwind_state *state,
>> + struct pt_regs *regs)
>> +{
>> + state->fp = regs->regs[29];
>> + state->pc = regs->pc;
>> +}
>
> When I suggested this back in:
>
> https://lore.kernel.org/linux-arm-kernel/20211123193723.12112-1-madvenka@xxxxxxxxxxxxxxxxxxx/T/#md91fbfe08ceab2a02d9f5c326e17997786e53208
>
> ... my intent was that each unwind_init_from_*() helpers was the sole
> initializer of the structure, and the caller only had to call one function.
> That way it's not possible to construct an object with an erroneous combination
> of arguments because the prototype enforces the set of arguments, and the
> helper function can operate on a consistent snapshot of those arguments.
>
> So I'd much prefer that each of these helpers called unwind_init_common(),
> rather than leaving that to the caller to do. I don't mind if those pass
> arguments to unwind_init_common(), or explciitly initialize their respective
> fields, but I don' think the caller should have to care about unwind_init_common().
>
> I'd also prefer the unwind_init_from*() naming I'd previously suggested, so
> that it's clear which direction information is flowing.
>
OK. No problem.
>>
>> +
>> +/*
>> + * TODO: document requirements here.
>> + *
>> + * Note: this is always inlined, and we expect our caller to be a noinline
>> + * function, such that this starts from our caller's caller.
>> + */
>> +static __always_inline void unwind_init_current(struct unwind_state *state)
>> +{
>> + state->fp = (unsigned long)__builtin_frame_address(1);
>> + state->pc = (unsigned long)__builtin_return_address(0);
>> +}
>> +
>> +/*
>> + * TODO: document requirements here.
>> + *
>> + * The caller guarantees that the task is not running.
>> + */
>> +static inline void unwind_init_task(struct unwind_state *state,
>> + struct task_struct *task)
>> +{
>> + state->fp = thread_saved_fp(task);
>> + state->pc = thread_saved_pc(task);
>> +}
>> +
>> /*
>> * Unwind from one frame record (A) to the next frame record (B).
>> *
>> @@ -194,15 +225,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>> {
>> struct unwind_state state;
>>
>> + unwind_init_common(&state);
>
> As above, I really don't like that the caller has to call both the common
> initializer and a specialized initializer here.
>
OK. Will change this.
Thanks.
Madhavan
