[PATCH bpf-next 0/3] btf_dump fixes for s390

[PATCH bpf-next 0/3] btf_dump fixes for s390

[Ilya Leoshkevich]

Hi,

This series along with [1] and [2] fixes all the failures in the
btf_dump testsuite currently present on s390, in particular:

* [1] fixes intermittent build bug causing "failed to encode tag ..."
  * error messages.
* [2] fixes missing VAR entries on s390.
* Patch 1 disables Intel-specific code in a testcase.
* Patch 2 fixes an endianness-related bug.
* Patch 3 fixes an alignment-related bug.

[1] https://lore.kernel.org/bpf/20211012022521.399302-1-iii@linux.ibm.com/
[2] https://lore.kernel.org/bpf/20211012022637.399365-1-iii@linux.ibm.com/

Best regards,
Ilya

Ilya Leoshkevich (3):
  selftests/bpf: Use cpu_number only on arches that have it
  libbpf: Fix dumping big-endian bitfields
  libbpf: Fix dumping __int128

 tools/lib/bpf/btf_dump.c                          | 12 ++++++++----
 tools/testing/selftests/bpf/prog_tests/btf_dump.c |  2 ++
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.31.1

[PATCH bpf-next 1/3] selftests/bpf: Use cpu_number only on arches that have it

[Ilya Leoshkevich]

