diff mbox series

[v2,dwarves,2/4] btf_encoder: emit type tags for bpf_arena pointers

Message ID 20250212201552.1431219-3-ihor.solodrai@linux.dev (mailing list archive)
State Not Applicable
Headers show
Series btf_encoder: emit type tags for bpf_arena pointers | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Ihor Solodrai Feb. 12, 2025, 8:15 p.m. UTC 
When adding a kfunc prototype to BTF, check for the flags indicating
bpf_arena pointers and emit a type tag encoding
__attribute__((address_space(1))) for them. This also requires
updating BTF type ids in the btf_encoder_func_state, which is done as
a side effect in the tagging functions.

This feature depends on recent update in libbpf, supporting arbitrarty
attribute encoding [1].

[1] https://lore.kernel.org/bpf/20250130201239.1429648-1-ihor.solodrai@linux.dev/

Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
---
 btf_encoder.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 1 deletion(-)

Comments

Eduard Zingerman Feb. 19, 2025, 4:36 a.m. UTC | #1 
On Wed, 2025-02-12 at 12:15 -0800, Ihor Solodrai wrote:

[...]

> diff --git a/btf_encoder.c b/btf_encoder.c
> index 965e8f0..3cec106 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c

[...]

> +static int btf__tag_bpf_arena_ptr(struct btf *btf, int ptr_id)
> +{
> +	const struct btf_type *ptr;
> +	int tagged_type_id;
> +
> +	ptr = btf__type_by_id(btf, ptr_id);
> +	if (!btf_is_ptr(ptr))
> +		return -EINVAL;
> +
> +	tagged_type_id = btf__add_type_attr(btf, BPF_ARENA_ATTR, ptr->type);
> +	if (tagged_type_id < 0)
> +		return tagged_type_id;
> +
> +	return btf__add_ptr(btf, tagged_type_id);
> +}

I might be confused, but this is a bit strange.
The type constructed here is: ptr -> type_tag -> t.
However, address_space is an attribute of a pointer, not a pointed type.
I think that correct sequence should be: type_tag -> ptr -> t.
This would make libbpf emit C declaration as follows:

  void * __attribute__((address_space(1))) ptr;

Instead of current:

  void __attribute__((address_space(1))) * ptr;

clang generates identical IR for both declarations:

  @ptr = dso_local global ptr addrspace(1) null, align 8

Thus, imo, this function should be simplified as below:

  static int btf__tag_bpf_arena_ptr(struct btf *btf, int ptr_id)
  {
	const struct btf_type *ptr;

	ptr = btf__type_by_id(btf, ptr_id);
	if (!btf_is_ptr(ptr))
		return -EINVAL;

	return btf__add_type_attr(btf, BPF_ARENA_ATTR, ptr_id);
  }

[...]
Eduard Zingerman Feb. 19, 2025, 5:45 a.m. UTC | #2 
On Tue, 2025-02-18 at 20:36 -0800, Eduard Zingerman wrote:
> On Wed, 2025-02-12 at 12:15 -0800, Ihor Solodrai wrote:
> 
> [...]
> 
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index 965e8f0..3cec106 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> 
> [...]
> 
> > +static int btf__tag_bpf_arena_ptr(struct btf *btf, int ptr_id)
> > +{
> > +	const struct btf_type *ptr;
> > +	int tagged_type_id;
> > +
> > +	ptr = btf__type_by_id(btf, ptr_id);
> > +	if (!btf_is_ptr(ptr))
> > +		return -EINVAL;
> > +
> > +	tagged_type_id = btf__add_type_attr(btf, BPF_ARENA_ATTR, ptr->type);
> > +	if (tagged_type_id < 0)
> > +		return tagged_type_id;
> > +
> > +	return btf__add_ptr(btf, tagged_type_id);
> > +}
> 
> I might be confused, but this is a bit strange.
> The type constructed here is: ptr -> type_tag -> t.
> However, address_space is an attribute of a pointer, not a pointed type.
> I think that correct sequence should be: type_tag -> ptr -> t.
> This would make libbpf emit C declaration as follows:
> 
>   void * __attribute__((address_space(1))) ptr;
> 
> Instead of current:
> 
>   void __attribute__((address_space(1))) * ptr;
> 
> clang generates identical IR for both declarations:
> 
>   @ptr = dso_local global ptr addrspace(1) null, align 8
> 
> Thus, imo, this function should be simplified as below:
> 
>   static int btf__tag_bpf_arena_ptr(struct btf *btf, int ptr_id)
>   {
> 	const struct btf_type *ptr;
> 
> 	ptr = btf__type_by_id(btf, ptr_id);
> 	if (!btf_is_ptr(ptr))
> 		return -EINVAL;
> 
> 	return btf__add_type_attr(btf, BPF_ARENA_ATTR, ptr_id);
>   }
> 
> [...]
>

