Hello, On Wed, Mar 8, 2023 at 10:45 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Tue, Mar 7, 2023 at 9:02 PM Puranjay Mohan <puranjay12@xxxxxxxxx> wrote: > > > > Hi, > > Thanks for applying the series. > > > > On Wed, Mar 8, 2023 at 12:41 AM Andrii Nakryiko > > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > > > On Tue, Mar 7, 2023 at 4:04 AM Puranjay Mohan <puranjay12@xxxxxxxxx> wrote: > > > > > > > > Parsing of USDT arguments is architecture-specific; on arm it is > > > > relatively easy since registers used are r[0-10], fp, ip, sp, lr, > > > > pc. Format is slightly different compared to aarch64; forms are > > > > > > > > - "size @ [ reg, #offset ]" for dereferences, for example > > > > "-8 @ [ sp, #76 ]" ; " -4 @ [ sp ]" > > > > - "size @ reg" for register values; for example > > > > "-4@r0" > > > > - "size @ #value" for raw values; for example > > > > "-8@#1" > > > > > > > > Add support for parsing USDT arguments for ARM architecture. > > > > > > > > To test the above changes QEMU's virt[1] board with cortex-a15 > > > > CPU was used. libbpf-bootstrap's usdt example[2] was modified to attach > > > > to a test program with DTRACE_PROBE1/2/3/4... probes to test different > > > > combinations. > > > > > > > > [1] https://www.qemu.org/docs/master/system/arm/virt.html > > > > [2] https://github.com/libbpf/libbpf-bootstrap/blob/master/examples/c/usdt.bpf.c > > > > > > > > Signed-off-by: Puranjay Mohan <puranjay12@xxxxxxxxx> > > > > --- > > > > tools/lib/bpf/usdt.c | 80 ++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 80 insertions(+) > > > > > > > > diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c > > > > index 293b7a37f8a1..27a4589eda1c 100644 > > > > --- a/tools/lib/bpf/usdt.c > > > > +++ b/tools/lib/bpf/usdt.c > > > > @@ -1466,6 +1466,86 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec > > > > return len; > > > > } > > > > > > > > +#elif defined(__arm__) > > > > + > > > > +static int calc_pt_regs_off(const char *reg_name) > > > > +{ > > > > + static struct { > > > > + const char *name; > > > > + size_t pt_regs_off; > > > > + } reg_map[] = { > > > > + { "r0", offsetof(struct pt_regs, uregs[0]) }, > > > > + { "r1", offsetof(struct pt_regs, uregs[1]) }, > > > > + { "r2", offsetof(struct pt_regs, uregs[2]) }, > > > > + { "r3", offsetof(struct pt_regs, uregs[3]) }, > > > > + { "r4", offsetof(struct pt_regs, uregs[4]) }, > > > > + { "r5", offsetof(struct pt_regs, uregs[5]) }, > > > > + { "r6", offsetof(struct pt_regs, uregs[6]) }, > > > > + { "r7", offsetof(struct pt_regs, uregs[7]) }, > > > > + { "r8", offsetof(struct pt_regs, uregs[8]) }, > > > > + { "r9", offsetof(struct pt_regs, uregs[9]) }, > > > > + { "r10", offsetof(struct pt_regs, uregs[10]) }, > > > > + { "fp", offsetof(struct pt_regs, uregs[11]) }, > > > > + { "ip", offsetof(struct pt_regs, uregs[12]) }, > > > > + { "sp", offsetof(struct pt_regs, uregs[13]) }, > > > > + { "lr", offsetof(struct pt_regs, uregs[14]) }, > > > > + { "pc", offsetof(struct pt_regs, uregs[15]) }, > > > > + }; > > > > + int i; > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(reg_map); i++) { > > > > + if (strcmp(reg_name, reg_map[i].name) == 0) > > > > + return reg_map[i].pt_regs_off; > > > > + } > > > > + > > > > + pr_warn("usdt: unrecognized register '%s'\n", reg_name); > > > > + return -ENOENT; > > > > +} > > > > + > > > > +static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec *arg, int *arg_sz) > > > > +{ > > > > + char reg_name[16]; > > > > + int len, reg_off; > > > > + long off; > > > > + > > > > + if (sscanf(arg_str, " %d @ \[ %15[a-z0-9], #%ld ] %n", > > > > > > I've added space before , and applied to bpf-next. > > > > > > Thanks, it's a nice clean up and wider architecture support! > > > > > > BTW, I noticed that we don't support fp, ip, lr, and pc (only sp) for > > > __aarch64__, why such a difference between 32-bit and 64-bit arms? > > > > Actually, it is just a naming convention. > > in AARCH64 R29 is FP and R30 is LR, etc. so, they are decoded with > > that. Only SP is seperate. > > in ARM the register names are explicitly named. > > ah, ok, makes sense, thanks for explaining! > > > > > For ARM: https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#machine-registers > > For AARCH64: https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#machine-registers > > > > P.S.: For adding more BPF features for ARM, I feel the following > > things are remaining: > > 1. Adding CI for ARM: > > - Can you give me the steps needed to do this? > > I'm CC'ing Manu and Daniel, who are working on BPF CI. We are (mostly, > with the exception for s390x) using AWS to get runners on native > architecture. I'm not sure if AWS provides arm32 instances, so to > enable ARM32 in CI we'd need to resort to cross-compilation, probably. > But let's see if Manu and Daniel have specific suggestions. > Thanks for the details, this should be the first thing to do. If it is possible to cross-compile and run on qemu in the CI. AWS doesn't provide ARM32 instances. so we can't get runners on native architectures. > > > 2. Implement BPF trampoline for ARM. > > - This should be a little complicated and might need other dependent > > features that might not be available in arm. > > trampoline for ARM makes sense. Not sure if anyone else is working on > this or not, so feel free to create a separate email thread to discuss > plans and details. Sure, I will start a new thread to discuss this. > > > > 3.??? > > Assuming we have CI, there probably will be a bunch of clean up work > to make existing test work on 32-bit host architectures properly. Yes, the frequent problem here is tests using "long" variables in maps and pointers which are 32-bit/64-bit in ARM/BPF. > > > Also please see how much work it is to start supporting kfuncs for > arm32, see bpf_jit_supports_kfunc_call() and related infrastructure. > It should be a smaller undertaking compared to BPF trampoline. I looked at the patch series for "bpf: Support kernel function call in 32-bit ARM" https://lore.kernel.org/all/20221126094530.226629-1-yangjihong1@xxxxxxxxxx/ I think Yang Jihong is planning to send more versions of this series. CC'ing him on this thread. > > > > > > > > > > + arg_sz, reg_name, &off, &len) == 3) { > > > > + /* Memory dereference case, e.g., -4@[fp, #96] */ > > > > + arg->arg_type = USDT_ARG_REG_DEREF; > > > > + arg->val_off = off; > > > > + reg_off = calc_pt_regs_off(reg_name); > > > > + if (reg_off < 0) > > > > + return reg_off; > > > > + arg->reg_off = reg_off; > > > > + } else if (sscanf(arg_str, " %d @ \[ %15[a-z0-9] ] %n", arg_sz, reg_name, &len) == 2) { > > > > + /* Memory dereference case, e.g., -4@[sp] */ > > > > + arg->arg_type = USDT_ARG_REG_DEREF; > > > > + arg->val_off = 0; > > > > + reg_off = calc_pt_regs_off(reg_name); > > > > + if (reg_off < 0) > > > > + return reg_off; > > > > + arg->reg_off = reg_off; > > > > + } else if (sscanf(arg_str, " %d @ #%ld %n", arg_sz, &off, &len) == 2) { > > > > + /* Constant value case, e.g., 4@#5 */ > > > > + arg->arg_type = USDT_ARG_CONST; > > > > + arg->val_off = off; > > > > + arg->reg_off = 0; > > > > + } else if (sscanf(arg_str, " %d @ %15[a-z0-9] %n", arg_sz, reg_name, &len) == 2) { > > > > + /* Register read case, e.g., -8@r4 */ > > > > + arg->arg_type = USDT_ARG_REG; > > > > + arg->val_off = 0; > > > > + reg_off = calc_pt_regs_off(reg_name); > > > > + if (reg_off < 0) > > > > + return reg_off; > > > > + arg->reg_off = reg_off; > > > > + } else { > > > > + pr_warn("usdt: unrecognized arg #%d spec '%s'\n", arg_num, arg_str); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + return len; > > > > +} > > > > + > > > > #else > > > > > > > > static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec *arg, int *arg_sz) > > > > -- > > > > 2.39.1 > > > > > > > > > > > > -- > > Thanks and Regards > > > > Yours Truly, > > > > Puranjay Mohan -- Thanks and Regards Yours Truly, Puranjay Mohan
