Received-SPF: pass (google.com: domain of linux-kernel+bounces-197317-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240528-hid_bpf_struct_ops-v1-0-8c6663df27d8@kernel.org>
 <20240528-hid_bpf_struct_ops-v1-3-8c6663df27d8@kernel.org>
 <CAADnVQJbc4uVmhh+ABPMBGFjTYwQQp3wGLeTBYEUWti3R8V8aw@mail.gmail.com> <bbmrcmm2tsbxm3f7bdjbdprqt62hkrw3d7w4w4taxaxx24rlu5@iu5krfvhcaru>
In-Reply-To: <bbmrcmm2tsbxm3f7bdjbdprqt62hkrw3d7w4w4taxaxx24rlu5@iu5krfvhcaru>
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date: Fri, 31 May 2024 11:52:47 -0700
Message-ID: <CAADnVQJfNxdZebtu9f8a+JwXn0tJLhRK7bnG+dhcnb0BN2kOSw@mail.gmail.com>
Subject: Re: [PATCH HID 03/13] HID: bpf: implement HID-BPF through bpf_struct_ops
To: Benjamin Tissoires <bentiss@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>, Jiri Kosina <jikos@kernel.org>, Jonathan Corbet <corbet@lwn.net>, 
	Alexei Starovoitov <ast@kernel.org>, 
	"open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, 
	bpf <bpf@vger.kernel.org>, 
	"open list:HID CORE LAYER" <linux-input@vger.kernel.org>, 
	"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, May 29, 2024 at 12:38=E2=80=AFAM Benjamin Tissoires <bentiss@kernel=
org> wrote:
>
> > Take a look at include/linux/bpf_mprog.h
> > I suspect it might be useful here too.
>
> From the quick look of it this is interesting, but for plain struct
> bpf_prog, right?
>
> So I would have to adapt this include for struct_ops?

yeah. It needs work to be adapted to work with struct-ops.
I mentioned it mainly from uapi perspective.
BPF_F_BEFORE, BPF_F_AFTER, BPF_F_REPLACE.
Would be good to keep things consistent.

> > > @@ -112,6 +113,63 @@ struct hid_ops {
> > >
> > >  extern struct hid_ops *hid_ops;
> > >
> > > +/**
> > > + * struct hid_bpf_ops - A BPF struct_ops of callbacks allowing to at=
tach HID-BPF
> > > + *                     programs to a HID device
> > > + * @hid_id: the HID uniq ID to attach to. This is writeable before `=
`load()``, and
> > > + *         cannot be changed after
> > > + * @flags: &enum hid_bpf_attach_flags to assign flags before ``load(=
)``.
> > > + *        Writeable only before ``load()``
> > > + */
> > > +struct hid_bpf_ops {
> > > +       /* hid_id needs to stay first so we can easily change it
> > > +        * from userspace.
> > > +        */
> >
> > hmm.
> > patch 5 just does:
> > +       skel->struct_ops.mouse_invert->hid_id =3D hid_id;
> >
> > I don't see a reason why it needs to be first.
>
> See the selftests (and my external loader, udev-hid-bpf):
>
>                 map =3D bpf_object__find_map_by_name(*self->skel->skeleto=
n->obj,
>                                                           programs[i].nam=
e + 4);
>                 ASSERT_OK_PTR(map) TH_LOG("can not find struct_ops by nam=
e '%s'",
>                                           programs[i].name + 4);
>
>                 /* hid_id is the first field of struct hid_bpf_ops */
>                 ops_hid_id =3D bpf_map__initial_value(map, NULL);
>                 ASSERT_OK_PTR(ops_hid_id) TH_LOG("unable to retrieve stru=
ct_ops data");
>
>                 *ops_hid_id =3D self->hid_id;
>
> Basically, I want to have to deal with .bpf.o and not the skeletons for
> the generic case. I want to be able to drop a file in /lib/firmware/hid
> and have udev-hid-bpf load it without having to recompile udev-hid-bpf.

and with additional restriction of only one struct-ops prog per elf file,
right?
Otherwise multiple hid_bpf_ops will be in the same elf section and
the same map.

> I couldn't seem to be able to access the struct_ops BTF information from
> libbpf, so I decided to have hid_id and flags first and consider this to
> be ABI while I figure out how to get to the real offsets :)

I see. I guess it's ok for now.