On 2024/8/31 3:34, Andrii Nakryiko wrote:
On Thu, Aug 29, 2024 at 6:32 AM Pu Lehui <pulehui@xxxxxxxxxxxxxxx> wrote:
[SNIP]
-#define __PT_PARM1_SYSCALL_REG __PT_PARM1_REG +#define __PT_PARM1_SYSCALL_REG orig_a0 #define __PT_PARM2_SYSCALL_REG __PT_PARM2_REG #define __PT_PARM3_SYSCALL_REG __PT_PARM3_REG #define __PT_PARM4_SYSCALL_REG __PT_PARM4_REG #define __PT_PARM5_SYSCALL_REG __PT_PARM5_REG #define __PT_PARM6_SYSCALL_REG __PT_PARM6_REG +#define PT_REGS_PARM1_SYSCALL(x) PT_REGS_PARM1_CORE_SYSCALL(x) +#define PT_REGS_PARM1_CORE_SYSCALL(x) \ + BPF_CORE_READ((const struct pt_regs___riscv *)(x), __PT_PARM1_SYSCALL_REG)I feel like what we did for s390x is a bit suboptimal, so let's (try to) improve that and then do the same for RV64. What I mean is that PT_REGS_PARMn_SYSCALL macros are used to read pt_regs coming directly from context, right? In that case we don't need to pay the price of BPF_CORE_READ(), we can just access memory directly (but we still need CO-RE relocation!). So I think what we should do is 1) mark pt_regs___riscv {} with __attribute__((preserve_access_index)) so that normal field accesses are CO-RE-relocated 2) change PT_REGS_PARM1_SYSCALL(x) to be `((const struct_pt_regs___riscv *)(x))->orig_a0`, which will directly read memory 3) keep PT_REGS_PARM1_CORE_SYSCALL() as is But having written all the above, I'm not sure whether we allow CO-RE relocated direct context accesses (verifier might complain about modifying ctx register offset or something). So can you please check it either on s390 or RV64 and let me know? I'm not marking it as "Changes Requested" for that reason, because that might not work and we'll have to do BPF_CORE_READ().
Hi Andrii, thanks for your suggestion, it's really cool. I check that work for RV64, and send a new version:
https://lore.kernel.org/bpf/20240831041934.1629216-1-pulehui@xxxxxxxxxxxxxxx
#define __PT_RET_REG ra #define __PT_FP_REG s0 -- 2.34.1
