Received-SPF: pass (google.com: domain of linux-kernel+bounces-103456-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: =?utf-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>
To: Puranjay Mohan <puranjay12@gmail.com>, Mark Rutland
 <mark.rutland@arm.com>, Andy Chiu <andy.chiu@sifive.com>
Cc: Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt
 <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, Steven Rostedt
 <rostedt@goodmis.org>, Masami
 Hiramatsu <mhiramat@kernel.org>, Sami Tolvanen <samitolvanen@google.com>,
 Guo Ren <guoren@kernel.org>, Ley Foon Tan <leyfoon.tan@starfivetech.com>,
 Deepak Gupta <debug@rivosinc.com>, Sia Jee Heng
 <jeeheng.sia@starfivetech.com>, Bjorn Topel <bjorn@rivosinc.com>, Song
 Shuai <suagrfillet@gmail.com>, Cl'ement L'eger <cleger@rivosinc.com>, Al
 Viro <viro@zeniv.linux.org.uk>, Jisheng Zhang <jszhang@kernel.org>,
 linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
 linux-trace-kernel@vger.kernel.org, Robbin Ehn <rehn@rivosinc.com>
Subject: Re: [RFC PATCH] riscv: Implement HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS
In-Reply-To: <mb61pplvw6grf.fsf@gmail.com>
References: <20240306165904.108141-1-puranjay12@gmail.com>
 <87ttlhdeqb.fsf@all.your.base.are.belong.to.us>
 <ZfBbxPDd0rz6FN2T@FVFF77S0Q05N>
 <8734suqsth.fsf@all.your.base.are.belong.to.us>
 <mb61pplvw6grf.fsf@gmail.com>
Date: Thu, 14 Mar 2024 16:07:33 +0100
Message-ID: <87zfv0onre.fsf@all.your.base.are.belong.to.us>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Puranjay Mohan <puranjay12@gmail.com> writes:

> Bj=C3=B6rn T=C3=B6pel <bjorn@kernel.org> writes:
>
>>
>> Hmm, depending on RISC-V's CMODX path, the pro/cons CALL_OPS vs dynamic
>> trampolines changes quite a bit.
>>
>> The more I look at the pains of patching two instruction ("split
>> immediates"), the better "patch data" + one insn patching look.
>
> I was looking at how dynamic trampolines would be implemented for RISC-V.
>
> With CALL-OPS we need to patch the auipc+jalr at function entry only, the
> ops pointer above the function can be patched atomically.
>
> With a dynamic trampoline we need a auipc+jalr pair at function entry to =
jump
> to the trampoline and then another auipc+jalr pair to jump from trampolin=
e to
> ops->func. When the ops->func is modified, we would need to update the
> auipc+jalr at in the trampoline.
>
> So, I am not sure how to move forward here, CALL-OPS or Dynamic trampolin=
es?

Yeah. Honestly, we need to figure out the patching story prior
choosing the path, so let's start there.

After reading Mark's reply, and discussing with OpenJDK folks (who does
the most crazy text patching on all platforms), having to patch multiple
instructions (where the address materialization is split over multiple
instructions) is a no-go. It's just a too big can of worms. So, if we
can only patch one insn, it's CALL_OPS.

A couple of options (in addition to Andy's), and all require a
per-function landing address ala CALL_OPS) tweaking what Mark is doing
on Arm (given the poor branch range).

..and maybe we'll get RISC-V rainbows/unicorns in the future getting
better reach (full 64b! ;-)).

A) Use auipc/jalr, only patch jalr to take us to a common
   dispatcher/trampoline
=20=20
 | <func_trace_target_data_8B> # probably on a data cache-line !=3D func .t=
ext to avoid ping-pong
 | ...
 | func:
 |   ...make sure ra isn't messed up...
 |   aupic
 |   nop <=3D> jalr # Text patch point -> common_dispatch
 |   ACTUAL_FUNC
 |=20
 | common_dispatch:
 |   load <func_trace_target_data_8B> based on ra
 |   jalr
 |   ...

The auipc is never touched, and will be overhead. Also, we need a mv to
store ra in a scratch register as well -- like Arm. We'll have two insn
per-caller overhead for a disabled caller.

B) Use jal, which can only take us +/-1M, and requires multiple
   dispatchers (and tracking which one to use, and properly distribute
   them. Ick.)

 | <func_trace_target_data_8B> # probably on a data cache-line !=3D func .t=
ext to avoid ping-pong
 | ...
 | func:
 |   ...make sure ra isn't messed up...
 |   nop <=3D> jal # Text patch point -> within_1M_to_func_dispatch
 |   ACTUAL_FUNC
 |=20
 | within_1M_to_func_dispatch:
 |   load <func_trace_target_data_8B> based on ra
 |   jalr

C) Use jal, which can only take us +/-1M, and use a per-function
   trampoline requires multiple dispatchers (and tracking which one to
   use). Blows up text size A LOT.

 | <func_trace_target_data_8B> # somewhere, but probably on a different cac=
heline than the .text to avoid ping-ongs
 | ...
 | per_func_dispatch
 |   load <func_trace_target_data_8B> based on ra
 |   jalr
 | func:
 |   ...make sure ra isn't messed up...
 |   nop <=3D> jal # Text patch point -> per_func_dispatch
 |   ACTUAL_FUNC

It's a bit sad that we'll always have to have a dispatcher/trampoline,
but it's still better than stop_machine(). (And we'll need a fencei IPI
as well, but only one. ;-))

Today, I'm leaning towards A (which is what Mark suggested, and also
Robbin).. Any other options?

[Now how do we implement OPTPROBES? I'm kidding. ;-)]


Bj=C3=B6rn