Re: objtool query: section start/end symbols?

[Linus Torvalds]

On Thu, 6 Jun 2024 at 15:19, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
>
> The "key" idea was probably misguided.  I didn't mean to suggest to make
> it all complex like static branches/calls, I was just hoping there's a
> simple way to implement "static consts" without needing objtool.

I don't think you ever actually looked at the patch I pointed to, did you?

The patch already works. This all works without any objtool magic. See
here again:

   https://lore.kernel.org/all/CAHk-=whFSz=usMPHHGAQBnJRVAFfuH4gFHtgyLe0YET75zYRzA@xxxxxxxxxxxxxx/

but in that patch, look at this part:

        while (start < end) {
                unsigned long where, sym;

                where = start[0] + (unsigned long)(start+0);
                sym = start[1] + (unsigned long)(start+1);
                start += 2;

                if (sym != (unsigned long)addr)
                        continue;

where we traverse all the static constant uses (for the right size),
and where that 'sym' is the key that we then use to say "this is the
one I actually am modifying".

Is it complex? Not really.

But it's *unnecessary*.

It would be so nice to just have a "this is the start/end of the
section dedicated to this symbol".

In fact, the only use of that key is exactly that fixup code. It's
literally only used for matching the address of the "constant pool"
entry that we're replacing. So if we just had better link-time
information, all of this would just GoAway(tm).

Now, the part that really made me want this wasn't actually that
existing patch that has that one extra "match symbol address" thing,
but thinking about making the associated helpers cleaner.

In particular, this part:

        static_const_init(1, d_hash_shift);
        static_const_init(8, dentry_hashtable);

that initializes the constants has this very ugly hardcoded knowledge
about the particular bucket that those constants are in. And it was
when removing that magic "1" and "8" that I went "wouldn't it be nice
if I could look up the bucket data for this thing directly"

(Note that the 1/8 is *not* "sizeof()" the data - it's the
architecture-specific size, and it's combined using preprocessor '##'
magic into the right address).

And yes, I can fix this with some more wrapper magic when declaring
the variable. It's not going to be even remotely as nasty as static
calls, but some little objtool magic would obviate the need for all of
this.

> Aside from the idea of avoiding "magic", another small downside of using
> objtool is that isn't currently supported on most arches.

Honestly, I don't think that's a problem. If the requirement for
run-time constant support is objtool support, that's more than fine.

> So while I'm not yet necessarily conceding that objtool is really needed
> here, I could work up a quick objtool patch.  It would just be x86-only,
> is that ok for the time being?

Absolutely.

Yes, this kind of "section start/end" symbol would be possibly useful
as a generic feature across all architectures, but even as a
x86-specific one there are already other things that could use it and
not just the static constant hack.

We'd be able to simplify the x86 vmlinux.lds.S file - even if we
couldn't necessarily simplify the generic one. There are several cases
of exactly the same kind of "start/end of section symbols" in that
file, eg

    __x86_cpu_dev_start = .;
    *(.x86_cpu_dev.init)
    __x86_cpu_dev_end = .;

    __x86_intel_mid_dev_start = .;
    *(.x86_intel_mid_dev.init)
    __x86_intel_mid_dev_end = .;

    __retpoline_sites = .;
    *(.retpoline_sites)
    __retpoline_sites_end = .;

    __return_sites = .;
    *(.return_sites)
    __return_sites_end = .;

    __cfi_sites = .;
    *(.cfi_sites)
    __cfi_sites_end = .;

    __alt_instructions = .;
    *(.altinstructions)
    __alt_instructions_end = .;

    __apicdrivers = .;
    *(.apicdrivers);
    __apicdrivers_end = .;

just from a quick look. And yes, they could also just use the
BOUNDED_SECTION() macro we have for this pattern.

Imagine if we could just *not* touch that magical vmlinux.lds.S file
at all when we add some new random section for some random new use -
CPU bugs, firmware tables, things like that. And when we want the
beginning of the section, we'd just use a generic section name, like

   unsigned char __sec_apicdrivers_start[], __sec_apicdrivers_end[];

instead of those magic hardcoded ones that don't even have a pattern
(it looks like start/end is about a third of the cases, and
nothing/end is two thirds).

So my real point here is that this pattern isn't actually anything
unusual - and the only thing special about my static const patch is
that I don't have one single fixed section name, I have a "section
name pattern", which is why I'd like to automate it rather than list
them individually in the vmlinux.lds.S file.

Because yes, I considered just having another helper macro and then
just a "list each constant name in the vmlinux.lds.h file", and
decided that that would be just too ugly.

It's clearly what our *existing* usage pattern is. But I think we can
improve on it.

                Linus