diff mbox series

[bpf-next,v3,2/2] libbpf: usdt arm arg parsing support

Message ID 20230307120440.25941-3-puranjay12@gmail.com (mailing list archive)
State Accepted
Commit 720d93b60aec22abcb2a5b6d8de472f3a50694b1
Delegated to: BPF
Headers show
Series libbpf: usdt arm arg parsing support | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 20 this patch: 20
netdev/cc_maintainers warning 5 maintainers not CCed: sdf@google.com haoluo@google.com john.fastabend@gmail.com kpsingh@kernel.org jolsa@kernel.org
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 20 this patch: 20
netdev/checkpatch warning WARNING: line length of 84 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 95 exceeds 80 columns WARNING: line length of 99 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-37 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 fail Logs for test_progs_no_alu32 on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for test_progs_parallel on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for test_progs_parallel on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for test_verifier on aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-38 success Logs for test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_progs_no_alu32_parallel on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for test_progs_parallel on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-7 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix

Commit Message

Puranjay Mohan March 7, 2023, 12:04 p.m. UTC
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@gmail.com>
---
 tools/lib/bpf/usdt.c | 80 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

Comments

Andrii Nakryiko March 7, 2023, 11:41 p.m. UTC | #1
On Tue, Mar 7, 2023 at 4:04 AM Puranjay Mohan <puranjay12@gmail.com> 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@gmail.com>
> ---
>  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?

> +                  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
>
Puranjay Mohan March 8, 2023, 5:01 a.m. UTC | #2
Hi,
Thanks for applying the series.

On Wed, Mar 8, 2023 at 12:41 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Mar 7, 2023 at 4:04 AM Puranjay Mohan <puranjay12@gmail.com> 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@gmail.com>
> > ---
> >  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.

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?
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.
3.???

>
> > +                  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
> >
Andrii Nakryiko March 8, 2023, 5:15 p.m. UTC | #3
On Tue, Mar 7, 2023 at 9:02 PM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> Hi,
> Thanks for applying the series.
>
> On Wed, Mar 8, 2023 at 12:41 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Mar 7, 2023 at 4:04 AM Puranjay Mohan <puranjay12@gmail.com> 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@gmail.com>
> > > ---
> > >  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.


> 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.


> 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.


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.

>
> >
> > > +                  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
Puranjay Mohan March 8, 2023, 6:36 p.m. UTC | #4
Hello,

On Wed, Mar 8, 2023 at 10:45 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Mar 7, 2023 at 9:02 PM Puranjay Mohan <puranjay12@gmail.com> wrote:
> >
> > Hi,
> > Thanks for applying the series.
> >
> > On Wed, Mar 8, 2023 at 12:41 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Tue, Mar 7, 2023 at 4:04 AM Puranjay Mohan <puranjay12@gmail.com> 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@gmail.com>
> > > > ---
> > > >  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@huawei.com/

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
diff mbox series

Patch

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",
+		   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)