cpu_number exists only on Intel and aarch64, so skip the test involing
it on other arches. An alternative would be to replace it with an
exported non-ifdefed primitive-typed percpu variable from the common
code, but there appears to be none.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/bpf/prog_tests/btf_dump.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
index 87f9df653e4e..12f457b6786d 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
@@ -778,8 +778,10 @@ static void test_btf_dump_struct_data(struct btf *btf, struct btf_dump *d,
 static void test_btf_dump_var_data(struct btf *btf, struct btf_dump *d,
 				   char *str)
 {
+#if defined(__i386__) || defined(__x86_64__) || defined(__aarch64__)
 	TEST_BTF_DUMP_VAR(btf, d, NULL, str, "cpu_number", int, BTF_F_COMPACT,
 			  "int cpu_number = (int)100", 100);
+#endif
 	TEST_BTF_DUMP_VAR(btf, d, NULL, str, "cpu_profile_flip", int, BTF_F_COMPACT,
 			  "static int cpu_profile_flip = (int)2", 2);
 }
-- 
2.31.1

[PATCH bpf-next 2/3] libbpf: Fix dumping big-endian bitfields

[Ilya Leoshkevich]

On big-endian arches not only bytes, but also bits are numbered in
reverse order (see e.g. S/390 ELF ABI Supplement, but this is also true
for other big-endian arches as well).

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/btf_dump.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index ad6df97295ae..ab45771d0cb4 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -1577,14 +1577,15 @@ static int btf_dump_get_bitfield_value(struct btf_dump *d,
 	/* Bitfield value retrieval is done in two steps; first relevant bytes are
 	 * stored in num, then we left/right shift num to eliminate irrelevant bits.
 	 */
-	nr_copy_bits = bit_sz + bits_offset;
 	nr_copy_bytes = t->size;
 #if __BYTE_ORDER == __LITTLE_ENDIAN
 	for (i = nr_copy_bytes - 1; i >= 0; i--)
 		num = num * 256 + bytes[i];
+	nr_copy_bits = bit_sz + bits_offset;
 #elif __BYTE_ORDER == __BIG_ENDIAN
 	for (i = 0; i < nr_copy_bytes; i++)
 		num = num * 256 + bytes[i];
+	nr_copy_bits = nr_copy_bytes * 8 - bits_offset;
 #else
 # error "Unrecognized __BYTE_ORDER__"
 #endif
-- 
2.31.1

[PATCH bpf-next 3/3] libbpf: Fix dumping __int128

[Ilya Leoshkevich]

On s390 __int128 can be 8-byte aligned, therefore in libbpf will
occasionally consider variables of this type non-aligned and try to
dump them as a bitfield, which is supported for at most 64-bit
integers.

Fix by using the same trick as btf_dump_float_data(): copy non-aligned
values to the local buffer.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/lib/bpf/btf_dump.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index ab45771d0cb4..d8264c1762e8 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -1672,9 +1672,10 @@ static int btf_dump_int_data(struct btf_dump *d,
 {
 	__u8 encoding = btf_int_encoding(t);
 	bool sign = encoding & BTF_INT_SIGNED;
+	char buf[16] __aligned(16);
 	int sz = t->size;
 
-	if (sz == 0) {
+	if (sz == 0 || sz > sizeof(buf)) {
 		pr_warn("unexpected size %d for id [%u]\n", sz, type_id);
 		return -EINVAL;
 	}
@@ -1682,8 +1683,10 @@ static int btf_dump_int_data(struct btf_dump *d,
 	/* handle packed int data - accesses of integers not aligned on
 	 * int boundaries can cause problems on some platforms.
 	 */
-	if (!ptr_is_aligned(data, sz))
-		return btf_dump_bitfield_data(d, t, data, 0, 0);
+	if (!ptr_is_aligned(data, sz)) {
+		memcpy(buf, data, sz);
+		data = buf;
+	}
 
 	switch (sz) {
 	case 16: {
-- 
2.31.1

Re: [PATCH bpf-next 3/3] libbpf: Fix dumping __int128

[Andrii Nakryiko]

On Tue, Oct 12, 2021 at 4:32 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On s390 __int128 can be 8-byte aligned, therefore in libbpf will
> occasionally consider variables of this type non-aligned and try to
> dump them as a bitfield, which is supported for at most 64-bit
> integers.
>
> Fix by using the same trick as btf_dump_float_data(): copy non-aligned
> values to the local buffer.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/lib/bpf/btf_dump.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index ab45771d0cb4..d8264c1762e8 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -1672,9 +1672,10 @@ static int btf_dump_int_data(struct btf_dump *d,
>  {
>         __u8 encoding = btf_int_encoding(t);
>         bool sign = encoding & BTF_INT_SIGNED;
> +       char buf[16] __aligned(16);
>         int sz = t->size;
>
> -       if (sz == 0) {
> +       if (sz == 0 || sz > sizeof(buf)) {
>                 pr_warn("unexpected size %d for id [%u]\n", sz, type_id);
>                 return -EINVAL;
>         }
> @@ -1682,8 +1683,10 @@ static int btf_dump_int_data(struct btf_dump *d,
>         /* handle packed int data - accesses of integers not aligned on
>          * int boundaries can cause problems on some platforms.
>          */
> -       if (!ptr_is_aligned(data, sz))
> -               return btf_dump_bitfield_data(d, t, data, 0, 0);
> +       if (!ptr_is_aligned(data, sz)) {

I think ptr_is_aligned() logic is wrong. We should probably not assume
that __int128 has 16-byte alignment. Can you try fixing it by using
btf__align_of() API to get the natural alignment?

> +               memcpy(buf, data, sz);
> +               data = buf;
> +       }
>
>         switch (sz) {
>         case 16: {
> --
> 2.31.1
>

Re: [PATCH bpf-next 1/3] selftests/bpf: Use cpu_number only on arches that have it

[Andrii Nakryiko]

On Tue, Oct 12, 2021 at 4:51 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> cpu_number exists only on Intel and aarch64, so skip the test involing
> it on other arches. An alternative would be to replace it with an
> exported non-ifdefed primitive-typed percpu variable from the common
> code, but there appears to be none.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/testing/selftests/bpf/prog_tests/btf_dump.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> index 87f9df653e4e..12f457b6786d 100644
> --- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> +++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> @@ -778,8 +778,10 @@ static void test_btf_dump_struct_data(struct btf *btf, struct btf_dump *d,
>  static void test_btf_dump_var_data(struct btf *btf, struct btf_dump *d,
>                                    char *str)
>  {
> +#if defined(__i386__) || defined(__x86_64__) || defined(__aarch64__)
>         TEST_BTF_DUMP_VAR(btf, d, NULL, str, "cpu_number", int, BTF_F_COMPACT,
>                           "int cpu_number = (int)100", 100);
> +#endif

We are in the talks about supporting cross-compilation of selftests,
and this will be just another breakage that we'll have to undo.

Can we find some other variable that will be available on all
architectures? Maybe "runqueues"?

>         TEST_BTF_DUMP_VAR(btf, d, NULL, str, "cpu_profile_flip", int, BTF_F_COMPACT,
>                           "static int cpu_profile_flip = (int)2", 2);
>  }
> --
> 2.31.1
>

Re: [PATCH bpf-next 2/3] libbpf: Fix dumping big-endian bitfields

[Andrii Nakryiko]

On Tue, Oct 12, 2021 at 4:32 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On big-endian arches not only bytes, but also bits are numbered in
> reverse order (see e.g. S/390 ELF ABI Supplement, but this is also true
> for other big-endian arches as well).
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/lib/bpf/btf_dump.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index ad6df97295ae..ab45771d0cb4 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -1577,14 +1577,15 @@ static int btf_dump_get_bitfield_value(struct btf_dump *d,
>         /* Bitfield value retrieval is done in two steps; first relevant bytes are
>          * stored in num, then we left/right shift num to eliminate irrelevant bits.
>          */
> -       nr_copy_bits = bit_sz + bits_offset;
>         nr_copy_bytes = t->size;
>  #if __BYTE_ORDER == __LITTLE_ENDIAN
>         for (i = nr_copy_bytes - 1; i >= 0; i--)
>                 num = num * 256 + bytes[i];
> +       nr_copy_bits = bit_sz + bits_offset;
>  #elif __BYTE_ORDER == __BIG_ENDIAN
>         for (i = 0; i < nr_copy_bytes; i++)
>                 num = num * 256 + bytes[i];
> +       nr_copy_bits = nr_copy_bytes * 8 - bits_offset;

oh, I remember dealing with this in the context of pahole. Just one
nit, please use t->size instead of nr_copy_bytes, I think it will make
it a bit more explicit (nr_copy_bytes is logically mutable, though
only in little-endian case, but still).

>  #else
>  # error "Unrecognized __BYTE_ORDER__"
>  #endif
> --
> 2.31.1
>

Re: [PATCH bpf-next 1/3] selftests/bpf: Use cpu_number only on arches that have it

[Ilya Leoshkevich]

On Tue, 2021-10-12 at 05:56 +0200, Andrii Nakryiko wrote:
> On Tue, Oct 12, 2021 at 4:51 AM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > cpu_number exists only on Intel and aarch64, so skip the test
> > involing
> > it on other arches. An alternative would be to replace it with an
> > exported non-ifdefed primitive-typed percpu variable from the
> > common
> > code, but there appears to be none.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  tools/testing/selftests/bpf/prog_tests/btf_dump.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> > b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> > index 87f9df653e4e..12f457b6786d 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> > @@ -778,8 +778,10 @@ static void test_btf_dump_struct_data(struct
> > btf *btf, struct btf_dump *d,
> >  static void test_btf_dump_var_data(struct btf *btf, struct
> > btf_dump *d,
> >                                    char *str)
> >  {
> > +#if defined(__i386__) || defined(__x86_64__) ||
> > defined(__aarch64__)
> >         TEST_BTF_DUMP_VAR(btf, d, NULL, str, "cpu_number", int,
> > BTF_F_COMPACT,
> >                           "int cpu_number = (int)100", 100);
> > +#endif
> 
> We are in the talks about supporting cross-compilation of selftests,
> and this will be just another breakage that we'll have to undo.

Why would this break? Cross-compilation should define these macros
based on target, not build system.

> Can we find some other variable that will be available on all
> architectures? Maybe "runqueues"?

Wouldn't runqueues be pointless? We already have cpu_profile_flip. I
thought the idea here was to have something marked with
EXPORT_PER_CPU_SYMBOL.

> >         TEST_BTF_DUMP_VAR(btf, d, NULL, str, "cpu_profile_flip",
> > int, BTF_F_COMPACT,
> >                           "static int cpu_profile_flip = (int)2",
> > 2);
> >  }
> > --
> > 2.31.1

Re: [PATCH bpf-next 2/3] libbpf: Fix dumping big-endian bitfields

[Ilya Leoshkevich]

On Tue, 2021-10-12 at 06:03 +0200, Andrii Nakryiko wrote:
> On Tue, Oct 12, 2021 at 4:32 AM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > On big-endian arches not only bytes, but also bits are numbered in
> > reverse order (see e.g. S/390 ELF ABI Supplement, but this is also
> > true
> > for other big-endian arches as well).
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  tools/lib/bpf/btf_dump.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> > index ad6df97295ae..ab45771d0cb4 100644
> > --- a/tools/lib/bpf/btf_dump.c
> > +++ b/tools/lib/bpf/btf_dump.c
> > @@ -1577,14 +1577,15 @@ static int
> > btf_dump_get_bitfield_value(struct btf_dump *d,
> >         /* Bitfield value retrieval is done in two steps; first
> > relevant bytes are
> >          * stored in num, then we left/right shift num to eliminate
> > irrelevant bits.
> >          */
> > -       nr_copy_bits = bit_sz + bits_offset;
> >         nr_copy_bytes = t->size;
> >  #if __BYTE_ORDER == __LITTLE_ENDIAN
> >         for (i = nr_copy_bytes - 1; i >= 0; i--)
> >                 num = num * 256 + bytes[i];
> > +       nr_copy_bits = bit_sz + bits_offset;
> >  #elif __BYTE_ORDER == __BIG_ENDIAN
> >         for (i = 0; i < nr_copy_bytes; i++)
> >                 num = num * 256 + bytes[i];
> > +       nr_copy_bits = nr_copy_bytes * 8 - bits_offset;
> 
> oh, I remember dealing with this in the context of pahole. Just one
> nit, please use t->size instead of nr_copy_bytes, I think it will
> make
> it a bit more explicit (nr_copy_bytes is logically mutable, though
> only in little-endian case, but still).

Both sz and num_copy_bytes look redundant to be honest. What do you
think about dropping both completely and just using t->size everywhere
instead?

> 
> >  #else
> >  # error "Unrecognized __BYTE_ORDER__"
> >  #endif
> > --
> > 2.31.1
> > 

Re: [PATCH bpf-next 3/3] libbpf: Fix dumping __int128

[Ilya Leoshkevich]

On Tue, 2021-10-12 at 05:52 +0200, Andrii Nakryiko wrote:
> On Tue, Oct 12, 2021 at 4:32 AM Ilya Leoshkevich <iii@linux.ibm.com>
> wrote:
> > 
> > On s390 __int128 can be 8-byte aligned, therefore in libbpf will
> > occasionally consider variables of this type non-aligned and try to
> > dump them as a bitfield, which is supported for at most 64-bit
> > integers.
> > 
> > Fix by using the same trick as btf_dump_float_data(): copy non-
> > aligned
> > values to the local buffer.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  tools/lib/bpf/btf_dump.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> > index ab45771d0cb4..d8264c1762e8 100644
> > --- a/tools/lib/bpf/btf_dump.c
> > +++ b/tools/lib/bpf/btf_dump.c
> > @@ -1672,9 +1672,10 @@ static int btf_dump_int_data(struct btf_dump
> > *d,
> >  {
> >         __u8 encoding = btf_int_encoding(t);
> >         bool sign = encoding & BTF_INT_SIGNED;
> > +       char buf[16] __aligned(16);
> >         int sz = t->size;
> > 
> > -       if (sz == 0) {
> > +       if (sz == 0 || sz > sizeof(buf)) {
> >                 pr_warn("unexpected size %d for id [%u]\n", sz,
> > type_id);
> >                 return -EINVAL;
> >         }
> > @@ -1682,8 +1683,10 @@ static int btf_dump_int_data(struct btf_dump
> > *d,
> >         /* handle packed int data - accesses of integers not
> > aligned on
> >          * int boundaries can cause problems on some platforms.
> >          */
> > -       if (!ptr_is_aligned(data, sz))
> > -               return btf_dump_bitfield_data(d, t, data, 0, 0);
> > +       if (!ptr_is_aligned(data, sz)) {
> 
> I think ptr_is_aligned() logic is wrong. We should probably not
> assume
> that __int128 has 16-byte alignment. Can you try fixing it by using
> btf__align_of() API to get the natural alignment?

Ok, but this patch makes sense anyway, doesn't it? I can fix
ptr_is_aligned() separately.

> 
> > +               memcpy(buf, data, sz);
> > +               data = buf;
> > +       }
> > 
> >         switch (sz) {
> >         case 16: {
> > --
> > 2.31.1
> > 

Re: [PATCH bpf-next 1/3] selftests/bpf: Use cpu_number only on arches that have it

[Andrii Nakryiko]

On Tue, Oct 12, 2021 at 4:03 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Tue, 2021-10-12 at 05:56 +0200, Andrii Nakryiko wrote:
> > On Tue, Oct 12, 2021 at 4:51 AM Ilya Leoshkevich <iii@linux.ibm.com>
> > wrote:
> > >
> > > cpu_number exists only on Intel and aarch64, so skip the test
> > > involing
> > > it on other arches. An alternative would be to replace it with an
> > > exported non-ifdefed primitive-typed percpu variable from the
> > > common
> > > code, but there appears to be none.
> > >
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > ---
> > >  tools/testing/selftests/bpf/prog_tests/btf_dump.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> > > b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> > > index 87f9df653e4e..12f457b6786d 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/btf_dump.c
> > > @@ -778,8 +778,10 @@ static void test_btf_dump_struct_data(struct
> > > btf *btf, struct btf_dump *d,
> > >  static void test_btf_dump_var_data(struct btf *btf, struct
> > > btf_dump *d,
> > >                                    char *str)
> > >  {
> > > +#if defined(__i386__) || defined(__x86_64__) ||
> > > defined(__aarch64__)
> > >         TEST_BTF_DUMP_VAR(btf, d, NULL, str, "cpu_number", int,
> > > BTF_F_COMPACT,
> > >                           "int cpu_number = (int)100", 100);
> > > +#endif
> >
> > We are in the talks about supporting cross-compilation of selftests,
> > and this will be just another breakage that we'll have to undo.
>
> Why would this break? Cross-compilation should define these macros
> based on target, not build system.

Ok, then it should be good.

>
> > Can we find some other variable that will be available on all
> > architectures? Maybe "runqueues"?
>
> Wouldn't runqueues be pointless? We already have cpu_profile_flip. I
> thought the idea here was to have something marked with
> EXPORT_PER_CPU_SYMBOL.

No idea what the idea was, tbh.

>
> > >         TEST_BTF_DUMP_VAR(btf, d, NULL, str, "cpu_profile_flip",
> > > int, BTF_F_COMPACT,
> > >                           "static int cpu_profile_flip = (int)2",
> > > 2);
> > >  }
> > > --
> > > 2.31.1
>