On Sun, 30 May 2021 at 01:51, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Fri, May 28, 2021 at 1:30 AM Lorenz Bauer <lmb@xxxxxxxxxxxxxx> wrote:
> >
> > On Wed, 26 May 2021 at 19:34, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
> > >
> > > So I did a bit of investigation and gathered struct pt_regs
> > > definitions from all the "supported" architectures in bpf_tracing.h.
> > > I'll leave it here for further reference.
> > >
> > > static unsigned long bpf_pt_regs_parm1(const void *regs)
> > > {
> > > if (___arch_is_x86)
> > > return ((struct pt_regs___x86 *)regs)->di;
> > > else if (___arch_is_s390)
> > > return ((struct pt_regs___s390 *)regs)->gprs[2];
> > > else if (___arch_is_powerpc)
> > > return ((struct pt_regs___powerpc *)regs)->gpr[3];
> > > else
> > > while(1); /* need some better way to force BPF verification failure */
> > > }
> > >
> > > And so on for other architectures and other helpers, you should get
> > > the idea from the above.
> >
> > The idea of basing this on unique fields in types is neat, the
> > downside I see is that we encode the logic in the BPF bitstream. If in
> > the future struct pt_regs is changed, code breaks and we can't do much
>
> If pt_regs fields are renamed all PT_REGS-related stuff, provided by
> libbpf in bpf_tracing.h will break as well and will require
> re-compilation of BPF application.
I'm thinking more along the lines of, if a PT_REGS definition changes
so that the unique field isn't unique anymore. The BPF is still valid,
but the logic that determines the platform isn't.
> This piece of code is going to be
> part of the same bpf_tracing.h, so if something changes in newer
> kernel version, libbpf will accommodate that in the latest version.
> You'd still need to re-compile your BPF application, but I don't see
> how that's avoidable even with your proposal.
>
> > about it. What if instead we replace ___arch_is_x86, etc. with a
> > .kconfig style constant load? The platform detection logic can then
> > live in libbpf or cilium/ebpf and can be evolved if needed. Instead of
>
> That might be worthwhile to do (similarly to how we have a special
> LINUX_KERNEL_VERSION extern) regardless. But again, detection of the
> architecture is just one part. Once you know the architecture, you are
> still relying on knowing pt_regs field names to extract the data. So
> if anything changes about that, you'd need to update bpf_tracing.h and
> re-compile.
Yes. It'd be nice to fix that, but I don't see how to do that in a
generic fashion. So I'd deal with it when it happens.
> > > How so? If someone is using PT_REGS_PARM1 without setting target arch
> > > they should get compilation error about undefined macro. Here it will
> > > be the same thing, only if someone tries to use PT_REGS_PARM1() will
> > > they reach that _Pragma.
> > >
> > > Or am I missing something?
> >
> > Right! Doing this makes sense regardless of the outcome of our discussion above.
>
> Cool, feel free to send a patch with _Pragmas and no extra #defines ;)
I'll give it a shot.
--
Lorenz Bauer | Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
www.cloudflare.com
