[PATCH bpf-next 0/2] libbpf: usdt aarch64 support

[PATCH bpf-next 0/2] libbpf: usdt aarch64 support

[Alan Maguire]

USDT support [1] requires architecture-specific handling for translating
from argument strings describing each probe argument to appropriate values
that can be made available to the BPF program.  Determining value size,
whether it refers to a dereference (and if there is an offset from the
register value), a register value or a constant all has to be parsed
slightly differently for different architectures.

However a common representation is created for use within BPF programs
(via usdt.bpf.h), and patch 1 abstracts out the initialization of
struct usdt_arg_spec associated with the argument, rather than repeating
those steps for each architecture.

Patch 2 then adds aarch64-specific argument parsing.

[1] https://lore.kernel.org/bpf/20220404234202.331384-1-andrii@kernel.org/

Alan Maguire (2):
  libbpf: usdt: factor out common USDT arg handling
  libbpf: usdt aarch64 arg parsing support

 tools/lib/bpf/usdt.c | 134 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 87 insertions(+), 47 deletions(-)

-- 
1.8.3.1

[PATCH bpf-next 1/2] libbpf: usdt: factor out common USDT arg handling

[Alan Maguire]

Common code to initialize a struct usdt_arg_spec can be factored out
from arch-specific flavours of parse_usdt_arg(); signed size,
bitshift handling etc.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 tools/lib/bpf/usdt.c | 90 ++++++++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 49 deletions(-)

diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
index 30c495a..0677bbd 100644
--- a/tools/lib/bpf/usdt.c
+++ b/tools/lib/bpf/usdt.c
@@ -1170,6 +1170,31 @@ static int parse_usdt_spec(struct usdt_spec *spec, const struct usdt_note *note,
 
 /* Architecture-specific logic for parsing USDT argument location specs */
 
+#if defined(__x86_64__) || defined(__i386__) || defined(__s390x__)
+
+static int init_usdt_arg_spec(struct usdt_arg_spec *arg, enum usdt_arg_type arg_type, int arg_sz,
+			      __u64 val_off, int reg_off)
+{
+	if (reg_off < 0)
+		return reg_off;
+	arg->arg_type = arg_type;
+	arg->val_off = val_off;
+	arg->reg_off = reg_off;
+	arg->arg_signed = arg_sz < 0;
+	if (arg_sz < 0)
+		arg_sz = -arg_sz;
+
+	switch (arg_sz) {
+	case 1: case 2: case 4: case 8:
+		arg->arg_bitshift = 64 - arg_sz * 8;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+#endif
+
 #if defined(__x86_64__) || defined(__i386__)
 
 static int calc_pt_regs_off(const char *reg_name)
@@ -1220,52 +1245,32 @@ static int calc_pt_regs_off(const char *reg_name)
 static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec *arg)
 {
 	char *reg_name = NULL;
-	int arg_sz, len, reg_off;
+	int arg_sz, len, ret;
 	long off;
 
 	if (sscanf(arg_str, " %d @ %ld ( %%%m[^)] ) %n", &arg_sz, &off, &reg_name, &len) == 3) {
 		/* Memory dereference case, e.g., -4@-20(%rbp) */
-		arg->arg_type = USDT_ARG_REG_DEREF;
-		arg->val_off = off;
-		reg_off = calc_pt_regs_off(reg_name);
+		ret = init_usdt_arg_spec(arg, USDT_ARG_REG_DEREF, arg_sz, off,
+					 calc_pt_regs_off(reg_name));
 		free(reg_name);
-		if (reg_off < 0)
-			return reg_off;
-		arg->reg_off = reg_off;
 	} else if (sscanf(arg_str, " %d @ %%%ms %n", &arg_sz, &reg_name, &len) == 2) {
 		/* Register read case, e.g., -4@%eax */
-		arg->arg_type = USDT_ARG_REG;
-		arg->val_off = 0;
-
-		reg_off = calc_pt_regs_off(reg_name);
+		ret = init_usdt_arg_spec(arg, USDT_ARG_REG, arg_sz, 0,
+					 calc_pt_regs_off(reg_name));
 		free(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@$71 */
-		arg->arg_type = USDT_ARG_CONST;
-		arg->val_off = off;
-		arg->reg_off = 0;
+		ret = init_usdt_arg_spec(arg, USDT_ARG_CONST, arg_sz, off, 0);
 	} else {
 		pr_warn("usdt: unrecognized arg #%d spec '%s'\n", arg_num, arg_str);
 		return -EINVAL;
 	}
 
-	arg->arg_signed = arg_sz < 0;
-	if (arg_sz < 0)
-		arg_sz = -arg_sz;
-
-	switch (arg_sz) {
-	case 1: case 2: case 4: case 8:
-		arg->arg_bitshift = 64 - arg_sz * 8;
-		break;
-	default:
+	if (ret < 0) {
 		pr_warn("usdt: unsupported arg #%d (spec '%s') size: %d\n",
 			arg_num, arg_str, arg_sz);
-		return -EINVAL;
+		return ret;
 	}
-
 	return len;
 }
 
@@ -1276,51 +1281,38 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
 static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec *arg)
 {
 	unsigned int reg;
-	int arg_sz, len;
+	int arg_sz, len, ret;
 	long off;
 
 	if (sscanf(arg_str, " %d @ %ld ( %%r%u ) %n", &arg_sz, &off, &reg, &len) == 3) {
 		/* Memory dereference case, e.g., -2@-28(%r15) */
-		arg->arg_type = USDT_ARG_REG_DEREF;
-		arg->val_off = off;
 		if (reg > 15) {
 			pr_warn("usdt: unrecognized register '%%r%u'\n", reg);
 			return -EINVAL;
 		}
-		arg->reg_off = offsetof(user_pt_regs, gprs[reg]);
+		ret = init_usdt_arg_spec(arg, USDT_ARG_REG_DEREF, arg_sz, off,
+					 offsetof(user_pt_regs, gprs[reg]));
 	} else if (sscanf(arg_str, " %d @ %%r%u %n", &arg_sz, &reg, &len) == 2) {
 		/* Register read case, e.g., -8@%r0 */
-		arg->arg_type = USDT_ARG_REG;
-		arg->val_off = 0;
 		if (reg > 15) {
 			pr_warn("usdt: unrecognized register '%%r%u'\n", reg);
 			return -EINVAL;
 		}
-		arg->reg_off = offsetof(user_pt_regs, gprs[reg]);
+		ret = init_usdt_arg_spec(arg, USDT_ARG_REG, arg_sz, 0,
+					 offsetof(user_pt_regs, gprs[reg]));
 	} else if (sscanf(arg_str, " %d @ %ld %n", &arg_sz, &off, &len) == 2) {
 		/* Constant value case, e.g., 4@71 */
-		arg->arg_type = USDT_ARG_CONST;
-		arg->val_off = off;
-		arg->reg_off = 0;
+		ret = init_usdt_arg_spec(arg, USDT_ARG_CONST, arg_sz, off, 0);
 	} else {
 		pr_warn("usdt: unrecognized arg #%d spec '%s'\n", arg_num, arg_str);
 		return -EINVAL;
 	}
 
-	arg->arg_signed = arg_sz < 0;
-	if (arg_sz < 0)
-		arg_sz = -arg_sz;
-
-	switch (arg_sz) {
-	case 1: case 2: case 4: case 8:
-		arg->arg_bitshift = 64 - arg_sz * 8;
-		break;
-	default:
+	if (ret < 0) {
 		pr_warn("usdt: unsupported arg #%d (spec '%s') size: %d\n",
 			arg_num, arg_str, arg_sz);
-		return -EINVAL;
+		return ret;
 	}
-
 	return len;
 }
 
-- 
1.8.3.1

Re: [PATCH bpf-next 1/2] libbpf: usdt: factor out common USDT arg handling

[Andrii Nakryiko]

On Fri, Apr 8, 2022 at 3:53 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> Common code to initialize a struct usdt_arg_spec can be factored out
> from arch-specific flavours of parse_usdt_arg(); signed size,
> bitshift handling etc.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---

I understand that there is a bit of repetition between multiple
architectures, but I think this init_usdt_arg_spec() helper is
actually hindering understand. Can you please drop this refactoring
for now? It's just a few lines of code that you'll need to copy/paste
for argument bit shift handling, not a big deal, right?

>  tools/lib/bpf/usdt.c | 90 ++++++++++++++++++++++++----------------------------
>  1 file changed, 41 insertions(+), 49 deletions(-)
>

[...]

[PATCH bpf-next 2/2] libbpf: usdt aarch64 arg parsing support

[Alan Maguire]

Parsing of USDT arguments is architecture-specific; on aarch64 it is
relatively easy since registers used are x[0-31], sp.  Format is
slightly different compared to x86_64; forms are

- "size @ [ reg[,offset] ]" for dereferences, for example
  "-8 @ [ sp, 76 ]" ; " -4 @ [ sp ]"
- "size @ reg" for register values; for example
  "-4@x0"
- "size @ value" for raw values; for example
  "-8@1"

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 tools/lib/bpf/usdt.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
index 0677bbd..6165d40 100644
--- a/tools/lib/bpf/usdt.c
+++ b/tools/lib/bpf/usdt.c
@@ -1170,7 +1170,7 @@ static int parse_usdt_spec(struct usdt_spec *spec, const struct usdt_note *note,
 
 /* Architecture-specific logic for parsing USDT argument location specs */
 
-#if defined(__x86_64__) || defined(__i386__) || defined(__s390x__)
+#if defined(__x86_64__) || defined(__i386__) || defined(__s390x__) || defined(__aarch64__)
 
 static int init_usdt_arg_spec(struct usdt_arg_spec *arg, enum usdt_arg_type arg_type, int arg_sz,
 			      __u64 val_off, int reg_off)
@@ -1316,6 +1316,54 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
 	return len;
 }
 
+#elif defined(__aarch64__)
+
+static int calc_pt_regs_off(const char *reg_name)
+{
+	int reg_num;
+
+	if (sscanf(reg_name, "x%d", &reg_num) == 1) {
+		if (reg_num >= 0 && reg_num < 31)
+			return offsetof(struct user_pt_regs, regs[reg_num]);
+	} else if (strcmp(reg_name, "sp") == 0) {
+		return offsetof(struct user_pt_regs, sp);
+	}
+	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)
+{
+	char *reg_name = NULL;
+	int arg_sz, len, ret;
+	long off = 0;
+
+	if (sscanf(arg_str, " %d @ \[ %m[^,], %ld ] %n", &arg_sz, &reg_name, &off, &len) == 3 ||
+	    sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, &reg_name, &len) == 2) {
+		/* Memory dereference case, e.g., -4@[sp, 96], -4@[sp] */
+		ret = init_usdt_arg_spec(arg, USDT_ARG_REG_DEREF, arg_sz, off,
+					 calc_pt_regs_off(reg_name));
+		free(reg_name);
+	} else if (sscanf(arg_str, " %d @ %ld %n", &arg_sz, &off, &len) == 2) {
+		/* Constant value case, e.g., 4@5 */
+		ret = init_usdt_arg_spec(arg, USDT_ARG_CONST, arg_sz, off, 0);
+	} else if (sscanf(arg_str, " %d @ %ms %n", &arg_sz, &reg_name, &len) == 2) {
+		/* Register read case, e.g., -8@x4 */
+		ret = init_usdt_arg_spec(arg, USDT_ARG_REG, arg_sz, 0, calc_pt_regs_off(reg_name));
+		free(reg_name);
+	} else {
+		pr_warn("usdt: unrecognized arg #%d spec '%s'\n", arg_num, arg_str);
+		return -EINVAL;
+	}
+
+	if (ret < 0) {
+		pr_warn("usdt: unsupported arg #%d (spec '%s') size: %d\n",
+			arg_num, arg_str, arg_sz);
+		return ret;
+	}
+	return len;
+}
+
 #else
 
 static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec *arg)
-- 
1.8.3.1

Re: [PATCH bpf-next 2/2] libbpf: usdt aarch64 arg parsing support

[Andrii Nakryiko]

On Fri, Apr 8, 2022 at 3:53 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> Parsing of USDT arguments is architecture-specific; on aarch64 it is
> relatively easy since registers used are x[0-31], sp.  Format is
> slightly different compared to x86_64; forms are
>
> - "size @ [ reg[,offset] ]" for dereferences, for example
>   "-8 @ [ sp, 76 ]" ; " -4 @ [ sp ]"
> - "size @ reg" for register values; for example
>   "-4@x0"
> - "size @ value" for raw values; for example
>   "-8@1"
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  tools/lib/bpf/usdt.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
> index 0677bbd..6165d40 100644
> --- a/tools/lib/bpf/usdt.c
> +++ b/tools/lib/bpf/usdt.c
> @@ -1170,7 +1170,7 @@ static int parse_usdt_spec(struct usdt_spec *spec, const struct usdt_note *note,
>
>  /* Architecture-specific logic for parsing USDT argument location specs */
>
> -#if defined(__x86_64__) || defined(__i386__) || defined(__s390x__)
> +#if defined(__x86_64__) || defined(__i386__) || defined(__s390x__) || defined(__aarch64__)
>
>  static int init_usdt_arg_spec(struct usdt_arg_spec *arg, enum usdt_arg_type arg_type, int arg_sz,
>                               __u64 val_off, int reg_off)
> @@ -1316,6 +1316,54 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
>         return len;
>  }
>
> +#elif defined(__aarch64__)
> +
> +static int calc_pt_regs_off(const char *reg_name)
> +{
> +       int reg_num;
> +
> +       if (sscanf(reg_name, "x%d", &reg_num) == 1) {
> +               if (reg_num >= 0 && reg_num < 31)
> +                       return offsetof(struct user_pt_regs, regs[reg_num]);
> +       } else if (strcmp(reg_name, "sp") == 0) {
> +               return offsetof(struct user_pt_regs, sp);
> +       }
> +       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)
> +{
> +       char *reg_name = NULL;
> +       int arg_sz, len, ret;
> +       long off = 0;
> +
> +       if (sscanf(arg_str, " %d @ \[ %m[^,], %ld ] %n", &arg_sz, &reg_name, &off, &len) == 3 ||
> +           sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, &reg_name, &len) == 2) {

I'm not sure about the behavior here w.r.t. reg_name and memory
allocation. What if first sscanf() matches reg_name but fails at %ld,
will reg_name be allocated and then second sscanf() will reallocate
(and thus we'll have a memory leak).

We might have similar problems in other implementations, actually...

Either way, came here to ask to split two sscanfs into two separate
branches, so that we have a clear linear pattern. One sscanf, handle
it if successful, otherwise move on to next case.

Also a question about [a-z0-9] for register in one case and [^,] in
another. Should the first one be [a-z0-9] as well?

> +               /* Memory dereference case, e.g., -4@[sp, 96], -4@[sp] */
> +               ret = init_usdt_arg_spec(arg, USDT_ARG_REG_DEREF, arg_sz, off,
> +                                        calc_pt_regs_off(reg_name));
> +               free(reg_name);
> +       } else if (sscanf(arg_str, " %d @ %ld %n", &arg_sz, &off, &len) == 2) {
> +               /* Constant value case, e.g., 4@5 */
> +               ret = init_usdt_arg_spec(arg, USDT_ARG_CONST, arg_sz, off, 0);
> +       } else if (sscanf(arg_str, " %d @ %ms %n", &arg_sz, &reg_name, &len) == 2) {
> +               /* Register read case, e.g., -8@x4 */
> +               ret = init_usdt_arg_spec(arg, USDT_ARG_REG, arg_sz, 0, calc_pt_regs_off(reg_name));
> +               free(reg_name);
> +       } else {
> +               pr_warn("usdt: unrecognized arg #%d spec '%s'\n", arg_num, arg_str);
> +               return -EINVAL;
> +       }
> +
> +       if (ret < 0) {
> +               pr_warn("usdt: unsupported arg #%d (spec '%s') size: %d\n",
> +                       arg_num, arg_str, arg_sz);
> +               return ret;
> +       }
> +       return len;
> +}
> +
>  #else
>
>  static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec *arg)
> --
> 1.8.3.1
>

Re: [PATCH bpf-next 2/2] libbpf: usdt aarch64 arg parsing support

[Alan Maguire]

On Mon, 11 Apr 2022, Andrii Nakryiko wrote:

> On Fri, Apr 8, 2022 at 3:53 PM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > Parsing of USDT arguments is architecture-specific; on aarch64 it is
> > relatively easy since registers used are x[0-31], sp.  Format is
> > slightly different compared to x86_64; forms are
> >
> > - "size @ [ reg[,offset] ]" for dereferences, for example
> >   "-8 @ [ sp, 76 ]" ; " -4 @ [ sp ]"
> > - "size @ reg" for register values; for example
> >   "-4@x0"
> > - "size @ value" for raw values; for example
> >   "-8@1"
> >
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > ---
> >  tools/lib/bpf/usdt.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/usdt.c b/tools/lib/bpf/usdt.c
> > index 0677bbd..6165d40 100644
> > --- a/tools/lib/bpf/usdt.c
> > +++ b/tools/lib/bpf/usdt.c
> > @@ -1170,7 +1170,7 @@ static int parse_usdt_spec(struct usdt_spec *spec, const struct usdt_note *note,
> >
> >  /* Architecture-specific logic for parsing USDT argument location specs */
> >
> > -#if defined(__x86_64__) || defined(__i386__) || defined(__s390x__)
> > +#if defined(__x86_64__) || defined(__i386__) || defined(__s390x__) || defined(__aarch64__)
> >
> >  static int init_usdt_arg_spec(struct usdt_arg_spec *arg, enum usdt_arg_type arg_type, int arg_sz,
> >                               __u64 val_off, int reg_off)
> > @@ -1316,6 +1316,54 @@ static int parse_usdt_arg(const char *arg_str, int arg_num, struct usdt_arg_spec
> >         return len;
> >  }
> >
> > +#elif defined(__aarch64__)
> > +
> > +static int calc_pt_regs_off(const char *reg_name)
> > +{
> > +       int reg_num;
> > +
> > +       if (sscanf(reg_name, "x%d", &reg_num) == 1) {
> > +               if (reg_num >= 0 && reg_num < 31)
> > +                       return offsetof(struct user_pt_regs, regs[reg_num]);
> > +       } else if (strcmp(reg_name, "sp") == 0) {
> > +               return offsetof(struct user_pt_regs, sp);
> > +       }
> > +       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)
> > +{
> > +       char *reg_name = NULL;
> > +       int arg_sz, len, ret;
> > +       long off = 0;
> > +
> > +       if (sscanf(arg_str, " %d @ \[ %m[^,], %ld ] %n", &arg_sz, &reg_name, &off, &len) == 3 ||
> > +           sscanf(arg_str, " %d @ \[ %m[a-z0-9] ] %n", &arg_sz, &reg_name, &len) == 2) {
> 
> I'm not sure about the behavior here w.r.t. reg_name and memory
> allocation. What if first sscanf() matches reg_name but fails at %ld,
> will reg_name be allocated and then second sscanf() will reallocate
> (and thus we'll have a memory leak).
> 
> We might have similar problems in other implementations, actually...
> 
> Either way, came here to ask to split two sscanfs into two separate
> branches, so that we have a clear linear pattern. One sscanf, handle
> it if successful, otherwise move on to next case.
> 

good point; I'll separate the sscanfs into branches for v2.

> Also a question about [a-z0-9] for register in one case and [^,] in
> another. Should the first one be [a-z0-9] as well?
>

probably no harm, yep.

I'll drop the refactoring patch too; I was a bit worried I'd break
Ilya's s390 code anyhow. 

Thanks!

Alan