On Sun, 30 May 2021 at 01:51, Andrii Nakryiko [off-list ref] wrote:

On Fri, May 28, 2021 at 1:30 AM Lorenz Bauer [off-list ref] wrote:

quoted

On Wed, 26 May 2021 at 19:34, Andrii Nakryiko [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

quoted

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