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

[Eduard Zingerman]

On Mon, 2023-09-11 at 15:39 +0200, Benjamin Tissoires wrote:
> 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 :)

Got it, thank you for explanation :)

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