Ok, this comment can be ignored.
The following C code:

int foo(void * __attribute__((address_space(1))) ptr) {
  return ptr != 0;
}

does not compile, with the following error reported:

test3.c:1:49: error: parameter may not be qualified with an address space
    1 | int foo(void *__attribute__((address_space(1))) ptr) {
      |

While the following works:

int foo(void __attribute__((address_space(1))) *ptr) {
  return ptr != 0;
}

With the following IR generated:

define dso_local i32 @foo(ptr addrspace(1) noundef %0) #0 { ... }

Strange.
Ihor Solodrai Feb. 19, 2025, 6:03 p.m. UTC | #3 
On 2/18/25 9:45 PM, Eduard Zingerman wrote:
> On Tue, 2025-02-18 at 20:36 -0800, Eduard Zingerman wrote:
>> On Wed, 2025-02-12 at 12:15 -0800, Ihor Solodrai wrote:
>>
>> [...]
>>
>>> diff --git a/btf_encoder.c b/btf_encoder.c
>>> index 965e8f0..3cec106 100644
>>> --- a/btf_encoder.c
>>> +++ b/btf_encoder.c
>>
>> [...]
>>
>>> +static int btf__tag_bpf_arena_ptr(struct btf *btf, int ptr_id)
>>> +{
>>> +	const struct btf_type *ptr;
>>> +	int tagged_type_id;
>>> +
>>> +	ptr = btf__type_by_id(btf, ptr_id);
>>> +	if (!btf_is_ptr(ptr))
>>> +		return -EINVAL;
>>> +
>>> +	tagged_type_id = btf__add_type_attr(btf, BPF_ARENA_ATTR, ptr->type);
>>> +	if (tagged_type_id < 0)
>>> +		return tagged_type_id;
>>> +
>>> +	return btf__add_ptr(btf, tagged_type_id);
>>> +}
>>
>> I might be confused, but this is a bit strange.
>> The type constructed here is: ptr -> type_tag -> t.
>> However, address_space is an attribute of a pointer, not a pointed type.
>> I think that correct sequence should be: type_tag -> ptr -> t.
>> This would make libbpf emit C declaration as follows:
>>
>>   void * __attribute__((address_space(1))) ptr;
>>
>> Instead of current:
>>
>>   void __attribute__((address_space(1))) * ptr;

I was also confused about this.

The goal I had in mind is reproducing bpf_arena_* function
declarations, which are:

    void __arena* bpf_arena_alloc_pages(void *map, void __arena *addr, __u32 page_cnt,
    				    int node_id, __u64 flags) __ksym __weak;
    void bpf_arena_free_pages(void *map, void __arena *ptr, __u32 page_cnt) __ksym __weak;

AFAIU this is by design. From BTF documentation[1]:

    Currently, BTF_KIND_TYPE_TAG is only emitted for pointer types. It has
    the following btf type chain:

    ptr -> [type_tag]*
        -> [const | volatile | restrict | typedef]*
        -> base_type

    Basically, a pointer type points to zero or more type_tag, then zero
    or more const/volatile/restrict/typedef and finally the base type. The
    base type is one of int, ptr, array, struct, union, enum, func_proto
    and float types.

So yeah, unintuitively a tagged pointer in BTF is actually a pointer
to a tagged type. My guess is, it is so because of how C compilers
interpret type attributes, as evident by an example you've tested.

[1] https://docs.kernel.org/bpf/btf.html#btf-kind-type-tag

>>
>> clang generates identical IR for both declarations:
>>
>>   @ptr = dso_local global ptr addrspace(1) null, align 8
>>
>> Thus, imo, this function should be simplified as below:
>>
>>   static int btf__tag_bpf_arena_ptr(struct btf *btf, int ptr_id)
>>   {
>> 	const struct btf_type *ptr;
>>
>> 	ptr = btf__type_by_id(btf, ptr_id);
>> 	if (!btf_is_ptr(ptr))
>> 		return -EINVAL;
>>
>> 	return btf__add_type_attr(btf, BPF_ARENA_ATTR, ptr_id);
>>   }
>>
>> [...]
>>
>
> Ok, this comment can be ignored.
> The following C code:
>
> int foo(void * __attribute__((address_space(1))) ptr) {
>   return ptr != 0;
> }
>
> does not compile, with the following error reported:
>
> test3.c:1:49: error: parameter may not be qualified with an address space
>     1 | int foo(void *__attribute__((address_space(1))) ptr) {
>       |
>
> While the following works:
>
> int foo(void __attribute__((address_space(1))) *ptr) {
>   return ptr != 0;
> }
>
> With the following IR generated:
>
> define dso_local i32 @foo(ptr addrspace(1) noundef %0) #0 { ... }
>
> Strange.
>
diff mbox series

Patch

diff --git a/btf_encoder.c b/btf_encoder.c
index 965e8f0..3cec106 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -40,7 +40,13 @@ 
 #define BTF_SET8_KFUNCS		(1 << 0)
 #define BTF_KFUNC_TYPE_TAG	"bpf_kfunc"
 #define BTF_FASTCALL_TAG       "bpf_fastcall"
-#define KF_FASTCALL            (1 << 12)
+#define BPF_ARENA_ATTR         "address_space(1)"
+
+/* kfunc flags, see include/linux/btf.h in the kernel source */
+#define KF_FASTCALL   (1 << 12)
+#define KF_ARENA_RET  (1 << 13)
+#define KF_ARENA_ARG1 (1 << 14)
+#define KF_ARENA_ARG2 (1 << 15)
 
 struct btf_id_and_flag {
 	uint32_t id;
@@ -731,6 +737,78 @@  static int32_t btf_encoder__tag_type(struct btf_encoder *encoder, uint32_t tag_t
 	return encoder->type_id_off + tag_type;
 }
 
+#if LIBBPF_MAJOR_VERSION >= 1 && LIBBPF_MINOR_VERSION >= 6
+static int btf__tag_bpf_arena_ptr(struct btf *btf, int ptr_id)
+{
+	const struct btf_type *ptr;
+	int tagged_type_id;
+
+	ptr = btf__type_by_id(btf, ptr_id);
+	if (!btf_is_ptr(ptr))
+		return -EINVAL;
+
+	tagged_type_id = btf__add_type_attr(btf, BPF_ARENA_ATTR, ptr->type);
+	if (tagged_type_id < 0)
+		return tagged_type_id;
+
+	return btf__add_ptr(btf, tagged_type_id);
+}
+
+static int btf__tag_bpf_arena_arg(struct btf *btf, struct btf_encoder_func_state *state, int idx)
+{
+	int id;
+
+	if (state->nr_parms <= idx)
+		return -EINVAL;
+
+	id = btf__tag_bpf_arena_ptr(btf, state->parms[idx].type_id);
+	if (id < 0) {
+		btf__log_err(btf, BTF_KIND_TYPE_TAG, BPF_ARENA_ATTR, true, id,
+			"Error adding BPF_ARENA_ATTR for an argument of kfunc '%s'", state->elf->name);
+		return id;
+	}
+	state->parms[idx].type_id = id;
+
+	return id;
+}
+
+static int btf__add_bpf_arena_type_tags(struct btf *btf, struct btf_encoder_func_state *state)
+{
+	uint32_t flags = state->elf->kfunc_flags;
+	int ret_type_id;
+	int err;
+
+	if (KF_ARENA_RET & flags) {
+		ret_type_id = btf__tag_bpf_arena_ptr(btf, state->ret_type_id);
+		if (ret_type_id < 0) {
+			btf__log_err(btf, BTF_KIND_TYPE_TAG, BPF_ARENA_ATTR, true, ret_type_id,
+				"Error adding BPF_ARENA_ATTR for return type of kfunc '%s'", state->elf->name);
+			return ret_type_id;
+		}
+		state->ret_type_id = ret_type_id;
+	}
+
+	if (KF_ARENA_ARG1 & flags) {
+		err = btf__tag_bpf_arena_arg(btf, state, 0);
+		if (err < 0)
+			return err;
+	}
+
+	if (KF_ARENA_ARG2 & flags) {
+		err = btf__tag_bpf_arena_arg(btf, state, 1);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+#endif // LIBBPF_MAJOR_VERSION >= 1 && LIBBPF_MINOR_VERSION >= 6
+
+static inline bool is_kfunc_state(struct btf_encoder_func_state *state)
+{
+	return state && state->elf && state->elf->kfunc;
+}
+
 static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct ftype *ftype,
 					   struct btf_encoder_func_state *state)
 {
@@ -744,6 +822,12 @@  static int32_t btf_encoder__add_func_proto(struct btf_encoder *encoder, struct f
 
 	assert(ftype != NULL || state != NULL);
 
+#if LIBBPF_MAJOR_VERSION >= 1 && LIBBPF_MINOR_VERSION >= 6
+	if (is_kfunc_state(state) && encoder->tag_kfuncs)
+		if (btf__add_bpf_arena_type_tags(encoder->btf, state) < 0)
+			return -1;
+#endif
+
 	/* add btf_type for func_proto */
 	if (ftype) {
 		btf = encoder->btf;