Re: [PATCH v2 1/3] selftests/hid: ensure we can compile the tests on kernels pre-6.3

[Benjamin Tissoires]

On Sep 11 2023, Eduard Zingerman wrote:
> On Fri, 2023-09-08 at 22:22 +0000, Justin Stitt wrote:
> > From: Benjamin Tissoires <bentiss@xxxxxxxxxx>
> > 
> > For the hid-bpf tests to compile, we need to have the definition of
> > struct hid_bpf_ctx. This definition is an internal one from the kernel
> > and it is supposed to be defined in the generated vmlinux.h.
> > 
> > This vmlinux.h header is generated based on the currently running kernel
> > or if the kernel was already compiled in the tree. If you just compile
> > the selftests without compiling the kernel beforehand and you are running
> > on a 6.2 kernel, you'll end up with a vmlinux.h without the hid_bpf_ctx
> > definition.
> > 
> > Use the clever trick from tools/testing/selftests/bpf/progs/bpf_iter.h
> > to force the definition of that symbol in case we don't find it in the
> > BTF and also add __attribute__((preserve_access_index)) to further
> > support CO-RE functionality for these tests.
> > 
> > Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx>
> > Signed-off-by: Benjamin Tissoires <bentiss@xxxxxxxxxx>
> > ---
> >  tools/testing/selftests/hid/progs/hid.c            |  3 --
> >  .../testing/selftests/hid/progs/hid_bpf_helpers.h  | 49 ++++++++++++++++++++++
> >  2 files changed, 49 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
> > index 88c593f753b5..1e558826b809 100644
> > --- a/tools/testing/selftests/hid/progs/hid.c
> > +++ b/tools/testing/selftests/hid/progs/hid.c
> > @@ -1,8 +1,5 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /* Copyright (c) 2022 Red hat */
> > -#include "vmlinux.h"
> > -#include <bpf/bpf_helpers.h>
> > -#include <bpf/bpf_tracing.h>
> >  #include "hid_bpf_helpers.h"
> >  
> >  char _license[] SEC("license") = "GPL";
> > diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > index 4fff31dbe0e7..ab3b18ba48c4 100644
> > --- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > +++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > @@ -5,6 +5,55 @@
> >  #ifndef __HID_BPF_HELPERS_H
> >  #define __HID_BPF_HELPERS_H
> >  
> > +/* "undefine" structs in vmlinux.h, because we "override" them below */
> 
> Hi Justin,
> 
> What you have here should work, however I still think that the trick
> with "___local" suffix I refer to in [1] is a bit less hacky, e.g.:
> 
>     enum hid_report_type___local { ... };
>     struct hid_bpf_ctx___local {
>        __u32 index;
>        const struct hid_device *hid; // this one should be in vmlinux.h with any config
>        __u32 allocated_size;
>        enum hid_report_type___local report_type;
>        union {
>            __s32 retval;
>            __s32 size;
>        };
>     } __attribute__((preserve_access_index));
>     
>     enum hid_class_request___local { ... };
>     enum hid_bpf_attach_flags___local { ... };
>     ...
>     extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx___local *ctx,
>                                   unsigned int offset,
> 
> 
> (sorry for being a bore, won't bring this up anymore).

No need to apologies for trying to make the code better :)

I specifically asked Justin to not use this because I intend the
examples to be here to use the same API in the selftests dir than users
in the wild. So if your suggestion definitely makes the header code
much better, it also means that everybody will start using `___local`
annotations for anything HID-BPF related, which is not what I want.

This header file is supposed to be included in the BPF, and IMO it's not
that important that we have the cleanest code, as long as the users have
the proper API.

Feel free to share your concerns :)

Cheers,
Benjamin

> 
> Thanks,
> Eduard
> 
> [1] https://lore.kernel.org/bpf/e99b4226bd450fedfebd4eb5c37054f032432b4f.camel@xxxxxxxxx/
> 
> > +#define hid_bpf_ctx hid_bpf_ctx___not_used
> > +#define hid_report_type hid_report_type___not_used
> > +#define hid_class_request hid_class_request___not_used
> > +#define hid_bpf_attach_flags hid_bpf_attach_flags___not_used
> > +#include "vmlinux.h"
> > +#undef hid_bpf_ctx
> > +#undef hid_report_type
> > +#undef hid_class_request
> > +#undef hid_bpf_attach_flags
> > +
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +#include <linux/const.h>
> > +
> > +enum hid_report_type {
> > +	HID_INPUT_REPORT		= 0,
> > +	HID_OUTPUT_REPORT		= 1,
> > +	HID_FEATURE_REPORT		= 2,
> > +
> > +	HID_REPORT_TYPES,
> > +};
> > +
> > +struct hid_bpf_ctx {
> > +	__u32 index;
> > +	const struct hid_device *hid;
> > +	__u32 allocated_size;
> > +	enum hid_report_type report_type;
> > +	union {
> > +		__s32 retval;
> > +		__s32 size;
> > +	};
> > +} __attribute__((preserve_access_index));
> > +
> > +enum hid_class_request {
> > +	HID_REQ_GET_REPORT		= 0x01,
> > +	HID_REQ_GET_IDLE		= 0x02,
> > +	HID_REQ_GET_PROTOCOL		= 0x03,
> > +	HID_REQ_SET_REPORT		= 0x09,
> > +	HID_REQ_SET_IDLE		= 0x0A,
> > +	HID_REQ_SET_PROTOCOL		= 0x0B,
> > +};
> > +
> > +enum hid_bpf_attach_flags {
> > +	HID_BPF_FLAG_NONE = 0,
> > +	HID_BPF_FLAG_INSERT_HEAD = _BITUL(0),
> > +	HID_BPF_FLAG_MAX,
> > +};
> > +
> >  /* following are kfuncs exported by HID for HID-BPF */
> >  extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
> >  			      unsigned int offset,
> > 
>